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

[Gavin Shan <gshan@redhat.com>]

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.

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

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