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

[Gavin Shan]

Hi Connie,

On 10/26/22 7:10 PM, Cornelia Huck wrote:
> On Wed, Oct 26 2022, Gavin Shan <gshan@redhat.com> wrote:
> > 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
>
> OK, hopefully this is enough for anyone building a command line
> directly. (Do we want to encourage management software like libvirt to
> switch off regions that are not needed?)

Yeah, to disable those regions aren't needed will make libvirt smart.
I think it's worthy for libvirt to do it. I'm not sure if arbitrary
machine properties have been supported by libvirt or not. For example,
'highmem-redists' can still be turned off from 'vm1.xml' even though
it's not explicitly supported in libvirt's code.

> > > > 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.
>
> OK, sounds reasonable to me.

Ok, Thanks for your confirm.

> > 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");
>
> Hm, the doc for error_report() states that "The resulting message should
> be a single phrase, with no newline or trailing punctuation." Maybe
>
>      if (max_cpus > virt_max_cpus) {
>          error_report("Number of SMP CPUs requested (%d) exceeds max CPUs "
>                       "supported by machine 'mach-virt' (%d)",
>                       max_cpus, virt_max_cpus);
>          if (vms->gic_version != VIRT_GIC_VERSION_2 &&
>              !vms->higmem_redists) {
>              error_printf("Try 'highmem-redists=on' for more CPUs\n");
>          }
>          exit(1);
>      }

Ok, I didn't notice the message raised by error_report() has this sort
of requirements. Your suggested snippet looks good to me and it will
be included in next respin. However, I would hold next respin (v7) for
a while to see if I can receive comments from Peter/Marc on v6 :)

Thanks,
Gavin