Re: [PATCH v7 1/3] arm64: kvm: Save/restore MTE registers

[Marc Zyngier]

On 2021-02-04 14:33, Steven Price wrote:
> On 02/02/2021 15:36, Marc Zyngier wrote:
> > On 2021-01-15 15:28, Steven Price wrote:
> > > Define the new system registers that MTE introduces and context 
> > > switch
> > > them. The MTE feature is still hidden from the ID register as it 
> > > isn't
> > > supported in a VM yet.
> > >
> > > Signed-off-by: Steven Price <steven.price@arm.com>
> > > ---
> > >  arch/arm64/include/asm/kvm_host.h          |  4 ++
> > >  arch/arm64/include/asm/kvm_mte.h           | 74 
> > > ++++++++++++++++++++++
> > >  arch/arm64/include/asm/sysreg.h            |  3 +-
> > >  arch/arm64/kernel/asm-offsets.c            |  3 +
> > >  arch/arm64/kvm/hyp/entry.S                 |  7 ++
> > >  arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h |  4 ++
> > >  arch/arm64/kvm/sys_regs.c                  | 14 ++--
> > >  7 files changed, 104 insertions(+), 5 deletions(-)
> > >  create mode 100644 arch/arm64/include/asm/kvm_mte.h
> > >
> > > diff --git a/arch/arm64/include/asm/kvm_host.h
> > > b/arch/arm64/include/asm/kvm_host.h
> > > index 11beda85ee7e..51590a397e4b 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -148,6 +148,8 @@ enum vcpu_sysreg {
> > >      SCTLR_EL1,    /* System Control Register */
> > >      ACTLR_EL1,    /* Auxiliary Control Register */
> > >      CPACR_EL1,    /* Coprocessor Access Control */
> > > +    RGSR_EL1,    /* Random Allocation Tag Seed Register */
> > > +    GCR_EL1,    /* Tag Control Register */
> > >      ZCR_EL1,    /* SVE Control */
> > >      TTBR0_EL1,    /* Translation Table Base Register 0 */
> > >      TTBR1_EL1,    /* Translation Table Base Register 1 */
> > > @@ -164,6 +166,8 @@ enum vcpu_sysreg {
> > >      TPIDR_EL1,    /* Thread ID, Privileged */
> > >      AMAIR_EL1,    /* Aux Memory Attribute Indirection Register */
> > >      CNTKCTL_EL1,    /* Timer Control Register (EL1) */
> > > +    TFSRE0_EL1,    /* Tag Fault Status Register (EL0) */
> > > +    TFSR_EL1,    /* Tag Fault Stauts Register (EL1) */
> >
> > s/Stauts/Status/
> >
> > Is there any reason why the MTE registers aren't grouped together?
>
> I has been under the impression this list is sorted by the encoding of
> the system registers, although double checking I've screwed up the
> order of TFSRE0_EL1/TFSR_EL1, and not all the other fields are sorted
> that way.

It grew organically, and was initially matching the original order
of the save/restore sequence. This order has long disappeared with
VHE, and this is essentially nothing more than a bag of indices
(although NV does bring some order back to deal with VNCR-backed
registers).

[...]

> > > diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> > > b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> > > index cce43bfe158f..94d9736f0133 100644
> > > --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> > > +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> > > @@ -45,6 +45,8 @@ static inline void __sysreg_save_el1_state(struct
> > > kvm_cpu_context *ctxt)
> > >      ctxt_sys_reg(ctxt, CNTKCTL_EL1)    = 
> > > read_sysreg_el1(SYS_CNTKCTL);
> > >      ctxt_sys_reg(ctxt, PAR_EL1)    = read_sysreg_par();
> > >      ctxt_sys_reg(ctxt, TPIDR_EL1)    = read_sysreg(tpidr_el1);
> > > +    if (system_supports_mte())
> > > +        ctxt_sys_reg(ctxt, TFSR_EL1) = read_sysreg_el1(SYS_TFSR);
> >
> > I already asked for it, and I'm going to ask for it again:
> > Most of the sysreg save/restore is guarded by a per-vcpu check
> > (HCR_EL2.ATA), while this one is unconditionally saved/restore
> > if the host is MTE capable. Why is that so?
>
> Sorry, I thought your concern was for registers that affect the host
> (as they are obviously more performance critical as they are hit on
> every guest exit). Although I guess that's incorrect for nVHE which is
> what all the cool kids want now ;)

I think we want both correctness *and* performance, for both VHE
and nVHE. Things like EL0 registers should be able to be moved
to load/put on all implementations, and the correct switching
be done at the right spot only when required.

Thanks,

        M.
--
Jazz is not dead. It just smells funny...