diff mbox series

[1/2] hw/arm/virt: Add a new 256MB ECAM region

Message ID 1527690380-9782-2-git-send-email-eric.auger@redhat.com
State New
Headers show
Series ARM virt: Support up to 256 PCIe buses | expand

Commit Message

Eric Auger May 30, 2018, 2:26 p.m. UTC
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(-)

Comments

Laszlo Ersek May 30, 2018, 4:11 p.m. UTC | #1
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
Eric Auger May 31, 2018, 6:55 a.m. UTC | #2
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
>
Laszlo Ersek May 31, 2018, 8:41 a.m. UTC | #3
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
Eric Auger May 31, 2018, 8:50 a.m. UTC | #4
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
>
diff mbox series

Patch

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;