Re: [PATCH 06/17] hvmloader: Move pci devices setup to a separate function

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

On Fri, Mar 13, 2026 at 04:35:02PM +0000, Thierry Escande wrote:
> For readability and code simplification, this patch moves PCI-device
> specific initializations out of the pci_setup() function to a new
> function class_specific_pci_device_setup().

AFAICT this is a non-functional change.  Should likely be mentioned in
the commit message to avoid any doubts.

> Signed-off-by: Thierry Escande <thierry.escande@vates.tech>
> ---
>  tools/firmware/hvmloader/pci.c      | 117 +++++++++++++++-------------
>  tools/firmware/hvmloader/pci_regs.h |   4 +
>  2 files changed, 68 insertions(+), 53 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> index c41c8d946a..a76d051bdf 100644
> --- a/tools/firmware/hvmloader/pci.c
> +++ b/tools/firmware/hvmloader/pci.c
> @@ -84,12 +84,71 @@ static int find_next_rmrr(uint32_t base)
>      return next_rmrr;
>  }
>  
> +static void class_specific_pci_device_setup(uint16_t vendor_id,
> +                                            uint16_t device_id,
> +                                            uint16_t class,

It's a bit weird to pass the class value into the function, the value
is only used inside the function itself, and hence could be fetched
inside the function as the device BDF is provided as parameters?

> +                                            uint8_t bus,
> +                                            uint8_t devfn, uint8_t *vga_devfn)
> +{
> +    switch ( class )
> +    {
> +    case PCI_CLASS_DISPLAY_VGA:
> +        /* If emulated VGA is found, preserve it as primary VGA. */
> +        if ( (vendor_id == 0x1234) && (device_id == 0x1111) )
> +        {
> +            *vga_devfn = devfn;
> +            virtual_vga = VGA_std;
> +        }
> +        else if ( (vendor_id == 0x1013) && (device_id == 0xb8) )

Since you introduce defines for the device classes, could you also
introduce defines for the vendor and device IDs used here?

> +        {
> +            *vga_devfn = devfn;
> +            virtual_vga = VGA_cirrus;
> +        }
> +        else if ( virtual_vga == VGA_none )
> +        {
> +            *vga_devfn = devfn;
> +            virtual_vga = VGA_pt;
> +            if ( vendor_id == 0x8086 )

This one is PCI_VENDOR_ID_INTEL, also a couple of more instances below.

> +            {
> +                igd_opregion_pgbase = mem_hole_alloc(IGD_OPREGION_PAGES);
> +                /*
> +                 * Write the the OpRegion offset to give the opregion
> +                 * address to the device model. The device model will trap
> +                 * and map the OpRegion at the give address.
> +                 */
> +                pci_writel(*vga_devfn, PCI_INTEL_OPREGION,
> +                           igd_opregion_pgbase << PAGE_SHIFT);
> +            }
> +        }
> +        break;

Newlines after break statements.

> +    case PCI_CLASS_BRIDGE_OTHER:
> +        /* PIIX4 ACPI PM. Special device with special PCI config space. */
> +        ASSERT((vendor_id == 0x8086) && (device_id == 0x7113));
> +        pci_writew(devfn, 0x20, 0x0000); /* No smb bus IO enable */
> +        pci_writew(devfn, 0xd2, 0x0000); /* No smb bus IO enable */
> +        pci_writew(devfn, 0x22, 0x0000);
> +        pci_writew(devfn, 0x3c, 0x0009); /* Hardcoded IRQ9 */
> +        pci_writew(devfn, 0x3d, 0x0001);
> +        pci_writel(devfn, 0x40, ACPI_PM1A_EVT_BLK_ADDRESS_V1 | 1);
> +        pci_writeb(devfn, 0x80, 0x01); /* enable PM io space */
> +        break;
> +    case PCI_CLASS_STORAGE_IDE:
> +        if ( vendor_id == 0x8086 )
> +        {
> +            /* Intel ICHs since PIIX3: enable IDE legacy mode. */
> +            pci_writew(devfn, 0x40, 0x8000); /* enable IDE0 */
> +            pci_writew(devfn, 0x42, 0x8000); /* enable IDE1 */
> +        }
> +        break;
> +    }
> +}
> +
>  void pci_setup(void)
>  {
>      uint8_t is_64bar, using_64bar, bar64_relocate = 0;
>      uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
>      uint64_t base, bar_sz, bar_sz_upper, mmio_total = 0;
> -    uint32_t vga_devfn = 256;
> +    uint8_t vga_devfn = 0xff;
>      uint16_t class, vendor_id, device_id;
>      unsigned int bar, pin, link, isa_irq;
>      uint8_t pci_devfn_decode_type[256] = {};
> @@ -170,57 +229,9 @@ void pci_setup(void)
>          ASSERT((devfn != PCI_ISA_DEVFN) ||
>                 ((vendor_id == 0x8086) && (device_id == 0x7000)));
>  
> -        switch ( class )
> -        {
> -        case 0x0300:
> -            /* If emulated VGA is found, preserve it as primary VGA. */
> -            if ( (vendor_id == 0x1234) && (device_id == 0x1111) )
> -            {
> -                vga_devfn = devfn;
> -                virtual_vga = VGA_std;
> -            }
> -            else if ( (vendor_id == 0x1013) && (device_id == 0xb8) )
> -            {
> -                vga_devfn = devfn;
> -                virtual_vga = VGA_cirrus;
> -            }
> -            else if ( virtual_vga == VGA_none )
> -            {
> -                vga_devfn = devfn;
> -                virtual_vga = VGA_pt;
> -                if ( vendor_id == 0x8086 )
> -                {
> -                    igd_opregion_pgbase = mem_hole_alloc(IGD_OPREGION_PAGES);
> -                    /*
> -                     * Write the the OpRegion offset to give the opregion
> -                     * address to the device model. The device model will trap 
> -                     * and map the OpRegion at the give address.
> -                     */
> -                    pci_writel(vga_devfn, PCI_INTEL_OPREGION,
> -                               igd_opregion_pgbase << PAGE_SHIFT);
> -                }
> -            }
> -            break;
> -        case 0x0680:
> -            /* PIIX4 ACPI PM. Special device with special PCI config space. */
> -            ASSERT((vendor_id == 0x8086) && (device_id == 0x7113));
> -            pci_writew(devfn, 0x20, 0x0000); /* No smb bus IO enable */
> -            pci_writew(devfn, 0xd2, 0x0000); /* No smb bus IO enable */
> -            pci_writew(devfn, 0x22, 0x0000);
> -            pci_writew(devfn, 0x3c, 0x0009); /* Hardcoded IRQ9 */
> -            pci_writew(devfn, 0x3d, 0x0001);
> -            pci_writel(devfn, 0x40, ACPI_PM1A_EVT_BLK_ADDRESS_V1 | 1);
> -            pci_writeb(devfn, 0x80, 0x01); /* enable PM io space */
> -            break;
> -        case 0x0101:
> -            if ( vendor_id == 0x8086 )
> -            {
> -                /* Intel ICHs since PIIX3: enable IDE legacy mode. */
> -                pci_writew(devfn, 0x40, 0x8000); /* enable IDE0 */
> -                pci_writew(devfn, 0x42, 0x8000); /* enable IDE1 */
> -            }
> -            break;
> -        }
> +        class_specific_pci_device_setup(vendor_id, device_id, class,
> +                                        0 /* virt_bus support TBD */,
> +                                        devfn, &vga_devfn);
>  
>          /*
>           * It is recommended that BAR programming be done whilst decode
> @@ -583,7 +594,7 @@ void pci_setup(void)
>                            ((pci_hi_mem_start & -pci_hi_mem_start) - 1)) + 1;
>      }
>  
> -    if ( vga_devfn != 256 )
> +    if ( vga_devfn != 0xff )
>      {
>          /*
>           * VGA registers live in I/O space so ensure that primary VGA
> diff --git a/tools/firmware/hvmloader/pci_regs.h b/tools/firmware/hvmloader/pci_regs.h
> index 4d4dc0cd01..c94278855b 100644
> --- a/tools/firmware/hvmloader/pci_regs.h
> +++ b/tools/firmware/hvmloader/pci_regs.h
> @@ -111,6 +111,10 @@
>  #define PCI_DEVICE_ID_INTEL_82441        0x1237
>  #define PCI_DEVICE_ID_INTEL_Q35_MCH      0x29c0
>  
> +#define PCI_CLASS_STORAGE_IDE            0x0101
> +#define PCI_CLASS_DISPLAY_VGA            0x0300
> +#define PCI_CLASS_BRIDGE_OTHER           0x0680

As mentioned in a previous patch, this would better be placed in a
pci_ids.h header.

Thanks, Roger.