Hi Gavin,
On 8/2/22 08:45, Gavin Shan wrote:
> There are 3 highmem IO regions as below. They can be disabled in
> two situations: (a) The specific region is disabled by user. (b)
> The specific region doesn't fit in the PA space. However, the base
> address and highest_gpa are still updated no matter if the region
> is enabled or disabled. It's incorrectly incurring waste in the PA
> space.
If I am not wrong highmem_redists and highmem_mmio are not user selectable
Only highmem ecam depends on machine type & ACPI setup. But I would say
that in server use case it is always set. So is that optimization really
needed?
>
> Improve address assignment for highmem IO regions to avoid the waste
> in the PA space by putting the logic into virt_memmap_fits().
>
> Signed-off-by: Gavin Shan <gs...@redhat.com>
> ---
> hw/arm/virt.c | 54 +++++++++++++++++++++++++++++----------------------
> 1 file changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 9633f822f3..bc0cd218f9 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1688,6 +1688,34 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState
> *vms, int idx)
> return arm_cpu_mp_affinity(idx, clustersz);
> }
>
> +static void virt_memmap_fits(VirtMachineState *vms, int index,
> + bool *enabled, hwaddr *base, int pa_bits)
> +{
> + hwaddr size = extended_memmap[index].size;
> +
> + /* The region will be disabled if its size isn't given */
> + if (!*enabled || !size) {
In which case do you have !size?
> + *enabled = false;
> + vms->memmap[index].base = 0;
> + vms->memmap[index].size = 0;
It looks dangerous to me to reset the region's base and size like that.
for instance fdt_add_gic_node() will add dummy data in the dt.
> + return;
> + }
> +
> + /*
> + * Check if the memory region fits in the PA space. The memory map
> + * and highest_gpa are updated if it fits. Otherwise, it's disabled.
> + */
> + *enabled = (ROUND_UP(*base, size) + size <= BIT_ULL(pa_bits));
using a 'fits' local variable would make the code more obvious I think
> + if (*enabled) {
> + *base = ROUND_UP(*base, size);
> + vms->memmap[index].base = *base;
> + vms->memmap[index].size = size;
> + vms->highest_gpa = *base + size - 1;
> +
> + *base = *base + size;
> + }
> +}
> +
> static void virt_set_memmap(VirtMachineState *vms, int pa_bits)
> {
> MachineState *ms = MACHINE(vms);
> @@ -1744,37 +1772,17 @@ static void virt_set_memmap(VirtMachineState *vms,
> int pa_bits)
> vms->highest_gpa = memtop - 1;
>
> for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) {
> - hwaddr size = extended_memmap[i].size;
> - bool fits;
> -
> - base = ROUND_UP(base, size);
> - vms->memmap[i].base = base;
> - vms->memmap[i].size = size;
> -
> - /*
> - * Check each device to see if they fit in the PA space,
> - * moving highest_gpa as we go.
> - *
> - * For each device that doesn't fit, disable it.
> - */
> - fits = (base + size) <= BIT_ULL(pa_bits);
> - if (fits) {
> - vms->highest_gpa = base + size - 1;
> - }
> -
we could avoid running the code below in case highmem is not set. We would need
to reset that flags though.
> switch (i) {
> case VIRT_HIGH_GIC_REDIST2:
> - vms->highmem_redists &= fits;
> + virt_memmap_fits(vms, i, &vms->highmem_redists, &base, pa_bits);
> break;
> case VIRT_HIGH_PCIE_ECAM:
> - vms->highmem_ecam &= fits;
> + virt_memmap_fits(vms, i, &vms->highmem_ecam, &base, pa_bits);
> break;
> case VIRT_HIGH_PCIE_MMIO:
> - vms->highmem_mmio &= fits;
> + virt_memmap_fits(vms, i, &vms->highmem_mmio, &base, pa_bits);
> break;
> }
> -
> - base += size;
> }
>
> if (device_memory_size > 0) {
Thanks
Eric
