Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Matthias Kaehlcke]

El Fri, Apr 28, 2017 at 03:33:33PM +0100 Mark Rutland ha dit:

> On Fri, Apr 28, 2017 at 08:18:52AM +0100, Ard Biesheuvel wrote:
> > On 27 April 2017 at 23:52, Matthias Kaehlcke  wrote:
> > > El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:
> > >> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
> > >> > Many inline assembly statements don't include the 'x' modifier when
> > >> > using xN registers as operands. This is perfectly valid, however it
> > >> > causes clang to raise warnings like this:
> > >> >
> > >> > warning: value size does not match register size specified by the
> > >> >   constraint and modifier [-Wasm-operand-widths]
> 
> [...]
> 
> > >> > -   asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
> > >> > +   asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
> > >>
> > >> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
> > >> address must be 64-bit, so this would mask a legitimate warning.
> > >>
> > >> Given the prototype of this function the code if fine either way, but
> > >> were we to refactor things (e.g. making this a macro), that might not be
> > >> true.
> > >>
> > >> ... so I'm not sure it make sense to alter instances used for addresses.
> > >
> > > Good point, I'll leave instances dealing with addresses untouched for now.
> > >
> > 
> > OK, I am confused now. We started this thread under the assumption
> > that all unqualified placeholders are warned about by Clang. Given
> > that this appears not to be the case, could we please first find out
> > what causes the warnings? Is it necessary at all to add the x
> > modifiers for 64-bit types?
> 
> FWIW, I grabbed a clang 4.0.0 binary and had a play.
> 
> It looks like clang only warns when an operand is less than 64 bits
> wide, and there is no 'x' or 'w' modifier. Pointers a 64 bits wide, so
> never produce a warning.
> 
> As far as I can tell, applying to both integers and pointers:
> 
> * GCC and clang always treat %N as meaning xN for an r constraint, and
>   you need to use %wN to get wN.
> 
> * If an operand type is 64 bits in size, clang will not produce a warning
>   regarding the operand size.
> 
> * If an x or w modifier is used, clang will not produce a warning
>   regarding the operand size, regardless of whether it matches the
>   register size. Clang is happy for %wN to be used on a pointer type.
> 
> * If an operand type is less than 64 bits in size, and neither an x or
>   w modifier is used, clang will produce a warning as above.
> 
> * If an operand type is greater than 64 bits in size, clang encounters
>   an internal error.
> 
> Given that, I think we *should not* use the x modifier to suppress this
> warning, as I think for those cases we have a potential bug as outlined
> in my prior reply.
> 
> Instead, we should use a temporary 64-bit variable (or cast input
> operands to 64-bit), which avoids that and makes clang happy.
> 
> I've included my test below. Note that clang will produce other errors for
> invalid asm (e.g. for mov w0, x0).

Thanks for your investigation!

I apologize for the noise, my expertise with inline assembly is
extremely limited, and admittedly I need a bit of handholding in this
area. Not without reason changes like this or the prefetch code are at
the very top of my clang stack (i.e. postponed until the other less
scary issues were solved). Hopefully the discussion was still useful.

I'll prepare a short patch that only fixes the warnings encountered in
my build in the way you suggested.

Thanks

Matthias

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Matthias Kaehlcke]

El Fri, Apr 28, 2017 at 03:33:33PM +0100 Mark Rutland ha dit:

> On Fri, Apr 28, 2017 at 08:18:52AM +0100, Ard Biesheuvel wrote:
> > On 27 April 2017 at 23:52, Matthias Kaehlcke  wrote:
> > > El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:
> > >> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
> > >> > Many inline assembly statements don't include the 'x' modifier when
> > >> > using xN registers as operands. This is perfectly valid, however it
> > >> > causes clang to raise warnings like this:
> > >> >
> > >> > warning: value size does not match register size specified by the
> > >> >   constraint and modifier [-Wasm-operand-widths]
> 
> [...]
> 
> > >> > -   asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
> > >> > +   asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
> > >>
> > >> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
> > >> address must be 64-bit, so this would mask a legitimate warning.
> > >>
> > >> Given the prototype of this function the code if fine either way, but
> > >> were we to refactor things (e.g. making this a macro), that might not be
> > >> true.
> > >>
> > >> ... so I'm not sure it make sense to alter instances used for addresses.
> > >
> > > Good point, I'll leave instances dealing with addresses untouched for now.
> > >
> > 
> > OK, I am confused now. We started this thread under the assumption
> > that all unqualified placeholders are warned about by Clang. Given
> > that this appears not to be the case, could we please first find out
> > what causes the warnings? Is it necessary at all to add the x
> > modifiers for 64-bit types?
> 
> FWIW, I grabbed a clang 4.0.0 binary and had a play.
> 
> It looks like clang only warns when an operand is less than 64 bits
> wide, and there is no 'x' or 'w' modifier. Pointers a 64 bits wide, so
> never produce a warning.
> 
> As far as I can tell, applying to both integers and pointers:
> 
> * GCC and clang always treat %N as meaning xN for an r constraint, and
>   you need to use %wN to get wN.
> 
> * If an operand type is 64 bits in size, clang will not produce a warning
>   regarding the operand size.
> 
> * If an x or w modifier is used, clang will not produce a warning
>   regarding the operand size, regardless of whether it matches the
>   register size. Clang is happy for %wN to be used on a pointer type.
> 
> * If an operand type is less than 64 bits in size, and neither an x or
>   w modifier is used, clang will produce a warning as above.
> 
> * If an operand type is greater than 64 bits in size, clang encounters
>   an internal error.
> 
> Given that, I think we *should not* use the x modifier to suppress this
> warning, as I think for those cases we have a potential bug as outlined
> in my prior reply.
> 
> Instead, we should use a temporary 64-bit variable (or cast input
> operands to 64-bit), which avoids that and makes clang happy.
> 
> I've included my test below. Note that clang will produce other errors for
> invalid asm (e.g. for mov w0, x0).

Thanks for your investigation!

I apologize for the noise, my expertise with inline assembly is
extremely limited, and admittedly I need a bit of handholding in this
area. Not without reason changes like this or the prefetch code are at
the very top of my clang stack (i.e. postponed until the other less
scary issues were solved). Hopefully the discussion was still useful.

I'll prepare a short patch that only fixes the warnings encountered in
my build in the way you suggested.

Thanks

Matthias

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Will Deacon]

On Fri, Apr 28, 2017 at 03:43:56PM +0100, Ard Biesheuvel wrote:
> On 28 April 2017 at 15:33, Mark Rutland  wrote:
> > On Fri, Apr 28, 2017 at 08:18:52AM +0100, Ard Biesheuvel wrote:
> OK, good. That is a departure from previous behavior of Clang, which
> was causing build errors before due to the fact that msr/mrs
> instructions involving 32-bit values must still use x registers.
> 
> > * If an operand type is 64 bits in size, clang will not produce a warning
> >   regarding the operand size.
> >
> > * If an x or w modifier is used, clang will not produce a warning
> >   regarding the operand size, regardless of whether it matches the
> >   register size. Clang is happy for %wN to be used on a pointer type.
> >
> > * If an operand type is less than 64 bits in size, and neither an x or
> >   w modifier is used, clang will produce a warning as above.
> >
> > * If an operand type is greater than 64 bits in size, clang encounters
> >   an internal error.
> >
> > Given that, I think we *should not* use the x modifier to suppress this
> > warning, as I think for those cases we have a potential bug as outlined
> > in my prior reply.
> >
> > Instead, we should use a temporary 64-bit variable (or cast input
> > operands to 64-bit), which avoids that and makes clang happy.
> >
> 
> Yes, I think that makes sense.

Likewise, we could even raise a feature request against GCC because these
warnings actually sound useful. Thanks for getting to the bottom of this.

Will

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Will Deacon]

On Fri, Apr 28, 2017 at 03:43:56PM +0100, Ard Biesheuvel wrote:
> On 28 April 2017 at 15:33, Mark Rutland  wrote:
> > On Fri, Apr 28, 2017 at 08:18:52AM +0100, Ard Biesheuvel wrote:
> OK, good. That is a departure from previous behavior of Clang, which
> was causing build errors before due to the fact that msr/mrs
> instructions involving 32-bit values must still use x registers.
> 
> > * If an operand type is 64 bits in size, clang will not produce a warning
> >   regarding the operand size.
> >
> > * If an x or w modifier is used, clang will not produce a warning
> >   regarding the operand size, regardless of whether it matches the
> >   register size. Clang is happy for %wN to be used on a pointer type.
> >
> > * If an operand type is less than 64 bits in size, and neither an x or
> >   w modifier is used, clang will produce a warning as above.
> >
> > * If an operand type is greater than 64 bits in size, clang encounters
> >   an internal error.
> >
> > Given that, I think we *should not* use the x modifier to suppress this
> > warning, as I think for those cases we have a potential bug as outlined
> > in my prior reply.
> >
> > Instead, we should use a temporary 64-bit variable (or cast input
> > operands to 64-bit), which avoids that and makes clang happy.
> >
> 
> Yes, I think that makes sense.

Likewise, we could even raise a feature request against GCC because these
warnings actually sound useful. Thanks for getting to the bottom of this.

Will

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Ard Biesheuvel]

On 28 April 2017 at 15:33, Mark Rutland  wrote:
> On Fri, Apr 28, 2017 at 08:18:52AM +0100, Ard Biesheuvel wrote:
>> On 27 April 2017 at 23:52, Matthias Kaehlcke  wrote:
>> > El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:
>> >> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
>> >> > Many inline assembly statements don't include the 'x' modifier when
>> >> > using xN registers as operands. This is perfectly valid, however it
>> >> > causes clang to raise warnings like this:
>> >> >
>> >> > warning: value size does not match register size specified by the
>> >> >   constraint and modifier [-Wasm-operand-widths]
>
> [...]
>
>> >> > -   asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
>> >> > +   asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
>> >>
>> >> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
>> >> address must be 64-bit, so this would mask a legitimate warning.
>> >>
>> >> Given the prototype of this function the code if fine either way, but
>> >> were we to refactor things (e.g. making this a macro), that might not be
>> >> true.
>> >>
>> >> ... so I'm not sure it make sense to alter instances used for addresses.
>> >
>> > Good point, I'll leave instances dealing with addresses untouched for now.
>> >
>>
>> OK, I am confused now. We started this thread under the assumption
>> that all unqualified placeholders are warned about by Clang. Given
>> that this appears not to be the case, could we please first find out
>> what causes the warnings? Is it necessary at all to add the x
>> modifiers for 64-bit types?
>
> FWIW, I grabbed a clang 4.0.0 binary and had a play.
>
> It looks like clang only warns when an operand is less than 64 bits
> wide, and there is no 'x' or 'w' modifier. Pointers a 64 bits wide, so
> never produce a warning.
>
> As far as I can tell, applying to both integers and pointers:
>
> * GCC and clang always treat %N as meaning xN for an r constraint, and
>   you need to use %wN to get wN.
>

OK, good. That is a departure from previous behavior of Clang, which
was causing build errors before due to the fact that msr/mrs
instructions involving 32-bit values must still use x registers.

> * If an operand type is 64 bits in size, clang will not produce a warning
>   regarding the operand size.
>
> * If an x or w modifier is used, clang will not produce a warning
>   regarding the operand size, regardless of whether it matches the
>   register size. Clang is happy for %wN to be used on a pointer type.
>
> * If an operand type is less than 64 bits in size, and neither an x or
>   w modifier is used, clang will produce a warning as above.
>
> * If an operand type is greater than 64 bits in size, clang encounters
>   an internal error.
>
> Given that, I think we *should not* use the x modifier to suppress this
> warning, as I think for those cases we have a potential bug as outlined
> in my prior reply.
>
> Instead, we should use a temporary 64-bit variable (or cast input
> operands to 64-bit), which avoids that and makes clang happy.
>

Yes, I think that makes sense.

> I've included my test below. Note that clang will produce other errors for
> invalid asm (e.g. for mov w0, x0).
>
> Thanks,
> Mark.
>
> >8
> #define TEST(t, w1, w2) \
> t foo_##t##w1##_##w2(t a, t b)  \
> {   \
> asm (   \
> "mov %" #w1 "0, %" #w2 "1"  \
> : "=r" (a) : "r" (b)\
> );  \
> \
> return a;   \
> }
>
> #define TEST_TYPE(t)\
> TEST(t,  ,  )   \
> TEST(t, w,  )   \
> TEST(t, w, w)   \
> TEST(t, w, x)   \
> TEST(t, x,  )   \
> TEST(t, x, w)   \
> TEST(t, x, x)   \
>
> TEST_TYPE(int)
>
> TEST_TYPE(long)
>
> typedef long * longp;
> TEST_TYPE(longp)
>
> TEST_TYPE(__int128)

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Ard Biesheuvel]

On 28 April 2017 at 15:33, Mark Rutland  wrote:
> On Fri, Apr 28, 2017 at 08:18:52AM +0100, Ard Biesheuvel wrote:
>> On 27 April 2017 at 23:52, Matthias Kaehlcke  wrote:
>> > El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:
>> >> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
>> >> > Many inline assembly statements don't include the 'x' modifier when
>> >> > using xN registers as operands. This is perfectly valid, however it
>> >> > causes clang to raise warnings like this:
>> >> >
>> >> > warning: value size does not match register size specified by the
>> >> >   constraint and modifier [-Wasm-operand-widths]
>
> [...]
>
>> >> > -   asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
>> >> > +   asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
>> >>
>> >> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
>> >> address must be 64-bit, so this would mask a legitimate warning.
>> >>
>> >> Given the prototype of this function the code if fine either way, but
>> >> were we to refactor things (e.g. making this a macro), that might not be
>> >> true.
>> >>
>> >> ... so I'm not sure it make sense to alter instances used for addresses.
>> >
>> > Good point, I'll leave instances dealing with addresses untouched for now.
>> >
>>
>> OK, I am confused now. We started this thread under the assumption
>> that all unqualified placeholders are warned about by Clang. Given
>> that this appears not to be the case, could we please first find out
>> what causes the warnings? Is it necessary at all to add the x
>> modifiers for 64-bit types?
>
> FWIW, I grabbed a clang 4.0.0 binary and had a play.
>
> It looks like clang only warns when an operand is less than 64 bits
> wide, and there is no 'x' or 'w' modifier. Pointers a 64 bits wide, so
> never produce a warning.
>
> As far as I can tell, applying to both integers and pointers:
>
> * GCC and clang always treat %N as meaning xN for an r constraint, and
>   you need to use %wN to get wN.
>

OK, good. That is a departure from previous behavior of Clang, which
was causing build errors before due to the fact that msr/mrs
instructions involving 32-bit values must still use x registers.

> * If an operand type is 64 bits in size, clang will not produce a warning
>   regarding the operand size.
>
> * If an x or w modifier is used, clang will not produce a warning
>   regarding the operand size, regardless of whether it matches the
>   register size. Clang is happy for %wN to be used on a pointer type.
>
> * If an operand type is less than 64 bits in size, and neither an x or
>   w modifier is used, clang will produce a warning as above.
>
> * If an operand type is greater than 64 bits in size, clang encounters
>   an internal error.
>
> Given that, I think we *should not* use the x modifier to suppress this
> warning, as I think for those cases we have a potential bug as outlined
> in my prior reply.
>
> Instead, we should use a temporary 64-bit variable (or cast input
> operands to 64-bit), which avoids that and makes clang happy.
>

Yes, I think that makes sense.

> I've included my test below. Note that clang will produce other errors for
> invalid asm (e.g. for mov w0, x0).
>
> Thanks,
> Mark.
>
> >8
> #define TEST(t, w1, w2) \
> t foo_##t##w1##_##w2(t a, t b)  \
> {   \
> asm (   \
> "mov %" #w1 "0, %" #w2 "1"  \
> : "=r" (a) : "r" (b)\
> );  \
> \
> return a;   \
> }
>
> #define TEST_TYPE(t)\
> TEST(t,  ,  )   \
> TEST(t, w,  )   \
> TEST(t, w, w)   \
> TEST(t, w, x)   \
> TEST(t, x,  )   \
> TEST(t, x, w)   \
> TEST(t, x, x)   \
>
> TEST_TYPE(int)
>
> TEST_TYPE(long)
>
> typedef long * longp;
> TEST_TYPE(longp)
>
> TEST_TYPE(__int128)

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Mark Rutland]

On Fri, Apr 28, 2017 at 08:18:52AM +0100, Ard Biesheuvel wrote:
> On 27 April 2017 at 23:52, Matthias Kaehlcke  wrote:
> > El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:
> >> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
> >> > Many inline assembly statements don't include the 'x' modifier when
> >> > using xN registers as operands. This is perfectly valid, however it
> >> > causes clang to raise warnings like this:
> >> >
> >> > warning: value size does not match register size specified by the
> >> >   constraint and modifier [-Wasm-operand-widths]

[...]

> >> > -   asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
> >> > +   asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
> >>
> >> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
> >> address must be 64-bit, so this would mask a legitimate warning.
> >>
> >> Given the prototype of this function the code if fine either way, but
> >> were we to refactor things (e.g. making this a macro), that might not be
> >> true.
> >>
> >> ... so I'm not sure it make sense to alter instances used for addresses.
> >
> > Good point, I'll leave instances dealing with addresses untouched for now.
> >
> 
> OK, I am confused now. We started this thread under the assumption
> that all unqualified placeholders are warned about by Clang. Given
> that this appears not to be the case, could we please first find out
> what causes the warnings? Is it necessary at all to add the x
> modifiers for 64-bit types?

FWIW, I grabbed a clang 4.0.0 binary and had a play.

It looks like clang only warns when an operand is less than 64 bits
wide, and there is no 'x' or 'w' modifier. Pointers a 64 bits wide, so
never produce a warning.

As far as I can tell, applying to both integers and pointers:

* GCC and clang always treat %N as meaning xN for an r constraint, and
  you need to use %wN to get wN.

* If an operand type is 64 bits in size, clang will not produce a warning
  regarding the operand size.

* If an x or w modifier is used, clang will not produce a warning
  regarding the operand size, regardless of whether it matches the
  register size. Clang is happy for %wN to be used on a pointer type.

* If an operand type is less than 64 bits in size, and neither an x or
  w modifier is used, clang will produce a warning as above.

* If an operand type is greater than 64 bits in size, clang encounters
  an internal error.

Given that, I think we *should not* use the x modifier to suppress this
warning, as I think for those cases we have a potential bug as outlined
in my prior reply.

Instead, we should use a temporary 64-bit variable (or cast input
operands to 64-bit), which avoids that and makes clang happy.

I've included my test below. Note that clang will produce other errors for
invalid asm (e.g. for mov w0, x0).

Thanks,
Mark.

>8
#define TEST(t, w1, w2) \
t foo_##t##w1##_##w2(t a, t b)  \
{   \
asm (   \
"mov %" #w1 "0, %" #w2 "1"  \
: "=r" (a) : "r" (b)\
);  \
\
return a;   \
}

#define TEST_TYPE(t)\
TEST(t,  ,  )   \
TEST(t, w,  )   \
TEST(t, w, w)   \
TEST(t, w, x)   \
TEST(t, x,  )   \
TEST(t, x, w)   \
TEST(t, x, x)   \

TEST_TYPE(int)

TEST_TYPE(long)

typedef long * longp;
TEST_TYPE(longp)

TEST_TYPE(__int128)

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Mark Rutland]

On Fri, Apr 28, 2017 at 08:18:52AM +0100, Ard Biesheuvel wrote:
> On 27 April 2017 at 23:52, Matthias Kaehlcke  wrote:
> > El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:
> >> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
> >> > Many inline assembly statements don't include the 'x' modifier when
> >> > using xN registers as operands. This is perfectly valid, however it
> >> > causes clang to raise warnings like this:
> >> >
> >> > warning: value size does not match register size specified by the
> >> >   constraint and modifier [-Wasm-operand-widths]

[...]

> >> > -   asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
> >> > +   asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
> >>
> >> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
> >> address must be 64-bit, so this would mask a legitimate warning.
> >>
> >> Given the prototype of this function the code if fine either way, but
> >> were we to refactor things (e.g. making this a macro), that might not be
> >> true.
> >>
> >> ... so I'm not sure it make sense to alter instances used for addresses.
> >
> > Good point, I'll leave instances dealing with addresses untouched for now.
> >
> 
> OK, I am confused now. We started this thread under the assumption
> that all unqualified placeholders are warned about by Clang. Given
> that this appears not to be the case, could we please first find out
> what causes the warnings? Is it necessary at all to add the x
> modifiers for 64-bit types?

FWIW, I grabbed a clang 4.0.0 binary and had a play.

It looks like clang only warns when an operand is less than 64 bits
wide, and there is no 'x' or 'w' modifier. Pointers a 64 bits wide, so
never produce a warning.

As far as I can tell, applying to both integers and pointers:

* GCC and clang always treat %N as meaning xN for an r constraint, and
  you need to use %wN to get wN.

* If an operand type is 64 bits in size, clang will not produce a warning
  regarding the operand size.

* If an x or w modifier is used, clang will not produce a warning
  regarding the operand size, regardless of whether it matches the
  register size. Clang is happy for %wN to be used on a pointer type.

* If an operand type is less than 64 bits in size, and neither an x or
  w modifier is used, clang will produce a warning as above.

* If an operand type is greater than 64 bits in size, clang encounters
  an internal error.

Given that, I think we *should not* use the x modifier to suppress this
warning, as I think for those cases we have a potential bug as outlined
in my prior reply.

Instead, we should use a temporary 64-bit variable (or cast input
operands to 64-bit), which avoids that and makes clang happy.

I've included my test below. Note that clang will produce other errors for
invalid asm (e.g. for mov w0, x0).

Thanks,
Mark.

>8
#define TEST(t, w1, w2) \
t foo_##t##w1##_##w2(t a, t b)  \
{   \
asm (   \
"mov %" #w1 "0, %" #w2 "1"  \
: "=r" (a) : "r" (b)\
);  \
\
return a;   \
}

#define TEST_TYPE(t)\
TEST(t,  ,  )   \
TEST(t, w,  )   \
TEST(t, w, w)   \
TEST(t, w, x)   \
TEST(t, x,  )   \
TEST(t, x, w)   \
TEST(t, x, x)   \

TEST_TYPE(int)

TEST_TYPE(long)

typedef long * longp;
TEST_TYPE(longp)

TEST_TYPE(__int128)

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Mark Rutland]

On Fri, Apr 28, 2017 at 11:20:21AM +0100, Ard Biesheuvel wrote:
> On 28 April 2017 at 10:53, Mark Rutland  wrote:
> > On Fri, Apr 28, 2017 at 08:18:52AM +0100, Ard Biesheuvel wrote:
> >> On 27 April 2017 at 23:52, Matthias Kaehlcke  wrote:
> >> > El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:
> >> >> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
> >
> >> >> > -   asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
> >> >> > +   asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
> >> >>
> >> >> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
> >> >> address must be 64-bit, so this would mask a legitimate warning.
> >> >>
> >> >> Given the prototype of this function the code if fine either way, but
> >> >> were we to refactor things (e.g. making this a macro), that might not be
> >> >> true.
> >> >>
> >> >> ... so I'm not sure it make sense to alter instances used for addresses.
> >> >
> >> > Good point, I'll leave instances dealing with addresses untouched for 
> >> > now.
> >> >
> >>
> >> OK, I am confused now. We started this thread under the assumption
> >> that all unqualified placeholders are warned about by Clang. Given
> >> that this appears not to be the case, could we please first find out
> >> what causes the warnings?
> >
> > Yes please.
> >
> >> Is it necessary at all to add the x modifiers for 64-bit types?
> >
> > Having delved a little deeper, I think this is actively harmful, and
> > clang's warning indicates potential problems even when compiling with
> > GCC.
> >
> > The below test simulates how we might write to control regs and so on,
> > with a mov in asm simulating something like an msr.
> >
> > >8
> > #include 
> >
> > static inline unsigned long generate_val(void)
> > {
> > unsigned long val;
> >
> > /* hide value generation from GCC */
> > asm (
> > "movn %0, #0"
> > : "=r" (val)
> > );
> >
> > return val;
> > }
> >
> > static inline unsigned long use_val_32(unsigned int in)
> > {
> > unsigned long out;
> >
> > /* simulate what we might write to a sysreg */
> > asm (
> > "mov %x0, %x1"
> > : "=r" (out)
> > : "r" (in)
> > );
> >
> > return out;
> > }
> >
> > int main(int argc, char *argv)
> > {
> > printf("32-bit val is: 0x%016lx\n", use_val_32(generate_val()));
> >
> > return 0;
> > }
> > >8
> >
> > Depending on optimization level, bits that we do not expect can flow 
> > through:
> >
> > $ gcc test.c -o test
> > $ ./test
> > 32-bit val is: 0x
> > $ gcc test.c -O1 -o test
> > $ ./test
> > 32-bit val is: 0x
> > $ gcc test.c -O2 -o test
> > $ ./test
> > 32-bit val is: 0x
> >
> > ... that could be disastrous depending on how the result was used.
> >
> > With "in" cast to an unsigned long, the compiler realises it needs to 
> > perform
> > any necessary truncation itself:
> >
> > $ gcc test.c -o test
> > $ ./test
> > 32-bit val is: 0x
> > $ gcc test.c -O1 -o test
> > $ ./test
> > 32-bit val is: 0x
> > $ gcc test.c -O2 -o test
> > $ ./test
> > 32-bit val is: 0x
> > $ gcc test.c -O3 -o test
> > $ ./test
> > 32-bit val is: 0x
> >
> > I think that the correct fix is to use intermediate 64-bit variables, or
> > casts, so that the compiler *must* use an x register, and consequently
> > guarantees that all 64-bits of the register are as we expect.
> 
> But do we care about those top bits when writing a 32-bit system
> register from a X register?

Well, that only means the upper 32 bits are RES0, so yes. They could
gain a meaning on some future HW.

For sysregs, write_sysreg*() already solves this, as there's an implicit
cast to unsigned long via the function prototype. The 'x' modifier there
is only to ensure xzr can be used.

... however, this is a problem for any asm, as it can take input bits we
don't expect, and consequently generate output that we don't expect.
There's a potential functional correctness issue.

We need the precise set of warnings so that for each case we can
determine whether there is a potential issue today, or whether something
else protects us.

Thanks,
Mark.

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Mark Rutland]

On Fri, Apr 28, 2017 at 11:20:21AM +0100, Ard Biesheuvel wrote:
> On 28 April 2017 at 10:53, Mark Rutland  wrote:
> > On Fri, Apr 28, 2017 at 08:18:52AM +0100, Ard Biesheuvel wrote:
> >> On 27 April 2017 at 23:52, Matthias Kaehlcke  wrote:
> >> > El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:
> >> >> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
> >
> >> >> > -   asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
> >> >> > +   asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
> >> >>
> >> >> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
> >> >> address must be 64-bit, so this would mask a legitimate warning.
> >> >>
> >> >> Given the prototype of this function the code if fine either way, but
> >> >> were we to refactor things (e.g. making this a macro), that might not be
> >> >> true.
> >> >>
> >> >> ... so I'm not sure it make sense to alter instances used for addresses.
> >> >
> >> > Good point, I'll leave instances dealing with addresses untouched for 
> >> > now.
> >> >
> >>
> >> OK, I am confused now. We started this thread under the assumption
> >> that all unqualified placeholders are warned about by Clang. Given
> >> that this appears not to be the case, could we please first find out
> >> what causes the warnings?
> >
> > Yes please.
> >
> >> Is it necessary at all to add the x modifiers for 64-bit types?
> >
> > Having delved a little deeper, I think this is actively harmful, and
> > clang's warning indicates potential problems even when compiling with
> > GCC.
> >
> > The below test simulates how we might write to control regs and so on,
> > with a mov in asm simulating something like an msr.
> >
> > >8
> > #include 
> >
> > static inline unsigned long generate_val(void)
> > {
> > unsigned long val;
> >
> > /* hide value generation from GCC */
> > asm (
> > "movn %0, #0"
> > : "=r" (val)
> > );
> >
> > return val;
> > }
> >
> > static inline unsigned long use_val_32(unsigned int in)
> > {
> > unsigned long out;
> >
> > /* simulate what we might write to a sysreg */
> > asm (
> > "mov %x0, %x1"
> > : "=r" (out)
> > : "r" (in)
> > );
> >
> > return out;
> > }
> >
> > int main(int argc, char *argv)
> > {
> > printf("32-bit val is: 0x%016lx\n", use_val_32(generate_val()));
> >
> > return 0;
> > }
> > >8
> >
> > Depending on optimization level, bits that we do not expect can flow 
> > through:
> >
> > $ gcc test.c -o test
> > $ ./test
> > 32-bit val is: 0x
> > $ gcc test.c -O1 -o test
> > $ ./test
> > 32-bit val is: 0x
> > $ gcc test.c -O2 -o test
> > $ ./test
> > 32-bit val is: 0x
> >
> > ... that could be disastrous depending on how the result was used.
> >
> > With "in" cast to an unsigned long, the compiler realises it needs to 
> > perform
> > any necessary truncation itself:
> >
> > $ gcc test.c -o test
> > $ ./test
> > 32-bit val is: 0x
> > $ gcc test.c -O1 -o test
> > $ ./test
> > 32-bit val is: 0x
> > $ gcc test.c -O2 -o test
> > $ ./test
> > 32-bit val is: 0x
> > $ gcc test.c -O3 -o test
> > $ ./test
> > 32-bit val is: 0x
> >
> > I think that the correct fix is to use intermediate 64-bit variables, or
> > casts, so that the compiler *must* use an x register, and consequently
> > guarantees that all 64-bits of the register are as we expect.
> 
> But do we care about those top bits when writing a 32-bit system
> register from a X register?

Well, that only means the upper 32 bits are RES0, so yes. They could
gain a meaning on some future HW.

For sysregs, write_sysreg*() already solves this, as there's an implicit
cast to unsigned long via the function prototype. The 'x' modifier there
is only to ensure xzr can be used.

... however, this is a problem for any asm, as it can take input bits we
don't expect, and consequently generate output that we don't expect.
There's a potential functional correctness issue.

We need the precise set of warnings so that for each case we can
determine whether there is a potential issue today, or whether something
else protects us.

Thanks,
Mark.

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Ard Biesheuvel]

On 28 April 2017 at 10:53, Mark Rutland  wrote:
> On Fri, Apr 28, 2017 at 08:18:52AM +0100, Ard Biesheuvel wrote:
>> On 27 April 2017 at 23:52, Matthias Kaehlcke  wrote:
>> > El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:
>> >> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
>
>> >> > -   asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
>> >> > +   asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
>> >>
>> >> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
>> >> address must be 64-bit, so this would mask a legitimate warning.
>> >>
>> >> Given the prototype of this function the code if fine either way, but
>> >> were we to refactor things (e.g. making this a macro), that might not be
>> >> true.
>> >>
>> >> ... so I'm not sure it make sense to alter instances used for addresses.
>> >
>> > Good point, I'll leave instances dealing with addresses untouched for now.
>> >
>>
>> OK, I am confused now. We started this thread under the assumption
>> that all unqualified placeholders are warned about by Clang. Given
>> that this appears not to be the case, could we please first find out
>> what causes the warnings?
>
> Yes please.
>
>> Is it necessary at all to add the x modifiers for 64-bit types?
>
> Having delved a little deeper, I think this is actively harmful, and
> clang's warning indicates potential problems even when compiling with
> GCC.
>
> The below test simulates how we might write to control regs and so on,
> with a mov in asm simulating something like an msr.
>
> >8
> #include 
>
> static inline unsigned long generate_val(void)
> {
> unsigned long val;
>
> /* hide value generation from GCC */
> asm (
> "movn %0, #0"
> : "=r" (val)
> );
>
> return val;
> }
>
> static inline unsigned long use_val_32(unsigned int in)
> {
> unsigned long out;
>
> /* simulate what we might write to a sysreg */
> asm (
> "mov %x0, %x1"
> : "=r" (out)
> : "r" (in)
> );
>
> return out;
> }
>
> int main(int argc, char *argv)
> {
> printf("32-bit val is: 0x%016lx\n", use_val_32(generate_val()));
>
> return 0;
> }
> >8
>
> Depending on optimization level, bits that we do not expect can flow through:
>
> $ gcc test.c -o test
> $ ./test
> 32-bit val is: 0x
> $ gcc test.c -O1 -o test
> $ ./test
> 32-bit val is: 0x
> $ gcc test.c -O2 -o test
> $ ./test
> 32-bit val is: 0x
>
> ... that could be disastrous depending on how the result was used.
>
> With "in" cast to an unsigned long, the compiler realises it needs to perform
> any necessary truncation itself:
>
> $ gcc test.c -o test
> $ ./test
> 32-bit val is: 0x
> $ gcc test.c -O1 -o test
> $ ./test
> 32-bit val is: 0x
> $ gcc test.c -O2 -o test
> $ ./test
> 32-bit val is: 0x
> $ gcc test.c -O3 -o test
> $ ./test
> 32-bit val is: 0x
>
> I think that the correct fix is to use intermediate 64-bit variables, or
> casts, so that the compiler *must* use an x register, and consequently
> guarantees that all 64-bits of the register are as we expect.
>

But do we care about those top bits when writing a 32-bit system
register from a X register?

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Ard Biesheuvel]

On 28 April 2017 at 10:53, Mark Rutland  wrote:
> On Fri, Apr 28, 2017 at 08:18:52AM +0100, Ard Biesheuvel wrote:
>> On 27 April 2017 at 23:52, Matthias Kaehlcke  wrote:
>> > El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:
>> >> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
>
>> >> > -   asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
>> >> > +   asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
>> >>
>> >> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
>> >> address must be 64-bit, so this would mask a legitimate warning.
>> >>
>> >> Given the prototype of this function the code if fine either way, but
>> >> were we to refactor things (e.g. making this a macro), that might not be
>> >> true.
>> >>
>> >> ... so I'm not sure it make sense to alter instances used for addresses.
>> >
>> > Good point, I'll leave instances dealing with addresses untouched for now.
>> >
>>
>> OK, I am confused now. We started this thread under the assumption
>> that all unqualified placeholders are warned about by Clang. Given
>> that this appears not to be the case, could we please first find out
>> what causes the warnings?
>
> Yes please.
>
>> Is it necessary at all to add the x modifiers for 64-bit types?
>
> Having delved a little deeper, I think this is actively harmful, and
> clang's warning indicates potential problems even when compiling with
> GCC.
>
> The below test simulates how we might write to control regs and so on,
> with a mov in asm simulating something like an msr.
>
> >8
> #include 
>
> static inline unsigned long generate_val(void)
> {
> unsigned long val;
>
> /* hide value generation from GCC */
> asm (
> "movn %0, #0"
> : "=r" (val)
> );
>
> return val;
> }
>
> static inline unsigned long use_val_32(unsigned int in)
> {
> unsigned long out;
>
> /* simulate what we might write to a sysreg */
> asm (
> "mov %x0, %x1"
> : "=r" (out)
> : "r" (in)
> );
>
> return out;
> }
>
> int main(int argc, char *argv)
> {
> printf("32-bit val is: 0x%016lx\n", use_val_32(generate_val()));
>
> return 0;
> }
> >8
>
> Depending on optimization level, bits that we do not expect can flow through:
>
> $ gcc test.c -o test
> $ ./test
> 32-bit val is: 0x
> $ gcc test.c -O1 -o test
> $ ./test
> 32-bit val is: 0x
> $ gcc test.c -O2 -o test
> $ ./test
> 32-bit val is: 0x
>
> ... that could be disastrous depending on how the result was used.
>
> With "in" cast to an unsigned long, the compiler realises it needs to perform
> any necessary truncation itself:
>
> $ gcc test.c -o test
> $ ./test
> 32-bit val is: 0x
> $ gcc test.c -O1 -o test
> $ ./test
> 32-bit val is: 0x
> $ gcc test.c -O2 -o test
> $ ./test
> 32-bit val is: 0x
> $ gcc test.c -O3 -o test
> $ ./test
> 32-bit val is: 0x
>
> I think that the correct fix is to use intermediate 64-bit variables, or
> casts, so that the compiler *must* use an x register, and consequently
> guarantees that all 64-bits of the register are as we expect.
>

But do we care about those top bits when writing a 32-bit system
register from a X register?

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Mark Rutland]

On Fri, Apr 28, 2017 at 08:18:52AM +0100, Ard Biesheuvel wrote:
> On 27 April 2017 at 23:52, Matthias Kaehlcke  wrote:
> > El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:
> >> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:

> >> > -   asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
> >> > +   asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
> >>
> >> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
> >> address must be 64-bit, so this would mask a legitimate warning.
> >>
> >> Given the prototype of this function the code if fine either way, but
> >> were we to refactor things (e.g. making this a macro), that might not be
> >> true.
> >>
> >> ... so I'm not sure it make sense to alter instances used for addresses.
> >
> > Good point, I'll leave instances dealing with addresses untouched for now.
> >
> 
> OK, I am confused now. We started this thread under the assumption
> that all unqualified placeholders are warned about by Clang. Given
> that this appears not to be the case, could we please first find out
> what causes the warnings?

Yes please.

> Is it necessary at all to add the x modifiers for 64-bit types?

Having delved a little deeper, I think this is actively harmful, and
clang's warning indicates potential problems even when compiling with
GCC.

The below test simulates how we might write to control regs and so on,
with a mov in asm simulating something like an msr.

>8
#include 

static inline unsigned long generate_val(void)
{
unsigned long val;

/* hide value generation from GCC */
asm (
"movn %0, #0"
: "=r" (val)
);

return val;
}

static inline unsigned long use_val_32(unsigned int in)
{
unsigned long out;

/* simulate what we might write to a sysreg */
asm (
"mov %x0, %x1"
: "=r" (out)
: "r" (in)
);

return out;
}

int main(int argc, char *argv)
{
printf("32-bit val is: 0x%016lx\n", use_val_32(generate_val()));

return 0;
}
>8

Depending on optimization level, bits that we do not expect can flow through:

$ gcc test.c -o test
$ ./test
32-bit val is: 0x
$ gcc test.c -O1 -o test
$ ./test
32-bit val is: 0x
$ gcc test.c -O2 -o test
$ ./test
32-bit val is: 0x

... that could be disastrous depending on how the result was used.

With "in" cast to an unsigned long, the compiler realises it needs to perform
any necessary truncation itself:

$ gcc test.c -o test
$ ./test
32-bit val is: 0x
$ gcc test.c -O1 -o test
$ ./test
32-bit val is: 0x
$ gcc test.c -O2 -o test
$ ./test
32-bit val is: 0x
$ gcc test.c -O3 -o test
$ ./test
32-bit val is: 0x

I think that the correct fix is to use intermediate 64-bit variables, or
casts, so that the compiler *must* use an x register, and consequently
guarantees that all 64-bits of the register are as we expect.

Thanks,
Mark.

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Mark Rutland]

On Fri, Apr 28, 2017 at 08:18:52AM +0100, Ard Biesheuvel wrote:
> On 27 April 2017 at 23:52, Matthias Kaehlcke  wrote:
> > El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:
> >> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:

> >> > -   asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
> >> > +   asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
> >>
> >> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
> >> address must be 64-bit, so this would mask a legitimate warning.
> >>
> >> Given the prototype of this function the code if fine either way, but
> >> were we to refactor things (e.g. making this a macro), that might not be
> >> true.
> >>
> >> ... so I'm not sure it make sense to alter instances used for addresses.
> >
> > Good point, I'll leave instances dealing with addresses untouched for now.
> >
> 
> OK, I am confused now. We started this thread under the assumption
> that all unqualified placeholders are warned about by Clang. Given
> that this appears not to be the case, could we please first find out
> what causes the warnings?

Yes please.

> Is it necessary at all to add the x modifiers for 64-bit types?

Having delved a little deeper, I think this is actively harmful, and
clang's warning indicates potential problems even when compiling with
GCC.

The below test simulates how we might write to control regs and so on,
with a mov in asm simulating something like an msr.

>8
#include 

static inline unsigned long generate_val(void)
{
unsigned long val;

/* hide value generation from GCC */
asm (
"movn %0, #0"
: "=r" (val)
);

return val;
}

static inline unsigned long use_val_32(unsigned int in)
{
unsigned long out;

/* simulate what we might write to a sysreg */
asm (
"mov %x0, %x1"
: "=r" (out)
: "r" (in)
);

return out;
}

int main(int argc, char *argv)
{
printf("32-bit val is: 0x%016lx\n", use_val_32(generate_val()));

return 0;
}
>8

Depending on optimization level, bits that we do not expect can flow through:

$ gcc test.c -o test
$ ./test
32-bit val is: 0x
$ gcc test.c -O1 -o test
$ ./test
32-bit val is: 0x
$ gcc test.c -O2 -o test
$ ./test
32-bit val is: 0x

... that could be disastrous depending on how the result was used.

With "in" cast to an unsigned long, the compiler realises it needs to perform
any necessary truncation itself:

$ gcc test.c -o test
$ ./test
32-bit val is: 0x
$ gcc test.c -O1 -o test
$ ./test
32-bit val is: 0x
$ gcc test.c -O2 -o test
$ ./test
32-bit val is: 0x
$ gcc test.c -O3 -o test
$ ./test
32-bit val is: 0x

I think that the correct fix is to use intermediate 64-bit variables, or
casts, so that the compiler *must* use an x register, and consequently
guarantees that all 64-bits of the register are as we expect.

Thanks,
Mark.

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Ard Biesheuvel]

On 27 April 2017 at 23:52, Matthias Kaehlcke  wrote:
> Hi Mark,
>
> Thanks for your comments.
>
> El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:
>
>> Hi,
>>
>> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
>> > Many inline assembly statements don't include the 'x' modifier when
>> > using xN registers as operands. This is perfectly valid, however it
>> > causes clang to raise warnings like this:
>> >
>> > warning: value size does not match register size specified by the
>> >   constraint and modifier [-Wasm-operand-widths]
>> > ...
>> > arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
>> >   '__smp_store_release'
>> > asm volatile ("stlr %1, %0"
>> >
>> > Add the modifiers to keep clang happy.
>>
>> If we're going to make this consistent, it would make sense to similarly
>> annotate 'w' regs. That will make it easier going forward to enforce a
>> policy that registers are suitably annotated.
>
> Ok
>
>> Also, there's a risk that we silently mask a bug here, for which clang's
>> warning is legitimate, so we need to review this very carefully...
>>
>> > Signed-off-by: Matthias Kaehlcke 
>> > ---
>> > Changes in v2:
>> > - also add modifiers to multiline ASM statements in include/asm/
>> >   {atomic_ll_sc.h,irqflags.h,pgtable.h,uaccess.h,word-at-a-time.h}
>> >   that were missed on v1
>> >
>> >  arch/arm64/include/asm/arch_gicv3.h |  2 +-
>> >  arch/arm64/include/asm/atomic_ll_sc.h   | 36 
>> > -
>> >  arch/arm64/include/asm/barrier.h|  4 ++--
>> >  arch/arm64/include/asm/io.h | 24 +++---
>> >  arch/arm64/include/asm/irqflags.h   | 10 -
>> >  arch/arm64/include/asm/kvm_hyp.h| 10 -
>> >  arch/arm64/include/asm/kvm_mmu.h| 12 +--
>> >  arch/arm64/include/asm/percpu.h |  4 ++--
>> >  arch/arm64/include/asm/pgtable.h| 20 +-
>> >  arch/arm64/include/asm/sysreg.h |  4 ++--
>> >  arch/arm64/include/asm/uaccess.h| 14 ++---
>> >  arch/arm64/include/asm/word-at-a-time.h | 14 ++---
>> >  arch/arm64/kernel/armv8_deprecated.c|  4 ++--
>> >  arch/arm64/kernel/probes/kprobes.c  |  2 +-
>> >  arch/arm64/kvm/hyp/switch.c |  4 ++--
>> >  15 files changed, 82 insertions(+), 82 deletions(-)
>>
>> ... to that end, could you split these into a few patches?
>>
>> That way, knowledgeable people can focus their review on the code they
>> understand.
>>
>> That doesn't need to be a patch per file; all the KVM bits can be
>> collated in one patch, for example. However, the atomics, kvm, and
>> uaccess+word-at-a-time bits should certainly be separate patches given
>> their (existing) complexity.
>
> I agree the patch is too large, I considered to split it up but wasn't
> sure where to draw the line(s). Will try to find halfway reasonable
> batches :)
>
>> Otherwise, I have a couple of comments below.
>>
>> > diff --git a/arch/arm64/include/asm/arch_gicv3.h 
>> > b/arch/arm64/include/asm/arch_gicv3.h
>> > index f37e3a21f6e7..ba54e5bee885 100644
>> > --- a/arch/arm64/include/asm/arch_gicv3.h
>> > +++ b/arch/arm64/include/asm/arch_gicv3.h
>> > @@ -166,7 +166,7 @@ static inline void gic_write_sre(u32 val)
>> >
>> >  static inline void gic_write_bpr1(u32 val)
>> >  {
>> > -   asm volatile("msr_s " __stringify(ICC_BPR1_EL1) ", %0" : : "r" (val));
>> > +   asm volatile("msr_s " __stringify(ICC_BPR1_EL1) ", %x0" : : "r" (val));
>> >  }
>>
>> Please make this use write_sysreg_s() instead, i.e.
>>
>> static inline void gic_write_bpr1(u32 val)
>> {
>>   write_sysreg_s(var, ICC_BPR1_EL1);
>> }
>>
>> ... that uses the 'x' modifier internally, and it's what we do for the
>> other GIC sysreg accesors.
>>
>> This accessor was missed by commit:
>>
>>   d44ffa5ae70a15a1 ("irqchip/gic-v3: Convert arm64 GIC accessors to 
>> {read,write}_sysreg_s")
>>
>> ... because it was added concurrently by commitL
>>
>>   91ef84428a86b75a ("irqchip/gic-v3: Reset BPR during initialization")
>>
>> ... i.e. it was not deliberately omitted.
>
> Will do
>
>> [...]
>>
>> > -   asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
>> > +   asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
>>
>> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
>> address must be 64-bit, so this would mask a legitimate warning.
>>
>> Given the prototype of this function the code if fine either way, but
>> were we to refactor things (e.g. making this a macro), that might not be
>> true.
>>
>> ... so I'm not sure it make sense to alter instances used for addresses.
>
> Good point, I'll leave instances dealing with addresses untouched for now.
>

OK, I am confused now. We started this thread under the assumption
that all unqualified placeholders are warned about by Clang. Given
that this appears not to be the case, could we please first find out
what causes the warnings? 

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Ard Biesheuvel]

On 27 April 2017 at 23:52, Matthias Kaehlcke  wrote:
> Hi Mark,
>
> Thanks for your comments.
>
> El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:
>
>> Hi,
>>
>> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
>> > Many inline assembly statements don't include the 'x' modifier when
>> > using xN registers as operands. This is perfectly valid, however it
>> > causes clang to raise warnings like this:
>> >
>> > warning: value size does not match register size specified by the
>> >   constraint and modifier [-Wasm-operand-widths]
>> > ...
>> > arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
>> >   '__smp_store_release'
>> > asm volatile ("stlr %1, %0"
>> >
>> > Add the modifiers to keep clang happy.
>>
>> If we're going to make this consistent, it would make sense to similarly
>> annotate 'w' regs. That will make it easier going forward to enforce a
>> policy that registers are suitably annotated.
>
> Ok
>
>> Also, there's a risk that we silently mask a bug here, for which clang's
>> warning is legitimate, so we need to review this very carefully...
>>
>> > Signed-off-by: Matthias Kaehlcke 
>> > ---
>> > Changes in v2:
>> > - also add modifiers to multiline ASM statements in include/asm/
>> >   {atomic_ll_sc.h,irqflags.h,pgtable.h,uaccess.h,word-at-a-time.h}
>> >   that were missed on v1
>> >
>> >  arch/arm64/include/asm/arch_gicv3.h |  2 +-
>> >  arch/arm64/include/asm/atomic_ll_sc.h   | 36 
>> > -
>> >  arch/arm64/include/asm/barrier.h|  4 ++--
>> >  arch/arm64/include/asm/io.h | 24 +++---
>> >  arch/arm64/include/asm/irqflags.h   | 10 -
>> >  arch/arm64/include/asm/kvm_hyp.h| 10 -
>> >  arch/arm64/include/asm/kvm_mmu.h| 12 +--
>> >  arch/arm64/include/asm/percpu.h |  4 ++--
>> >  arch/arm64/include/asm/pgtable.h| 20 +-
>> >  arch/arm64/include/asm/sysreg.h |  4 ++--
>> >  arch/arm64/include/asm/uaccess.h| 14 ++---
>> >  arch/arm64/include/asm/word-at-a-time.h | 14 ++---
>> >  arch/arm64/kernel/armv8_deprecated.c|  4 ++--
>> >  arch/arm64/kernel/probes/kprobes.c  |  2 +-
>> >  arch/arm64/kvm/hyp/switch.c |  4 ++--
>> >  15 files changed, 82 insertions(+), 82 deletions(-)
>>
>> ... to that end, could you split these into a few patches?
>>
>> That way, knowledgeable people can focus their review on the code they
>> understand.
>>
>> That doesn't need to be a patch per file; all the KVM bits can be
>> collated in one patch, for example. However, the atomics, kvm, and
>> uaccess+word-at-a-time bits should certainly be separate patches given
>> their (existing) complexity.
>
> I agree the patch is too large, I considered to split it up but wasn't
> sure where to draw the line(s). Will try to find halfway reasonable
> batches :)
>
>> Otherwise, I have a couple of comments below.
>>
>> > diff --git a/arch/arm64/include/asm/arch_gicv3.h 
>> > b/arch/arm64/include/asm/arch_gicv3.h
>> > index f37e3a21f6e7..ba54e5bee885 100644
>> > --- a/arch/arm64/include/asm/arch_gicv3.h
>> > +++ b/arch/arm64/include/asm/arch_gicv3.h
>> > @@ -166,7 +166,7 @@ static inline void gic_write_sre(u32 val)
>> >
>> >  static inline void gic_write_bpr1(u32 val)
>> >  {
>> > -   asm volatile("msr_s " __stringify(ICC_BPR1_EL1) ", %0" : : "r" (val));
>> > +   asm volatile("msr_s " __stringify(ICC_BPR1_EL1) ", %x0" : : "r" (val));
>> >  }
>>
>> Please make this use write_sysreg_s() instead, i.e.
>>
>> static inline void gic_write_bpr1(u32 val)
>> {
>>   write_sysreg_s(var, ICC_BPR1_EL1);
>> }
>>
>> ... that uses the 'x' modifier internally, and it's what we do for the
>> other GIC sysreg accesors.
>>
>> This accessor was missed by commit:
>>
>>   d44ffa5ae70a15a1 ("irqchip/gic-v3: Convert arm64 GIC accessors to 
>> {read,write}_sysreg_s")
>>
>> ... because it was added concurrently by commitL
>>
>>   91ef84428a86b75a ("irqchip/gic-v3: Reset BPR during initialization")
>>
>> ... i.e. it was not deliberately omitted.
>
> Will do
>
>> [...]
>>
>> > -   asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
>> > +   asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
>>
>> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
>> address must be 64-bit, so this would mask a legitimate warning.
>>
>> Given the prototype of this function the code if fine either way, but
>> were we to refactor things (e.g. making this a macro), that might not be
>> true.
>>
>> ... so I'm not sure it make sense to alter instances used for addresses.
>
> Good point, I'll leave instances dealing with addresses untouched for now.
>

OK, I am confused now. We started this thread under the assumption
that all unqualified placeholders are warned about by Clang. Given
that this appears not to be the case, could we please first find out
what causes the warnings? Is it necessary at all to add the x

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Matthias Kaehlcke]

Hi Mark,

Thanks for your comments.

El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:

> Hi,
> 
> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
> > Many inline assembly statements don't include the 'x' modifier when
> > using xN registers as operands. This is perfectly valid, however it
> > causes clang to raise warnings like this:
> > 
> > warning: value size does not match register size specified by the
> >   constraint and modifier [-Wasm-operand-widths]
> > ...
> > arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
> >   '__smp_store_release'
> > asm volatile ("stlr %1, %0"
> > 
> > Add the modifiers to keep clang happy.
> 
> If we're going to make this consistent, it would make sense to similarly
> annotate 'w' regs. That will make it easier going forward to enforce a
> policy that registers are suitably annotated.

Ok

> Also, there's a risk that we silently mask a bug here, for which clang's
> warning is legitimate, so we need to review this very carefully...
>
> > Signed-off-by: Matthias Kaehlcke 
> > ---
> > Changes in v2:
> > - also add modifiers to multiline ASM statements in include/asm/
> >   {atomic_ll_sc.h,irqflags.h,pgtable.h,uaccess.h,word-at-a-time.h}
> >   that were missed on v1
> > 
> >  arch/arm64/include/asm/arch_gicv3.h |  2 +-
> >  arch/arm64/include/asm/atomic_ll_sc.h   | 36 
> > -
> >  arch/arm64/include/asm/barrier.h|  4 ++--
> >  arch/arm64/include/asm/io.h | 24 +++---
> >  arch/arm64/include/asm/irqflags.h   | 10 -
> >  arch/arm64/include/asm/kvm_hyp.h| 10 -
> >  arch/arm64/include/asm/kvm_mmu.h| 12 +--
> >  arch/arm64/include/asm/percpu.h |  4 ++--
> >  arch/arm64/include/asm/pgtable.h| 20 +-
> >  arch/arm64/include/asm/sysreg.h |  4 ++--
> >  arch/arm64/include/asm/uaccess.h| 14 ++---
> >  arch/arm64/include/asm/word-at-a-time.h | 14 ++---
> >  arch/arm64/kernel/armv8_deprecated.c|  4 ++--
> >  arch/arm64/kernel/probes/kprobes.c  |  2 +-
> >  arch/arm64/kvm/hyp/switch.c |  4 ++--
> >  15 files changed, 82 insertions(+), 82 deletions(-)
> 
> ... to that end, could you split these into a few patches?
> 
> That way, knowledgeable people can focus their review on the code they
> understand.
> 
> That doesn't need to be a patch per file; all the KVM bits can be
> collated in one patch, for example. However, the atomics, kvm, and
> uaccess+word-at-a-time bits should certainly be separate patches given
> their (existing) complexity.

I agree the patch is too large, I considered to split it up but wasn't
sure where to draw the line(s). Will try to find halfway reasonable
batches :)

> Otherwise, I have a couple of comments below.
> 
> > diff --git a/arch/arm64/include/asm/arch_gicv3.h 
> > b/arch/arm64/include/asm/arch_gicv3.h
> > index f37e3a21f6e7..ba54e5bee885 100644
> > --- a/arch/arm64/include/asm/arch_gicv3.h
> > +++ b/arch/arm64/include/asm/arch_gicv3.h
> > @@ -166,7 +166,7 @@ static inline void gic_write_sre(u32 val)
> >  
> >  static inline void gic_write_bpr1(u32 val)
> >  {
> > -   asm volatile("msr_s " __stringify(ICC_BPR1_EL1) ", %0" : : "r" (val));
> > +   asm volatile("msr_s " __stringify(ICC_BPR1_EL1) ", %x0" : : "r" (val));
> >  }
> 
> Please make this use write_sysreg_s() instead, i.e.
> 
> static inline void gic_write_bpr1(u32 val)
> {
>   write_sysreg_s(var, ICC_BPR1_EL1);
> }
> 
> ... that uses the 'x' modifier internally, and it's what we do for the
> other GIC sysreg accesors.
> 
> This accessor was missed by commit:
> 
>   d44ffa5ae70a15a1 ("irqchip/gic-v3: Convert arm64 GIC accessors to 
> {read,write}_sysreg_s")
> 
> ... because it was added concurrently by commitL
> 
>   91ef84428a86b75a ("irqchip/gic-v3: Reset BPR during initialization")
> 
> ... i.e. it was not deliberately omitted.

Will do

> [...]
> 
> > -   asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
> > +   asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
> 
> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
> address must be 64-bit, so this would mask a legitimate warning.
> 
> Given the prototype of this function the code if fine either way, but
> were we to refactor things (e.g. making this a macro), that might not be
> true.
> 
> ... so I'm not sure it make sense to alter instances used for addresses.

Good point, I'll leave instances dealing with addresses untouched for now.

Cheers

Matthias

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Matthias Kaehlcke]

Hi Mark,

Thanks for your comments.

El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:

> Hi,
> 
> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
> > Many inline assembly statements don't include the 'x' modifier when
> > using xN registers as operands. This is perfectly valid, however it
> > causes clang to raise warnings like this:
> > 
> > warning: value size does not match register size specified by the
> >   constraint and modifier [-Wasm-operand-widths]
> > ...
> > arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
> >   '__smp_store_release'
> > asm volatile ("stlr %1, %0"
> > 
> > Add the modifiers to keep clang happy.
> 
> If we're going to make this consistent, it would make sense to similarly
> annotate 'w' regs. That will make it easier going forward to enforce a
> policy that registers are suitably annotated.

Ok

> Also, there's a risk that we silently mask a bug here, for which clang's
> warning is legitimate, so we need to review this very carefully...
>
> > Signed-off-by: Matthias Kaehlcke 
> > ---
> > Changes in v2:
> > - also add modifiers to multiline ASM statements in include/asm/
> >   {atomic_ll_sc.h,irqflags.h,pgtable.h,uaccess.h,word-at-a-time.h}
> >   that were missed on v1
> > 
> >  arch/arm64/include/asm/arch_gicv3.h |  2 +-
> >  arch/arm64/include/asm/atomic_ll_sc.h   | 36 
> > -
> >  arch/arm64/include/asm/barrier.h|  4 ++--
> >  arch/arm64/include/asm/io.h | 24 +++---
> >  arch/arm64/include/asm/irqflags.h   | 10 -
> >  arch/arm64/include/asm/kvm_hyp.h| 10 -
> >  arch/arm64/include/asm/kvm_mmu.h| 12 +--
> >  arch/arm64/include/asm/percpu.h |  4 ++--
> >  arch/arm64/include/asm/pgtable.h| 20 +-
> >  arch/arm64/include/asm/sysreg.h |  4 ++--
> >  arch/arm64/include/asm/uaccess.h| 14 ++---
> >  arch/arm64/include/asm/word-at-a-time.h | 14 ++---
> >  arch/arm64/kernel/armv8_deprecated.c|  4 ++--
> >  arch/arm64/kernel/probes/kprobes.c  |  2 +-
> >  arch/arm64/kvm/hyp/switch.c |  4 ++--
> >  15 files changed, 82 insertions(+), 82 deletions(-)
> 
> ... to that end, could you split these into a few patches?
> 
> That way, knowledgeable people can focus their review on the code they
> understand.
> 
> That doesn't need to be a patch per file; all the KVM bits can be
> collated in one patch, for example. However, the atomics, kvm, and
> uaccess+word-at-a-time bits should certainly be separate patches given
> their (existing) complexity.

I agree the patch is too large, I considered to split it up but wasn't
sure where to draw the line(s). Will try to find halfway reasonable
batches :)

> Otherwise, I have a couple of comments below.
> 
> > diff --git a/arch/arm64/include/asm/arch_gicv3.h 
> > b/arch/arm64/include/asm/arch_gicv3.h
> > index f37e3a21f6e7..ba54e5bee885 100644
> > --- a/arch/arm64/include/asm/arch_gicv3.h
> > +++ b/arch/arm64/include/asm/arch_gicv3.h
> > @@ -166,7 +166,7 @@ static inline void gic_write_sre(u32 val)
> >  
> >  static inline void gic_write_bpr1(u32 val)
> >  {
> > -   asm volatile("msr_s " __stringify(ICC_BPR1_EL1) ", %0" : : "r" (val));
> > +   asm volatile("msr_s " __stringify(ICC_BPR1_EL1) ", %x0" : : "r" (val));
> >  }
> 
> Please make this use write_sysreg_s() instead, i.e.
> 
> static inline void gic_write_bpr1(u32 val)
> {
>   write_sysreg_s(var, ICC_BPR1_EL1);
> }
> 
> ... that uses the 'x' modifier internally, and it's what we do for the
> other GIC sysreg accesors.
> 
> This accessor was missed by commit:
> 
>   d44ffa5ae70a15a1 ("irqchip/gic-v3: Convert arm64 GIC accessors to 
> {read,write}_sysreg_s")
> 
> ... because it was added concurrently by commitL
> 
>   91ef84428a86b75a ("irqchip/gic-v3: Reset BPR during initialization")
> 
> ... i.e. it was not deliberately omitted.

Will do

> [...]
> 
> > -   asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
> > +   asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
> 
> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
> address must be 64-bit, so this would mask a legitimate warning.
> 
> Given the prototype of this function the code if fine either way, but
> were we to refactor things (e.g. making this a macro), that might not be
> true.
> 
> ... so I'm not sure it make sense to alter instances used for addresses.

Good point, I'll leave instances dealing with addresses untouched for now.

Cheers

Matthias

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Mark Rutland]

Hi,

On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
> Many inline assembly statements don't include the 'x' modifier when
> using xN registers as operands. This is perfectly valid, however it
> causes clang to raise warnings like this:
> 
> warning: value size does not match register size specified by the
>   constraint and modifier [-Wasm-operand-widths]
> ...
> arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
>   '__smp_store_release'
> asm volatile ("stlr %1, %0"
> 
> Add the modifiers to keep clang happy.

If we're going to make this consistent, it would make sense to similarly
annotate 'w' regs. That will make it easier going forward to enforce a
policy that registers are suitably annotated.

Also, there's a risk that we silently mask a bug here, for which clang's
warning is legitimate, so we need to review this very carefully...

> 
> Signed-off-by: Matthias Kaehlcke 
> ---
> Changes in v2:
> - also add modifiers to multiline ASM statements in include/asm/
>   {atomic_ll_sc.h,irqflags.h,pgtable.h,uaccess.h,word-at-a-time.h}
>   that were missed on v1
> 
>  arch/arm64/include/asm/arch_gicv3.h |  2 +-
>  arch/arm64/include/asm/atomic_ll_sc.h   | 36 
> -
>  arch/arm64/include/asm/barrier.h|  4 ++--
>  arch/arm64/include/asm/io.h | 24 +++---
>  arch/arm64/include/asm/irqflags.h   | 10 -
>  arch/arm64/include/asm/kvm_hyp.h| 10 -
>  arch/arm64/include/asm/kvm_mmu.h| 12 +--
>  arch/arm64/include/asm/percpu.h |  4 ++--
>  arch/arm64/include/asm/pgtable.h| 20 +-
>  arch/arm64/include/asm/sysreg.h |  4 ++--
>  arch/arm64/include/asm/uaccess.h| 14 ++---
>  arch/arm64/include/asm/word-at-a-time.h | 14 ++---
>  arch/arm64/kernel/armv8_deprecated.c|  4 ++--
>  arch/arm64/kernel/probes/kprobes.c  |  2 +-
>  arch/arm64/kvm/hyp/switch.c |  4 ++--
>  15 files changed, 82 insertions(+), 82 deletions(-)

... to that end, could you split these into a few patches?

That way, knowledgeable people can focus their review on the code they
understand.

That doesn't need to be a patch per file; all the KVM bits can be
collated in one patch, for example. However, the atomics, kvm, and
uaccess+word-at-a-time bits should certainly be separate patches given
their (existing) complexity.

Otherwise, I have a couple of comments below.

> diff --git a/arch/arm64/include/asm/arch_gicv3.h 
> b/arch/arm64/include/asm/arch_gicv3.h
> index f37e3a21f6e7..ba54e5bee885 100644
> --- a/arch/arm64/include/asm/arch_gicv3.h
> +++ b/arch/arm64/include/asm/arch_gicv3.h
> @@ -166,7 +166,7 @@ static inline void gic_write_sre(u32 val)
>  
>  static inline void gic_write_bpr1(u32 val)
>  {
> - asm volatile("msr_s " __stringify(ICC_BPR1_EL1) ", %0" : : "r" (val));
> + asm volatile("msr_s " __stringify(ICC_BPR1_EL1) ", %x0" : : "r" (val));
>  }

Please make this use write_sysreg_s() instead, i.e.

static inline void gic_write_bpr1(u32 val)
{
write_sysreg_s(var, ICC_BPR1_EL1);
}

... that uses the 'x' modifier internally, and it's what we do for the
other GIC sysreg accesors.

This accessor was missed by commit:

  d44ffa5ae70a15a1 ("irqchip/gic-v3: Convert arm64 GIC accessors to 
{read,write}_sysreg_s")

... because it was added concurrently by commitL

  91ef84428a86b75a ("irqchip/gic-v3: Reset BPR during initialization")

... i.e. it was not deliberately omitted.

[...]

> - asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
> + asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));

In general, the '[%xN]' pattern looks *very* suspicious to me. Any
address must be 64-bit, so this would mask a legitimate warning.

Given the prototype of this function the code if fine either way, but
were we to refactor things (e.g. making this a macro), that might not be
true.

... so I'm not sure it make sense to alter instances used for addresses.

Thanks,
Mark.

Re: [PATCH v2] arm64: Add ASM modifier for xN register operands

[Mark Rutland]

Hi,

On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
> Many inline assembly statements don't include the 'x' modifier when
> using xN registers as operands. This is perfectly valid, however it
> causes clang to raise warnings like this:
> 
> warning: value size does not match register size specified by the
>   constraint and modifier [-Wasm-operand-widths]
> ...
> arch/arm64/include/asm/barrier.h:62:23: note: expanded from macro
>   '__smp_store_release'
> asm volatile ("stlr %1, %0"
> 
> Add the modifiers to keep clang happy.

If we're going to make this consistent, it would make sense to similarly
annotate 'w' regs. That will make it easier going forward to enforce a
policy that registers are suitably annotated.

Also, there's a risk that we silently mask a bug here, for which clang's
warning is legitimate, so we need to review this very carefully...

> 
> Signed-off-by: Matthias Kaehlcke 
> ---
> Changes in v2:
> - also add modifiers to multiline ASM statements in include/asm/
>   {atomic_ll_sc.h,irqflags.h,pgtable.h,uaccess.h,word-at-a-time.h}
>   that were missed on v1
> 
>  arch/arm64/include/asm/arch_gicv3.h |  2 +-
>  arch/arm64/include/asm/atomic_ll_sc.h   | 36 
> -
>  arch/arm64/include/asm/barrier.h|  4 ++--
>  arch/arm64/include/asm/io.h | 24 +++---
>  arch/arm64/include/asm/irqflags.h   | 10 -
>  arch/arm64/include/asm/kvm_hyp.h| 10 -
>  arch/arm64/include/asm/kvm_mmu.h| 12 +--
>  arch/arm64/include/asm/percpu.h |  4 ++--
>  arch/arm64/include/asm/pgtable.h| 20 +-
>  arch/arm64/include/asm/sysreg.h |  4 ++--
>  arch/arm64/include/asm/uaccess.h| 14 ++---
>  arch/arm64/include/asm/word-at-a-time.h | 14 ++---
>  arch/arm64/kernel/armv8_deprecated.c|  4 ++--
>  arch/arm64/kernel/probes/kprobes.c  |  2 +-
>  arch/arm64/kvm/hyp/switch.c |  4 ++--
>  15 files changed, 82 insertions(+), 82 deletions(-)

... to that end, could you split these into a few patches?

That way, knowledgeable people can focus their review on the code they
understand.

That doesn't need to be a patch per file; all the KVM bits can be
collated in one patch, for example. However, the atomics, kvm, and
uaccess+word-at-a-time bits should certainly be separate patches given
their (existing) complexity.

Otherwise, I have a couple of comments below.

> diff --git a/arch/arm64/include/asm/arch_gicv3.h 
> b/arch/arm64/include/asm/arch_gicv3.h
> index f37e3a21f6e7..ba54e5bee885 100644
> --- a/arch/arm64/include/asm/arch_gicv3.h
> +++ b/arch/arm64/include/asm/arch_gicv3.h
> @@ -166,7 +166,7 @@ static inline void gic_write_sre(u32 val)
>  
>  static inline void gic_write_bpr1(u32 val)
>  {
> - asm volatile("msr_s " __stringify(ICC_BPR1_EL1) ", %0" : : "r" (val));
> + asm volatile("msr_s " __stringify(ICC_BPR1_EL1) ", %x0" : : "r" (val));
>  }

Please make this use write_sysreg_s() instead, i.e.

static inline void gic_write_bpr1(u32 val)
{
write_sysreg_s(var, ICC_BPR1_EL1);
}

... that uses the 'x' modifier internally, and it's what we do for the
other GIC sysreg accesors.

This accessor was missed by commit:

  d44ffa5ae70a15a1 ("irqchip/gic-v3: Convert arm64 GIC accessors to 
{read,write}_sysreg_s")

... because it was added concurrently by commitL

  91ef84428a86b75a ("irqchip/gic-v3: Reset BPR during initialization")

... i.e. it was not deliberately omitted.

[...]

> - asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
> + asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));

In general, the '[%xN]' pattern looks *very* suspicious to me. Any
address must be 64-bit, so this would mask a legitimate warning.

Given the prototype of this function the code if fine either way, but
were we to refactor things (e.g. making this a macro), that might not be
true.

... so I'm not sure it make sense to alter instances used for addresses.

Thanks,
Mark.