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

[Gavin Shan <gshan@redhat.com>]

Hi Eric,

On 10/3/22 4:49 PM, Eric Auger wrote:
> On 9/29/22 01:49, Gavin Shan 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.
> 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.

Ok. I agree 'compact-highmem' is better. The backup variable name will
be still 'highmem_compact', which is consistent with the existing ones.

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

Yeah, what I need is define 'vms->highmem_compact' in PATCH[v3 4/5],
whose value is false. I also need to update @base and @vms->highest_gpa
on !vms->highmem_compact' in PATCH[v3 4/5].

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