Re: [PATCH] hvmloader/PCI: skip huge BARs in certain calculations

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.03.2024 10:25, Roger Pau Monné wrote:
> On Mon, Mar 04, 2024 at 02:25:45PM +0100, Jan Beulich wrote:
>> On 04.03.2024 11:02, Roger Pau Monné wrote:
>>>> --- a/tools/firmware/hvmloader/pci.c
>>>> +++ b/tools/firmware/hvmloader/pci.c
>>>> @@ -33,6 +33,13 @@ uint32_t pci_mem_start = HVM_BELOW_4G_MM
>>>>  const uint32_t pci_mem_end = RESERVED_MEMBASE;
>>>>  uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
>>>>  
>>>> +/*
>>>> + * BARs larger than this value are put in 64-bit space unconditionally.  
>>>> That
>>>> + * is, such BARs also don't play into the determination of how big the 
>>>> lowmem
>>>> + * MMIO hole needs to be.
>>>> + */
>>>> +#define HUGE_BAR_THRESH GB(1)
>>>
>>> I would be fine with defining this to an even lower number, like
>>> 256Mb, as to avoid as much as possible memory relocation in order to
>>> make the MMIO hole bigger.
>>
>> As suggested in a post-commit-message remark, the main question then is
>> how to justify this.
> 
> I think the justification is to avoid having to relocate memory in
> order to attempt to make the hole below 4Gb larger.

Upon further thinking about it, I'm now pretty convinced that any lowering
of the boundary would better be a separate change. Right here I'd like to
stick to just the technically implied boundary.

>>>> @@ -446,8 +455,9 @@ void pci_setup(void)
>>>>           *   the code here assumes it to be.)
>>>>           * Should either of those two conditions change, this code will 
>>>> break.
>>>>           */
>>>> -        using_64bar = bars[i].is_64bar && bar64_relocate
>>>> -            && (mmio_total > (mem_resource.max - mem_resource.base));
>>>> +        using_64bar = bars[i].is_64bar && bar64_relocate &&
>>>> +            (mmio_total > (mem_resource.max - mem_resource.base) ||
>>>> +             bar_sz > HUGE_BAR_THRESH);
>>>>          bar_data = pci_readl(devfn, bar_reg);
>>>>  
>>>>          if ( (bar_data & PCI_BASE_ADDRESS_SPACE) ==
>>>> @@ -467,7 +477,8 @@ void pci_setup(void)
>>>>                  resource = &mem_resource;
>>>>                  bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
>>>>              }
>>>> -            mmio_total -= bar_sz;
>>>> +            if ( bar_sz <= HUGE_BAR_THRESH )
>>>> +                mmio_total -= bar_sz;
>>>
>>> I'm missing the part where hvmloader notifies QEMU of the possibly
>>> expanded base and size memory PCI MMIO regions, so that those are
>>> reflected in the PCI root complex registers?
>>
>> I don't understand this comment: I'm not changing the interaction
>> with qemu at all. Whatever the new calculation it'll be communicated
>> to qemu just as before.
> 
> That wasn't a complain about the patch, just me failing to see where
> this is done.

I see. Is there any such needed though? There's nothing root-complex-ish
in PIIX4 after all, for not knowing of PCIe yet. The only datasheet I
have readily available is for a slightly older variant of the 82371AB,
yet I can't spot any registers there which would need updating (to
inform qemu).

>>> Overall I think we could simplify the code by having a hardcoded 1Gb
>>> PCI MMIO hole below 4Gb, fill it with all the 32bit BARs and
>>> (re)locate all 64bit BARs above 4Gb (not that I'm requesting you to do
>>> it here).
>>
>> I'm afraid that would not work very well with OSes which aren't 64-bit
>> BAR / PA aware (first and foremost non-PAE 32-bit ones). Iirc that's
>> the reason why it wasn't done like you suggest back at the time.
> 
> There will still be a ~1Gb window < 4Gb, so quite a bit of space.

Yet not enough to fit a single 1Gb BAR.

> I'm unsure whether such OSes will have drivers to manage devices with
> that huge BARs in the first place.

Assuming at least basic functionality of gfx cards is backwards
compatible, I see nothing wrong with an old driver successfully attaching
to a modern card surfacing, say, a 256Mb BAR.

I'm afraid we need to be conservative here and keep configurations working
which in principle can work without using any 64-bit addresses.

Jan