Re: [PATCH 02/13] arm64/kvm: Fix assembler compatibility of macros

From: Mark Rutland <mark.rutland@arm.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Geoff Levand <geoff@infradead.org>,
	"kexec@lists.infradead.org" <kexec@lists.infradead.org>,
	Will Deacon <Will.Deacon@arm.com>,
	Marc Zyngier <Marc.Zyngier@arm.com>,
	Christoffer Dall <christoffer.dall@linaro.org>,
	Catalin Marinas <Catalin.Marinas@arm.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 02/13] arm64/kvm: Fix assembler compatibility of macros
Date: Mon, 15 Sep 2014 17:14:51 +0100	[thread overview]
Message-ID: <20140915161451.GC13789@leverpostej> (raw)
In-Reply-To: <CAKv+Gu_uU3cQZbsa2w=GtXCGd59PhD-conRn3szLkOFKmTngSA@mail.gmail.com>

On Wed, Sep 10, 2014 at 06:09:07PM +0100, Ard Biesheuvel wrote:
> On 10 September 2014 18:35, Geoff Levand <geoff@infradead.org> wrote:
> > On Wed, 2014-09-10 at 10:40 +0200, Ard Biesheuvel wrote:
> >> On 10 September 2014 00:49, Geoff Levand <geoff@infradead.org> wrote:
> >> > Some of the macros defined in kvm_arm.h are useful in the exception vector
> >> > routines, but they are not compatible with the assembler.  Change the
> >> > definition of ESR_EL2_ISS to be compatible.
> >> >
> >> > Fixes build errors like these when using kvm_arm.h in assembly
> >> > source files:
> >> >
> >> >   Error: unexpected characters following instruction at operand 3 -- `add x0,x1,#((1U<<25)-1)'
> >> >
> >> > Signed-off-by: Geoff Levand <geoff@infradead.org>
> >> > ---
> >> >  arch/arm64/include/asm/kvm_arm.h | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> >> > index cc83520..e0e7e64 100644
> >> > --- a/arch/arm64/include/asm/kvm_arm.h
> >> > +++ b/arch/arm64/include/asm/kvm_arm.h
> >> > @@ -176,7 +176,7 @@
> >> >  #define ESR_EL2_EC_SHIFT       (26)
> >> >  #define ESR_EL2_EC             (0x3fU << ESR_EL2_EC_SHIFT)
> >> >  #define ESR_EL2_IL             (1U << 25)
> >> > -#define ESR_EL2_ISS            (ESR_EL2_IL - 1)
> >> > +#define ESR_EL2_ISS            (0xffff)
> >>
> >> Don't you mean 0x1ffffff?
> >
> > Hcalls have a 16 bit 'payload', the upper bits of the ISS field
> > are specified as zero by the architecture so 0xffff is the same
> > as 0x1ffffff.
> >
> 
> Even if HVC is currently the only exception we are taking in EL2 (is
> that the case btw?), it seems wrong to define this field in such a way
> that it
> (a) deviates from how the architecture specifies ESR_ELx.ISS and
> (b) may cause surprises once someone unsuspectingly starts and'ing his
> ESR values produced by another exception class with it, expecting the
> macro's value to reflect its name

Agreed. A macro called ESR_EL2_ISS should return the ISS field, and
nothing less.

> >> And, there is a macro UL() for this purpose, so I suppose you could
> >> redefine ESR_EL2_IL as (UL(1) << 25) as well. I know it is not
> >> strictly the same thing, but it should be good enough as this is arm64
> >> only

This sounds good to me.

Mark.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec