Re: [Xen-devel] [PATCH] xen/x86: Implement enable_nmis() in C

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.03.18 at 18:07, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 15/03/18 17:02, Jan Beulich wrote:
>>>>> On 15.03.18 at 17:43, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> +static inline void enable_nmis(void)
>>> +{
>>> +    unsigned long tmp;
>>> +
>>> +    asm volatile ( "mov %%rsp, %[sp]     \n\t"
>>> +                   "push %[ss]           \n\t"
>>> +                   "push %[sp]           \n\t"
>>> +                   "pushf                \n\t"
>>> +                   "push %[cs]           \n\t"
>>> +                   "lea 1f(%%rip), %[ip] \n\t"
>>> +                   "push %[ip]           \n\t"
>>> +                   "iretq; 1:            \n\t"
>>> +                   : [sp] "=r" (tmp), [ip] "=r" (tmp)
>> Strictly speaking this needs to be "=&r" in both cases. That'll
>> guarantee the compiler to pick two distinct registers (not sure
>> how that ends up being with the code you have), which is
>> more than we need want. How about having just a single
>> [tmp] output?
>>
>> With at least the missing & added
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> In this case, everything works fine even if the compiler picks the same
> register.  GCC 7.3 picks %rax for sp and %rdx for.

Well, my remark was meant the other way around - I _wanted_
the same register to be used, and assumed the compiler would
(pointlessly) pick separate ones.

> Then again, we can get away with a single tmp, so I'll switch to that.

One more remark (without meaning to retract my R-b) - considering
the big asm() and almost noting else in the function, I question
whether implementing something like this in C is actual a benefit. I
don't think the goal should be to reduce .S file size just for the size
reduction benefit alone. Otherwise we could move everything into
C files, wrapping things into giant asm()-s. I would sort of guess
that this was the reason for the function here to have been put in
entry.S in the first place.

Jan


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