Re: [PATCH] arm: Don't remove EL3 exposure for SMC conduit

[Alexander Graf]

On 14.11.21 18:41, Alexander Graf wrote:
> > Am 14.11.2021 um 18:20 schrieb Peter Maydell <peter.maydell@linaro.org>:
> >
> > On Sun, 14 Nov 2021 at 10:56, Alexander Graf <agraf@csgraf.de> wrote:
> > > When we expose an SMC conduit, we're implicitly telling the guest that
> > > there is EL3 available because it needs to call it. While that EL3 then
> > > is not backed by the emulated CPU, from the guest's EL2 point of view,
> > > it still means there is an EL3 to call into.
> > >
> > > This is a problem for VMware ESXi, which validates EL3 availability before
> > > doing SMC calls. With this patch, VMware ESXi works with SMP in TCG.
> > >
> > > Reported-by: Andrei Warkentin <andrey.warkentin@gmail.com>
> > > Signed-off-by: Alexander Graf <agraf@csgraf.de>
> > > ---
> > > target/arm/cpu.c | 20 +++++++++++++++-----
> > > 1 file changed, 15 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> > > index a211804fd3..21092c5242 100644
> > > --- a/target/arm/cpu.c
> > > +++ b/target/arm/cpu.c
> > > @@ -1782,11 +1782,21 @@ static void arm_cpu_realizefn(DeviceState *dev, Error 
> > > **errp)
> > >           */
> > >          unset_feature(env, ARM_FEATURE_EL3);
> > >
> > > -        /* Disable the security extension feature bits in the processor feature
> > > -         * registers as well. These are id_pfr1[7:4] and id_aa64pfr0[15:12].
> > > -         */
> > > -        cpu->isar.id_pfr1 &= ~0xf0;
> > > -        cpu->isar.id_aa64pfr0 &= ~0xf000;
> > > +        if (cpu->psci_conduit == QEMU_PSCI_CONDUIT_SMC) {
> > > +            /*
> > > +             * We tell the guest to use SMC calls into EL3 for PSCI calls, so
> > > +             * there has to be EL3 available. We merely execute it on the host
> > > +             * in QEMU rather than in actual EL3 inside the guest.
> > > +             */
> > > +        } else {
> > > +            /*
> > > +             * Disable the security extension feature bits in the processor
> > > +             * feature registers as well. These are id_pfr1[7:4] and
> > > +             * id_aa64pfr0[15:12].
> > > +             */
> > > +            cpu->isar.id_pfr1 &= ~0xf0;
> > > +            cpu->isar.id_aa64pfr0 &= ~0xf000;
> > > +        }
> > This is tricky, because we use the cpu->isar values to determine whether
> > we should be emulating things. So this change means we now create an
> > inconsistent CPU which in some ways claims to have EL3 (the ISAR ID
> > bits say so) and in some ways does not (the ARM_FEATURE_EL3 flag is
> > unset), and depending on which of the two "do we have EL3?" methods
> > any bit of the TCG code is using will give different results...
> Do you think it would be sufficient to go through all readers of the isar bits 
> and guard them behind an ARM_FEATURE_EL3 check in addition? I'll be happy to do 
> so then! :)


The aa32 pfr1 seems to have a single consumer:

static inline bool isar_feature_aa32_m_sec_state(const ARMISARegisters *id)
{
    /*
     * Return true if M-profile state handling insns
     * (VSCCLRM, CLRM, FPCTX access insns) are implemented
     */
    return FIELD_EX32(id->id_pfr1, ID_PFR1, SECURITY) >= 3;
}

which is only called for M profile. For M profile however, the patch is 
irrelevant as it's patching inside a !arm_feature(env, ARM_FEATURE_M) 
branch.

The only consumer of the AA64PFR.EL3 field I could find was in KVM code 
when assembling -cpu host. It again is unaffected by this patch.


Alex