Re: [PATCH 1/2] hw/arm/virt: Improve address assignment for highmem IO regions

[Gavin Shan]

Hi Eric,

On 8/2/22 7:41 PM, Eric Auger wrote:
> 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?

There are two other cases you missed.

- highmem_ecam is enabled after virt-2.12, meaning it stays disabled
  before that.

- The high memory region can be disabled if user is asking large
  (normal) memory space through 'maxmem=' option. When the requested
  memory by 'maxmem=' is large enough, the high memory regions are
  disabled. It means the normal memory has higher priority than those
  high memory regions. This is the case I provided in (b) of the
  commit log.

In the commit log, I was supposed to say something like below for
(a):

- The specific high memory region can be disabled through changing
  the code by user or developer. For example, 'vms->highmem_mmio'
  is changed from true to false in virt_instance_init().

> > 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 <gshan@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?

Yeah, we don't have !size and the condition should be removed.

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

I would guess you missed that the high memory regions won't be exported
through device-tree if they have been disabled. We have a check there,
which is "if (nb_redist_regions == 1)"

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

Lets confirm if you're suggesting something like below?

        bool fits;

        fits = (ROUND_UP(*base, size) + size <= BIT_ULL(pa_bits));

        if (fits) {
           /* update *base, memory mapping, highest_gpa */
        } else {
           *enabled = false;
        }

I guess we can simply do

        if (ROUND_UP(*base, size) + size <= BIT_ULL(pa_bits)) {
           /* update *base, memory mapping, highest_gpa */
        } else {
           *enabled = false;
        }

Please let me know which one looks best to you.

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

Yeah, I think it's not a big deal. My though is to update the flag in 
virt_memmap_fits().

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