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

[Alexander Graf <agraf@csgraf.de>]

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