Re: [Xen-devel] [PATCH] xstate: make use_xsave non-init

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jul 01, 2019 at 02:20:52PM +0000, Jan Beulich wrote:
> On 01.07.2019 16:01, Roger Pau Monné  wrote:
> > On Mon, Jul 01, 2019 at 11:39:16AM +0000, Jan Beulich wrote:
> >> On 01.07.2019 12:49, Roger Pau Monne wrote:
> >>> --- a/xen/arch/x86/xstate.c
> >>> +++ b/xen/arch/x86/xstate.c
> >>> @@ -577,7 +577,7 @@ unsigned int xstate_ctxt_size(u64 xcr0)
> >>>    /* Collect the information of processor's extended state */
> >>>    void xstate_init(struct cpuinfo_x86 *c)
> >>>    {
> >>> -    static bool __initdata use_xsave = true;
> >>> +    static bool use_xsave = true;
> >>
> >> Please attach at least a brief comment here, such that people
> >> won't consider the __initdata missing.
> > 
> > Sure.
> > 
> >>
> >> Of course I'd actually prefer the annotation to stay here in
> >> the gcc case. Iirc there was one other case where there was
> >> such an issue; I don't recall whether there it too got dealt
> >> with by removing an annotation.
> > 
> > Yes, in that other case the annotation was just removed, it's 43fa95ae [0]
> > 
> >> How about we introduce an
> >> annotation that expands to nothing in the clang case, but
> >> continues to provide the same functionality for gcc? That
> >> would then also clarify the reason for it being in any
> >> particular place (I guess there are going to be more) without
> >> the need for further commentary.
> > 
> > IMO that's a little bit dangerous, from the LLVM bug report it seems
> > like LLVM behaviour is not a bug, and hence I wouldn't be surprised if
> > newer versions of gcc also exhibit the same issue.
> 
> Okay, then let's go the route you suggested, just with a comment
> added to prevent re-addition of the annotation. I wonder whether
> we ought to do an audit of the code to find possible further
> offenders. It doesn't look very desirable to me to find the
> instances one by one by someone observing crashes.
> 
> Hmm, having looked at the older commit I'm again starting to wonder
> whether dropping the annotations is the right course of action. Did
> we consider adding volatile to the variable back then? That should
> make the compiler not pull ahead the memory access(es), and the
> downsides of this should be pretty limited for init-only items.

IIRC at some point I suggested using ACCESS_ONCE to read the init
opt_bootscrub variable. Adding the volatile keyword to the __initdata
macro is not going to work as-is, because we would also need to fixup
the existing declarations of the variables.

A more simple (and maybe easier to enforce rule) might be to forbid
the usage of init data/text in non-init functions, and that would
likely be easier to enforce automatically by some analysis tool.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel