Re: [PATCH v6 7/7] hw/arm/virt: Add properties to disable high memory regions

[Gavin Shan]

Hi Connie,

On 10/25/22 6:54 PM, Cornelia Huck wrote:
> On Mon, Oct 24 2022, Gavin Shan <gshan@redhat.com> wrote:
>
> > These 3 high memory regions are usually enabled by default, but
>
> s/These 3/The/ ?

Ok.

> > they may be not used. For example, VIRT_HIGH_GIC_REDIST2 isn't
> > needed by GICv2. This leads to waste in the PA space.
>
> When building the command line, do we have enough information on when
> the regions provide something useful, and when they just waste space?

I think the help messages are already indicative. For example, the help
messages for 'highmem-redist2' indicate the region is only needed by
GICv3 or GICv4. 'highmem-ecam' and 'highmem-mmio' are needed by PCI ECAM
and MMIO and the key words 'high' indicates they're the corresponding
second window.

#./qemu-system-aarch64 -M virt,?
highmem-ecam=<bool>    - Set on/off to enable/disable high memory region for 
PCI ECAM
highmem-mmio=<bool>    - Set on/off to enable/disable high memory region for 
PCI MMIO
highmem-redists=<bool> - Set on/off to enable/disable high memory region for 
GICv3 or GICv4 redistributor

> > Add properties to allow users selectively disable them if needed:
> > "highmem-redists", "highmem-ecam", "highmem-mmio".
> >
> > Suggested-by: Marc Zyngier <maz@kernel.org>
> > Signed-off-by: Gavin Shan <gshan@redhat.com>
> > ---
> >   docs/system/arm/virt.rst | 12 ++++++++
> >   hw/arm/virt.c            | 64 ++++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 76 insertions(+)
> >
> > diff --git a/docs/system/arm/virt.rst b/docs/system/arm/virt.rst
> > index 4454706392..a1668a969d 100644
> > --- a/docs/system/arm/virt.rst
> > +++ b/docs/system/arm/virt.rst
> > @@ -98,6 +98,18 @@ compact-highmem
> >     Set ``on``/``off`` to enable/disable the compact layout for high memory 
> > regions.
> >     The default is ``on`` for machine types later than ``virt-7.2``.
> >   
> > +highmem-redists
> > +  Set ``on``/``off`` to enable/disable the high memry region for GICv3/4
>
> s/memry/memory/

Ok, copy-and-paste error. Will be fixed.

> > +  redistributor. The default is ``on``.
>
> Do we need to add a note about what effects setting this to "off" may
> have, e.g. "Setting this to ``off`` may limit the maximum number of
> cpus." or so? And/or "Setting this to ``off`` when using GICv2 will save
> some space."?

We may not mention GICv2 since GICv3/v4 are already mentioned. It's a
good idea to mention that the maximum number of CPUs is reduced when
it's turned off. I will have something like below in next respin if
you agree.

highmem-redists
  Set ``on``/``off`` to enable/disable the high memroy region for GICv3 or
  GICv4 redistributor. The default is ``on``. Setting this to ``off`` will
  limit the maximum number of CPUs when GICv3 or GICv4 is used.

Since 'vms->highmem_redists' is changeable, the 'virt_max_cpus' in
machvirt_init() needs to be recalculated based on that. The code change
will be included into next respin. Besides, the follow-up error message
will be improved to something like below.

  error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
               "supported by machine 'mach-virt' (%d). The high memory "
               "region for GICv3 or GICv4 redistributor has been %s",
               max_cpus, virt_max_cpus,
               vms->highmem_redists ? "enabled" : "disabled");

> > +
> > +highmem-ecam
> > +  Set ``on``/``off`` to enable/disable the high memry region for PCI ECAM.
>
> s/memry/memory/

Ok, copy-and-paste error. Will be fixed.

> > +  The default is ``on`` for machine types later than ``virt-3.0``.
> > +
> > +highmem-mmio
> > +  Set ``on``/``off`` to enable/disable the high memry region for PCI MMIO.
>
> s/memry/memory/

Ok. copy-and-paste error. Will be fixed.

> > +  The default is ``on``.
> > +
> >   gic-version
> >     Specify the version of the Generic Interrupt Controller (GIC) to provide.
> >     Valid values are:

Thanks,
Gavin