Re: [PATCH 2/3] KVM: arm/arm64: correct CPSR on exception entry

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

 



On Fri, Dec 27, 2019 at 03:42:34PM +0000, Alexandru Elisei wrote:
> Hi,
> 
> On 12/20/19 3:05 PM, Mark Rutland wrote:
> > When KVM injects an exception into a guest, it generates the CPSR value
> > from scratch, configuring CPSR.{M,A,I,T,E}, and setting all other
> > bits to zero.
> >
> > This isn't correct, as the architecture specifies that some CPSR bits
> > are (conditionally) cleared or set upon an exception, and others are
> > unchanged from the original context.
> >
> > This patch adds logic to match the architectural behaviour. To make this
> > simple to follow/audit/extend, documentation references are provided,
> > and bits are configured in order of their layout in SPSR_EL2. This
> 
> This patch applies equally well to 32bit KVM, where we have a SPSR register.

Indeed. The above wasn't intended to imply otherwise, but I'll add the
following to make that clear:

| Note that this code is used by both arm and arm64, and is intended to
| fuction with the SPSR_EL2 and SPSR_hyp layouts.

[...]

> >  /* arm64 compatibility macros */
> > +#define PSR_AA32_MODE_FIQ	FIQ_MODE
> > +#define PSR_AA32_MODE_SVC	SVC_MODE
> >  #define PSR_AA32_MODE_ABT	ABT_MODE
> >  #define PSR_AA32_MODE_UND	UND_MODE
> >  #define PSR_AA32_T_BIT		PSR_T_BIT
> > +#define PSR_AA32_F_BIT		PSR_F_BIT
> >  #define PSR_AA32_I_BIT		PSR_I_BIT
> >  #define PSR_AA32_A_BIT		PSR_A_BIT
> >  #define PSR_AA32_E_BIT		PSR_E_BIT
> >  #define PSR_AA32_IT_MASK	PSR_IT_MASK
> > +#define PSR_AA32_GE_MASK	0x000f0000
> > +#define PSR_AA32_PAN_BIT	0x00400000
> > +#define PSR_AA32_SSBS_BIT	0x00800000
> > +#define PSR_AA32_DIT_BIT	0x01000000
> 
> This is actually PSR_J_BIT on arm. Maybe it's worth defining it like that to
> make the overlap obvious.

Another bug! These are intended to match the SPSR_hyp view, where DIT is
bit 21, and so this should be:

#define PSR_AA32_DIT_BIT	0x0x00200000

... placed immediately before the PAN bit.

We don't need a macro for the J bit as it was obsoleted and made RES0 by
the ARMv7 virtualization extensions.

[...]

> > +	// CPSR.PAN is unchanged unless overridden by SCTLR.SPAN
> > +	// See ARM DDI 0487E.a, page G8-6246
> > +	new |= (old & PSR_AA32_PAN_BIT);
> > +	if (sctlr & BIT(23))
> > +		new |= PSR_AA32_PAN_BIT;
> 
> PSTATE.PAN is unconditionally set when SCTLR.SPAN is clear.

Indeed, and this time the reference is explicit about that. :/

I've updated this to:

|	// CPSR.PAN is unchanged unless SCTLR.SPAN == 0b0
|	// SCTLR.SPAN is RES1 when ARMv8.1-PAN is not implemented
|	// See ARM DDI 0487E.a, page G8-6246
|	new |= (old & PSR_AA32_PAN_BIT);
|	if (!(sctlr & BIT(23)))
|		new |= PSR_AA32_PAN_BIT;

[...]

> I've also checked that the ARM documentation references match the code.

Thanks, your review is much appreciated!

Mark.



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux