All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 0/3] pci_expander_brdige:acpi:Support pxb-pcie for ARM
@ 2020-02-25  1:50 Yubo Miao
  2020-02-25  1:50 ` [PATCH v4 1/3] acpi:Extract two APIs from acpi_dsdt_add_pci Yubo Miao
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Yubo Miao @ 2020-02-25  1:50 UTC (permalink / raw)
  To: peter.maydell, shannon.zhaosl
  Cc: berrange, mst, qemu-devel, xiexiangyou, miaoyubo, imammedo

From: miaoyubo <miaoyubo@huawei.com>

Currently pxb-pcie is not supported by arm, the reason for it is
pxb-pcie is not described in DSDT table and only one main host bridge
is described in acpi tables, which means it is not impossible to
present different io numas for different devices, especially
host-passthrough devices.

This series of patches make arm to support PXB-PCIE.

Users can configure pxb-pcie with certain numa, Example command
is:

   -device pxb-pcie,id=pci.7,bus_nr=128,numa_node=0,bus=pcie.0,addr=0x9

Since the bus of pxb-pcie is root bus, devices could not be plugged
into pxb-pcie directly,pcie-root-port or pci-bridge should be defined
and plugged on pxb-pcie, then the device could be plugged onto the
pcie-root-port or pci-bridge.

With the patches,io numa could be presented to the guest by define a
pxb-pcie with the numa and plug the device on it.

miaoyubo (3):
  acpi:Extract two APIs from acpi_dsdt_add_pci
  acpi:pci-expender-bus: Add pxb support for arm
  ACPI/unit-test: Add a new test for pxb-pcie for arm

 hw/arm/virt-acpi-build.c                    | 232 +++++++++++++++-----
 hw/arm/virt.c                               |   3 +
 include/hw/arm/virt.h                       |   7 +
 tests/data/acpi/virt/DSDT.pxb               | Bin 0 -> 8048 bytes
 tests/qtest/bios-tables-test-allowed-diff.h |   1 +
 tests/qtest/bios-tables-test.c              |  54 ++++-
 6 files changed, 233 insertions(+), 64 deletions(-)
 create mode 100644 tests/data/acpi/virt/DSDT.pxb

-- 
2.19.1




^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v4 1/3] acpi:Extract two APIs from acpi_dsdt_add_pci
  2020-02-25  1:50 [PATCH v4 0/3] pci_expander_brdige:acpi:Support pxb-pcie for ARM Yubo Miao
@ 2020-02-25  1:50 ` Yubo Miao
  2020-02-25  1:50 ` [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm Yubo Miao
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 16+ messages in thread
From: Yubo Miao @ 2020-02-25  1:50 UTC (permalink / raw)
  To: peter.maydell, shannon.zhaosl
  Cc: berrange, mst, qemu-devel, xiexiangyou, miaoyubo, imammedo

From: miaoyubo <miaoyubo@huawei.com>

Extract two APIs acpi_dsdt_add_pci_route_table and
acpi_dsdt_add_pci_osc form acpi_dsdt_add_pci. The first
API is used to specify the pci route table and the second
API is used to declare the operation system capabilities.
These two APIs would be used to specify the pxb-pcie in DSDT.

Signed-off-by: miaoyubo <miaoyubo@huawei.com>
---
 hw/arm/virt-acpi-build.c | 129 ++++++++++++++++++++++-----------------
 1 file changed, 72 insertions(+), 57 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index fb4b166f82..37c34748a6 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -146,29 +146,11 @@ static void acpi_dsdt_add_virtio(Aml *scope,
     }
 }
 
-static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
-                              uint32_t irq, bool use_highmem, bool highmem_ecam)
+static void acpi_dsdt_add_pci_route_table(Aml *dev, Aml *scope,
+                                          uint32_t irq)
 {
-    int ecam_id = VIRT_ECAM_ID(highmem_ecam);
-    Aml *method, *crs, *ifctx, *UUID, *ifctx1, *elsectx, *buf;
     int i, slot_no;
-    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;
-    int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
-
-    Aml *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)));
-    aml_append(dev, aml_name_decl("_BBN", aml_int(0)));
-    aml_append(dev, aml_name_decl("_UID", aml_string("PCI0")));
-    aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0 Device")));
-    aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
-
+    Aml *method, *crs;
     /* Declare the PCI Routing Table. */
     Aml *rt_pkg = aml_varpackage(PCI_SLOT_MAX * PCI_NUM_PINS);
     for (slot_no = 0; slot_no < PCI_SLOT_MAX; slot_no++) {
@@ -204,41 +186,11 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
         aml_append(dev_gsi, method);
         aml_append(dev, dev_gsi);
     }
+}
 
-    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, 0x0000, nr_pcie_buses - 1, 0x0000,
-                            nr_pcie_buses));
-    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));
-    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));
-
-    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,
-                             base_mmio_high + size_mmio_high - 1, 0x0000,
-                             size_mmio_high));
-    }
-
-    aml_append(method, aml_return(rbuf));
-    aml_append(dev, method);
-
+static void acpi_dsdt_add_pci_osc(Aml *dev, Aml *scope)
+{
+    Aml *method, *UUID, *ifctx, *ifctx1, *elsectx, *buf;
     /* Declare an _OSC (OS Control Handoff) method */
     aml_append(dev, aml_name_decl("SUPP", aml_int(0)));
     aml_append(dev, aml_name_decl("CTRL", aml_int(0)));
@@ -246,7 +198,8 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
     aml_append(method,
         aml_create_dword_field(aml_arg(3), aml_int(0), "CDW1"));
 
-    /* PCI Firmware Specification 3.0
+    /*
+     * PCI Firmware Specification 3.0
      * 4.5.1. _OSC Interface for PCI Host Bridge Devices
      * The _OSC interface for a PCI/PCI-X/PCI Express hierarchy is
      * identified by the Universal Unique IDentifier (UUID)
@@ -291,7 +244,8 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
 
     method = aml_method("_DSM", 4, AML_NOTSERIALIZED);
 
-    /* PCI Firmware Specification 3.0
+    /*
+     * PCI Firmware Specification 3.0
      * 4.6.1. _DSM for PCI Express Slot Information
      * The UUID in _DSM in this context is
      * {E5C937D0-3553-4D7A-9117-EA4D19C3434D}
@@ -309,6 +263,67 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
     buf = aml_buffer(1, byte_list);
     aml_append(method, aml_return(buf));
     aml_append(dev, method);
+}
+
+static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
+                              uint32_t irq, bool use_highmem, bool highmem_ecam)
+{
+    int ecam_id = VIRT_ECAM_ID(highmem_ecam);
+    Aml *method, *crs;
+    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;
+    int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
+
+    Aml *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)));
+    aml_append(dev, aml_name_decl("_BBN", aml_int(0)));
+    aml_append(dev, aml_name_decl("_UID", aml_string("PCI0")));
+    aml_append(dev, aml_name_decl("_STR", aml_unicode("PCIe 0 Device")));
+    aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
+
+    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, 0x0000, nr_pcie_buses - 1, 0x0000,
+                            nr_pcie_buses));
+    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));
+    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));
+
+    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,
+                             base_mmio_high + size_mmio_high - 1, 0x0000,
+                             size_mmio_high));
+    }
+
+    aml_append(method, aml_return(rbuf));
+    aml_append(dev, method);
+
+    acpi_dsdt_add_pci_osc(dev, scope);
 
     Aml *dev_res0 = aml_device("%s", "RES0");
     aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));
-- 
2.19.1




^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
  2020-02-25  1:50 [PATCH v4 0/3] pci_expander_brdige:acpi:Support pxb-pcie for ARM Yubo Miao
  2020-02-25  1:50 ` [PATCH v4 1/3] acpi:Extract two APIs from acpi_dsdt_add_pci Yubo Miao
@ 2020-02-25  1:50 ` Yubo Miao
  2020-02-25  9:47   ` Philippe Mathieu-Daudé
  2020-02-25 13:14   ` Michael S. Tsirkin
  2020-02-25  1:50 ` [PATCH v4 3/3] ACPI/unit-test: Add a new test for pxb-pcie " Yubo Miao
  2020-02-25  2:23 ` [PATCH v4 0/3] pci_expander_brdige:acpi:Support pxb-pcie for ARM no-reply
  3 siblings, 2 replies; 16+ messages in thread
From: Yubo Miao @ 2020-02-25  1:50 UTC (permalink / raw)
  To: peter.maydell, shannon.zhaosl
  Cc: berrange, mst, qemu-devel, xiexiangyou, miaoyubo, imammedo

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
+     * 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;
+    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")));
+            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




^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v4 3/3] ACPI/unit-test: Add a new test for pxb-pcie for arm
  2020-02-25  1:50 [PATCH v4 0/3] pci_expander_brdige:acpi:Support pxb-pcie for ARM Yubo Miao
  2020-02-25  1:50 ` [PATCH v4 1/3] acpi:Extract two APIs from acpi_dsdt_add_pci Yubo Miao
  2020-02-25  1:50 ` [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm Yubo Miao
@ 2020-02-25  1:50 ` Yubo Miao
  2020-02-25 11:24   ` Michael S. Tsirkin
  2020-02-25  2:23 ` [PATCH v4 0/3] pci_expander_brdige:acpi:Support pxb-pcie for ARM no-reply
  3 siblings, 1 reply; 16+ messages in thread
From: Yubo Miao @ 2020-02-25  1:50 UTC (permalink / raw)
  To: peter.maydell, shannon.zhaosl
  Cc: berrange, mst, qemu-devel, xiexiangyou, miaoyubo, imammedo

From: miaoyubo <miaoyubo@huawei.com>

Currently, pxb-pcie could be defined by the cmdline like
    --device pxb-pcie,id=pci.9,bus_nr=128
However pxb-pcie is not described in acpi tables for arm.

The formal two patches support pxb-pcie for arm, escpcially the
specification for pxb-pcie in DSDT table.

Add a testcase to make sure the ACPI table is correct for guest.

The following table need to be added for this test:
    tests/data/acpi/virt/DSDT.pxb
Since the ASL diff has 1000+ lines, it would be presented in
commit log with the simply diff. the diff are:
    Device (PC80) is presented in DSDT.
    Resources allocated for Device (PCI0) is changed.

  * Disassembling to symbolic ASL+ operators
  *
- * Disassembly of /home/DSDT, Mon Feb 24 19:35:28 2020
+ * Disassembly of /home/DSDT.pxb, Mon Feb 24 19:33:38 2020
  *
  * Original Table Header:
  *     Signature        "DSDT"
- *     Length           0x000014BB (5307)
+ *     Length           0x00001F70 (8048)
  *     Revision         0x02
- *     Checksum         0xD1
+ *     Checksum         0xCF
  *     OEM ID           "BOCHS "
  *     OEM Table ID     "BXPCDSDT"
  *     OEM Revision     0x00000001 (1)
             })
         }

+        Device (PC80)
+        {
+            Name (_HID, "PNP0A08" /* PCI Express Bus */)  // _HID: Hardware ID
+            Name (_CID, "PNP0A03" /* PCI Bus */)  // _CID: Compatible ID
+            Name (_ADR, Zero)  // _ADR: Address
+            Name (_CCA, One)  // _CCA: Cache Coherency Attribute
+            Name (_SEG, Zero)  // _SEG: PCI Segment
+            Name (_BBN, 0x80)  // _BBN: BIOS Bus Number
+            Name (_UID, 0x80)  // _UID: Unique ID
+            Name (_STR, Unicode ("pxb Device"))  // _STR: Description String
+            Name (_PRT, Package (0x80)  // _PRT: PCI Routing Table
+            {
+                Package (0x04)
+                {
+                    0xFFFF,
+                    Zero,
+                    GSI0,
+                    Zero
+                },

Packages are omitted.

+
+                Package (0x04)
+                {
+                    0x001FFFFF,
+                    0x03,
+                    GSI2,
+                    Zero
+                }
+            })
+            Device (GSI0)
+            {
+                Name (_HID, "PNP0C0F" /* PCI Interrupt Link Device */)  // _HID: Hardware ID
+                Name (_UID, Zero)  // _UID: Unique ID
+                Name (_PRS, ResourceTemplate ()  // _PRS: Possible Resource Settings
+                {
+                    Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, ,, )
+                    {
+                        0x00000023,
+                    }
+                })
+                Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
+                {
+                    Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, ,, )
+                    {
+                        0x00000023,
+                    }
+                })
+                Method (_SRS, 1, NotSerialized)  // _SRS: Set Resource Settings
+                {
+                }
+            }
+

GSI1,2,3 are omitted.

+            Method (_CBA, 0, NotSerialized)  // _CBA: Configuration Base Address
+            {
+                Return (0x0000004010000000)
+            }
+
+            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
+            {
+                Name (RBUF, ResourceTemplate ()
+                {
+                    WordBusNumber (ResourceProducer, MinFixed, MaxFixed, PosDecode,
+                        0x0000,             // Granularity
+                        0x0080,             // Range Minimum
+                        0x0081,             // Range Maximum
+                        0x0000,             // Translation Offset
+                        0x0002,             // Length
+                        ,, )
+                    DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
+                        0x00000000,         // Granularity
+                        0x3E9F0000,         // Range Minimum
+                        0x3EFEFFFF,         // Range Maximum
+                        0x00000000,         // Translation Offset
+                        0x00600000,         // Length
+                        ,, , AddressRangeMemory, TypeStatic)
+                    DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
+                        0x00000000,         // Granularity
+                        0x0000C000,         // Range Minimum
+                        0x0000FFFF,         // Range Maximum
+                        0x3EFF0000,         // Translation Offset
+                        0x00004000,         // Length
+                        ,, , TypeStatic, DenseTranslation)
+                    QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
+                        0x0000000000000000, // Granularity
+                        0x000000FFFFA00000, // Range Minimum
+                        0x000000FFFFFFFFFF, // Range Maximum
+                        0x0000000000000000, // Translation Offset
+                        0x0000000000600000, // Length
+                        ,, , AddressRangeMemory, TypeStatic)
+                })
+                Return (ResourceTemplate ()
+                {
+                    WordBusNumber (ResourceProducer, MinFixed, MaxFixed, PosDecode,
+                        0x0000,             // Granularity
+                        0x0080,             // Range Minimum
+                        0x0081,             // Range Maximum
+                        0x0000,             // Translation Offset
+                        0x0002,             // Length
+                        ,, )
+                    DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
+                        0x00000000,         // Granularity
+                        0x3E9F0000,         // Range Minimum
+                        0x3EFEFFFF,         // Range Maximum
+                        0x00000000,         // Translation Offset
+                        0x00600000,         // Length
+                        ,, , AddressRangeMemory, TypeStatic)
+                    DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
+                        0x00000000,         // Granularity
+                        0x0000C000,         // Range Minimum
+                        0x0000FFFF,         // Range Maximum
+                        0x3EFF0000,         // Translation Offset
+                        0x00004000,         // Length
+                        ,, , TypeStatic, DenseTranslation)
+                    QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
+                        0x0000000000000000, // Granularity
+                        0x000000FFFFA00000, // Range Minimum
+                        0x000000FFFFFFFFFF, // Range Maximum
+                        0x0000000000000000, // Translation Offset
+                        0x0000000000600000, // Length
+                        ,, , AddressRangeMemory, TypeStatic)
+                })
+            }
+
+            Name (SUPP, Zero)
+            Name (CTRL, Zero)
+            Method (_OSC, 4, NotSerialized)  // _OSC: Operating System Capabilities
+            {
+                CreateDWordField (Arg3, Zero, CDW1)
+                If ((Arg0 == ToUUID ("33db4d5b-1ff7-401c-9657-7441c03dd766") /* PCI Host Bridge Device */))
+                {
+                    CreateDWordField (Arg3, 0x04, CDW2)
+                    CreateDWordField (Arg3, 0x08, CDW3)
+                    SUPP = CDW2 /* \_SB_.PC80._OSC.CDW2 */
+                    CTRL = CDW3 /* \_SB_.PC80._OSC.CDW3 */
+                    CTRL &= 0x1F
+                    If ((Arg1 != One))
+                    {
+                        CDW1 |= 0x08
+                    }
+
+                    If ((CDW3 != CTRL))
+                    {
+                        CDW1 |= 0x10
+                    }
+
+                    CDW3 = CTRL /* \_SB_.PC80.CTRL */
+                    Return (Arg3)
+                }
+                Else
+                {
+                    CDW1 |= 0x04
+                    Return (Arg3)
+                }
+            }
+
+            Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
+            {
+                If ((Arg0 == ToUUID ("e5c937d0-3553-4d7a-9117-ea4d19c3434d") /* Device Labeling Interface */))
+                {
+                    If ((Arg2 == Zero))
+                    {
+                        Return (Buffer (One)
+                        {
+                             0x01                                             // .
+                        })
+                    }
+                }
+
+                Return (Buffer (One)
+                {
+                     0x00                                             // .
+                })
+            }
+        }
+
         Device (PCI0)
         {
             Name (_HID, "PNP0A08" /* PCI Express Bus */)  // _HID: Hardware ID
                     WordBusNumber (ResourceProducer, MinFixed, MaxFixed, PosDecode,
                         0x0000,             // Granularity
                         0x0000,             // Range Minimum
-                        0x00FF,             // Range Maximum
+                        0x007F,             // Range Maximum
                         0x0000,             // Translation Offset
-                        0x0100,             // Length
+                        0x0080,             // Length
                         ,, )
                     DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
                         0x00000000,         // Granularity
                         0x10000000,         // Range Minimum
-                        0x3EFEFFFF,         // Range Maximum
+                        0x3E9EFFFF,         // Range Maximum
                         0x00000000,         // Translation Offset
-                        0x2EFF0000,         // Length
+                        0x2E9F0000,         // Length
                         ,, , AddressRangeMemory, TypeStatic)
                     DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
                         0x00000000,         // Granularity
                         0x00000000,         // Range Minimum
-                        0x0000FFFF,         // Range Maximum
+                        0x0000BFFF,         // Range Maximum
                         0x3EFF0000,         // Translation Offset
-                        0x00010000,         // Length
+                        0x0000C000,         // Length
                         ,, , TypeStatic, DenseTranslation)
                     QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
                         0x0000000000000000, // Granularity

Signed-off-by: miaoyubo <miaoyubo@huawei.com>
---
 tests/data/acpi/virt/DSDT.pxb               | Bin 0 -> 8048 bytes
 tests/qtest/bios-tables-test-allowed-diff.h |   1 +
 tests/qtest/bios-tables-test.c              |  54 +++++++++++++++++---
 3 files changed, 49 insertions(+), 6 deletions(-)
 create mode 100644 tests/data/acpi/virt/DSDT.pxb

diff --git a/tests/data/acpi/virt/DSDT.pxb b/tests/data/acpi/virt/DSDT.pxb
new file mode 100644
index 0000000000000000000000000000000000000000..6ac0b5212db49513c27ef50da838240826c2deb7
GIT binary patch
literal 8048
zcmeI1%WoT16o;=JC$Yzm*opISUQVG@JS#t%CM|7<J+_mSI5D22K}sb@AP^;+NK}YY
zA<`@glogG{k_IUYB>o60v0?=~7OdH@Mq<bCyK`rfbJDY8frM<S_MDk}zPX=!9sg!r
zb7~uVd8NWXSJ&-jXQujQ%dTNxNGYYa^=taeKSQ@VPPMarFm4wLg=lAarIxJNtrZk=
zD%#mxsj2OQtM+;`IM!}17YfCqe=ORu+fHWO8hsE`8W>Y)rM~D>q0l*0chOsHtU7-0
z>RNjpy|b)0W2O9FG^j|>QZ!THEg^Hd)0RlkGT+#;8>Z!rdUF%AFX1fCM#YMw$F7|1
z49CX&`Bfq}+kv<_TY*gQr1MnKQej`QKoM-h0YablDMVPEB8onus%KH6&H=&-ON<C%
zlqjPtGI}INA7PX!<C4gTNQ@|9T%wFwk<lwLEW((jjLRaUPhunp<1%H;iHv@Uks^#a
z%9s}!QHhZzjCsnqA~IqUV~{Ye5Jr)Gj84{vB{8yuag{Q}Wy(J!!BQU~j8`c`UZ)9Z
z9%F=YjWXnQnv@vhgmIlR<aL^o7?XstKpFBn9grB)gz*|>$m=vMF=hy3kuv0UnvodK
z5JrVE<aIhIG0qc4l``aYIwUbJ5Qa?{C3&4@CC2lFQKJlboeoQk7YU<I8S*+Ekr*!%
zMuRfsbvi0BE)vEPWytGvOkxxWW0^AKb()hHCBj&t40)Z7ON>i|af34Cbvhw2E)&M<
zlp(LvNr^E}7^{>auhS`sag{Kdgi)5)>9oYSMi^_9A+OWC#8@DVb)R8>-?HcZd9>DA
zg-T%_xy42ZGgTQ?rPkK_yd2@wm#|%}u37tDIMXlAMmv+UM)uT4>Mb>+YU&;Jp}MW!
z#;d~MR(oS;^#f~vFdm7!u3B<d3d(GUuw7Jx3BBJ6qbLPM4~nuHOhyu}i&$sI`IYX%
zz?3=W^<ijG>Q3;zvB*hgtY7KQU?@V6J|l*DohWrDX5)?R@j8PU^eEjK4DB(U80vMR
z)SZ}-*NH@E#`=}-=nX|^@-2gsdiFBUUhkZNXRqlDCXbSO_AyU{U(6sFijYM;QBu!-
z=Go6Y`?)7d>KSF8QLi(2OAyMaCrauWW1fiZ=H2t&k{I_yNj)v*X)#ZPIO>U#dd8V&
zoO#B%Crav>V4ex)iO@$qQBuz&^Gq_&B=<x~JyXmx#XJ!bsV7S6Ilw#znCAfZL`gky
z*+kZDnt38rQcslBGs8SH%rnD1QBu!A<~hhb5kjdaO6obpJcpR)5cfn$J+sU+%RCWU
zsV7S6Im|qVnddO~L`gkInCA%dM98I{D5>Wt^BiTKqudiE^~B{R`Trbao(RR%6D9S`
zG0z<H%yCbY)N`D9jx$e$XzGcQdQLFU3FbM$JyBB6N#;4pJQ2F7Crau$#XP5&=M?uu
zNj;~T=QQ&~NT;4CsV8oSn7_^5w?m$J=D8<I<GFpX64C#a<^oQCE!%|#zE}0F5&iAz
zWKve#yXEn$_oa@n>ZkT)`#MhB3Hc~LGcXkH6j0Q80cG}`pl;|{`4N|)^c@4rdQM=O
zeVpY-UCzRt1IoG}u*^Q6l}&f-YDFF6Z>UNy;wig6px+P`ZC5uNndN@#&gV%L#-DYy
zukPbHtUkl1EP<4vACDJY_iufHhR2RR5=tNo;PFH$gcm&A1+}Qu!<SBb^xh-ypsOFc
zJbCPRTX)s*e|F}mv)O8?sJ+o%RoO<Yv+mfDhr7zIZ55A}`w!pC3|Z;z=5OV{@{@~S
zZ{4Xpx&G_B4|lBy29)p`#cO#_7n-9t${KY~KUsPGe3X84csuk7jugGo#iNl&hru|r
zJFO4y>A`yc@$R=NvtOU2c)Xi^rBid7k)v~FHowgM^t&_rar3LuKbpB8?dDPJ@O`|2
zJ)w|(!Y#d}?^-tfJV>GSB&eECN>X=VQ)kt@HQ7?OT0uUYQ3d=vn^;ZM?dH}X&vkRF
zzSijGR@d$3R=pCsxz%;MxmB-(Zf<qmZf+Hlx%KaJYsCxlXOg2*pCVE2v&^c}uKSIj
zSsz~{nH3}5A9MgkW_|K>X4M>>YR_g?v;VI8A123Uq+Jhw4K~*q`e#~wy;QaeXEVLI
ze!#nvU&Q-F9=KOxep{~g-@2%Oy<V!<I%W}=pRSAgwkfX9k~rY=%{5w2=mpcdUvJ}1
zDy~AJF)OltKdwu=?$lRjvQwS<&5hWeE_RO6hxc|YeXsICYpY$g_O}mmdV{Vw+nh`N
E0}->6TL1t6

literal 0
HcmV?d00001

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..90c53925fc 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,2 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/virt/DSDT.pxb",
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index b4752c644c..91e91e0fec 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -620,12 +620,21 @@ static void test_acpi_one(const char *params, test_data *data)
          * TODO: convert '-drive if=pflash' to new syntax (see e33763be7cd3)
          * when arm/virt boad starts to support it.
          */
-        args = g_strdup_printf("-machine %s %s -accel tcg -nodefaults -nographic "
-            "-drive if=pflash,format=raw,file=%s,readonly "
-            "-drive if=pflash,format=raw,file=%s,snapshot=on -cdrom %s %s",
-            data->machine, data->tcg_only ? "" : "-accel kvm",
-            data->uefi_fl1, data->uefi_fl2, data->cd, params ? params : "");
-
+        if (data->cd) {
+            args = g_strdup_printf("-machine %s %s -accel tcg "
+                "-nodefaults -nographic "
+                "-drive if=pflash,format=raw,file=%s,readonly "
+                "-drive if=pflash,format=raw,file=%s,snapshot=on -cdrom %s %s",
+                data->machine, data->tcg_only ? "" : "-accel kvm",
+                data->uefi_fl1, data->uefi_fl2, data->cd, params ? params : "");
+        } else {
+            args = g_strdup_printf("-machine %s %s -accel tcg "
+                "-nodefaults -nographic "
+                "-drive if=pflash,format=raw,file=%s,readonly "
+                "-drive if=pflash,format=raw,file=%s,snapshot=on %s",
+                data->machine, data->tcg_only ? "" : "-accel kvm",
+                data->uefi_fl1, data->uefi_fl2, params ? params : "");
+        }
     } else {
         /* Disable kernel irqchip to be able to override apic irq0. */
         args = g_strdup_printf("-machine %s,kernel-irqchip=off %s -accel tcg "
@@ -960,6 +969,38 @@ static void test_acpi_virt_tcg_numamem(void)
 
 }
 
+static void test_acpi_virt_tcg_pxb(void)
+{
+    test_data data = {
+        .machine = "virt",
+        .tcg_only = true,
+        .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
+        .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
+        .ram_start = 0x40000000ULL,
+        .scan_len = 128ULL * 1024 * 1024,
+    };
+    /*
+     * While using -cdrom, the cdrom would auto plugged into pxb-pcie,
+     * the reason is the bus of pxb-pcie is also root bus, it would lead
+     * to the error only PCI/PCIE bridge could plug onto pxb.
+     * Therefore,thr cdrom is defined and plugged onto the scsi controller
+     * to solve the conflicts.
+     */
+    data.variant = ".pxb";
+    test_acpi_one(" -device pcie-root-port,chassis=1,id=pci.1"
+                  " -device virtio-scsi-pci,id=scsi0,bus=pci.1"
+                  " -drive file="
+                  "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2,"
+                  "if=none,media=cdrom,id=drive-scsi0-0-0-1,readonly=on"
+                  " -device scsi-cd,bus=scsi0.0,scsi-id=0,"
+                  "drive=drive-scsi0-0-0-1,id=scsi0-0-0-1,bootindex=1"
+                  " -cpu cortex-a57"
+                  " -device pxb-pcie,bus_nr=128",
+                  &data);
+
+    free_test_data(&data);
+}
+
 static void test_acpi_tcg_acpi_hmat(const char *machine)
 {
     test_data data;
@@ -1052,6 +1093,7 @@ int main(int argc, char *argv[])
         qtest_add_func("acpi/virt", test_acpi_virt_tcg);
         qtest_add_func("acpi/virt/numamem", test_acpi_virt_tcg_numamem);
         qtest_add_func("acpi/virt/memhp", test_acpi_virt_tcg_memhp);
+        qtest_add_func("acpi/virt/pxb", test_acpi_virt_tcg_pxb);
     }
     ret = g_test_run();
     boot_sector_cleanup(disk);
-- 
2.19.1




^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 0/3] pci_expander_brdige:acpi:Support pxb-pcie for ARM
  2020-02-25  1:50 [PATCH v4 0/3] pci_expander_brdige:acpi:Support pxb-pcie for ARM Yubo Miao
                   ` (2 preceding siblings ...)
  2020-02-25  1:50 ` [PATCH v4 3/3] ACPI/unit-test: Add a new test for pxb-pcie " Yubo Miao
@ 2020-02-25  2:23 ` no-reply
  3 siblings, 0 replies; 16+ messages in thread
From: no-reply @ 2020-02-25  2:23 UTC (permalink / raw)
  To: miaoyubo
  Cc: peter.maydell, berrange, mst, qemu-devel, xiexiangyou,
	shannon.zhaosl, miaoyubo, imammedo

Patchew URL: https://patchew.org/QEMU/20200225015026.940-1-miaoyubo@huawei.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

Using expected file 'tests/data/acpi/virt/DSDT.memhp'
qemu-system-aarch64: -device pxb-pcie,bus_nr=128: 'pxb-pcie' is not a valid device model name
Broken pipe
ERROR - too few tests run (expected 4, got 3)
/tmp/qemu-test/src/tests/qtest/libqtest.c:166: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
make: *** [check-qtest-aarch64] Error 1
make: *** Waiting for unfinished jobs....
  TEST    check-qtest-x86_64: tests/qtest/vmgenid-test
Could not access KVM kernel module: No such file or directory
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=8b83d9f25d6b41c892c069c8b4dc05ad', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-yz4axvvx/src/docker-src.2020-02-24-21.09.30.8626:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=8b83d9f25d6b41c892c069c8b4dc05ad
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-yz4axvvx/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    14m1.499s
user    0m8.878s


The full log is available at
http://patchew.org/logs/20200225015026.940-1-miaoyubo@huawei.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
  2020-02-25  1:50 ` [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm Yubo Miao
@ 2020-02-25  9:47   ` Philippe Mathieu-Daudé
  2020-02-25 11:20     ` Michael S. Tsirkin
  2020-02-25 12:12     ` miaoyubo
  2020-02-25 13:14   ` Michael S. Tsirkin
  1 sibling, 2 replies; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-02-25  9:47 UTC (permalink / raw)
  To: Yubo Miao, peter.maydell, shannon.zhaosl
  Cc: imammedo, berrange, qemu-devel, xiexiangyou, mst

On 2/25/20 2:50 AM, 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

Typos: "expander" in subject and "supported" here.

> 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
> +     * 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;
> +    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")));
> +            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

Typo "objects", but I don't understand the comment well.

> +     * could be gained.
> +     */
> +    PCIBus *bus;
>   } VirtMachineState;
>   
>   #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
  2020-02-25  9:47   ` Philippe Mathieu-Daudé
@ 2020-02-25 11:20     ` Michael S. Tsirkin
  2020-02-25 12:12     ` miaoyubo
  1 sibling, 0 replies; 16+ messages in thread
From: Michael S. Tsirkin @ 2020-02-25 11:20 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: peter.maydell, berrange, qemu-devel, xiexiangyou, shannon.zhaosl,
	Yubo Miao, imammedo

On Tue, Feb 25, 2020 at 10:47:47AM +0100, Philippe Mathieu-Daudé wrote:
> On 2/25/20 2:50 AM, 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
> 
> Typos: "expander" in subject and "supported" here.
> 
> > 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
> > +     * 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;
> > +    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")));
> > +            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
> 
> Typo "objects", but I don't understand the comment well.

Yes, I don't understand what it says either.

> > +     * could be gained.
> > +     */
> > +    PCIBus *bus;
> >   } VirtMachineState;
> >   #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
> > 



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 3/3] ACPI/unit-test: Add a new test for pxb-pcie for arm
  2020-02-25  1:50 ` [PATCH v4 3/3] ACPI/unit-test: Add a new test for pxb-pcie " Yubo Miao
@ 2020-02-25 11:24   ` Michael S. Tsirkin
  2020-02-25 12:25     ` miaoyubo
  0 siblings, 1 reply; 16+ messages in thread
From: Michael S. Tsirkin @ 2020-02-25 11:24 UTC (permalink / raw)
  To: Yubo Miao
  Cc: peter.maydell, berrange, qemu-devel, xiexiangyou, shannon.zhaosl,
	imammedo

On Tue, Feb 25, 2020 at 09:50:26AM +0800, Yubo Miao wrote:
> From: miaoyubo <miaoyubo@huawei.com>
> 
> Currently, pxb-pcie could be defined by the cmdline like
>     --device pxb-pcie,id=pci.9,bus_nr=128
> However pxb-pcie is not described in acpi tables for arm.
> 
> The formal two patches support pxb-pcie for arm, escpcially the
> specification for pxb-pcie in DSDT table.


especially? Pls spell-check comments and commit log, it's
not hard to do.

> Add a testcase to make sure the ACPI table is correct for guest.
> 
> The following table need to be added for this test:
>     tests/data/acpi/virt/DSDT.pxb
> Since the ASL diff has 1000+ lines, it would be presented in
> commit log with the simply diff. the diff are:
>     Device (PC80) is presented in DSDT.
>     Resources allocated for Device (PCI0) is changed.
> 
>   * Disassembling to symbolic ASL+ operators
>   *
> - * Disassembly of /home/DSDT, Mon Feb 24 19:35:28 2020
> + * Disassembly of /home/DSDT.pxb, Mon Feb 24 19:33:38 2020
>   *
>   * Original Table Header:
>   *     Signature        "DSDT"
> - *     Length           0x000014BB (5307)
> + *     Length           0x00001F70 (8048)
>   *     Revision         0x02
> - *     Checksum         0xD1
> + *     Checksum         0xCF
>   *     OEM ID           "BOCHS "
>   *     OEM Table ID     "BXPCDSDT"
>   *     OEM Revision     0x00000001 (1)
>              })
>          }
> 
> +        Device (PC80)
> +        {
> +            Name (_HID, "PNP0A08" /* PCI Express Bus */)  // _HID: Hardware ID
> +            Name (_CID, "PNP0A03" /* PCI Bus */)  // _CID: Compatible ID
> +            Name (_ADR, Zero)  // _ADR: Address
> +            Name (_CCA, One)  // _CCA: Cache Coherency Attribute
> +            Name (_SEG, Zero)  // _SEG: PCI Segment
> +            Name (_BBN, 0x80)  // _BBN: BIOS Bus Number
> +            Name (_UID, 0x80)  // _UID: Unique ID
> +            Name (_STR, Unicode ("pxb Device"))  // _STR: Description String
> +            Name (_PRT, Package (0x80)  // _PRT: PCI Routing Table
> +            {
> +                Package (0x04)
> +                {
> +                    0xFFFF,
> +                    Zero,
> +                    GSI0,
> +                    Zero
> +                },
> 
> Packages are omitted.
> 
> +
> +                Package (0x04)
> +                {
> +                    0x001FFFFF,
> +                    0x03,
> +                    GSI2,
> +                    Zero
> +                }
> +            })
> +            Device (GSI0)
> +            {
> +                Name (_HID, "PNP0C0F" /* PCI Interrupt Link Device */)  // _HID: Hardware ID
> +                Name (_UID, Zero)  // _UID: Unique ID
> +                Name (_PRS, ResourceTemplate ()  // _PRS: Possible Resource Settings
> +                {
> +                    Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, ,, )
> +                    {
> +                        0x00000023,
> +                    }
> +                })
> +                Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
> +                {
> +                    Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, ,, )
> +                    {
> +                        0x00000023,
> +                    }
> +                })
> +                Method (_SRS, 1, NotSerialized)  // _SRS: Set Resource Settings
> +                {
> +                }
> +            }
> +
> 
> GSI1,2,3 are omitted.
> 
> +            Method (_CBA, 0, NotSerialized)  // _CBA: Configuration Base Address
> +            {
> +                Return (0x0000004010000000)
> +            }
> +
> +            Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
> +            {
> +                Name (RBUF, ResourceTemplate ()
> +                {
> +                    WordBusNumber (ResourceProducer, MinFixed, MaxFixed, PosDecode,
> +                        0x0000,             // Granularity
> +                        0x0080,             // Range Minimum
> +                        0x0081,             // Range Maximum
> +                        0x0000,             // Translation Offset
> +                        0x0002,             // Length
> +                        ,, )
> +                    DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
> +                        0x00000000,         // Granularity
> +                        0x3E9F0000,         // Range Minimum
> +                        0x3EFEFFFF,         // Range Maximum
> +                        0x00000000,         // Translation Offset
> +                        0x00600000,         // Length
> +                        ,, , AddressRangeMemory, TypeStatic)
> +                    DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
> +                        0x00000000,         // Granularity
> +                        0x0000C000,         // Range Minimum
> +                        0x0000FFFF,         // Range Maximum
> +                        0x3EFF0000,         // Translation Offset
> +                        0x00004000,         // Length
> +                        ,, , TypeStatic, DenseTranslation)
> +                    QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
> +                        0x0000000000000000, // Granularity
> +                        0x000000FFFFA00000, // Range Minimum
> +                        0x000000FFFFFFFFFF, // Range Maximum
> +                        0x0000000000000000, // Translation Offset
> +                        0x0000000000600000, // Length
> +                        ,, , AddressRangeMemory, TypeStatic)
> +                })
> +                Return (ResourceTemplate ()
> +                {
> +                    WordBusNumber (ResourceProducer, MinFixed, MaxFixed, PosDecode,
> +                        0x0000,             // Granularity
> +                        0x0080,             // Range Minimum
> +                        0x0081,             // Range Maximum
> +                        0x0000,             // Translation Offset
> +                        0x0002,             // Length
> +                        ,, )
> +                    DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
> +                        0x00000000,         // Granularity
> +                        0x3E9F0000,         // Range Minimum
> +                        0x3EFEFFFF,         // Range Maximum
> +                        0x00000000,         // Translation Offset
> +                        0x00600000,         // Length
> +                        ,, , AddressRangeMemory, TypeStatic)
> +                    DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
> +                        0x00000000,         // Granularity
> +                        0x0000C000,         // Range Minimum
> +                        0x0000FFFF,         // Range Maximum
> +                        0x3EFF0000,         // Translation Offset
> +                        0x00004000,         // Length
> +                        ,, , TypeStatic, DenseTranslation)
> +                    QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
> +                        0x0000000000000000, // Granularity
> +                        0x000000FFFFA00000, // Range Minimum
> +                        0x000000FFFFFFFFFF, // Range Maximum
> +                        0x0000000000000000, // Translation Offset
> +                        0x0000000000600000, // Length
> +                        ,, , AddressRangeMemory, TypeStatic)
> +                })
> +            }
> +
> +            Name (SUPP, Zero)
> +            Name (CTRL, Zero)
> +            Method (_OSC, 4, NotSerialized)  // _OSC: Operating System Capabilities
> +            {
> +                CreateDWordField (Arg3, Zero, CDW1)
> +                If ((Arg0 == ToUUID ("33db4d5b-1ff7-401c-9657-7441c03dd766") /* PCI Host Bridge Device */))
> +                {
> +                    CreateDWordField (Arg3, 0x04, CDW2)
> +                    CreateDWordField (Arg3, 0x08, CDW3)
> +                    SUPP = CDW2 /* \_SB_.PC80._OSC.CDW2 */
> +                    CTRL = CDW3 /* \_SB_.PC80._OSC.CDW3 */
> +                    CTRL &= 0x1F
> +                    If ((Arg1 != One))
> +                    {
> +                        CDW1 |= 0x08
> +                    }
> +
> +                    If ((CDW3 != CTRL))
> +                    {
> +                        CDW1 |= 0x10
> +                    }
> +
> +                    CDW3 = CTRL /* \_SB_.PC80.CTRL */
> +                    Return (Arg3)
> +                }
> +                Else
> +                {
> +                    CDW1 |= 0x04
> +                    Return (Arg3)
> +                }
> +            }
> +
> +            Method (_DSM, 4, NotSerialized)  // _DSM: Device-Specific Method
> +            {
> +                If ((Arg0 == ToUUID ("e5c937d0-3553-4d7a-9117-ea4d19c3434d") /* Device Labeling Interface */))
> +                {
> +                    If ((Arg2 == Zero))
> +                    {
> +                        Return (Buffer (One)
> +                        {
> +                             0x01                                             // .
> +                        })
> +                    }
> +                }
> +
> +                Return (Buffer (One)
> +                {
> +                     0x00                                             // .
> +                })
> +            }
> +        }
> +
>          Device (PCI0)
>          {
>              Name (_HID, "PNP0A08" /* PCI Express Bus */)  // _HID: Hardware ID
>                      WordBusNumber (ResourceProducer, MinFixed, MaxFixed, PosDecode,
>                          0x0000,             // Granularity
>                          0x0000,             // Range Minimum
> -                        0x00FF,             // Range Maximum
> +                        0x007F,             // Range Maximum
>                          0x0000,             // Translation Offset
> -                        0x0100,             // Length
> +                        0x0080,             // Length
>                          ,, )
>                      DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
>                          0x00000000,         // Granularity
>                          0x10000000,         // Range Minimum
> -                        0x3EFEFFFF,         // Range Maximum
> +                        0x3E9EFFFF,         // Range Maximum
>                          0x00000000,         // Translation Offset
> -                        0x2EFF0000,         // Length
> +                        0x2E9F0000,         // Length
>                          ,, , AddressRangeMemory, TypeStatic)
>                      DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange,
>                          0x00000000,         // Granularity
>                          0x00000000,         // Range Minimum
> -                        0x0000FFFF,         // Range Maximum
> +                        0x0000BFFF,         // Range Maximum
>                          0x3EFF0000,         // Translation Offset
> -                        0x00010000,         // Length
> +                        0x0000C000,         // Length
>                          ,, , TypeStatic, DenseTranslation)
>                      QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, NonCacheable, ReadWrite,
>                          0x0000000000000000, // Granularity
> 
> Signed-off-by: miaoyubo <miaoyubo@huawei.com>


Seems to fail in patchew.

> ---
>  tests/data/acpi/virt/DSDT.pxb               | Bin 0 -> 8048 bytes
>  tests/qtest/bios-tables-test-allowed-diff.h |   1 +
>  tests/qtest/bios-tables-test.c              |  54 +++++++++++++++++---
>  3 files changed, 49 insertions(+), 6 deletions(-)
>  create mode 100644 tests/data/acpi/virt/DSDT.pxb

This needs to be in a separate patch.
See instructions in ./tests/qtest/bios-tables-test.c.


> new file mode 100644
> index 0000000000000000000000000000000000000000..6ac0b5212db49513c27ef50da838240826c2deb7
> GIT binary patch
> literal 8048
> zcmeI1%WoT16o;=JC$Yzm*opISUQVG@JS#t%CM|7<J+_mSI5D22K}sb@AP^;+NK}YY
> zA<`@glogG{k_IUYB>o60v0?=~7OdH@Mq<bCyK`rfbJDY8frM<S_MDk}zPX=!9sg!r
> zb7~uVd8NWXSJ&-jXQujQ%dTNxNGYYa^=taeKSQ@VPPMarFm4wLg=lAarIxJNtrZk=
> zD%#mxsj2OQtM+;`IM!}17YfCqe=ORu+fHWO8hsE`8W>Y)rM~D>q0l*0chOsHtU7-0
> z>RNjpy|b)0W2O9FG^j|>QZ!THEg^Hd)0RlkGT+#;8>Z!rdUF%AFX1fCM#YMw$F7|1
> z49CX&`Bfq}+kv<_TY*gQr1MnKQej`QKoM-h0YablDMVPEB8onus%KH6&H=&-ON<C%
> zlqjPtGI}INA7PX!<C4gTNQ@|9T%wFwk<lwLEW((jjLRaUPhunp<1%H;iHv@Uks^#a
> z%9s}!QHhZzjCsnqA~IqUV~{Ye5Jr)Gj84{vB{8yuag{Q}Wy(J!!BQU~j8`c`UZ)9Z
> z9%F=YjWXnQnv@vhgmIlR<aL^o7?XstKpFBn9grB)gz*|>$m=vMF=hy3kuv0UnvodK
> z5JrVE<aIhIG0qc4l``aYIwUbJ5Qa?{C3&4@CC2lFQKJlboeoQk7YU<I8S*+Ekr*!%
> zMuRfsbvi0BE)vEPWytGvOkxxWW0^AKb()hHCBj&t40)Z7ON>i|af34Cbvhw2E)&M<
> zlp(LvNr^E}7^{>auhS`sag{Kdgi)5)>9oYSMi^_9A+OWC#8@DVb)R8>-?HcZd9>DA
> zg-T%_xy42ZGgTQ?rPkK_yd2@wm#|%}u37tDIMXlAMmv+UM)uT4>Mb>+YU&;Jp}MW!
> z#;d~MR(oS;^#f~vFdm7!u3B<d3d(GUuw7Jx3BBJ6qbLPM4~nuHOhyu}i&$sI`IYX%
> zz?3=W^<ijG>Q3;zvB*hgtY7KQU?@V6J|l*DohWrDX5)?R@j8PU^eEjK4DB(U80vMR
> z)SZ}-*NH@E#`=}-=nX|^@-2gsdiFBUUhkZNXRqlDCXbSO_AyU{U(6sFijYM;QBu!-
> z=Go6Y`?)7d>KSF8QLi(2OAyMaCrauWW1fiZ=H2t&k{I_yNj)v*X)#ZPIO>U#dd8V&
> zoO#B%Crav>V4ex)iO@$qQBuz&^Gq_&B=<x~JyXmx#XJ!bsV7S6Ilw#znCAfZL`gky
> z*+kZDnt38rQcslBGs8SH%rnD1QBu!A<~hhb5kjdaO6obpJcpR)5cfn$J+sU+%RCWU
> zsV7S6Im|qVnddO~L`gkInCA%dM98I{D5>Wt^BiTKqudiE^~B{R`Trbao(RR%6D9S`
> zG0z<H%yCbY)N`D9jx$e$XzGcQdQLFU3FbM$JyBB6N#;4pJQ2F7Crau$#XP5&=M?uu
> zNj;~T=QQ&~NT;4CsV8oSn7_^5w?m$J=D8<I<GFpX64C#a<^oQCE!%|#zE}0F5&iAz
> zWKve#yXEn$_oa@n>ZkT)`#MhB3Hc~LGcXkH6j0Q80cG}`pl;|{`4N|)^c@4rdQM=O
> zeVpY-UCzRt1IoG}u*^Q6l}&f-YDFF6Z>UNy;wig6px+P`ZC5uNndN@#&gV%L#-DYy
> zukPbHtUkl1EP<4vACDJY_iufHhR2RR5=tNo;PFH$gcm&A1+}Qu!<SBb^xh-ypsOFc
> zJbCPRTX)s*e|F}mv)O8?sJ+o%RoO<Yv+mfDhr7zIZ55A}`w!pC3|Z;z=5OV{@{@~S
> zZ{4Xpx&G_B4|lBy29)p`#cO#_7n-9t${KY~KUsPGe3X84csuk7jugGo#iNl&hru|r
> zJFO4y>A`yc@$R=NvtOU2c)Xi^rBid7k)v~FHowgM^t&_rar3LuKbpB8?dDPJ@O`|2
> zJ)w|(!Y#d}?^-tfJV>GSB&eECN>X=VQ)kt@HQ7?OT0uUYQ3d=vn^;ZM?dH}X&vkRF
> zzSijGR@d$3R=pCsxz%;MxmB-(Zf<qmZf+Hlx%KaJYsCxlXOg2*pCVE2v&^c}uKSIj
> zSsz~{nH3}5A9MgkW_|K>X4M>>YR_g?v;VI8A123Uq+Jhw4K~*q`e#~wy;QaeXEVLI
> ze!#nvU&Q-F9=KOxep{~g-@2%Oy<V!<I%W}=pRSAgwkfX9k~rY=%{5w2=mpcdUvJ}1
> zDy~AJF)OltKdwu=?$lRjvQwS<&5hWeE_RO6hxc|YeXsICYpY$g_O}mmdV{Vw+nh`N
> E0}->6TL1t6
> 
> literal 0
> HcmV?d00001




> diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
> index dfb8523c8b..90c53925fc 100644
> --- a/tests/qtest/bios-tables-test-allowed-diff.h
> +++ b/tests/qtest/bios-tables-test-allowed-diff.h
> @@ -1 +1,2 @@
>  /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/virt/DSDT.pxb",
> diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
> index b4752c644c..91e91e0fec 100644
> --- a/tests/qtest/bios-tables-test.c
> +++ b/tests/qtest/bios-tables-test.c
> @@ -620,12 +620,21 @@ static void test_acpi_one(const char *params, test_data *data)
>           * TODO: convert '-drive if=pflash' to new syntax (see e33763be7cd3)
>           * when arm/virt boad starts to support it.
>           */
> -        args = g_strdup_printf("-machine %s %s -accel tcg -nodefaults -nographic "
> -            "-drive if=pflash,format=raw,file=%s,readonly "
> -            "-drive if=pflash,format=raw,file=%s,snapshot=on -cdrom %s %s",
> -            data->machine, data->tcg_only ? "" : "-accel kvm",
> -            data->uefi_fl1, data->uefi_fl2, data->cd, params ? params : "");
> -
> +        if (data->cd) {
> +            args = g_strdup_printf("-machine %s %s -accel tcg "
> +                "-nodefaults -nographic "
> +                "-drive if=pflash,format=raw,file=%s,readonly "
> +                "-drive if=pflash,format=raw,file=%s,snapshot=on -cdrom %s %s",
> +                data->machine, data->tcg_only ? "" : "-accel kvm",
> +                data->uefi_fl1, data->uefi_fl2, data->cd, params ? params : "");
> +        } else {
> +            args = g_strdup_printf("-machine %s %s -accel tcg "
> +                "-nodefaults -nographic "
> +                "-drive if=pflash,format=raw,file=%s,readonly "
> +                "-drive if=pflash,format=raw,file=%s,snapshot=on %s",
> +                data->machine, data->tcg_only ? "" : "-accel kvm",
> +                data->uefi_fl1, data->uefi_fl2, params ? params : "");
> +        }
>      } else {
>          /* Disable kernel irqchip to be able to override apic irq0. */
>          args = g_strdup_printf("-machine %s,kernel-irqchip=off %s -accel tcg "
> @@ -960,6 +969,38 @@ static void test_acpi_virt_tcg_numamem(void)
>  
>  }
>  
> +static void test_acpi_virt_tcg_pxb(void)
> +{
> +    test_data data = {
> +        .machine = "virt",
> +        .tcg_only = true,
> +        .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
> +        .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
> +        .ram_start = 0x40000000ULL,
> +        .scan_len = 128ULL * 1024 * 1024,
> +    };
> +    /*
> +     * While using -cdrom, the cdrom would auto plugged into pxb-pcie,
> +     * the reason is the bus of pxb-pcie is also root bus, it would lead
> +     * to the error only PCI/PCIE bridge could plug onto pxb.
> +     * Therefore,thr cdrom is defined and plugged onto the scsi controller
> +     * to solve the conflicts.
> +     */
> +    data.variant = ".pxb";
> +    test_acpi_one(" -device pcie-root-port,chassis=1,id=pci.1"
> +                  " -device virtio-scsi-pci,id=scsi0,bus=pci.1"
> +                  " -drive file="
> +                  "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2,"
> +                  "if=none,media=cdrom,id=drive-scsi0-0-0-1,readonly=on"
> +                  " -device scsi-cd,bus=scsi0.0,scsi-id=0,"
> +                  "drive=drive-scsi0-0-0-1,id=scsi0-0-0-1,bootindex=1"
> +                  " -cpu cortex-a57"
> +                  " -device pxb-pcie,bus_nr=128",
> +                  &data);
> +
> +    free_test_data(&data);
> +}
> +
>  static void test_acpi_tcg_acpi_hmat(const char *machine)
>  {
>      test_data data;
> @@ -1052,6 +1093,7 @@ int main(int argc, char *argv[])
>          qtest_add_func("acpi/virt", test_acpi_virt_tcg);
>          qtest_add_func("acpi/virt/numamem", test_acpi_virt_tcg_numamem);
>          qtest_add_func("acpi/virt/memhp", test_acpi_virt_tcg_memhp);
> +        qtest_add_func("acpi/virt/pxb", test_acpi_virt_tcg_pxb);
>      }
>      ret = g_test_run();
>      boot_sector_cleanup(disk);
> -- 
> 2.19.1
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
  2020-02-25  9:47   ` Philippe Mathieu-Daudé
  2020-02-25 11:20     ` Michael S. Tsirkin
@ 2020-02-25 12:12     ` miaoyubo
  2020-02-25 12:27       ` Michael S. Tsirkin
  1 sibling, 1 reply; 16+ messages in thread
From: miaoyubo @ 2020-02-25 12:12 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, peter.maydell, shannon.zhaosl
  Cc: imammedo, berrange, qemu-devel, Xiexiangyou, mst


> -----Original Message-----
> From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
> Sent: Tuesday, February 25, 2020 5:48 PM
> To: miaoyubo <miaoyubo@huawei.com>; peter.maydell@linaro.org;
> shannon.zhaosl@gmail.com
> Cc: berrange@redhat.com; mst@redhat.com; qemu-devel@nongnu.org;
> Xiexiangyou <xiexiangyou@huawei.com>; imammedo@redhat.com
> Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
> 
> On 2/25/20 2:50 AM, 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
> 
> Typos: "expander" in subject and "supported" here.
> 

Thanks for your reply and sorry for the mistakes.
I will check all the subjects and comments.

> > 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
> >
> >       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
> 
> Typo "objects", but I don't understand the comment well.
> 

Sorry for any confusion caused ,I will rewrite the comment 
/* point to the root bus, which is pcie.0 */
Does this comment make sense?

> > +     * could be gained.
> > +     */
> > +    PCIBus *bus;
> >   } VirtMachineState;
> >
> >   #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
> > VIRT_PCIE_ECAM)
> >

Regards,
Miao

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH v4 3/3] ACPI/unit-test: Add a new test for pxb-pcie for arm
  2020-02-25 11:24   ` Michael S. Tsirkin
@ 2020-02-25 12:25     ` miaoyubo
  0 siblings, 0 replies; 16+ messages in thread
From: miaoyubo @ 2020-02-25 12:25 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: peter.maydell, berrange, qemu-devel, Xiexiangyou, shannon.zhaosl,
	imammedo


> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Tuesday, February 25, 2020 7:25 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 3/3] ACPI/unit-test: Add a new test for pxb-pcie for
> arm
> 
> On Tue, Feb 25, 2020 at 09:50:26AM +0800, Yubo Miao wrote:
> > From: miaoyubo <miaoyubo@huawei.com>
> >
> > Currently, pxb-pcie could be defined by the cmdline like
> >     --device pxb-pcie,id=pci.9,bus_nr=128 However pxb-pcie is not
> > described in acpi tables for arm.
> >
> > The formal two patches support pxb-pcie for arm, escpcially the
> > specification for pxb-pcie in DSDT table.
> 
> 
> especially? Pls spell-check comments and commit log, it's not hard to do.
> 

Thanks for pointing out and sorry for the mistakes, 
I will check all the comments and commit log.

> > Add a testcase to make sure the ACPI table is correct for guest.
> >
> > The following table need to be added for this test:
> >     tests/data/acpi/virt/DSDT.pxb
> > Since the ASL diff has 1000+ lines, it would be presented in commit
> > log with the simply diff. the diff are:
> >     Device (PC80) is presented in DSDT.
> >     Resources allocated for Device (PCI0) is changed.
> >
> >   * Disassembling to symbolic ASL+ operators
> >   *
> > - * Disassembly of /home/DSDT, Mon Feb 24 19:35:28 2020
> > + * Disassembly of /home/DSDT.pxb, Mon Feb 24 19:33:38 2020
> >   *
> >   * Original Table Header:
> >   *     Signature        "DSDT"
> > - *     Length           0x000014BB (5307)
> > + *     Length           0x00001F70 (8048)
> >   *     Revision         0x02
> > - *     Checksum         0xD1
> > + *     Checksum         0xCF
> >   *     OEM ID           "BOCHS "
> >   *     OEM Table ID     "BXPCDSDT"
> >   *     OEM Revision     0x00000001 (1)
> >              })
> >          }
> >
> >          {
> >              Name (_HID, "PNP0A08" /* PCI Express Bus */)  // _HID: Hardware
> ID
> >                      WordBusNumber (ResourceProducer, MinFixed, MaxFixed,
> PosDecode,
> >                          0x0000,             // Granularity
> >                          0x0000,             // Range Minimum
> > -                        0x00FF,             // Range Maximum
> > +                        0x007F,             // Range Maximum
> >                          0x0000,             // Translation Offset
> > -                        0x0100,             // Length
> > +                        0x0080,             // Length
> >                          ,, )
> >                      DWordMemory (ResourceProducer, PosDecode, MinFixed,
> MaxFixed, NonCacheable, ReadWrite,
> >                          0x00000000,         // Granularity
> >                          0x10000000,         // Range Minimum
> > -                        0x3EFEFFFF,         // Range Maximum
> > +                        0x3E9EFFFF,         // Range Maximum
> >                          0x00000000,         // Translation Offset
> > -                        0x2EFF0000,         // Length
> > +                        0x2E9F0000,         // Length
> >                          ,, , AddressRangeMemory, TypeStatic)
> >                      DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode,
> EntireRange,
> >                          0x00000000,         // Granularity
> >                          0x00000000,         // Range Minimum
> > -                        0x0000FFFF,         // Range Maximum
> > +                        0x0000BFFF,         // Range Maximum
> >                          0x3EFF0000,         // Translation Offset
> > -                        0x00010000,         // Length
> > +                        0x0000C000,         // Length
> >                          ,, , TypeStatic, DenseTranslation)
> >                      QWordMemory (ResourceProducer, PosDecode, MinFixed,
> MaxFixed, NonCacheable, ReadWrite,
> >                          0x0000000000000000, // Granularity
> >
> > Signed-off-by: miaoyubo <miaoyubo@huawei.com>
> 
> 
> Seems to fail in patchew.
> 

The failure is due to CONFIG_PXB is not configured.
Since it is not configured by default, I will add ifdef CONFIG_PXB
before the pxb unit test to solve this problem. 

> > ---
> >  tests/data/acpi/virt/DSDT.pxb               | Bin 0 -> 8048 bytes
> >  tests/qtest/bios-tables-test-allowed-diff.h |   1 +
> >  tests/qtest/bios-tables-test.c              |  54 +++++++++++++++++---
> >  3 files changed, 49 insertions(+), 6 deletions(-)  create mode 100644
> > tests/data/acpi/virt/DSDT.pxb
> 
> This needs to be in a separate patch.
> See instructions in ./tests/qtest/bios-tables-test.c.
> 
> 

Alright, I would separate this patch into three patches.
1. tests/qtest/bios-tables-test-allowed-diff.h
2. Changes made to the unit tests (tests/qtest/bios-tables-test.c)
3. The binary file and clear tests/qtest/bios-tables-test-allowed-diff.h

> > new file mode 100644
> > index
> >
> 0000000000000000000000000000000000000000..6ac0b5212db49513c27ef50da
> 838
> > 240826c2deb7
> > GIT binary patch
> > literal 8048
> >
> >  static void test_acpi_tcg_acpi_hmat(const char *machine)  {
> >      test_data data;
> > @@ -1052,6 +1093,7 @@ int main(int argc, char *argv[])
> >          qtest_add_func("acpi/virt", test_acpi_virt_tcg);
> >          qtest_add_func("acpi/virt/numamem",
> test_acpi_virt_tcg_numamem);
> >          qtest_add_func("acpi/virt/memhp", test_acpi_virt_tcg_memhp);
> > +        qtest_add_func("acpi/virt/pxb", test_acpi_virt_tcg_pxb);
> >      }
> >      ret = g_test_run();
> >      boot_sector_cleanup(disk);
> > --
> > 2.19.1
> >

Regards,
Miao


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
  2020-02-25 12:12     ` miaoyubo
@ 2020-02-25 12:27       ` Michael S. Tsirkin
  2020-02-25 12:44         ` miaoyubo
  0 siblings, 1 reply; 16+ messages in thread
From: Michael S. Tsirkin @ 2020-02-25 12:27 UTC (permalink / raw)
  To: miaoyubo
  Cc: peter.maydell, berrange, qemu-devel, Xiexiangyou, shannon.zhaosl,
	imammedo, Philippe Mathieu-Daudé

On Tue, Feb 25, 2020 at 12:12:12PM +0000, miaoyubo wrote:
> 
> > -----Original Message-----
> > From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
> > Sent: Tuesday, February 25, 2020 5:48 PM
> > To: miaoyubo <miaoyubo@huawei.com>; peter.maydell@linaro.org;
> > shannon.zhaosl@gmail.com
> > Cc: berrange@redhat.com; mst@redhat.com; qemu-devel@nongnu.org;
> > Xiexiangyou <xiexiangyou@huawei.com>; imammedo@redhat.com
> > Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
> > 
> > On 2/25/20 2:50 AM, 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
> > 
> > Typos: "expander" in subject and "supported" here.
> > 
> 
> Thanks for your reply and sorry for the mistakes.
> I will check all the subjects and comments.
> 
> > > 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
> > >
> > >       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
> > 
> > Typo "objects", but I don't understand the comment well.
> > 
> 
> Sorry for any confusion caused ,I will rewrite the comment 
> /* point to the root bus, which is pcie.0 */
> Does this comment make sense?

Not really. E.g. it doesn't say what happens if there's more than one root.

> > > +     * could be gained.
> > > +     */
> > > +    PCIBus *bus;
> > >   } VirtMachineState;
> > >
> > >   #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
> > > VIRT_PCIE_ECAM)
> > >
> 
> Regards,
> Miao



^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
  2020-02-25 12:27       ` Michael S. Tsirkin
@ 2020-02-25 12:44         ` miaoyubo
  2020-02-25 13:11           ` Michael S. Tsirkin
  0 siblings, 1 reply; 16+ messages in thread
From: miaoyubo @ 2020-02-25 12:44 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: peter.maydell, berrange, qemu-devel, Xiexiangyou, shannon.zhaosl,
	imammedo, Philippe Mathieu-Daudé



> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Tuesday, February 25, 2020 8:27 PM
> To: miaoyubo <miaoyubo@huawei.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>;
> peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> berrange@redhat.com; qemu-devel@nongnu.org; Xiexiangyou
> <xiexiangyou@huawei.com>; imammedo@redhat.com
> Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
> 
> On Tue, Feb 25, 2020 at 12:12:12PM +0000, miaoyubo wrote:
> >
> > > -----Original Message-----
> > > From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
> > > Sent: Tuesday, February 25, 2020 5:48 PM
> > > To: miaoyubo <miaoyubo@huawei.com>; peter.maydell@linaro.org;
> > > shannon.zhaosl@gmail.com
> > > Cc: berrange@redhat.com; mst@redhat.com; qemu-devel@nongnu.org;
> > > Xiexiangyou <xiexiangyou@huawei.com>; imammedo@redhat.com
> > > Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support
> > > for arm
> > >
> > > On 2/25/20 2:50 AM, 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
> > >
> > > Typos: "expander" in subject and "supported" here.
> > >
> >
> > Thanks for your reply and sorry for the mistakes.
> > I will check all the subjects and comments.
> >
> > > > 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
> > > >
> > > >       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
> > >
> > > Typo "objects", but I don't understand the comment well.
> > >
> >
> > Sorry for any confusion caused ,I will rewrite the comment
> > /* point to the root bus, which is pcie.0 */ Does this comment make
> > sense?
> 
> Not really. E.g. it doesn't say what happens if there's more than one root.
> 

If there's more than one root, like pcie.0 and pcie.1, it still point to pcie.0.
In docs/pci_expander_bridge.txt, it points out pxb could be placed only
on bus 0 (pci.0). Therfore, the structure still could help us to find all pxb devices.
/* point to the bus 0, which is pcie.0
  * pxb devices could only be placed on bus 0.
  */
Is this ok?

> > > > +     * could be gained.
> > > > +     */
> > > > +    PCIBus *bus;
> > > >   } VirtMachineState;
> > > >
> > > >   #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
> > > > VIRT_PCIE_ECAM)
> > > >
> >
> > Regards,
> > Miao

Regards,
Miao


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
  2020-02-25 12:44         ` miaoyubo
@ 2020-02-25 13:11           ` Michael S. Tsirkin
  2020-02-26 10:42             ` miaoyubo
  0 siblings, 1 reply; 16+ messages in thread
From: Michael S. Tsirkin @ 2020-02-25 13:11 UTC (permalink / raw)
  To: miaoyubo
  Cc: peter.maydell, berrange, qemu-devel, Xiexiangyou, shannon.zhaosl,
	imammedo, Philippe Mathieu-Daudé

On Tue, Feb 25, 2020 at 12:44:15PM +0000, miaoyubo wrote:
> 
> 
> > -----Original Message-----
> > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > Sent: Tuesday, February 25, 2020 8:27 PM
> > To: miaoyubo <miaoyubo@huawei.com>
> > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>;
> > peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> > berrange@redhat.com; qemu-devel@nongnu.org; Xiexiangyou
> > <xiexiangyou@huawei.com>; imammedo@redhat.com
> > Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
> > 
> > On Tue, Feb 25, 2020 at 12:12:12PM +0000, miaoyubo wrote:
> > >
> > > > -----Original Message-----
> > > > From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
> > > > Sent: Tuesday, February 25, 2020 5:48 PM
> > > > To: miaoyubo <miaoyubo@huawei.com>; peter.maydell@linaro.org;
> > > > shannon.zhaosl@gmail.com
> > > > Cc: berrange@redhat.com; mst@redhat.com; qemu-devel@nongnu.org;
> > > > Xiexiangyou <xiexiangyou@huawei.com>; imammedo@redhat.com
> > > > Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support
> > > > for arm
> > > >
> > > > On 2/25/20 2:50 AM, 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
> > > >
> > > > Typos: "expander" in subject and "supported" here.
> > > >
> > >
> > > Thanks for your reply and sorry for the mistakes.
> > > I will check all the subjects and comments.
> > >
> > > > > 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
> > > > >
> > > > >       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
> > > >
> > > > Typo "objects", but I don't understand the comment well.
> > > >
> > >
> > > Sorry for any confusion caused ,I will rewrite the comment
> > > /* point to the root bus, which is pcie.0 */ Does this comment make
> > > sense?
> > 
> > Not really. E.g. it doesn't say what happens if there's more than one root.
> > 
> 
> If there's more than one root, like pcie.0 and pcie.1, it still point to pcie.0.
> In docs/pci_expander_bridge.txt, it points out pxb could be placed only
> on bus 0 (pci.0). Therfore, the structure still could help us to find all pxb devices.
> /* point to the bus 0, which is pcie.0
>   * pxb devices could only be placed on bus 0.
>   */
> Is this ok?

All this needs more comments in the code constructing the tables.

Also, instead of trying to store bus and spreading logic
around like this, how about just using object_resolve_path_type?


> > > > > +     * could be gained.
> > > > > +     */
> > > > > +    PCIBus *bus;
> > > > >   } VirtMachineState;
> > > > >
> > > > >   #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
> > > > > VIRT_PCIE_ECAM)
> > > > >
> > >
> > > Regards,
> > > Miao
> 
> Regards,
> Miao



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
  2020-02-25  1:50 ` [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm Yubo Miao
  2020-02-25  9:47   ` Philippe Mathieu-Daudé
@ 2020-02-25 13:14   ` Michael S. Tsirkin
  2020-02-26 10:56     ` miaoyubo
  1 sibling, 1 reply; 16+ messages in thread
From: Michael S. Tsirkin @ 2020-02-25 13:14 UTC (permalink / raw)
  To: Yubo Miao
  Cc: peter.maydell, berrange, qemu-devel, xiexiangyou, shannon.zhaosl,
	imammedo

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.

> +     * 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.


>      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 (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
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
  2020-02-25 13:11           ` Michael S. Tsirkin
@ 2020-02-26 10:42             ` miaoyubo
  0 siblings, 0 replies; 16+ messages in thread
From: miaoyubo @ 2020-02-26 10:42 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: peter.maydell, berrange, qemu-devel, Xiexiangyou, shannon.zhaosl,
	imammedo, Philippe Mathieu-Daudé


> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Tuesday, February 25, 2020 9:12 PM
> To: miaoyubo <miaoyubo@huawei.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>;
> peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> berrange@redhat.com; qemu-devel@nongnu.org; Xiexiangyou
> <xiexiangyou@huawei.com>; imammedo@redhat.com
> Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
> 
> On Tue, Feb 25, 2020 at 12:44:15PM +0000, miaoyubo wrote:
> >
> >
> > > -----Original Message-----
> > > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > > Sent: Tuesday, February 25, 2020 8:27 PM
> > > To: miaoyubo <miaoyubo@huawei.com>
> > > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>;
> > > peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> > > berrange@redhat.com; qemu-devel@nongnu.org; Xiexiangyou
> > > <xiexiangyou@huawei.com>; imammedo@redhat.com
> > > Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support
> > > for arm
> > >
> > > On Tue, Feb 25, 2020 at 12:12:12PM +0000, miaoyubo wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Philippe Mathieu-Daudé [mailto:philmd@redhat.com]
> > > > > Sent: Tuesday, February 25, 2020 5:48 PM
> > > > > To: miaoyubo <miaoyubo@huawei.com>; peter.maydell@linaro.org;
> > > > > shannon.zhaosl@gmail.com
> > > > > Cc: berrange@redhat.com; mst@redhat.com; qemu-
> devel@nongnu.org;
> > > > > Xiexiangyou <xiexiangyou@huawei.com>; imammedo@redhat.com
> > > > > Subject: Re: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb
> > > > > support for arm
> > > > >
> > > > > On 2/25/20 2:50 AM, 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
> > > > >
> > > > > Typos: "expander" in subject and "supported" here.
> > > > >
> > > >
> > > > Thanks for your reply and sorry for the mistakes.
> > > > I will check all the subjects and comments.
> > > >
> > > > > > 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
> > > > > >
> > > > > >       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
> > > > >
> > > > > Typo "objects", but I don't understand the comment well.
> > > > >
> > > >
> > > > Sorry for any confusion caused ,I will rewrite the comment
> > > > /* point to the root bus, which is pcie.0 */ Does this comment
> > > > make sense?
> > >
> > > Not really. E.g. it doesn't say what happens if there's more than one root.
> > >
> >
> > If there's more than one root, like pcie.0 and pcie.1, it still point to pcie.0.
> > In docs/pci_expander_bridge.txt, it points out pxb could be placed
> > only on bus 0 (pci.0). Therfore, the structure still could help us to find all
> pxb devices.
> > /* point to the bus 0, which is pcie.0
> >   * pxb devices could only be placed on bus 0.
> >   */
> > Is this ok?
> 
> All this needs more comments in the code constructing the tables.
> 

Thanks for replying, I will add more comments in the table construction.

> Also, instead of trying to store bus and spreading logic around like this, how
> about just using object_resolve_path_type?
> 
> 

Thanks for the suggestion, using object_resolve_path_type  seems to be better. 

> > > > > > +     * could be gained.
> > > > > > +     */
> > > > > > +    PCIBus *bus;
> > > > > >   } VirtMachineState;
> > > > > >
> > > > > >   #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
> > > > > > VIRT_PCIE_ECAM)
> > > > > >
> > > >
> > > > Regards,
> > > > Miao
> >
> > Regards,
> > Miao

Regards,
Miao


^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm
  2020-02-25 13:14   ` Michael S. Tsirkin
@ 2020-02-26 10:56     ` miaoyubo
  0 siblings, 0 replies; 16+ messages in thread
From: miaoyubo @ 2020-02-26 10:56 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: peter.maydell, berrange, qemu-devel, Xiexiangyou, shannon.zhaosl,
	imammedo



> -----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
> >



^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-02-26 10:57 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-25  1:50 [PATCH v4 0/3] pci_expander_brdige:acpi:Support pxb-pcie for ARM Yubo Miao
2020-02-25  1:50 ` [PATCH v4 1/3] acpi:Extract two APIs from acpi_dsdt_add_pci Yubo Miao
2020-02-25  1:50 ` [PATCH v4 2/3] acpi:pci-expender-bus: Add pxb support for arm Yubo Miao
2020-02-25  9:47   ` Philippe Mathieu-Daudé
2020-02-25 11:20     ` Michael S. Tsirkin
2020-02-25 12:12     ` miaoyubo
2020-02-25 12:27       ` Michael S. Tsirkin
2020-02-25 12:44         ` miaoyubo
2020-02-25 13:11           ` Michael S. Tsirkin
2020-02-26 10:42             ` miaoyubo
2020-02-25 13:14   ` Michael S. Tsirkin
2020-02-26 10:56     ` miaoyubo
2020-02-25  1:50 ` [PATCH v4 3/3] ACPI/unit-test: Add a new test for pxb-pcie " Yubo Miao
2020-02-25 11:24   ` Michael S. Tsirkin
2020-02-25 12:25     ` miaoyubo
2020-02-25  2:23 ` [PATCH v4 0/3] pci_expander_brdige:acpi:Support pxb-pcie for ARM no-reply

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.