[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Jan Beulich <jbeulich@xxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Thu, 9 Sep 2021 20:31:47 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=xw5biZfDNnOryD4SC+n2AwZEmSv15Wy4mSqEMZivfxg=; b=g3hm5uDy8y7l849Ieo5c5kJqlHxO2uJRr3EibQKlwYF0hzKCzCVIwEZzNB/eVJ1lEpUg1wKovdW+Pv9Ev57kydN5+uymd0NLMhC7LQ4RpX10kgHha+PiSx8LT41oeSxLZhYSJy71FhBFov8DIkZuE399ZoWxNaZ8GQoa0J1e6Q1D+0Jk274RzW6nWO246QWSFb8PfXlJM7MAAkVzkHdljGJeyEO2wCFD0PGLeQB+Rtr1J/xEGb9PAMfpQgKRpNRw1tPKpv6fP334oL/OOHJL70F/yZYU2mEPsZpAo5hzbY333hUe9oeUG7sj/qR19E9MyUlMEWvOhwBYFlOoGWPlqw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Fum0S/y0/m1tjEpBrZ5sth4We3h6/yBuMOFVJhHtKw7LB4NqDnfXwRrdus+CcClxXVGLUkpb1Kf51t3ajsUBqryO4IUHlfIpxJ7lWMcK+TALB+Jaf8cEWjDy3WnrKNcFTQ0wvSzJ50yZW05L9J7bIPUEEHqVbxT9dnPviIoRNAmbVJbO4W13z/L0EeA3lvXSkSd83Od/fFK3Sj+1MApyrM6auR5tdCLUw3fqBOjzBHzEq1rnW8kLgqh+vS/4RpjUZqJMUbuNWXi/hCHlF9qiQOQ/srkx0CykJZ7qN6WNhFp+rYllu6/HD+s7psQta0fQKcFzGvw5Elm+y4nBLnwiWQ==
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Thu, 09 Sep 2021 19:32:22 +0000
  • Ironport-hdrordr: A9a23:2o7wsqBbs5RZhtzlHelW55DYdb4zR+YMi2TDt3oddfWaSKylfq GV7ZAmPHrP4gr5N0tOpTntAse9qBDnhPtICOsqTNSftWDd0QPFEGgL1+DfKlbbak/DH4BmtJ uJc8JFeaDN5VoRt7eH3OFveexQv+Vu88qT9JnjJ28Gd3AMV0n5hT0JcTpyFCdNNW97LKt8Lr WwzOxdqQGtfHwGB/7LfEXsD4D41qT2fIuNW29/OyIa
  • Ironport-sdr: dw5OZMrsD+9zO8ImFGDaUpaAFDEzTI9boI3fr5inGP9zyGfrKUemh9ER7euQFr8iaJZW1Wb2bW YtRVDrc8Xw+txYOvkxSQl35ncscM4IOAW6siC8Uyg22CzrIoFZ2YfeUe96uSm85FZi2vtH2/eY EYKffUd8vilGKt+clWYJniuX/FyZDSMA38DSpOyUK83Kgv98SNdBTduC0VNGwATfjt4TPSsqXK PeU9X49TyI2DltAB8Pt3Blvy1UrRt+c6dsXGaxPGt+vxMaWIXFXmKihleh1bPJQ5iPmir7DpnP FNZ9WZ7arsXNrEc59wwBgR79
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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?

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


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.

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

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.

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.

> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -736,7 +736,7 @@ void __init detect_zen2_null_seg_behavio
>       uint64_t base;
>       wrmsrl(MSR_FS_BASE, 1);
> -     asm volatile ( "mov %0, %%fs" :: "rm" (0) );
> +     asm volatile ( "mov %0, %%fs" :: "r" (0) );
>       rdmsrl(MSR_FS_BASE, base);
>       if (base == 0)
> --- 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?




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.