| Message ID | 20241028154932.6797-2-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: Address Space Isolation FPU preparations | expand |
On 28/10/2024 3:49 pm, Alejandro Vallejo wrote: > The asserts' intent was to establish whether the xsave instruction was > usable or not, which at the time was strictly given by the presence of > the xsave area. After edb48e76458b("x86/fpu: Combine fpu_ctxt and > xsave_area in arch_vcpu"), that area is always present a more relevant > assert is that the host supports XSAVE. > > Fixes: edb48e76458b("x86/fpu: Combine fpu_ctxt and xsave_area in arch_vcpu") > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> > --- > I'd also be ok with removing the assertions altogether. They serve very > little purpose there after the merge of xsave and fpu_ctxt. I'd be fine with dropping them. If they're violated, the use of XSAVE/XRSTOR immediately afterwards will be fatal too. ~Andrew
On 28.10.2024 18:16, Andrew Cooper wrote: > On 28/10/2024 3:49 pm, Alejandro Vallejo wrote: >> The asserts' intent was to establish whether the xsave instruction was >> usable or not, which at the time was strictly given by the presence of >> the xsave area. After edb48e76458b("x86/fpu: Combine fpu_ctxt and >> xsave_area in arch_vcpu"), that area is always present a more relevant >> assert is that the host supports XSAVE. >> >> Fixes: edb48e76458b("x86/fpu: Combine fpu_ctxt and xsave_area in arch_vcpu") >> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> >> --- >> I'd also be ok with removing the assertions altogether. They serve very >> little purpose there after the merge of xsave and fpu_ctxt. > > I'd be fine with dropping them. +1 Jan > If they're violated, the use of > XSAVE/XRSTOR immediately afterwards will be fatal too. > > ~Andrew
On Tue Oct 29, 2024 at 8:13 AM GMT, Jan Beulich wrote: > On 28.10.2024 18:16, Andrew Cooper wrote: > > On 28/10/2024 3:49 pm, Alejandro Vallejo wrote: > >> The asserts' intent was to establish whether the xsave instruction was > >> usable or not, which at the time was strictly given by the presence of > >> the xsave area. After edb48e76458b("x86/fpu: Combine fpu_ctxt and > >> xsave_area in arch_vcpu"), that area is always present a more relevant > >> assert is that the host supports XSAVE. > >> > >> Fixes: edb48e76458b("x86/fpu: Combine fpu_ctxt and xsave_area in arch_vcpu") > >> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> > >> --- > >> I'd also be ok with removing the assertions altogether. They serve very > >> little purpose there after the merge of xsave and fpu_ctxt. > > > > I'd be fine with dropping them. > > +1 > > Jan > > > If they're violated, the use of > > XSAVE/XRSTOR immediately afterwards will be fatal too. > > > > ~Andrew Ok then, I'll re-send this one as a removal. Cheers, Alejandro
diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c index 83f9b2502bff..375a8274f632 100644 --- a/xen/arch/x86/i387.c +++ b/xen/arch/x86/i387.c @@ -24,7 +24,7 @@ static inline void fpu_xrstor(struct vcpu *v, uint64_t mask) { bool ok; - ASSERT(v->arch.xsave_area); + ASSERT(cpu_has_xsave); /* * XCR0 normally represents what guest OS set. In case of Xen itself, * we set the accumulated feature mask before doing save/restore. @@ -136,7 +136,7 @@ static inline void fpu_xsave(struct vcpu *v) uint64_t mask = vcpu_xsave_mask(v); ASSERT(mask); - ASSERT(v->arch.xsave_area); + ASSERT(cpu_has_xsave); /* * XCR0 normally represents what guest OS set. In case of Xen itself, * we set the accumulated feature mask before doing save/restore.
The asserts' intent was to establish whether the xsave instruction was usable or not, which at the time was strictly given by the presence of the xsave area. After edb48e76458b("x86/fpu: Combine fpu_ctxt and xsave_area in arch_vcpu"), that area is always present a more relevant assert is that the host supports XSAVE. Fixes: edb48e76458b("x86/fpu: Combine fpu_ctxt and xsave_area in arch_vcpu") Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> --- I'd also be ok with removing the assertions altogether. They serve very little purpose there after the merge of xsave and fpu_ctxt. --- xen/arch/x86/i387.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)