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

[Jan Beulich <JBeulich@xxxxxxxx>]

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.

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