From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59904) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afNml-0007UD-GA for qemu-devel@nongnu.org; Mon, 14 Mar 2016 04:23:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1afNmi-0003nH-6A for qemu-devel@nongnu.org; Mon, 14 Mar 2016 04:23:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59455) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afNmh-0003nC-Uc for qemu-devel@nongnu.org; Mon, 14 Mar 2016 04:23:28 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 5FDE951C for ; Mon, 14 Mar 2016 08:23:27 +0000 (UTC) References: <1457919739-18437-1-git-send-email-lersek@redhat.com> <56E67135.8070403@redhat.com> From: Laszlo Ersek Message-ID: <56E674FC.4060909@redhat.com> Date: Mon, 14 Mar 2016 09:23:24 +0100 MIME-Version: 1.0 In-Reply-To: <56E67135.8070403@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] hw/i386/acpi-build: place qword descriptors in bridge _CRS's when needed List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcel Apfelbaum , qemu-devel@nongnu.org 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 >> 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