From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49235) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZTltN-0007uT-L5 for qemu-devel@nongnu.org; Mon, 24 Aug 2015 03:10:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZTltI-0002vW-Jn for qemu-devel@nongnu.org; Mon, 24 Aug 2015 03:10:05 -0400 Received: from mx2.suse.de ([195.135.220.15]:43236) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZTltI-0002st-Ad for qemu-devel@nongnu.org; Mon, 24 Aug 2015 03:10:00 -0400 References: <02ba01d0de3a$f9cbd920$ed638b60$@samsung.com> From: Alexander Graf Message-ID: <55DAC342.3040202@suse.de> Date: Mon, 24 Aug 2015 00:09:54 -0700 MIME-Version: 1.0 In-Reply-To: <02ba01d0de3a$f9cbd920$ed638b60$@samsung.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7] hw/arm/virt: Add high MMIO PCI region, 512G in size List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Fedin , qemu-devel@nongnu.org Cc: peter.maydell@linaro.org 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 > --- > 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