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

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

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.

> Signed-off-by: Alexey Gerasimenko <x1917x@gmail.com>

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@vates.tech>
> ---
>  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.

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

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

Thanks, Roger.