[PATCH v4 25/40] KVM: arm64: Introduce framework for accessing deferred sysregs

[Christoffer Dall]

On Thu, Feb 22, 2018 at 02:40:52PM +0100, Andrew Jones wrote:
> On Thu, Feb 15, 2018 at 10:03:17PM +0100, Christoffer Dall wrote:
> > We are about to defer saving and restoring some groups of system
> > registers to vcpu_put and vcpu_load on supported systems.  This means
> > that we need some infrastructure to access system registes which
> > supports either accessing the memory backing of the register or directly
> > accessing the system registers, depending on the state of the system
> > when we access the register.
> > 
> > We do this by defining read/write accessor functions, which can handle
> > both "immediate" and "deferrable" system registers.  Immediate registers
> > are always saved/restored in the world-switch path, but deferrable
> > registers are only saved/restored in vcpu_put/vcpu_load when supported
> > and sysregs_loaded_on_cpu will be set in that case.
> > 
> > Note that we don't use the deferred mechanism yet in this patch, but only
> > introduce infrastructure.  This is to improve convenience of review in
> > the subsequent patches where it is clear which registers become
> > deferred.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall at linaro.org>
> > ---
> > 
> > Notes:
> >     Changes since v3:
> >      - Changed to a switch-statement based approach to improve
> >        readability.
> >     
> >     Changes since v2:
> >      - New patch (deferred register handling has been reworked)
> > 
> >  arch/arm64/include/asm/kvm_host.h |  8 ++++++--
> >  arch/arm64/kvm/sys_regs.c         | 33 +++++++++++++++++++++++++++++++++
> >  2 files changed, 39 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 68398bf7882f..b463b5e28959 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -284,6 +284,10 @@ struct kvm_vcpu_arch {
> >  
> >  	/* Virtual SError ESR to restore when HCR_EL2.VSE is set */
> >  	u64 vsesr_el2;
> > +
> > +	/* True when deferrable sysregs are loaded on the physical CPU,
> > +	 * see kvm_vcpu_load_sysregs and kvm_vcpu_put_sysregs. */
> > +	bool sysregs_loaded_on_cpu;
> >  };
> >  
> >  #define vcpu_gp_regs(v)		(&(v)->arch.ctxt.gp_regs)
> > @@ -296,8 +300,8 @@ struct kvm_vcpu_arch {
> >   */
> >  #define __vcpu_sys_reg(v,r)	((v)->arch.ctxt.sys_regs[(r)])
> >  
> > -#define vcpu_read_sys_reg(v,r)	__vcpu_sys_reg(v,r)
> > -#define vcpu_write_sys_reg(v,r,n)	do { __vcpu_sys_reg(v,r) = n; } while (0)
> > +u64 vcpu_read_sys_reg(struct kvm_vcpu *vcpu, int reg);
> > +void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, int reg, u64 val);
> >  
> >  /*
> >   * CP14 and CP15 live in the same array, as they are backed by the
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index a05d2c01c786..b3c3f014aa61 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -35,6 +35,7 @@
> >  #include <asm/kvm_coproc.h>
> >  #include <asm/kvm_emulate.h>
> >  #include <asm/kvm_host.h>
> > +#include <asm/kvm_hyp.h>
> >  #include <asm/kvm_mmu.h>
> >  #include <asm/perf_event.h>
> >  #include <asm/sysreg.h>
> > @@ -76,6 +77,38 @@ static bool write_to_read_only(struct kvm_vcpu *vcpu,
> >  	return false;
> >  }
> >  
> > +u64 vcpu_read_sys_reg(struct kvm_vcpu *vcpu, int reg)
> > +{
> > +	if (!vcpu->arch.sysregs_loaded_on_cpu)
> > +		goto immediate_read;
> > +
> > +	/*
> > +	 * All system registers listed in the switch are not saved on every
> > +	 * exit from the guest but are only saved on vcpu_put.
> 
> The "All ... are not" doesn't flow well for me. How about
> 
>  /*
>   * None of the system registers listed in the switch are saved on guest
>   * exit. These registers are only saved on vcpu_put.
>   */
> 
> > +	 */
> > +	switch (reg) {
> > +	}
> > +
> > +immediate_read:
> > +	return __vcpu_sys_reg(vcpu, reg);
> > +}
> > +
> > +void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, int reg, u64 val)
> > +{
> > +	if (!vcpu->arch.sysregs_loaded_on_cpu)
> > +		goto immediate_write;
> > +
> > +	/*
> > +	 * All system registers listed in the switch are not restored on every
> > +	 * entry to the guest but are only restored on vcpu_load.
> > +	 */
> 
>  /*
>   * None of the system registers listed in the switch are restored on
>   * guest entry. If these registers were saved due to a vcpu_put, then
>   * they will be restored by vcpu_load.
>   */
> 

Hmmm, not sure the last sentence helps here.

I'll think about some nicer wording for you for the next version.

> > +	switch (reg) {
> > +	}
> > +
> > +immediate_write:
> > +	 __vcpu_sys_reg(vcpu, reg) = val;
> > +}
> > +
> >  /* 3 bits per cache level, as per CLIDR, but non-existent caches always 0 */
> >  static u32 cache_levels;
> >  
> > -- 
> > 2.14.2
> >
> 
> Otherwise
> 
> Reviewed-by: Andrew Jones <drjones at redhat.com>

Thanks,
-Christoffer