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