| Message ID | 20210822144441.1290891-3-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Reduced-IPA space and highmem=off fixes | expand |
On Sun, 22 Aug 2021 at 15:45, Marc Zyngier <maz@kernel.org> wrote: > > Even when the VM is configured with highmem=off, the highest_gpa > field includes devices that are above the 4GiB limit, which is > what highmem=off is supposed to enforce. This leads to failures > in virt_kvm_type() on systems that have a crippled IPA range, > as the reported IPA space is larger than what it should be. > > Instead, honor the user-specified limit to only use the devices > at the lowest end of the spectrum. > > Note that this doesn't affect memory, which is still allowed to > go beyond 4GiB with highmem=on configurations. > > Cc: Andrew Jones <drjones@redhat.com> > Cc: Eric Auger <eric.auger@redhat.com> > Cc: Peter Maydell <peter.maydell@linaro.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > hw/arm/virt.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 81eda46b0b..bc189e30b8 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1598,7 +1598,7 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx) > static void virt_set_memmap(VirtMachineState *vms) > { > MachineState *ms = MACHINE(vms); > - hwaddr base, device_memory_base, device_memory_size; > + hwaddr base, device_memory_base, device_memory_size, ceiling; > int i; > > vms->memmap = extended_memmap; > @@ -1625,7 +1625,7 @@ static void virt_set_memmap(VirtMachineState *vms) > device_memory_size = ms->maxram_size - ms->ram_size + ms->ram_slots * GiB; > > /* Base address of the high IO region */ > - base = device_memory_base + ROUND_UP(device_memory_size, GiB); > + ceiling = base = device_memory_base + ROUND_UP(device_memory_size, GiB); > if (base < device_memory_base) { > error_report("maxmem/slots too huge"); > exit(EXIT_FAILURE); > @@ -1642,7 +1642,11 @@ static void virt_set_memmap(VirtMachineState *vms) > vms->memmap[i].size = size; > base += size; > } > - vms->highest_gpa = base - 1; > + if (vms->highmem) { > + /* If we have highmem, move the IPA limit to the top */ > + ceiling = base; > + } > + vms->highest_gpa = ceiling - 1; This doesn't look right to me. If highmem is false and the high IO region would be above the 4GB mark then we should not create the high IO region at all, surely? This code looks like it goes ahead and puts the high IO region above 4GB and then lies in the highest_gpa value about what the highest used GPA is. -- PMM
Hi On 9/7/21 2:58 PM, Peter Maydell wrote: > On Sun, 22 Aug 2021 at 15:45, Marc Zyngier <maz@kernel.org> wrote: >> Even when the VM is configured with highmem=off, the highest_gpa >> field includes devices that are above the 4GiB limit, which is >> what highmem=off is supposed to enforce. This leads to failures >> in virt_kvm_type() on systems that have a crippled IPA range, >> as the reported IPA space is larger than what it should be. >> >> Instead, honor the user-specified limit to only use the devices >> at the lowest end of the spectrum. >> >> Note that this doesn't affect memory, which is still allowed to >> go beyond 4GiB with highmem=on configurations. >> >> Cc: Andrew Jones <drjones@redhat.com> >> Cc: Eric Auger <eric.auger@redhat.com> >> Cc: Peter Maydell <peter.maydell@linaro.org> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> hw/arm/virt.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index 81eda46b0b..bc189e30b8 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -1598,7 +1598,7 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx) >> static void virt_set_memmap(VirtMachineState *vms) >> { >> MachineState *ms = MACHINE(vms); >> - hwaddr base, device_memory_base, device_memory_size; >> + hwaddr base, device_memory_base, device_memory_size, ceiling; >> int i; >> >> vms->memmap = extended_memmap; >> @@ -1625,7 +1625,7 @@ static void virt_set_memmap(VirtMachineState *vms) >> device_memory_size = ms->maxram_size - ms->ram_size + ms->ram_slots * GiB; >> >> /* Base address of the high IO region */ >> - base = device_memory_base + ROUND_UP(device_memory_size, GiB); >> + ceiling = base = device_memory_base + ROUND_UP(device_memory_size, GiB); >> if (base < device_memory_base) { >> error_report("maxmem/slots too huge"); >> exit(EXIT_FAILURE); >> @@ -1642,7 +1642,11 @@ static void virt_set_memmap(VirtMachineState *vms) >> vms->memmap[i].size = size; >> base += size; >> } >> - vms->highest_gpa = base - 1; >> + if (vms->highmem) { >> + /* If we have highmem, move the IPA limit to the top */ >> + ceiling = base; >> + } >> + vms->highest_gpa = ceiling - 1; > This doesn't look right to me. If highmem is false and the > high IO region would be above the 4GB mark then we should not > create the high IO region at all, surely? This code looks like > it goes ahead and puts the high IO region above 4GB and then > lies in the highest_gpa value about what the highest used GPA is. > > -- PMM > Doesn't the problem come from "if maxram_size is < 255GiB we keep the legacy memory map" and set base = vms->memmap[VIRT_MEM].base + LEGACY_RAMLIMIT_BYTES; leading to IO regions allocated above? Instead shouldn't we condition this to highmem=on only then? But by the way do we need to added extended_memmap IO regions at all if highmem=off? I am not wrong the VIRT_HIGH_PCIE_ECAM and VIRT_HIGH_PCIE_MMIO only are used if highmem=on. In create_pcie(), base_mmio_high/size_mmio_high are used if vms->highmem and we have ecam_id = VIRT_ECAM_ID(vms->highmem_ecam); with vms->highmem_ecam &= vms->highmem && (!firmware_loaded || aarch64); So if I do not miss anything maybe we could skip the allocation of the extended_memmap IO regions if highmem=off? And doesn't it look reasonable to limit the number of vcpus if highmem=off? Thanks Eric
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index 81eda46b0b..bc189e30b8 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1598,7 +1598,7 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx) static void virt_set_memmap(VirtMachineState *vms) { MachineState *ms = MACHINE(vms); - hwaddr base, device_memory_base, device_memory_size; + hwaddr base, device_memory_base, device_memory_size, ceiling; int i; vms->memmap = extended_memmap; @@ -1625,7 +1625,7 @@ static void virt_set_memmap(VirtMachineState *vms) device_memory_size = ms->maxram_size - ms->ram_size + ms->ram_slots * GiB; /* Base address of the high IO region */ - base = device_memory_base + ROUND_UP(device_memory_size, GiB); + ceiling = base = device_memory_base + ROUND_UP(device_memory_size, GiB); if (base < device_memory_base) { error_report("maxmem/slots too huge"); exit(EXIT_FAILURE); @@ -1642,7 +1642,11 @@ static void virt_set_memmap(VirtMachineState *vms) vms->memmap[i].size = size; base += size; } - vms->highest_gpa = base - 1; + if (vms->highmem) { + /* If we have highmem, move the IPA limit to the top */ + ceiling = base; + } + vms->highest_gpa = ceiling - 1; if (device_memory_size > 0) { ms->device_memory = g_malloc0(sizeof(*ms->device_memory)); ms->device_memory->base = device_memory_base;
Even when the VM is configured with highmem=off, the highest_gpa field includes devices that are above the 4GiB limit, which is what highmem=off is supposed to enforce. This leads to failures in virt_kvm_type() on systems that have a crippled IPA range, as the reported IPA space is larger than what it should be. Instead, honor the user-specified limit to only use the devices at the lowest end of the spectrum. Note that this doesn't affect memory, which is still allowed to go beyond 4GiB with highmem=on configurations. Cc: Andrew Jones <drjones@redhat.com> Cc: Eric Auger <eric.auger@redhat.com> Cc: Peter Maydell <peter.maydell@linaro.org> Signed-off-by: Marc Zyngier <maz@kernel.org> --- hw/arm/virt.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)