Re: [PATCH v3 4/5] hw/arm/virt: Improve high memory region address assignment

[Gavin Shan]

Hi Eric,

On 9/28/22 10:51 PM, Eric Auger wrote:
> 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 <gshan@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.

The pointer is needed so that we can disable the region by setting 'false'
to it at later point. Yeah, I think you're correct that 'unknown region'
is a bug and we need to do assert(region_enabled), or something like below.

> >       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.

It makes sense to me. How about to have something like below in v4?

static inline bool *virt_get_high_memmap_enabled(VirtMachineState *vms, int 
index)
{
    bool *enabled_array[] = {
          &vms->highmem_redists,
          &vms->highmem_ecam,
          &vms->highmem_mmio,
    };

    assert(index - VIRT_LOWMEMMAP_LAST < ARRAY_SIZE(enabled_array));

    return enabled_array[index - VIRT_LOWMEMMAP_LAST];
}

> > +        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,
Gavin