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>