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

[Eric Auger <eric.auger@redhat.com>]

Hi Gavin,

On 9/29/22 01:49, Gavin Shan 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.
To me the property name should rather sound 'english' with the adjective
before the name 'high memory"' but I am not a native english speaker
either.
>
>>> 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.
>
>>> +
>>>   gic-version
>>>     Specify the version of the Generic Interrupt Controller (GIC) to
>>> provide.
>>>     Valid values are:
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index b702f8f2b5..a4fbdaef91 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -1734,6 +1734,13 @@ static void
>>> virt_set_high_memmap(VirtMachineState *vms,
>>>               base = region_base + region_size;
>>>           } else {
>>>               *region_enabled = false;
>>> +
>>> +            if (!vms->highmem_compact) {
>> this snippet should be already present in previous patch otherwise this
>> will break bisectability.
>>
>
> Hmm, nice catch! I think I need to swap PATCH[4] and PATCH[5] in next
> revision. In that order, 'compact-highmem' is introduced in PATCH[4],
> but not used yet. PATCH[5] has the optimization and 'compact-highmem'
> is used.
No in general you introduce the property at the very end with the code
guarded with an unset vms->highmem_compact in the previous patch.

Thanks

Eric
>
>>> +                base = region_base + region_size;
>>> +                if (fits) {
>>> +                    vms->highest_gpa = region_base + region_size - 1;
>>> +                }
>>> +            }
>>>           }
>>>       }
>>>   }
>>> @@ -2348,6 +2355,20 @@ static void virt_set_highmem(Object *obj,
>>> bool value, Error **errp)
>>>       vms->highmem = value;
>>>   }
>>>   +static bool virt_get_highmem_compact(Object *obj, Error **errp)
>>> +{
>>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>>> +
>>> +    return vms->highmem_compact;
>>> +}
>>> +
>>> +static void virt_set_highmem_compact(Object *obj, bool value, Error
>>> **errp)
>>> +{
>>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>>> +
>>> +    vms->highmem_compact = value;
>>> +}
>>> +
>>>   static bool virt_get_its(Object *obj, Error **errp)
>>>   {
>>>       VirtMachineState *vms = VIRT_MACHINE(obj);
>>> @@ -2966,6 +2987,13 @@ static void
>>> virt_machine_class_init(ObjectClass *oc, void *data)
>>>                                             "Set on/off to
>>> enable/disable using "
>>>                                             "physical address space
>>> above 32 bits");
>>>   +    object_class_property_add_bool(oc, "highmem-compact",
>>> +                                   virt_get_highmem_compact,
>>> +                                   virt_set_highmem_compact);
>>> +    object_class_property_set_description(oc, "highmem-compact",
>>> +                                          "Set on/off to
>>> enable/disable compact "
>>> +                                          "space for high memory
>>> regions");
>>> +
>>>       object_class_property_add_str(oc, "gic-version",
>>> virt_get_gic_version,
>>>                                     virt_set_gic_version);
>>>       object_class_property_set_description(oc, "gic-version",
>>> @@ -3050,6 +3078,7 @@ static void virt_instance_init(Object *obj)
>>>         /* High memory is enabled by default */
>>>       vms->highmem = true;
>>> +    vms->highmem_compact = !vmc->no_highmem_compact;
>>>       vms->gic_version = VIRT_GIC_VERSION_NOSEL;
>>>         vms->highmem_ecam = !vmc->no_highmem_ecam;
>>> @@ -3119,8 +3148,12 @@ DEFINE_VIRT_MACHINE_AS_LATEST(7, 2)
>>>     static void virt_machine_7_1_options(MachineClass *mc)
>>>   {
>>> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
>>> +
>>>       virt_machine_7_2_options(mc);
>>>       compat_props_add(mc->compat_props, hw_compat_7_1,
>>> hw_compat_7_1_len);
>>> +    /* Compact space for high memory regions was introduced with
>>> 7.2 */
>>> +    vmc->no_highmem_compact = true;
>>>   }
>>>   DEFINE_VIRT_MACHINE(7, 1)
>>>   diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>>> index 6ec479ca2b..c7dd59d7f1 100644
>>> --- a/include/hw/arm/virt.h
>>> +++ b/include/hw/arm/virt.h
>>> @@ -125,6 +125,7 @@ struct VirtMachineClass {
>>>       bool no_pmu;
>>>       bool claim_edge_triggered_timers;
>>>       bool smbios_old_sys_ver;
>>> +    bool no_highmem_compact;
>>>       bool no_highmem_ecam;
>>>       bool no_ged;   /* Machines < 4.2 have no support for ACPI GED
>>> device */
>>>       bool kvm_no_adjvtime;
>>> @@ -144,6 +145,7 @@ struct VirtMachineState {
>>>       PFlashCFI01 *flash[2];
>>>       bool secure;
>>>       bool highmem;
>>> +    bool highmem_compact;
>>>       bool highmem_ecam;
>>>       bool highmem_mmio;
>>>       bool highmem_redists;
>>
>
> Thanks,
> Gavin
>