On Wed, Oct 26 2022, Gavin Shan <gs...@redhat.com> wrote:
> Hi Connie,
>
> On 10/25/22 6:54 PM, Cornelia Huck wrote:
>> On Mon, Oct 24 2022, Gavin Shan <gs...@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 <m...@kernel.org>
>>> Signed-off-by: Gavin Shan <gs...@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);
}
