From: christoffer.dall@linaro.org (Christoffer Dall)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v4 24/40] KVM: arm64: Rewrite system register accessors to read/write functions
Date: Thu, 22 Feb 2018 12:10:05 +0100 [thread overview]
Message-ID: <20180222111005.GL29376@cbox> (raw)
In-Reply-To: <86po4xjo5x.wl-marc.zyngier@arm.com>
On Thu, Feb 22, 2018 at 10:48:10AM +0000, Marc Zyngier wrote:
> On Thu, 22 Feb 2018 09:22:37 +0000,
> Christoffer Dall wrote:
> >
> > On Wed, Feb 21, 2018 at 01:32:45PM +0000, Marc Zyngier wrote:
> > > On Thu, 15 Feb 2018 21:03:16 +0000,
> > > Christoffer Dall wrote:
> > > >
> > > > From: Christoffer Dall <cdall@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
> > > > 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 <cdall@cs.columbia.edu>
> > > > ---
> > > >
> > > > Notes:
> > > > Changes since v2:
> > > > - New patch (deferred register handling has been reworked)
> > > >
> > > > arch/arm64/include/asm/kvm_emulate.h | 13 ++++---
> > > > arch/arm64/include/asm/kvm_host.h | 13 ++++++-
> > > > arch/arm64/include/asm/kvm_mmu.h | 2 +-
> > > > arch/arm64/kvm/debug.c | 27 +++++++++-----
> > > > arch/arm64/kvm/inject_fault.c | 8 ++--
> > > > arch/arm64/kvm/sys_regs.c | 71 ++++++++++++++++++------------------
> > > > arch/arm64/kvm/sys_regs.h | 4 +-
> > > > arch/arm64/kvm/sys_regs_generic_v8.c | 4 +-
> > > > virt/kvm/arm/pmu.c | 37 ++++++++++---------
> > > > 9 files changed, 102 insertions(+), 77 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> > > > index 3cc535591bdf..d313aaae5c38 100644
> > > > --- a/arch/arm64/include/asm/kvm_emulate.h
> > > > +++ b/arch/arm64/include/asm/kvm_emulate.h
> > > > @@ -290,15 +290,18 @@ static inline int kvm_vcpu_sys_get_rt(struct kvm_vcpu *vcpu)
> > > >
> > > > static inline unsigned long kvm_vcpu_get_mpidr_aff(struct kvm_vcpu *vcpu)
> > > > {
> > > > - return vcpu_sys_reg(vcpu, MPIDR_EL1) & MPIDR_HWID_BITMASK;
> > > > + return vcpu_read_sys_reg(vcpu, MPIDR_EL1) & MPIDR_HWID_BITMASK;
> > > > }
> > > >
> > > > static inline void kvm_vcpu_set_be(struct kvm_vcpu *vcpu)
> > > > {
> > > > - if (vcpu_mode_is_32bit(vcpu))
> > > > + if (vcpu_mode_is_32bit(vcpu)) {
> > > > *vcpu_cpsr(vcpu) |= COMPAT_PSR_E_BIT;
> > > > - else
> > > > - vcpu_sys_reg(vcpu, SCTLR_EL1) |= (1 << 25);
> > > > + } else {
> > > > + u64 sctlr = vcpu_read_sys_reg(vcpu, SCTLR_EL1);
> > > > + sctlr |= (1 << 25);
> > > > + vcpu_write_sys_reg(vcpu, SCTLR_EL1, sctlr);
> > >
> > > General comment: it is slightly annoying that vcpu_write_sys_reg takes
> > > its parameters in an order different from that of write_sysreg
> > > (register followed with value, instead of value followed with
> > > register). Not a deal breaker, but slightly confusing.
> > >
> >
> > Ah, I didn't compare to write_sysreg, I was thinking that
> >
> > vcpu_read_sys_reg(vcpu, SCTLR_EL1);
> > vcpu_write_sys_reg(vcpu, SCTLR_EL1, val);
> >
> > looked more symmetrical because the write just takes an extra value, but
> > I can see your argument as well.
> >
> > I don't mind changing it if it matters to you?
>
> I'd like to see that changed, but it doesn't have to be as part of
> this series if it is going to cause a refactoring mess. We can address
> it as a blanket fix after this series.
>
I think it's reasonably self-contained.
Just so I'm sure, are these the primitives you'd like to see?
vcpu_read_sys_reg(struct kvm_vcpu *vcpu, int reg);
vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg);
> >
> > > > + }
> > > > }
> > > >
> > > > static inline bool kvm_vcpu_is_be(struct kvm_vcpu *vcpu)
> > > > @@ -306,7 +309,7 @@ static inline bool kvm_vcpu_is_be(struct kvm_vcpu *vcpu)
> > > > if (vcpu_mode_is_32bit(vcpu))
> > > > return !!(*vcpu_cpsr(vcpu) & COMPAT_PSR_E_BIT);
> > > >
> > > > - return !!(vcpu_sys_reg(vcpu, SCTLR_EL1) & (1 << 25));
> > > > + return !!(vcpu_read_sys_reg(vcpu, SCTLR_EL1) & (1 << 25));
> > > > }
> > > >
> > > > static inline unsigned long vcpu_data_guest_to_host(struct kvm_vcpu *vcpu,
> > > > 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
> > > > + * example, for userpace access or for system registers that are never context
> > > > + * 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/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
> > > > index 9679067a1574..95f46e73c4dc 100644
> > > > --- a/arch/arm64/include/asm/kvm_mmu.h
> > > > +++ b/arch/arm64/include/asm/kvm_mmu.h
> > > > @@ -249,7 +249,7 @@ struct kvm;
> > > >
> > > > static inline bool vcpu_has_cache_enabled(struct kvm_vcpu *vcpu)
> > > > {
> > > > - return (vcpu_sys_reg(vcpu, SCTLR_EL1) & 0b101) == 0b101;
> > > > + return (vcpu_read_sys_reg(vcpu, SCTLR_EL1) & 0b101) == 0b101;
> > > > }
> > > >
> > > > static inline void __clean_dcache_guest_page(kvm_pfn_t pfn, unsigned long size)
> > > > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> > > > index feedb877cff8..db32d10a56a1 100644
> > > > --- a/arch/arm64/kvm/debug.c
> > > > +++ b/arch/arm64/kvm/debug.c
> > > > @@ -46,7 +46,8 @@ static DEFINE_PER_CPU(u32, mdcr_el2);
> > > > */
> > > > static void save_guest_debug_regs(struct kvm_vcpu *vcpu)
> > > > {
> > > > - vcpu->arch.guest_debug_preserved.mdscr_el1 = vcpu_sys_reg(vcpu, MDSCR_EL1);
> > > > + vcpu->arch.guest_debug_preserved.mdscr_el1 =
> > > > + vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> > > >
> > > > trace_kvm_arm_set_dreg32("Saved MDSCR_EL1",
> > > > vcpu->arch.guest_debug_preserved.mdscr_el1);
> > > > @@ -54,10 +55,11 @@ static void save_guest_debug_regs(struct kvm_vcpu *vcpu)
> > > >
> > > > static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
> > > > {
> > > > - vcpu_sys_reg(vcpu, MDSCR_EL1) = vcpu->arch.guest_debug_preserved.mdscr_el1;
> > > > + vcpu_write_sys_reg(vcpu, MDSCR_EL1,
> > > > + vcpu->arch.guest_debug_preserved.mdscr_el1);
> > > >
> > > > trace_kvm_arm_set_dreg32("Restored MDSCR_EL1",
> > > > - vcpu_sys_reg(vcpu, MDSCR_EL1));
> > > > + vcpu_read_sys_reg(vcpu, MDSCR_EL1));
> > > > }
> > > >
> > > > /**
> > > > @@ -108,6 +110,7 @@ void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu)
> > > > void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
> > > > {
> > > > bool trap_debug = !(vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY);
> > > > + unsigned long mdscr;
> > > >
> > > > trace_kvm_arm_setup_debug(vcpu, vcpu->guest_debug);
> > > >
> > > > @@ -152,9 +155,13 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
> > > > */
> > > > if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> > > > *vcpu_cpsr(vcpu) |= DBG_SPSR_SS;
> > > > - vcpu_sys_reg(vcpu, MDSCR_EL1) |= DBG_MDSCR_SS;
> > > > + mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> > > > + mdscr |= DBG_MDSCR_SS;
> > > > + vcpu_write_sys_reg(vcpu, MDSCR_EL1, mdscr);
> > >
> > > I have the feeling that we're going to need some clearbits/setbits
> > > variants of vcpu_write_sysreg at some point.
> > >
> >
> > I can introduce these now if you prefer?
>
> Probably not yet. There is a number of places where we could do a
> batter job at dealing with bitfields, the GICv3 cpuif emulation code
> being a primary offender. If we start having these kind of primitives,
> we can derive sysreg accessors from them in the long run.
>
Ok, I'll leave this alone for now then.
Thanks,
-Christoffer
next prev parent reply other threads:[~2018-02-22 11:10 UTC|newest]
Thread overview: 138+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-15 21:02 [PATCH v4 00/40] Optimize KVM/ARM for VHE systems Christoffer Dall
2018-02-15 21:02 ` [PATCH v4 01/40] KVM: arm/arm64: Avoid vcpu_load for other vcpu ioctls than KVM_RUN Christoffer Dall
2018-02-21 9:32 ` Marc Zyngier
2018-02-21 17:05 ` Andrew Jones
2018-02-15 21:02 ` [PATCH v4 02/40] KVM: arm/arm64: Move vcpu_load call after kvm_vcpu_first_run_init Christoffer Dall
2018-02-21 11:05 ` Marc Zyngier
2018-02-15 21:02 ` [PATCH v4 03/40] KVM: arm64: Avoid storing the vcpu pointer on the stack Christoffer Dall
2018-02-19 15:50 ` Julien Grall
2018-02-22 8:43 ` Christoffer Dall
2018-02-21 11:34 ` Marc Zyngier
2018-02-22 9:02 ` Christoffer Dall
2018-02-22 9:35 ` Marc Zyngier
2018-02-22 9:49 ` Andrew Jones
2018-02-21 17:32 ` Andrew Jones
2018-02-22 9:10 ` Christoffer Dall
2018-02-22 9:56 ` Andrew Jones
2018-02-22 10:24 ` Christoffer Dall
2018-02-15 21:02 ` [PATCH v4 04/40] KVM: arm64: Rework hyp_panic for VHE and non-VHE Christoffer Dall
2018-02-21 17:32 ` Andrew Jones
2018-02-15 21:02 ` [PATCH v4 05/40] KVM: arm64: Move HCR_INT_OVERRIDE to default HCR_EL2 guest flag Christoffer Dall
2018-02-15 21:02 ` [PATCH v4 06/40] KVM: arm/arm64: Get rid of vcpu->arch.irq_lines Christoffer Dall
2018-02-15 21:02 ` [PATCH v4 07/40] KVM: arm/arm64: Add kvm_vcpu_load_sysregs and kvm_vcpu_put_sysregs Christoffer Dall
2018-02-15 21:03 ` [PATCH v4 08/40] KVM: arm/arm64: Introduce vcpu_el1_is_32bit Christoffer Dall
2018-02-21 12:05 ` Marc Zyngier
2018-02-22 9:11 ` Christoffer Dall
2018-02-21 17:34 ` Andrew Jones
2018-02-15 21:03 ` [PATCH v4 09/40] KVM: arm64: Move debug dirty flag calculation out of world switch Christoffer Dall
2018-02-21 17:35 ` Andrew Jones
2018-02-15 21:03 ` [PATCH v4 10/40] KVM: arm64: Slightly improve debug save/restore functions Christoffer Dall
2018-02-21 17:39 ` Andrew Jones
2018-02-21 17:52 ` Marc Zyngier
2018-02-22 8:05 ` Andrew Jones
2018-02-24 18:32 ` Christoffer Dall
2018-02-24 20:16 ` Marc Zyngier
2018-02-15 21:03 ` [PATCH v4 11/40] KVM: arm64: Improve debug register save/restore flow Christoffer Dall
2018-02-21 17:40 ` Andrew Jones
2018-02-15 21:03 ` [PATCH v4 12/40] KVM: arm64: Factor out fault info population and gic workarounds Christoffer Dall
2018-02-21 17:41 ` Andrew Jones
2018-02-15 21:03 ` [PATCH v4 13/40] KVM: arm64: Introduce VHE-specific kvm_vcpu_run Christoffer Dall
2018-02-21 17:43 ` Andrew Jones
2018-02-21 18:18 ` Andrew Jones
2018-02-22 9:16 ` Christoffer Dall
2018-02-15 21:03 ` [PATCH v4 14/40] KVM: arm64: Remove kern_hyp_va() use in VHE switch function Christoffer Dall
2018-02-15 21:03 ` [PATCH v4 15/40] KVM: arm64: Don't deactivate VM on VHE systems Christoffer Dall
2018-02-15 21:03 ` [PATCH v4 16/40] KVM: arm64: Remove noop calls to timer save/restore from VHE switch Christoffer Dall
2018-02-21 17:43 ` Andrew Jones
2018-02-15 21:03 ` [PATCH v4 17/40] KVM: arm64: Move userspace system registers into separate function Christoffer Dall
2018-02-19 17:21 ` Julien Grall
2018-02-22 9:18 ` Christoffer Dall
2018-02-21 12:21 ` Marc Zyngier
2018-02-15 21:03 ` [PATCH v4 18/40] KVM: arm64: Rewrite sysreg alternatives to static keys Christoffer Dall
2018-02-21 17:44 ` Andrew Jones
2018-02-15 21:03 ` [PATCH v4 19/40] KVM: arm64: Introduce separate VHE/non-VHE sysreg save/restore functions Christoffer Dall
2018-02-15 21:03 ` [PATCH v4 20/40] KVM: arm/arm64: Remove leftover comment from kvm_vcpu_run_vhe Christoffer Dall
2018-02-21 17:47 ` Andrew Jones
2018-02-15 21:03 ` [PATCH v4 21/40] KVM: arm64: Unify non-VHE host/guest sysreg save and restore functions Christoffer Dall
2018-02-15 21:03 ` [PATCH v4 22/40] KVM: arm64: Don't save the host ELR_EL2 and SPSR_EL2 on VHE systems Christoffer Dall
2018-02-21 17:49 ` Andrew Jones
2018-02-15 21:03 ` [PATCH v4 23/40] KVM: arm64: Change 32-bit handling of VM system registers Christoffer Dall
2018-02-15 21:03 ` [PATCH v4 24/40] KVM: arm64: Rewrite system register accessors to read/write functions Christoffer Dall
2018-02-19 18:12 ` Julien Grall
2018-02-22 9:18 ` Christoffer Dall
2018-02-21 13:32 ` Marc Zyngier
2018-02-22 9:22 ` Christoffer Dall
2018-02-22 10:48 ` Marc Zyngier
2018-02-22 11:10 ` Christoffer Dall [this message]
2018-02-22 13:26 ` Marc Zyngier
2018-02-22 13:34 ` Andrew Jones
2018-02-22 14:35 ` Christoffer Dall
2018-02-22 15:11 ` Andrew Jones
2018-02-22 15:58 ` Christoffer Dall
2018-02-15 21:03 ` [PATCH v4 25/40] KVM: arm64: Introduce framework for accessing deferred sysregs Christoffer Dall
2018-02-21 14:16 ` Marc Zyngier
2018-02-22 13:40 ` Andrew Jones
2018-02-22 14:56 ` Christoffer Dall
2018-02-22 17:40 ` Julien Grall
2018-02-15 21:03 ` [PATCH v4 26/40] KVM: arm/arm64: Prepare to handle deferred save/restore of SPSR_EL1 Christoffer Dall
2018-02-21 14:47 ` Marc Zyngier
2018-02-22 9:25 ` Christoffer Dall
2018-02-22 13:49 ` Andrew Jones
2018-02-15 21:03 ` [PATCH v4 27/40] KVM: arm64: Prepare to handle deferred save/restore of ELR_EL1 Christoffer Dall
2018-02-21 15:08 ` Marc Zyngier
2018-02-22 13:51 ` Andrew Jones
2018-02-15 21:03 ` [PATCH v4 28/40] KVM: arm64: Defer saving/restoring 64-bit sysregs to vcpu load/put on VHE Christoffer Dall
2018-02-21 15:33 ` Marc Zyngier
2018-02-23 18:05 ` Christoffer Dall
2018-02-22 14:04 ` Andrew Jones
2018-02-22 18:30 ` Julien Grall
2018-02-22 18:31 ` Julien Grall
2018-02-22 19:49 ` Christoffer Dall
2018-02-22 19:48 ` Christoffer Dall
2018-02-15 21:03 ` [PATCH v4 29/40] KVM: arm64: Prepare to handle deferred save/restore of 32-bit registers Christoffer Dall
2018-02-21 15:44 ` Marc Zyngier
2018-02-22 14:30 ` Andrew Jones
2018-02-23 11:35 ` Julien Grall
2018-02-15 21:03 ` [PATCH v4 30/40] KVM: arm64: Defer saving/restoring 32-bit sysregs to vcpu load/put Christoffer Dall
2018-02-21 16:27 ` Marc Zyngier
2018-02-22 18:15 ` Christoffer Dall
2018-02-22 14:35 ` Andrew Jones
2018-02-22 14:58 ` Christoffer Dall
2018-02-15 21:03 ` [PATCH v4 31/40] KVM: arm64: Move common VHE/non-VHE trap config in separate functions Christoffer Dall
2018-02-21 17:59 ` Marc Zyngier
2018-02-22 18:17 ` Christoffer Dall
2018-02-25 21:27 ` Christoffer Dall
2018-02-22 15:34 ` Andrew Jones
2018-02-23 14:30 ` Julien Grall
2018-02-23 17:48 ` Christoffer Dall
2018-02-25 21:29 ` Christoffer Dall
2018-02-15 21:03 ` [PATCH v4 32/40] KVM: arm64: Directly call VHE and non-VHE FPSIMD enabled functions Christoffer Dall
2018-02-21 18:11 ` Marc Zyngier
2018-02-22 15:38 ` Andrew Jones
2018-02-15 21:03 ` [PATCH v4 33/40] KVM: arm64: Configure c15, PMU, and debug register traps on cpu load/put for VHE Christoffer Dall
2018-02-21 18:20 ` Marc Zyngier
2018-02-22 18:57 ` Christoffer Dall
2018-02-22 15:51 ` Andrew Jones
2018-02-15 21:03 ` [PATCH v4 34/40] KVM: arm64: Cleanup __activate_traps and __deactive_traps for VHE and non-VHE Christoffer Dall
2018-02-21 18:26 ` Marc Zyngier
2018-02-22 19:04 ` Christoffer Dall
2018-02-22 15:54 ` Andrew Jones
2018-02-15 21:03 ` [PATCH v4 35/40] KVM: arm/arm64: Get rid of vgic_elrsr Christoffer Dall
2018-02-22 9:45 ` Marc Zyngier
2018-02-23 14:44 ` Julien Grall
2018-02-23 17:50 ` Christoffer Dall
2018-02-15 21:03 ` [PATCH v4 36/40] KVM: arm/arm64: Handle VGICv2 save/restore from the main VGIC code Christoffer Dall
2018-02-15 21:03 ` [PATCH v4 37/40] KVM: arm/arm64: Move arm64-only vgic-v2-sr.c file to arm64 Christoffer Dall
2018-02-22 12:33 ` Marc Zyngier
2018-02-22 14:37 ` Christoffer Dall
2018-02-15 21:03 ` [PATCH v4 38/40] KVM: arm/arm64: Handle VGICv3 save/restore from the main VGIC code on VHE Christoffer Dall
2018-02-22 12:32 ` Marc Zyngier
2018-02-22 14:42 ` Christoffer Dall
2018-02-22 15:01 ` Marc Zyngier
2018-02-22 16:02 ` Christoffer Dall
2018-02-22 17:21 ` Marc Zyngier
2018-02-22 19:28 ` Christoffer Dall
2018-02-15 21:03 ` [PATCH v4 39/40] KVM: arm/arm64: Move VGIC APR save/restore to vgic put/load Christoffer Dall
2018-02-22 13:11 ` Marc Zyngier
2018-02-22 14:44 ` Christoffer Dall
2018-02-15 21:03 ` [PATCH v4 40/40] KVM: arm/arm64: Avoid VGICv3 save/restore on VHE with no IRQs Christoffer Dall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180222111005.GL29376@cbox \
--to=christoffer.dall@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).