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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.04.2026 12:39, Roger Pau Monné wrote:
> On Fri, Mar 13, 2026 at 04:35:01PM +0000, Thierry Escande wrote:
>> --- 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.

Can't hvmloader simply re-use Xen's header(s)?

>> --- 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.
> 
>> +{
>> +    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'd generally advise against use of # with a width specifier, as that ends
up awkward for 0. That is, %#x is fine and generally to be preferred, but
for a specific with it might better be 0x%0<n>x (with n=4 here). Arguably
here we don't really expect either of the values to be 0, so the suggested
use may indeed be okay in this case (while still introducing an example
which later may be copied elsewhere without much thought).

Jan