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

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

[Alexander Graf]

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;
+        }
     }
 
     if (!cpu->has_el2) {
-- 
2.30.1 (Apple Git-130)

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

[Peter Maydell]

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...

-- PMM

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

[Alexander Graf]

> 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! :)

Alex

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

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

[Peter Maydell]

On Sun, 14 Nov 2021 at 17:41, Alexander Graf <agraf@csgraf.de> wrote:
>
>
>
> > Am 14.11.2021 um 18:20 schrieb Peter Maydell <peter.maydell@linaro.org>:
> > 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! :)

That would be a big reverse-course on a design choice we made that
the preference is to look at the ID registers and phase out the
use of ARM_FEATURE bits where possible.

-- PMM

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

[Alexander Graf]

On 15.11.21 11:46, Peter Maydell wrote:
> On Sun, 14 Nov 2021 at 17:41, Alexander Graf <agraf@csgraf.de> wrote:
>>
>>
>>> Am 14.11.2021 um 18:20 schrieb Peter Maydell <peter.maydell@linaro.org>:
>>> 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! :)
> That would be a big reverse-course on a design choice we made that
> the preference is to look at the ID registers and phase out the
> use of ARM_FEATURE bits where possible.


I'm open to alternatives. As it stands, we're lying to the guest because 
we tell it "SMC is not available" but ask it to call SMC for PSCI, which 
is bad too.


Alex

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

[Alex Bennée]

Alexander Graf <agraf@csgraf.de> writes:

> On 15.11.21 11:46, Peter Maydell wrote:
>> On Sun, 14 Nov 2021 at 17:41, Alexander Graf <agraf@csgraf.de> wrote:
>>>
>>>
>>>> Am 14.11.2021 um 18:20 schrieb Peter Maydell <peter.maydell@linaro.org>:
>>>> 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! :)
>> That would be a big reverse-course on a design choice we made that
>> the preference is to look at the ID registers and phase out the
>> use of ARM_FEATURE bits where possible.
>
>
> I'm open to alternatives. As it stands, we're lying to the guest
> because we tell it "SMC is not available" but ask it to call SMC for
> PSCI, which is bad too.

Is testing the ISAR bits actually telling a guest that SMC exists or
just the CPU is capable of handling it? I guess -kernel only is a weird
case because otherwise if EL3 is available some sort of firmware has to
have gotten the CPU into a state a kernel can boot. It doesn't imply
that firmware knows how to do a PSCI call though - surely there is some
firmware configuration/probing mechanism you need to rely on for that?

-- 
Alex Bennée

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

[Alexander Graf]

On 15.11.21 13:08, Alex Bennée wrote:
> Alexander Graf <agraf@csgraf.de> writes:
>
>> On 15.11.21 11:46, Peter Maydell wrote:
>>> On Sun, 14 Nov 2021 at 17:41, Alexander Graf <agraf@csgraf.de> wrote:
>>>>
>>>>> Am 14.11.2021 um 18:20 schrieb Peter Maydell <peter.maydell@linaro.org>:
>>>>> 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! :)
>>> That would be a big reverse-course on a design choice we made that
>>> the preference is to look at the ID registers and phase out the
>>> use of ARM_FEATURE bits where possible.
>>
>> I'm open to alternatives. As it stands, we're lying to the guest
>> because we tell it "SMC is not available" but ask it to call SMC for
>> PSCI, which is bad too.
> Is testing the ISAR bits actually telling a guest that SMC exists or
> just the CPU is capable of handling it? I guess -kernel only is a weird


The way I understand it, it tells you whether SMC is a #UD or not. 
Whether that SMC call goes into happy PSCI land or into nirvana is a 
different question - but there's nothing you can do from EL<3 to figure 
that out I believe?


> case because otherwise if EL3 is available some sort of firmware has to
> have gotten the CPU into a state a kernel can boot. It doesn't imply
> that firmware knows how to do a PSCI call though - surely there is some
> firmware configuration/probing mechanism you need to rely on for that?


Oh, absolutely! As an OS, you should look up in ACPI or DT whether to do 
an SMC call for PSCI operations or not.

The problem here is that we tell the OS in ACPI to do an SMC call, yet 
we tell it in AA64PFR0 that SMC is not implemented and thus would 
trigger a #UD.


Alex