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

[Gavin Shan]

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