| Message ID | 20211003164605.3116450-4-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Reduced-IPA space and highmem=off fixes | expand |
On Sun, Oct 03, 2021 at 05:46:03PM +0100, Marc Zyngier wrote: > Even when the VM is configured with highmem=off, the highest_gpa > field includes devices that are above the 4GiB limit. > Similarily, nothing seem to check that the memory is within > the limit set by the highmem=off option. > > 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, and fail if we have memory > crossing the 4GiB limit. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > hw/arm/virt.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index bcf58f677d..9d2abdbd5f 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1628,6 +1628,11 @@ static void virt_set_memmap(VirtMachineState *vms) > exit(EXIT_FAILURE); > } > > + if (!vms->highmem && > + vms->memmap[VIRT_MEM].base + ms->maxram_size > 4 * GiB) { > + error_report("highmem=off, but memory crosses the 4GiB limit\n"); > + exit(EXIT_FAILURE); > + } > /* > * We compute the base of the high IO region depending on the > * amount of initial and device memory. The device memory start/size > @@ -1657,7 +1662,9 @@ static void virt_set_memmap(VirtMachineState *vms) > vms->memmap[i].size = size; > base += size; > } > - vms->highest_gpa = base - 1; > + vms->highest_gpa = (vms->highmem ? > + base : > + vms->memmap[VIRT_MEM].base + ms->maxram_size) - 1; > if (device_memory_size > 0) { > ms->device_memory = g_malloc0(sizeof(*ms->device_memory)); > ms->device_memory->base = device_memory_base; > -- > 2.30.2 > Reviewed-by: Andrew Jones <drjones@redhat.com>
Hi Marc, On 10/3/21 6:46 PM, Marc Zyngier wrote: > Even when the VM is configured with highmem=off, the highest_gpa > field includes devices that are above the 4GiB limit. > Similarily, nothing seem to check that the memory is within > the limit set by the highmem=off option. > > 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, and fail if we have memory > crossing the 4GiB limit. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > hw/arm/virt.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index bcf58f677d..9d2abdbd5f 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1628,6 +1628,11 @@ static void virt_set_memmap(VirtMachineState *vms) > exit(EXIT_FAILURE); > } > > + if (!vms->highmem && > + vms->memmap[VIRT_MEM].base + ms->maxram_size > 4 * GiB) { > + error_report("highmem=off, but memory crosses the 4GiB limit\n"); > + exit(EXIT_FAILURE); > + } > /* > * We compute the base of the high IO region depending on the > * amount of initial and device memory. The device memory start/size > @@ -1657,7 +1662,9 @@ static void virt_set_memmap(VirtMachineState *vms) > vms->memmap[i].size = size; > base += size; > } > - vms->highest_gpa = base - 1; > + vms->highest_gpa = (vms->highmem ? > + base : > + vms->memmap[VIRT_MEM].base + ms->maxram_size) - 1; I think I would have preferred to have if (vms->highmem) { for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) { hwaddr size = extended_memmap[i].size; base = ROUND_UP(base, size); vms->memmap[i].base = base; vms->memmap[i].size = size; base += size; } } as it is useless to execute that code and create new memmap entries in case of !highmem. But nevertheless, this looks correct Eric > if (device_memory_size > 0) { > ms->device_memory = g_malloc0(sizeof(*ms->device_memory)); > ms->device_memory->base = device_memory_base;
On Mon, 04 Oct 2021 13:23:41 +0100, Eric Auger <eric.auger@redhat.com> wrote: > > Hi Marc, > > On 10/3/21 6:46 PM, Marc Zyngier wrote: > > Even when the VM is configured with highmem=off, the highest_gpa > > field includes devices that are above the 4GiB limit. > > Similarily, nothing seem to check that the memory is within > > the limit set by the highmem=off option. > > > > 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, and fail if we have memory > > crossing the 4GiB limit. > > > > Signed-off-by: Marc Zyngier <maz@kernel.org> > > --- > > hw/arm/virt.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index bcf58f677d..9d2abdbd5f 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -1628,6 +1628,11 @@ static void virt_set_memmap(VirtMachineState *vms) > > exit(EXIT_FAILURE); > > } > > > > + if (!vms->highmem && > > + vms->memmap[VIRT_MEM].base + ms->maxram_size > 4 * GiB) { > > + error_report("highmem=off, but memory crosses the 4GiB limit\n"); > > + exit(EXIT_FAILURE); > > + } > > /* > > * We compute the base of the high IO region depending on the > > * amount of initial and device memory. The device memory start/size > > @@ -1657,7 +1662,9 @@ static void virt_set_memmap(VirtMachineState *vms) > > vms->memmap[i].size = size; > > base += size; > > } > > - vms->highest_gpa = base - 1; > > + vms->highest_gpa = (vms->highmem ? > > + base : > > + vms->memmap[VIRT_MEM].base + ms->maxram_size) - 1; > I think I would have preferred to have > > if (vms->highmem) { > for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) { > hwaddr size = extended_memmap[i].size; > > base = ROUND_UP(base, size); > vms->memmap[i].base = base; > vms->memmap[i].size = size; > base += size; > } > } > as it is useless to execute that code and create new memmap entries in > case of !highmem. I agree that it is a bit useless when we only have highmem. But we really want to deal with arbitrary IPA spaces (see how this changes in the follow-up patches), and we need to check that everything fits in the IPA space (and fix things up if they don't). > > But nevertheless, this looks correct Thanks, M.
diff --git a/hw/arm/virt.c b/hw/arm/virt.c index bcf58f677d..9d2abdbd5f 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -1628,6 +1628,11 @@ static void virt_set_memmap(VirtMachineState *vms) exit(EXIT_FAILURE); } + if (!vms->highmem && + vms->memmap[VIRT_MEM].base + ms->maxram_size > 4 * GiB) { + error_report("highmem=off, but memory crosses the 4GiB limit\n"); + exit(EXIT_FAILURE); + } /* * We compute the base of the high IO region depending on the * amount of initial and device memory. The device memory start/size @@ -1657,7 +1662,9 @@ static void virt_set_memmap(VirtMachineState *vms) vms->memmap[i].size = size; base += size; } - vms->highest_gpa = base - 1; + vms->highest_gpa = (vms->highmem ? + base : + vms->memmap[VIRT_MEM].base + ms->maxram_size) - 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. Similarily, nothing seem to check that the memory is within the limit set by the highmem=off option. 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, and fail if we have memory crossing the 4GiB limit. Signed-off-by: Marc Zyngier <maz@kernel.org> --- hw/arm/virt.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)