Re: [PATCH 05/17] hvmloader: add Q35 DSDT table loading

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Mar 13, 2026 at 04:35:02PM +0000, Thierry Escande wrote:
> This patch allows to select Q35 DSDT table in the function
> hvmloader_acpi_build_tables(). The machine_type global variable is used
> to select a proper table (i440/q35).
> 
> As we are bound to the qemu-xen device model for Q35, there is no need
> to initialize config->dsdt_15cpu/config->dsdt_15cpu_len fields.
> 
> Signed-off-by: Alexey Gerasimenko <x1917x@gmail.com>
> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
> ---
>  tools/firmware/hvmloader/util.c | 17 +++++++++++++++--
>  tools/firmware/hvmloader/util.h |  2 ++
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/util.c b/tools/firmware/hvmloader/util.c
> index f9116bea4d..45519ea583 100644
> --- a/tools/firmware/hvmloader/util.c
> +++ b/tools/firmware/hvmloader/util.c
> @@ -885,8 +885,21 @@ void hvmloader_acpi_build_tables(struct acpi_config *config,
>      s = xenstore_read("platform/device-model", "");
>      if ( !strncmp(s, "qemu_xen", 9) )

Tying Q35 to xenstore seems a bit sub-optimal, as Q35 detection is
done purely from the PCI config space.  It would be better if we could
avoid relying on the xenstore node, and hence also fixup at least
ovmf_acpi_build_tables() and seabios_acpi_build_tables() to use the
Q35 ACPI tables when Q35 is detected, regardless of the device-model
xenstore node.

>      {
> -        config->dsdt_anycpu = dsdt_i440_anycpu_qemu_xen;
> -        config->dsdt_anycpu_len = dsdt_i440_anycpu_qemu_xen_len;
> +        switch ( machine_type )
> +        {
> +        case MACHINE_TYPE_Q35:
> +            config->dsdt_anycpu = dsdt_q35_anycpu_qemu_xen;
> +            config->dsdt_anycpu_len = dsdt_q35_anycpu_qemu_xen_len;
> +            break;
> +        case MACHINE_TYPE_I440:
> +            config->dsdt_anycpu = dsdt_i440_anycpu_qemu_xen;
> +            config->dsdt_anycpu_len = dsdt_i440_anycpu_qemu_xen_len;
> +            break;
> +        default:
> +            /* Not likely to happen */
> +            BUG();
> +        }
> +
>          config->dsdt_15cpu = NULL;
>          config->dsdt_15cpu_len = 0;
>      }
> diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h
> index 2f37504aca..4641ca0c46 100644
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -393,7 +393,9 @@ bool check_overlap(uint64_t start, uint64_t size,
>                     uint64_t reserved_start, uint64_t reserved_size);
>  
>  extern const unsigned char dsdt_i440_anycpu_qemu_xen[], dsdt_anycpu[], dsdt_15cpu[];
> +extern const unsigned char dsdt_q35_anycpu_qemu_xen[];

If I'm not mistaken, patch 2 introduces the dsdt_q35_anycpu_qemu_xen
symbol to the hvmloader build, so that you can reference it here?

At first I was going to comment that you are referencing a symbol not
included in the patch, but I think such symbol was previously added to
the build in patch 2 (and was unused there).

>  extern const int dsdt_i440_anycpu_qemu_xen_len, dsdt_anycpu_len, dsdt_15cpu_len;
> +extern const int dsdt_q35_anycpu_qemu_xen_len;

unsigned int would be better here.

Thanks, Roger.