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

[Cornelia Huck]

On Wed, Oct 26 2022, Gavin Shan <gshan@redhat.com> wrote:

> 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

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?)

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

>
> 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);
    }