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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Mar 04, 2024 at 08:32:22AM +0100, Jan Beulich wrote:
> BARs of size 2Gb and up can't possibly fit below 4Gb: Both the bottom of
> the lower 2Gb range and the top of the higher 2Gb range have special
> purpose. Don't even have them influence whether to (perhaps) relocate
> low RAM.

Here you mention 2Gb BARs, yet the code below sets the maximum BAR
size supported below 4Gb to 1Gb.

> Reported-by: Neowutran <xen@xxxxxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> If we wanted to fit e.g. multiple 1Gb BARs, it would likely be prudent
> to similarly avoid low RAM relocation in the first place. Yet accounting
> for things differently depending on how many large BARs there are would
> require more intrusive code changes.
> 
> That said, I'm open to further lowering of the threshold. That'll
> require different justification then, though.
> 
> --- 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.

> +
>  enum virtual_vga virtual_vga = VGA_none;
>  unsigned long igd_opregion_pgbase = 0;
>  
> @@ -286,9 +293,11 @@ void pci_setup(void)
>              bars[i].bar_reg = bar_reg;
>              bars[i].bar_sz  = bar_sz;
>  
> -            if ( ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
> -                  PCI_BASE_ADDRESS_SPACE_MEMORY) ||
> -                 (bar_reg == PCI_ROM_ADDRESS) )
> +            if ( is_64bar && bar_sz > HUGE_BAR_THRESH )
> +                bar64_relocate = 1;
> +            else if ( ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
> +                       PCI_BASE_ADDRESS_SPACE_MEMORY) ||
> +                      (bar_reg == PCI_ROM_ADDRESS) )
>                  mmio_total += bar_sz;
>  
>              nr_bars++;
> @@ -367,7 +376,7 @@ void pci_setup(void)
>              pci_mem_start = hvm_info->low_mem_pgend << PAGE_SHIFT;
>      }
>  
> -    if ( mmio_total > (pci_mem_end - pci_mem_start) )
> +    if ( mmio_total > (pci_mem_end - pci_mem_start) || bar64_relocate )
>      {
>          printf("Low MMIO hole not large enough for all devices,"
>                 " relocating some BARs to 64-bit\n");

Is the above message now accurate?  Given the current code the low
MMIO could be expanded up to 2Gb, yet BAR relocation will happen
unconditionally once a 1Gb BAR is found.

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

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

Thanks, Roger.