Re: [PATCH v8 07/11] KVM: arm64: Add handlers for protected VM System Registers

[Fuad Tabba <tabba@google.com>]

Hi Marc,


On Mon, Oct 11, 2021 at 12:39 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Sun, 10 Oct 2021 15:56:32 +0100,
> Fuad Tabba <tabba@google.com> wrote:
> >
> > Add system register handlers for protected VMs. These cover Sys64
> > registers (including feature id registers), and debug.
> >
> > No functional change intended as these are not hooked in yet to
> > the guest exit handlers introduced earlier. So when trapping is
> > triggered, the exit handlers let the host handle it, as before.
> >
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > Signed-off-by: Fuad Tabba <tabba@google.com>
>
> [...]
>
> > +/*
> > + * Architected system registers.
> > + * Important: Must be sorted ascending by Op0, Op1, CRn, CRm, Op2
> > + *
> > + * NOTE: Anything not explicitly listed here is *restricted by default*, i.e.,
> > + * it will lead to injecting an exception into the guest.
> > + */
> > +static const struct sys_reg_desc pvm_sys_reg_descs[] = {
> > +     /* Cache maintenance by set/way operations are restricted. */
> > +
> > +     /* Debug and Trace Registers are restricted. */
> > +
> > +     /* AArch64 mappings of the AArch32 ID registers */
> > +     /* CRm=1 */
> > +     AARCH32(SYS_ID_PFR0_EL1),
> > +     AARCH32(SYS_ID_PFR1_EL1),
> > +     AARCH32(SYS_ID_DFR0_EL1),
> > +     AARCH32(SYS_ID_AFR0_EL1),
> > +     AARCH32(SYS_ID_MMFR0_EL1),
> > +     AARCH32(SYS_ID_MMFR1_EL1),
> > +     AARCH32(SYS_ID_MMFR2_EL1),
> > +     AARCH32(SYS_ID_MMFR3_EL1),
> > +
> > +     /* CRm=2 */
> > +     AARCH32(SYS_ID_ISAR0_EL1),
> > +     AARCH32(SYS_ID_ISAR1_EL1),
> > +     AARCH32(SYS_ID_ISAR2_EL1),
> > +     AARCH32(SYS_ID_ISAR3_EL1),
> > +     AARCH32(SYS_ID_ISAR4_EL1),
> > +     AARCH32(SYS_ID_ISAR5_EL1),
> > +     AARCH32(SYS_ID_MMFR4_EL1),
> > +     AARCH32(SYS_ID_ISAR6_EL1),
> > +
> > +     /* CRm=3 */
> > +     AARCH32(SYS_MVFR0_EL1),
> > +     AARCH32(SYS_MVFR1_EL1),
> > +     AARCH32(SYS_MVFR2_EL1),
> > +     AARCH32(SYS_ID_PFR2_EL1),
> > +     AARCH32(SYS_ID_DFR1_EL1),
> > +     AARCH32(SYS_ID_MMFR5_EL1),
> > +
> > +     /* AArch64 ID registers */
> > +     /* CRm=4 */
> > +     AARCH64(SYS_ID_AA64PFR0_EL1),
> > +     AARCH64(SYS_ID_AA64PFR1_EL1),
> > +     AARCH64(SYS_ID_AA64ZFR0_EL1),
> > +     AARCH64(SYS_ID_AA64DFR0_EL1),
> > +     AARCH64(SYS_ID_AA64DFR1_EL1),
> > +     AARCH64(SYS_ID_AA64AFR0_EL1),
> > +     AARCH64(SYS_ID_AA64AFR1_EL1),
> > +     AARCH64(SYS_ID_AA64ISAR0_EL1),
> > +     AARCH64(SYS_ID_AA64ISAR1_EL1),
> > +     AARCH64(SYS_ID_AA64MMFR0_EL1),
> > +     AARCH64(SYS_ID_AA64MMFR1_EL1),
> > +     AARCH64(SYS_ID_AA64MMFR2_EL1),
> > +
> > +     HOST_HANDLED(SYS_SCTLR_EL1),
> > +     HOST_HANDLED(SYS_ACTLR_EL1),
> > +     HOST_HANDLED(SYS_CPACR_EL1),
> > +
> > +     HOST_HANDLED(SYS_RGSR_EL1),
> > +     HOST_HANDLED(SYS_GCR_EL1),
>
> What is the expected semantics of this handling? These registers are
> free to use by the guest unless MTE is disabled. Either the guest
> accesses them directly (no trap), accesses them while MTE is disabled
> (trap), or access them when MTE doesn't exist.
>
> The first and last cases are invisible to EL2. For the second case,
> why should we go back to EL1 rather than injecting an UNDEF directly?

In hindsight, my approach to constructing this table was not the right
one. I took the one for EL1, and filtered it based on the trapping
bits. So yes, you are right that I need to re-examine this table with
that in mind.

Thanks,
/fuad

> > +
> > +     /* Scalable Vector Registers are restricted. */
> > +
> > +     HOST_HANDLED(SYS_TTBR0_EL1),
> > +     HOST_HANDLED(SYS_TTBR1_EL1),
> > +     HOST_HANDLED(SYS_TCR_EL1),
>
> None of these should normally trap unless we are handling an erratum
> (such as Cavium 219) or that we have HCR_EL2.TVM set. The former is
> handled at EL2, and I don't expect any Set/Way emulation to require
> the latter.
>
> > +
> > +     HOST_HANDLED(SYS_APIAKEYLO_EL1),
> > +     HOST_HANDLED(SYS_APIAKEYHI_EL1),
> > +     HOST_HANDLED(SYS_APIBKEYLO_EL1),
> > +     HOST_HANDLED(SYS_APIBKEYHI_EL1),
> > +     HOST_HANDLED(SYS_APDAKEYLO_EL1),
> > +     HOST_HANDLED(SYS_APDAKEYHI_EL1),
> > +     HOST_HANDLED(SYS_APDBKEYLO_EL1),
> > +     HOST_HANDLED(SYS_APDBKEYHI_EL1),
> > +     HOST_HANDLED(SYS_APGAKEYLO_EL1),
> > +     HOST_HANDLED(SYS_APGAKEYHI_EL1),
>
> This is debatable too. If the guest has started using PtrAuth and that
> we haven't handled things in fixup_guest_exit(), why returning to the
> host? This should directly UNDEF.
>
> > +
> > +     HOST_HANDLED(SYS_AFSR0_EL1),
> > +     HOST_HANDLED(SYS_AFSR1_EL1),
> > +     HOST_HANDLED(SYS_ESR_EL1),
>
> Same as TTBR*/TCR.
>
> > +
> > +     HOST_HANDLED(SYS_ERRIDR_EL1),
> > +     HOST_HANDLED(SYS_ERRSELR_EL1),
> > +     HOST_HANDLED(SYS_ERXFR_EL1),
> > +     HOST_HANDLED(SYS_ERXCTLR_EL1),
> > +     HOST_HANDLED(SYS_ERXSTATUS_EL1),
> > +     HOST_HANDLED(SYS_ERXADDR_EL1),
> > +     HOST_HANDLED(SYS_ERXMISC0_EL1),
> > +     HOST_HANDLED(SYS_ERXMISC1_EL1),
>
> This really should be handled as RAZ/WI at EL2.
>
> > +
> > +     HOST_HANDLED(SYS_TFSR_EL1),
> > +     HOST_HANDLED(SYS_TFSRE0_EL1),
>
> Same as RCSR/GSR.
>
> > +
> > +     HOST_HANDLED(SYS_FAR_EL1),
>
> Same as TTBR
>
> > +     HOST_HANDLED(SYS_PAR_EL1),
>
> Does not trap in the absence of FGT (which we don't use yet).
>
> > +
> > +     /* Performance Monitoring Registers are restricted. */
> > +
> > +     HOST_HANDLED(SYS_MAIR_EL1),
> > +     HOST_HANDLED(SYS_AMAIR_EL1),
>
> Same as TTBR.
>
> > +
> > +     /* Limited Ordering Regions Registers are restricted. */
> > +
> > +     HOST_HANDLED(SYS_VBAR_EL1),
>
> Doesn't trap in the absence of FGT.
>
> > +     HOST_HANDLED(SYS_DISR_EL1),
>
> If RAS exists, a DISR_EL1 access is routed to VDISR_EL2. If RAS isn't
> present, this UNDEFs. In any case, there is no trap.
>
> > +
> > +     /* GIC CPU Interface registers are restricted. */
>
> Err... Does this include ICC_SGI*R_EL1/ICC_SRE_EL1? Not going to work
> if you don't let EL1 dealing with this.
>
> > +
> > +     HOST_HANDLED(SYS_CONTEXTIDR_EL1),
>
> Same as TTBR.
>
> > +     HOST_HANDLED(SYS_TPIDR_EL1),
>
> Doesn't trap in the absence of FGT.
>
> > +
> > +     HOST_HANDLED(SYS_SCXTNUM_EL1),
>
> Should UNDEF at EL2 until we actually enable FEAT_CSV2_2.
>
> > +
> > +     HOST_HANDLED(SYS_CNTKCTL_EL1),
>
> Never traps.
>
> > +
> > +     HOST_HANDLED(SYS_CCSIDR_EL1),
> > +     HOST_HANDLED(SYS_CLIDR_EL1),
> > +     HOST_HANDLED(SYS_CSSELR_EL1),
> > +     HOST_HANDLED(SYS_CTR_EL0),
>
> Eventually, we should expose a synthetic version of these at EL2.
>
> > +
> > +     /* Performance Monitoring Registers are restricted. */
> > +
> > +     HOST_HANDLED(SYS_TPIDR_EL0),
> > +     HOST_HANDLED(SYS_TPIDRRO_EL0),
>
> Do not trap in the absence of FGT.
>
> > +
> > +     HOST_HANDLED(SYS_SCXTNUM_EL0),
>
> Should UNDEF at EL2 until we actually enable FEAT_CSV2_2.
>
> > +
> > +     /* Activity Monitoring Registers are restricted. */
> > +
> > +     HOST_HANDLED(SYS_CNTP_TVAL_EL0),
> > +     HOST_HANDLED(SYS_CNTP_CTL_EL0),
> > +     HOST_HANDLED(SYS_CNTP_CVAL_EL0),
> > +
> > +     /* Performance Monitoring Registers are restricted. */
> > +
> > +     HOST_HANDLED(SYS_DACR32_EL2),
> > +     HOST_HANDLED(SYS_IFSR32_EL2),
> > +     HOST_HANDLED(SYS_FPEXC32_EL2),
>
> I don't understand the presence of these registers here. As the name
> indicates, they are 32bit only.
>
> We need a complete overhaul of this table. I'm going to go through the
> rest of the patches, and we can then fix this.
>
> Thanks,
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.