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(-)

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.
-- 
2.47.0

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