* [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled @ 2020-07-03 9:43 Heyi Guo 2020-07-03 10:37 ` Peter Maydell 2020-07-07 10:14 ` Andrew Jones 0 siblings, 2 replies; 9+ messages in thread From: Heyi Guo @ 2020-07-03 9:43 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, yitian.ly, Michael S. Tsirkin, Shannon Zhao, qemu-arm, Heyi Guo, Igor Mammedov 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled 2020-07-03 9:43 [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled Heyi Guo @ 2020-07-03 10:37 ` Peter Maydell 2020-07-03 14:36 ` Heyi Guo 2020-07-04 18:43 ` Michael S. Tsirkin 2020-07-07 10:14 ` Andrew Jones 1 sibling, 2 replies; 9+ messages in thread From: Peter Maydell @ 2020-07-03 10:37 UTC (permalink / raw) To: Heyi Guo Cc: yitian.ly, Michael S. Tsirkin, QEMU Developers, Shannon Zhao, qemu-arm, Igor Mammedov 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled 2020-07-03 10:37 ` Peter Maydell @ 2020-07-03 14:36 ` Heyi Guo 2020-07-03 14:41 ` Peter Maydell 2020-07-04 18:43 ` Michael S. Tsirkin 1 sibling, 1 reply; 9+ messages in thread From: Heyi Guo @ 2020-07-03 14:36 UTC (permalink / raw) To: Peter Maydell Cc: yitian.ly, Michael S. Tsirkin, QEMU Developers, Shannon Zhao, qemu-arm, Igor Mammedov 在 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled 2020-07-03 14:36 ` Heyi Guo @ 2020-07-03 14:41 ` Peter Maydell 2020-07-07 10:04 ` Andrew Jones 0 siblings, 1 reply; 9+ messages in thread From: Peter Maydell @ 2020-07-03 14:41 UTC (permalink / raw) To: Heyi Guo Cc: Andrew Jones, yitian.ly, Michael S. Tsirkin, QEMU Developers, Shannon Zhao, qemu-arm, Igor Mammedov 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled 2020-07-03 14:41 ` Peter Maydell @ 2020-07-07 10:04 ` Andrew Jones 2020-07-07 10:15 ` Peter Maydell 0 siblings, 1 reply; 9+ messages in thread From: Andrew Jones @ 2020-07-07 10:04 UTC (permalink / raw) To: Peter Maydell Cc: yitian.ly, Michael S. Tsirkin, QEMU Developers, Shannon Zhao, qemu-arm, Heyi Guo, Igor Mammedov 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" ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled 2020-07-07 10:04 ` Andrew Jones @ 2020-07-07 10:15 ` Peter Maydell 2020-07-07 10:28 ` Andrew Jones 0 siblings, 1 reply; 9+ messages in thread From: Peter Maydell @ 2020-07-07 10:15 UTC (permalink / raw) To: Andrew Jones Cc: yitian.ly, Michael S. Tsirkin, QEMU Developers, Shannon Zhao, qemu-arm, Heyi Guo, Igor Mammedov 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled 2020-07-07 10:15 ` Peter Maydell @ 2020-07-07 10:28 ` Andrew Jones 0 siblings, 0 replies; 9+ messages in thread From: Andrew Jones @ 2020-07-07 10:28 UTC (permalink / raw) To: Peter Maydell Cc: yitian.ly, Michael S. Tsirkin, QEMU Developers, Shannon Zhao, qemu-arm, Heyi Guo, Igor Mammedov 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled 2020-07-03 10:37 ` Peter Maydell 2020-07-03 14:36 ` Heyi Guo @ 2020-07-04 18:43 ` Michael S. Tsirkin 1 sibling, 0 replies; 9+ messages in thread From: Michael S. Tsirkin @ 2020-07-04 18:43 UTC (permalink / raw) To: Peter Maydell Cc: yitian.ly, QEMU Developers, Shannon Zhao, qemu-arm, Heyi Guo, Igor Mammedov 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled 2020-07-03 9:43 [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled Heyi Guo 2020-07-03 10:37 ` Peter Maydell @ 2020-07-07 10:14 ` Andrew Jones 1 sibling, 0 replies; 9+ messages in thread From: Andrew Jones @ 2020-07-07 10:14 UTC (permalink / raw) To: Heyi Guo Cc: Peter Maydell, yitian.ly, Michael S. Tsirkin, qemu-devel, Shannon Zhao, qemu-arm, Igor Mammedov 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> ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-07-07 10:30 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-03 9:43 [RFC] virt/acpi: set PSCI flag even when psci_conduit is disabled Heyi Guo 2020-07-03 10:37 ` Peter Maydell 2020-07-03 14:36 ` Heyi Guo 2020-07-03 14:41 ` Peter Maydell 2020-07-07 10:04 ` Andrew Jones 2020-07-07 10:15 ` Peter Maydell 2020-07-07 10:28 ` Andrew Jones 2020-07-04 18:43 ` Michael S. Tsirkin 2020-07-07 10:14 ` Andrew Jones
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.