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

[Matthias Kaehlcke <mka@chromium.org>]

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 <mka@chromium.org>
> > ---
> > 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