Re: [Xen-devel] [PATCH v1] hvmloader, pci: Don't try to relocate memory if 64-bit BAR is bigger than 4GB

[Jan Beulich]

>>> On 28.09.16 at 21:23,  wrote:
> .. snip..
>>> +{
>>> +/* If bigger than 4GB, don't try to put under 4GB. */
>>> +if ( is_64bar && bar_sz > (1ull<<32) )
>>
>> Clearly at the very least this should be >=. However, even when
>> it's a 2Gb BAR, we won't be able to fit it (as it can't go at address
>> zero, nor at address 0x8000, both for different reasons).
> 
> 
> The reason 2GB is also problematic is that we still have other things
> that need to live under 4GB region: ACPI, APIC, and BAR of the PCI
> platform device (16MB), VGA emulated device (32MB).
> 
> mmio_total counts all of those up, and the emulated devices are
> enumerated before the passthrough devices - so I can do:
> 
>  if ( is_64bar && bar_sz > (GB(2) - mmio_total - HVM_BELOW_4G_MMIO_LENGTH) )
> 
> or such?

Or such. Subtracting mmio_total is problematic, as that may cause
the expression to wrap through zero. And I also don't see why you
would want to subtract it in the first place: If the overall size is too
big, code elsewhere is responsible for taking care. Once removed
I'd then think you should once again use >= (even if it doesn't
really matter much, i.e. mostly as documentation of the intentions).

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v1] hvmloader, pci: Don't try to relocate memory if 64-bit BAR is bigger than 4GB

[Konrad Rzeszutek Wilk]

.. snip..
>> +{
>> +/* If bigger than 4GB, don't try to put under 4GB. */
>> +if ( is_64bar && bar_sz > (1ull<<32) )
>
> Clearly at the very least this should be >=. However, even when
> it's a 2Gb BAR, we won't be able to fit it (as it can't go at address
> zero, nor at address 0x8000, both for different reasons).


The reason 2GB is also problematic is that we still have other things
that need to live under 4GB region: ACPI, APIC, and BAR of the PCI
platform device (16MB), VGA emulated device (32MB).

mmio_total counts all of those up, and the emulated devices are
enumerated before the passthrough devices - so I can do:

 if ( is_64bar && bar_sz > (GB(2) - mmio_total - HVM_BELOW_4G_MMIO_LENGTH) )

or such?

Let me prep a patch to this effect and also another prereq that adds
MB(x) and GB(x) macros.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v1] hvmloader, pci: Don't try to relocate memory if 64-bit BAR is bigger than 4GB

[Jan Beulich]

>>> On 07.07.16 at 17:27,  wrote:
>> > @@ -451,7 +462,10 @@ void pci_setup(void)
>> >  resource = _resource;
>> >  bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
>> >  }
>> > -mmio_total -= bar_sz;
>> > +if ( bars[i].is_64bar && bar_sz > (1ull<<32) )
>> 
>> Now that you use this constant a second time, it clearly needs to
>> become a #define.
> 
> Do you have a preference on the ull? Most of the code has ull but there
> are two instances of ULL?

Not really. I kind of dislike both of them.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v1] hvmloader, pci: Don't try to relocate memory if 64-bit BAR is bigger than 4GB

[Konrad Rzeszutek Wilk]

> > +{
> > +/* If bigger than 4GB, don't try to put under 4GB. */
> > +if ( is_64bar && bar_sz > (1ull<<32) )
> 
> Clearly at the very least this should be >=. However, even when
> it's a 2Gb BAR, we won't be able to fit it (as it can't go at address
> zero, nor at address 0x8000, both for different reasons).


.. snip..
> > @@ -451,7 +462,10 @@ void pci_setup(void)
> >  resource = _resource;
> >  bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
> >  }
> > -mmio_total -= bar_sz;
> > +if ( bars[i].is_64bar && bar_sz > (1ull<<32) )
> 
> Now that you use this constant a second time, it clearly needs to
> become a #define.

Do you have a preference on the ull? Most of the code has ull but there
are two instances of ULL?

> 
> Jan
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v1] hvmloader, pci: Don't try to relocate memory if 64-bit BAR is bigger than 4GB

[Jan Beulich]

>>> On 06.07.16 at 20:16,  wrote:
> @@ -269,8 +269,19 @@ void pci_setup(void)
>  if ( ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
>PCI_BASE_ADDRESS_SPACE_MEMORY) ||
>   (bar_reg == PCI_ROM_ADDRESS) )
> -mmio_total += bar_sz;
> -

Please retain this blank line (below your addition).

> +{
> +/* If bigger than 4GB, don't try to put under 4GB. */
> +if ( is_64bar && bar_sz > (1ull<<32) )

Clearly at the very least this should be >=. However, even when
it's a 2Gb BAR, we won't be able to fit it (as it can't go at address
zero, nor at address 0x8000, both for different reasons).

> +{
> +mmio_64bit_total += bar_sz;
> +/*
> + * As this may not trigger now that mmio_total could be
> + * less than 2GB, so force it.
> + */
> +bar64_relocate = 1;
> +} else

Coding style.

> @@ -431,7 +442,7 @@ void pci_setup(void)
>   * 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));
> +&& (mmio_total + mmio_64bit_total > (mem_resource.max - 
> mem_resource.base));

Please retain consistent parenthesization: Either drop the ones
around the operands of - or add a pair around the operands of +.

> @@ -451,7 +462,10 @@ void pci_setup(void)
>  resource = _resource;
>  bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
>  }
> -mmio_total -= bar_sz;
> +if ( bars[i].is_64bar && bar_sz > (1ull<<32) )

Now that you use this constant a second time, it clearly needs to
become a #define.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v1] hvmloader, pci: Don't try to relocate memory if 64-bit BAR is bigger than 4GB

[Konrad Rzeszutek Wilk]

There is no point. We can try to balloon out the memory between
hvm_info->low_mem_pgend to pci_mem_end and we will never be able
have a hole big enough for 4GB MMIO.

Instead just let it go above 4GB in the 64-bit zone.

Note that prior to this patch the hvmloader would relocate as much
memory as it could under 4GB:
Low MMIO hole not large enough for all devices, relocating some BARs to 64-bit
Relocating 0x pages from 0e0001000 to 21000 for lowmem MMIO hole
Relocating 0x pages from 0d0002000 to 21000 for lowmem MMIO hole
Relocating 0x pages from 0c0003000 to 22fffe000 for lowmem MMIO hole
Relocating 0x pages from 0b0004000 to 23fffd000 for lowmem MMIO hole
Relocating 0x pages from 0a0005000 to 24fffc000 for lowmem MMIO hole
Relocating 0x pages from 090006000 to 25fffb000 for lowmem MMIO hole
Relocating 0x pages from 080007000 to 26fffa000 for lowmem MMIO hole
Relocating 0x7 pages from 08000 to 27fff9000 for lowmem MMIO hole

which is completely pointless.

Signed-off-by: Konrad Rzeszutek Wilk 
---
Cc: Jan Beulich 
Cc: Andrew Cooper 
---
 tools/firmware/hvmloader/pci.c | 26 --
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
index 4eb1a31..29c0a9a 100644
--- a/tools/firmware/hvmloader/pci.c
+++ b/tools/firmware/hvmloader/pci.c
@@ -80,7 +80,7 @@ void pci_setup(void)
 {
 uint8_t is_64bar, using_64bar, bar64_relocate = 0;
 uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
-uint64_t base, bar_sz, bar_sz_upper, mmio_total = 0;
+uint64_t base, bar_sz, bar_sz_upper, mmio_total = 0, mmio_64bit_total = 0;
 uint32_t vga_devfn = 256;
 uint16_t class, vendor_id, device_id;
 unsigned int bar, pin, link, isa_irq;
@@ -269,8 +269,19 @@ void pci_setup(void)
 if ( ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
   PCI_BASE_ADDRESS_SPACE_MEMORY) ||
  (bar_reg == PCI_ROM_ADDRESS) )
-mmio_total += bar_sz;
-
+{
+/* If bigger than 4GB, don't try to put under 4GB. */
+if ( is_64bar && bar_sz > (1ull<<32) )
+{
+mmio_64bit_total += bar_sz;
+/*
+ * As this may not trigger now that mmio_total could be
+ * less than 2GB, so force it.
+ */
+bar64_relocate = 1;
+} else
+mmio_total += bar_sz;
+}
 nr_bars++;
 
 /*The upper half is already calculated, skip it! */
@@ -349,7 +360,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");
@@ -431,7 +442,7 @@ void pci_setup(void)
  * 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));
+&& (mmio_total + mmio_64bit_total > (mem_resource.max - 
mem_resource.base));
 bar_data = pci_readl(devfn, bar_reg);
 
 if ( (bar_data & PCI_BASE_ADDRESS_SPACE) ==
@@ -451,7 +462,10 @@ void pci_setup(void)
 resource = _resource;
 bar_data &= ~PCI_BASE_ADDRESS_MEM_MASK;
 }
-mmio_total -= bar_sz;
+if ( bars[i].is_64bar && bar_sz > (1ull<<32) )
+mmio_64bit_total -= bar_sz;
+else
+mmio_total -= bar_sz;
 }
 else
 {
-- 
2.4.11


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel