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

[Jan Beulich <JBeulich@xxxxxxxx>]

On 01.07.2019 17:16, Roger Pau Monné  wrote:
> 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.

ACCESS_ONCE() is not good here because you need to then catch all
_uses_ of a variable. Dealing with it by changing the declaration/
definition makes it a one (or at most two) place change. I wouldn't
be worried about a header file also needing to change.

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

If you or anyone else is up for integrating such a tool with the
build system - fine. Right now we have none, and I don't want such
issues to be caught be solely dependent on the issue getting noticed
the latest during review.

That said, I'm not actually convinced unconditionally forbidding
such references (like Linux does, requiring further annotations to
override the ban) is the best model. There are clearly cases where
having such references is appropriate and useful. But I can accept
that Linux'es model is overall more safe than our current
effectively non-existent one.

Jan

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