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