Re: [PATCH v3 06/15] KVM: arm64: Restore mdcr_el2 from vcpu

[Fuad Tabba <tabba@google.com>]

Hi Drew,

On Tue, Jul 20, 2021 at 3:53 PM Andrew Jones <drjones@redhat.com> wrote:
>
> On Mon, Jul 19, 2021 at 05:03:37PM +0100, Fuad Tabba wrote:
> > On deactivating traps, restore the value of mdcr_el2 from the
> > newly created and preserved host value vcpu context, rather than
> > directly reading the hardware register.
> >
> > Up until and including this patch the two values are the same,
> > i.e., the hardware register and the vcpu one. A future patch will
> > be changing the value of mdcr_el2 on activating traps, and this
> > ensures that its value will be restored.
> >
> > No functional change intended.
>
> I'm probably missing something, but I can't convince myself that the host
> will end up with the same mdcr_el2 value after deactivating traps after
> this patch as before. We clearly now restore whatever we had when
> activating traps (presumably whatever we configured at init_el2_state
> time), but is that equivalent to what we had before with the masking and
> ORing that this patch drops?

You're right. I thought that these were actually being initialized to
the same values, but having a closer look at the code the mdcr values
are not the same as pre-patch. I will fix this.

Thanks!
/fuad

> Thanks,
> drew
>
> >
> > Signed-off-by: Fuad Tabba <tabba@google.com>
> > ---
> >  arch/arm64/include/asm/kvm_host.h       |  5 ++++-
> >  arch/arm64/include/asm/kvm_hyp.h        |  2 +-
> >  arch/arm64/kvm/hyp/include/hyp/switch.h |  6 +++++-
> >  arch/arm64/kvm/hyp/nvhe/switch.c        | 11 ++---------
> >  arch/arm64/kvm/hyp/vhe/switch.c         | 12 ++----------
> >  arch/arm64/kvm/hyp/vhe/sysreg-sr.c      |  2 +-
> >  6 files changed, 15 insertions(+), 23 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index 4d2d974c1522..76462c6a91ee 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -287,10 +287,13 @@ struct kvm_vcpu_arch {
> >       /* Stage 2 paging state used by the hardware on next switch */
> >       struct kvm_s2_mmu *hw_mmu;
> >
> > -     /* HYP configuration */
> > +     /* Values of trap registers for the guest. */
> >       u64 hcr_el2;
> >       u64 mdcr_el2;
> >
> > +     /* Values of trap registers for the host before guest entry. */
> > +     u64 mdcr_el2_host;
> > +
> >       /* Exception Information */
> >       struct kvm_vcpu_fault_info fault;
> >
> > diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> > index 9d60b3006efc..657d0c94cf82 100644
> > --- a/arch/arm64/include/asm/kvm_hyp.h
> > +++ b/arch/arm64/include/asm/kvm_hyp.h
> > @@ -95,7 +95,7 @@ void __sve_restore_state(void *sve_pffr, u32 *fpsr);
> >
> >  #ifndef __KVM_NVHE_HYPERVISOR__
> >  void activate_traps_vhe_load(struct kvm_vcpu *vcpu);
> > -void deactivate_traps_vhe_put(void);
> > +void deactivate_traps_vhe_put(struct kvm_vcpu *vcpu);
> >  #endif
> >
> >  u64 __guest_enter(struct kvm_vcpu *vcpu);
> > diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> > index e4a2f295a394..a0e78a6027be 100644
> > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> > @@ -92,11 +92,15 @@ static inline void __activate_traps_common(struct kvm_vcpu *vcpu)
> >               write_sysreg(0, pmselr_el0);
> >               write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0);
> >       }
> > +
> > +     vcpu->arch.mdcr_el2_host = read_sysreg(mdcr_el2);
> >       write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
> >  }
> >
> > -static inline void __deactivate_traps_common(void)
> > +static inline void __deactivate_traps_common(struct kvm_vcpu *vcpu)
> >  {
> > +     write_sysreg(vcpu->arch.mdcr_el2_host, mdcr_el2);
> > +
> >       write_sysreg(0, hstr_el2);
> >       if (kvm_arm_support_pmu_v3())
> >               write_sysreg(0, pmuserenr_el0);
> > diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
> > index f7af9688c1f7..1778593a08a9 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/switch.c
> > +++ b/arch/arm64/kvm/hyp/nvhe/switch.c
> > @@ -69,12 +69,10 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
> >  static void __deactivate_traps(struct kvm_vcpu *vcpu)
> >  {
> >       extern char __kvm_hyp_host_vector[];
> > -     u64 mdcr_el2, cptr;
> > +     u64 cptr;
> >
> >       ___deactivate_traps(vcpu);
> >
> > -     mdcr_el2 = read_sysreg(mdcr_el2);
> > -
> >       if (cpus_have_final_cap(ARM64_WORKAROUND_SPECULATIVE_AT)) {
> >               u64 val;
> >
> > @@ -92,13 +90,8 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu)
> >               isb();
> >       }
> >
> > -     __deactivate_traps_common();
> > -
> > -     mdcr_el2 &= MDCR_EL2_HPMN_MASK;
> > -     mdcr_el2 |= MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT;
> > -     mdcr_el2 |= MDCR_EL2_E2TB_MASK << MDCR_EL2_E2TB_SHIFT;
> > +     __deactivate_traps_common(vcpu);
> >
> > -     write_sysreg(mdcr_el2, mdcr_el2);
> >       write_sysreg(this_cpu_ptr(&kvm_init_params)->hcr_el2, hcr_el2);
> >
> >       cptr = CPTR_EL2_DEFAULT;
> > diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
> > index b3229924d243..0d0c9550fb08 100644
> > --- a/arch/arm64/kvm/hyp/vhe/switch.c
> > +++ b/arch/arm64/kvm/hyp/vhe/switch.c
> > @@ -91,17 +91,9 @@ void activate_traps_vhe_load(struct kvm_vcpu *vcpu)
> >       __activate_traps_common(vcpu);
> >  }
> >
> > -void deactivate_traps_vhe_put(void)
> > +void deactivate_traps_vhe_put(struct kvm_vcpu *vcpu)
> >  {
> > -     u64 mdcr_el2 = read_sysreg(mdcr_el2);
> > -
> > -     mdcr_el2 &= MDCR_EL2_HPMN_MASK |
> > -                 MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT |
> > -                 MDCR_EL2_TPMS;
> > -
> > -     write_sysreg(mdcr_el2, mdcr_el2);
> > -
> > -     __deactivate_traps_common();
> > +     __deactivate_traps_common(vcpu);
> >  }
> >
> >  /* Switch to the guest for VHE systems running in EL2 */
> > diff --git a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> > index 2a0b8c88d74f..007a12dd4351 100644
> > --- a/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> > +++ b/arch/arm64/kvm/hyp/vhe/sysreg-sr.c
> > @@ -101,7 +101,7 @@ void kvm_vcpu_put_sysregs_vhe(struct kvm_vcpu *vcpu)
> >       struct kvm_cpu_context *host_ctxt;
> >
> >       host_ctxt = &this_cpu_ptr(&kvm_host_data)->host_ctxt;
> > -     deactivate_traps_vhe_put();
> > +     deactivate_traps_vhe_put(vcpu);
> >
> >       __sysreg_save_el1_state(guest_ctxt);
> >       __sysreg_save_user_state(guest_ctxt);
> > --
> > 2.32.0.402.g57bb445576-goog
> >
>