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

[Thierry Escande <thierry.escande@xxxxxxxxxx>]

On 4/28/26 13:08, Roger Pau Monné wrote:
> 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@xxxxxxxxx>
>> Signed-off-by: Thierry Escande <thierry.escande@xxxxxxxxxx>
>> ---
>>  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.

Looking at the acpi_build_tables() for ovmf, seabios, and rombios, I
suppose the idea here was to set the correct ACPI tables for guest using
Rombios with qemu-xen.

So I can move the qemu-xen check in rombios_acpi_build_tables() and do
the machine_type check in all acpi_build_tables callbacks. I'm a bit
puzzled by the duplication, though...

> 
>>      {
>> -        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).

Right, dsdt_q35_anycpu_qemu_xen.c that declares
dsdt_q35_anycpu_qemu_xen[] is generated by patch 2 and used in this patch.

> 
>>  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.

Yes, that will be done in a previous patch when dsdt_anycpu_qemu_xen_len
is renamed to dsdt_i440_anycpu_qemu_xen_len.

Regards,


--
Thierry Escande | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech