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

[Alexander Graf]

On 12.08.15 05:12, 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 <address@hidden>
> ---
> 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                    | 63 
> +++++++++++++++++++++++++++++++++++-----
>  include/hw/arm/virt-acpi-build.h |  1 +
>  include/hw/arm/virt.h            |  1 +
>  4 files changed, 73 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 4846892..44dcd0c 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -77,6 +77,7 @@ typedef struct {
>  typedef struct {
>      MachineState parent;
>      bool secure;
> +    bool highmem;
>  } VirtMachineState;
>  
>  #define TYPE_VIRT_MACHINE   "virt"
> @@ -117,6 +118,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[] = {
> @@ -658,7 +660,8 @@ 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;
> @@ -719,11 +722,33 @@ 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) {
> +        /* High MMIO space */
> +        hwaddr base_mmio_high = vbi->memmap[VIRT_PCIE_MMIO_HIGH].base;
> +        hwaddr size_mmio_high = vbi->memmap[VIRT_PCIE_MMIO_HIGH].size;
> +
> +        mmio_alias = g_new0(MemoryRegion, 1);
> +        memory_region_init_alias(mmio_alias, OBJECT(dev), "pcie-mmio-high",

Does this work? You're reinitializing the region after it's already been
passed to the PCIe initialization at this point, no?


Alex

> +                                 mmio_reg, base_mmio_high, size_mmio_high);
> +        memory_region_add_subregion(get_system_memory(), base_mmio_high,
> +                                    mmio_alias);
> +
> +        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);
> @@ -881,7 +906,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
> @@ -896,6 +921,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);
>  
> @@ -933,6 +959,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);
> @@ -945,6 +985,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 {
>