Re: [Qemu-devel] [PATCH] hw/i386/acpi-build: place qword descriptors in bridge _CRS's when needed

2016-03-14 Thread Marcel Apfelbaum 

On 03/14/2016 10:23 AM, Laszlo Ersek wrote:

On 03/14/16 09:07, Marcel Apfelbaum wrote:

On 03/14/2016 03:42 AM, Laszlo Ersek wrote:

In build_crs(), the calculation & merging of the ranges already
happens in
64-bit, but the entry boundaries are silently truncated to 32-bit in the
call to aml_dword_memory(). Use aml_qword_memory() when necessary -- this
fixes 64-bit BARs behind PXBs.



Hi Laszlo,
Thanks for the patch.

Please see below some comments.



Cc: Marcel Apfelbaum 
Cc: Michael S. Tsirkin 
Signed-off-by: Laszlo Ersek 
---
   hw/i386/acpi-build.c | 24 ++--
   1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b88800883944..3157cc36db98 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -938,13 +938,25 @@ static Aml *build_crs(PCIHostState *host,

   crs_range_merge(host_mem_ranges);
   for (i = 0; i < host_mem_ranges->len; i++) {
+Aml *mem;
+uint64_t length;
+
   entry = g_ptr_array_index(host_mem_ranges, i);
-aml_append(crs,
-   aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
-AML_MAX_FIXED, AML_NON_CACHEABLE,
-AML_READ_WRITE,
-0, entry->base, entry->limit, 0,
-entry->limit - entry->base + 1));
+length = entry->limit - entry->base + 1;
+if (entry->limit <= UINT32_MAX && length <= UINT32_MAX) {


Why do we need to check the length if we've already checked the
entry->limit ?


For mathematical completeness :) When limit is <= UINT32_MAX, that
implies that base is also <= UINT32_MAX, so that's why I'm not checking
base. However, length = limit - base + 1, and in theory, it can mean
length = UINT32_MAX - 0 + 1, which doesn't fit in a uint32_t.


You got me there, while a machine with no IO/RAM under 4Gb would be interesting,
I will not interfere with your math skills :)



In other words, limit is inclusive but length is exclusive, so it can be
1 higher (when base is 0).

(And checking only length is also not sufficient, of course.)




+  mem = aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
+ AML_MAX_FIXED, AML_NON_CACHEABLE,
+ AML_READ_WRITE,
+ 0, entry->base, entry->limit, 0,
+ length);
+} else {
+  mem = aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
+ AML_MAX_FIXED, AML_NON_CACHEABLE,
+ AML_READ_WRITE,
+ 0, entry->base, entry->limit, 0,
+ length);
+}
+aml_append(crs, mem);
   crs_range_insert(mem_ranges, entry->base, entry->limit);
   }
   g_ptr_array_free(host_mem_ranges, true);



I think it is correct, but this also means the mem_ranges array can have
64-bit ranges =>
the 'crs_replace_with_free_ranges' call for mem_ranges is also incorrect
because
it assumes all the ranges are between [pci->w32.begin, pci->w32.end - 1].


Hm. :)


And of course this would also interfere with the crs building for pci->w64.
We can't assign all the [pci->w64.begin, pci->w64.end - 1] range to bus
0 anymore,
we need to take out the ranges used by pxbs. (same as we did for pci->w32)

Indeed, this is one of the pxb limitations, supporting only 32bit BARs


Ah! Now that was a question I considered, but I couldn't decide if it
was a known / by-design choice (or limitation), or just an oversight in
the code. I figured I'd ask with a patch. :)


and your patch is going in the right direction.

Do you want to continue it? I will not be available for one week, but I
can take care of it after that.


It seems to require a more complex patch than this, so I'd prefer to
leave it to you. (I have my hands full :))

Being 64-bit clean would be really nice, since the edk2 PCI host bridge
/ root bridge driver that OVMF uses really likes to allocate 64-bit BARs
high (unlike SeaBIOS which strives to keep everything low). Since many
devices have 64-bit capable BARs, they won't work behind PXBs (when
booting with OVMF) until this limitation is lifted.

But, as I said, I'd like to leave this to you, if you have time for it.
It is not urgent, just would be real good eventually.



Of course, I'll get to it as soon as I am back and CC you once is ready.
Thanks,
Marcel


Thanks!
Laszlo

Re: [Qemu-devel] [PATCH] hw/i386/acpi-build: place qword descriptors in bridge _CRS's when needed

2016-03-14 Thread Laszlo Ersek 
On 03/14/16 09:07, Marcel Apfelbaum wrote:
> On 03/14/2016 03:42 AM, Laszlo Ersek wrote:
>> In build_crs(), the calculation & merging of the ranges already
>> happens in
>> 64-bit, but the entry boundaries are silently truncated to 32-bit in the
>> call to aml_dword_memory(). Use aml_qword_memory() when necessary -- this
>> fixes 64-bit BARs behind PXBs.
>>
> 
> Hi Laszlo,
> Thanks for the patch.
> 
> Please see below some comments.
> 
> 
>> Cc: Marcel Apfelbaum 
>> Cc: Michael S. Tsirkin 
>> Signed-off-by: Laszlo Ersek 
>> ---
>>   hw/i386/acpi-build.c | 24 ++--
>>   1 file changed, 18 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index b88800883944..3157cc36db98 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -938,13 +938,25 @@ static Aml *build_crs(PCIHostState *host,
>>
>>   crs_range_merge(host_mem_ranges);
>>   for (i = 0; i < host_mem_ranges->len; i++) {
>> +Aml *mem;
>> +uint64_t length;
>> +
>>   entry = g_ptr_array_index(host_mem_ranges, i);
>> -aml_append(crs,
>> -   aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
>> -AML_MAX_FIXED, AML_NON_CACHEABLE,
>> -AML_READ_WRITE,
>> -0, entry->base, entry->limit, 0,
>> -entry->limit - entry->base + 1));
>> +length = entry->limit - entry->base + 1;
>> +if (entry->limit <= UINT32_MAX && length <= UINT32_MAX) {
> 
> Why do we need to check the length if we've already checked the
> entry->limit ?

For mathematical completeness :) When limit is <= UINT32_MAX, that
implies that base is also <= UINT32_MAX, so that's why I'm not checking
base. However, length = limit - base + 1, and in theory, it can mean
length = UINT32_MAX - 0 + 1, which doesn't fit in a uint32_t.

In other words, limit is inclusive but length is exclusive, so it can be
1 higher (when base is 0).

(And checking only length is also not sufficient, of course.)

> 
>> +  mem = aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
>> + AML_MAX_FIXED, AML_NON_CACHEABLE,
>> + AML_READ_WRITE,
>> + 0, entry->base, entry->limit, 0,
>> + length);
>> +} else {
>> +  mem = aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
>> + AML_MAX_FIXED, AML_NON_CACHEABLE,
>> + AML_READ_WRITE,
>> + 0, entry->base, entry->limit, 0,
>> + length);
>> +}
>> +aml_append(crs, mem);
>>   crs_range_insert(mem_ranges, entry->base, entry->limit);
>>   }
>>   g_ptr_array_free(host_mem_ranges, true);
>>
> 
> I think it is correct, but this also means the mem_ranges array can have
> 64-bit ranges =>
> the 'crs_replace_with_free_ranges' call for mem_ranges is also incorrect
> because
> it assumes all the ranges are between [pci->w32.begin, pci->w32.end - 1].

Hm. :)

> And of course this would also interfere with the crs building for pci->w64.
> We can't assign all the [pci->w64.begin, pci->w64.end - 1] range to bus
> 0 anymore,
> we need to take out the ranges used by pxbs. (same as we did for pci->w32)
> 
> Indeed, this is one of the pxb limitations, supporting only 32bit BARs

Ah! Now that was a question I considered, but I couldn't decide if it
was a known / by-design choice (or limitation), or just an oversight in
the code. I figured I'd ask with a patch. :)

> and your patch is going in the right direction.
> 
> Do you want to continue it? I will not be available for one week, but I
> can take care of it after that.

It seems to require a more complex patch than this, so I'd prefer to
leave it to you. (I have my hands full :))

Being 64-bit clean would be really nice, since the edk2 PCI host bridge
/ root bridge driver that OVMF uses really likes to allocate 64-bit BARs
high (unlike SeaBIOS which strives to keep everything low). Since many
devices have 64-bit capable BARs, they won't work behind PXBs (when
booting with OVMF) until this limitation is lifted.

But, as I said, I'd like to leave this to you, if you have time for it.
It is not urgent, just would be real good eventually.

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH] hw/i386/acpi-build: place qword descriptors in bridge _CRS's when needed

2016-03-14 Thread Marcel Apfelbaum 

On 03/14/2016 03:42 AM, Laszlo Ersek wrote:

In build_crs(), the calculation & merging of the ranges already happens in
64-bit, but the entry boundaries are silently truncated to 32-bit in the
call to aml_dword_memory(). Use aml_qword_memory() when necessary -- this
fixes 64-bit BARs behind PXBs.



Hi Laszlo,
Thanks for the patch.

Please see below some comments.



Cc: Marcel Apfelbaum 
Cc: Michael S. Tsirkin 
Signed-off-by: Laszlo Ersek 
---
  hw/i386/acpi-build.c | 24 ++--
  1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b88800883944..3157cc36db98 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -938,13 +938,25 @@ static Aml *build_crs(PCIHostState *host,

  crs_range_merge(host_mem_ranges);
  for (i = 0; i < host_mem_ranges->len; i++) {
+Aml *mem;
+uint64_t length;
+
  entry = g_ptr_array_index(host_mem_ranges, i);
-aml_append(crs,
-   aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
-AML_MAX_FIXED, AML_NON_CACHEABLE,
-AML_READ_WRITE,
-0, entry->base, entry->limit, 0,
-entry->limit - entry->base + 1));
+length = entry->limit - entry->base + 1;
+if (entry->limit <= UINT32_MAX && length <= UINT32_MAX) {


Why do we need to check the length if we've already checked the entry->limit ?


+  mem = aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
+ AML_MAX_FIXED, AML_NON_CACHEABLE,
+ AML_READ_WRITE,
+ 0, entry->base, entry->limit, 0,
+ length);
+} else {
+  mem = aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
+ AML_MAX_FIXED, AML_NON_CACHEABLE,
+ AML_READ_WRITE,
+ 0, entry->base, entry->limit, 0,
+ length);
+}
+aml_append(crs, mem);
  crs_range_insert(mem_ranges, entry->base, entry->limit);
  }
  g_ptr_array_free(host_mem_ranges, true);



I think it is correct, but this also means the mem_ranges array can have 64-bit 
ranges =>
the 'crs_replace_with_free_ranges' call for mem_ranges is also incorrect because
it assumes all the ranges are between [pci->w32.begin, pci->w32.end - 1].

And of course this would also interfere with the crs building for pci->w64.
We can't assign all the [pci->w64.begin, pci->w64.end - 1] range to bus 0 
anymore,
we need to take out the ranges used by pxbs. (same as we did for pci->w32)

Indeed, this is one of the pxb limitations, supporting only 32bit BARs and your
patch is going in the right direction.

Do you want to continue it? I will not be available for one week, but I can 
take care of it
after that.

Thanks!
Marcel

[Qemu-devel] [PATCH] hw/i386/acpi-build: place qword descriptors in bridge _CRS's when needed

2016-03-13 Thread Laszlo Ersek 
In build_crs(), the calculation & merging of the ranges already happens in
64-bit, but the entry boundaries are silently truncated to 32-bit in the
call to aml_dword_memory(). Use aml_qword_memory() when necessary -- this
fixes 64-bit BARs behind PXBs.

Cc: Marcel Apfelbaum 
Cc: Michael S. Tsirkin 
Signed-off-by: Laszlo Ersek 
---
 hw/i386/acpi-build.c | 24 ++--
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b88800883944..3157cc36db98 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -938,13 +938,25 @@ static Aml *build_crs(PCIHostState *host,
 
 crs_range_merge(host_mem_ranges);
 for (i = 0; i < host_mem_ranges->len; i++) {
+Aml *mem;
+uint64_t length;
+
 entry = g_ptr_array_index(host_mem_ranges, i);
-aml_append(crs,
-   aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
-AML_MAX_FIXED, AML_NON_CACHEABLE,
-AML_READ_WRITE,
-0, entry->base, entry->limit, 0,
-entry->limit - entry->base + 1));
+length = entry->limit - entry->base + 1;
+if (entry->limit <= UINT32_MAX && length <= UINT32_MAX) {
+  mem = aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
+ AML_MAX_FIXED, AML_NON_CACHEABLE,
+ AML_READ_WRITE,
+ 0, entry->base, entry->limit, 0,
+ length);
+} else {
+  mem = aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
+ AML_MAX_FIXED, AML_NON_CACHEABLE,
+ AML_READ_WRITE,
+ 0, entry->base, entry->limit, 0,
+ length);
+}
+aml_append(crs, mem);
 crs_range_insert(mem_ranges, entry->base, entry->limit);
 }
 g_ptr_array_free(host_mem_ranges, true);
-- 
1.8.3.1