Re: [PATCH] x86: correct asm() constraints when dealing with immediate selector values

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.09.2021 21:31, Andrew Cooper wrote:
> On 09/09/2021 15:56, Jan Beulich wrote:
>> asm() constraints need to fit both the intended insn(s) which the
>> respective operands are going to be used with as well as the actual kind
>> of value specified. "m" (alone) together with a constant, however, leads
>> to gcc saying
>>
>> error: memory input <N> is not directly addressable
>>
>> while clang complains
>>
>> error: invalid lvalue in asm input for constraint 'm'
>>
>> And rightly so - in order to access a memory operand, an address needs
>> to be specified to the insn. In some cases it might be possible for a
>> compiler to synthesize a memory operand holding the requested constant,
>> but I think any solution there would have sharp edges.
> 
> It's precisely what happens in the other uses of constants which you
> haven't adjusted below.  Constants are fine if being propagated into a
> static inline which has properly typed parameters.
> 
> Or are you saying automatic spilling when a width isn't necessarily known?

The lack of width information is a secondary aspect, yes. But the primary
aspects with inline functions (as opposed to open-coded uses) is that the
inline function's parameter can be taken the address of, and hence is
both addressable (gcc) and an lvalue (clang).

>> If "m" alone doesn't work with constants, it is at best pointless (and
>> perhaps misleading or even risky - the compiler might decide to actually
>> pick "m" and not try very hard to find a suitable register) to specify
>> it alongside "r".
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> I'm slightly surprised you didn't spot and comment about what Clang does
> with this.
> 
> https://godbolt.org/z/M4nrerrWM
> 
> For "rm" (0), clang really does spill the constant into a global and
> generate a rip-relative mov to fs, which is especially funny considering
> the rejection of "m" as a constraint.

I had no reason to suspect such imo entirely inconsistent behavior, so
I didn't look at the generated code.

> Clang even spills "rm" (local) into a global, while "m" (local) does
> come from the stack.

"rm" (local)? That would be racy (and hence imo a compiler bug). DYM
"rm" (constant)? Or DYM spilling onto the stack (which is what I
observe with clang5, oddly enough independent of optimization level)?

> I think there is a reasonable argument to say "m" (const) doesn't have
> enough type (width) information for a compiler to do anything sensible
> with, and therefore it is fair to be dropped.
> 
> But for "rm" (var), where there is enough width information, I don't
> think it is reasonable to drop the "m" and restrict the flexibility.

Correct, and I don't do this. What I alter are instances of "rm" (const).

> Furthermore, I'm going to argue that we shouldn't work around this
> behaviour by removing "m" elsewhere.  This code generation
> bug/misfeature affects every use of "rm", even when the referenced
> operand is on the stack and can be used without unspilling first.

As long as the generated code is correct, I agree. See above for a case
where it might actually not be.

>> --- a/xen/arch/x86/x86_64/traps.c
>> +++ b/xen/arch/x86/x86_64/traps.c
>> @@ -248,7 +248,7 @@ void do_double_fault(struct cpu_user_reg
>>  
>>      console_force_unlock();
>>  
>> -    asm ( "lsll %1, %0" : "=r" (cpu) : "rm" (PER_CPU_SELECTOR) );
>> +    asm ( "lsll %1, %0" : "=r" (cpu) : "r" (PER_CPU_SELECTOR) );
> 
> Any chance we can clean this up to: lsl %[lim], %[sel] seeing as the
> line is being edited?

Sure, no problem at all.

Jan