Re: [PATCH 07/59] KVM: arm64: nv: Add EL2 system registers to vcpu context

[Dave Martin <Dave.Martin@arm.com>]

On Wed, Jul 03, 2019 at 01:20:55PM +0100, Marc Zyngier wrote:
> On 24/06/2019 13:54, Dave Martin wrote:
> > On Fri, Jun 21, 2019 at 10:37:51AM +0100, Marc Zyngier wrote:
> >> From: Jintack Lim <jintack.lim@linaro.org>
> >>
> >> ARM v8.3 introduces a new bit in the HCR_EL2, which is the NV bit. When
> >> this bit is set, accessing EL2 registers in EL1 traps to EL2. In
> >> addition, executing the following instructions in EL1 will trap to EL2:
> >> tlbi, at, eret, and msr/mrs instructions to access SP_EL1. Most of the
> >> instructions that trap to EL2 with the NV bit were undef at EL1 prior to
> >> ARM v8.3. The only instruction that was not undef is eret.
> >>
> >> This patch sets up a handler for EL2 registers and SP_EL1 register
> >> accesses at EL1. The host hypervisor keeps those register values in
> >> memory, and will emulate their behavior.
> >>
> >> This patch doesn't set the NV bit yet. It will be set in a later patch
> >> once nested virtualization support is completed.
> >>
> >> Signed-off-by: Jintack Lim <jintack.lim@linaro.org>
> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> >> ---
> >>  arch/arm64/include/asm/kvm_host.h | 37 +++++++++++++++-
> >>  arch/arm64/include/asm/sysreg.h   | 50 ++++++++++++++++++++-
> >>  arch/arm64/kvm/sys_regs.c         | 74 ++++++++++++++++++++++++++++---
> >>  3 files changed, 154 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> >> index 4bcd9c1291d5..2d4290d2513a 100644
> >> --- a/arch/arm64/include/asm/kvm_host.h
> >> +++ b/arch/arm64/include/asm/kvm_host.h
> >> @@ -173,12 +173,47 @@ enum vcpu_sysreg {
> >>  	APGAKEYLO_EL1,
> >>  	APGAKEYHI_EL1,
> >>  
> >> -	/* 32bit specific registers. Keep them at the end of the range */
> >> +	/* 32bit specific registers. */
> > 
> > Out of interest, why did we originally want these to be at the end?
> > Because they're not at the end any more...
> 
> I seem to remember the original assembly switch code used that property.
> This is a long gone requirement, thankfully.

Ah, right.

> >>  	DACR32_EL2,	/* Domain Access Control Register */
> >>  	IFSR32_EL2,	/* Instruction Fault Status Register */
> >>  	FPEXC32_EL2,	/* Floating-Point Exception Control Register */
> >>  	DBGVCR32_EL2,	/* Debug Vector Catch Register */
> >>  
> >> +	/* EL2 registers sorted ascending by Op0, Op1, CRn, CRm, Op2 */
> >> +	FIRST_EL2_SYSREG,
> >> +	VPIDR_EL2 = FIRST_EL2_SYSREG,
> >> +			/* Virtualization Processor ID Register */
> >> +	VMPIDR_EL2,	/* Virtualization Multiprocessor ID Register */
> >> +	SCTLR_EL2,	/* System Control Register (EL2) */
> >> +	ACTLR_EL2,	/* Auxiliary Control Register (EL2) */
> >> +	HCR_EL2,	/* Hypervisor Configuration Register */
> >> +	MDCR_EL2,	/* Monitor Debug Configuration Register (EL2) */
> >> +	CPTR_EL2,	/* Architectural Feature Trap Register (EL2) */
> >> +	HSTR_EL2,	/* Hypervisor System Trap Register */
> >> +	HACR_EL2,	/* Hypervisor Auxiliary Control Register */
> >> +	TTBR0_EL2,	/* Translation Table Base Register 0 (EL2) */
> >> +	TTBR1_EL2,	/* Translation Table Base Register 1 (EL2) */
> >> +	TCR_EL2,	/* Translation Control Register (EL2) */
> >> +	VTTBR_EL2,	/* Virtualization Translation Table Base Register */
> >> +	VTCR_EL2,	/* Virtualization Translation Control Register */
> >> +	SPSR_EL2,	/* EL2 saved program status register */
> >> +	ELR_EL2,	/* EL2 exception link register */
> >> +	AFSR0_EL2,	/* Auxiliary Fault Status Register 0 (EL2) */
> >> +	AFSR1_EL2,	/* Auxiliary Fault Status Register 1 (EL2) */
> >> +	ESR_EL2,	/* Exception Syndrome Register (EL2) */
> >> +	FAR_EL2,	/* Hypervisor IPA Fault Address Register */
> >> +	HPFAR_EL2,	/* Hypervisor IPA Fault Address Register */
> >> +	MAIR_EL2,	/* Memory Attribute Indirection Register (EL2) */
> >> +	AMAIR_EL2,	/* Auxiliary Memory Attribute Indirection Register (EL2) */
> >> +	VBAR_EL2,	/* Vector Base Address Register (EL2) */
> >> +	RVBAR_EL2,	/* Reset Vector Base Address Register */
> >> +	RMR_EL2,	/* Reset Management Register */
> >> +	CONTEXTIDR_EL2,	/* Context ID Register (EL2) */
> >> +	TPIDR_EL2,	/* EL2 Software Thread ID Register */
> >> +	CNTVOFF_EL2,	/* Counter-timer Virtual Offset register */
> >> +	CNTHCTL_EL2,	/* Counter-timer Hypervisor Control register */
> >> +	SP_EL2,		/* EL2 Stack Pointer */
> >> +
> > 
> > I wonder whether we could make these conditionally present somehow.  Not
> > worth worrying about for now to save 200-odd bytes per vcpu though.
> 
> With 8.4-NV, these 200 bytes turn into a whole 8kB (4kB page, plus
> almost 4kB of padding that I need to reduce one way or another). So I'm
> not too worried about this for now.
> 
> I really want the NV code to always be present though, in order to avoid
> configuration related regressions. I'm not sure how to make this better.

Fair enough -- sounds like addressing this would probably be premature
optimisation, then.

I suppose we could have two alternate layouts, but would likely be a
source of overhead, and bugs...

Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm