Re: [PATCH v4 4/6] hw/arm/virt: Introduce virt_get_high_memmap_enabled() helper

[Gavin Shan]

On 10/12/22 12:45 AM, Eric Auger wrote:
> On 10/5/22 00:47, Gavin Shan wrote:
> > On 10/4/22 6:41 PM, Cornelia Huck wrote:
> > > On Tue, Oct 04 2022, Gavin Shan <gshan@redhat.com> wrote:
> > >
> > > > This introduces virt_get_high_memmap_enabled() helper, which returns
> > > > the pointer to vms->highmem_{redists, ecam, mmio}. The pointer will
> > > > be used in the subsequent patches.
> > > >
> > > > No functional change intended.
> > > >
> > > > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > > > ---
> > > >    hw/arm/virt.c | 30 +++++++++++++++++-------------
> > > >    1 file changed, 17 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > index b0b679d1f4..59de7b78b5 100644
> > > > --- a/hw/arm/virt.c
> > > > +++ b/hw/arm/virt.c
> > > > @@ -1689,14 +1689,29 @@ static uint64_t
> > > > virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
> > > >        return arm_cpu_mp_affinity(idx, clustersz);
> > > >    }
> > > >    +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));
> > >
> > > I wonder whether we want an assert(ARRAY_SIZE(extended_memmap) ==
> > > ARRAY_SIZE(enabled_array))? IIUC, we never want those two to get out of
> > > sync?
> >
> > Yeah, It makes sense to ensure both arrays synchronized. I will add
> > the extra check in next respin.
>
> With Connie's suggestion this looks good to me.

What we need is actually like below because the array (extended_memmap)
starts from VIRT_LOWMEMMAP_LAST instead of zero. I'm adding the extra
check into v5, which will be posted shortly.

   assert(ARRAY_SIZE(extended_memmap) - VIRT_LOWMEMMAP_LAST ==
          ARRAY_SIZE(enabled_array));

> > > > +
> > > > +    return enabled_array[index - VIRT_LOWMEMMAP_LAST];
> > > > +}
> > > > +

Thanks,
Gavin