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

[Thierry Escande <thierry.escande@xxxxxxxxxx>]

On 4/28/26 14:48, Roger Pau Monné wrote:
> 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.

Sure, will do.

> 
>> Signed-off-by: Thierry Escande <thierry.escande@xxxxxxxxxx>
>> ---
>>  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?

That was to have less modification and have all calls to pci_readw() in
one place. I can move this one to the new function, no problem.

> 
>> +                                            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?

Sure.

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

Right. Will do the same for all IDs.

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

Ok

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

Sure, will do.

Regards,


--
Thierry Escande | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech