qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI h


From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug
Date: Wed, 18 May 2016 17:07:05 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0

On 05/16/2016 05:19 PM, Igor Mammedov wrote:
On Mon, 16 May 2016 13:14:51 +0300
Marcel Apfelbaum <address@hidden> wrote:

On 05/16/2016 11:24 AM, Igor Mammedov wrote:
On Sun, 15 May 2016 22:23:32 +0300
Marcel Apfelbaum <address@hidden> 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 <address@hidden>
---
   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.

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)







reply via email to

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