On 26-09-2024 09:41 pm, Gustavo Romero wrote:
Hi Cornelia and Ganapatrao,
On 9/25/24 14:54, Cornelia Huck wrote:
On Fri, Sep 20 2024, Ganapatrao Kulkarni
<gankulka...@os.amperecomputing.com> wrote:
Mostly nit-picking below, otherwise LGTM.
Extend the 'mte' property for the virt machine to cover KVM as
well. For KVM, we don't allocate tag memory, but instead enable
the capability.
If MTE has been enabled, we need to disable migration, as we do not
yet have a way to migrate the tags as well. Therefore, MTE will stay
off with KVM unless requested explicitly.
This patch is rework of commit b320e21c48ce64853904bea6631c0158cc2ef227
which broke TCG since it made the TCG -cpu max
report the presence of MTE to the guest even if the board hadn't
enabled MTE by wiring up the tag RAM. This meant that if the guest
then tried to use MTE QEMU would segfault accessing the
non-existent tag RAM.
I think the more canonical way to express this would be
[$AUTHOR: reworked original patch by doing X to avoid problem Y]
Signed-off-by: Cornelia Huck <coh...@redhat.com>
Signed-off-by: Ganapatrao Kulkarni <gankulka...@os.amperecomputing.com>
Also, the S-o-b chain is a bit confusing that way, because you are
listed as author of the patch, but I'm in the chain in front of you -- I
think I should still be listed as the author?
---
Changes since V2:
Updated with review comments.
Changes since V1:
Added code to enable MTE before reading register
id_aa64pfr1 (unmasked MTE bits).
This patch is boot tested on ARM64 with KVM and on X86 with TCG for
mte=on
and default case(i.e, no mte).
hw/arm/virt.c | 72 ++++++++++++++++++++++++++------------------
target/arm/cpu.c | 11 +++++--
target/arm/cpu.h | 2 ++
target/arm/kvm.c | 57 +++++++++++++++++++++++++++++++++++
target/arm/kvm_arm.h | 19 ++++++++++++
5 files changed, 129 insertions(+), 32 deletions(-)
(...)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 19191c2391..8a2fc471ce 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2390,14 +2390,21 @@ static void arm_cpu_realizefn(DeviceState
*dev, Error **errp)
#ifndef CONFIG_USER_ONLY
/*
- * If we do not have tag-memory provided by the machine,
+ * If we do not have tag-memory provided by the TCG,
Maybe
"If we run with TCG and do not have tag-memory provided by the machine"
?
Yep, I agree, this is better.
Sure, will update the comment.
* reduce MTE support to instructions enabled at EL0.
* This matches Cortex-A710 BROADCASTMTE input being LOW.
*/
- if (cpu->tag_memory == NULL) {
+ if (tcg_enabled() && cpu->tag_memory == NULL) {
cpu->isar.id_aa64pfr1 =
FIELD_DP64(cpu->isar.id_aa64pfr1, ID_AA64PFR1, MTE,
1);
}
+
+ /*
+ * Clear MTE bits, if not enabled in KVM mode.
Maybe add "This matches the MTE bits being masked by KVM in that case."?
Clearing the MTE bits is also necessary when MTE is supported by the
host (and so KVM can enable the MTE capability - so won't mask the MTE
bits, but the user didn't want MTE enabled in the guest (mte=on no given
or explicitly set to =off), so this comment is not always true?
How about something like:
"If MTE is supported by the host but could not be enabled on KVM mode or
MTE should not be enabled on the guest (e.i. mte=off), clear guest's MTE
bits."
Ok thanks.
I do assume MTE is supported by the host (i.e. MTE bits >= 2 in the host)
because otherwise condition "if (cpu_isar_feature(aa64_mte, cpu)) { ...
}" is not
taken; and at this point cpu->isar->id_aa64pfr1 is set from the host's
bits via
kvm_arm_set_cpu_features_from_host() and kvm_arm_get_host_cpu_features ().
+ */
+ if (kvm_enabled() && !cpu->kvm_mte) {
+ FIELD_DP64(cpu->isar.id_aa64pfr1, ID_AA64PFR1, MTE, 0);
+ }
#endif
}
(...)
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 849e2e21b3..af7a98517d 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -39,6 +39,7 @@
#include "hw/acpi/acpi.h"
#include "hw/acpi/ghes.h"
#include "target/arm/gtimer.h"
+#include "migration/blocker.h"
const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
KVM_CAP_LAST_INFO
@@ -119,6 +120,20 @@ bool kvm_arm_create_scratch_host_vcpu(const
uint32_t *cpus_to_try,
if (vmfd < 0) {
goto err;
}
+
+ /*
+ * MTE capability must be enabled by the VMM before creating
+ * any VCPUs. The MTE bits of the ID_AA64PFR1 register are masked
+ * if MTE is not enabled, allowing them to be probed correctly.
This reads a bit confusing. Maybe
"The MTE capability must be enabled by the VMM before creating any VCPUs
in order to allow the MTE bits of the ID_AA64PFR1 register to be probed
correctly, as they are masked if MTE is not enabled."
I agree.
Ok, thanks.
+ */
+ if (kvm_arm_mte_supported()) {
+ KVMState kvm_state;
+
+ kvm_state.fd = kvmfd;
+ kvm_state.vmfd = vmfd;
+ kvm_vm_enable_cap(&kvm_state, KVM_CAP_ARM_MTE, 0);
+ }
+
cpufd = ioctl(vmfd, KVM_CREATE_VCPU, 0);
if (cpufd < 0) {
goto err;
(...)
Comments aside:
Reviewed-by: Gustavo Romero <gustavo.rom...@linaro.org>
Thanks Gustavo.
--
Thanks,
Ganapat/GK
