* [Qemu-devel] [PATCH 0/2] ARM virt: Support up to 256 PCIe buses @ 2018-05-30 14:26 Eric Auger 2018-05-30 14:26 ` [Qemu-devel] [PATCH 1/2] hw/arm/virt: Add a new 256MB ECAM region Eric Auger 2018-05-30 14:26 ` [Qemu-devel] [PATCH 2/2] hw/arm/virt: Add virt-3.0 machine type Eric Auger 0 siblings, 2 replies; 12+ messages in thread From: Eric Auger @ 2018-05-30 14:26 UTC (permalink / raw) To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell Cc: drjones, wei, zhaoshenglong, lersek, ard.biesheuvel Current Machvirt PCI host controller's ECAM region is 16MB large. This limits the number of PCIe buses to 16. PC/Q35 machines have a 256MB region allowing up to 256 buses. This series tries to bridge the gap. It declares a new ECAM region located beyond 256GB, of size 256MB The new ECAM region is used if: - highmem option is set (default) and, - either FW is not loaded or we are run an aarch64 guest - machine type >= 3.0. aarch32 FW does not support this highmem ECAM region. For guests without LPAE support the highmem option must be turned off. Best Regards Eric Git: complete series available at https://github.com/eauger/qemu/tree/v2.12.0-256MB-ECAM-PATCH-v1 Previous version: https://github.com/eauger/qemu/tree/v2.12.0-256MB-ECAM-RFCv1 - Tested with guest running in aarch64 and aarch32 modes (aarch64=off) - Tested with aarch32 FW - In aarch32 mode I encountered the issue the vmalloc region may be reported too small for the needs (dmesg excerpt below). So I had to extend the vmalloc size by passing the "vmalloc=512M" option to the bootargs and this eventually booted fine. [ 1.399581] pl061_gpio 9030000.pl061: PL061 GPIO chip @0x0000000009030000 registered [ 1.402636] OF: PCI: host bridge /pcie@10000000 ranges: [ 1.404506] OF: PCI: IO 0x3eff0000..0x3effffff -> 0x00000000 [ 1.406606] OF: PCI: MEM 0x10000000..0x3efeffff -> 0x10000000 [ 1.408690] OF: PCI: MEM 0x8000000000..0xffffffffff -> 0x8000000000 [ 1.411992] vmap allocation for size 1052672 failed: use vmalloc=<size> to increase size [ 1.414895] pci-host-generic 4010000000.pcie: ECAM ioremap failed [ 1.427472] pci-host-generic: probe of 4010000000.pcie failed with error -12 Eric Auger (2): hw/arm/virt: Add a new 256MB ECAM region hw/arm/virt: Add virt-3.0 machine type hw/arm/virt-acpi-build.c | 21 +++++++++++++-------- hw/arm/virt.c | 43 ++++++++++++++++++++++++++++++++++++------- include/hw/arm/virt.h | 3 +++ 3 files changed, 52 insertions(+), 15 deletions(-) -- 2.5.5 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH 1/2] hw/arm/virt: Add a new 256MB ECAM region 2018-05-30 14:26 [Qemu-devel] [PATCH 0/2] ARM virt: Support up to 256 PCIe buses Eric Auger @ 2018-05-30 14:26 ` Eric Auger 2018-05-30 16:11 ` Laszlo Ersek 2018-05-30 14:26 ` [Qemu-devel] [PATCH 2/2] hw/arm/virt: Add virt-3.0 machine type Eric Auger 1 sibling, 1 reply; 12+ messages in thread From: Eric Auger @ 2018-05-30 14:26 UTC (permalink / raw) To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell Cc: drjones, wei, zhaoshenglong, lersek, ard.biesheuvel This patch defines a new ECAM region located after the 256GB limit. The virt machine state is augmented with a new highmem_ecam field which guards the usage of this new ECAM region instead of the legacy 16MB one. With the highmem ECAM region, up to 256 PCIe buses can be used. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- RFC -> PATCH: - remove the newline at the end of acpi_dsdt_add_pci - use vms->highmem_ecam to select the memmap id --- hw/arm/virt-acpi-build.c | 21 +++++++++++++-------- hw/arm/virt.c | 12 ++++++++---- include/hw/arm/virt.h | 2 ++ 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index 92ceee9..c0370b2 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -150,16 +150,17 @@ static void acpi_dsdt_add_virtio(Aml *scope, } static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, - uint32_t irq, bool use_highmem) + uint32_t irq, bool use_highmem, bool highmem_ecam) { + int ecam_id = highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM; Aml *method, *crs, *ifctx, *UUID, *ifctx1, *elsectx, *buf; int i, bus_no; hwaddr base_mmio = memmap[VIRT_PCIE_MMIO].base; hwaddr size_mmio = memmap[VIRT_PCIE_MMIO].size; hwaddr base_pio = memmap[VIRT_PCIE_PIO].base; hwaddr size_pio = memmap[VIRT_PCIE_PIO].size; - hwaddr base_ecam = memmap[VIRT_PCIE_ECAM].base; - hwaddr size_ecam = memmap[VIRT_PCIE_ECAM].size; + hwaddr base_ecam = memmap[ecam_id].base; + hwaddr size_ecam = memmap[ecam_id].size; int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN; Aml *dev = aml_device("%s", "PCI0"); @@ -173,7 +174,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, aml_append(dev, aml_name_decl("_CCA", aml_int(1))); /* Declare the PCI Routing Table. */ - Aml *rt_pkg = aml_package(nr_pcie_buses * PCI_NUM_PINS); + Aml *rt_pkg = aml_varpackage(nr_pcie_buses * PCI_NUM_PINS); for (bus_no = 0; bus_no < nr_pcie_buses; bus_no++) { for (i = 0; i < PCI_NUM_PINS; i++) { int gsi = (i + bus_no) % PCI_NUM_PINS; @@ -316,7 +317,10 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, Aml *dev_res0 = aml_device("%s", "RES0"); aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02"))); crs = aml_resource_template(); - aml_append(crs, aml_memory32_fixed(base_ecam, size_ecam, AML_READ_WRITE)); + aml_append(crs, + aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED, + AML_NON_CACHEABLE, AML_READ_WRITE, 0x0000, base_ecam, + base_ecam + size_ecam - 1, 0x0000, size_ecam)); aml_append(dev_res0, aml_name_decl("_CRS", crs)); aml_append(dev, dev_res0); aml_append(scope, dev); @@ -563,16 +567,17 @@ build_mcfg(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) { AcpiTableMcfg *mcfg; const MemMapEntry *memmap = vms->memmap; + int ecam_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM; int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]); int mcfg_start = table_data->len; mcfg = acpi_data_push(table_data, len); - mcfg->allocation[0].address = cpu_to_le64(memmap[VIRT_PCIE_ECAM].base); + mcfg->allocation[0].address = cpu_to_le64(memmap[ecam_id].base); /* Only a single allocation so no need to play with segments */ mcfg->allocation[0].pci_segment = cpu_to_le16(0); mcfg->allocation[0].start_bus_number = 0; - mcfg->allocation[0].end_bus_number = (memmap[VIRT_PCIE_ECAM].size + mcfg->allocation[0].end_bus_number = (memmap[ecam_id].size / PCIE_MMCFG_SIZE_MIN) - 1; build_header(linker, table_data, (void *)(table_data->data + mcfg_start), @@ -747,7 +752,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) 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), - vms->highmem); + vms->highmem, vms->highmem_ecam); acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO], (irqmap[VIRT_GPIO] + ARM_SPI_BASE)); acpi_dsdt_add_power_button(scope); diff --git a/hw/arm/virt.c b/hw/arm/virt.c index a3a28e2..d4247d0 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -149,6 +149,7 @@ static const MemMapEntry a15memmap[] = { [VIRT_PCIE_PIO] = { 0x3eff0000, 0x00010000 }, [VIRT_PCIE_ECAM] = { 0x3f000000, 0x01000000 }, [VIRT_MEM] = { 0x40000000, RAMLIMIT_BYTES }, + [VIRT_PCIE_ECAM_HIGH] = { 0x4010000000ULL, 0x10000000 }, /* Second PCIe window, 512GB wide at the 512GB boundary */ [VIRT_PCIE_MMIO_HIGH] = { 0x8000000000ULL, 0x8000000000ULL }, }; @@ -1001,10 +1002,9 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic) hwaddr size_mmio_high = vms->memmap[VIRT_PCIE_MMIO_HIGH].size; hwaddr base_pio = vms->memmap[VIRT_PCIE_PIO].base; hwaddr size_pio = vms->memmap[VIRT_PCIE_PIO].size; - hwaddr base_ecam = vms->memmap[VIRT_PCIE_ECAM].base; - hwaddr size_ecam = vms->memmap[VIRT_PCIE_ECAM].size; + hwaddr base_ecam, size_ecam; hwaddr base = base_mmio; - int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN; + int nr_pcie_buses; int irq = vms->irqmap[VIRT_PCIE]; MemoryRegion *mmio_alias; MemoryRegion *mmio_reg; @@ -1012,12 +1012,16 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic) MemoryRegion *ecam_reg; DeviceState *dev; char *nodename; - int i; + int i, ecam_memmap_id; PCIHostState *pci; dev = qdev_create(NULL, TYPE_GPEX_HOST); qdev_init_nofail(dev); + ecam_memmap_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM; + base_ecam = vms->memmap[ecam_memmap_id].base; + size_ecam = vms->memmap[ecam_memmap_id].size; + nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN; /* Map only the first size_ecam bytes of ECAM space */ ecam_alias = g_new0(MemoryRegion, 1); ecam_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0); diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index 4ac7ef6..e9423a7 100644 --- a/include/hw/arm/virt.h +++ b/include/hw/arm/virt.h @@ -69,6 +69,7 @@ enum { VIRT_PCIE_MMIO, VIRT_PCIE_PIO, VIRT_PCIE_ECAM, + VIRT_PCIE_ECAM_HIGH, VIRT_PLATFORM_BUS, VIRT_PCIE_MMIO_HIGH, VIRT_GPIO, @@ -103,6 +104,7 @@ typedef struct { FWCfgState *fw_cfg; bool secure; bool highmem; + bool highmem_ecam; bool its; bool virt; int32_t gic_version; -- 2.5.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/arm/virt: Add a new 256MB ECAM region 2018-05-30 14:26 ` [Qemu-devel] [PATCH 1/2] hw/arm/virt: Add a new 256MB ECAM region Eric Auger @ 2018-05-30 16:11 ` Laszlo Ersek 2018-05-31 6:55 ` Auger Eric 0 siblings, 1 reply; 12+ messages in thread From: Laszlo Ersek @ 2018-05-30 16:11 UTC (permalink / raw) To: Eric Auger, eric.auger.pro, qemu-devel, qemu-arm, peter.maydell Cc: drjones, wei, zhaoshenglong, ard.biesheuvel On 05/30/18 16:26, Eric Auger wrote: > This patch defines a new ECAM region located after the 256GB limit. > > The virt machine state is augmented with a new highmem_ecam field > which guards the usage of this new ECAM region instead of the legacy > 16MB one. With the highmem ECAM region, up to 256 PCIe buses can be > used. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > RFC -> PATCH: > - remove the newline at the end of acpi_dsdt_add_pci > - use vms->highmem_ecam to select the memmap id > --- > hw/arm/virt-acpi-build.c | 21 +++++++++++++-------- > hw/arm/virt.c | 12 ++++++++---- > include/hw/arm/virt.h | 2 ++ > 3 files changed, 23 insertions(+), 12 deletions(-) > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 92ceee9..c0370b2 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -150,16 +150,17 @@ static void acpi_dsdt_add_virtio(Aml *scope, > } > > static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, > - uint32_t irq, bool use_highmem) > + uint32_t irq, bool use_highmem, bool highmem_ecam) > { > + int ecam_id = highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM; > Aml *method, *crs, *ifctx, *UUID, *ifctx1, *elsectx, *buf; > int i, bus_no; > hwaddr base_mmio = memmap[VIRT_PCIE_MMIO].base; > hwaddr size_mmio = memmap[VIRT_PCIE_MMIO].size; > hwaddr base_pio = memmap[VIRT_PCIE_PIO].base; > hwaddr size_pio = memmap[VIRT_PCIE_PIO].size; > - hwaddr base_ecam = memmap[VIRT_PCIE_ECAM].base; > - hwaddr size_ecam = memmap[VIRT_PCIE_ECAM].size; > + hwaddr base_ecam = memmap[ecam_id].base; > + hwaddr size_ecam = memmap[ecam_id].size; > int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN; > > Aml *dev = aml_device("%s", "PCI0"); > @@ -173,7 +174,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, > aml_append(dev, aml_name_decl("_CCA", aml_int(1))); > > /* Declare the PCI Routing Table. */ > - Aml *rt_pkg = aml_package(nr_pcie_buses * PCI_NUM_PINS); > + Aml *rt_pkg = aml_varpackage(nr_pcie_buses * PCI_NUM_PINS); > for (bus_no = 0; bus_no < nr_pcie_buses; bus_no++) { > for (i = 0; i < PCI_NUM_PINS; i++) { > int gsi = (i + bus_no) % PCI_NUM_PINS; > @@ -316,7 +317,10 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, > Aml *dev_res0 = aml_device("%s", "RES0"); > aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02"))); > crs = aml_resource_template(); > - aml_append(crs, aml_memory32_fixed(base_ecam, size_ecam, AML_READ_WRITE)); > + aml_append(crs, > + aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED, > + AML_NON_CACHEABLE, AML_READ_WRITE, 0x0000, base_ecam, > + base_ecam + size_ecam - 1, 0x0000, size_ecam)); > aml_append(dev_res0, aml_name_decl("_CRS", crs)); > aml_append(dev, dev_res0); > aml_append(scope, dev); > @@ -563,16 +567,17 @@ build_mcfg(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > { > AcpiTableMcfg *mcfg; > const MemMapEntry *memmap = vms->memmap; > + int ecam_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM; > int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]); > int mcfg_start = table_data->len; > > mcfg = acpi_data_push(table_data, len); > - mcfg->allocation[0].address = cpu_to_le64(memmap[VIRT_PCIE_ECAM].base); > + mcfg->allocation[0].address = cpu_to_le64(memmap[ecam_id].base); > > /* Only a single allocation so no need to play with segments */ > mcfg->allocation[0].pci_segment = cpu_to_le16(0); > mcfg->allocation[0].start_bus_number = 0; > - mcfg->allocation[0].end_bus_number = (memmap[VIRT_PCIE_ECAM].size > + mcfg->allocation[0].end_bus_number = (memmap[ecam_id].size > / PCIE_MMCFG_SIZE_MIN) - 1; > > build_header(linker, table_data, (void *)(table_data->data + mcfg_start), > @@ -747,7 +752,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > 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), > - vms->highmem); > + vms->highmem, vms->highmem_ecam); > acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO], > (irqmap[VIRT_GPIO] + ARM_SPI_BASE)); > acpi_dsdt_add_power_button(scope); > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index a3a28e2..d4247d0 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -149,6 +149,7 @@ static const MemMapEntry a15memmap[] = { > [VIRT_PCIE_PIO] = { 0x3eff0000, 0x00010000 }, > [VIRT_PCIE_ECAM] = { 0x3f000000, 0x01000000 }, > [VIRT_MEM] = { 0x40000000, RAMLIMIT_BYTES }, > + [VIRT_PCIE_ECAM_HIGH] = { 0x4010000000ULL, 0x10000000 }, > /* Second PCIe window, 512GB wide at the 512GB boundary */ > [VIRT_PCIE_MMIO_HIGH] = { 0x8000000000ULL, 0x8000000000ULL }, > }; > @@ -1001,10 +1002,9 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic) > hwaddr size_mmio_high = vms->memmap[VIRT_PCIE_MMIO_HIGH].size; > hwaddr base_pio = vms->memmap[VIRT_PCIE_PIO].base; > hwaddr size_pio = vms->memmap[VIRT_PCIE_PIO].size; > - hwaddr base_ecam = vms->memmap[VIRT_PCIE_ECAM].base; > - hwaddr size_ecam = vms->memmap[VIRT_PCIE_ECAM].size; > + hwaddr base_ecam, size_ecam; > hwaddr base = base_mmio; > - int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN; > + int nr_pcie_buses; > int irq = vms->irqmap[VIRT_PCIE]; > MemoryRegion *mmio_alias; > MemoryRegion *mmio_reg; > @@ -1012,12 +1012,16 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic) > MemoryRegion *ecam_reg; > DeviceState *dev; > char *nodename; > - int i; > + int i, ecam_memmap_id; > PCIHostState *pci; > > dev = qdev_create(NULL, TYPE_GPEX_HOST); > qdev_init_nofail(dev); > > + ecam_memmap_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM; > + base_ecam = vms->memmap[ecam_memmap_id].base; > + size_ecam = vms->memmap[ecam_memmap_id].size; > + nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN; > /* Map only the first size_ecam bytes of ECAM space */ > ecam_alias = g_new0(MemoryRegion, 1); > ecam_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0); > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h > index 4ac7ef6..e9423a7 100644 > --- a/include/hw/arm/virt.h > +++ b/include/hw/arm/virt.h > @@ -69,6 +69,7 @@ enum { > VIRT_PCIE_MMIO, > VIRT_PCIE_PIO, > VIRT_PCIE_ECAM, > + VIRT_PCIE_ECAM_HIGH, > VIRT_PLATFORM_BUS, > VIRT_PCIE_MMIO_HIGH, > VIRT_GPIO, > @@ -103,6 +104,7 @@ typedef struct { > FWCfgState *fw_cfg; > bool secure; > bool highmem; > + bool highmem_ecam; > bool its; > bool virt; > int32_t gic_version; > At this point, the order (and values) of the VIRT_* enum constants look mostly random :) , but I'm unaware of anything why that should be a problem. I compared this against the RFC version; from my side: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Can you please do the following additionally: - The aarch64 firmware packaged in Gerd's repo is built with the verbose log enabled, as far as I can see. Can you please capture two boot logs (from the serial console) until the guest kernel is reached, and send them to me (off-list if you prefer)? The first log should have the new feature disabled, the second one enabled; I'd like to compare them. - (I think the same test is useful with the guest kernel dmesg as well, passing ignore_loglevel, but I don't think my assistance in reviewing that difference is necessary or helpful :) ) Thanks! Laszlo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/arm/virt: Add a new 256MB ECAM region 2018-05-30 16:11 ` Laszlo Ersek @ 2018-05-31 6:55 ` Auger Eric 2018-05-31 8:41 ` Laszlo Ersek 0 siblings, 1 reply; 12+ messages in thread From: Auger Eric @ 2018-05-31 6:55 UTC (permalink / raw) To: Laszlo Ersek, eric.auger.pro, qemu-devel, qemu-arm, peter.maydell Cc: wei, drjones, ard.biesheuvel, zhaoshenglong Hi Laszlo, On 05/30/2018 06:11 PM, Laszlo Ersek wrote: > On 05/30/18 16:26, Eric Auger wrote: >> This patch defines a new ECAM region located after the 256GB limit. >> >> The virt machine state is augmented with a new highmem_ecam field >> which guards the usage of this new ECAM region instead of the legacy >> 16MB one. With the highmem ECAM region, up to 256 PCIe buses can be >> used. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> --- >> >> RFC -> PATCH: >> - remove the newline at the end of acpi_dsdt_add_pci >> - use vms->highmem_ecam to select the memmap id >> --- >> hw/arm/virt-acpi-build.c | 21 +++++++++++++-------- >> hw/arm/virt.c | 12 ++++++++---- >> include/hw/arm/virt.h | 2 ++ >> 3 files changed, 23 insertions(+), 12 deletions(-) >> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c >> index 92ceee9..c0370b2 100644 >> --- a/hw/arm/virt-acpi-build.c >> +++ b/hw/arm/virt-acpi-build.c >> @@ -150,16 +150,17 @@ static void acpi_dsdt_add_virtio(Aml *scope, >> } >> >> static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, >> - uint32_t irq, bool use_highmem) >> + uint32_t irq, bool use_highmem, bool highmem_ecam) >> { >> + int ecam_id = highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM; >> Aml *method, *crs, *ifctx, *UUID, *ifctx1, *elsectx, *buf; >> int i, bus_no; >> hwaddr base_mmio = memmap[VIRT_PCIE_MMIO].base; >> hwaddr size_mmio = memmap[VIRT_PCIE_MMIO].size; >> hwaddr base_pio = memmap[VIRT_PCIE_PIO].base; >> hwaddr size_pio = memmap[VIRT_PCIE_PIO].size; >> - hwaddr base_ecam = memmap[VIRT_PCIE_ECAM].base; >> - hwaddr size_ecam = memmap[VIRT_PCIE_ECAM].size; >> + hwaddr base_ecam = memmap[ecam_id].base; >> + hwaddr size_ecam = memmap[ecam_id].size; >> int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN; >> >> Aml *dev = aml_device("%s", "PCI0"); >> @@ -173,7 +174,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, >> aml_append(dev, aml_name_decl("_CCA", aml_int(1))); >> >> /* Declare the PCI Routing Table. */ >> - Aml *rt_pkg = aml_package(nr_pcie_buses * PCI_NUM_PINS); >> + Aml *rt_pkg = aml_varpackage(nr_pcie_buses * PCI_NUM_PINS); >> for (bus_no = 0; bus_no < nr_pcie_buses; bus_no++) { >> for (i = 0; i < PCI_NUM_PINS; i++) { >> int gsi = (i + bus_no) % PCI_NUM_PINS; >> @@ -316,7 +317,10 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, >> Aml *dev_res0 = aml_device("%s", "RES0"); >> aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02"))); >> crs = aml_resource_template(); >> - aml_append(crs, aml_memory32_fixed(base_ecam, size_ecam, AML_READ_WRITE)); >> + aml_append(crs, >> + aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED, >> + AML_NON_CACHEABLE, AML_READ_WRITE, 0x0000, base_ecam, >> + base_ecam + size_ecam - 1, 0x0000, size_ecam)); >> aml_append(dev_res0, aml_name_decl("_CRS", crs)); >> aml_append(dev, dev_res0); >> aml_append(scope, dev); >> @@ -563,16 +567,17 @@ build_mcfg(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) >> { >> AcpiTableMcfg *mcfg; >> const MemMapEntry *memmap = vms->memmap; >> + int ecam_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM; >> int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]); >> int mcfg_start = table_data->len; >> >> mcfg = acpi_data_push(table_data, len); >> - mcfg->allocation[0].address = cpu_to_le64(memmap[VIRT_PCIE_ECAM].base); >> + mcfg->allocation[0].address = cpu_to_le64(memmap[ecam_id].base); >> >> /* Only a single allocation so no need to play with segments */ >> mcfg->allocation[0].pci_segment = cpu_to_le16(0); >> mcfg->allocation[0].start_bus_number = 0; >> - mcfg->allocation[0].end_bus_number = (memmap[VIRT_PCIE_ECAM].size >> + mcfg->allocation[0].end_bus_number = (memmap[ecam_id].size >> / PCIE_MMCFG_SIZE_MIN) - 1; >> >> build_header(linker, table_data, (void *)(table_data->data + mcfg_start), >> @@ -747,7 +752,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) >> 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), >> - vms->highmem); >> + vms->highmem, vms->highmem_ecam); >> acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO], >> (irqmap[VIRT_GPIO] + ARM_SPI_BASE)); >> acpi_dsdt_add_power_button(scope); >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index a3a28e2..d4247d0 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -149,6 +149,7 @@ static const MemMapEntry a15memmap[] = { >> [VIRT_PCIE_PIO] = { 0x3eff0000, 0x00010000 }, >> [VIRT_PCIE_ECAM] = { 0x3f000000, 0x01000000 }, >> [VIRT_MEM] = { 0x40000000, RAMLIMIT_BYTES }, >> + [VIRT_PCIE_ECAM_HIGH] = { 0x4010000000ULL, 0x10000000 }, >> /* Second PCIe window, 512GB wide at the 512GB boundary */ >> [VIRT_PCIE_MMIO_HIGH] = { 0x8000000000ULL, 0x8000000000ULL }, >> }; >> @@ -1001,10 +1002,9 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic) >> hwaddr size_mmio_high = vms->memmap[VIRT_PCIE_MMIO_HIGH].size; >> hwaddr base_pio = vms->memmap[VIRT_PCIE_PIO].base; >> hwaddr size_pio = vms->memmap[VIRT_PCIE_PIO].size; >> - hwaddr base_ecam = vms->memmap[VIRT_PCIE_ECAM].base; >> - hwaddr size_ecam = vms->memmap[VIRT_PCIE_ECAM].size; >> + hwaddr base_ecam, size_ecam; >> hwaddr base = base_mmio; >> - int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN; >> + int nr_pcie_buses; >> int irq = vms->irqmap[VIRT_PCIE]; >> MemoryRegion *mmio_alias; >> MemoryRegion *mmio_reg; >> @@ -1012,12 +1012,16 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic) >> MemoryRegion *ecam_reg; >> DeviceState *dev; >> char *nodename; >> - int i; >> + int i, ecam_memmap_id; >> PCIHostState *pci; >> >> dev = qdev_create(NULL, TYPE_GPEX_HOST); >> qdev_init_nofail(dev); >> >> + ecam_memmap_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM; >> + base_ecam = vms->memmap[ecam_memmap_id].base; >> + size_ecam = vms->memmap[ecam_memmap_id].size; >> + nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN; >> /* Map only the first size_ecam bytes of ECAM space */ >> ecam_alias = g_new0(MemoryRegion, 1); >> ecam_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0); >> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h >> index 4ac7ef6..e9423a7 100644 >> --- a/include/hw/arm/virt.h >> +++ b/include/hw/arm/virt.h >> @@ -69,6 +69,7 @@ enum { >> VIRT_PCIE_MMIO, >> VIRT_PCIE_PIO, >> VIRT_PCIE_ECAM, >> + VIRT_PCIE_ECAM_HIGH, >> VIRT_PLATFORM_BUS, >> VIRT_PCIE_MMIO_HIGH, >> VIRT_GPIO, >> @@ -103,6 +104,7 @@ typedef struct { >> FWCfgState *fw_cfg; >> bool secure; >> bool highmem; >> + bool highmem_ecam; >> bool its; >> bool virt; >> int32_t gic_version; >> > > At this point, the order (and values) of the VIRT_* enum constants look > mostly random :) , but I'm unaware of anything why that should be a problem. > > I compared this against the RFC version; from my side: > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > Can you please do the following additionally: > > - The aarch64 firmware packaged in Gerd's repo is built with the verbose > log enabled, as far as I can see. Can you please capture two boot logs > (from the serial console) until the guest kernel is reached, and send > them to me (off-list if you prefer)? The first log should have the new > feature disabled, the second one enabled; I'd like to compare them. > > - (I think the same test is useful with the guest kernel dmesg as well, > passing ignore_loglevel, but I don't think my assistance in reviewing > that difference is necessary or helpful :) ) OK I will prepare those logs Thanks Eric > > Thanks! > Laszlo > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/arm/virt: Add a new 256MB ECAM region 2018-05-31 6:55 ` Auger Eric @ 2018-05-31 8:41 ` Laszlo Ersek 2018-05-31 8:50 ` Auger Eric 0 siblings, 1 reply; 12+ messages in thread From: Laszlo Ersek @ 2018-05-31 8:41 UTC (permalink / raw) To: Auger Eric, eric.auger.pro, qemu-devel, qemu-arm, peter.maydell Cc: wei, drjones, ard.biesheuvel, zhaoshenglong On 05/31/18 08:55, Auger Eric wrote: > Hi Laszlo, > > On 05/30/2018 06:11 PM, Laszlo Ersek wrote: >> On 05/30/18 16:26, Eric Auger wrote: >>> This patch defines a new ECAM region located after the 256GB limit. >>> >>> The virt machine state is augmented with a new highmem_ecam field >>> which guards the usage of this new ECAM region instead of the legacy >>> 16MB one. With the highmem ECAM region, up to 256 PCIe buses can be >>> used. >>> >>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>> >>> --- >>> >>> RFC -> PATCH: >>> - remove the newline at the end of acpi_dsdt_add_pci >>> - use vms->highmem_ecam to select the memmap id >>> --- >>> hw/arm/virt-acpi-build.c | 21 +++++++++++++-------- >>> hw/arm/virt.c | 12 ++++++++---- >>> include/hw/arm/virt.h | 2 ++ >>> 3 files changed, 23 insertions(+), 12 deletions(-) >>> >>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c >>> index 92ceee9..c0370b2 100644 >>> --- a/hw/arm/virt-acpi-build.c >>> +++ b/hw/arm/virt-acpi-build.c >>> @@ -150,16 +150,17 @@ static void acpi_dsdt_add_virtio(Aml *scope, >>> } >>> >>> static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, >>> - uint32_t irq, bool use_highmem) >>> + uint32_t irq, bool use_highmem, bool highmem_ecam) >>> { >>> + int ecam_id = highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM; >>> Aml *method, *crs, *ifctx, *UUID, *ifctx1, *elsectx, *buf; >>> int i, bus_no; >>> hwaddr base_mmio = memmap[VIRT_PCIE_MMIO].base; >>> hwaddr size_mmio = memmap[VIRT_PCIE_MMIO].size; >>> hwaddr base_pio = memmap[VIRT_PCIE_PIO].base; >>> hwaddr size_pio = memmap[VIRT_PCIE_PIO].size; >>> - hwaddr base_ecam = memmap[VIRT_PCIE_ECAM].base; >>> - hwaddr size_ecam = memmap[VIRT_PCIE_ECAM].size; >>> + hwaddr base_ecam = memmap[ecam_id].base; >>> + hwaddr size_ecam = memmap[ecam_id].size; >>> int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN; >>> >>> Aml *dev = aml_device("%s", "PCI0"); >>> @@ -173,7 +174,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, >>> aml_append(dev, aml_name_decl("_CCA", aml_int(1))); >>> >>> /* Declare the PCI Routing Table. */ >>> - Aml *rt_pkg = aml_package(nr_pcie_buses * PCI_NUM_PINS); >>> + Aml *rt_pkg = aml_varpackage(nr_pcie_buses * PCI_NUM_PINS); >>> for (bus_no = 0; bus_no < nr_pcie_buses; bus_no++) { >>> for (i = 0; i < PCI_NUM_PINS; i++) { >>> int gsi = (i + bus_no) % PCI_NUM_PINS; >>> @@ -316,7 +317,10 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, >>> Aml *dev_res0 = aml_device("%s", "RES0"); >>> aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02"))); >>> crs = aml_resource_template(); >>> - aml_append(crs, aml_memory32_fixed(base_ecam, size_ecam, AML_READ_WRITE)); >>> + aml_append(crs, >>> + aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED, >>> + AML_NON_CACHEABLE, AML_READ_WRITE, 0x0000, base_ecam, >>> + base_ecam + size_ecam - 1, 0x0000, size_ecam)); >>> aml_append(dev_res0, aml_name_decl("_CRS", crs)); >>> aml_append(dev, dev_res0); >>> aml_append(scope, dev); >>> @@ -563,16 +567,17 @@ build_mcfg(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) >>> { >>> AcpiTableMcfg *mcfg; >>> const MemMapEntry *memmap = vms->memmap; >>> + int ecam_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM; >>> int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]); >>> int mcfg_start = table_data->len; >>> >>> mcfg = acpi_data_push(table_data, len); >>> - mcfg->allocation[0].address = cpu_to_le64(memmap[VIRT_PCIE_ECAM].base); >>> + mcfg->allocation[0].address = cpu_to_le64(memmap[ecam_id].base); >>> >>> /* Only a single allocation so no need to play with segments */ >>> mcfg->allocation[0].pci_segment = cpu_to_le16(0); >>> mcfg->allocation[0].start_bus_number = 0; >>> - mcfg->allocation[0].end_bus_number = (memmap[VIRT_PCIE_ECAM].size >>> + mcfg->allocation[0].end_bus_number = (memmap[ecam_id].size >>> / PCIE_MMCFG_SIZE_MIN) - 1; >>> >>> build_header(linker, table_data, (void *)(table_data->data + mcfg_start), >>> @@ -747,7 +752,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) >>> 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), >>> - vms->highmem); >>> + vms->highmem, vms->highmem_ecam); >>> acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO], >>> (irqmap[VIRT_GPIO] + ARM_SPI_BASE)); >>> acpi_dsdt_add_power_button(scope); >>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >>> index a3a28e2..d4247d0 100644 >>> --- a/hw/arm/virt.c >>> +++ b/hw/arm/virt.c >>> @@ -149,6 +149,7 @@ static const MemMapEntry a15memmap[] = { >>> [VIRT_PCIE_PIO] = { 0x3eff0000, 0x00010000 }, >>> [VIRT_PCIE_ECAM] = { 0x3f000000, 0x01000000 }, >>> [VIRT_MEM] = { 0x40000000, RAMLIMIT_BYTES }, >>> + [VIRT_PCIE_ECAM_HIGH] = { 0x4010000000ULL, 0x10000000 }, >>> /* Second PCIe window, 512GB wide at the 512GB boundary */ >>> [VIRT_PCIE_MMIO_HIGH] = { 0x8000000000ULL, 0x8000000000ULL }, >>> }; >>> @@ -1001,10 +1002,9 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic) >>> hwaddr size_mmio_high = vms->memmap[VIRT_PCIE_MMIO_HIGH].size; >>> hwaddr base_pio = vms->memmap[VIRT_PCIE_PIO].base; >>> hwaddr size_pio = vms->memmap[VIRT_PCIE_PIO].size; >>> - hwaddr base_ecam = vms->memmap[VIRT_PCIE_ECAM].base; >>> - hwaddr size_ecam = vms->memmap[VIRT_PCIE_ECAM].size; >>> + hwaddr base_ecam, size_ecam; >>> hwaddr base = base_mmio; >>> - int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN; >>> + int nr_pcie_buses; >>> int irq = vms->irqmap[VIRT_PCIE]; >>> MemoryRegion *mmio_alias; >>> MemoryRegion *mmio_reg; >>> @@ -1012,12 +1012,16 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic) >>> MemoryRegion *ecam_reg; >>> DeviceState *dev; >>> char *nodename; >>> - int i; >>> + int i, ecam_memmap_id; >>> PCIHostState *pci; >>> >>> dev = qdev_create(NULL, TYPE_GPEX_HOST); >>> qdev_init_nofail(dev); >>> >>> + ecam_memmap_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM; >>> + base_ecam = vms->memmap[ecam_memmap_id].base; >>> + size_ecam = vms->memmap[ecam_memmap_id].size; >>> + nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN; >>> /* Map only the first size_ecam bytes of ECAM space */ >>> ecam_alias = g_new0(MemoryRegion, 1); >>> ecam_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0); >>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h >>> index 4ac7ef6..e9423a7 100644 >>> --- a/include/hw/arm/virt.h >>> +++ b/include/hw/arm/virt.h >>> @@ -69,6 +69,7 @@ enum { >>> VIRT_PCIE_MMIO, >>> VIRT_PCIE_PIO, >>> VIRT_PCIE_ECAM, >>> + VIRT_PCIE_ECAM_HIGH, >>> VIRT_PLATFORM_BUS, >>> VIRT_PCIE_MMIO_HIGH, >>> VIRT_GPIO, >>> @@ -103,6 +104,7 @@ typedef struct { >>> FWCfgState *fw_cfg; >>> bool secure; >>> bool highmem; >>> + bool highmem_ecam; >>> bool its; >>> bool virt; >>> int32_t gic_version; >>> >> >> At this point, the order (and values) of the VIRT_* enum constants >> look mostly random :) , but I'm unaware of anything why that should >> be a problem. >> >> I compared this against the RFC version; from my side: >> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >> >> Can you please do the following additionally: >> >> - The aarch64 firmware packaged in Gerd's repo is built with the >> verbose log enabled, as far as I can see. Can you please capture >> two boot logs (from the serial console) until the guest kernel is >> reached, and send them to me (off-list if you prefer)? The first >> log should have the new feature disabled, the second one enabled; >> I'd like to compare them. >> >> - (I think the same test is useful with the guest kernel dmesg as >> well, passing ignore_loglevel, but I don't think my assistance in >> reviewing that difference is necessary or helpful :) ) > > OK I will prepare those logs Thanks. The important stuff is fine: > -ProcessPciHost: Config[0x3F000000+0x1000000) Bus[0x0..0xF] Io[0x0+0x10000)@0x3EFF0000 Mem32[0x10000000+0x2EFF0000)@0x0 Mem64[0x8000000000+0x8000000000)@0x0 > +ProcessPciHost: Config[0x4010000000+0x10000000) Bus[0x0..0xFF] Io[0x0+0x10000)@0x3EFF0000 Mem32[0x10000000+0x2EFF0000)@0x0 Mem64[0x8000000000+0x8000000000)@0x0 > RootBridge: PciRoot(0x0) > Support/Attr: 70001 / 70001 > DmaAbove4G: Yes > NoExtConfSpace: No > AllocAttr: 3 (CombineMemPMem Mem64Decode) > - Bus: 0 - F Translation=0 > + Bus: 0 - FF Translation=0 > Io: 0 - FFFF Translation=0 > Mem: 10000000 - 3EFEFFFF Translation=0 > MemAbove4G: 8000000000 - FFFFFFFFFF Translation=0 > PMem: FFFFFFFFFFFFFFFF - 0 Translation=0 > PMemAbove4G: FFFFFFFFFFFFFFFF - 0 Translation=0 > PciHostBridgeDxe: IntersectIoDescriptor: add [0, 10000): Success > PciHostBridgeDxe: IntersectMemoryDescriptor: add [10000000, 3EFF0000): Success > PciHostBridgeDxe: IntersectMemoryDescriptor: add [8000000000, 10000000000): Success However, another change surprises me. It might be totally independent of your patch, but because your patch also touches ACPI generation, I'd like to highlight it: the size of the DSDT grows from 0x11BD bytes to 0x4848 bytes. This is confirmed by both the firmware log and the guest dmesg; quoting the latter: > -[ 0.000000] ACPI: DSDT 0x000000013BED0000 0011BD (v02 BOCHS BXPCDSDT 00000001 BXPC 00000001) > +[ 0.000000] ACPI: DSDT 0x000000013BED0000 004848 (v02 BOCHS BXPCDSDT 00000001 BXPC 00000001) Given that your series just introduces the virt-3.0 machine type, I don't think it enables some other ACPI feature -- intentionally anyway! -- that explains this size growth. Can you dump the ACPI tables in the guest and decompile them with iasl? ... Ah wait, I'm being silly. "nr_pcie_buses" in acpi_dsdt_add_pci() affects the size of the PRT; you even changed the PRT from an AML "package" to an AML "varpackage", because the loop that adds the interrupt mapping packages to the PRT runs much higher now. I vaguely recall that Linux used to dump the PRT at boot; from those messages I would have realized earlier. Anyway, everything looks fine to me. Thanks! Laszlo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hw/arm/virt: Add a new 256MB ECAM region 2018-05-31 8:41 ` Laszlo Ersek @ 2018-05-31 8:50 ` Auger Eric 0 siblings, 0 replies; 12+ messages in thread From: Auger Eric @ 2018-05-31 8:50 UTC (permalink / raw) To: Laszlo Ersek, eric.auger.pro, qemu-devel, qemu-arm, peter.maydell Cc: wei, drjones, zhaoshenglong, ard.biesheuvel Hi Laszlo, On 05/31/2018 10:41 AM, Laszlo Ersek wrote: > On 05/31/18 08:55, Auger Eric wrote: >> Hi Laszlo, >> >> On 05/30/2018 06:11 PM, Laszlo Ersek wrote: >>> On 05/30/18 16:26, Eric Auger wrote: >>>> This patch defines a new ECAM region located after the 256GB limit. >>>> >>>> The virt machine state is augmented with a new highmem_ecam field >>>> which guards the usage of this new ECAM region instead of the legacy >>>> 16MB one. With the highmem ECAM region, up to 256 PCIe buses can be >>>> used. >>>> >>>> Signed-off-by: Eric Auger <eric.auger@redhat.com> >>>> >>>> --- >>>> >>>> RFC -> PATCH: >>>> - remove the newline at the end of acpi_dsdt_add_pci >>>> - use vms->highmem_ecam to select the memmap id >>>> --- >>>> hw/arm/virt-acpi-build.c | 21 +++++++++++++-------- >>>> hw/arm/virt.c | 12 ++++++++---- >>>> include/hw/arm/virt.h | 2 ++ >>>> 3 files changed, 23 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c >>>> index 92ceee9..c0370b2 100644 >>>> --- a/hw/arm/virt-acpi-build.c >>>> +++ b/hw/arm/virt-acpi-build.c >>>> @@ -150,16 +150,17 @@ static void acpi_dsdt_add_virtio(Aml *scope, >>>> } >>>> >>>> static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, >>>> - uint32_t irq, bool use_highmem) >>>> + uint32_t irq, bool use_highmem, bool highmem_ecam) >>>> { >>>> + int ecam_id = highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM; >>>> Aml *method, *crs, *ifctx, *UUID, *ifctx1, *elsectx, *buf; >>>> int i, bus_no; >>>> hwaddr base_mmio = memmap[VIRT_PCIE_MMIO].base; >>>> hwaddr size_mmio = memmap[VIRT_PCIE_MMIO].size; >>>> hwaddr base_pio = memmap[VIRT_PCIE_PIO].base; >>>> hwaddr size_pio = memmap[VIRT_PCIE_PIO].size; >>>> - hwaddr base_ecam = memmap[VIRT_PCIE_ECAM].base; >>>> - hwaddr size_ecam = memmap[VIRT_PCIE_ECAM].size; >>>> + hwaddr base_ecam = memmap[ecam_id].base; >>>> + hwaddr size_ecam = memmap[ecam_id].size; >>>> int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN; >>>> >>>> Aml *dev = aml_device("%s", "PCI0"); >>>> @@ -173,7 +174,7 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, >>>> aml_append(dev, aml_name_decl("_CCA", aml_int(1))); >>>> >>>> /* Declare the PCI Routing Table. */ >>>> - Aml *rt_pkg = aml_package(nr_pcie_buses * PCI_NUM_PINS); >>>> + Aml *rt_pkg = aml_varpackage(nr_pcie_buses * PCI_NUM_PINS); >>>> for (bus_no = 0; bus_no < nr_pcie_buses; bus_no++) { >>>> for (i = 0; i < PCI_NUM_PINS; i++) { >>>> int gsi = (i + bus_no) % PCI_NUM_PINS; >>>> @@ -316,7 +317,10 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, >>>> Aml *dev_res0 = aml_device("%s", "RES0"); >>>> aml_append(dev_res0, aml_name_decl("_HID", aml_string("PNP0C02"))); >>>> crs = aml_resource_template(); >>>> - aml_append(crs, aml_memory32_fixed(base_ecam, size_ecam, AML_READ_WRITE)); >>>> + aml_append(crs, >>>> + aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED, >>>> + AML_NON_CACHEABLE, AML_READ_WRITE, 0x0000, base_ecam, >>>> + base_ecam + size_ecam - 1, 0x0000, size_ecam)); >>>> aml_append(dev_res0, aml_name_decl("_CRS", crs)); >>>> aml_append(dev, dev_res0); >>>> aml_append(scope, dev); >>>> @@ -563,16 +567,17 @@ build_mcfg(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) >>>> { >>>> AcpiTableMcfg *mcfg; >>>> const MemMapEntry *memmap = vms->memmap; >>>> + int ecam_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM; >>>> int len = sizeof(*mcfg) + sizeof(mcfg->allocation[0]); >>>> int mcfg_start = table_data->len; >>>> >>>> mcfg = acpi_data_push(table_data, len); >>>> - mcfg->allocation[0].address = cpu_to_le64(memmap[VIRT_PCIE_ECAM].base); >>>> + mcfg->allocation[0].address = cpu_to_le64(memmap[ecam_id].base); >>>> >>>> /* Only a single allocation so no need to play with segments */ >>>> mcfg->allocation[0].pci_segment = cpu_to_le16(0); >>>> mcfg->allocation[0].start_bus_number = 0; >>>> - mcfg->allocation[0].end_bus_number = (memmap[VIRT_PCIE_ECAM].size >>>> + mcfg->allocation[0].end_bus_number = (memmap[ecam_id].size >>>> / PCIE_MMCFG_SIZE_MIN) - 1; >>>> >>>> build_header(linker, table_data, (void *)(table_data->data + mcfg_start), >>>> @@ -747,7 +752,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) >>>> 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), >>>> - vms->highmem); >>>> + vms->highmem, vms->highmem_ecam); >>>> acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO], >>>> (irqmap[VIRT_GPIO] + ARM_SPI_BASE)); >>>> acpi_dsdt_add_power_button(scope); >>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >>>> index a3a28e2..d4247d0 100644 >>>> --- a/hw/arm/virt.c >>>> +++ b/hw/arm/virt.c >>>> @@ -149,6 +149,7 @@ static const MemMapEntry a15memmap[] = { >>>> [VIRT_PCIE_PIO] = { 0x3eff0000, 0x00010000 }, >>>> [VIRT_PCIE_ECAM] = { 0x3f000000, 0x01000000 }, >>>> [VIRT_MEM] = { 0x40000000, RAMLIMIT_BYTES }, >>>> + [VIRT_PCIE_ECAM_HIGH] = { 0x4010000000ULL, 0x10000000 }, >>>> /* Second PCIe window, 512GB wide at the 512GB boundary */ >>>> [VIRT_PCIE_MMIO_HIGH] = { 0x8000000000ULL, 0x8000000000ULL }, >>>> }; >>>> @@ -1001,10 +1002,9 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic) >>>> hwaddr size_mmio_high = vms->memmap[VIRT_PCIE_MMIO_HIGH].size; >>>> hwaddr base_pio = vms->memmap[VIRT_PCIE_PIO].base; >>>> hwaddr size_pio = vms->memmap[VIRT_PCIE_PIO].size; >>>> - hwaddr base_ecam = vms->memmap[VIRT_PCIE_ECAM].base; >>>> - hwaddr size_ecam = vms->memmap[VIRT_PCIE_ECAM].size; >>>> + hwaddr base_ecam, size_ecam; >>>> hwaddr base = base_mmio; >>>> - int nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN; >>>> + int nr_pcie_buses; >>>> int irq = vms->irqmap[VIRT_PCIE]; >>>> MemoryRegion *mmio_alias; >>>> MemoryRegion *mmio_reg; >>>> @@ -1012,12 +1012,16 @@ static void create_pcie(VirtMachineState *vms, qemu_irq *pic) >>>> MemoryRegion *ecam_reg; >>>> DeviceState *dev; >>>> char *nodename; >>>> - int i; >>>> + int i, ecam_memmap_id; >>>> PCIHostState *pci; >>>> >>>> dev = qdev_create(NULL, TYPE_GPEX_HOST); >>>> qdev_init_nofail(dev); >>>> >>>> + ecam_memmap_id = vms->highmem_ecam ? VIRT_PCIE_ECAM_HIGH : VIRT_PCIE_ECAM; >>>> + base_ecam = vms->memmap[ecam_memmap_id].base; >>>> + size_ecam = vms->memmap[ecam_memmap_id].size; >>>> + nr_pcie_buses = size_ecam / PCIE_MMCFG_SIZE_MIN; >>>> /* Map only the first size_ecam bytes of ECAM space */ >>>> ecam_alias = g_new0(MemoryRegion, 1); >>>> ecam_reg = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0); >>>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h >>>> index 4ac7ef6..e9423a7 100644 >>>> --- a/include/hw/arm/virt.h >>>> +++ b/include/hw/arm/virt.h >>>> @@ -69,6 +69,7 @@ enum { >>>> VIRT_PCIE_MMIO, >>>> VIRT_PCIE_PIO, >>>> VIRT_PCIE_ECAM, >>>> + VIRT_PCIE_ECAM_HIGH, >>>> VIRT_PLATFORM_BUS, >>>> VIRT_PCIE_MMIO_HIGH, >>>> VIRT_GPIO, >>>> @@ -103,6 +104,7 @@ typedef struct { >>>> FWCfgState *fw_cfg; >>>> bool secure; >>>> bool highmem; >>>> + bool highmem_ecam; >>>> bool its; >>>> bool virt; >>>> int32_t gic_version; >>>> >>> >>> At this point, the order (and values) of the VIRT_* enum constants >>> look mostly random :) , but I'm unaware of anything why that should >>> be a problem. >>> >>> I compared this against the RFC version; from my side: >>> >>> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >>> >>> Can you please do the following additionally: >>> >>> - The aarch64 firmware packaged in Gerd's repo is built with the >>> verbose log enabled, as far as I can see. Can you please capture >>> two boot logs (from the serial console) until the guest kernel is >>> reached, and send them to me (off-list if you prefer)? The first >>> log should have the new feature disabled, the second one enabled; >>> I'd like to compare them. >>> >>> - (I think the same test is useful with the guest kernel dmesg as >>> well, passing ignore_loglevel, but I don't think my assistance in >>> reviewing that difference is necessary or helpful :) ) >> >> OK I will prepare those logs > > Thanks. The important stuff is fine: > >> -ProcessPciHost: Config[0x3F000000+0x1000000) Bus[0x0..0xF] Io[0x0+0x10000)@0x3EFF0000 Mem32[0x10000000+0x2EFF0000)@0x0 Mem64[0x8000000000+0x8000000000)@0x0 >> +ProcessPciHost: Config[0x4010000000+0x10000000) Bus[0x0..0xFF] Io[0x0+0x10000)@0x3EFF0000 Mem32[0x10000000+0x2EFF0000)@0x0 Mem64[0x8000000000+0x8000000000)@0x0 >> RootBridge: PciRoot(0x0) >> Support/Attr: 70001 / 70001 >> DmaAbove4G: Yes >> NoExtConfSpace: No >> AllocAttr: 3 (CombineMemPMem Mem64Decode) >> - Bus: 0 - F Translation=0 >> + Bus: 0 - FF Translation=0 >> Io: 0 - FFFF Translation=0 >> Mem: 10000000 - 3EFEFFFF Translation=0 >> MemAbove4G: 8000000000 - FFFFFFFFFF Translation=0 >> PMem: FFFFFFFFFFFFFFFF - 0 Translation=0 >> PMemAbove4G: FFFFFFFFFFFFFFFF - 0 Translation=0 >> PciHostBridgeDxe: IntersectIoDescriptor: add [0, 10000): Success >> PciHostBridgeDxe: IntersectMemoryDescriptor: add [10000000, 3EFF0000): Success >> PciHostBridgeDxe: IntersectMemoryDescriptor: add [8000000000, 10000000000): Success > > However, another change surprises me. It might be totally independent of > your patch, but because your patch also touches ACPI generation, I'd > like to highlight it: the size of the DSDT grows from 0x11BD bytes to > 0x4848 bytes. This is confirmed by both the firmware log and the guest > dmesg; quoting the latter: > >> -[ 0.000000] ACPI: DSDT 0x000000013BED0000 0011BD (v02 BOCHS BXPCDSDT 00000001 BXPC 00000001) >> +[ 0.000000] ACPI: DSDT 0x000000013BED0000 004848 (v02 BOCHS BXPCDSDT 00000001 BXPC 00000001) > > Given that your series just introduces the virt-3.0 machine type, I > don't think it enables some other ACPI feature -- intentionally anyway! > -- that explains this size growth. Can you dump the ACPI tables in the > guest and decompile them with iasl? > > ... Ah wait, I'm being silly. "nr_pcie_buses" in acpi_dsdt_add_pci() > affects the size of the PRT; you even changed the PRT from an AML > "package" to an AML "varpackage", because the loop that adds the > interrupt mapping packages to the PRT runs much higher now. > > I vaguely recall that Linux used to dump the PRT at boot; from those > messages I would have realized earlier. Anyway, everything looks fine to > me. Cool! Thanks Eric > > Thanks! > Laszlo > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH 2/2] hw/arm/virt: Add virt-3.0 machine type 2018-05-30 14:26 [Qemu-devel] [PATCH 0/2] ARM virt: Support up to 256 PCIe buses Eric Auger 2018-05-30 14:26 ` [Qemu-devel] [PATCH 1/2] hw/arm/virt: Add a new 256MB ECAM region Eric Auger @ 2018-05-30 14:26 ` Eric Auger 2018-05-30 16:18 ` Laszlo Ersek 2018-06-15 12:37 ` Peter Maydell 1 sibling, 2 replies; 12+ messages in thread From: Eric Auger @ 2018-05-30 14:26 UTC (permalink / raw) To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell Cc: drjones, wei, zhaoshenglong, lersek, ard.biesheuvel Add virt-3.0 machine type. This machine type supports highmem 256MB ECAM by default. This feature is disabled for earlier machine types and if highmem is off. The high 256MB ECAM region is chosen instead of the legacy 16MB one if the machine type allows it, if highmem is set (LPAE supported by the guest) and (!firmware_loaded || aarch64). Indeed aarch32 mode FW may not support this high ECAM region. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- RFC -> v1 - check firmware_loaded and aarch64 value - do all the computation in machvirt_init --- hw/arm/virt.c | 31 ++++++++++++++++++++++++++++--- include/hw/arm/virt.h | 1 + 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/hw/arm/virt.c b/hw/arm/virt.c index d4247d0..83c2b5a 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1275,6 +1275,7 @@ static void machvirt_init(MachineState *machine) int n, virt_max_cpus; MemoryRegion *ram = g_new(MemoryRegion, 1); bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0); + bool aarch64 = true; /* We can probe only here because during property set * KVM is not available yet @@ -1389,6 +1390,8 @@ static void machvirt_init(MachineState *machine) numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj), &error_fatal); + aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL); + if (!vms->secure) { object_property_set_bool(cpuobj, false, "has_el3", NULL); } @@ -1447,6 +1450,8 @@ static void machvirt_init(MachineState *machine) create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem, serial_hd(1)); } + vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); + create_rtc(vms, pic); create_pcie(vms, pic); @@ -1697,7 +1702,7 @@ static void machvirt_machine_init(void) } type_init(machvirt_machine_init); -static void virt_2_12_instance_init(Object *obj) +static void virt_3_0_instance_init(Object *obj) { VirtMachineState *vms = VIRT_MACHINE(obj); VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms); @@ -1740,6 +1745,8 @@ static void virt_2_12_instance_init(Object *obj) "Set GIC version. " "Valid values are 2, 3 and host", NULL); + vms->highmem_ecam = vmc->no_highmem_ecam ? false : true; + if (vmc->no_its) { vms->its = false; } else { @@ -1765,10 +1772,28 @@ static void virt_2_12_instance_init(Object *obj) vms->irqmap = a15irqmap; } -static void virt_machine_2_12_options(MachineClass *mc) +static void virt_machine_3_0_options(MachineClass *mc) { } -DEFINE_VIRT_MACHINE_AS_LATEST(2, 12) +DEFINE_VIRT_MACHINE_AS_LATEST(3, 0) + +#define VIRT_COMPAT_2_12 \ + HW_COMPAT_2_12 + +static void virt_2_12_instance_init(Object *obj) +{ + virt_3_0_instance_init(obj); +} + +static void virt_machine_2_12_options(MachineClass *mc) +{ + VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc)); + + virt_machine_3_0_options(mc); + SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_12); + vmc->no_highmem_ecam = true; +} +DEFINE_VIRT_MACHINE(2, 12) #define VIRT_COMPAT_2_11 \ HW_COMPAT_2_11 diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index e9423a7..10a5c71 100644 --- a/include/hw/arm/virt.h +++ b/include/hw/arm/virt.h @@ -95,6 +95,7 @@ typedef struct { bool no_pmu; bool claim_edge_triggered_timers; bool smbios_old_sys_ver; + bool no_highmem_ecam; } VirtMachineClass; typedef struct { -- 2.5.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Add virt-3.0 machine type 2018-05-30 14:26 ` [Qemu-devel] [PATCH 2/2] hw/arm/virt: Add virt-3.0 machine type Eric Auger @ 2018-05-30 16:18 ` Laszlo Ersek 2018-05-31 1:42 ` Shannon Zhao 2018-05-31 6:52 ` Auger Eric 2018-06-15 12:37 ` Peter Maydell 1 sibling, 2 replies; 12+ messages in thread From: Laszlo Ersek @ 2018-05-30 16:18 UTC (permalink / raw) To: Eric Auger, eric.auger.pro, qemu-devel, qemu-arm, peter.maydell Cc: drjones, wei, zhaoshenglong, ard.biesheuvel On 05/30/18 16:26, Eric Auger wrote: > Add virt-3.0 machine type. > > This machine type supports highmem 256MB ECAM by default. > This feature is disabled for earlier machine types and > if highmem is off. > > The high 256MB ECAM region is chosen instead of the legacy > 16MB one if the machine type allows it, if highmem is set > (LPAE supported by the guest) and (!firmware_loaded || aarch64). > Indeed aarch32 mode FW may not support this high ECAM region. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> > > --- > > RFC -> v1 > - check firmware_loaded and aarch64 value > - do all the computation in machvirt_init > --- > hw/arm/virt.c | 31 ++++++++++++++++++++++++++++--- > include/hw/arm/virt.h | 1 + > 2 files changed, 29 insertions(+), 3 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index d4247d0..83c2b5a 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1275,6 +1275,7 @@ static void machvirt_init(MachineState *machine) > int n, virt_max_cpus; > MemoryRegion *ram = g_new(MemoryRegion, 1); > bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0); > + bool aarch64 = true; > > /* We can probe only here because during property set > * KVM is not available yet > @@ -1389,6 +1390,8 @@ static void machvirt_init(MachineState *machine) > numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj), > &error_fatal); > > + aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL); > + > if (!vms->secure) { > object_property_set_bool(cpuobj, false, "has_el3", NULL); > } > @@ -1447,6 +1450,8 @@ static void machvirt_init(MachineState *machine) > create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem, serial_hd(1)); > } > > + vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); > + > create_rtc(vms, pic); > > create_pcie(vms, pic); > @@ -1697,7 +1702,7 @@ static void machvirt_machine_init(void) > } > type_init(machvirt_machine_init); > > -static void virt_2_12_instance_init(Object *obj) > +static void virt_3_0_instance_init(Object *obj) > { > VirtMachineState *vms = VIRT_MACHINE(obj); > VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms); > @@ -1740,6 +1745,8 @@ static void virt_2_12_instance_init(Object *obj) > "Set GIC version. " > "Valid values are 2, 3 and host", NULL); > > + vms->highmem_ecam = vmc->no_highmem_ecam ? false : true; > + I think this should be written as: vms->highmem_ecam = !vmc->no_highmem_ecam; With that change: Reviewed-by: Laszlo Ersek <lersek@redhat.com> Obviously the series has to be reviewed by others as well. Thanks! Laszlo > if (vmc->no_its) { > vms->its = false; > } else { > @@ -1765,10 +1772,28 @@ static void virt_2_12_instance_init(Object *obj) > vms->irqmap = a15irqmap; > } > > -static void virt_machine_2_12_options(MachineClass *mc) > +static void virt_machine_3_0_options(MachineClass *mc) > { > } > -DEFINE_VIRT_MACHINE_AS_LATEST(2, 12) > +DEFINE_VIRT_MACHINE_AS_LATEST(3, 0) > + > +#define VIRT_COMPAT_2_12 \ > + HW_COMPAT_2_12 > + > +static void virt_2_12_instance_init(Object *obj) > +{ > + virt_3_0_instance_init(obj); > +} > + > +static void virt_machine_2_12_options(MachineClass *mc) > +{ > + VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc)); > + > + virt_machine_3_0_options(mc); > + SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_12); > + vmc->no_highmem_ecam = true; > +} > +DEFINE_VIRT_MACHINE(2, 12) > > #define VIRT_COMPAT_2_11 \ > HW_COMPAT_2_11 > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h > index e9423a7..10a5c71 100644 > --- a/include/hw/arm/virt.h > +++ b/include/hw/arm/virt.h > @@ -95,6 +95,7 @@ typedef struct { > bool no_pmu; > bool claim_edge_triggered_timers; > bool smbios_old_sys_ver; > + bool no_highmem_ecam; > } VirtMachineClass; > > typedef struct { > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Add virt-3.0 machine type 2018-05-30 16:18 ` Laszlo Ersek @ 2018-05-31 1:42 ` Shannon Zhao 2018-05-31 6:23 ` Auger Eric 2018-05-31 6:52 ` Auger Eric 1 sibling, 1 reply; 12+ messages in thread From: Shannon Zhao @ 2018-05-31 1:42 UTC (permalink / raw) To: Laszlo Ersek, Eric Auger, eric.auger.pro, qemu-devel, qemu-arm, peter.maydell Cc: drjones, wei, ard.biesheuvel, liujinsong On 2018/5/31 0:18, Laszlo Ersek wrote: > + vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); >> + Does it need a info log here to tell user that even though you enable the highmem_ecam but due to some other reasons it's disabled. Also, if user enables highmem_ecam but finally it can't enable, does it need to error out? Thanks, -- Shannon ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Add virt-3.0 machine type 2018-05-31 1:42 ` Shannon Zhao @ 2018-05-31 6:23 ` Auger Eric 0 siblings, 0 replies; 12+ messages in thread From: Auger Eric @ 2018-05-31 6:23 UTC (permalink / raw) To: Shannon Zhao, Laszlo Ersek, eric.auger.pro, qemu-devel, qemu-arm, peter.maydell Cc: wei, liujinsong, drjones, ard.biesheuvel Hi Shannon, On 05/31/2018 03:42 AM, Shannon Zhao wrote: > > > On 2018/5/31 0:18, Laszlo Ersek wrote: >> + vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); >>> + > Does it need a info log here to tell user that even though you enable > the highmem_ecam but due to some other reasons it's disabled. > > Also, if user enables highmem_ecam but finally it can't enable, does it > need to error out? highmem_ecam is not a user settable option. highmem is. highmem_ecam is computed based on the machine type capability to support this feature, the fact highmem is set and we don't have an aarch32 FW being used. Thanks Eric > > Thanks, > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Add virt-3.0 machine type 2018-05-30 16:18 ` Laszlo Ersek 2018-05-31 1:42 ` Shannon Zhao @ 2018-05-31 6:52 ` Auger Eric 1 sibling, 0 replies; 12+ messages in thread From: Auger Eric @ 2018-05-31 6:52 UTC (permalink / raw) To: Laszlo Ersek, eric.auger.pro, qemu-devel, qemu-arm, peter.maydell Cc: wei, drjones, ard.biesheuvel, zhaoshenglong On 05/30/2018 06:18 PM, Laszlo Ersek wrote: > On 05/30/18 16:26, Eric Auger wrote: >> Add virt-3.0 machine type. >> >> This machine type supports highmem 256MB ECAM by default. >> This feature is disabled for earlier machine types and >> if highmem is off. >> >> The high 256MB ECAM region is chosen instead of the legacy >> 16MB one if the machine type allows it, if highmem is set >> (LPAE supported by the guest) and (!firmware_loaded || aarch64). >> Indeed aarch32 mode FW may not support this high ECAM region. >> >> Signed-off-by: Eric Auger <eric.auger@redhat.com> >> >> --- >> >> RFC -> v1 >> - check firmware_loaded and aarch64 value >> - do all the computation in machvirt_init >> --- >> hw/arm/virt.c | 31 ++++++++++++++++++++++++++++--- >> include/hw/arm/virt.h | 1 + >> 2 files changed, 29 insertions(+), 3 deletions(-) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index d4247d0..83c2b5a 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -1275,6 +1275,7 @@ static void machvirt_init(MachineState *machine) >> int n, virt_max_cpus; >> MemoryRegion *ram = g_new(MemoryRegion, 1); >> bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0); >> + bool aarch64 = true; >> >> /* We can probe only here because during property set >> * KVM is not available yet >> @@ -1389,6 +1390,8 @@ static void machvirt_init(MachineState *machine) >> numa_cpu_pre_plug(&possible_cpus->cpus[cs->cpu_index], DEVICE(cpuobj), >> &error_fatal); >> >> + aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL); >> + >> if (!vms->secure) { >> object_property_set_bool(cpuobj, false, "has_el3", NULL); >> } >> @@ -1447,6 +1450,8 @@ static void machvirt_init(MachineState *machine) >> create_uart(vms, pic, VIRT_SECURE_UART, secure_sysmem, serial_hd(1)); >> } >> >> + vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); >> + >> create_rtc(vms, pic); >> >> create_pcie(vms, pic); >> @@ -1697,7 +1702,7 @@ static void machvirt_machine_init(void) >> } >> type_init(machvirt_machine_init); >> >> -static void virt_2_12_instance_init(Object *obj) >> +static void virt_3_0_instance_init(Object *obj) >> { >> VirtMachineState *vms = VIRT_MACHINE(obj); >> VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(vms); >> @@ -1740,6 +1745,8 @@ static void virt_2_12_instance_init(Object *obj) >> "Set GIC version. " >> "Valid values are 2, 3 and host", NULL); >> >> + vms->highmem_ecam = vmc->no_highmem_ecam ? false : true; >> + > > I think this should be written as: > > vms->highmem_ecam = !vmc->no_highmem_ecam; sure ;-) > > With that change: > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> Thank you for the early feedbacks. Eric > > Obviously the series has to be reviewed by others as well. > > Thanks! > Laszlo > > >> if (vmc->no_its) { >> vms->its = false; >> } else { >> @@ -1765,10 +1772,28 @@ static void virt_2_12_instance_init(Object *obj) >> vms->irqmap = a15irqmap; >> } >> >> -static void virt_machine_2_12_options(MachineClass *mc) >> +static void virt_machine_3_0_options(MachineClass *mc) >> { >> } >> -DEFINE_VIRT_MACHINE_AS_LATEST(2, 12) >> +DEFINE_VIRT_MACHINE_AS_LATEST(3, 0) >> + >> +#define VIRT_COMPAT_2_12 \ >> + HW_COMPAT_2_12 >> + >> +static void virt_2_12_instance_init(Object *obj) >> +{ >> + virt_3_0_instance_init(obj); >> +} >> + >> +static void virt_machine_2_12_options(MachineClass *mc) >> +{ >> + VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc)); >> + >> + virt_machine_3_0_options(mc); >> + SET_MACHINE_COMPAT(mc, VIRT_COMPAT_2_12); >> + vmc->no_highmem_ecam = true; >> +} >> +DEFINE_VIRT_MACHINE(2, 12) >> >> #define VIRT_COMPAT_2_11 \ >> HW_COMPAT_2_11 >> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h >> index e9423a7..10a5c71 100644 >> --- a/include/hw/arm/virt.h >> +++ b/include/hw/arm/virt.h >> @@ -95,6 +95,7 @@ typedef struct { >> bool no_pmu; >> bool claim_edge_triggered_timers; >> bool smbios_old_sys_ver; >> + bool no_highmem_ecam; >> } VirtMachineClass; >> >> typedef struct { >> > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hw/arm/virt: Add virt-3.0 machine type 2018-05-30 14:26 ` [Qemu-devel] [PATCH 2/2] hw/arm/virt: Add virt-3.0 machine type Eric Auger 2018-05-30 16:18 ` Laszlo Ersek @ 2018-06-15 12:37 ` Peter Maydell 1 sibling, 0 replies; 12+ messages in thread From: Peter Maydell @ 2018-06-15 12:37 UTC (permalink / raw) To: Eric Auger Cc: Eric Auger, QEMU Developers, qemu-arm, Andrew Jones, Wei Huang, Shannon Zhao, Laszlo Ersek, Ard Biesheuvel On 30 May 2018 at 15:26, Eric Auger <eric.auger@redhat.com> wrote: > Add virt-3.0 machine type. > > This machine type supports highmem 256MB ECAM by default. > This feature is disabled for earlier machine types and > if highmem is off. > > The high 256MB ECAM region is chosen instead of the legacy > 16MB one if the machine type allows it, if highmem is set > (LPAE supported by the guest) and (!firmware_loaded || aarch64). > Indeed aarch32 mode FW may not support this high ECAM region. > > Signed-off-by: Eric Auger <eric.auger@redhat.com> I think these patches should be the other way round -- create the new 3.0 machine type first, and then in the patch adding the high ECAM region you can make it be only if newer machine type. thanks -- PMM ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-06-15 12:37 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-30 14:26 [Qemu-devel] [PATCH 0/2] ARM virt: Support up to 256 PCIe buses Eric Auger 2018-05-30 14:26 ` [Qemu-devel] [PATCH 1/2] hw/arm/virt: Add a new 256MB ECAM region Eric Auger 2018-05-30 16:11 ` Laszlo Ersek 2018-05-31 6:55 ` Auger Eric 2018-05-31 8:41 ` Laszlo Ersek 2018-05-31 8:50 ` Auger Eric 2018-05-30 14:26 ` [Qemu-devel] [PATCH 2/2] hw/arm/virt: Add virt-3.0 machine type Eric Auger 2018-05-30 16:18 ` Laszlo Ersek 2018-05-31 1:42 ` Shannon Zhao 2018-05-31 6:23 ` Auger Eric 2018-05-31 6:52 ` Auger Eric 2018-06-15 12:37 ` Peter Maydell
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.