lkml.org 
[lkml]   [2017]   [Jul]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RFC PATCH v2 32/38] KVM: arm64: Trap and emulate CPTR_EL2 accesses via CPACR_EL1 from the virtual EL2 with VHE
    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
    \
     
     \ /
      Last update: 2017-07-31 14:04    [W:5.689 / U:0.124 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site