Re: [PATCH v3 5/5] hw/arm/virt: Add 'highmem-compact' property

[Gavin Shan]

Hi Cornelia,

On 9/29/22 8:27 PM, Cornelia Huck wrote:
> On Thu, Sep 29 2022, Gavin Shan <gshan@redhat.com> wrote:
> > On 9/28/22 10:22 PM, Eric Auger wrote:
> > > On 9/22/22 01:13, Gavin Shan wrote:
> > > > After the improvement to high memory region address assignment is
> > > > applied, the memory layout is changed. For example, VIRT_HIGH_PCIE_MMIO
> > > s/the memory layout is changed./the memory layout is changed,
> > > introducing possible migration breakage.
> >
> > Ok, much clearer.
> >
> > > > memory region is enabled when the improvement is applied, but it's
> > > > disabled if the improvement isn't applied.
> > > >
> > > >       pa_bits              = 40;
> > > >       vms->highmem_redists = false;
> > > >       vms->highmem_ecam    = false;
> > > >       vms->highmem_mmio    = true;
> > > >
> > > >       # qemu-system-aarch64 -accel kvm -cpu host \
> > > >         -machine virt-7.2 -m 4G,maxmem=511G      \
> > > >         -monitor stdio
> > > >
> > > > In order to keep backwords compatibility, we need to disable the
> > > > optimization on machines, which is virt-7.1 or ealier than it. It
> > > > means the optimization is enabled by default from virt-7.2. Besides,
> > > > 'highmem-compact' property is added so that the optimization can be
> > > I would rather rename the property into compact-highmem even if the vms
> > > field is name highmem_compact to align with other highmem fields
> >
> > Ok, but I would love to know why. Note that we already have
> > 'highmem=on|off'. 'highmem_compact=on|off' seems consistent
> > to me.
>
> FWIW, I initially misread 'highmem_compact' as 'highmem_compat' (and had
> to re-read because I got confused). At least to me, 'compact_highmem'
> has less chance of being parsed incorrectly :) (although that is
> probably a personal thing.)

Ok. 'compact-highmem' is also fine to me. I'm really bad at naming :)

> > > > explicitly enabled or disabled on all machine types by users.
> > > >
> > > > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > > > ---
> > > >    docs/system/arm/virt.rst |  4 ++++
> > > >    hw/arm/virt.c            | 33 +++++++++++++++++++++++++++++++++
> > > >    include/hw/arm/virt.h    |  2 ++
> > > >    3 files changed, 39 insertions(+)
> > > >
> > > > diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
> > > > index 20442ea2c1..f05ec2253b 100644
> > > > --- a/docs/system/arm/virt.rst
> > > > +++ b/docs/system/arm/virt.rst
> > > > @@ -94,6 +94,10 @@ highmem
> > > >      address space above 32 bits. The default is ``on`` for machine types
> > > >      later than ``virt-2.12``.
> > > >    
> > > > +highmem-compact
> > > > +  Set ``on``/``off`` to enable/disable compact space for high memory regions.
> > > > +  The default is ``on`` for machine types later than ``virt-7.2``
> > > I think you should document what is compact layout versus legacy one,
> > > both in the commit msg and maybe as a comment in a code along with the
> > > comment in hw/arm/virt.c starting with 'Highmem IO Regions: '
> >
> > Ok, I will add this into the commit log in v4. I don't think it's necessary
> > to add duplicate comment in the code. People can check the commit log for
> > details if needed.
>
> Rather explain it in this file here, maybe? I'd prefer to be able to
> find out what 'compact' means without digging through the commit log.

Ok, lets do as Eric suggested. There are existing comments about
@extended_memmap[] in hw/arm/virt.c. We need to explain the legacy/modern
laoyout and 'compact-highmem' property there.

Thanks,
Gavin