From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752104AbdGaMEH (ORCPT ); Mon, 31 Jul 2017 08:04:07 -0400 Received: from mail-wm0-f51.google.com ([74.125.82.51]:35242 "EHLO mail-wm0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751950AbdGaMEF (ORCPT ); Mon, 31 Jul 2017 08:04:05 -0400 Date: Mon, 31 Jul 2017 14:04:01 +0200 From: Christoffer Dall To: Jintack Lim Cc: kvmarm@lists.cs.columbia.edu, christoffer.dall@linaro.org, marc.zyngier@arm.com, corbet@lwn.net, pbonzini@redhat.com, rkrcmar@redhat.com, linux@armlinux.org.uk, catalin.marinas@arm.com, will.deacon@arm.com, akpm@linux-foundation.org, mchehab@kernel.org, cov@codeaurora.org, daniel.lezcano@linaro.org, david.daney@cavium.com, mark.rutland@arm.com, suzuki.poulose@arm.com, stefan@hello-penguin.com, andy.gross@linaro.org, wcohen@redhat.com, ard.biesheuvel@linaro.org, shankerd@codeaurora.org, vladimir.murzin@arm.com, james.morse@arm.com, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [RFC PATCH v2 32/38] KVM: arm64: Trap and emulate CPTR_EL2 accesses via CPACR_EL1 from the virtual EL2 with VHE Message-ID: <20170731120401.GS5176@cbox> References: <1500397144-16232-1-git-send-email-jintack.lim@linaro.org> <1500397144-16232-33-git-send-email-jintack.lim@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1500397144-16232-33-git-send-email-jintack.lim@linaro.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jul 18, 2017 at 11:58:58AM -0500, Jintack Lim wrote: > While the EL1 virtual memory control registers can be accessed in the > virtual EL2 with VHE without trap to manuplate the virtual EL2 states, > we can't do that for CPTR_EL2 for an unfortunate reason. > > This is because the top bit of CPTR_EL2, which is TCPAC, will be ignored > if it is accessed via CPACR_EL1 in the virtual EL2 without trap since > the top bot of cpacr_el1 is RES0. Therefore we need to trap CPACR_EL1 top bit ? > accesses from the virtual EL2 to emulate this bit correctly. > > Set CPTR_EL2.TCPAC bit to trap CPACR_EL1 accesses and handle them in the > existing handler considering that they could be meant to access CPTR_EL2 > instead in the virtual EL2 with VHE. > > Note that CPTR_EL2 format depends on HCR_EL2.E2H bit. We always keep it > in v8.0 format for the convenience. Otherwise, we need to check E2H bit > and use different bit masks in the entry.S, and we also check E2H bit in > all places we access virtual CPTR_EL2. The downside of using v8.0 format > is to convert the format when copying states between CPTR_EL2 and > CPACR_EL1 to support the virtual EL2 with VHE. The decision is subject > to change depending on the future discussion. I would remove the last sentence here for the actual commit message, that is already implied by sending these patches for review. > > Signed-off-by: Jintack Lim > --- > arch/arm64/include/asm/kvm_emulate.h | 2 ++ > arch/arm64/kvm/context.c | 29 ++++++++++++++++++++++++++--- > arch/arm64/kvm/hyp/switch.c | 2 ++ > arch/arm64/kvm/sys_regs.c | 18 +++++++++++++++++- > 4 files changed, 47 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index 68aafbd..4776bfc 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -59,6 +59,8 @@ enum exception_type { > void kvm_arm_setup_shadow_state(struct kvm_vcpu *vcpu); > void kvm_arm_restore_shadow_state(struct kvm_vcpu *vcpu); > void kvm_arm_init_cpu_context(kvm_cpu_context_t *cpu_ctxt); > +u64 cptr_to_cpacr(u64 cptr_el2); > +u64 cpacr_to_cptr(u64 cpacr_el1); > > static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu) > { > diff --git a/arch/arm64/kvm/context.c b/arch/arm64/kvm/context.c > index 9947bc8..a7811e1 100644 > --- a/arch/arm64/kvm/context.c > +++ b/arch/arm64/kvm/context.c > @@ -66,7 +66,7 @@ static inline u64 tcr_el2_ips_to_tcr_el1_ps(u64 tcr_el2) > << TCR_IPS_SHIFT; > } > > -static inline u64 cptr_to_cpacr(u64 cptr_el2) > +u64 cptr_to_cpacr(u64 cptr_el2) > { > u64 cpacr_el1 = 0; > > @@ -78,6 +78,21 @@ static inline u64 cptr_to_cpacr(u64 cptr_el2) > return cpacr_el1; > } > > +u64 cpacr_to_cptr(u64 cpacr_el1) > +{ > + u64 cptr_el2; > + > + cptr_el2 = CPTR_EL2_DEFAULT; > + if (!(cpacr_el1 & CPACR_EL1_FPEN)) > + cptr_el2 |= CPTR_EL2_TFP; > + if (cpacr_el1 & CPACR_EL1_TTA) > + cptr_el2 |= CPTR_EL2_TTA; > + if (cpacr_el1 & CPTR_EL2_TCPAC) > + cptr_el2 |= CPTR_EL2_TCPAC; > + > + return cptr_el2; > +} > + > static void sync_shadow_el1_sysregs(struct kvm_vcpu *vcpu) > { > u64 *s_sys_regs = vcpu->arch.ctxt.shadow_sys_regs; > @@ -93,8 +108,12 @@ static void sync_shadow_el1_sysregs(struct kvm_vcpu *vcpu) > > for (i = 0; i < ARRAY_SIZE(vhe_map); i++) { > const struct el1_el2_map *map = &vhe_map[i]; > + u64 *el2_reg = &vcpu_sys_reg(vcpu, map->el2); > > - vcpu_sys_reg(vcpu, map->el2) = s_sys_regs[map->el1]; > + /* We do trap-and-emulate CPACR_EL1 accesses. So, don't sync */ > + if (map->el2 == CPTR_EL2) > + continue; > + *el2_reg = s_sys_regs[map->el1]; > } > } > > @@ -138,8 +157,12 @@ static void flush_shadow_el1_sysregs_vhe(struct kvm_vcpu *vcpu) > */ > for (i = 0; i < ARRAY_SIZE(vhe_map); i++) { > const struct el1_el2_map *map = &vhe_map[i]; > + u64 *el1_reg = &s_sys_regs[map->el1]; > > - s_sys_regs[map->el1] = vcpu_sys_reg(vcpu, map->el2); > + if (map->el2 == CPTR_EL2) > + *el1_reg = cptr_to_cpacr(vcpu_sys_reg(vcpu, map->el2)); > + else > + *el1_reg = vcpu_sys_reg(vcpu, map->el2); nit: you could add a translation function to the map array and call that if it's set, otherwise default to copying values as they are, something like: if (map->translate) *el1_reg = map->translate(vcpu_sys_reg(vcpu, map->el2)); else *el1_reg = vcpu_sys_reg(vcpu, map->el2); > } > } > > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index fffd0c7..50c90f2 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -50,6 +50,8 @@ static void __hyp_text __activate_traps_vhe(struct kvm_vcpu *vcpu) > val = read_sysreg(cpacr_el1); > val |= CPACR_EL1_TTA; > val &= ~CPACR_EL1_FPEN; > + if (is_hyp_ctxt(vcpu)) > + val |= CPTR_EL2_TCPAC; also, I think we'll forget why this gets set for hyp context here, so a short comment would be nice. what if the guest hypervisor has set CPTR_EL2.TCPAC and runs a VM don't we also need to set the CPTR_EL2.TCPAC in the hardware and forward the exception to the VM in that case? > write_sysreg(val, cpacr_el1); > > write_sysreg(__kvm_hyp_vector, vbar_el1); > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 2aa922c..79980be 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -972,7 +972,23 @@ static bool access_cpacr(struct kvm_vcpu *vcpu, > struct sys_reg_params *p, > const struct sys_reg_desc *r) > { > - access_rw(p, &vcpu_sys_reg(vcpu, r->reg)); > + u64 reg = sys_reg(p->Op0, p->Op1, p->CRn, p->CRm, p->Op2); > + > + /* > + * When the virtual HCR_EL2.E2H == 1, an access to CPACR_EL1 > + * in the virtual EL2 is to access CPTR_EL2. > + */ > + if (vcpu_el2_e2h_is_set(vcpu) && (reg == SYS_CPACR_EL1)) { you don't check here if we're in virtual el2 mode, because you rely on only ever getting here if we had is_hyp_ctxt() when entering the VM, right? > + u64 *sysreg = &vcpu_sys_reg(vcpu, CPTR_EL2); > + > + /* We keep the value in ARMv8.0 CPTR_EL2 format. */ > + if (!p->is_write) > + p->regval = cptr_to_cpacr(*sysreg); > + else > + *sysreg = cpacr_to_cptr(p->regval); > + } else /* CPACR_EL1 access with E2H == 0 or CPACR_EL12 access */ > + access_rw(p, &vcpu_sys_reg(vcpu, r->reg)); > + again, I think you can improve your commenting style to make it clear which comment belongs to which block and only put a comment above the entire if-statement if it applies to the logic as a whole. the coding style also prefers that you use braces in both branches if only one of the branches is a single statement. > return true; > } > > -- > 1.9.1 > Thanks, -Christoffer