[PATCH] x86/FPU: make vcpu_reset_fpu() build again with old gcc

[PATCH] x86/FPU: make vcpu_reset_fpu() build again with old gcc

[Jan Beulich]

Fields of anonymous structs/unions may not be part of an initializer for
rather old gcc.

Fixes: 49a068471d77 ("x86/fpu: Rework fpu_setup_fpu() uses to split it in two")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -306,13 +306,13 @@ void vcpu_reset_fpu(struct vcpu *v)
 {
     v->fpu_initialised = false;
     *v->arch.xsave_area = (struct xsave_struct) {
-        .fpu_sse = {
-            .mxcsr = MXCSR_DEFAULT,
-            .fcw = FCW_RESET,
-            .ftw = FXSAVE_FTW_RESET,
-        },
         .xsave_hdr.xstate_bv = X86_XCR0_X87,
     };
+
+    /* Old gcc doesn't permit these to be part of the initializer. */
+    v->arch.xsave_area->fpu_sse.mxcsr = MXCSR_DEFAULT;
+    v->arch.xsave_area->fpu_sse.fcw = FCW_RESET;
+    v->arch.xsave_area->fpu_sse.ftw = FXSAVE_FTW_RESET;
 }
 
 void vcpu_setup_fpu(struct vcpu *v, const void *data)

Re: [PATCH] x86/FPU: make vcpu_reset_fpu() build again with old gcc

[Alejandro Vallejo]

On Mon Dec 9, 2024 at 3:13 PM GMT, Jan Beulich wrote:
> Fields of anonymous structs/unions may not be part of an initializer for
> rather old gcc.

Can you add the specific version for tracking purposes?

>
> Fixes: 49a068471d77 ("x86/fpu: Rework fpu_setup_fpu() uses to split it in two")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/i387.c
> +++ b/xen/arch/x86/i387.c
> @@ -306,13 +306,13 @@ void vcpu_reset_fpu(struct vcpu *v)
>  {
>      v->fpu_initialised = false;
>      *v->arch.xsave_area = (struct xsave_struct) {
> -        .fpu_sse = {
> -            .mxcsr = MXCSR_DEFAULT,
> -            .fcw = FCW_RESET,
> -            .ftw = FXSAVE_FTW_RESET,
> -        },
>          .xsave_hdr.xstate_bv = X86_XCR0_X87,
>      };
> +
> +    /* Old gcc doesn't permit these to be part of the initializer. */
> +    v->arch.xsave_area->fpu_sse.mxcsr = MXCSR_DEFAULT;
> +    v->arch.xsave_area->fpu_sse.fcw = FCW_RESET;
> +    v->arch.xsave_area->fpu_sse.ftw = FXSAVE_FTW_RESET;

That's not quite the same though. A more apt equivalence would be to memset the
area to zero ahead of the assignments. Otherwise rubble will be left behind.

>  }
>  
>  void vcpu_setup_fpu(struct vcpu *v, const void *data)

Out of context and not triggering the GCC bug, but vcpu_setup_fpu() should
probably share the same initialization style as vcpu_reset_fpu(), imo.

Cheers,
Alejandro

Re: [PATCH] x86/FPU: make vcpu_reset_fpu() build again with old gcc

[Jan Beulich]

On 10.12.2024 15:25, Alejandro Vallejo wrote:
> On Mon Dec 9, 2024 at 3:13 PM GMT, Jan Beulich wrote:
>> Fields of anonymous structs/unions may not be part of an initializer for
>> rather old gcc.
> 
> Can you add the specific version for tracking purposes?

It's all the same as before, and I really didn't want to waste time on
once again figuring out which exact version it was that the behavior
changed to the better.

>> Fixes: 49a068471d77 ("x86/fpu: Rework fpu_setup_fpu() uses to split it in two")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/i387.c
>> +++ b/xen/arch/x86/i387.c
>> @@ -306,13 +306,13 @@ void vcpu_reset_fpu(struct vcpu *v)
>>  {
>>      v->fpu_initialised = false;
>>      *v->arch.xsave_area = (struct xsave_struct) {
>> -        .fpu_sse = {
>> -            .mxcsr = MXCSR_DEFAULT,
>> -            .fcw = FCW_RESET,
>> -            .ftw = FXSAVE_FTW_RESET,
>> -        },
>>          .xsave_hdr.xstate_bv = X86_XCR0_X87,
>>      };
>> +
>> +    /* Old gcc doesn't permit these to be part of the initializer. */
>> +    v->arch.xsave_area->fpu_sse.mxcsr = MXCSR_DEFAULT;
>> +    v->arch.xsave_area->fpu_sse.fcw = FCW_RESET;
>> +    v->arch.xsave_area->fpu_sse.ftw = FXSAVE_FTW_RESET;
> 
> That's not quite the same though. A more apt equivalence would be to memset the
> area to zero ahead of the assignments. Otherwise rubble will be left behind.

No. I didn't delete the initializer. All fields not mentioned there will
be default-initialized.

>>  }
>>  
>>  void vcpu_setup_fpu(struct vcpu *v, const void *data)
> 
> Out of context and not triggering the GCC bug, but vcpu_setup_fpu() should
> probably share the same initialization style as vcpu_reset_fpu(), imo.

Why?

Jan

Re: [PATCH] x86/FPU: make vcpu_reset_fpu() build again with old gcc

[Alejandro Vallejo]

On Tue Dec 10, 2024 at 2:34 PM GMT, Jan Beulich wrote:
> On 10.12.2024 15:25, Alejandro Vallejo wrote:
> > On Mon Dec 9, 2024 at 3:13 PM GMT, Jan Beulich wrote:
> >> Fields of anonymous structs/unions may not be part of an initializer for
> >> rather old gcc.
> > 
> > Can you add the specific version for tracking purposes?
>
> It's all the same as before, and I really didn't want to waste time on
> once again figuring out which exact version it was that the behavior
> changed to the better.

Just checked on Godbolt. 4.7.1 works and 4.6.4 doesn't. Adding that data point
to the commit message really helps when navigating git-blame, even if it's not
as precise as it could be. Particularly if one wants to understand exactly
which quirk of which version of which compiler is being dealt with.

>
> >> Fixes: 49a068471d77 ("x86/fpu: Rework fpu_setup_fpu() uses to split it in two")
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> --- a/xen/arch/x86/i387.c
> >> +++ b/xen/arch/x86/i387.c
> >> @@ -306,13 +306,13 @@ void vcpu_reset_fpu(struct vcpu *v)
> >>  {
> >>      v->fpu_initialised = false;
> >>      *v->arch.xsave_area = (struct xsave_struct) {
> >> -        .fpu_sse = {
> >> -            .mxcsr = MXCSR_DEFAULT,
> >> -            .fcw = FCW_RESET,
> >> -            .ftw = FXSAVE_FTW_RESET,
> >> -        },
> >>          .xsave_hdr.xstate_bv = X86_XCR0_X87,
> >>      };
> >> +
> >> +    /* Old gcc doesn't permit these to be part of the initializer. */
> >> +    v->arch.xsave_area->fpu_sse.mxcsr = MXCSR_DEFAULT;
> >> +    v->arch.xsave_area->fpu_sse.fcw = FCW_RESET;
> >> +    v->arch.xsave_area->fpu_sse.ftw = FXSAVE_FTW_RESET;
> > 
> > That's not quite the same though. A more apt equivalence would be to memset the
> > area to zero ahead of the assignments. Otherwise rubble will be left behind.
>
> No. I didn't delete the initializer. All fields not mentioned there will
> be default-initialized.

Right. I misread the diff and thought you had nuked the initializer. That's
indeed all fine. Which means...

>
> >>  }
> >>  
> >>  void vcpu_setup_fpu(struct vcpu *v, const void *data)
> > 
> > Out of context and not triggering the GCC bug, but vcpu_setup_fpu() should
> > probably share the same initialization style as vcpu_reset_fpu(), imo.
>
> Why?

... there's indeed no reason to touch that.

>
> Jan

With the commit message adjusted with the offending GCC version (i.e: <4.7.1):

  Acked-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Cheers,
Alejandro

Re: [PATCH] x86/FPU: make vcpu_reset_fpu() build again with old gcc

[Jan Beulich]

On 10.12.2024 16:12, Alejandro Vallejo wrote:
> On Tue Dec 10, 2024 at 2:34 PM GMT, Jan Beulich wrote:
>> On 10.12.2024 15:25, Alejandro Vallejo wrote:
>>> On Mon Dec 9, 2024 at 3:13 PM GMT, Jan Beulich wrote:
>>>> Fields of anonymous structs/unions may not be part of an initializer for
>>>> rather old gcc.
>>>
>>> Can you add the specific version for tracking purposes?
>>
>> It's all the same as before, and I really didn't want to waste time on
>> once again figuring out which exact version it was that the behavior
>> changed to the better.
> 
> Just checked on Godbolt. 4.7.1 works and 4.6.4 doesn't. Adding that data point
> to the commit message really helps when navigating git-blame, even if it's not
> as precise as it could be. Particularly if one wants to understand exactly
> which quirk of which version of which compiler is being dealt with.

Well, thanks for sorting that out. I've added that info.

> With the commit message adjusted with the offending GCC version (i.e: <4.7.1):
> 
>   Acked-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Thanks here as well. Any chance though you would be willing to upgrade that
to R-b? Only that would allow me to put in the patch without waiting for yet
another tag.

Jan

Re: [PATCH] x86/FPU: make vcpu_reset_fpu() build again with old gcc

[Alejandro Vallejo]

On Tue, Dec 10, 2024 at 4:14 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 10.12.2024 16:12, Alejandro Vallejo wrote:
> > On Tue Dec 10, 2024 at 2:34 PM GMT, Jan Beulich wrote:
> >> On 10.12.2024 15:25, Alejandro Vallejo wrote:
> >>> On Mon Dec 9, 2024 at 3:13 PM GMT, Jan Beulich wrote:
> >>>> Fields of anonymous structs/unions may not be part of an initializer for
> >>>> rather old gcc.
> >>>
> >>> Can you add the specific version for tracking purposes?
> >>
> >> It's all the same as before, and I really didn't want to waste time on
> >> once again figuring out which exact version it was that the behavior
> >> changed to the better.
> >
> > Just checked on Godbolt. 4.7.1 works and 4.6.4 doesn't. Adding that data point
> > to the commit message really helps when navigating git-blame, even if it's not
> > as precise as it could be. Particularly if one wants to understand exactly
> > which quirk of which version of which compiler is being dealt with.
>
> Well, thanks for sorting that out. I've added that info.
>
> > With the commit message adjusted with the offending GCC version (i.e: <4.7.1):
> >
> >   Acked-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
>
> Thanks here as well. Any chance though you would be willing to upgrade that
> to R-b? Only that would allow me to put in the patch without waiting for yet
> another tag.
>
> Jan

Sure.

  Reviewed-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Cheers,
Alejandro