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