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

[Gavin Shan <gshan@redhat.com>]

Hi Cornelia,

On 9/29/22 8:27 PM, Cornelia Huck wrote:
> On Thu, Sep 29 2022, Gavin Shan <gshan@redhat.com> wrote:
>> 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.)
> 

Ok. 'compact-highmem' is also fine to me. I'm really bad at naming :)

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

Ok, lets do as Eric suggested. There are existing comments about
@extended_memmap[] in hw/arm/virt.c. We need to explain the legacy/modern
laoyout and 'compact-highmem' property there.

Thanks,
Gavin