| Message ID | 20180622125713.15303-14-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/28] hw/intc/arm_gicv3: fix an extra left-shift when reading IPRIORITYR | expand |
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 >
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)