| Message ID | 20220921231349.274049-6-gshan@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/virt: Improve address assignment for high memory regions | expand |
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
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
On Thu, Sep 29 2022, Gavin Shan <gshan@redhat.com> 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. FWIW, I initially misread 'highmem_compact' as 'highmem_compat' (and had to re-read because I got confused). At least to me, 'compact_highmem' has less chance of being parsed incorrectly :) (although that is probably a personal thing.) > >>> 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. Rather explain it in this file here, maybe? I'd prefer to be able to find out what 'compact' means without digging through the commit log.
Hi Cornelia, On 9/29/22 8:27 PM, Cornelia Huck wrote: > On Thu, Sep 29 2022, Gavin Shan <gshan@redhat.com> 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. > > FWIW, I initially misread 'highmem_compact' as 'highmem_compat' (and had > to re-read because I got confused). At least to me, 'compact_highmem' > has less chance of being parsed incorrectly :) (although that is > probably a personal thing.) > Ok. 'compact-highmem' is also fine to me. I'm really bad at naming :) >> >>>> 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. > > Rather explain it in this file here, maybe? I'd prefer to be able to > find out what 'compact' means without digging through the commit log. > Ok, lets do as Eric suggested. There are existing comments about @extended_memmap[] in hw/arm/virt.c. We need to explain the legacy/modern laoyout and 'compact-highmem' property there. Thanks, Gavin
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 <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. 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 >
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
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`` + 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) { + 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;
After the improvement to high memory region address assignment is applied, the memory layout is changed. For example, VIRT_HIGH_PCIE_MMIO 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 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(+)