On Mon, Feb 19, 2018 at 06:12:29PM +0000, Julien Grall wrote:
> Hi Christoffer,
>
> On 15/02/18 21:03, Christoffer Dall wrote:
> >From: Christoffer Dall <cd...@cs.columbia.edu>
> >
> >Currently we access the system registers array via the vcpu_sys_reg()
> >macro. However, we are about to change the behavior to some times
> >modify the register file directly, so let's change this to two
> >primitives:
> >
> > * Accessor macros vcpu_write_sys_reg() and vcpu_read_sys_reg()
> > * Direct array access macro __vcpu_sys_reg()
> >
> >The first primitive should be used in places where the code needs to
> >access the currently loaded VCPU's state as observed by the guest. For
> >example, when trapping on cache related registers, a write to a system
> >register should go directly to the VCPU version of the register.
> >
> >The second primitive can be used in places where the VCPU is known to
>
> "second primitive" is a bit confusing here. I count 3 primitives above:
> (vcpu_write_sys_reg(), vcpu_read_sys_reg() and __vcpu_sys_reg(). From the
> description, I would say to refer to the latter (i.e third one).
>
Good point. I'll clarify.
> >never be running (for example userspace access) or for registers which
> >are never context switched (for example all the PMU system registers).
> >
> >This rewrites all users of vcpu_sys_regs to one of the two primitives
> >above.
> >
> >No functional change.
> >
> >Signed-off-by: Christoffer Dall <cd...@cs.columbia.edu>
>
> [...]
>
> >diff --git a/arch/arm64/include/asm/kvm_host.h
> >b/arch/arm64/include/asm/kvm_host.h
> >index f2a6f39aec87..68398bf7882f 100644
> >--- a/arch/arm64/include/asm/kvm_host.h
> >+++ b/arch/arm64/include/asm/kvm_host.h
> >@@ -287,7 +287,18 @@ struct kvm_vcpu_arch {
> > };
> > #define vcpu_gp_regs(v) (&(v)->arch.ctxt.gp_regs)
> >-#define vcpu_sys_reg(v,r) ((v)->arch.ctxt.sys_regs[(r)])
> >+
> >+/*
> >+ * Only use __vcpu_sys_reg if you know you want the memory backed version
> >of a
> >+ * register, and not the one most recently accessed by a runnning VCPU. For
>
> NIT: s/runnning/running/
>
> >+ * example, for userpace access or for system registers that are never
> >context
>
> NIT: s/userpace/userspace/
>
> >+ * switched, but only emulated.
> >+ */
> >+#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)
> >+
> > /*
> > * CP14 and CP15 live in the same array, as they are backed by the
> > * same system registers.
>
> [...]
>
> >diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> >index b48af790615e..a05d2c01c786 100644
> >--- a/arch/arm64/kvm/sys_regs.c
> >+++ b/arch/arm64/kvm/sys_regs.c
>
> [...]
>
> >@@ -817,10 +818,10 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu,
> >struct sys_reg_params *p,
> > return false;
> > }
> >- vcpu_sys_reg(vcpu, PMUSERENR_EL0) = p->regval
> >- & ARMV8_PMU_USERENR_MASK;
> >- } else {
> >- p->regval = vcpu_sys_reg(vcpu, PMUSERENR_EL0)
> >+ __vcpu_sys_reg(vcpu, PMUSERENR_EL0) =
> >+ p->regval & ARMV8_PMU_USERENR_MASK;
> >+ } else {
>
> NIT: There is a double space between else and {.
>
> >+ p->regval = __vcpu_sys_reg(vcpu, PMUSERENR_EL0)
> > & ARMV8_PMU_USERENR_MASK;
> > }
>
Thanks,
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
