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

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

[Jan Beulich]

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.

Reported-by: Neowutran <xen@neowutran.ovh>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
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)
+
 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");
@@ -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;
         }
         else
         {

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

[Roger Pau Monné]

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@neowutran.ovh>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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.

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

[Jan Beulich]

On 04.03.2024 11:02, Roger Pau Monné wrote:
> 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.

Hmm, I'm puzzled: There are no other BAR sizes between 1Gb and 2Gb.
Anything up to 1Gb continues to work as is, while everything 2Gb and
up has behavior changed.

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

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

Well, "all" may not be quite accurate anymore, yet would making it e.g.
"all applicable" really much more meaningful?

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

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

Jan

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

[Roger Pau Monné]

On Mon, Mar 04, 2024 at 02:25:45PM +0100, Jan Beulich wrote:
> On 04.03.2024 11:02, Roger Pau Monné wrote:
> > 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.
> 
> Hmm, I'm puzzled: There are no other BAR sizes between 1Gb and 2Gb.
> Anything up to 1Gb continues to work as is, while everything 2Gb and
> up has behavior changed.

My bad, I was confused.

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

> >> @@ -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.
> 
> Well, "all" may not be quite accurate anymore, yet would making it e.g.
> "all applicable" really much more meaningful?
> 
> >> @@ -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.

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

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

Thanks, Roger.

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

[Jan Beulich]

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

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

[Alejandro Vallejo]

Hi,

On 04/03/2024 07:32, 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.
>
> Reported-by: Neowutran <xen@neowutran.ovh>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> 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.
>
GPUs without resizable BARs tend to expose 256MiB on the BAR regardless
of the amount of internal VRAM, AFAIK. So IMO 256 MiB is not a totally
unmotivated number. I don't know which other PCIe devices might expose
such big BARs for comparison.

Cheers,
Alejandro

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

[Roger Pau Monné]

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.
> 
> Reported-by: Neowutran <xen@neowutran.ovh>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> 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 maybe name this `BAR_RELOCATE_THRESH, `HUGE_BAR` is too
generic IMO.

And also use 256Mb instead of 1GB, but just having a limit is good
enough, we can further tune it afterwards.

> +
>  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");
> @@ -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);

There's a comment above this that starts with:

"Relocate to high memory if the total amount of MMIO needed is more
than the low MMIO available."

I would slightly reword it to:

"Relocate to high memory if the total amount of MMIO needed is more
than the low MMIO available or BARs bigger that HUGE_BAR_THRESH are
present."

Thanks, Roger.

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

[Jan Beulich]

On 05.03.2024 10:31, Roger Pau Monné wrote:
> 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.
>>
>> Reported-by: Neowutran <xen@neowutran.ovh>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> --- 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 maybe name this `BAR_RELOCATE_THRESH, `HUGE_BAR` is too
> generic IMO.

BAR_RELOC_THRESH it is then.

> And also use 256Mb instead of 1GB, but just having a limit is good
> enough, we can further tune it afterwards.

As indicated in an earlier reply, I now firmly think that if we want to
do so, it wants doing separately / incrementally.

>> @@ -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);
> 
> There's a comment above this that starts with:
> 
> "Relocate to high memory if the total amount of MMIO needed is more
> than the low MMIO available."
> 
> I would slightly reword it to:
> 
> "Relocate to high memory if the total amount of MMIO needed is more
> than the low MMIO available or BARs bigger that HUGE_BAR_THRESH are
> present."

Hmm, yes, done.

Jan