[RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled

[RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled

[Heyi Guo]

vms->psci_conduit being disabled only means PSCI is not implemented by
qemu; it doesn't mean PSCI is not supported on this virtual machine.
Actually vms->psci_conduit is set to disabled when vms->secure and
firmware_loaded are both set, which means we will run ARM trusted
firmware, which will definitely provide PSCI.

The issue can be reproduced when running qemu in TCG mode with secure
enabled, while using ARM trusted firmware + qemu virt UEFI as firmware
binaries, and we can see secondary cores will not be waken up.

Signed-off-by: Heyi Guo <guoheyi@linux.alibaba.com>

---
Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
---
 hw/arm/virt-acpi-build.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 1384a2c..7622b97 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -728,13 +728,16 @@ static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
     };
 
     switch (vms->psci_conduit) {
-    case QEMU_PSCI_CONDUIT_DISABLED:
-        fadt.arm_boot_arch = 0;
-        break;
     case QEMU_PSCI_CONDUIT_HVC:
         fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT |
                              ACPI_FADT_ARM_PSCI_USE_HVC;
         break;
+    /*
+     * QEMU_PSCI_CONDUIT_DISABLED only means PSCI is not implemented by qemu,
+     * but typically it will still be provided by secure firmware, and it should
+     * use SMC as PSCI conduit.
+     */
+    case QEMU_PSCI_CONDUIT_DISABLED:
     case QEMU_PSCI_CONDUIT_SMC:
         fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT;
         break;
-- 
2.7.4

Re: [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled

[Peter Maydell]

On Fri, 3 Jul 2020 at 10:44, Heyi Guo <guoheyi@linux.alibaba.com> wrote:
>
> vms->psci_conduit being disabled only means PSCI is not implemented by
> qemu; it doesn't mean PSCI is not supported on this virtual machine.
> Actually vms->psci_conduit is set to disabled when vms->secure and
> firmware_loaded are both set, which means we will run ARM trusted
> firmware, which will definitely provide PSCI.
>
> The issue can be reproduced when running qemu in TCG mode with secure
> enabled, while using ARM trusted firmware + qemu virt UEFI as firmware
> binaries, and we can see secondary cores will not be waken up.

If you're using a real EL3 guest firmware then it's the job of
the guest firmware to provide a DTB to the guest EL2/EL1 that says
"and I support PSCI" if it supports PSCI, surely? QEMU can't tell
whether the EL3 code does or doesn't do that...

thanks
-- PMM

Re: [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled

[Heyi Guo]

在 2020/7/3 下午6:37, Peter Maydell 写道:
> On Fri, 3 Jul 2020 at 10:44, Heyi Guo <guoheyi@linux.alibaba.com> wrote:
>> vms->psci_conduit being disabled only means PSCI is not implemented by
>> qemu; it doesn't mean PSCI is not supported on this virtual machine.
>> Actually vms->psci_conduit is set to disabled when vms->secure and
>> firmware_loaded are both set, which means we will run ARM trusted
>> firmware, which will definitely provide PSCI.
>>
>> The issue can be reproduced when running qemu in TCG mode with secure
>> enabled, while using ARM trusted firmware + qemu virt UEFI as firmware
>> binaries, and we can see secondary cores will not be waken up.
> If you're using a real EL3 guest firmware then it's the job of
> the guest firmware to provide a DTB to the guest EL2/EL1 that says
> "and I support PSCI" if it supports PSCI, surely? QEMU can't tell
> whether the EL3 code does or doesn't do that...

Thanks, Peter. Does that mean the ACPI tables generated in qemu are only 
templates and firmware should update them if necessary?

Heyi

>
> thanks
> -- PMM

Re: [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled

[Peter Maydell]

On Fri, 3 Jul 2020 at 15:36, Heyi Guo <guoheyi@linux.alibaba.com> wrote:
>
>
> 在 2020/7/3 下午6:37, Peter Maydell 写道:
> > On Fri, 3 Jul 2020 at 10:44, Heyi Guo <guoheyi@linux.alibaba.com> wrote:
> >> vms->psci_conduit being disabled only means PSCI is not implemented by
> >> qemu; it doesn't mean PSCI is not supported on this virtual machine.
> >> Actually vms->psci_conduit is set to disabled when vms->secure and
> >> firmware_loaded are both set, which means we will run ARM trusted
> >> firmware, which will definitely provide PSCI.
> >>
> >> The issue can be reproduced when running qemu in TCG mode with secure
> >> enabled, while using ARM trusted firmware + qemu virt UEFI as firmware
> >> binaries, and we can see secondary cores will not be waken up.
> > If you're using a real EL3 guest firmware then it's the job of
> > the guest firmware to provide a DTB to the guest EL2/EL1 that says
> > "and I support PSCI" if it supports PSCI, surely? QEMU can't tell
> > whether the EL3 code does or doesn't do that...
>
> Thanks, Peter. Does that mean the ACPI tables generated in qemu are only
> templates and firmware should update them if necessary?

I don't really know enough about ACPI to say. I hadn't noticed
that this patch only updated the ACPI tables, sorry. Perhaps it
is correct; Andrew will probably know better than me.

thanks
-- PMM

Re: [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled

[Michael S. Tsirkin]

On Fri, Jul 03, 2020 at 11:37:02AM +0100, Peter Maydell wrote:
> On Fri, 3 Jul 2020 at 10:44, Heyi Guo <guoheyi@linux.alibaba.com> wrote:
> >
> > vms->psci_conduit being disabled only means PSCI is not implemented by
> > qemu; it doesn't mean PSCI is not supported on this virtual machine.
> > Actually vms->psci_conduit is set to disabled when vms->secure and
> > firmware_loaded are both set, which means we will run ARM trusted
> > firmware, which will definitely provide PSCI.
> >
> > The issue can be reproduced when running qemu in TCG mode with secure
> > enabled, while using ARM trusted firmware + qemu virt UEFI as firmware
> > binaries, and we can see secondary cores will not be waken up.
> 
> If you're using a real EL3 guest firmware then it's the job of
> the guest firmware to provide a DTB to the guest EL2/EL1 that says
> "and I support PSCI" if it supports PSCI, surely? QEMU can't tell
> whether the EL3 code does or doesn't do that...
> 
> thanks
> -- PMM

I guess this means qemu needs to find this out from firmware?
Perhaps through fwcfg ...
Don't really know about PSCI specifically, just a general
comment from ACPI POV.

-- 
MST

Re: [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled

[Andrew Jones]

On Fri, Jul 03, 2020 at 03:41:05PM +0100, Peter Maydell wrote:
> On Fri, 3 Jul 2020 at 15:36, Heyi Guo <guoheyi@linux.alibaba.com> wrote:
> >
> >
> > 在 2020/7/3 下午6:37, Peter Maydell 写道:
> > > On Fri, 3 Jul 2020 at 10:44, Heyi Guo <guoheyi@linux.alibaba.com> wrote:
> > >> vms->psci_conduit being disabled only means PSCI is not implemented by
> > >> qemu; it doesn't mean PSCI is not supported on this virtual machine.
> > >> Actually vms->psci_conduit is set to disabled when vms->secure and
> > >> firmware_loaded are both set, which means we will run ARM trusted
> > >> firmware, which will definitely provide PSCI.
> > >>
> > >> The issue can be reproduced when running qemu in TCG mode with secure
> > >> enabled, while using ARM trusted firmware + qemu virt UEFI as firmware
> > >> binaries, and we can see secondary cores will not be waken up.
> > > If you're using a real EL3 guest firmware then it's the job of
> > > the guest firmware to provide a DTB to the guest EL2/EL1 that says
> > > "and I support PSCI" if it supports PSCI, surely? QEMU can't tell
> > > whether the EL3 code does or doesn't do that...
> >
> > Thanks, Peter. Does that mean the ACPI tables generated in qemu are only
> > templates and firmware should update them if necessary?
> 
> I don't really know enough about ACPI to say. I hadn't noticed
> that this patch only updated the ACPI tables, sorry. Perhaps it
> is correct; Andrew will probably know better than me.
>

This seems a bit messy to me. With an EL3 firmware, the DTB is provided
by the EL3 firmware. I guess that's why when I look at the DTB generation
in virt.c we don't properly set "enable-method" of the CPUs to
"spin-table", even though we don't set it to "psci"[*]. However, if the
EL3 firmware isn't providing the ACPI tables too, then how do the DTB
and ACPI tables stay in synch? We can't be sure that just because we
have (vms->secure && firmware_loaded) that we should / should not
generate certain ACPI table entries when we don't also know what the
corresponding DTB will be.

So, I think the EL3 firmware should also provide the ACPI tables.
However, this patch it probably fine too. For a configuration where
the EL3 firmware provides the ACPI tables, it will do no harm. For
configurations where EL3 firmware isn't involved, it will do no harm.
And, for configurations like this, which I consider a bit hacky, it's
probably better to assume PSCI than not.

Thanks,
drew

[*] kernel doc Documentation/devicetree/bindings/arm/cpus.yaml says
    "enable-method" must be "spin-table" or "psci"

Re: [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled

[Andrew Jones]

On Fri, Jul 03, 2020 at 05:43:29PM +0800, Heyi Guo wrote:
> vms->psci_conduit being disabled only means PSCI is not implemented by
> qemu; it doesn't mean PSCI is not supported on this virtual machine.
> Actually vms->psci_conduit is set to disabled when vms->secure and
> firmware_loaded are both set, which means we will run ARM trusted
> firmware, which will definitely provide PSCI.
> 
> The issue can be reproduced when running qemu in TCG mode with secure
> enabled, while using ARM trusted firmware + qemu virt UEFI as firmware
> binaries, and we can see secondary cores will not be waken up.
> 
> Signed-off-by: Heyi Guo <guoheyi@linux.alibaba.com>
> 
> ---
> Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: qemu-arm@nongnu.org
> ---
>  hw/arm/virt-acpi-build.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 1384a2c..7622b97 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -728,13 +728,16 @@ static void build_fadt_rev5(GArray *table_data, BIOSLinker *linker,
>      };
>  
>      switch (vms->psci_conduit) {
> -    case QEMU_PSCI_CONDUIT_DISABLED:
> -        fadt.arm_boot_arch = 0;
> -        break;
>      case QEMU_PSCI_CONDUIT_HVC:
>          fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT |
>                               ACPI_FADT_ARM_PSCI_USE_HVC;
>          break;
> +    /*
> +     * QEMU_PSCI_CONDUIT_DISABLED only means PSCI is not implemented by qemu,
> +     * but typically it will still be provided by secure firmware, and it should
> +     * use SMC as PSCI conduit.
> +     */

How about elaborating on this in the comment like below?

 QEMU_PSCI_CONDUIT_DISABLED means PSCI is not implemented by QEMU. In
 this case we must have an EL3 firmware, which will most likely provide
 PSCI to the guest with the SMC conduit. If this assumption is wrong,
 then it is no worse than assuming PSCI is not available to the guest
 when it should be. The only way a user can be sure that the ACPI tables
 match what the firmware supports is if the firmware provides the ACPI
 tables itself.

> +     * but typically it will still be provided by secure firmware, and it should
> +     * use SMC as PSCI conduit.


> +    case QEMU_PSCI_CONDUIT_DISABLED:
>      case QEMU_PSCI_CONDUIT_SMC:
>          fadt.arm_boot_arch = ACPI_FADT_ARM_PSCI_COMPLIANT;
>          break;
> -- 
> 2.7.4
> 
>

Otherwise

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled

[Peter Maydell]

On Tue, 7 Jul 2020 at 11:04, Andrew Jones <drjones@redhat.com> wrote:
> This seems a bit messy to me. With an EL3 firmware, the DTB is provided
> by the EL3 firmware. I guess that's why when I look at the DTB generation
> in virt.c we don't properly set "enable-method" of the CPUs to
> "spin-table", even though we don't set it to "psci"[*].

Well, there's no way in the DTB to say "all the CPUs start at once" :-)
"spin-table" would be just as wrong as "psci" for us in that case.

> So, I think the EL3 firmware should also provide the ACPI tables.

Mmm, but I thought the general design for QEMU was that we have
to help the EL3 firmware along by providing ACPI fragments for it
to assemble. As I understand it, this is a pragmatic decision
because the binary format of a complete ACPI table is painful
to edit. So I suppose one question here is "if QEMU doesn't set
the PSCI flag in the ACPI tables, how hard is it for the EL3
firmware to edit the table to add the flag?".

> However, this patch it probably fine too. For a configuration where
> the EL3 firmware provides the ACPI tables, it will do no harm. For
> configurations where EL3 firmware isn't involved, it will do no harm.
> And, for configurations like this, which I consider a bit hacky, it's
> probably better to assume PSCI than not.

Is this really a 'hacky' configuration? I sort of expected it to
be a fairly common one for the 'virt' board. (For sbsa-ref the
EL3 firmware would provide a complete canned ACPI table, I think,
but for virt it can't and shouldn't do that.)

thanks
-- PMM

Re: [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled

[Andrew Jones]

On Tue, Jul 07, 2020 at 11:15:30AM +0100, Peter Maydell wrote:
> On Tue, 7 Jul 2020 at 11:04, Andrew Jones <drjones@redhat.com> wrote:
> > This seems a bit messy to me. With an EL3 firmware, the DTB is provided
> > by the EL3 firmware. I guess that's why when I look at the DTB generation
> > in virt.c we don't properly set "enable-method" of the CPUs to
> > "spin-table", even though we don't set it to "psci"[*].
> 
> Well, there's no way in the DTB to say "all the CPUs start at once" :-)
> "spin-table" would be just as wrong as "psci" for us in that case.
> 
> > So, I think the EL3 firmware should also provide the ACPI tables.
> 
> Mmm, but I thought the general design for QEMU was that we have
> to help the EL3 firmware along by providing ACPI fragments for it
> to assemble. As I understand it, this is a pragmatic decision
> because the binary format of a complete ACPI table is painful
> to edit. So I suppose one question here is "if QEMU doesn't set
> the PSCI flag in the ACPI tables, how hard is it for the EL3
> firmware to edit the table to add the flag?".
> 
> > However, this patch it probably fine too. For a configuration where
> > the EL3 firmware provides the ACPI tables, it will do no harm. For
> > configurations where EL3 firmware isn't involved, it will do no harm.
> > And, for configurations like this, which I consider a bit hacky, it's
> > probably better to assume PSCI than not.
> 
> Is this really a 'hacky' configuration? I sort of expected it to
> be a fairly common one for the 'virt' board. (For sbsa-ref the
> EL3 firmware would provide a complete canned ACPI table, I think,
> but for virt it can't and shouldn't do that.)

IMO, if the EL3 firmware is providing the complete DTB, then it should
provide the complete ACPI tables. Otherwise we should expose machine
properties allowing the virt board to generate both DTB and ACPI for an
EL3 firmware configuration.

The other option of using fw-cfg to tweak ACPI tables may work too, but
only for tweaks. If the EL3 firmware controlled DTB changed in a way
that diverges too much from QEMU's ACPI generation, then there'd still
be a problem.

Thanks,
drew