Hi Gavin, On 9/22/22 01:13, Gavin Shan wrote: > There are three high memory regions, which are VIRT_HIGH_REDIST2, > VIRT_HIGH_PCIE_ECAM and VIRT_HIGH_PCIE_MMIO. Their base addresses > are floating on highest RAM address. However, they can be disabled > in several cases. > > (1) One specific high memory region is disabled by developer by > toggling vms->highmem_{redists, ecam, mmio}. > > (2) VIRT_HIGH_PCIE_ECAM region is disabled on machine, which is > 'virt-2.12' or ealier than it. > > (3) VIRT_HIGH_PCIE_ECAM region is disabled when firmware is loaded > on 32-bits system. > > (4) One specific high memory region is disabled when it breaks the > PA space limit. > > The current implementation of virt_set_memmap() isn't comprehensive > because the space for one specific high memory region is always > reserved from the PA space for case (1), (2) and (3). In the code, > 'base' and 'vms->highest_gpa' are always increased for those three > cases. It's unnecessary since the assigned space of the disabled > high memory region won't be used afterwards. > > This improves the address assignment for those three high memory > region by skipping the address assignment for one specific high > memory region if it has been disabled in case (1), (2) and (3). > > Signed-off-by: Gavin Shan <gs...@redhat.com> > --- > hw/arm/virt.c | 44 ++++++++++++++++++++++++++------------------ > 1 file changed, 26 insertions(+), 18 deletions(-) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index b0b679d1f4..b702f8f2b5 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -1693,15 +1693,31 @@ static void virt_set_high_memmap(VirtMachineState > *vms, > hwaddr base, int pa_bits) > { > hwaddr region_base, region_size; > - bool fits; > + bool *region_enabled, fits; IDo you really need a pointer? If the region is unknown this is a bug in virt code. > int i; > > for (i = VIRT_LOWMEMMAP_LAST; i < ARRAY_SIZE(extended_memmap); i++) { > region_base = ROUND_UP(base, extended_memmap[i].size); > region_size = extended_memmap[i].size; > > - vms->memmap[i].base = region_base; > - vms->memmap[i].size = region_size; > + switch (i) { > + case VIRT_HIGH_GIC_REDIST2: > + region_enabled = &vms->highmem_redists; > + break; > + case VIRT_HIGH_PCIE_ECAM: > + region_enabled = &vms->highmem_ecam; > + break; > + case VIRT_HIGH_PCIE_MMIO: > + region_enabled = &vms->highmem_mmio; > + break; While we are at it I would change the vms fields dealing with those highmem regions and turn those fields into an array of bool indexed using i - VIRT_LOWMEMMAP_LAST (using a macro or something alike). We would not be obliged to have this switch, now duplicated. > + default: > + region_enabled = NULL; > + } > + > + /* Skip unknown region */ > + if (!region_enabled) { > + continue; > + } > > /* > * Check each device to see if they fit in the PA space, > @@ -1710,23 +1726,15 @@ static void virt_set_high_memmap(VirtMachineState > *vms, > * For each device that doesn't fit, disable it. > */ > fits = (region_base + region_size) <= BIT_ULL(pa_bits); > - if (fits) { > - vms->highest_gpa = region_base + region_size - 1; > - } > + if (*region_enabled && fits) { > + vms->memmap[i].base = region_base; > + vms->memmap[i].size = region_size; > > - switch (i) { > - case VIRT_HIGH_GIC_REDIST2: > - vms->highmem_redists &= fits; > - break; > - case VIRT_HIGH_PCIE_ECAM: > - vms->highmem_ecam &= fits; > - break; > - case VIRT_HIGH_PCIE_MMIO: > - vms->highmem_mmio &= fits; > - break; > + vms->highest_gpa = region_base + region_size - 1; > + base = region_base + region_size; > + } else { > + *region_enabled = false; > } > - > - base = region_base + region_size; > } > } > Thanks
Eric