[Qemu-devel] [PATCH 0/2] ARM virt: Support up to 256 PCIe buses

[Qemu-devel] [PATCH 0/2] ARM virt: Support up to 256 PCIe buses

[Eric Auger]

Current Machvirt PCI host controller's ECAM region is 16MB large.
This limits the number of PCIe buses to 16.

PC/Q35 machines have a 256MB region allowing up to 256 buses.
This series tries to bridge the gap.

It declares a new ECAM region located beyond 256GB, of size 256MB
The new ECAM region is used if:
- highmem option is set (default) and,
- either FW is not loaded or we are run an aarch64 guest
- machine type >= 3.0.

aarch32 FW does not support this highmem ECAM region. For guests without
LPAE support the highmem option must be turned off.

Best Regards

Eric

Git: complete series available at
https://github.com/eauger/qemu/tree/v2.12.0-256MB-ECAM-PATCH-v1
Previous version:
https://github.com/eauger/qemu/tree/v2.12.0-256MB-ECAM-RFCv1

- Tested with guest running in aarch64 and aarch32 modes (aarch64=off)
- Tested with aarch32 FW
- In aarch32 mode I encountered the issue the vmalloc region may be
  reported too small for the needs (dmesg excerpt below). So I had to
  extend the vmalloc size by passing the "vmalloc=512M" option to the
  bootargs and this eventually booted fine.

[    1.399581] pl061_gpio 9030000.pl061: PL061 GPIO chip @0x0000000009030000 registered
[    1.402636] OF: PCI: host bridge /pcie@10000000 ranges:
[    1.404506] OF: PCI:    IO 0x3eff0000..0x3effffff -> 0x00000000
[    1.406606] OF: PCI:   MEM 0x10000000..0x3efeffff -> 0x10000000
[    1.408690] OF: PCI:   MEM 0x8000000000..0xffffffffff -> 0x8000000000
[    1.411992] vmap allocation for size 1052672 failed: use vmalloc=<size> to increase size
[    1.414895] pci-host-generic 4010000000.pcie: ECAM ioremap failed
[    1.427472] pci-host-generic: probe of 4010000000.pcie failed with error -12


Eric Auger (2):
  hw/arm/virt: Add a new 256MB ECAM region
  hw/arm/virt: Add virt-3.0 machine type

 hw/arm/virt-acpi-build.c | 21 +++++++++++++--------
 hw/arm/virt.c            | 43 ++++++++++++++++++++++++++++++++++++-------
 include/hw/arm/virt.h    |  3 +++
 3 files changed, 52 insertions(+), 15 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 1/2] hw/arm/virt: Add a new 256MB ECAM region

[Eric Auger]

This patch defines a new ECAM region located after the 256GB limit.

The virt machine state is augmented with a new highmem_ecam field
which guards the usage of this new ECAM region instead of the legacy
16MB one. With the highmem ECAM region, up to 256 PCIe buses can be
used.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

RFC -> PATCH:
- remove the newline at the end of acpi_dsdt_add_pci
- use vms->highmem_ecam to select the memmap id
---
 hw/arm/virt-acpi-build.c | 21 +++++++++++++--------
 hw/arm/virt.c            | 12 ++++++++----
 include/hw/arm/virt.h    |  2 ++
 3 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 92ceee9..c0370b2 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -150,16 +150,17 @@ 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)
+                              uint32_t irq, bool use_highmem, bool highmem_ecam)
 {
+    int ecam_id = highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM;
     Aml *method, *crs, *ifctx, *UUID, *ifctx1, *elsectx, *buf;
     int i, bus_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[VIRT_PCIE_ECAM].base;
-    hwaddr size_ecam = memmap[VIRT_PCIE_ECAM].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");
@@ -173,7 +174,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
     aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
 
     /* Declare the PCI Routing Table. */
-    Aml *rt_pkg = aml_package(nr_pcie_buses * PCI_NUM_PINS);
+    Aml *rt_pkg = aml_varpackage(nr_pcie_buses * PCI_NUM_PINS);
     for (bus_no = 0; bus_no < nr_pcie_buses; bus_no++) {
         for (i = 0; i < PCI_NUM_PINS; i++) {
             int gsi = (i + bus_no) % PCI_NUM_PINS;
@@ -316,7 +317,10 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
     Aml *dev_res0 = aml_device("%s", "RES0");
     aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));
     crs = aml_resource_template();
-    aml_append(crs, aml_memory32_fixed(base_ecam, size_ecam, AML_READ_WRITE));
+    aml_append(crs,
+        aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED,
+                         AML_NON_CACHEABLE, AML_READ_WRITE, 0x0000, base_ecam,
+                         base_ecam + size_ecam - 1, 0x0000, size_ecam));
     aml_append(dev_res0, aml_name_decl("_CRS", crs));
     aml_append(dev, dev_res0);
     aml_append(scope, dev);
@@ -563,16 +567,17 @@ build_mcfg(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {
     AcpiTableMcfg *mcfg;
     const MemMapEntry *memmap = vms->memmap;
+    int ecam_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM;
     int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]);
     int mcfg_start = table_data->len;
 
     mcfg = acpi_data_push(table_data, len);
-    mcfg->allocation[0].address = cpu_to_le64(memmap[VIRT_PCIE_ECAM].base);
+    mcfg->allocation[0].address = cpu_to_le64(memmap[ecam_id].base);
 
     /* Only a single allocation so no need to play with segments */
     mcfg->allocation[0].pci_segment = cpu_to_le16(0);
     mcfg->allocation[0].start_bus_number = 0;
-    mcfg->allocation[0].end_bus_number = (memmap[VIRT_PCIE_ECAM].size
+    mcfg->allocation[0].end_bus_number = (memmap[ecam_id].size
                                           / PCIE_MMCFG_SIZE_MIN) - 1;
 
     build_header(linker, table_data, (void *)(table_data->data + mcfg_start),
@@ -747,7 +752,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, vms->highmem_ecam);
     acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
                        (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
     acpi_dsdt_add_power_button(scope);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a3a28e2..d4247d0 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -149,6 +149,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
     [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },
     [VIRT_MEM] =                { 0x40000000, RAMLIMIT_BYTES },
+    [VIRT_PCIE_ECAM_HIGH] =     { 0x4010000000ULL, 0x10000000 },
     /* Second PCIe window, 512GB wide at the 512GB boundary */
     [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000ULL, 0x8000000000ULL },
 };
@@ -1001,10 +1002,9 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic)
     hwaddr size_mmio_high = vms->memmap[VIRT_PCIE_MMIO_HIGH].size;
     hwaddr base_pio = vms->memmap[VIRT_PCIE_PIO].base;
     hwaddr size_pio = vms->memmap[VIRT_PCIE_PIO].size;
-    hwaddr base_ecam = vms->memmap[VIRT_PCIE_ECAM].base;
-    hwaddr size_ecam = vms->memmap[VIRT_PCIE_ECAM].size;
+    hwaddr base_ecam, size_ecam;
     hwaddr base = base_mmio;
-    int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
+    int nr_pcie_buses;
     int irq = vms->irqmap[VIRT_PCIE];
     MemoryRegion *mmio_alias;
     MemoryRegion *mmio_reg;
@@ -1012,12 +1012,16 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic)
     MemoryRegion *ecam_reg;
     DeviceState *dev;
     char *nodename;
-    int i;
+    int i, ecam_memmap_id;
     PCIHostState *pci;
 
     dev = qdev_create(NULL, TYPE_GPEX_HOST);
     qdev_init_nofail(dev);
 
+    ecam_memmap_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM;
+    base_ecam = vms->memmap[ecam_memmap_id].base;
+    size_ecam = vms->memmap[ecam_memmap_id].size;
+    nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
     /* Map only the first size_ecam bytes of ECAM space */
     ecam_alias = g_new0(MemoryRegion, 1);
     ecam_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 4ac7ef6..e9423a7 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -69,6 +69,7 @@ enum {
     VIRT_PCIE_MMIO,
     VIRT_PCIE_PIO,
     VIRT_PCIE_ECAM,
+    VIRT_PCIE_ECAM_HIGH,
     VIRT_PLATFORM_BUS,
     VIRT_PCIE_MMIO_HIGH,
     VIRT_GPIO,
@@ -103,6 +104,7 @@ typedef struct {
     FWCfgState *fw_cfg;
     bool secure;
     bool highmem;
+    bool highmem_ecam;
     bool its;
     bool virt;
     int32_t gic_version;
-- 
2.5.5

[Qemu-devel] [PATCH 2/2] hw/arm/virt: Add virt-3.0 machine type

[Eric Auger]

Add virt-3.0 machine type.

This machine type supports highmem 256MB ECAM by default.
This feature is disabled for earlier machine types and
if highmem is off.

The high 256MB ECAM region is chosen instead of the legacy
16MB one if the machine type allows it, if highmem is set
(LPAE supported by the guest) and (!firmware_loaded || aarch64).
Indeed aarch32 mode FW may not support this high ECAM region.

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

RFC -> v1
- check firmware_loaded and aarch64 value
- do all the computation in machvirt_init
---
 hw/arm/virt.c         | 31 ++++++++++++++++++++++++++++---
 include/hw/arm/virt.h |  1 +
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d4247d0..83c2b5a 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1275,6 +1275,7 @@ static void machvirt_init(MachineState *machine)
     int n, virt_max_cpus;
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
+    bool aarch64 = true;
 
     /* We can probe only here because during property set
      * KVM is not available yet
@@ -1389,6 +1390,8 @@ static void machvirt_init(MachineState *machine)
         numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
                           &error_fatal);
 
+        aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL);
+
         if (!vms->secure) {
             object_property_set_bool(cpuobj, false, "has_el3", NULL);
         }
@@ -1447,6 +1450,8 @@ static void machvirt_init(MachineState *machine)
         create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem, serial_hd(1));
     }
 
+    vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64);
+
     create_rtc(vms, pic);
 
     create_pcie(vms, pic);
@@ -1697,7 +1702,7 @@ static void machvirt_machine_init(void)
 }
 type_init(machvirt_machine_init);
 
-static void virt_2_12_instance_init(Object *obj)
+static void virt_3_0_instance_init(Object *obj)
 {
     VirtMachineState *vms = VIRT_MACHINE(obj);
     VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
@@ -1740,6 +1745,8 @@ static void virt_2_12_instance_init(Object *obj)
                                     "Set GIC version. "
                                     "Valid values are 2, 3 and host", NULL);
 
+    vms->highmem_ecam = vmc->no_highmem_ecam ? false : true;
+
     if (vmc->no_its) {
         vms->its = false;
     } else {
@@ -1765,10 +1772,28 @@ static void virt_2_12_instance_init(Object *obj)
     vms->irqmap = a15irqmap;
 }
 
-static void virt_machine_2_12_options(MachineClass *mc)
+static void virt_machine_3_0_options(MachineClass *mc)
 {
 }
-DEFINE_VIRT_MACHINE_AS_LATEST(2, 12)
+DEFINE_VIRT_MACHINE_AS_LATEST(3, 0)
+
+#define VIRT_COMPAT_2_12 \
+    HW_COMPAT_2_12
+
+static void virt_2_12_instance_init(Object *obj)
+{
+    virt_3_0_instance_init(obj);
+}
+
+static void virt_machine_2_12_options(MachineClass *mc)
+{
+    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
+
+    virt_machine_3_0_options(mc);
+    SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_12);
+    vmc->no_highmem_ecam = true;
+}
+DEFINE_VIRT_MACHINE(2, 12)
 
 #define VIRT_COMPAT_2_11 \
     HW_COMPAT_2_11
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index e9423a7..10a5c71 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -95,6 +95,7 @@ typedef struct {
     bool no_pmu;
     bool claim_edge_triggered_timers;
     bool smbios_old_sys_ver;
+    bool no_highmem_ecam;
 } VirtMachineClass;
 
 typedef struct {
-- 
2.5.5

Re: [Qemu-devel] [PATCH 1/2] hw/arm/virt: Add a new 256MB ECAM region

[Laszlo Ersek]

On 05/30/18 16:26, Eric Auger wrote:
> This patch defines a new ECAM region located after the 256GB limit.
> 
> The virt machine state is augmented with a new highmem_ecam field
> which guards the usage of this new ECAM region instead of the legacy
> 16MB one. With the highmem ECAM region, up to 256 PCIe buses can be
> used.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> RFC -> PATCH:
> - remove the newline at the end of acpi_dsdt_add_pci
> - use vms->highmem_ecam to select the memmap id
> ---
>  hw/arm/virt-acpi-build.c | 21 +++++++++++++--------
>  hw/arm/virt.c            | 12 ++++++++----
>  include/hw/arm/virt.h    |  2 ++
>  3 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 92ceee9..c0370b2 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -150,16 +150,17 @@ 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)
> +                              uint32_t irq, bool use_highmem, bool highmem_ecam)
>  {
> +    int ecam_id = highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM;
>      Aml *method, *crs, *ifctx, *UUID, *ifctx1, *elsectx, *buf;
>      int i, bus_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[VIRT_PCIE_ECAM].base;
> -    hwaddr size_ecam = memmap[VIRT_PCIE_ECAM].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");
> @@ -173,7 +174,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
>      aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
>  
>      /* Declare the PCI Routing Table. */
> -    Aml *rt_pkg = aml_package(nr_pcie_buses * PCI_NUM_PINS);
> +    Aml *rt_pkg = aml_varpackage(nr_pcie_buses * PCI_NUM_PINS);
>      for (bus_no = 0; bus_no < nr_pcie_buses; bus_no++) {
>          for (i = 0; i < PCI_NUM_PINS; i++) {
>              int gsi = (i + bus_no) % PCI_NUM_PINS;
> @@ -316,7 +317,10 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
>      Aml *dev_res0 = aml_device("%s", "RES0");
>      aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));
>      crs = aml_resource_template();
> -    aml_append(crs, aml_memory32_fixed(base_ecam, size_ecam, AML_READ_WRITE));
> +    aml_append(crs,
> +        aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED,
> +                         AML_NON_CACHEABLE, AML_READ_WRITE, 0x0000, base_ecam,
> +                         base_ecam + size_ecam - 1, 0x0000, size_ecam));
>      aml_append(dev_res0, aml_name_decl("_CRS", crs));
>      aml_append(dev, dev_res0);
>      aml_append(scope, dev);
> @@ -563,16 +567,17 @@ build_mcfg(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  {
>      AcpiTableMcfg *mcfg;
>      const MemMapEntry *memmap = vms->memmap;
> +    int ecam_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM;
>      int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]);
>      int mcfg_start = table_data->len;
>  
>      mcfg = acpi_data_push(table_data, len);
> -    mcfg->allocation[0].address = cpu_to_le64(memmap[VIRT_PCIE_ECAM].base);
> +    mcfg->allocation[0].address = cpu_to_le64(memmap[ecam_id].base);
>  
>      /* Only a single allocation so no need to play with segments */
>      mcfg->allocation[0].pci_segment = cpu_to_le16(0);
>      mcfg->allocation[0].start_bus_number = 0;
> -    mcfg->allocation[0].end_bus_number = (memmap[VIRT_PCIE_ECAM].size
> +    mcfg->allocation[0].end_bus_number = (memmap[ecam_id].size
>                                            / PCIE_MMCFG_SIZE_MIN) - 1;
>  
>      build_header(linker, table_data, (void *)(table_data->data + mcfg_start),
> @@ -747,7 +752,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, vms->highmem_ecam);
>      acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
>                         (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
>      acpi_dsdt_add_power_button(scope);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index a3a28e2..d4247d0 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -149,6 +149,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
>      [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },
>      [VIRT_MEM] =                { 0x40000000, RAMLIMIT_BYTES },
> +    [VIRT_PCIE_ECAM_HIGH] =     { 0x4010000000ULL, 0x10000000 },
>      /* Second PCIe window, 512GB wide at the 512GB boundary */
>      [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000ULL, 0x8000000000ULL },
>  };
> @@ -1001,10 +1002,9 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic)
>      hwaddr size_mmio_high = vms->memmap[VIRT_PCIE_MMIO_HIGH].size;
>      hwaddr base_pio = vms->memmap[VIRT_PCIE_PIO].base;
>      hwaddr size_pio = vms->memmap[VIRT_PCIE_PIO].size;
> -    hwaddr base_ecam = vms->memmap[VIRT_PCIE_ECAM].base;
> -    hwaddr size_ecam = vms->memmap[VIRT_PCIE_ECAM].size;
> +    hwaddr base_ecam, size_ecam;
>      hwaddr base = base_mmio;
> -    int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
> +    int nr_pcie_buses;
>      int irq = vms->irqmap[VIRT_PCIE];
>      MemoryRegion *mmio_alias;
>      MemoryRegion *mmio_reg;
> @@ -1012,12 +1012,16 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic)
>      MemoryRegion *ecam_reg;
>      DeviceState *dev;
>      char *nodename;
> -    int i;
> +    int i, ecam_memmap_id;
>      PCIHostState *pci;
>  
>      dev = qdev_create(NULL, TYPE_GPEX_HOST);
>      qdev_init_nofail(dev);
>  
> +    ecam_memmap_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM;
> +    base_ecam = vms->memmap[ecam_memmap_id].base;
> +    size_ecam = vms->memmap[ecam_memmap_id].size;
> +    nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
>      /* Map only the first size_ecam bytes of ECAM space */
>      ecam_alias = g_new0(MemoryRegion, 1);
>      ecam_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 4ac7ef6..e9423a7 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -69,6 +69,7 @@ enum {
>      VIRT_PCIE_MMIO,
>      VIRT_PCIE_PIO,
>      VIRT_PCIE_ECAM,
> +    VIRT_PCIE_ECAM_HIGH,
>      VIRT_PLATFORM_BUS,
>      VIRT_PCIE_MMIO_HIGH,
>      VIRT_GPIO,
> @@ -103,6 +104,7 @@ typedef struct {
>      FWCfgState *fw_cfg;
>      bool secure;
>      bool highmem;
> +    bool highmem_ecam;
>      bool its;
>      bool virt;
>      int32_t gic_version;
> 

At this point, the order (and values) of the VIRT_* enum constants look
mostly random :) , but I'm unaware of anything why that should be a problem.

I compared this against the RFC version; from my side:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Can you please do the following additionally:

- The aarch64 firmware packaged in Gerd's repo is built with the verbose
  log enabled, as far as I can see. Can you please capture two boot logs
  (from the serial console) until the guest kernel is reached, and send
  them to me (off-list if you prefer)? The first log should have the new
  feature disabled, the second one enabled; I'd like to compare them.

- (I think the same test is useful with the guest kernel dmesg as well,
  passing ignore_loglevel, but I don't think my assistance in reviewing
  that difference is necessary or helpful :) )

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Add virt-3.0 machine type

[Laszlo Ersek]

On 05/30/18 16:26, Eric Auger wrote:
> Add virt-3.0 machine type.
> 
> This machine type supports highmem 256MB ECAM by default.
> This feature is disabled for earlier machine types and
> if highmem is off.
> 
> The high 256MB ECAM region is chosen instead of the legacy
> 16MB one if the machine type allows it, if highmem is set
> (LPAE supported by the guest) and (!firmware_loaded || aarch64).
> Indeed aarch32 mode FW may not support this high ECAM region.
> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> 
> ---
> 
> RFC -> v1
> - check firmware_loaded and aarch64 value
> - do all the computation in machvirt_init
> ---
>  hw/arm/virt.c         | 31 ++++++++++++++++++++++++++++---
>  include/hw/arm/virt.h |  1 +
>  2 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d4247d0..83c2b5a 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1275,6 +1275,7 @@ static void machvirt_init(MachineState *machine)
>      int n, virt_max_cpus;
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>      bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
> +    bool aarch64 = true;
>  
>      /* We can probe only here because during property set
>       * KVM is not available yet
> @@ -1389,6 +1390,8 @@ static void machvirt_init(MachineState *machine)
>          numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
>                            &error_fatal);
>  
> +        aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL);
> +
>          if (!vms->secure) {
>              object_property_set_bool(cpuobj, false, "has_el3", NULL);
>          }
> @@ -1447,6 +1450,8 @@ static void machvirt_init(MachineState *machine)
>          create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem, serial_hd(1));
>      }
>  
> +    vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64);
> +
>      create_rtc(vms, pic);
>  
>      create_pcie(vms, pic);
> @@ -1697,7 +1702,7 @@ static void machvirt_machine_init(void)
>  }
>  type_init(machvirt_machine_init);
>  
> -static void virt_2_12_instance_init(Object *obj)
> +static void virt_3_0_instance_init(Object *obj)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(obj);
>      VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
> @@ -1740,6 +1745,8 @@ static void virt_2_12_instance_init(Object *obj)
>                                      "Set GIC version. "
>                                      "Valid values are 2, 3 and host", NULL);
>  
> +    vms->highmem_ecam = vmc->no_highmem_ecam ? false : true;
> +

I think this should be written as:

    vms->highmem_ecam = !vmc->no_highmem_ecam;

With that change:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Obviously the series has to be reviewed by others as well.

Thanks!
Laszlo


>      if (vmc->no_its) {
>          vms->its = false;
>      } else {
> @@ -1765,10 +1772,28 @@ static void virt_2_12_instance_init(Object *obj)
>      vms->irqmap = a15irqmap;
>  }
>  
> -static void virt_machine_2_12_options(MachineClass *mc)
> +static void virt_machine_3_0_options(MachineClass *mc)
>  {
>  }
> -DEFINE_VIRT_MACHINE_AS_LATEST(2, 12)
> +DEFINE_VIRT_MACHINE_AS_LATEST(3, 0)
> +
> +#define VIRT_COMPAT_2_12 \
> +    HW_COMPAT_2_12
> +
> +static void virt_2_12_instance_init(Object *obj)
> +{
> +    virt_3_0_instance_init(obj);
> +}
> +
> +static void virt_machine_2_12_options(MachineClass *mc)
> +{
> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
> +    virt_machine_3_0_options(mc);
> +    SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_12);
> +    vmc->no_highmem_ecam = true;
> +}
> +DEFINE_VIRT_MACHINE(2, 12)
>  
>  #define VIRT_COMPAT_2_11 \
>      HW_COMPAT_2_11
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index e9423a7..10a5c71 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -95,6 +95,7 @@ typedef struct {
>      bool no_pmu;
>      bool claim_edge_triggered_timers;
>      bool smbios_old_sys_ver;
> +    bool no_highmem_ecam;
>  } VirtMachineClass;
>  
>  typedef struct {
> 

Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Add virt-3.0 machine type

[Shannon Zhao]

On 2018/5/31 0:18, Laszlo Ersek wrote:
> +    vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64);
>> +
Does it need a info log here to tell user that even though you enable
the highmem_ecam but due to some other reasons it's disabled.

Also, if user enables highmem_ecam but finally it can't enable, does it
need to error out?

Thanks,
-- 
Shannon

Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Add virt-3.0 machine type

[Auger Eric]

Hi Shannon,

On 05/31/2018 03:42 AM, Shannon Zhao wrote:
> 
> 
> On 2018/5/31 0:18, Laszlo Ersek wrote:
>> +    vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64);
>>> +
> Does it need a info log here to tell user that even though you enable
> the highmem_ecam but due to some other reasons it's disabled.
> 
> Also, if user enables highmem_ecam but finally it can't enable, does it
> need to error out?

highmem_ecam is not a user settable option. highmem is. highmem_ecam is
computed based on the machine type capability to support this feature,
the fact highmem is set and we don't have an aarch32 FW being used.

Thanks

Eric
> 
> Thanks,
> 

Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Add virt-3.0 machine type

[Auger Eric]

On 05/30/2018 06:18 PM, Laszlo Ersek wrote:
> On 05/30/18 16:26, Eric Auger wrote:
>> Add virt-3.0 machine type.
>>
>> This machine type supports highmem 256MB ECAM by default.
>> This feature is disabled for earlier machine types and
>> if highmem is off.
>>
>> The high 256MB ECAM region is chosen instead of the legacy
>> 16MB one if the machine type allows it, if highmem is set
>> (LPAE supported by the guest) and (!firmware_loaded || aarch64).
>> Indeed aarch32 mode FW may not support this high ECAM region.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> RFC -> v1
>> - check firmware_loaded and aarch64 value
>> - do all the computation in machvirt_init
>> ---
>>  hw/arm/virt.c         | 31 ++++++++++++++++++++++++++++---
>>  include/hw/arm/virt.h |  1 +
>>  2 files changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index d4247d0..83c2b5a 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1275,6 +1275,7 @@ static void machvirt_init(MachineState *machine)
>>      int n, virt_max_cpus;
>>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>>      bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
>> +    bool aarch64 = true;
>>  
>>      /* We can probe only here because during property set
>>       * KVM is not available yet
>> @@ -1389,6 +1390,8 @@ static void machvirt_init(MachineState *machine)
>>          numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj),
>>                            &error_fatal);
>>  
>> +        aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL);
>> +
>>          if (!vms->secure) {
>>              object_property_set_bool(cpuobj, false, "has_el3", NULL);
>>          }
>> @@ -1447,6 +1450,8 @@ static void machvirt_init(MachineState *machine)
>>          create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem, serial_hd(1));
>>      }
>>  
>> +    vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64);
>> +
>>      create_rtc(vms, pic);
>>  
>>      create_pcie(vms, pic);
>> @@ -1697,7 +1702,7 @@ static void machvirt_machine_init(void)
>>  }
>>  type_init(machvirt_machine_init);
>>  
>> -static void virt_2_12_instance_init(Object *obj)
>> +static void virt_3_0_instance_init(Object *obj)
>>  {
>>      VirtMachineState *vms = VIRT_MACHINE(obj);
>>      VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms);
>> @@ -1740,6 +1745,8 @@ static void virt_2_12_instance_init(Object *obj)
>>                                      "Set GIC version. "
>>                                      "Valid values are 2, 3 and host", NULL);
>>  
>> +    vms->highmem_ecam = vmc->no_highmem_ecam ? false : true;
>> +
> 
> I think this should be written as:
> 
>     vms->highmem_ecam = !vmc->no_highmem_ecam;
sure ;-)
> 
> With that change:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thank you for the early feedbacks.

Eric
> 
> Obviously the series has to be reviewed by others as well.
> 
> Thanks!
> Laszlo
> 
> 
>>      if (vmc->no_its) {
>>          vms->its = false;
>>      } else {
>> @@ -1765,10 +1772,28 @@ static void virt_2_12_instance_init(Object *obj)
>>      vms->irqmap = a15irqmap;
>>  }
>>  
>> -static void virt_machine_2_12_options(MachineClass *mc)
>> +static void virt_machine_3_0_options(MachineClass *mc)
>>  {
>>  }
>> -DEFINE_VIRT_MACHINE_AS_LATEST(2, 12)
>> +DEFINE_VIRT_MACHINE_AS_LATEST(3, 0)
>> +
>> +#define VIRT_COMPAT_2_12 \
>> +    HW_COMPAT_2_12
>> +
>> +static void virt_2_12_instance_init(Object *obj)
>> +{
>> +    virt_3_0_instance_init(obj);
>> +}
>> +
>> +static void virt_machine_2_12_options(MachineClass *mc)
>> +{
>> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
>> +
>> +    virt_machine_3_0_options(mc);
>> +    SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_12);
>> +    vmc->no_highmem_ecam = true;
>> +}
>> +DEFINE_VIRT_MACHINE(2, 12)
>>  
>>  #define VIRT_COMPAT_2_11 \
>>      HW_COMPAT_2_11
>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>> index e9423a7..10a5c71 100644
>> --- a/include/hw/arm/virt.h
>> +++ b/include/hw/arm/virt.h
>> @@ -95,6 +95,7 @@ typedef struct {
>>      bool no_pmu;
>>      bool claim_edge_triggered_timers;
>>      bool smbios_old_sys_ver;
>> +    bool no_highmem_ecam;
>>  } VirtMachineClass;
>>  
>>  typedef struct {
>>
> 
> 

Re: [Qemu-devel] [PATCH 1/2] hw/arm/virt: Add a new 256MB ECAM region

[Auger Eric]

Hi Laszlo,

On 05/30/2018 06:11 PM, Laszlo Ersek wrote:
> On 05/30/18 16:26, Eric Auger wrote:
>> This patch defines a new ECAM region located after the 256GB limit.
>>
>> The virt machine state is augmented with a new highmem_ecam field
>> which guards the usage of this new ECAM region instead of the legacy
>> 16MB one. With the highmem ECAM region, up to 256 PCIe buses can be
>> used.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> RFC -> PATCH:
>> - remove the newline at the end of acpi_dsdt_add_pci
>> - use vms->highmem_ecam to select the memmap id
>> ---
>>  hw/arm/virt-acpi-build.c | 21 +++++++++++++--------
>>  hw/arm/virt.c            | 12 ++++++++----
>>  include/hw/arm/virt.h    |  2 ++
>>  3 files changed, 23 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 92ceee9..c0370b2 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -150,16 +150,17 @@ 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)
>> +                              uint32_t irq, bool use_highmem, bool highmem_ecam)
>>  {
>> +    int ecam_id = highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM;
>>      Aml *method, *crs, *ifctx, *UUID, *ifctx1, *elsectx, *buf;
>>      int i, bus_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[VIRT_PCIE_ECAM].base;
>> -    hwaddr size_ecam = memmap[VIRT_PCIE_ECAM].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");
>> @@ -173,7 +174,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
>>      aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
>>  
>>      /* Declare the PCI Routing Table. */
>> -    Aml *rt_pkg = aml_package(nr_pcie_buses * PCI_NUM_PINS);
>> +    Aml *rt_pkg = aml_varpackage(nr_pcie_buses * PCI_NUM_PINS);
>>      for (bus_no = 0; bus_no < nr_pcie_buses; bus_no++) {
>>          for (i = 0; i < PCI_NUM_PINS; i++) {
>>              int gsi = (i + bus_no) % PCI_NUM_PINS;
>> @@ -316,7 +317,10 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
>>      Aml *dev_res0 = aml_device("%s", "RES0");
>>      aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));
>>      crs = aml_resource_template();
>> -    aml_append(crs, aml_memory32_fixed(base_ecam, size_ecam, AML_READ_WRITE));
>> +    aml_append(crs,
>> +        aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED,
>> +                         AML_NON_CACHEABLE, AML_READ_WRITE, 0x0000, base_ecam,
>> +                         base_ecam + size_ecam - 1, 0x0000, size_ecam));
>>      aml_append(dev_res0, aml_name_decl("_CRS", crs));
>>      aml_append(dev, dev_res0);
>>      aml_append(scope, dev);
>> @@ -563,16 +567,17 @@ build_mcfg(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>  {
>>      AcpiTableMcfg *mcfg;
>>      const MemMapEntry *memmap = vms->memmap;
>> +    int ecam_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM;
>>      int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]);
>>      int mcfg_start = table_data->len;
>>  
>>      mcfg = acpi_data_push(table_data, len);
>> -    mcfg->allocation[0].address = cpu_to_le64(memmap[VIRT_PCIE_ECAM].base);
>> +    mcfg->allocation[0].address = cpu_to_le64(memmap[ecam_id].base);
>>  
>>      /* Only a single allocation so no need to play with segments */
>>      mcfg->allocation[0].pci_segment = cpu_to_le16(0);
>>      mcfg->allocation[0].start_bus_number = 0;
>> -    mcfg->allocation[0].end_bus_number = (memmap[VIRT_PCIE_ECAM].size
>> +    mcfg->allocation[0].end_bus_number = (memmap[ecam_id].size
>>                                            / PCIE_MMCFG_SIZE_MIN) - 1;
>>  
>>      build_header(linker, table_data, (void *)(table_data->data + mcfg_start),
>> @@ -747,7 +752,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, vms->highmem_ecam);
>>      acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
>>                         (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
>>      acpi_dsdt_add_power_button(scope);
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index a3a28e2..d4247d0 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -149,6 +149,7 @@ static const MemMapEntry a15memmap[] = {
>>      [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
>>      [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },
>>      [VIRT_MEM] =                { 0x40000000, RAMLIMIT_BYTES },
>> +    [VIRT_PCIE_ECAM_HIGH] =     { 0x4010000000ULL, 0x10000000 },
>>      /* Second PCIe window, 512GB wide at the 512GB boundary */
>>      [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000ULL, 0x8000000000ULL },
>>  };
>> @@ -1001,10 +1002,9 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic)
>>      hwaddr size_mmio_high = vms->memmap[VIRT_PCIE_MMIO_HIGH].size;
>>      hwaddr base_pio = vms->memmap[VIRT_PCIE_PIO].base;
>>      hwaddr size_pio = vms->memmap[VIRT_PCIE_PIO].size;
>> -    hwaddr base_ecam = vms->memmap[VIRT_PCIE_ECAM].base;
>> -    hwaddr size_ecam = vms->memmap[VIRT_PCIE_ECAM].size;
>> +    hwaddr base_ecam, size_ecam;
>>      hwaddr base = base_mmio;
>> -    int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
>> +    int nr_pcie_buses;
>>      int irq = vms->irqmap[VIRT_PCIE];
>>      MemoryRegion *mmio_alias;
>>      MemoryRegion *mmio_reg;
>> @@ -1012,12 +1012,16 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic)
>>      MemoryRegion *ecam_reg;
>>      DeviceState *dev;
>>      char *nodename;
>> -    int i;
>> +    int i, ecam_memmap_id;
>>      PCIHostState *pci;
>>  
>>      dev = qdev_create(NULL, TYPE_GPEX_HOST);
>>      qdev_init_nofail(dev);
>>  
>> +    ecam_memmap_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM;
>> +    base_ecam = vms->memmap[ecam_memmap_id].base;
>> +    size_ecam = vms->memmap[ecam_memmap_id].size;
>> +    nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
>>      /* Map only the first size_ecam bytes of ECAM space */
>>      ecam_alias = g_new0(MemoryRegion, 1);
>>      ecam_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>> index 4ac7ef6..e9423a7 100644
>> --- a/include/hw/arm/virt.h
>> +++ b/include/hw/arm/virt.h
>> @@ -69,6 +69,7 @@ enum {
>>      VIRT_PCIE_MMIO,
>>      VIRT_PCIE_PIO,
>>      VIRT_PCIE_ECAM,
>> +    VIRT_PCIE_ECAM_HIGH,
>>      VIRT_PLATFORM_BUS,
>>      VIRT_PCIE_MMIO_HIGH,
>>      VIRT_GPIO,
>> @@ -103,6 +104,7 @@ typedef struct {
>>      FWCfgState *fw_cfg;
>>      bool secure;
>>      bool highmem;
>> +    bool highmem_ecam;
>>      bool its;
>>      bool virt;
>>      int32_t gic_version;
>>
> 
> At this point, the order (and values) of the VIRT_* enum constants look
> mostly random :) , but I'm unaware of anything why that should be a problem.
> 
> I compared this against the RFC version; from my side:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Can you please do the following additionally:
> 
> - The aarch64 firmware packaged in Gerd's repo is built with the verbose
>   log enabled, as far as I can see. Can you please capture two boot logs
>   (from the serial console) until the guest kernel is reached, and send
>   them to me (off-list if you prefer)? The first log should have the new
>   feature disabled, the second one enabled; I'd like to compare them.
> 
> - (I think the same test is useful with the guest kernel dmesg as well,
>   passing ignore_loglevel, but I don't think my assistance in reviewing
>   that difference is necessary or helpful :) )

OK I will prepare those logs

Thanks

Eric
> 
> Thanks!
> Laszlo
> 

Re: [Qemu-devel] [PATCH 1/2] hw/arm/virt: Add a new 256MB ECAM region

[Laszlo Ersek]

On 05/31/18 08:55, Auger Eric wrote:
> Hi Laszlo,
>
> On 05/30/2018 06:11 PM, Laszlo Ersek wrote:
>> On 05/30/18 16:26, Eric Auger wrote:
>>> This patch defines a new ECAM region located after the 256GB limit.
>>>
>>> The virt machine state is augmented with a new highmem_ecam field
>>> which guards the usage of this new ECAM region instead of the legacy
>>> 16MB one. With the highmem ECAM region, up to 256 PCIe buses can be
>>> used.
>>>
>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>
>>> ---
>>>
>>> RFC -> PATCH:
>>> - remove the newline at the end of acpi_dsdt_add_pci
>>> - use vms->highmem_ecam to select the memmap id
>>> ---
>>>  hw/arm/virt-acpi-build.c | 21 +++++++++++++--------
>>>  hw/arm/virt.c            | 12 ++++++++----
>>>  include/hw/arm/virt.h    |  2 ++
>>>  3 files changed, 23 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index 92ceee9..c0370b2 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -150,16 +150,17 @@ 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)
>>> +                              uint32_t irq, bool use_highmem, bool highmem_ecam)
>>>  {
>>> +    int ecam_id = highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM;
>>>      Aml *method, *crs, *ifctx, *UUID, *ifctx1, *elsectx, *buf;
>>>      int i, bus_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[VIRT_PCIE_ECAM].base;
>>> -    hwaddr size_ecam = memmap[VIRT_PCIE_ECAM].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");
>>> @@ -173,7 +174,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
>>>      aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
>>>
>>>      /* Declare the PCI Routing Table. */
>>> -    Aml *rt_pkg = aml_package(nr_pcie_buses * PCI_NUM_PINS);
>>> +    Aml *rt_pkg = aml_varpackage(nr_pcie_buses * PCI_NUM_PINS);
>>>      for (bus_no = 0; bus_no < nr_pcie_buses; bus_no++) {
>>>          for (i = 0; i < PCI_NUM_PINS; i++) {
>>>              int gsi = (i + bus_no) % PCI_NUM_PINS;
>>> @@ -316,7 +317,10 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
>>>      Aml *dev_res0 = aml_device("%s", "RES0");
>>>      aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));
>>>      crs = aml_resource_template();
>>> -    aml_append(crs, aml_memory32_fixed(base_ecam, size_ecam, AML_READ_WRITE));
>>> +    aml_append(crs,
>>> +        aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED,
>>> +                         AML_NON_CACHEABLE, AML_READ_WRITE, 0x0000, base_ecam,
>>> +                         base_ecam + size_ecam - 1, 0x0000, size_ecam));
>>>      aml_append(dev_res0, aml_name_decl("_CRS", crs));
>>>      aml_append(dev, dev_res0);
>>>      aml_append(scope, dev);
>>> @@ -563,16 +567,17 @@ build_mcfg(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>>  {
>>>      AcpiTableMcfg *mcfg;
>>>      const MemMapEntry *memmap = vms->memmap;
>>> +    int ecam_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM;
>>>      int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]);
>>>      int mcfg_start = table_data->len;
>>>
>>>      mcfg = acpi_data_push(table_data, len);
>>> -    mcfg->allocation[0].address = cpu_to_le64(memmap[VIRT_PCIE_ECAM].base);
>>> +    mcfg->allocation[0].address = cpu_to_le64(memmap[ecam_id].base);
>>>
>>>      /* Only a single allocation so no need to play with segments */
>>>      mcfg->allocation[0].pci_segment = cpu_to_le16(0);
>>>      mcfg->allocation[0].start_bus_number = 0;
>>> -    mcfg->allocation[0].end_bus_number = (memmap[VIRT_PCIE_ECAM].size
>>> +    mcfg->allocation[0].end_bus_number = (memmap[ecam_id].size
>>>                                            / PCIE_MMCFG_SIZE_MIN) - 1;
>>>
>>>      build_header(linker, table_data, (void *)(table_data->data + mcfg_start),
>>> @@ -747,7 +752,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, vms->highmem_ecam);
>>>      acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
>>>                         (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
>>>      acpi_dsdt_add_power_button(scope);
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index a3a28e2..d4247d0 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -149,6 +149,7 @@ static const MemMapEntry a15memmap[] = {
>>>      [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
>>>      [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },
>>>      [VIRT_MEM] =                { 0x40000000, RAMLIMIT_BYTES },
>>> +    [VIRT_PCIE_ECAM_HIGH] =     { 0x4010000000ULL, 0x10000000 },
>>>      /* Second PCIe window, 512GB wide at the 512GB boundary */
>>>      [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000ULL, 0x8000000000ULL },
>>>  };
>>> @@ -1001,10 +1002,9 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic)
>>>      hwaddr size_mmio_high = vms->memmap[VIRT_PCIE_MMIO_HIGH].size;
>>>      hwaddr base_pio = vms->memmap[VIRT_PCIE_PIO].base;
>>>      hwaddr size_pio = vms->memmap[VIRT_PCIE_PIO].size;
>>> -    hwaddr base_ecam = vms->memmap[VIRT_PCIE_ECAM].base;
>>> -    hwaddr size_ecam = vms->memmap[VIRT_PCIE_ECAM].size;
>>> +    hwaddr base_ecam, size_ecam;
>>>      hwaddr base = base_mmio;
>>> -    int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
>>> +    int nr_pcie_buses;
>>>      int irq = vms->irqmap[VIRT_PCIE];
>>>      MemoryRegion *mmio_alias;
>>>      MemoryRegion *mmio_reg;
>>> @@ -1012,12 +1012,16 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic)
>>>      MemoryRegion *ecam_reg;
>>>      DeviceState *dev;
>>>      char *nodename;
>>> -    int i;
>>> +    int i, ecam_memmap_id;
>>>      PCIHostState *pci;
>>>
>>>      dev = qdev_create(NULL, TYPE_GPEX_HOST);
>>>      qdev_init_nofail(dev);
>>>
>>> +    ecam_memmap_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM;
>>> +    base_ecam = vms->memmap[ecam_memmap_id].base;
>>> +    size_ecam = vms->memmap[ecam_memmap_id].size;
>>> +    nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
>>>      /* Map only the first size_ecam bytes of ECAM space */
>>>      ecam_alias = g_new0(MemoryRegion, 1);
>>>      ecam_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
>>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>>> index 4ac7ef6..e9423a7 100644
>>> --- a/include/hw/arm/virt.h
>>> +++ b/include/hw/arm/virt.h
>>> @@ -69,6 +69,7 @@ enum {
>>>      VIRT_PCIE_MMIO,
>>>      VIRT_PCIE_PIO,
>>>      VIRT_PCIE_ECAM,
>>> +    VIRT_PCIE_ECAM_HIGH,
>>>      VIRT_PLATFORM_BUS,
>>>      VIRT_PCIE_MMIO_HIGH,
>>>      VIRT_GPIO,
>>> @@ -103,6 +104,7 @@ typedef struct {
>>>      FWCfgState *fw_cfg;
>>>      bool secure;
>>>      bool highmem;
>>> +    bool highmem_ecam;
>>>      bool its;
>>>      bool virt;
>>>      int32_t gic_version;
>>>
>>
>> At this point, the order (and values) of the VIRT_* enum constants
>> look mostly random :) , but I'm unaware of anything why that should
>> be a problem.
>>
>> I compared this against the RFC version; from my side:
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Can you please do the following additionally:
>>
>> - The aarch64 firmware packaged in Gerd's repo is built with the
>>   verbose log enabled, as far as I can see. Can you please capture
>>   two boot logs (from the serial console) until the guest kernel is
>>   reached, and send them to me (off-list if you prefer)? The first
>>   log should have the new feature disabled, the second one enabled;
>>   I'd like to compare them.
>>
>> - (I think the same test is useful with the guest kernel dmesg as
>>   well, passing ignore_loglevel, but I don't think my assistance in
>>   reviewing that difference is necessary or helpful :) )
>
> OK I will prepare those logs

Thanks. The important stuff is fine:

> -ProcessPciHost: Config[0x3F000000+0x1000000) Bus[0x0..0xF] Io[0x0+0x10000)@0x3EFF0000 Mem32[0x10000000+0x2EFF0000)@0x0 Mem64[0x8000000000+0x8000000000)@0x0
> +ProcessPciHost: Config[0x4010000000+0x10000000) Bus[0x0..0xFF] Io[0x0+0x10000)@0x3EFF0000 Mem32[0x10000000+0x2EFF0000)@0x0 Mem64[0x8000000000+0x8000000000)@0x0
>  RootBridge: PciRoot(0x0)
>    Support/Attr: 70001 / 70001
>      DmaAbove4G: Yes
>  NoExtConfSpace: No
>       AllocAttr: 3 (CombineMemPMem Mem64Decode)
> -           Bus: 0 - F Translation=0
> +           Bus: 0 - FF Translation=0
>              Io: 0 - FFFF Translation=0
>             Mem: 10000000 - 3EFEFFFF Translation=0
>      MemAbove4G: 8000000000 - FFFFFFFFFF Translation=0
>            PMem: FFFFFFFFFFFFFFFF - 0 Translation=0
>     PMemAbove4G: FFFFFFFFFFFFFFFF - 0 Translation=0
>  PciHostBridgeDxe: IntersectIoDescriptor: add [0, 10000): Success
>  PciHostBridgeDxe: IntersectMemoryDescriptor: add [10000000, 3EFF0000): Success
>  PciHostBridgeDxe: IntersectMemoryDescriptor: add [8000000000, 10000000000): Success

However, another change surprises me. It might be totally independent of
your patch, but because your patch also touches ACPI generation, I'd
like to highlight it: the size of the DSDT grows from 0x11BD bytes to
0x4848 bytes. This is confirmed by both the firmware log and the guest
dmesg; quoting the latter:

> -[    0.000000] ACPI: DSDT 0x000000013BED0000 0011BD (v02 BOCHS  BXPCDSDT 00000001 BXPC 00000001)
> +[    0.000000] ACPI: DSDT 0x000000013BED0000 004848 (v02 BOCHS  BXPCDSDT 00000001 BXPC 00000001)

Given that your series just introduces the virt-3.0 machine type, I
don't think it enables some other ACPI feature -- intentionally anyway!
-- that explains this size growth. Can you dump the ACPI tables in the
guest and decompile them with iasl?

... Ah wait, I'm being silly. "nr_pcie_buses" in acpi_dsdt_add_pci()
affects the size of the PRT; you even changed the PRT from an AML
"package" to an AML "varpackage", because the loop that adds the
interrupt mapping packages to the PRT runs much higher now.

I vaguely recall that Linux used to dump the PRT at boot; from those
messages I would have realized earlier. Anyway, everything looks fine to
me.

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH 1/2] hw/arm/virt: Add a new 256MB ECAM region

[Auger Eric]

Hi Laszlo,

On 05/31/2018 10:41 AM, Laszlo Ersek wrote:
> On 05/31/18 08:55, Auger Eric wrote:
>> Hi Laszlo,
>>
>> On 05/30/2018 06:11 PM, Laszlo Ersek wrote:
>>> On 05/30/18 16:26, Eric Auger wrote:
>>>> This patch defines a new ECAM region located after the 256GB limit.
>>>>
>>>> The virt machine state is augmented with a new highmem_ecam field
>>>> which guards the usage of this new ECAM region instead of the legacy
>>>> 16MB one. With the highmem ECAM region, up to 256 PCIe buses can be
>>>> used.
>>>>
>>>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>>>
>>>> ---
>>>>
>>>> RFC -> PATCH:
>>>> - remove the newline at the end of acpi_dsdt_add_pci
>>>> - use vms->highmem_ecam to select the memmap id
>>>> ---
>>>>  hw/arm/virt-acpi-build.c | 21 +++++++++++++--------
>>>>  hw/arm/virt.c            | 12 ++++++++----
>>>>  include/hw/arm/virt.h    |  2 ++
>>>>  3 files changed, 23 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>>> index 92ceee9..c0370b2 100644
>>>> --- a/hw/arm/virt-acpi-build.c
>>>> +++ b/hw/arm/virt-acpi-build.c
>>>> @@ -150,16 +150,17 @@ 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)
>>>> +                              uint32_t irq, bool use_highmem, bool highmem_ecam)
>>>>  {
>>>> +    int ecam_id = highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM;
>>>>      Aml *method, *crs, *ifctx, *UUID, *ifctx1, *elsectx, *buf;
>>>>      int i, bus_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[VIRT_PCIE_ECAM].base;
>>>> -    hwaddr size_ecam = memmap[VIRT_PCIE_ECAM].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");
>>>> @@ -173,7 +174,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
>>>>      aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
>>>>
>>>>      /* Declare the PCI Routing Table. */
>>>> -    Aml *rt_pkg = aml_package(nr_pcie_buses * PCI_NUM_PINS);
>>>> +    Aml *rt_pkg = aml_varpackage(nr_pcie_buses * PCI_NUM_PINS);
>>>>      for (bus_no = 0; bus_no < nr_pcie_buses; bus_no++) {
>>>>          for (i = 0; i < PCI_NUM_PINS; i++) {
>>>>              int gsi = (i + bus_no) % PCI_NUM_PINS;
>>>> @@ -316,7 +317,10 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
>>>>      Aml *dev_res0 = aml_device("%s", "RES0");
>>>>      aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02")));
>>>>      crs = aml_resource_template();
>>>> -    aml_append(crs, aml_memory32_fixed(base_ecam, size_ecam, AML_READ_WRITE));
>>>> +    aml_append(crs,
>>>> +        aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED,
>>>> +                         AML_NON_CACHEABLE, AML_READ_WRITE, 0x0000, base_ecam,
>>>> +                         base_ecam + size_ecam - 1, 0x0000, size_ecam));
>>>>      aml_append(dev_res0, aml_name_decl("_CRS", crs));
>>>>      aml_append(dev, dev_res0);
>>>>      aml_append(scope, dev);
>>>> @@ -563,16 +567,17 @@ build_mcfg(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>>>>  {
>>>>      AcpiTableMcfg *mcfg;
>>>>      const MemMapEntry *memmap = vms->memmap;
>>>> +    int ecam_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM;
>>>>      int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]);
>>>>      int mcfg_start = table_data->len;
>>>>
>>>>      mcfg = acpi_data_push(table_data, len);
>>>> -    mcfg->allocation[0].address = cpu_to_le64(memmap[VIRT_PCIE_ECAM].base);
>>>> +    mcfg->allocation[0].address = cpu_to_le64(memmap[ecam_id].base);
>>>>
>>>>      /* Only a single allocation so no need to play with segments */
>>>>      mcfg->allocation[0].pci_segment = cpu_to_le16(0);
>>>>      mcfg->allocation[0].start_bus_number = 0;
>>>> -    mcfg->allocation[0].end_bus_number = (memmap[VIRT_PCIE_ECAM].size
>>>> +    mcfg->allocation[0].end_bus_number = (memmap[ecam_id].size
>>>>                                            / PCIE_MMCFG_SIZE_MIN) - 1;
>>>>
>>>>      build_header(linker, table_data, (void *)(table_data->data + mcfg_start),
>>>> @@ -747,7 +752,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, vms->highmem_ecam);
>>>>      acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
>>>>                         (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
>>>>      acpi_dsdt_add_power_button(scope);
>>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>>> index a3a28e2..d4247d0 100644
>>>> --- a/hw/arm/virt.c
>>>> +++ b/hw/arm/virt.c
>>>> @@ -149,6 +149,7 @@ static const MemMapEntry a15memmap[] = {
>>>>      [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
>>>>      [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },
>>>>      [VIRT_MEM] =                { 0x40000000, RAMLIMIT_BYTES },
>>>> +    [VIRT_PCIE_ECAM_HIGH] =     { 0x4010000000ULL, 0x10000000 },
>>>>      /* Second PCIe window, 512GB wide at the 512GB boundary */
>>>>      [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000ULL, 0x8000000000ULL },
>>>>  };
>>>> @@ -1001,10 +1002,9 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic)
>>>>      hwaddr size_mmio_high = vms->memmap[VIRT_PCIE_MMIO_HIGH].size;
>>>>      hwaddr base_pio = vms->memmap[VIRT_PCIE_PIO].base;
>>>>      hwaddr size_pio = vms->memmap[VIRT_PCIE_PIO].size;
>>>> -    hwaddr base_ecam = vms->memmap[VIRT_PCIE_ECAM].base;
>>>> -    hwaddr size_ecam = vms->memmap[VIRT_PCIE_ECAM].size;
>>>> +    hwaddr base_ecam, size_ecam;
>>>>      hwaddr base = base_mmio;
>>>> -    int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
>>>> +    int nr_pcie_buses;
>>>>      int irq = vms->irqmap[VIRT_PCIE];
>>>>      MemoryRegion *mmio_alias;
>>>>      MemoryRegion *mmio_reg;
>>>> @@ -1012,12 +1012,16 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic)
>>>>      MemoryRegion *ecam_reg;
>>>>      DeviceState *dev;
>>>>      char *nodename;
>>>> -    int i;
>>>> +    int i, ecam_memmap_id;
>>>>      PCIHostState *pci;
>>>>
>>>>      dev = qdev_create(NULL, TYPE_GPEX_HOST);
>>>>      qdev_init_nofail(dev);
>>>>
>>>> +    ecam_memmap_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM;
>>>> +    base_ecam = vms->memmap[ecam_memmap_id].base;
>>>> +    size_ecam = vms->memmap[ecam_memmap_id].size;
>>>> +    nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN;
>>>>      /* Map only the first size_ecam bytes of ECAM space */
>>>>      ecam_alias = g_new0(MemoryRegion, 1);
>>>>      ecam_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
>>>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>>>> index 4ac7ef6..e9423a7 100644
>>>> --- a/include/hw/arm/virt.h
>>>> +++ b/include/hw/arm/virt.h
>>>> @@ -69,6 +69,7 @@ enum {
>>>>      VIRT_PCIE_MMIO,
>>>>      VIRT_PCIE_PIO,
>>>>      VIRT_PCIE_ECAM,
>>>> +    VIRT_PCIE_ECAM_HIGH,
>>>>      VIRT_PLATFORM_BUS,
>>>>      VIRT_PCIE_MMIO_HIGH,
>>>>      VIRT_GPIO,
>>>> @@ -103,6 +104,7 @@ typedef struct {
>>>>      FWCfgState *fw_cfg;
>>>>      bool secure;
>>>>      bool highmem;
>>>> +    bool highmem_ecam;
>>>>      bool its;
>>>>      bool virt;
>>>>      int32_t gic_version;
>>>>
>>>
>>> At this point, the order (and values) of the VIRT_* enum constants
>>> look mostly random :) , but I'm unaware of anything why that should
>>> be a problem.
>>>
>>> I compared this against the RFC version; from my side:
>>>
>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>>
>>> Can you please do the following additionally:
>>>
>>> - The aarch64 firmware packaged in Gerd's repo is built with the
>>>   verbose log enabled, as far as I can see. Can you please capture
>>>   two boot logs (from the serial console) until the guest kernel is
>>>   reached, and send them to me (off-list if you prefer)? The first
>>>   log should have the new feature disabled, the second one enabled;
>>>   I'd like to compare them.
>>>
>>> - (I think the same test is useful with the guest kernel dmesg as
>>>   well, passing ignore_loglevel, but I don't think my assistance in
>>>   reviewing that difference is necessary or helpful :) )
>>
>> OK I will prepare those logs
> 
> Thanks. The important stuff is fine:
> 
>> -ProcessPciHost: Config[0x3F000000+0x1000000) Bus[0x0..0xF] Io[0x0+0x10000)@0x3EFF0000 Mem32[0x10000000+0x2EFF0000)@0x0 Mem64[0x8000000000+0x8000000000)@0x0
>> +ProcessPciHost: Config[0x4010000000+0x10000000) Bus[0x0..0xFF] Io[0x0+0x10000)@0x3EFF0000 Mem32[0x10000000+0x2EFF0000)@0x0 Mem64[0x8000000000+0x8000000000)@0x0
>>  RootBridge: PciRoot(0x0)
>>    Support/Attr: 70001 / 70001
>>      DmaAbove4G: Yes
>>  NoExtConfSpace: No
>>       AllocAttr: 3 (CombineMemPMem Mem64Decode)
>> -           Bus: 0 - F Translation=0
>> +           Bus: 0 - FF Translation=0
>>              Io: 0 - FFFF Translation=0
>>             Mem: 10000000 - 3EFEFFFF Translation=0
>>      MemAbove4G: 8000000000 - FFFFFFFFFF Translation=0
>>            PMem: FFFFFFFFFFFFFFFF - 0 Translation=0
>>     PMemAbove4G: FFFFFFFFFFFFFFFF - 0 Translation=0
>>  PciHostBridgeDxe: IntersectIoDescriptor: add [0, 10000): Success
>>  PciHostBridgeDxe: IntersectMemoryDescriptor: add [10000000, 3EFF0000): Success
>>  PciHostBridgeDxe: IntersectMemoryDescriptor: add [8000000000, 10000000000): Success
> 
> However, another change surprises me. It might be totally independent of
> your patch, but because your patch also touches ACPI generation, I'd
> like to highlight it: the size of the DSDT grows from 0x11BD bytes to
> 0x4848 bytes. This is confirmed by both the firmware log and the guest
> dmesg; quoting the latter:
> 
>> -[    0.000000] ACPI: DSDT 0x000000013BED0000 0011BD (v02 BOCHS  BXPCDSDT 00000001 BXPC 00000001)
>> +[    0.000000] ACPI: DSDT 0x000000013BED0000 004848 (v02 BOCHS  BXPCDSDT 00000001 BXPC 00000001)
> 
> Given that your series just introduces the virt-3.0 machine type, I
> don't think it enables some other ACPI feature -- intentionally anyway!
> -- that explains this size growth. Can you dump the ACPI tables in the
> guest and decompile them with iasl?
> 
> ... Ah wait, I'm being silly. "nr_pcie_buses" in acpi_dsdt_add_pci()
> affects the size of the PRT; you even changed the PRT from an AML
> "package" to an AML "varpackage", because the loop that adds the
> interrupt mapping packages to the PRT runs much higher now.
> 
> I vaguely recall that Linux used to dump the PRT at boot; from those
> messages I would have realized earlier. Anyway, everything looks fine to
> me.

Cool!

Thanks

Eric
> 
> Thanks!
> Laszlo
> 

Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Add virt-3.0 machine type

[Peter Maydell]

On 30 May 2018 at 15:26, Eric Auger <eric.auger@redhat.com> wrote:
> Add virt-3.0 machine type.
>
> This machine type supports highmem 256MB ECAM by default.
> This feature is disabled for earlier machine types and
> if highmem is off.
>
> The high 256MB ECAM region is chosen instead of the legacy
> 16MB one if the machine type allows it, if highmem is set
> (LPAE supported by the guest) and (!firmware_loaded || aarch64).
> Indeed aarch32 mode FW may not support this high ECAM region.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>

I think these patches should be the other way round --
create the new 3.0 machine type first, and then in the
patch adding the high ECAM region you can make it be
only if newer machine type.

thanks
-- PMM