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",
> +                                 mmio_reg, base_mmio_high, size_mmio_high);

Meh, some times I wish for the new gmail feature that allows you to not
send emails after you pressed send ;).

So you're just reusing the variable here and create a new alias into
mmio_reg. I think it would be more readable if you actually made that a
new variable as well.

The rest looks good. I can't really review the ACPI parts though.


Alex