RE: [RFC v3 2/3] acpi:pci-expender-bus: Add pxb support for arm

[miaoyubo]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:address@hidden]
> Sent: Friday, February 21, 2020 7:18 PM
> To: miaoyubo <address@hidden>
> Cc: address@hidden; address@hidden; Xiexiangyou
> <address@hidden>; address@hidden;
> address@hidden
> Subject: Re: [RFC v3 2/3] acpi:pci-expender-bus: Add pxb support for arm
> 
> On Fri, Feb 21, 2020 at 02:35:11PM +0800, Yubo Miao wrote:
> > From: miaoyubo <address@hidden>
> >
> > Currently virt machine is not supported by pxb-pcie, and only one main
> > host bridge described in ACPI tables.
> > In this patch,PXB-PCIE is supproted by arm and certain resource is
> > allocated for each pxb-pcie in acpi table.
> > The resource for the main host bridge is also reallocated.
> >
> > Signed-off-by: miaoyubo <address@hidden>
> > ---
> >  hw/arm/virt-acpi-build.c | 125
> +++++++++++++++++++++++++++++++++++----
> >  hw/pci-host/gpex.c       |   4 ++
> >  include/hw/arm/virt.h    |   7 +++
> >  3 files changed, 125 insertions(+), 11 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index
> > 0540234b8a..2c1e0d2aaa 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -49,6 +49,8 @@
> >  #include "kvm_arm.h"
> >  #include "migration/vmstate.h"
> >
> > +#include "hw/arm/virt.h"
> > +#include "hw/pci/pci_bus.h"
> >  #define ARM_SPI_BASE 32
> >
> >  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus) @@ -271,19
> > +273,117 @@ static void acpi_dsdt_add_pci_osc(Aml *dev, Aml *scope)  }
> >
> >  static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry
> *memmap,
> > -                              uint32_t irq, bool use_highmem, bool
> highmem_ecam)
> > +                              uint32_t irq, bool use_highmem, bool
> highmem_ecam,
> > +                              VirtMachineState *vms)
> >  {
> >      int ecam_id = VIRT_ECAM_ID(highmem_ecam);
> > -    Aml *method, *crs;
> > +    Aml *method, *dev, *crs;
> > +    int count = 0;
> >      hwaddr base_mmio = memmap[VIRT_PCIE_MMIO].base;
> >      hwaddr size_mmio = memmap[VIRT_PCIE_MMIO].size;
> >      hwaddr base_pio = memmap[VIRT_PCIE_PIO].base;
> >      hwaddr size_pio = memmap[VIRT_PCIE_PIO].size;
> >      hwaddr base_ecam = memmap[ecam_id].base;
> >      hwaddr size_ecam = memmap[ecam_id].size;
> > +    /*
> > +     * 0x600000 would be enough for pxb device
> > +     * if it is too small, there is no enough space
> > +     * for a pcie device plugged in a pcie-root port
> > +     */
> > +    hwaddr size_addr = 0x600000;
> > +    hwaddr size_io = 0x4000;
> >      int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
> > +    int root_bus_limit = 0xFF;
> 
> what's this?
> 

Thanks for replying.
This is used to define the bus numbers for the main host bridge, 
If no pxb-pcie is defined, the bus number for the main host bridge
would range form 0 to 255.

> > +    PCIBus *bus = NULL;
> > +    bus = VIRT_MACHINE(vms)->bus;
> 
> So just move assignment as part of declaration.
> 

Thanks for the suggestion!

> > +
> > +    if (bus) {
> > +        QLIST_FOREACH(bus, &bus->child, sibling) {
> > +            uint8_t bus_num = pci_bus_num(bus);
> > +            uint8_t numa_node = pci_bus_numa_node(bus);
> > +
> > +            if (!pci_bus_is_root(bus)) {
> > +                continue;
> > +            }
> > +            if (bus_num < root_bus_limit) {
> > +                root_bus_limit = bus_num - 1;
> 
> what is this doing? manually coded up MIN?
> 

This coded up the MIN of busNr defined in pxb-pcie devices, 
and the Min busNr-1 would be the MAX bus Number for the main host bridge.
For example, if one pxb-pcie with busNr 128(which is 80) defined, 
The bus for the main host bridge would be 0-7F, and the bus for pxb-pcie
would be 80-81(just allocate two buses,keep the same with X86).
If pxb-pcie is not defined, the bus for main host bridge would be 0-FF.

> > +            }
> > +            count++;
> > +            dev = aml_device("PC%.02X", bus_num);
> > +            aml_append(dev, aml_name_decl("_HID",
> aml_string("PNP0A08")));
> > +            aml_append(dev, aml_name_decl("_CID",
> aml_string("PNP0A03")));
> > +            aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > +            aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
> > +            aml_append(dev, aml_name_decl("_SEG", aml_int(0)));
> > +            aml_append(dev, aml_name_decl("_BBN",
> aml_int(bus_num)));
> > +            aml_append(dev, aml_name_decl("_UID",
> aml_int(bus_num)));
> > +            aml_append(dev, aml_name_decl("_STR", aml_unicode("pxb
> Device")));
> > +            if (numa_node != NUMA_NODE_UNASSIGNED) {
> > +                method = aml_method("_PXM", 0,
> AML_NOTSERIALIZED);
> > +                aml_append(method,
> aml_return(aml_int(numa_node)));
> > +                aml_append(dev, method);
> > +            }
> > +
> >   * GPEX host
> > @@ -98,6 +99,9 @@ static void gpex_host_realize(DeviceState *dev, Error
> **errp)
> >                                       pci_swizzle_map_irq_fn, s,
> &s->io_mmio,
> >                                       &s->io_ioport, 0, 4,
> > TYPE_PCIE_BUS);
> >
> > +#ifdef __aarch64__
> > +    VIRT_MACHINE(qdev_get_machine())->bus = pci->bus; #endif
> 
> I'm repeating myself but - what does this have to do with building on
> __aarch64__?
> 

I would move this into machvirt_init, therefore, the __aarch64__ is not needed.

> >      qdev_set_parent_bus(DEVICE(&s->gpex_root), BUS(pci->bus));
> >      pci_bus_set_route_irq_fn(pci->bus, gpex_route_intx_pin_to_irq);
> >      qdev_init_nofail(DEVICE(&s->gpex_root));
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index
> > 71508bf40c..9accaf2b1b 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -140,6 +140,13 @@ typedef struct {
> >      DeviceState *gic;
> >      DeviceState *acpi_dev;
> >      Notifier powerdown_notifier;
> > +    /*
> > +     * pointer to devices and objects
> > +     * Via going through the bus, all
> > +     * pci devices and related objectes
> > +     * could be gained.
> > +     * */
> > +    PCIBus *bus;
> 
> That comment doesn't really tell me what this is.
> Is this the root bus?
> With an extender, don't we have lots of roots?
> 
> 

Yes, this is the root bus.(The pci.0 or pcie.0) 
The main root is pci.0 or pcie.0, the extender plays the role of root for 
devices 
plugged on it. For the extender, there is only one root,pci.0 or pcie.0.
But for devices like pcie-root-port, they could also regard pxb-pcie as root.
Hope this solve your question.

> >  } VirtMachineState;
> >
> >  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
> > VIRT_PCIE_ECAM)
> > --
> > 2.19.1
> >

Regards,
Miao