From: Christoffer Dall <cdall@linaro.org>
To: Jintack Lim <jintack.lim@linaro.org>
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
Date: Mon, 31 Jul 2017 14:04:01 +0200 [thread overview]
Message-ID: <20170731120401.GS5176@cbox> (raw)
In-Reply-To: <1500397144-16232-33-git-send-email-jintack.lim@linaro.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 <jintack.lim@linaro.org>
> ---
> 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
next prev parent reply other threads:[~2017-07-31 12:04 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-18 16:58 [RFC PATCH v2 00/38] Nested Virtualization on KVM/ARM Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 01/38] arm64: Add ARM64_HAS_NESTED_VIRT feature Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 02/38] KVM: arm/arm64: Enable nested virtualization via command-line Jintack Lim
2017-07-30 19:59 ` Christoffer Dall
2017-08-01 13:56 ` Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 03/38] KVM: arm64: Add KVM nesting feature Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 04/38] KVM: arm/arm64: Check if nested virtualization is in use Jintack Lim
2017-07-30 19:59 ` Christoffer Dall
2017-08-01 13:59 ` Jintack Lim
2017-07-30 19:59 ` Christoffer Dall
2017-08-01 14:07 ` Jintack Lim
2017-08-01 14:58 ` Christoffer Dall
2017-07-18 16:58 ` [RFC PATCH v2 05/38] KVM: arm64: Allow userspace to set PSR_MODE_EL2x Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 06/38] KVM: arm64: Add vcpu_mode_el2 primitive to support nesting Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 07/38] KVM: arm64: Add EL2 system registers to vcpu context Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 08/38] KVM: arm64: Add EL2 special " Jintack Lim
2017-07-30 19:59 ` Christoffer Dall
2017-08-01 14:08 ` Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 09/38] KVM: arm64: Add the shadow context for virtual EL2 execution Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 10/38] KVM: arm/arm64: Add a framework to prepare " Jintack Lim
2017-07-30 12:02 ` Christoffer Dall
2017-07-18 16:58 ` [RFC PATCH v2 11/38] KVM: arm64: Set vcpu context depending on the guest exception level Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 12/38] arm64: Add missing TCR hw defines Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 13/38] KVM: arm64: Create shadow EL1 registers Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 14/38] KVM: arm64: Synchronize EL1 system registers on virtual EL2 entry and exit Jintack Lim
2017-07-30 20:00 ` Christoffer Dall
2017-07-18 16:58 ` [RFC PATCH v2 15/38] KVM: arm64: Move exception macros and enums to a common file Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 16/38] KVM: arm64: Support to inject exceptions to the virtual EL2 Jintack Lim
2017-07-30 20:00 ` Christoffer Dall
2017-07-18 16:58 ` [RFC PATCH v2 17/38] KVM: arm64: Trap EL1 VM register accesses in " Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 18/38] KVM: arm64: Trap SPSR_EL1, ELR_EL1 and VBAR_EL1 from " Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 19/38] KVM: arm64: Trap CPACR_EL1 access in " Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 20/38] KVM: arm64: Handle eret instruction traps Jintack Lim
2017-07-30 20:00 ` Christoffer Dall
2017-08-01 14:11 ` Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 21/38] KVM: arm64: Set a handler for the system " Jintack Lim
2017-07-30 20:00 ` Christoffer Dall
2017-07-18 16:58 ` [RFC PATCH v2 22/38] KVM: arm64: Handle PSCI call via smc from the guest Jintack Lim
2017-07-30 20:00 ` Christoffer Dall
2017-07-18 16:58 ` [RFC PATCH v2 23/38] KVM: arm64: Inject HVC exceptions to the virtual EL2 Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 24/38] KVM: arm64: Respect virtual HCR_EL2.TWX setting Jintack Lim
2017-07-30 20:00 ` Christoffer Dall
2017-07-18 16:58 ` [RFC PATCH v2 25/38] KVM: arm64: Respect virtual CPTR_EL2.TFP setting Jintack Lim
2017-07-30 20:00 ` Christoffer Dall
2017-07-18 16:58 ` [RFC PATCH v2 26/38] KVM: arm64: Add macros to support the virtual EL2 with VHE Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 27/38] KVM: arm64: Add EL2 registers defined in ARMv8.1 to vcpu context Jintack Lim
2017-07-18 16:58 ` [RFC PATCH v2 28/38] KVM: arm64: Emulate EL12 register accesses from the virtual EL2 Jintack Lim
2017-07-31 8:44 ` Christoffer Dall
2017-07-18 16:58 ` [RFC PATCH v2 29/38] KVM: arm64: Support a VM with VHE considering EL0 of the VHE host Jintack Lim
2017-07-31 9:01 ` Christoffer Dall
2017-07-18 16:58 ` [RFC PATCH v2 30/38] KVM: arm64: Allow the virtual EL2 to access EL2 states without trap Jintack Lim
2017-07-31 9:37 ` Christoffer Dall
2017-07-18 16:58 ` [RFC PATCH v2 31/38] KVM: arm64: Manage the shadow states when virtual E2H bit enabled Jintack Lim
2017-07-31 9:57 ` Christoffer Dall
2017-07-18 16:58 ` [RFC PATCH v2 32/38] KVM: arm64: Trap and emulate CPTR_EL2 accesses via CPACR_EL1 from the virtual EL2 with VHE Jintack Lim
2017-07-31 12:04 ` Christoffer Dall [this message]
2017-07-18 16:58 ` [RFC PATCH v2 33/38] KVM: arm64: Emulate appropriate VM control system registers Jintack Lim
2017-07-31 12:09 ` Christoffer Dall
2017-07-18 16:59 ` [RFC PATCH v2 34/38] KVM: arm64: Respect the virtual HCR_EL2.NV bit setting Jintack Lim
2017-07-18 16:59 ` [RFC PATCH v2 35/38] KVM: arm64: Respect the virtual HCR_EL2.NV bit setting for EL12 register traps Jintack Lim
2017-07-31 12:39 ` Christoffer Dall
2017-07-18 16:59 ` [RFC PATCH v2 36/38] KVM: arm64: Respect virtual HCR_EL2.TVM and TRVM settings Jintack Lim
2017-07-31 12:42 ` Christoffer Dall
2017-07-18 16:59 ` [RFC PATCH v2 37/38] KVM: arm64: Respect the virtual HCR_EL2.NV1 bit setting Jintack Lim
2017-07-19 2:24 ` Jintack Lim
2017-07-31 12:53 ` Christoffer Dall
2017-07-18 16:59 ` [RFC PATCH v2 38/38] KVM: arm64: Respect the virtual CPTR_EL2.TCPAC setting Jintack Lim
2017-07-31 12:59 ` Christoffer Dall
2017-08-01 11:03 ` Jintack Lim
2017-08-01 11:20 ` Christoffer Dall
2017-07-19 2:23 ` [RFC PATCH v2 00/38] Nested Virtualization on KVM/ARM Jintack Lim
2017-07-19 8:49 ` Christoffer Dall
2017-07-19 14:35 ` Jintack Lim
2017-07-28 20:13 ` Bandan Das
2017-07-28 21:45 ` Jintack Lim
2017-07-31 13:00 ` Christoffer Dall
2017-08-01 10:48 ` Jintack Lim
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=20170731120401.GS5176@cbox \
--to=cdall@linaro.org \
--cc=akpm@linux-foundation.org \
--cc=andy.gross@linaro.org \
--cc=ard.biesheuvel@linaro.org \
--cc=catalin.marinas@arm.com \
--cc=christoffer.dall@linaro.org \
--cc=corbet@lwn.net \
--cc=cov@codeaurora.org \
--cc=daniel.lezcano@linaro.org \
--cc=david.daney@cavium.com \
--cc=james.morse@arm.com \
--cc=jintack.lim@linaro.org \
--cc=kvm@vger.kernel.org \
--cc=kvmarm@lists.cs.columbia.edu \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=marc.zyngier@arm.com \
--cc=mark.rutland@arm.com \
--cc=mchehab@kernel.org \
--cc=pbonzini@redhat.com \
--cc=rkrcmar@redhat.com \
--cc=shankerd@codeaurora.org \
--cc=stefan@hello-penguin.com \
--cc=suzuki.poulose@arm.com \
--cc=vladimir.murzin@arm.com \
--cc=wcohen@redhat.com \
--cc=will.deacon@arm.com \
/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).