Re: [PATCH 08/17] hvmloader: Extend PCI BAR struct

[Thierry Escande <thierry.escande@xxxxxxxxxx>]

On 4/28/26 15:31, Roger Pau Monné wrote:
> On Fri, Mar 13, 2026 at 04:35:03PM +0000, Thierry Escande wrote:
>> For the upcoming allocation of the MMCONFIG range in MMIO hole, this
>> patch extends the 'bars' structure to make it universal for any
>> arbitrary BAR type. Either IO, MMIO, ROM or a chipset-specific resource.
>>
>> One important new field is addr_mask, which tells which bits of the base
>> address can (should) be written. Different address types (ROM, MMIO BAR,
>> PCIEXBAR) will have different addr_mask values.
>>
>> For every assignable BAR range we store its size, PCI device BDF (devfn
>> actually) to which it belongs, BAR type (mem/io/mem64) and corresponding
>> register offset in device PCI conf space.
>>
>> Also, to reduce code complexity, all long mem/mem64 BAR flags checks are
>> replaced by simple bars[i] field probing, eg.:
>> -        if ( (bar_reg == PCI_ROM_ADDRESS) ||
>> -             ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
>> -              PCI_BASE_ADDRESS_SPACE_MEMORY) )
>> +        if ( bars[i].is_mem )
> 
> I think this is also supposed to be a non-functional change, just
> adding new fields and adjusting the code to make use of them?

Right. It's preparation work for MMCONFIG area setup in a next patch.

> 
>>
>> Signed-off-by: Alexey Gerasimenko <x1917x@xxxxxxxxx>
>> Signed-off-by: Thierry Escande <thierry.escande@xxxxxxxxxx>
>> ---
>>  tools/firmware/hvmloader/pci.c | 58 ++++++++++++++++++++--------------
>>  1 file changed, 35 insertions(+), 23 deletions(-)
>>
>> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
>> index 91c7fd2171..6e6720adae 100644
>> --- a/tools/firmware/hvmloader/pci.c
>> +++ b/tools/firmware/hvmloader/pci.c
>> @@ -160,9 +160,10 @@ static void class_specific_pci_device_setup(uint16_t 
>> vendor_id,
>>  
>>  void pci_setup(void)
>>  {
>> -    uint8_t is_64bar, using_64bar, bar64_relocate = 0;
>> +    uint8_t is_64bar, using_64bar, bar64_relocate = 0, is_mem;
> 
> The newly introduce fields want to be booleans types.

Ok. Should I also change the existing ones to bool or this must be in a
separate patch?

> 
>>      uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
>>      uint64_t base, bar_sz, bar_sz_upper, mmio_total = 0;
>> +    uint64_t addr_mask;
>>      uint8_t vga_devfn = 0xff;
>>      uint16_t class, vendor_id, device_id;
>>      unsigned int bar, pin, link, isa_irq;
>> @@ -176,10 +177,13 @@ void pci_setup(void)
>>  
>>      /* Create a list of device BARs in descending order of size. */
>>      struct bars {
>> -        uint32_t is_64bar;
>>          uint32_t devfn;
>>          uint32_t bar_reg;
>>          uint64_t bar_sz;
>> +        uint64_t addr_mask; /* which bits of the base address can be 
>> written */
>> +        uint32_t bar_data;  /* initial value - BAR flags here */
> 
> Hm, that's just storing the flags of the BAR, given that you already
> store the 64bit and memory flags, you just need the prefetch and ROM
> enabled booleans to have the full set, and then you can remove the
> bar_data field from the struct.

Do you mean reading the bar data from their registers in the resource
assignation loop instead of using the bar_data field? Storing it in this
struct saves a few back and forth with Qemu.

Or I didn't get the point...

> 
>> +        uint8_t  is_64bar;
>> +        uint8_t  is_mem;
> 
> Use bool types please for the is_ fields.

Sure.

Regards,


--
Thierry Escande | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech