* [Qemu-devel] [PATCH] hw/i386/acpi-build: place qword descriptors in bridge _CRS's when needed
@ 2016-03-14 1:42 Laszlo Ersek
2016-03-14 8:07 ` Marcel Apfelbaum
0 siblings, 1 reply; 4+ messages in thread
From: Laszlo Ersek @ 2016-03-14 1:42 UTC (permalink / raw)
To: qemu-devel; +Cc: Marcel Apfelbaum, Michael S. Tsirkin
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 <marcel@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/i386/acpi-build: place qword descriptors in bridge _CRS's when needed
2016-03-14 1:42 [Qemu-devel] [PATCH] hw/i386/acpi-build: place qword descriptors in bridge _CRS's when needed Laszlo Ersek
@ 2016-03-14 8:07 ` Marcel Apfelbaum
2016-03-14 8:23 ` Laszlo Ersek
0 siblings, 1 reply; 4+ messages in thread
From: Marcel Apfelbaum @ 2016-03-14 8:07 UTC (permalink / raw)
To: Laszlo Ersek, qemu-devel; +Cc: Michael S. Tsirkin
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 <marcel@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/i386/acpi-build: place qword descriptors in bridge _CRS's when needed
2016-03-14 8:07 ` Marcel Apfelbaum
@ 2016-03-14 8:23 ` Laszlo Ersek
2016-03-14 8:33 ` Marcel Apfelbaum
0 siblings, 1 reply; 4+ messages in thread
From: Laszlo Ersek @ 2016-03-14 8:23 UTC (permalink / raw)
To: Marcel Apfelbaum, qemu-devel; +Cc: Michael S. Tsirkin
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 <marcel@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] hw/i386/acpi-build: place qword descriptors in bridge _CRS's when needed
2016-03-14 8:23 ` Laszlo Ersek
@ 2016-03-14 8:33 ` Marcel Apfelbaum
0 siblings, 0 replies; 4+ messages in thread
From: Marcel Apfelbaum @ 2016-03-14 8:33 UTC (permalink / raw)
To: Laszlo Ersek, qemu-devel; +Cc: Michael S. Tsirkin
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 <marcel@redhat.com>
>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>> 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-03-14 8:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-14 1:42 [Qemu-devel] [PATCH] hw/i386/acpi-build: place qword descriptors in bridge _CRS's when needed Laszlo Ersek
2016-03-14 8:07 ` Marcel Apfelbaum
2016-03-14 8:23 ` Laszlo Ersek
2016-03-14 8:33 ` Marcel Apfelbaum
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).