From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57362) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afNXB-0003VK-Fq for qemu-devel@nongnu.org; Mon, 14 Mar 2016 04:07:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1afNX7-0008Mi-F3 for qemu-devel@nongnu.org; Mon, 14 Mar 2016 04:07:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56843) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afNX7-0008MW-82 for qemu-devel@nongnu.org; Mon, 14 Mar 2016 04:07:21 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id B7ADC3299 for ; Mon, 14 Mar 2016 08:07:20 +0000 (UTC) References: <1457919739-18437-1-git-send-email-lersek@redhat.com> From: Marcel Apfelbaum Message-ID: <56E67135.8070403@redhat.com> Date: Mon, 14 Mar 2016 10:07:17 +0200 MIME-Version: 1.0 In-Reply-To: <1457919739-18437-1-git-send-email-lersek@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed 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: Laszlo Ersek , qemu-devel@nongnu.org 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 > 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