From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49375) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b3UUJ-00084w-Bg for qemu-devel@nongnu.org; Thu, 19 May 2016 16:24:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b3UUD-000556-A8 for qemu-devel@nongnu.org; Thu, 19 May 2016 16:24:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52671) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b3UUD-00054t-1k for qemu-devel@nongnu.org; Thu, 19 May 2016 16:24:01 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C13EE63172 for ; Thu, 19 May 2016 20:23:59 +0000 (UTC) References: <1463340214-8721-1-git-send-email-marcel@redhat.com> <20160518155308.34cb52b7@nial.brq.redhat.com> <573C7AB3.9010308@redhat.com> <20160519110447.7fb0e2da@nial.brq.redhat.com> From: Marcel Apfelbaum Message-ID: <573E20D7.10801@redhat.com> Date: Thu, 19 May 2016 23:23:51 +0300 MIME-Version: 1.0 In-Reply-To: <20160519110447.7fb0e2da@nial.brq.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V2 0/4] pci: better support for 64-bit MMIO allocation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: mst@redhat.com, lersek@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com On 05/19/2016 12:04 PM, Igor Mammedov wrote: > On Wed, 18 May 2016 17:22:43 +0300 > Marcel Apfelbaum wrote: > >> On 05/18/2016 04:53 PM, Igor Mammedov wrote: >>> On Sun, 15 May 2016 22:23:30 +0300 >>> Marcel Apfelbaum wrote: >>> >>>> Hi, >>>> >>>> First two patches allocate (max_reserved_ram - max_addr_cpu_addressable) range for PCI hotplug >>>> (for PC Machines) instead of the previous 64-bit PCI window that included only >>>> the ranges allocated by the firmware. >>>> >>>> The next two patches fix 64-bit CRS computations. >>> I'd would add test case + expected tables as the first 2 patches >>> and then finish series with expected tables update with fixed 64bit range >>> >>> as experiment I've hacked existing piix4 case: >>> >>> @@ -744,7 +744,9 @@ static void test_acpi_piix4_tcg(void) >>> */ >>> memset(&data, 0, sizeof(data)); >>> data.machine = MACHINE_PC; >>> - test_acpi_one("-machine accel=tcg", &data); >>> + test_acpi_one("-machine accel=tcg" >>> + " -device pxb,id=bridge1,bus=pci.0,bus_nr=4" >>> + " -device ivshmem,bus=bridge1,size=4G,shm", &data); >>> free_test_data(&data); >>> } >>> >>> And it shows not related to this series, but another pxb issue >>> >>> + External (_SB_.PCI0.S18_.PCNT, MethodObj) // Warning: Unresolved method, guessing 0 arguments >>> ... >>> @@ -1197,8 +1322,8 @@ DefinitionBlock ("tests/acpi-test-data/pc/DSDT.aml", "DSDT", 1, "BOCHS ", "BXPCD >>> >>> Device (S18) >>> { >>> - Name (_SUN, 0x03) // _SUN: Slot User Number >>> Name (_ADR, 0x00030000) // _ADR: Address >>> + Name (_SUN, 0x03) // _SUN: Slot User Number >>> Method (_EJ0, 1, NotSerialized) // _EJx: Eject Device >>> { >>> PCEJ (BSEL, _SUN) >>> @@ -1638,6 +1763,7 @@ DefinitionBlock ("tests/acpi-test-data/pc/DSDT.aml", "DSDT", 1, "BOCHS ", "BXPCD >>> BNUM = Zero >>> DVNT (PCIU, One) >>> DVNT (PCID, 0x03) >>> + ^S18.PCNT () >>> } >>> } >>> } >>> >>> so it's better to have test case in place so that changes to pxb >>> parts wouldn't go unnoticed and would be observable. >>> >> I'll add the test, thanks, and also I'll look for the PXB warning. >> >>> >>> Also from above experiment I see that pxb duplicates and uses >>> the same _PRT method as PCI0, probably should be changed to >>> something like: >>> >>> Method(_PRT) >>> return ^PCI0._PRT() >>> >> >> Their PRT are not exactly the same, please see build_prt in hw/i386/acpi-build.c . > build_prt() can generate shared AML if is_pci0_prt > would be checked in AML, something like this: > > @@@ -667,13 -684,13 +667,13 @@@ static Aml *build_prt(bool is_pci0_prt > > /* route[2] = "LNK[D|A|B|C]", selection based on pin % 3 */ > aml_append(while_ctx, initialize_route(route, "LNKD", lnk_idx, 0)); > -- if (is_pci0_prt) { > ++ { > Aml *if_device_1, *if_pin_4, *else_pin_4; > > /* device 1 is the power-management device, needs SCI */ > if_device_1 = aml_if(aml_equal(lnk_idx, aml_int(1))); > { > -- if_pin_4 = aml_if(aml_equal(pin, aml_int(4))); > ++ if_pin_4 = aml_if(aml_and(aml_equal(pin, aml_int(4)), is_pci0_prt, NULL)); > { > aml_append(if_pin_4, > aml_store(build_prt_entry("LNKS"), route)); > @@@ -687,8 -704,8 +687,6 @@@ > aml_append(if_device_1, else_pin_4); > } > aml_append(while_ctx, if_device_1); > -- } else { > -- aml_append(while_ctx, initialize_route(route, "LNKA", lnk_idx, 1)); > } > aml_append(while_ctx, initialize_route(route, "LNKB", lnk_idx, 2)); > aml_append(while_ctx, initialize_route(route, "LNKC", lnk_idx, 3)); > > > as is_pci0_prt key, it's probably possible to use _UID == 0 condition > or make a wrapper method per bus that explicitly will tell > common_prt() if it's root bus or not. > Thanks, I'll give a try (separate patch, of course) Marcel >> >> Thanks, >> Marcel >> >>>> v1 -> v2: >>>> - resolved some styling issues (Laszlo) >>>> - rebase on latest master (Laszlo) >>>> >>>> Thank you, >>>> Marcel >>>> >>>> Marcel Apfelbaum (4): >>>> hw/pc: extract reserved memory end computation to a standalone >>>> function >>>> pci: reserve 64 bit MMIO range for PCI hotplug >>>> acpi: refactor pxb crs computation >>>> hw/apci: handle 64-bit MMIO regions correctly >>>> >>>> hw/i386/acpi-build.c | 127 ++++++++++++++++++++++++++++++++++++--------------- >>>> hw/i386/pc.c | 29 ++++++++---- >>>> hw/pci/pci.c | 16 ++++++- >>>> include/hw/i386/pc.h | 1 + >>>> 4 files changed, 127 insertions(+), 46 deletions(-) >>>> >>> >> >> >