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

[miaoyubo <miaoyubo@huawei.com>]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Tuesday, February 25, 2020 9:15 PM
> To: miaoyubo <miaoyubo@huawei.com>
> Cc: peter.maydell@linaro.org; shannon.zhaosl@gmail.com; Xiexiangyou
> <xiexiangyou@huawei.com>; imammedo@redhat.com; qemu-
> devel@nongnu.org; berrange@redhat.com
> Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
> 
> On Tue, Feb 25, 2020 at 09:50:25AM +0800, Yubo Miao wrote:
> > From: miaoyubo <miaoyubo@huawei.com>
> >
> > 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 <miaoyubo@huawei.com>
> > ---
> >  hw/arm/virt-acpi-build.c | 115
> ++++++++++++++++++++++++++++++++++++---
> >  hw/arm/virt.c            |   3 +
> >  include/hw/arm/virt.h    |   7 +++
> >  3 files changed, 118 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index
> > 37c34748a6..be1986c60d 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) @@ -266,19
> > +268,116 @@ 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, *crs, *dev;
> > +    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
> 
> It's not clear where does pxb come in here.
> 

Thanks for replying, I would add comments when pxb comes.

> > +     * 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;
> 
> Please explain how are memory and io partitioned.
> Looks like you are forcing a very specific layout here, but it's not documented
> anywhere, which can cause issues like running out of memory where we
> previously were ok.
> 
> 

The resources allocated for pxb is  in the end of the resources used to
be allocated for pci host bridge. The memory length for each pxb-pcie is
0x600000 and the io is 16k. The resources allocated for pxbs are quiet small,
Therefore, the resources for the main host bridge should be large enough.
 
For piix4, the memory resource length for each PXB is  0x200000, but
In arm, 200000 is not enough, if one nvme is plugged on pcie-root-port and plug
on the pxb, 600000 is enough for the memory, therefore, the memory length are defined
as 0x600000. 
 
Just using 0x600000 and 0x4000 is not so decent, 
I will try to find if there's a better way to allocate the resources.

> >      int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
> > +    PCIBus *bus = VIRT_MACHINE(vms)->bus;
> > +
> > +    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;
> > +            }
> > +            /*
> > +             * Coded up the MIN of the busNr defined for pxb-pcie,
> > +             * the MIN - 1 would be the MAX bus number for the main
> > +             * host bridge.
> > +             */
> > +            if (bus_num < nr_pcie_buses) {
> > +                nr_pcie_buses = bus_num;
> > +            }
> > +            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")));
> 
> how do you know it's a pxb device?
> 

if (!pci_bus_is_root(bus)) 
continue;
Just go through the children of bus.0, if it is root bus, then it is a 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);
> > +            }
> > +
> > +            acpi_dsdt_add_pci_route_table(dev, scope, irq);
> > +
> > +            method = aml_method("_CBA", 0, AML_NOTSERIALIZED);
> > +            aml_append(method, aml_return(aml_int(base_ecam)));
> > +            aml_append(dev, method);
> > +
> > +            method = aml_method("_CRS", 0, AML_NOTSERIALIZED);
> > +            Aml *rbuf = aml_resource_template();
> > +            aml_append(rbuf,
> > +                       aml_word_bus_number(AML_MIN_FIXED, AML_MAX_FIXED,
> > +                                           AML_POS_DECODE, 0x0000,
> > +                                           bus_num, bus_num + 1, 0x0000,
> > +                                           2));
> > +            aml_append(rbuf,
> > +                       aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
> > +                                        AML_MAX_FIXED, AML_NON_CACHEABLE,
> > +                                        AML_READ_WRITE, 0x0000,
> > +                                        base_mmio + size_mmio -
> > +                                        size_addr * count,
> > +                                        base_mmio + size_mmio - 1 -
> > +                                        size_addr * (count - 1),
> > +                                        0x0000, size_addr));
> > +            aml_append(rbuf,
> > +                       aml_dword_io(AML_MIN_FIXED, AML_MAX_FIXED,
> > +                       AML_POS_DECODE, AML_ENTIRE_RANGE,
> > +                       0x0000, size_pio - size_io * count,
> > +                       size_pio - 1 - size_io * (count - 1),
> > +                       base_pio, size_io));
> > +
> > +            if (use_highmem) {
> > +                hwaddr base_mmio_high =
> memmap[VIRT_HIGH_PCIE_MMIO].base;
> > +                hwaddr size_mmio_high =
> > + memmap[VIRT_HIGH_PCIE_MMIO].size;
> > +
> > +                aml_append(rbuf,
> > +                       aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
> > +                                        AML_MAX_FIXED, AML_NON_CACHEABLE,
> > +                                        AML_READ_WRITE, 0x0000,
> > +                                        base_mmio_high + size_mmio_high -
> > +                                        size_addr * count,
> > +                                        base_mmio_high + size_mmio_high -
> > +                                        1 - size_addr * (count - 1),
> > +                                        0x0000, size_addr));
> > +            }
> > +
> > +            aml_append(method, aml_name_decl("RBUF", rbuf));
> > +            aml_append(method, aml_return(rbuf));
> > +            aml_append(dev, method);
> > +
> > +            acpi_dsdt_add_pci_osc(dev, scope);
> > +
> > +            aml_append(scope, dev);
> > +
> > +        }
> > +    }
> >
> > -    Aml *dev = aml_device("%s", "PCI0");
> > +    dev = aml_device("%s", "PCI0");
> >      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("_SEG", aml_int(0))); @@ -302,11
> > +401,13 @@ static void acpi_dsdt_add_pci(Aml *scope, const
> MemMapEntry *memmap,
> >      aml_append(rbuf,
> >          aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
> AML_MAX_FIXED,
> >                           AML_NON_CACHEABLE, AML_READ_WRITE, 0x0000,
> base_mmio,
> > -                         base_mmio + size_mmio - 1, 0x0000, size_mmio));
> > +                         base_mmio + size_mmio - 1 - size_addr * count,
> > +                         0x0000, size_mmio - size_addr * count));
> >      aml_append(rbuf,
> >          aml_dword_io(AML_MIN_FIXED, AML_MAX_FIXED,
> AML_POS_DECODE,
> > -                     AML_ENTIRE_RANGE, 0x0000, 0x0000, size_pio - 1, base_pio,
> > -                     size_pio));
> > +                     AML_ENTIRE_RANGE, 0x0000, 0x0000,
> > +                     size_pio - 1 - size_io * count, base_pio,
> > +                     size_pio - size_io * count));
> >
> >      if (use_highmem) {
> >          hwaddr base_mmio_high = memmap[VIRT_HIGH_PCIE_MMIO].base;
> @@
> > -746,7 +847,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
> >      acpi_dsdt_add_virtio(scope, &memmap[VIRT_MMIO],
> >                      (irqmap[VIRT_MMIO] + ARM_SPI_BASE),
> NUM_VIRTIO_TRANSPORTS);
> >      acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] +
> ARM_SPI_BASE),
> > -                      vms->highmem, vms->highmem_ecam);
> > +                      vms->highmem, vms->highmem_ecam, vms);
> >      if (vms->acpi_dev) {
> >          build_ged_aml(scope, "\\_SB."GED_DEVICE,
> >                        HOTPLUG_HANDLER(vms->acpi_dev), diff --git
> > a/hw/arm/virt.c b/hw/arm/virt.c index f788fe27d6..6314928671 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1246,6 +1246,9 @@ static void create_pcie(VirtMachineState *vms)
> >      }
> >
> >      pci = PCI_HOST_BRIDGE(dev);
> > +
> > +    VIRT_MACHINE(qdev_get_machine())->bus = pci->bus;
> > +
> >      if (pci->bus) {
> >          for (i = 0; i < nb_nics; i++) {
> >              NICInfo *nd = &nd_table[i]; diff --git
> > a/include/hw/arm/virt.h b/include/hw/arm/virt.h index
> > 71508bf40c..90f10a1e46 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;
> >  } VirtMachineState;
> >
> >  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
> > VIRT_PCIE_ECAM)
> > --
> > 2.19.1
> >