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

[Cornelia Huck]

On Thu, Sep 29 2022, Gavin Shan <gshan@redhat.com> wrote:

> Hi Eric,
>
> 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.)

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