| Message ID | da8320f7-1baf-41f5-b7ac-c05b6371e1e4@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/FPU: make vcpu_reset_fpu() build again with old gcc | expand |
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
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
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
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
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
--- 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)
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>