Re: [PATCH v3 3/5] hw/arm/virt: Introduce variable region_base in virt_set_high_memmap()

[Gavin Shan]

Hi Eric,

On 9/28/22 10:10 PM, Eric Auger wrote:
> On 9/22/22 01:13, Gavin Shan wrote:
> > This introduces variable 'region_base' for the base address of the
> > specific high memory region. It's the preparatory work to optimize
> > high memory region address assignment.
> Why is it a preparatory work (same comment for previous patch, ie [2/5]
> ). Are those changes really needed? why?

In PATCH[4/5], @base argument is added to virt_set_high_memmap(), to
represent current global base address. With the optimization applied
in PATCH[4/5], @base isn't unconditionally updated to the top of the
iterated high memory region. So we need @region_base here (PATCH[3/5])
to track the aligned base address for the iterated high memory region,
which may or may be not updated to @base.

Since we have @region_base in PATCH[3/5], it'd better to have @region_size
in PATCH[2/5].

Actually, PATCH[1-3/5] are all preparatory patches for PATCH[4/5]. My
intention was to organize the patches in a way to keep the logical
change part simple enough, for easier review.

Thanks,
Gavin

> > No functional change intended.
> >
> > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > ---
> >   hw/arm/virt.c | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 187b3ee0e2..b0b679d1f4 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -1692,15 +1692,15 @@ static uint64_t virt_cpu_mp_affinity(VirtMachineState 
> > *vms, int idx)
> >   static void virt_set_high_memmap(VirtMachineState *vms,
> >                                    hwaddr base, int pa_bits)
> >   {
> > -    hwaddr region_size;
> > +    hwaddr region_base, region_size;
> >       bool fits;
> >       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;
> >   
> > -        base = ROUND_UP(base, region_size);
> > -        vms->memmap[i].base = base;
> > +        vms->memmap[i].base = region_base;
> >           vms->memmap[i].size = region_size;
> >   
> >           /*
> > @@ -1709,9 +1709,9 @@ static void virt_set_high_memmap(VirtMachineState *vms,
> >            *
> >            * For each device that doesn't fit, disable it.
> >            */
> > -        fits = (base + region_size) <= BIT_ULL(pa_bits);
> > +        fits = (region_base + region_size) <= BIT_ULL(pa_bits);
> >           if (fits) {
> > -            vms->highest_gpa = base + region_size - 1;
> > +            vms->highest_gpa = region_base + region_size - 1;
> >           }
> >   
> >           switch (i) {
> > @@ -1726,7 +1726,7 @@ static void virt_set_high_memmap(VirtMachineState *vms,
> >               break;
> >           }
> >   
> > -        base += region_size;
> > +        base = region_base + region_size;
> >       }
> >   }
> >