Re: [Qemu-devel] [PATCH v2 15/16] hw/arm/virt-acpi-build: use SMC if booting in EL2

From: Peter Maydell <peter.maydell@linaro.org>
To: Andrew Jones <drjones@redhat.com>
Cc: QEMU Developers <qemu-devel@nongnu.org>,
	qemu-arm <qemu-arm@nongnu.org>,
	Igor Mammedov <imammedo@redhat.com>,
	Shannon Zhao <zhaoshenglong@huawei.com>,
	Shannon Zhao <shannon.zhao@linaro.org>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Eduardo Habkost <ehabkost@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v2 15/16] hw/arm/virt-acpi-build: use SMC if booting in EL2
Date: Fri, 6 Jan 2017 17:12:29 +0000	[thread overview]
Message-ID: <CAFEAcA_bdPDyemXcnTAM497RKhXhYRpk5KJNs=h+7OtEkQ+60A@mail.gmail.com> (raw)
In-Reply-To: <20170102200153.28864-16-drjones@redhat.com>

On 2 January 2017 at 20:01, Andrew Jones <drjones@redhat.com> wrote:
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index b5e17a680d51..7c00ee683f44 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -643,16 +643,16 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  }
>
>  /* FADT */
> -static void
> -build_fadt(GArray *table_data, BIOSLinker *linker, unsigned dsdt_tbl_offset)
> +static void build_fadt(GArray *table_data, BIOSLinker *linker,
> +                       VirtMachineState *vms, unsigned dsdt_tbl_offset)
>  {
>      AcpiFadtDescriptorRev5_1 *fadt = acpi_data_push(table_data, sizeof(*fadt));
>      unsigned dsdt_entry_offset = (char *)&fadt->dsdt - table_data->data;
> +    uint16_t use_hvc = vms->virt ? 0 : ACPI_FADT_ARM_PSCI_USE_HVC;

This is the wrong condition -- you should just check whether
vms->psci_conduit is QEMU_PSCI_CONDUIT_HVC (vs _SMC or _DISABLED), something
like:

    switch (vms->psci_conduit) {
    case QEMU_PSCI_CONDUIT_DISABLED:
        /* XXX should we fail, or boot without the PSCI_COMPLIANT flag? */
        use_hvc = 0;
        break;
    case QEMU_PSCI_CONDUIT_HVC:
        use_hvc = ACPI_FADT_ARM_PSCI_USE_HVC;
        break;
    case QEMU_PSCI_CONDUIT_SMC:
        use_hvc = 0;
        break;
    default:
        g_assert_not_reached();
    }

What should we do in the DISABLED case ?

> -    /* Hardware Reduced = 1 and use PSCI 0.2+ and with HVC */
> +    /* Hardware Reduced = 1 and use PSCI 0.2+ */
>      fadt->flags = cpu_to_le32(1 << ACPI_FADT_F_HW_REDUCED_ACPI);
> -    fadt->arm_boot_flags = cpu_to_le16(ACPI_FADT_ARM_PSCI_COMPLIANT |
> -                                       ACPI_FADT_ARM_PSCI_USE_HVC);
> +    fadt->arm_boot_flags = cpu_to_le16(ACPI_FADT_ARM_PSCI_COMPLIANT | use_hvc);
>
>      /* ACPI v5.1 (fadt->revision.fadt->minor_revision) */
>      fadt->minor_revision = 0x1;
> @@ -738,7 +738,7 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>
>      /* FADT MADT GTDT MCFG SPCR pointed to by RSDT */
>      acpi_add_table(table_offsets, tables_blob);
> -    build_fadt(tables_blob, tables->linker, dsdt);
> +    build_fadt(tables_blob, tables->linker, vms, dsdt);
>
>      acpi_add_table(table_offsets, tables_blob);
>      build_madt(tables_blob, tables->linker, vms);

thanks
-- PMM