hw/arm/virt-acpi-build.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
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
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
在 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
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
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"
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
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
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
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>
© 2016 - 2024 Red Hat, Inc.