[Qemu-devel] [PATCH v8] hw/arm/virt: Add high MMIO PCI region, 512G in size

[Qemu-devel] [PATCH v8] hw/arm/virt: Add high MMIO PCI region, 512G in size

[Pavel Fedin]

This large region is necessary for some devices like ivshmem and video cards
32-bit kernels can be built without LPAE support. In this case such a kernel
will not be able to use PCI controller which has windows in high addresses.
In order to work around the problem, "highmem" option is introduced. It
defaults to on on, but can be manually set to off in order to be able to run
those old 32-bit guests.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
v7 => v8:
- Split up between region and FDT creation
- Renamed alias variable to high_mmio_alias
v6 => v7:
- Renamed alias variable to mmio64_alias
v5 => v6:
- Specify correct FDT_PCI_RANGE_MMIO_64BIT type for the region, the bug
  was discovered by running UEFI
v4 => v5:
- Removed machine-dependent "highmem" default, now always ON
v3 => v4:
- Added "highmem" option which controls presence of this region. Default
  value is on for 64-bit CPUs and off for 32-bit CPUs.
- Supply correct min and max address to aml_qword_memory()
v2 => v3:
- Region size increased to 512G
- Added ACPI description
v1 => v2:
- Region address changed to 512G, leaving more space for RAM
---
 hw/arm/virt-acpi-build.c         | 17 +++++++++--
 hw/arm/virt.c                    | 65 +++++++++++++++++++++++++++++++++++-----
 include/hw/arm/virt-acpi-build.h |  1 +
 include/hw/arm/virt.h            |  1 +
 4 files changed, 75 insertions(+), 9 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index f365140..9088248 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -159,7 +159,8 @@ static void acpi_dsdt_add_virtio(Aml *scope,
     }
 }
 
-static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, int irq)
+static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, int irq,
+                              bool use_highmem)
 {
     Aml *method, *crs, *ifctx, *UUID, *ifctx1, *elsectx, *buf;
     int i, bus_no;
@@ -234,6 +235,17 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, int irq)
                      AML_ENTIRE_RANGE, 0x0000, 0x0000, size_pio - 1, base_pio,
                      size_pio));
 
+    if (use_highmem) {
+        hwaddr base_mmio_high = memmap[VIRT_PCIE_MMIO_HIGH].base;
+        hwaddr size_mmio_high = memmap[VIRT_PCIE_MMIO_HIGH].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, 0x0000,
+                             size_mmio_high));
+    }
+
     aml_append(method, aml_name_decl("RBUF", rbuf));
     aml_append(method, aml_return(rbuf));
     aml_append(dev, method);
@@ -510,7 +522,8 @@ build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
     acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
     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));
+    acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] + ARM_SPI_BASE),
+                      guest_info->use_highmem);
 
     aml_append(dsdt, scope);
 
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d5a8417..003a47c 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -79,6 +79,7 @@ typedef struct {
 typedef struct {
     MachineState parent;
     bool secure;
+    bool highmem;
 } VirtMachineState;
 
 #define TYPE_VIRT_MACHINE   "virt"
@@ -119,6 +120,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
     [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },
     [VIRT_MEM] =                { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
+    [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000, 0x8000000000 },
 };
 
 static const int a15irqmap[] = {
@@ -666,10 +668,13 @@ static void create_pcie_irq_map(const VirtBoardInfo *vbi, uint32_t
gic_phandle,
                            0x7           /* PCI irq */);
 }
 
-static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic)
+static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic,
+                        bool use_highmem)
 {
     hwaddr base_mmio = vbi->memmap[VIRT_PCIE_MMIO].base;
     hwaddr size_mmio = vbi->memmap[VIRT_PCIE_MMIO].size;
+    hwaddr base_mmio_high = vbi->memmap[VIRT_PCIE_MMIO_HIGH].base;
+    hwaddr size_mmio_high = vbi->memmap[VIRT_PCIE_MMIO_HIGH].size;
     hwaddr base_pio = vbi->memmap[VIRT_PCIE_PIO].base;
     hwaddr size_pio = vbi->memmap[VIRT_PCIE_PIO].size;
     hwaddr base_ecam = vbi->memmap[VIRT_PCIE_ECAM].base;
@@ -706,6 +711,16 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic)
                              mmio_reg, base_mmio, size_mmio);
     memory_region_add_subregion(get_system_memory(), base_mmio, mmio_alias);
 
+    if (use_highmem) {
+        /* Map high MMIO space */
+        MemoryRegion *high_mmio_alias = g_new0(MemoryRegion, 1);
+
+        memory_region_init_alias(high_mmio_alias, OBJECT(dev), "pcie-mmio-high",
+                                 mmio_reg, base_mmio_high, size_mmio_high);
+        memory_region_add_subregion(get_system_memory(), base_mmio_high,
+                                    high_mmio_alias);
+    }
+
     /* Map IO port space */
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, base_pio);
 
@@ -727,11 +742,23 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic)
 
     qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
                                  2, base_ecam, 2, size_ecam);
-    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges",
-                                 1, FDT_PCI_RANGE_IOPORT, 2, 0,
-                                 2, base_pio, 2, size_pio,
-                                 1, FDT_PCI_RANGE_MMIO, 2, base_mmio,
-                                 2, base_mmio, 2, size_mmio);
+
+    if (use_highmem) {
+        qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges",
+                                     1, FDT_PCI_RANGE_IOPORT, 2, 0,
+                                     2, base_pio, 2, size_pio,
+                                     1, FDT_PCI_RANGE_MMIO, 2, base_mmio,
+                                     2, base_mmio, 2, size_mmio,
+                                     1, FDT_PCI_RANGE_MMIO_64BIT,
+                                     2, base_mmio_high,
+                                     2, base_mmio_high, 2, size_mmio_high);
+    } else {
+        qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges",
+                                     1, FDT_PCI_RANGE_IOPORT, 2, 0,
+                                     2, base_pio, 2, size_pio,
+                                     1, FDT_PCI_RANGE_MMIO, 2, base_mmio,
+                                     2, base_mmio, 2, size_mmio);
+    }
 
     qemu_fdt_setprop_cell(vbi->fdt, nodename, "#interrupt-cells", 1);
     create_pcie_irq_map(vbi, vbi->gic_phandle, irq, nodename);
@@ -889,7 +916,7 @@ static void machvirt_init(MachineState *machine)
 
     create_rtc(vbi, pic);
 
-    create_pcie(vbi, pic);
+    create_pcie(vbi, pic, vms->highmem);
 
     /* Create mmio transports, so the user can create virtio backends
      * (which will be automatically plugged in to the transports). If
@@ -904,6 +931,7 @@ static void machvirt_init(MachineState *machine)
     guest_info->fw_cfg = fw_cfg_find();
     guest_info->memmap = vbi->memmap;
     guest_info->irqmap = vbi->irqmap;
+    guest_info->use_highmem = vms->highmem;
     guest_info_state->machine_done.notify = virt_guest_info_machine_done;
     qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
 
@@ -941,6 +969,20 @@ static void virt_set_secure(Object *obj, bool value, Error **errp)
     vms->secure = value;
 }
 
+static bool virt_get_highmem(Object *obj, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    return vms->highmem;
+}
+
+static void virt_set_highmem(Object *obj, bool value, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    vms->highmem = value;
+}
+
 static void virt_instance_init(Object *obj)
 {
     VirtMachineState *vms = VIRT_MACHINE(obj);
@@ -953,6 +995,15 @@ static void virt_instance_init(Object *obj)
                                     "Set on/off to enable/disable the ARM "
                                     "Security Extensions (TrustZone)",
                                     NULL);
+
+    /* High memory is enabled by default */
+    vms->highmem = true;
+    object_property_add_bool(obj, "highmem", virt_get_highmem,
+                             virt_set_highmem, NULL);
+    object_property_set_description(obj, "highmem",
+                                    "Set on/off to enable/disable using "
+                                    "physical address space above 32 bits",
+                                    NULL);
 }
 
 static void virt_class_init(ObjectClass *oc, void *data)
diff --git a/include/hw/arm/virt-acpi-build.h b/include/hw/arm/virt-acpi-build.h
index 04f174d..19b68a4 100644
--- a/include/hw/arm/virt-acpi-build.h
+++ b/include/hw/arm/virt-acpi-build.h
@@ -31,6 +31,7 @@ typedef struct VirtGuestInfo {
     FWCfgState *fw_cfg;
     const MemMapEntry *memmap;
     const int *irqmap;
+    bool use_highmem;
 } VirtGuestInfo;
 
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index d22fd8e..808753f 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -56,6 +56,7 @@ enum {
     VIRT_PCIE_ECAM,
     VIRT_GIC_V2M,
     VIRT_PLATFORM_BUS,
+    VIRT_PCIE_MMIO_HIGH,
 };
 
 typedef struct MemMapEntry {
-- 
1.9.5.msysgit.0

Re: [Qemu-devel] [PATCH v8] hw/arm/virt: Add high MMIO PCI region, 512G in size

[Alexander Graf]

On 24.08.15 00:31, Pavel Fedin wrote:
> This large region is necessary for some devices like ivshmem and video cards
> 32-bit kernels can be built without LPAE support. In this case such a kernel
> will not be able to use PCI controller which has windows in high addresses.
> In order to work around the problem, "highmem" option is introduced. It
> defaults to on on, but can be manually set to off in order to be able to run
> those old 32-bit guests.
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>

Except for the ACPI parts:

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

Re: [Qemu-devel] [PATCH v8] hw/arm/virt: Add high MMIO PCI region, 512G in size

[Igor Mammedov]

On Mon, 24 Aug 2015 10:31:54 +0300
Pavel Fedin <p.fedin@samsung.com> wrote:

> This large region is necessary for some devices like ivshmem and video cards
> 32-bit kernels can be built without LPAE support. In this case such a kernel
> will not be able to use PCI controller which has windows in high addresses.
> In order to work around the problem, "highmem" option is introduced. It
> defaults to on on, but can be manually set to off in order to be able to run
> those old 32-bit guests.
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>

For ACPI parts:
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
> v7 => v8:
> - Split up between region and FDT creation
> - Renamed alias variable to high_mmio_alias
> v6 => v7:
> - Renamed alias variable to mmio64_alias
> v5 => v6:
> - Specify correct FDT_PCI_RANGE_MMIO_64BIT type for the region, the bug
>   was discovered by running UEFI
> v4 => v5:
> - Removed machine-dependent "highmem" default, now always ON
> v3 => v4:
> - Added "highmem" option which controls presence of this region. Default
>   value is on for 64-bit CPUs and off for 32-bit CPUs.
> - Supply correct min and max address to aml_qword_memory()
> v2 => v3:
> - Region size increased to 512G
> - Added ACPI description
> v1 => v2:
> - Region address changed to 512G, leaving more space for RAM
> ---
>  hw/arm/virt-acpi-build.c         | 17 +++++++++--
>  hw/arm/virt.c                    | 65 +++++++++++++++++++++++++++++++++++-----
>  include/hw/arm/virt-acpi-build.h |  1 +
>  include/hw/arm/virt.h            |  1 +
>  4 files changed, 75 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index f365140..9088248 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -159,7 +159,8 @@ static void acpi_dsdt_add_virtio(Aml *scope,
>      }
>  }
>  
> -static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, int irq)
> +static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, int irq,
> +                              bool use_highmem)
>  {
>      Aml *method, *crs, *ifctx, *UUID, *ifctx1, *elsectx, *buf;
>      int i, bus_no;
> @@ -234,6 +235,17 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, int irq)
>                       AML_ENTIRE_RANGE, 0x0000, 0x0000, size_pio - 1, base_pio,
>                       size_pio));
>  
> +    if (use_highmem) {
> +        hwaddr base_mmio_high = memmap[VIRT_PCIE_MMIO_HIGH].base;
> +        hwaddr size_mmio_high = memmap[VIRT_PCIE_MMIO_HIGH].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, 0x0000,
> +                             size_mmio_high));
> +    }
> +
>      aml_append(method, aml_name_decl("RBUF", rbuf));
>      aml_append(method, aml_return(rbuf));
>      aml_append(dev, method);
> @@ -510,7 +522,8 @@ build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>      acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
>      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));
> +    acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] + ARM_SPI_BASE),
> +                      guest_info->use_highmem);
>  
>      aml_append(dsdt, scope);
>  
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d5a8417..003a47c 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -79,6 +79,7 @@ typedef struct {
>  typedef struct {
>      MachineState parent;
>      bool secure;
> +    bool highmem;
>  } VirtMachineState;
>  
>  #define TYPE_VIRT_MACHINE   "virt"
> @@ -119,6 +120,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
>      [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },
>      [VIRT_MEM] =                { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
> +    [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000, 0x8000000000 },
>  };
>  
>  static const int a15irqmap[] = {
> @@ -666,10 +668,13 @@ static void create_pcie_irq_map(const VirtBoardInfo *vbi, uint32_t
> gic_phandle,
>                             0x7           /* PCI irq */);
>  }
>  
> -static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic)
> +static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic,
> +                        bool use_highmem)
>  {
>      hwaddr base_mmio = vbi->memmap[VIRT_PCIE_MMIO].base;
>      hwaddr size_mmio = vbi->memmap[VIRT_PCIE_MMIO].size;
> +    hwaddr base_mmio_high = vbi->memmap[VIRT_PCIE_MMIO_HIGH].base;
> +    hwaddr size_mmio_high = vbi->memmap[VIRT_PCIE_MMIO_HIGH].size;
>      hwaddr base_pio = vbi->memmap[VIRT_PCIE_PIO].base;
>      hwaddr size_pio = vbi->memmap[VIRT_PCIE_PIO].size;
>      hwaddr base_ecam = vbi->memmap[VIRT_PCIE_ECAM].base;
> @@ -706,6 +711,16 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic)
>                               mmio_reg, base_mmio, size_mmio);
>      memory_region_add_subregion(get_system_memory(), base_mmio, mmio_alias);
>  
> +    if (use_highmem) {
> +        /* Map high MMIO space */
> +        MemoryRegion *high_mmio_alias = g_new0(MemoryRegion, 1);
> +
> +        memory_region_init_alias(high_mmio_alias, OBJECT(dev), "pcie-mmio-high",
> +                                 mmio_reg, base_mmio_high, size_mmio_high);
> +        memory_region_add_subregion(get_system_memory(), base_mmio_high,
> +                                    high_mmio_alias);
> +    }
> +
>      /* Map IO port space */
>      sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, base_pio);
>  
> @@ -727,11 +742,23 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic)
>  
>      qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
>                                   2, base_ecam, 2, size_ecam);
> -    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges",
> -                                 1, FDT_PCI_RANGE_IOPORT, 2, 0,
> -                                 2, base_pio, 2, size_pio,
> -                                 1, FDT_PCI_RANGE_MMIO, 2, base_mmio,
> -                                 2, base_mmio, 2, size_mmio);
> +
> +    if (use_highmem) {
> +        qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges",
> +                                     1, FDT_PCI_RANGE_IOPORT, 2, 0,
> +                                     2, base_pio, 2, size_pio,
> +                                     1, FDT_PCI_RANGE_MMIO, 2, base_mmio,
> +                                     2, base_mmio, 2, size_mmio,
> +                                     1, FDT_PCI_RANGE_MMIO_64BIT,
> +                                     2, base_mmio_high,
> +                                     2, base_mmio_high, 2, size_mmio_high);
> +    } else {
> +        qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges",
> +                                     1, FDT_PCI_RANGE_IOPORT, 2, 0,
> +                                     2, base_pio, 2, size_pio,
> +                                     1, FDT_PCI_RANGE_MMIO, 2, base_mmio,
> +                                     2, base_mmio, 2, size_mmio);
> +    }
>  
>      qemu_fdt_setprop_cell(vbi->fdt, nodename, "#interrupt-cells", 1);
>      create_pcie_irq_map(vbi, vbi->gic_phandle, irq, nodename);
> @@ -889,7 +916,7 @@ static void machvirt_init(MachineState *machine)
>  
>      create_rtc(vbi, pic);
>  
> -    create_pcie(vbi, pic);
> +    create_pcie(vbi, pic, vms->highmem);
>  
>      /* Create mmio transports, so the user can create virtio backends
>       * (which will be automatically plugged in to the transports). If
> @@ -904,6 +931,7 @@ static void machvirt_init(MachineState *machine)
>      guest_info->fw_cfg = fw_cfg_find();
>      guest_info->memmap = vbi->memmap;
>      guest_info->irqmap = vbi->irqmap;
> +    guest_info->use_highmem = vms->highmem;
>      guest_info_state->machine_done.notify = virt_guest_info_machine_done;
>      qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
>  
> @@ -941,6 +969,20 @@ static void virt_set_secure(Object *obj, bool value, Error **errp)
>      vms->secure = value;
>  }
>  
> +static bool virt_get_highmem(Object *obj, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    return vms->highmem;
> +}
> +
> +static void virt_set_highmem(Object *obj, bool value, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    vms->highmem = value;
> +}
> +
>  static void virt_instance_init(Object *obj)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(obj);
> @@ -953,6 +995,15 @@ static void virt_instance_init(Object *obj)
>                                      "Set on/off to enable/disable the ARM "
>                                      "Security Extensions (TrustZone)",
>                                      NULL);
> +
> +    /* High memory is enabled by default */
> +    vms->highmem = true;
> +    object_property_add_bool(obj, "highmem", virt_get_highmem,
> +                             virt_set_highmem, NULL);
> +    object_property_set_description(obj, "highmem",
> +                                    "Set on/off to enable/disable using "
> +                                    "physical address space above 32 bits",
> +                                    NULL);
>  }
>  
>  static void virt_class_init(ObjectClass *oc, void *data)
> diff --git a/include/hw/arm/virt-acpi-build.h b/include/hw/arm/virt-acpi-build.h
> index 04f174d..19b68a4 100644
> --- a/include/hw/arm/virt-acpi-build.h
> +++ b/include/hw/arm/virt-acpi-build.h
> @@ -31,6 +31,7 @@ typedef struct VirtGuestInfo {
>      FWCfgState *fw_cfg;
>      const MemMapEntry *memmap;
>      const int *irqmap;
> +    bool use_highmem;
>  } VirtGuestInfo;
>  
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index d22fd8e..808753f 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -56,6 +56,7 @@ enum {
>      VIRT_PCIE_ECAM,
>      VIRT_GIC_V2M,
>      VIRT_PLATFORM_BUS,
> +    VIRT_PCIE_MMIO_HIGH,
>  };
>  
>  typedef struct MemMapEntry {

Re: [Qemu-devel] [PATCH v8] hw/arm/virt: Add high MMIO PCI region, 512G in size

[Shannon Zhao]

On 2015/8/24 15:31, Pavel Fedin wrote:
> This large region is necessary for some devices like ivshmem and video cards
> 32-bit kernels can be built without LPAE support. In this case such a kernel
> will not be able to use PCI controller which has windows in high addresses.
> In order to work around the problem, "highmem" option is introduced. It
> defaults to on on, but can be manually set to off in order to be able to run
> those old 32-bit guests.
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>

The change to virt ACPI is fine to me.

Reviewed-by: Shannon Zhao <shannon.zhao@linaro.org>

> ---
> v7 => v8:
> - Split up between region and FDT creation
> - Renamed alias variable to high_mmio_alias
> v6 => v7:
> - Renamed alias variable to mmio64_alias
> v5 => v6:
> - Specify correct FDT_PCI_RANGE_MMIO_64BIT type for the region, the bug
>    was discovered by running UEFI
> v4 => v5:
> - Removed machine-dependent "highmem" default, now always ON
> v3 => v4:
> - Added "highmem" option which controls presence of this region. Default
>    value is on for 64-bit CPUs and off for 32-bit CPUs.
> - Supply correct min and max address to aml_qword_memory()
> v2 => v3:
> - Region size increased to 512G
> - Added ACPI description
> v1 => v2:
> - Region address changed to 512G, leaving more space for RAM
> ---
>   hw/arm/virt-acpi-build.c         | 17 +++++++++--
>   hw/arm/virt.c                    | 65 +++++++++++++++++++++++++++++++++++-----
>   include/hw/arm/virt-acpi-build.h |  1 +
>   include/hw/arm/virt.h            |  1 +
>   4 files changed, 75 insertions(+), 9 deletions(-)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index f365140..9088248 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -159,7 +159,8 @@ static void acpi_dsdt_add_virtio(Aml *scope,
>       }
>   }
>
> -static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, int irq)
> +static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, int irq,
> +                              bool use_highmem)
>   {
>       Aml *method, *crs, *ifctx, *UUID, *ifctx1, *elsectx, *buf;
>       int i, bus_no;
> @@ -234,6 +235,17 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, int irq)
>                        AML_ENTIRE_RANGE, 0x0000, 0x0000, size_pio - 1, base_pio,
>                        size_pio));
>
> +    if (use_highmem) {
> +        hwaddr base_mmio_high = memmap[VIRT_PCIE_MMIO_HIGH].base;
> +        hwaddr size_mmio_high = memmap[VIRT_PCIE_MMIO_HIGH].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, 0x0000,
> +                             size_mmio_high));
> +    }
> +
>       aml_append(method, aml_name_decl("RBUF", rbuf));
>       aml_append(method, aml_return(rbuf));
>       aml_append(dev, method);
> @@ -510,7 +522,8 @@ build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>       acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
>       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));
> +    acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] + ARM_SPI_BASE),
> +                      guest_info->use_highmem);
>
>       aml_append(dsdt, scope);
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d5a8417..003a47c 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -79,6 +79,7 @@ typedef struct {
>   typedef struct {
>       MachineState parent;
>       bool secure;
> +    bool highmem;
>   } VirtMachineState;
>
>   #define TYPE_VIRT_MACHINE   "virt"
> @@ -119,6 +120,7 @@ static const MemMapEntry a15memmap[] = {
>       [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
>       [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },
>       [VIRT_MEM] =                { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
> +    [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000, 0x8000000000 },
>   };
>
>   static const int a15irqmap[] = {
> @@ -666,10 +668,13 @@ static void create_pcie_irq_map(const VirtBoardInfo *vbi, uint32_t
> gic_phandle,
>                              0x7           /* PCI irq */);
>   }
>
> -static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic)
> +static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic,
> +                        bool use_highmem)
>   {
>       hwaddr base_mmio = vbi->memmap[VIRT_PCIE_MMIO].base;
>       hwaddr size_mmio = vbi->memmap[VIRT_PCIE_MMIO].size;
> +    hwaddr base_mmio_high = vbi->memmap[VIRT_PCIE_MMIO_HIGH].base;
> +    hwaddr size_mmio_high = vbi->memmap[VIRT_PCIE_MMIO_HIGH].size;
>       hwaddr base_pio = vbi->memmap[VIRT_PCIE_PIO].base;
>       hwaddr size_pio = vbi->memmap[VIRT_PCIE_PIO].size;
>       hwaddr base_ecam = vbi->memmap[VIRT_PCIE_ECAM].base;
> @@ -706,6 +711,16 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic)
>                                mmio_reg, base_mmio, size_mmio);
>       memory_region_add_subregion(get_system_memory(), base_mmio, mmio_alias);
>
> +    if (use_highmem) {
> +        /* Map high MMIO space */
> +        MemoryRegion *high_mmio_alias = g_new0(MemoryRegion, 1);
> +
> +        memory_region_init_alias(high_mmio_alias, OBJECT(dev), "pcie-mmio-high",
> +                                 mmio_reg, base_mmio_high, size_mmio_high);
> +        memory_region_add_subregion(get_system_memory(), base_mmio_high,
> +                                    high_mmio_alias);
> +    }
> +
>       /* Map IO port space */
>       sysbus_mmio_map(SYS_BUS_DEVICE(dev), 2, base_pio);
>
> @@ -727,11 +742,23 @@ static void create_pcie(const VirtBoardInfo *vbi, qemu_irq *pic)
>
>       qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
>                                    2, base_ecam, 2, size_ecam);
> -    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges",
> -                                 1, FDT_PCI_RANGE_IOPORT, 2, 0,
> -                                 2, base_pio, 2, size_pio,
> -                                 1, FDT_PCI_RANGE_MMIO, 2, base_mmio,
> -                                 2, base_mmio, 2, size_mmio);
> +
> +    if (use_highmem) {
> +        qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges",
> +                                     1, FDT_PCI_RANGE_IOPORT, 2, 0,
> +                                     2, base_pio, 2, size_pio,
> +                                     1, FDT_PCI_RANGE_MMIO, 2, base_mmio,
> +                                     2, base_mmio, 2, size_mmio,
> +                                     1, FDT_PCI_RANGE_MMIO_64BIT,
> +                                     2, base_mmio_high,
> +                                     2, base_mmio_high, 2, size_mmio_high);
> +    } else {
> +        qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "ranges",
> +                                     1, FDT_PCI_RANGE_IOPORT, 2, 0,
> +                                     2, base_pio, 2, size_pio,
> +                                     1, FDT_PCI_RANGE_MMIO, 2, base_mmio,
> +                                     2, base_mmio, 2, size_mmio);
> +    }
>
>       qemu_fdt_setprop_cell(vbi->fdt, nodename, "#interrupt-cells", 1);
>       create_pcie_irq_map(vbi, vbi->gic_phandle, irq, nodename);
> @@ -889,7 +916,7 @@ static void machvirt_init(MachineState *machine)
>
>       create_rtc(vbi, pic);
>
> -    create_pcie(vbi, pic);
> +    create_pcie(vbi, pic, vms->highmem);
>
>       /* Create mmio transports, so the user can create virtio backends
>        * (which will be automatically plugged in to the transports). If
> @@ -904,6 +931,7 @@ static void machvirt_init(MachineState *machine)
>       guest_info->fw_cfg = fw_cfg_find();
>       guest_info->memmap = vbi->memmap;
>       guest_info->irqmap = vbi->irqmap;
> +    guest_info->use_highmem = vms->highmem;
>       guest_info_state->machine_done.notify = virt_guest_info_machine_done;
>       qemu_add_machine_init_done_notifier(&guest_info_state->machine_done);
>
> @@ -941,6 +969,20 @@ static void virt_set_secure(Object *obj, bool value, Error **errp)
>       vms->secure = value;
>   }
>
> +static bool virt_get_highmem(Object *obj, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    return vms->highmem;
> +}
> +
> +static void virt_set_highmem(Object *obj, bool value, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    vms->highmem = value;
> +}
> +
>   static void virt_instance_init(Object *obj)
>   {
>       VirtMachineState *vms = VIRT_MACHINE(obj);
> @@ -953,6 +995,15 @@ static void virt_instance_init(Object *obj)
>                                       "Set on/off to enable/disable the ARM "
>                                       "Security Extensions (TrustZone)",
>                                       NULL);
> +
> +    /* High memory is enabled by default */
> +    vms->highmem = true;
> +    object_property_add_bool(obj, "highmem", virt_get_highmem,
> +                             virt_set_highmem, NULL);
> +    object_property_set_description(obj, "highmem",
> +                                    "Set on/off to enable/disable using "
> +                                    "physical address space above 32 bits",
> +                                    NULL);
>   }
>
>   static void virt_class_init(ObjectClass *oc, void *data)
> diff --git a/include/hw/arm/virt-acpi-build.h b/include/hw/arm/virt-acpi-build.h
> index 04f174d..19b68a4 100644
> --- a/include/hw/arm/virt-acpi-build.h
> +++ b/include/hw/arm/virt-acpi-build.h
> @@ -31,6 +31,7 @@ typedef struct VirtGuestInfo {
>       FWCfgState *fw_cfg;
>       const MemMapEntry *memmap;
>       const int *irqmap;
> +    bool use_highmem;
>   } VirtGuestInfo;
>
>
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index d22fd8e..808753f 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -56,6 +56,7 @@ enum {
>       VIRT_PCIE_ECAM,
>       VIRT_GIC_V2M,
>       VIRT_PLATFORM_BUS,
> +    VIRT_PCIE_MMIO_HIGH,
>   };
>
>   typedef struct MemMapEntry {
>

-- 
Shannon

Re: [Qemu-devel] [PATCH v8] hw/arm/virt: Add high MMIO PCI region, 512G in size

[Peter Maydell]

On 24 August 2015 at 08:31, Pavel Fedin <p.fedin@samsung.com> wrote:
> This large region is necessary for some devices like ivshmem and video cards
> 32-bit kernels can be built without LPAE support. In this case such a kernel
> will not be able to use PCI controller which has windows in high addresses.
> In order to work around the problem, "highmem" option is introduced. It
> defaults to on on, but can be manually set to off in order to be able to run
> those old 32-bit guests.
>
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  #define TYPE_VIRT_MACHINE   "virt"
> @@ -119,6 +120,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_PCIE_PIO] =           { 0x3eff0000, 0x00010000 },
>      [VIRT_PCIE_ECAM] =          { 0x3f000000, 0x01000000 },
>      [VIRT_MEM] =                { 0x40000000, 30ULL * 1024 * 1024 * 1024 },
> +    [VIRT_PCIE_MMIO_HIGH] =   { 0x8000000000, 0x8000000000 },

These large constants need an ULL suffix, but I'll just add it rather than
forcing you to respin again.

I also added a comment
/* Second PCIe window, 512GB wide at the 512GB boundary */
for the benefit of people like me whose hex-to-gigabytes mental
arithmetic is rusty :-)

>  };
>
>  static const int a15irqmap[] = {
> @@ -666,10 +668,13 @@ static void create_pcie_irq_map(const VirtBoardInfo *vbi, uint32_t
> gic_phandle,

Something in your patch submission path seems to be wrapping long
lines. It would be nice if you could fix it -- I couldn't apply this
patch without manually editing it first.

> @@ -953,6 +995,15 @@ static void virt_instance_init(Object *obj)
>                                      "Set on/off to enable/disable the ARM "
>                                      "Security Extensions (TrustZone)",
>                                      NULL);
> +
> +    /* High memory is enabled by default */
> +    vms->highmem = true;
> +    object_property_add_bool(obj, "highmem", virt_get_highmem,
> +                             virt_set_highmem, NULL);
> +    object_property_set_description(obj, "highmem",
> +                                    "Set on/off to enable/disable using "
> +                                    "physical address space above 32 bits",
> +                                    NULL);
>  }

We should add a note to QEMU's changelog to mention that if
they have a 32-bit kernel on the virt board and the PCI has
stopped working then they need to use this option.
(http://wiki.qemu.org/ChangeLog/2.5)

Did you report the bug where the pci controller driver
fails to start if the second region is out of its range
to the kernel mailing list? (It would be nice to be able
to point to a kernel patch in the changelog too.)

Anyway, applied to target-arm.next.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8] hw/arm/virt: Add high MMIO PCI region, 512G in size

[Pavel Fedin]

 Hi!

> Something in your patch submission path seems to be wrapping long
> lines. It would be nice if you could fix it -- I couldn't apply this
> patch without manually editing it first.

 Damn, stupid Outlook... I already set maximum width to ~100, ok, will increase it further.

> We should add a note to QEMU's changelog to mention that if
> they have a 32-bit kernel on the virt board and the PCI has
> stopped working then they need to use this option.
> (http://wiki.qemu.org/ChangeLog/2.5)

 I tried to, but "log in" page seems to be missing "register" link, so i cannot create an account.

> Did you report the bug where the pci controller driver
> fails to start if the second region is out of its range
> to the kernel mailing list? (It would be nice to be able
> to point to a kernel patch in the changelog too.)

 I didn't yet, because have to time to retest it. Well, OK, will do it. But, can be already irrelevant because probably this was my fault - in previous version of the patch i forgot to add "64-bit" flag to the resource. So need to retest. Just don't have non-LPAE kernel build around at the moment.

> Anyway, applied to target-arm.next.

 Thank you very much.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH v8] hw/arm/virt: Add high MMIO PCI region, 512G in size

[Peter Maydell]

On 4 September 2015 at 08:13, Pavel Fedin <p.fedin@samsung.com> wrote:
>> We should add a note to QEMU's changelog to mention that if
>> they have a 32-bit kernel on the virt board and the PCI has
>> stopped working then they need to use this option.
>> (http://wiki.qemu.org/ChangeLog/2.5)
>
>  I tried to, but "log in" page seems to be missing "register"
> link, so i cannot create an account.

Yeah, we only let people with accounts create new ones, to
avoid spam. If you tell me what username you'd like I'll
create an account for you.

>> Did you report the bug where the pci controller driver
>> fails to start if the second region is out of its range
>> to the kernel mailing list? (It would be nice to be able
>> to point to a kernel patch in the changelog too.)
>
>  I didn't yet, because have to time to retest it. Well, OK,
> will do it. But, can be already irrelevant because probably
> this was my fault - in previous version of the patch i forgot
> to add "64-bit" flag to the resource. So need to retest.
> Just don't have non-LPAE kernel build around at the moment.

We need to actually confirm that there's a problem before
we put this in master, because if there aren't guests
that complain then we don't need to have the userfacing
switch which avoids using high memory.

Can you test this reasonably soon, please? Otherwise
I'll have to drop it from target-arm again.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8] hw/arm/virt: Add high MMIO PCI region, 512G in size

[Pavel Fedin]

 Hi!

> Yeah, we only let people with accounts create new ones, to
> avoid spam. If you tell me what username you'd like I'll
> create an account for you.

 p.fedin or pfedin, depending on whether dots are allowed or not.

> Can you test this reasonably soon, please? Otherwise
> I'll have to drop it from target-arm again.

 Ok, will test it hopefully today.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH v8] hw/arm/virt: Add high MMIO PCI region, 512G in size

[Pavel Fedin]

 Hello!

> Can you test this reasonably soon, please? Otherwise
> I'll have to drop it from target-arm again.

 Tested, confirm, the problem persists with Linux v4.1.4, LPAE disabled:
--- cut ---
PCI host bridge /pcie@10000000 ranges:
   IO 0x3eff0000..0x3effffff -> 0x00000000
  MEM 0x10000000..0x3efeffff -> 0x10000000
  MEM 0x8000000000..0xffffffffff -> 0x8000000000
pci-host-generic 3f000000.pcie: resource collision: [mem 0x00000000-0xffffffff] conflicts with /pl011@9000000 [mem 0x09000000-0x09000fff]
pci-host-generic: probe of 3f000000.pcie failed with error -16
--- cut ---

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH v8] hw/arm/virt: Add high MMIO PCI region, 512G in size

[Peter Maydell]

On 4 September 2015 at 00:13, Pavel Fedin <p.fedin@samsung.com> wrote:
> Peter Maydell wrote:
>> Did you report the bug where the pci controller driver
>> fails to start if the second region is out of its range
>> to the kernel mailing list? (It would be nice to be able
>> to point to a kernel patch in the changelog too.)
>
>  I didn't yet, because have to time to retest it. Well, OK, will
> do it.

Nudge -- have you reported this as a kernel bug against the
PCI generic driver yet?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8] hw/arm/virt: Add high MMIO PCI region, 512G in size

[Peter Crosthwaite]

On Thu, Sep 24, 2015 at 5:13 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 4 September 2015 at 00:13, Pavel Fedin <p.fedin@samsung.com> wrote:
>> Peter Maydell wrote:
>>> Did you report the bug where the pci controller driver
>>> fails to start if the second region is out of its range
>>> to the kernel mailing list? (It would be nice to be able
>>> to point to a kernel patch in the changelog too.)
>>
>>  I didn't yet, because have to time to retest it. Well, OK, will
>> do it.
>
> Nudge -- have you reported this as a kernel bug against the
> PCI generic driver yet?
>

So the use case that causes this bug is very vanilla (I just got bit).
Can we reverse the default of the highmem switch so the unmodified
command line continues to work?

Regards,
Peter

> than
> -- PMM
>

Re: [Qemu-devel] [PATCH v8] hw/arm/virt: Add high MMIO PCI region, 512G in size

[Peter Maydell]

On 3 October 2015 at 19:04, Peter Crosthwaite
<crosthwaitepeter@gmail.com> wrote:
> On Thu, Sep 24, 2015 at 5:13 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 4 September 2015 at 00:13, Pavel Fedin <p.fedin@samsung.com> wrote:
>>> Peter Maydell wrote:
>>>> Did you report the bug where the pci controller driver
>>>> fails to start if the second region is out of its range
>>>> to the kernel mailing list? (It would be nice to be able
>>>> to point to a kernel patch in the changelog too.)
>>>
>>>  I didn't yet, because have to time to retest it. Well, OK, will
>>> do it.
>>
>> Nudge -- have you reported this as a kernel bug against the
>> PCI generic driver yet?
>>
>
> So the use case that causes this bug is very vanilla (I just got bit).
> Can we reverse the default of the highmem switch so the unmodified
> command line continues to work?

That would then mean we practically never would use the
highmem window, and we'd have to tell everybody "you need
to use this extra weird command line option", which I don't
like.

Once again: has anybody reported this as a kernel bug?

thanks
-- PMM

Re: [Qemu-devel] [PATCH v8] hw/arm/virt: Add high MMIO PCI region, 512G in size

[Pavel Fedin]

 Hello!

> Nudge -- have you reported this as a kernel bug against the
> PCI generic driver yet?

 Sorry, stopped tracking this topic after option upstreaming. Just sent out patches, cc'ed to you.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH v8] hw/arm/virt: Add high MMIO PCI region, 512G in size

[Peter Maydell]

On 7 October 2015 at 11:50, Pavel Fedin <p.fedin@samsung.com> wrote:
>  Hello!
>
>> Nudge -- have you reported this as a kernel bug against the
>> PCI generic driver yet?
>
>  Sorry, stopped tracking this topic after option upstreaming. Just
> sent out patches, cc'ed to you.

Yes, just saw those, thanks. (I'm slightly surprised that
for_each_of_pci_range doesn't return OF_BAD_ADDR for
the out-of-range addresses, but I'll let the kernel
folk do the review, since I'm mostly just guessing about
good vs bad kernel code :-))

thanks
-- PMM