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

[Alexander Graf]

On 24.08.15 00:03, 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>
> ---
> 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                    | 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 d5a8417..91d975c 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,7 +668,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;
> @@ -727,11 +730,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;
> +        MemoryRegion *mmio64_alias = g_new0(MemoryRegion, 1);
> +
> +        memory_region_init_alias(mmio64_alias, OBJECT(dev), "pcie-mmio-high",
> +                                 mmio_reg, base_mmio_high, size_mmio_high);
> +        memory_region_add_subregion(get_system_memory(), base_mmio_high,
> +                                    mmio64_alias);

Sorry for making you go through all of this churn :(. Could you please
quickly split up the MMIO alias mapping and the FDT creation? Just have
2 if clauses and move the variable definitions for base_mmio_high and
size_mmio_high to the top of the function.

Also I'm not 100% happy with the naming deviation between "mmio64" and
mmio_high". It's a nit pick but since you're splitting this anyway,
please make sure that the naming is consistent throughout the file.


Thanks a lot!

Alex