From: Eric Auger <eric.auger@redhat.com>

With this patch, virt-3.0 machine uses a new 256MB ECAM region
by default instead of the legacy 16MB one, 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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
Message-id: 1529072910-16156-11-git-send-email-eric.auger@redhat.com
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/virt.h |  1 +
 hw/arm/virt.c         | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 085fdcc2879..9a870ccb6a5 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -98,6 +98,7 @@ typedef struct {
     bool no_pmu;
     bool claim_edge_triggered_timers;
     bool smbios_old_sys_ver;
+    bool no_highmem_ecam;
 } VirtMachineClass;
 
 typedef struct {
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d8abf89e8c8..0f8bfa57d7e 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1318,6 +1318,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
@@ -1433,6 +1434,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);
         }
@@ -1491,6 +1494,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);
@@ -1788,6 +1793,8 @@ static void virt_3_0_instance_init(Object *obj)
                                     "Set GIC version. "
                                     "Valid values are 2, 3 and host", NULL);
 
+    vms->highmem_ecam = !vmc->no_highmem_ecam;
+
     if (vmc->no_its) {
         vms->its = false;
     } else {
@@ -1825,8 +1832,11 @@ static void virt_2_12_instance_init(Object *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)
 
-- 
2.17.1

On Fri, 22 Jun 2018 at 14:09, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> From: Eric Auger <eric.auger@redhat.com>
>
> With this patch, virt-3.0 machine uses a new 256MB ECAM region
> by default instead of the legacy 16MB one, if highmem is set
> (LPAE supported by the guest) and (!firmware_loaded || aarch64).
>
> Indeed aarch32 mode FW may not support this high ECAM region.

This is a rather old change by now, but I've been looking
at it because it exposes an issue which was previously
masked by a different bug...

> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d8abf89e8c8..0f8bfa57d7e 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1318,6 +1318,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
> @@ -1433,6 +1434,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);
>          }
> @@ -1491,6 +1494,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);

Do you remember why this conditional is the way it is?
As it stands, it will disable the high-memory ECAM for
an AArch32 VM that's loaded firmware, but leaves it enabled
if we're direct booting Linux. That's a problem because 32-bit
Linux falls over if you pass it a highmem-ECAM, even if LPAE
is enabled (somewhere along the line it discards the high 32
bits of the address of the ECAM in the dtb, so it thinks the
ECAM overlaps with another memory region, and won't recognize
the pci controller; I have a feeling this is a regression in
the kernel). Plus, we have no way to tell if the guest
kernel has LPAE enabled at all.

Maybe it would be safer to insist that the guest is aarch64
before we enable highmem ECAM?

thanks
-- PMM

Hi Peter,

On 1/8/24 16:52, Peter Maydell wrote:
> On Fri, 22 Jun 2018 at 14:09, Peter Maydell <peter.maydell@linaro.org> wrote:
>> From: Eric Auger <eric.auger@redhat.com>
>>
>> With this patch, virt-3.0 machine uses a new 256MB ECAM region
>> by default instead of the legacy 16MB one, if highmem is set
>> (LPAE supported by the guest) and (!firmware_loaded || aarch64).
>>
>> Indeed aarch32 mode FW may not support this high ECAM region.
> This is a rather old change by now, but I've been looking
> at it because it exposes an issue which was previously
> masked by a different bug...
>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index d8abf89e8c8..0f8bfa57d7e 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1318,6 +1318,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
>> @@ -1433,6 +1434,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);
>>          }
>> @@ -1491,6 +1494,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);
> Do you remember why this conditional is the way it is?
It comes from Laszlo's suggestion at
http://patchwork.ozlabs.org/project/qemu-devel/cover/1527091418-11874-1-git-send-email-eric.auger@redhat.com/#1920422

"I'd rather restrict the large/high ECAM feature to 64-bit guests (with or without
firmware), and to 32-bit LPAE kernels that are launched without firmware
(which, I think, has been the case for most of their history)."


See the associated thread too.

Hope this helps

Eric
> As it stands, it will disable the high-memory ECAM for
> an AArch32 VM that's loaded firmware, but leaves it enabled
> if we're direct booting Linux. That's a problem because 32-bit
> Linux falls over if you pass it a highmem-ECAM, even if LPAE
> is enabled (somewhere along the line it discards the high 32
> bits of the address of the ECAM in the dtb, so it thinks the
> ECAM overlaps with another memory region, and won't recognize
> the pci controller; I have a feeling this is a regression in
> the kernel). Plus, we have no way to tell if the guest
> kernel has LPAE enabled at all.
>
> Maybe it would be safer to insist that the guest is aarch64
> before we enable highmem ECAM?
>
> thanks
> -- PMM
>