[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] hw/i386/acpi-build: place qword descriptors in

From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH] hw/i386/acpi-build: place qword descriptors in bridge _CRS's when needed
Date: Mon, 14 Mar 2016 10:33:03 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0

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 <address@hidden>
Cc: Michael S. Tsirkin <address@hidden>
Signed-off-by: Laszlo Ersek <address@hidden>
   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,

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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]