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

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

Hi Gavin,
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.
> 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
> 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: '
> +
>  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.

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

Eric