Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements

From: Peter Maydell <peter.maydell@linaro.org>
To: Marc Zyngier <maz@kernel.org>
Cc: Will Deacon <will@kernel.org>,
	QEMU Developers <qemu-devel@nongnu.org>,
	kvmarm@lists.cs.columbia.edu
Subject: Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements
Date: Mon, 25 Nov 2019 16:21:54 +0000	[thread overview]
Message-ID: <CAFEAcA_MQpJ=8UFnP=Qnt+4GWMUO_AtJBBNz-bM2L2wf5htuaQ@mail.gmail.com> (raw)
In-Reply-To: <20191123115618.29230-1-maz@kernel.org>

On Sat, 23 Nov 2019 at 11:56, Marc Zyngier <maz@kernel.org> wrote:
>
> HCR_EL2.TID3 mandates that access from EL1 to a long list of id
> registers traps to EL2, and QEMU has so far ignored this requirement.
>
> This breaks (among other things) KVM guests that have PtrAuth enabled,
> while the hypervisor doesn't want to expose the feature to its guest.
> To achieve this, KVM traps the ID registers (ID_AA64ISAR1_EL1 in this
> case), and masks out the unsupported feature.
>
> QEMU not honoring the trap request means that the guest observes
> that the feature is present in the HW, starts using it, and dies
> a horrible death when KVM injects an UNDEF, because the feature
> *really* isn't supported.
>
> Do the right thing by trapping to EL2 if HCR_EL2.TID3 is set.
>
> Reported-by: Will Deacon <will@kernel.org>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> There is a number of other trap bits missing (TID[0-2], for example),
> but this at least gets a mainline Linux going with cpu=max.

I guess this ought to go into 4.2, given that it gets recent
Linux guest kernels to work.

> @@ -6185,11 +6221,13 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>              { .name = "ID_AA64PFR0_EL1", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 0,
>                .access = PL1_R, .type = ARM_CP_NO_RAW,
> +              .accessfn = access_aa64idreg,
>                .readfn = id_aa64pfr0_read,
>                .writefn = arm_cp_write_ignore },
>              { .name = "ID_AA64PFR1_EL1", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 1,
>                .access = PL1_R, .type = ARM_CP_CONST,
> +              .accessfn = access_aa64idreg,
>                .resetvalue = cpu->isar.id_aa64pfr1},
>              { .name = "ID_AA64PFR2_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 2,
> @@ -6198,151 +6236,188 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>              { .name = "ID_AA64PFR3_EL1_RESERVED", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 3,
>                .access = PL1_R, .type = ARM_CP_CONST,
> +              .accessfn = access_aa64idreg,
>                .resetvalue = 0 },

Missing .accessfn for ID_AA4PFR2_EL1_RESERVED ?

>              { .name = "ID_AA64ZFR0_EL1", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 4, .opc2 = 4,
>                .access = PL1_R, .type = ARM_CP_CONST,
> +              .accessfn = access_aa64idreg,
>                /* At present, only SVEver == 0 is defined anyway.  */
>                .resetvalue = 0 },

>              { .name = "MVFR0_EL1", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 0,
>                .access = PL1_R, .type = ARM_CP_CONST,
> +              .accessfn = access_aa64idreg,
>                .resetvalue = cpu->isar.mvfr0 },

These are the AArch64 accessors for the aarch32 MVFR registers,
but this trap bit is also supposed to trap aarch32 accesses to
the MVFR registers, which are via the vmrs insn. That needs
to be done in trans_VMSR_VMRS(); we can do that with a
followup patch, since the mechanism there is completely different.

thanks
-- PMM