Re: [PATCH 03/17] hvmloader: add function to set the emulated machine type (i440/Q35)

[Thierry Escande <thierry.escande@xxxxxxxxxx>]

On 4/28/26 12:39, Roger Pau Monné wrote:
> On Fri, Mar 13, 2026 at 04:35:01PM +0000, Thierry Escande wrote:
>> This adds a new function init_pc_machine_type() which allows to
>> determine and set the emulated chipset type. Possible values are
>> MACHINE_TYPE_I440 and MACHINE_TYPE_Q35 and stored in the global variable
>> machine_type.
>>
>> The machine_type variable will be used from multiple places in following
>> commits.
> 
> Is this initialization something that OVMF or SeaBIOS also does?
> (maybe not for Xen ATM)
> 
> Asking myself because as said earlier we want to possibly get rid of
> hvmloader, plus we will want ECAM support in PVH at some point.

Possibly. There are surely overlaps in term of initialization between
the toolstack, hvmloader and the firmwares but that's another story imo.

> 
>> Signed-off-by: Alexey Gerasimenko <x1917x@xxxxxxxxx>
> 
> Same as previous patch, if the first SoB is from Alexey the From:
> (patch author) should also match.
> 
>> Signed-off-by: Thierry Escande <thierry.escande@xxxxxxxxxx>
>> ---
>>  tools/firmware/hvmloader/hvmloader.c |  2 ++
>>  tools/firmware/hvmloader/pci_regs.h  |  4 +++
>>  tools/firmware/hvmloader/util.c      | 42 ++++++++++++++++++++++++++++
>>  tools/firmware/hvmloader/util.h      | 11 ++++++++
>>  4 files changed, 59 insertions(+)
>>
>> diff --git a/tools/firmware/hvmloader/hvmloader.c 
>> b/tools/firmware/hvmloader/hvmloader.c
>> index 6d23150fc9..626cc53649 100644
>> --- a/tools/firmware/hvmloader/hvmloader.c
>> +++ b/tools/firmware/hvmloader/hvmloader.c
>> @@ -332,6 +332,8 @@ int main(void)
>>  
>>      init_hypercalls();
>>  
>> +    init_pc_machine_type();
>> +
>>      memory_map_setup();
>>  
>>      xenbus_setup();
>> diff --git a/tools/firmware/hvmloader/pci_regs.h 
>> b/tools/firmware/hvmloader/pci_regs.h
>> index 7bf2d873ab..4d4dc0cd01 100644
>> --- a/tools/firmware/hvmloader/pci_regs.h
>> +++ b/tools/firmware/hvmloader/pci_regs.h
>> @@ -107,6 +107,10 @@
>>  
>>  #define PCI_INTEL_OPREGION 0xfc /* 4 bits */
>>  
>> +#define PCI_VENDOR_ID_INTEL              0x8086
>> +#define PCI_DEVICE_ID_INTEL_82441        0x1237
>> +#define PCI_DEVICE_ID_INTEL_Q35_MCH      0x29c0
> 
> In Xen we have a separate file for vendor and device IDs, called
> pci_ids.h.  Maybe it would be better to use a similar approach in
> hvmloader, and keep pci_regs.h only containing PCI register offsets.

As suggested by Jan, I'll use pci_ids.h from Xen (if it's ok to add a
symlink to pci_ids.h in tools/include/xen)

And is it ok to add the PCI device IDs to pci_ids.h in this commit or
should it be done in a separate one ?

> 
>> +
>>  #endif /* __HVMLOADER_PCI_REGS_H__ */
>>  
>>  /*
>> diff --git a/tools/firmware/hvmloader/util.c 
>> b/tools/firmware/hvmloader/util.c
>> index f1ed1eb48d..f9116bea4d 100644
>> --- a/tools/firmware/hvmloader/util.c
>> +++ b/tools/firmware/hvmloader/util.c
>> @@ -22,6 +22,7 @@
>>  #include "hypercall.h"
>>  #include "ctype.h"
>>  #include "vnuma.h"
>> +#include "pci_regs.h"
>>  #include <acpi2_0.h>
>>  #include <libacpi.h>
>>  #include <stdint.h>
>> @@ -648,6 +649,47 @@ void __bug(const char *file, int line)
>>      crash();
>>  }
>>  
>> +machine_type_t machine_type;
>> +
>> +void init_pc_machine_type(void)
> 
> Since detection is done based on PCI device IDs, it might be better
> placed in pci.c, and so you don't need to include pci_regs.h in
> util.c.

Yes, that makes more sense. And I'll rename it as
pci_init_machine_type() then.

> 
>> +{
>> +    uint16_t vendor_id;
>> +    uint16_t device_id;
>> +
>> +    if ( machine_type != MACHINE_TYPE_UNDEFINED )
>> +        return;
>> +
>> +    vendor_id = pci_readw(0, PCI_VENDOR_ID);
>> +    device_id = pci_readw(0, PCI_DEVICE_ID);
>> +
>> +    /* only Intel platforms are emulated currently */
>> +    if ( vendor_id != PCI_VENDOR_ID_INTEL )
>> +        goto error;
>> +
>> +    switch ( device_id )
>> +    {
>> +    case PCI_DEVICE_ID_INTEL_82441:
>> +        machine_type = MACHINE_TYPE_I440;
>> +        printf("Detected i440 chipset\n");
>> +        break;
>> +
>> +    case PCI_DEVICE_ID_INTEL_Q35_MCH:
>> +        machine_type = MACHINE_TYPE_Q35;
>> +        printf("Detected Q35 chipset\n");
>> +        break;
>> +
>> +    default:
>> +        goto error;
>> +    }
>> +
>> +    return;
>> +
>> +error:
>> +    printf("Unknown emulated chipset encountered, VID=%04Xh, DID=%04Xh\n",
> 
> We don't usually use the h suffix in hex numbers in hvmloader, it's
> more common to prefix them with 0x, so I would recommend to use the %#06x
> formatter instead.

I did some tests regarding Jan comment and yes, # and width specifier
seem to act weirdly. Will use 0x%0nx instead.

Regards,


--
Thierry Escande | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech