From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39354) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b32X8-0007ov-Ao for qemu-devel@nongnu.org; Wed, 18 May 2016 10:33:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b32X4-0003ur-3o for qemu-devel@nongnu.org; Wed, 18 May 2016 10:33:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40226) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b32X3-0003un-S9 for qemu-devel@nongnu.org; Wed, 18 May 2016 10:33:06 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (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 7414B61A11 for ; Wed, 18 May 2016 14:33:05 +0000 (UTC) References: <1463340214-8721-1-git-send-email-marcel@redhat.com> <1463340214-8721-3-git-send-email-marcel@redhat.com> <20160516102419.43e4b016@nial.brq.redhat.com> <57399D9B.50407@redhat.com> <20160516161933.7e0c58cc@nial.brq.redhat.com> <573C7709.7000109@redhat.com> <20160518162638.0a67a80f@nial.brq.redhat.com> From: Marcel Apfelbaum Message-ID: <573C7D1D.7050004@redhat.com> Date: Wed, 18 May 2016 17:33:01 +0300 MIME-Version: 1.0 In-Reply-To: <20160518162638.0a67a80f@nial.brq.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: qemu-devel@nongnu.org, mst@redhat.com, lersek@redhat.com, ehabkost@redhat.com, berrange@redhat.com On 05/18/2016 05:26 PM, Igor Mammedov wrote: > On Wed, 18 May 2016 17:07:05 +0300 > Marcel Apfelbaum wrote: > >> On 05/16/2016 05:19 PM, Igor Mammedov wrote: >>> On Mon, 16 May 2016 13:14:51 +0300 >>> Marcel Apfelbaum wrote: >>> >>>> On 05/16/2016 11:24 AM, Igor Mammedov wrote: >>>>> On Sun, 15 May 2016 22:23:32 +0300 >>>>> Marcel Apfelbaum wrote: >>>>> >>>>>> Using the firmware assigned MMIO ranges for 64-bit PCI window >>>>>> leads to zero space for hot-plugging PCI devices over 4G. >>>>>> >>>>>> PC machines can use the whole CPU addressable range after >>>>>> the space reserved for memory-hotplug. >>>>>> >>>>>> Signed-off-by: Marcel Apfelbaum >>>>>> --- >>>>>> hw/pci/pci.c | 16 ++++++++++++++-- >>>>>> 1 file changed, 14 insertions(+), 2 deletions(-) >>>>>> >>>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >>>>>> index bb605ef..44dd949 100644 >>>>>> --- a/hw/pci/pci.c >>>>>> +++ b/hw/pci/pci.c >>>>>> @@ -41,6 +41,7 @@ >>>>>> #include "hw/hotplug.h" >>>>>> #include "hw/boards.h" >>>>>> #include "qemu/cutils.h" >>>>>> +#include "hw/i386/pc.h" >>>>>> >>>>>> //#define DEBUG_PCI >>>>>> #ifdef DEBUG_PCI >>>>>> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque) >>>>>> >>>> >>>> >>>> Hi Igor, >>>> Thanks for reviewing this series. >>>> >>>>>> void pci_bus_get_w64_range(PCIBus *bus, Range *range) >>>>>> { >>>>>> - range->begin = range->end = 0; >>>>>> - pci_for_each_device_under_bus(bus, pci_dev_get_w64, range); >>>>>> + Object *machine = qdev_get_machine(); >>>>>> + if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) { >>>>>> + PCMachineState *pcms = PC_MACHINE(machine); >>>>>> + range->begin = pc_machine_get_reserved_memory_end(pcms); >>>>> that line should break linking on other targets which don't have >>>>> pc_machine_get_reserved_memory_end() >>>>> probably for got to add stub. >>>>> >>>> >>>> I cross-compiled all the targets with no problem. It seems no stub is required. >>> ./configure && make >>> >>> LINK aarch64-softmmu/qemu-system-aarch64 >>> ../hw/pci/pci.o: In function `pci_bus_get_w64_range': >>> /home/imammedo/builds/qemu/hw/pci/pci.c:2474: undefined reference to `pc_machine_get_reserved_memory_end' >>> collect2: error: ld returned 1 exit status >> >> Ooops, I did configured it to cross-compile everything, but I somehow missed it. >> I'll take care of it. >> >> >>> >>>> >>>> >>>>>> + if (!range->begin) { >>>>>> + range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size, >>>>>> + 1ULL << 30); >>>>>> + } >>> mayby move above hunk to pc_machine_get_reserved_memory_end() >>> so it would always return valid value. >>> >>>>>> + range->end = 1ULL << 40; /* 40 bits physical */ >>>>> x86 specific in generic code >>>>> >>>> >>>> I am aware I put x86 code in the generic pci code, but I limited it with >>>> if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) { >>>> So we have a generic rule for getting the w64 range and a specific one for PC machines. >>>> >>>> The alternative would be a w64 range per host-bridge, not bus. >>>> Adding a pci_bus_get_w64_range function to PCIHostBridgeClass, >>>> defaulting in current implementation for the base class and >>>> overriding it for piix/q35 looks OK to you? >>>> >>>> I thought about it, but it seemed over-engineered as opposed >>>> to the 'simple' if statement, however I can try it if you think is better. >>>> >>>>> ARM also has 64-bit PCI MMIO /git grep VIRT_PCIE_MMIO_HIGH/ >>>> >>>> I had a look and ARM does not use this infrastructure, it has its own abstractions, >>>> a memmap list. This is the other reason I selected this implementation, >>>> because is not really used by other targets (but it can be used in the future) >>> >>> if it's only for x86 and whatever was programmed by BIOS is ignored, >>> I'd drop PCI_HOST_PROP_PCI_HOLE64_*/pci_bus_get_w64_range() indirection >>> and just directly use pc_machine_get_reserved_memory_end() >>> from acpi-build.c >>> >>> in that case you won't need a stub for pc_machine_get_reserved_memory_end() >>> as its usage will be limited only to hw/i386 scope. >>> >> >> Good point, I'll try this. >> >>> Given opportunity, I'd drop PCI_HOST_PROP_PCI_HOLE* properties, >>> but for it we need ACK from libvirt side in case they are using it >>> for some reason. >> >> It seems out of the scope of this series, however I can do it on top. > it's just nice to have cleanup, but I don't insist on it. > > the thing here is that if pc_machine_get_reserved_memory_end() were used > directly for acpi-build.c then properties as is would give old/different values. You are right, I'll remove at least the 64-bit properties. Thanks, Marcel > >> >> Thanks, >> Marcel >> >>> >>>> >>>> >>>> Thanks, >>>> Marcel >>>> >>>>> perhaps range should be a property of PCI bus, >>>>> where a board sets its own values for start/size >>>>> >>>>>> + } else { >>>>>> + range->begin = range->end = 0; >>>>>> + pci_for_each_device_under_bus(bus, pci_dev_get_w64, range); >>>>>> + } >>>>>> } >>>>>> >>>>>> static bool pcie_has_upstream_port(PCIDevice *dev) >>>>> >>>> >>> >> >