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

[Gavin Shan]

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