On Wed, 18 May 2016 17:07:05 +0300
Marcel Apfelbaum <mar...@redhat.com> wrote:
> On 05/16/2016 05:19 PM, Igor Mammedov wrote:
> > On Mon, 16 May 2016 13:14:51 +0300
> > Marcel Apfelbaum <mar...@redhat.com> wrote:
> >
> >> On 05/16/2016 11:24 AM, Igor Mammedov wrote:
> >>> On Sun, 15 May 2016 22:23:32 +0300
> >>> Marcel Apfelbaum <mar...@redhat.com> 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 <mar...@redhat.com>
> >>>> ---
> >>>> 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.
>
> 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)
> >>>
> >>
> >
>
