[RFC PATCH v2 0/5] target/arm: Add MTE support to KVM guest

[RFC PATCH v2 0/5] target/arm: Add MTE support to KVM guest

[Haibo Xu]

This version is rebased against kernel patches v10, and only header
files are updated since v1[1].

This series add support for MTE(Memory Tagging Extension)[2]
in KVM guest. It's based on Steven Price's kernel KVM patches
v10[3], and has been tested to ensure that test case[4] can be
passed in a KVM guest. Basic pre-copy migration test also passed
between two MTE enabled kvm guest. 

This is a RFC patch series becuase:
(1) Need to add some arm MTE specific codes to the ram migration
    loop. There may be better way to do that in a more abstract way.
(2) Only pre-copy migration is supported and tested currently,
    post-copy as well as compress/zero page migration are still WIP.
    
All kinds of feedbacks are very welcomed, especially for the migration
support. 

Note:
(1) To support MTE migration, tags for one page are appended to  
    the page data during ram save iteration which make it easier
    to sync the page data and tags.

[1] https://lore.kernel.org/qemu-devel/cover.1612747873.git.haibo.xu@linaro.org/
[2] https://community.arm.com/developer/ip-products/processors/b/
    processors-ip-blog/posts/enhancing-memory-safety
[3] https://patchew.org/QEMU/20210312151902.17853-1-steven.price@arm.com/
[4] https://elixir.bootlin.com/linux/latest/source/Documentation/
    arm64/memory-tagging-extension.rst

Haibo Xu (5):
  Update Linux headers with new MTE support
  Add basic MTE support to KVM guest
  Add APIs to get/set MTE tags
  Add migration support for KVM guest with MTE
  Enable the MTE support for KVM guest

 hw/arm/virt.c             | 44 +++++++++++++-------
 include/hw/arm/virt.h     |  2 +
 include/migration/misc.h  |  1 +
 linux-headers/linux/kvm.h | 16 ++++++++
 migration/ram.c           | 86 ++++++++++++++++++++++++++++++++++++++-
 target/arm/cpu.c          |  2 +-
 target/arm/kvm.c          |  9 ++++
 target/arm/kvm64.c        | 31 ++++++++++++++
 target/arm/kvm_arm.h      |  2 +
 9 files changed, 177 insertions(+), 16 deletions(-)

-- 
2.17.1

[RFC PATCH v2 1/5] Update Linux headers with new MTE support

[Haibo Xu]

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 linux-headers/linux/kvm.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 020b62a619..941743b3a7 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1056,6 +1056,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_ENFORCE_PV_FEATURE_CPUID 190
 #define KVM_CAP_SYS_HYPERV_CPUID 191
 #define KVM_CAP_DIRTY_LOG_RING 192
+#define KVM_CAP_ARM_MTE 195
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -1241,6 +1242,20 @@ struct kvm_arm_device_addr {
 	__u64 addr;
 };
 
+#define KVM_ARM_TAGS_TO_GUEST           0
+#define KVM_ARM_TAGS_FROM_GUEST         1
+
+struct kvm_arm_copy_mte_tags {
+	__u64 guest_ipa;
+	__u64 length;
+	union {
+		void *addr;
+		__u64 padding;
+	};
+	__u64 flags;
+	__u64 reserved[2];
+};
+
 /*
  * Device control API, available with KVM_CAP_DEVICE_CTRL
  */
@@ -1396,6 +1411,7 @@ struct kvm_s390_ucas_mapping {
 /* Available with KVM_CAP_PMU_EVENT_FILTER */
 #define KVM_SET_PMU_EVENT_FILTER  _IOW(KVMIO,  0xb2, struct kvm_pmu_event_filter)
 #define KVM_PPC_SVM_OFF		  _IO(KVMIO,  0xb3)
+#define KVM_ARM_MTE_COPY_TAGS	  _IOR(KVMIO,  0xb4, struct kvm_arm_copy_mte_tags)
 
 /* ioctl for vm fd */
 #define KVM_CREATE_DEVICE	  _IOWR(KVMIO,  0xe0, struct kvm_create_device)
-- 
2.17.1

[RFC PATCH v2 2/5] Add basic MTE support to KVM guest

[Haibo Xu]

Enable the virt machine feature "mte" to work with
KVM guest. This feature is still hiden from the user
in this patch, and will be available in a later patch.

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 hw/arm/virt.c      | 22 +++++++++++-----------
 target/arm/cpu.c   |  2 +-
 target/arm/kvm.c   |  9 +++++++++
 target/arm/kvm64.c |  7 +++++++
 4 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index aa2bbd14e0..76658b93a3 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1988,18 +1988,18 @@ static void machvirt_init(MachineState *machine)
         }
 
         if (vms->mte) {
+            /*
+             * The property exists only if MemTag is supported.
+             * If it is, we must allocate the ram to back that up.
+             */
+            if (!object_property_find(cpuobj, "tag-memory")) {
+                error_report("MTE requested, but not supported "
+                             "by the guest CPU");
+                exit(1);
+            }
+
             /* Create the memory region only once, but link to all cpus. */
-            if (!tag_sysmem) {
-                /*
-                 * The property exists only if MemTag is supported.
-                 * If it is, we must allocate the ram to back that up.
-                 */
-                if (!object_property_find(cpuobj, "tag-memory")) {
-                    error_report("MTE requested, but not supported "
-                                 "by the guest CPU");
-                    exit(1);
-                }
-
+            if (!tag_sysmem && !kvm_enabled()) {
                 tag_sysmem = g_new(MemoryRegion, 1);
                 memory_region_init(tag_sysmem, OBJECT(machine),
                                    "tag-memory", UINT64_MAX / 32);
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index ae04884408..47bf817b61 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1847,7 +1847,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
                                cpu->secure_memory);
     }
 
-    if (cpu->tag_memory != NULL) {
+    if (cpu->tag_memory != NULL && !kvm_enabled()) {
         cpu_address_space_init(cs, ARMASIdx_TagNS, "cpu-tag-memory",
                                cpu->tag_memory);
         if (has_secure) {
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index d8381ba224..3403e621ac 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -32,6 +32,7 @@
 #include "hw/boards.h"
 #include "hw/irq.h"
 #include "qemu/log.h"
+#include "hw/arm/virt.h"
 
 const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
     KVM_CAP_LAST_INFO
@@ -274,6 +275,14 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
         }
     }
 
+    if (kvm_check_extension(s, KVM_CAP_ARM_MTE) &&
+        object_dynamic_cast(OBJECT(ms), TYPE_VIRT_MACHINE) &&
+        VIRT_MACHINE(ms)->mte) {
+            if (kvm_vm_enable_cap(s, KVM_CAP_ARM_MTE, 0)) {
+                error_report("Failed to enable KVM_CAP_ARM_MTE cap");
+            }
+    }
+
     return ret;
 }
 
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index dff85f6db9..73a191f8e1 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -500,6 +500,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
      */
     int fdarray[3];
     bool sve_supported;
+    bool mte_supported;
     uint64_t features = 0;
     uint64_t t;
     int err;
@@ -646,6 +647,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
     }
 
     sve_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_SVE) > 0;
+    mte_supported = ioctl(fdarray[0], KVM_CHECK_EXTENSION, KVM_CAP_ARM_MTE) > 0;
 
     kvm_arm_destroy_scratch_host_vcpu(fdarray);
 
@@ -659,6 +661,11 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
         t = FIELD_DP64(t, ID_AA64PFR0, SVE, 1);
         ahcf->isar.id_aa64pfr0 = t;
     }
+    if (mte_supported) {
+        t = ahcf->isar.id_aa64pfr1;
+        t = FIELD_DP64(t, ID_AA64PFR1, MTE, 2);
+        ahcf->isar.id_aa64pfr1 = t;
+    }
 
     /*
      * We can assume any KVM supporting CPU is at least a v8
-- 
2.17.1

[RFC PATCH v2 3/5] Add APIs to get/set MTE tags

[Haibo Xu]

MTE spec provide instructions to retrieve the memory tags:
(1) LDG, at 16 bytes granularity, and available in both user
    and kernel space;
(2) LDGM, at 256 bytes granularity in maximum, and only
    available in kernel space

To improve the performance, KVM has exposed the LDGM capability
to user space by providing a new APIs. This patch is just a
wrapper for the KVM APIs.

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 target/arm/kvm64.c   | 24 ++++++++++++++++++++++++
 target/arm/kvm_arm.h |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 73a191f8e1..3157025316 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -1606,3 +1606,27 @@ bool kvm_arm_verify_ext_dabt_pending(CPUState *cs)
     }
     return false;
 }
+
+int kvm_arm_mte_get_tags(uint64_t ipa, uint64_t len, uint8_t *buf)
+{
+    struct kvm_arm_copy_mte_tags args = {
+        .guest_ipa = ipa,
+        .length = len,
+        .addr = buf,
+        .flags = KVM_ARM_TAGS_FROM_GUEST,
+    };
+
+    return kvm_vm_ioctl(kvm_state, KVM_ARM_MTE_COPY_TAGS, &args);
+}
+
+int kvm_arm_mte_set_tags(uint64_t ipa, uint64_t len, uint8_t *buf)
+{
+    struct kvm_arm_copy_mte_tags args = {
+        .guest_ipa = ipa,
+        .length = len,
+        .addr = buf,
+        .flags = KVM_ARM_TAGS_TO_GUEST,
+    };
+
+    return kvm_vm_ioctl(kvm_state, KVM_ARM_MTE_COPY_TAGS, &args);
+}
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 34f8daa377..bbb833d6c6 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -360,6 +360,8 @@ int kvm_arm_vgic_probe(void);
 
 void kvm_arm_pmu_set_irq(CPUState *cs, int irq);
 void kvm_arm_pmu_init(CPUState *cs);
+int kvm_arm_mte_get_tags(uint64_t ipa, uint64_t len, uint8_t *buf);
+int kvm_arm_mte_set_tags(uint64_t ipa, uint64_t len, uint8_t *buf);
 
 /**
  * kvm_arm_pvtime_init:
-- 
2.17.1

[RFC PATCH v2 4/5] Add migration support for KVM guest with MTE

[Haibo Xu]

To make it easier to keep the page tags sync with
the page data, tags for one page are appended to
the data during ram save iteration.

This patch only add the pre-copy migration support.
Post-copy and compress as well as zero page saving
are not supported yet.

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 include/hw/arm/virt.h    |  2 +
 include/migration/misc.h |  1 +
 migration/ram.c          | 86 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 921416f918..8b28cde8bf 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -166,6 +166,8 @@ struct VirtMachineState {
     PCIBus *bus;
     char *oem_id;
     char *oem_table_id;
+    /* migrate memory tags */
+    NotifierWithReturn precopy_notifier;
 };
 
 #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
diff --git a/include/migration/misc.h b/include/migration/misc.h
index bccc1b6b44..005133f471 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -38,6 +38,7 @@ void precopy_add_notifier(NotifierWithReturn *n);
 void precopy_remove_notifier(NotifierWithReturn *n);
 int precopy_notify(PrecopyNotifyReason reason, Error **errp);
 void precopy_enable_free_page_optimization(void);
+void precopy_enable_metadata_migration(void);
 
 void ram_mig_init(void);
 void qemu_guest_free_page_hint(void *addr, size_t len);
diff --git a/migration/ram.c b/migration/ram.c
index 72143da0ac..e67b798c3b 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -53,10 +53,12 @@
 #include "block.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/cpu-throttle.h"
+#include "sysemu/kvm.h"
 #include "savevm.h"
 #include "qemu/iov.h"
 #include "multifd.h"
 #include "sysemu/runstate.h"
+#include "kvm_arm.h"
 
 #if defined(__linux__)
 #include "qemu/userfaultfd.h"
@@ -80,6 +82,9 @@
 #define RAM_SAVE_FLAG_XBZRLE   0x40
 /* 0x80 is reserved in migration.h start with 0x100 next */
 #define RAM_SAVE_FLAG_COMPRESS_PAGE    0x100
+#define RAM_SAVE_FLAG_MTE              0x200
+
+#define MTE_GRANULE_SIZE   (16)
 
 static inline bool is_zero_range(uint8_t *p, uint64_t size)
 {
@@ -317,6 +322,8 @@ struct RAMState {
     bool ram_bulk_stage;
     /* The free page optimization is enabled */
     bool fpo_enabled;
+    /* The RAM meta data(e.t memory tag) is enabled */
+    bool metadata_enabled;
     /* How many times we have dirty too many pages */
     int dirty_rate_high_cnt;
     /* these variables are used for bitmap sync */
@@ -394,6 +401,15 @@ void precopy_enable_free_page_optimization(void)
     ram_state->fpo_enabled = true;
 }
 
+void precopy_enable_metadata_migration(void)
+{
+    if (!ram_state) {
+        return;
+    }
+
+    ram_state->metadata_enabled = true;
+}
+
 uint64_t ram_bytes_remaining(void)
 {
     return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) :
@@ -1134,6 +1150,61 @@ static bool control_save_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
     return true;
 }
 
+static int save_normal_page_mte_tags(QEMUFile *f, uint8_t *addr)
+{
+    uint8_t *tag_buf = NULL;
+    uint64_t ipa;
+    int size = TARGET_PAGE_SIZE / MTE_GRANULE_SIZE;
+
+    if (kvm_physical_memory_addr_from_host(kvm_state, addr, &ipa)) {
+        /* Buffer for the page tags(one byte per tag) */
+        tag_buf = g_try_malloc0(size);
+        if (!tag_buf) {
+            error_report("%s: Error allocating MTE tag_buf", __func__);
+            return 0;
+        }
+
+        if (kvm_arm_mte_get_tags(ipa, TARGET_PAGE_SIZE, tag_buf) < 0) {
+            error_report("%s: Can't get MTE tags from guest", __func__);
+            g_free(tag_buf);
+            return 0;
+        }
+
+        qemu_put_buffer(f, tag_buf, size);
+
+        g_free(tag_buf);
+
+        return size;
+    }
+
+    return 0;
+}
+
+static void load_normal_page_mte_tags(QEMUFile *f, uint8_t *addr)
+{
+    uint8_t *tag_buf = NULL;
+    uint64_t ipa;
+    int size = TARGET_PAGE_SIZE / MTE_GRANULE_SIZE;
+
+    if (kvm_physical_memory_addr_from_host(kvm_state, addr, &ipa)) {
+        /* Buffer for the page tags(one byte per tag) */
+        tag_buf = g_try_malloc0(size);
+        if (!tag_buf) {
+            error_report("%s: Error allocating MTE tag_buf", __func__);
+            return;
+        }
+
+        qemu_get_buffer(f, tag_buf, size);
+        if (kvm_arm_mte_set_tags(ipa, TARGET_PAGE_SIZE, tag_buf) < 0) {
+            error_report("%s: Can't set MTE tags to guest", __func__);
+        }
+
+        g_free(tag_buf);
+    }
+
+    return;
+}
+
 /*
  * directly send the page to the stream
  *
@@ -1148,6 +1219,10 @@ static bool control_save_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
 static int save_normal_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
                             uint8_t *buf, bool async)
 {
+    if (rs->metadata_enabled) {
+        offset |= RAM_SAVE_FLAG_MTE;
+    }
+
     ram_counters.transferred += save_page_header(rs, rs->f, block,
                                                  offset | RAM_SAVE_FLAG_PAGE);
     if (async) {
@@ -1159,6 +1234,11 @@ static int save_normal_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
     }
     ram_counters.transferred += TARGET_PAGE_SIZE;
     ram_counters.normal++;
+
+    if (rs->metadata_enabled) {
+        ram_counters.transferred += save_normal_page_mte_tags(rs->f, buf);
+    }
+
     return 1;
 }
 
@@ -2189,6 +2269,7 @@ static void ram_state_reset(RAMState *rs)
     rs->last_version = ram_list.version;
     rs->ram_bulk_stage = true;
     rs->fpo_enabled = false;
+    rs->metadata_enabled = false;
 }
 
 #define MAX_WAIT 50 /* ms, half buffered_file limit */
@@ -3779,7 +3860,7 @@ static int ram_load_precopy(QEMUFile *f)
             trace_ram_load_loop(block->idstr, (uint64_t)addr, flags, host);
         }
 
-        switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
+        switch (flags & ~(RAM_SAVE_FLAG_CONTINUE | RAM_SAVE_FLAG_MTE)) {
         case RAM_SAVE_FLAG_MEM_SIZE:
             /* Synchronize RAM block list */
             total_ram_bytes = addr;
@@ -3849,6 +3930,9 @@ static int ram_load_precopy(QEMUFile *f)
 
         case RAM_SAVE_FLAG_PAGE:
             qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
+            if (flags & RAM_SAVE_FLAG_MTE) {
+                load_normal_page_mte_tags(f, host);
+            }
             break;
 
         case RAM_SAVE_FLAG_COMPRESS_PAGE:
-- 
2.17.1

[RFC PATCH v2 5/5] Enable the MTE support for KVM guest

[Haibo Xu]

Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
---
 hw/arm/virt.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 76658b93a3..36cfdb29e9 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -79,6 +79,7 @@
 #include "hw/virtio/virtio-iommu.h"
 #include "hw/char/pl011.h"
 #include "qemu/guest-random.h"
+#include "migration/misc.h"
 
 #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
     static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
@@ -828,6 +829,21 @@ static void virt_powerdown_req(Notifier *n, void *opaque)
     }
 }
 
+static int virt_precopy_notify(NotifierWithReturn *n, void *data)
+{
+    PrecopyNotifyData *pnd = data;
+
+    switch (pnd->reason) {
+    case PRECOPY_NOTIFY_SETUP:
+        precopy_enable_metadata_migration();
+        break;
+    default:
+        break;
+    }
+
+    return 0;
+}
+
 static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
                              uint32_t phandle)
 {
@@ -1912,9 +1928,9 @@ static void machvirt_init(MachineState *machine)
     }
 
     if (vms->mte && kvm_enabled()) {
-        error_report("mach-virt: KVM does not support providing "
-                     "MTE to the guest CPU");
-        exit(1);
+        /* connect migration precopy request */
+        vms->precopy_notifier.notify = virt_precopy_notify;
+        precopy_add_notifier(&vms->precopy_notifier);
     }
 
     create_fdt(vms);
-- 
2.17.1

Re: [RFC PATCH v2 4/5] Add migration support for KVM guest with MTE

[Dr. David Alan Gilbert]

* Haibo Xu (haibo.xu@linaro.org) wrote:
> To make it easier to keep the page tags sync with
> the page data, tags for one page are appended to
> the data during ram save iteration.
> 
> This patch only add the pre-copy migration support.
> Post-copy and compress as well as zero page saving
> are not supported yet.
> 
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>

My guess is that this doesn't work with a lot of other options; e.g.
postcopy and probably compression and a bunch of other things.
Postcopy I can see you'll need some interesting kernel changes for -
you'd need to be able to atomically place a  page with it's tag data.

You probably need to add stuff to migrate_caps_check  to disable
features that you don't support.

> ---
>  include/hw/arm/virt.h    |  2 +
>  include/migration/misc.h |  1 +
>  migration/ram.c          | 86 +++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 88 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 921416f918..8b28cde8bf 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -166,6 +166,8 @@ struct VirtMachineState {
>      PCIBus *bus;
>      char *oem_id;
>      char *oem_table_id;
> +    /* migrate memory tags */
> +    NotifierWithReturn precopy_notifier;
>  };
>  
>  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index bccc1b6b44..005133f471 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -38,6 +38,7 @@ void precopy_add_notifier(NotifierWithReturn *n);
>  void precopy_remove_notifier(NotifierWithReturn *n);
>  int precopy_notify(PrecopyNotifyReason reason, Error **errp);
>  void precopy_enable_free_page_optimization(void);
> +void precopy_enable_metadata_migration(void);
>  
>  void ram_mig_init(void);
>  void qemu_guest_free_page_hint(void *addr, size_t len);
> diff --git a/migration/ram.c b/migration/ram.c
> index 72143da0ac..e67b798c3b 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -53,10 +53,12 @@
>  #include "block.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/cpu-throttle.h"
> +#include "sysemu/kvm.h"
>  #include "savevm.h"
>  #include "qemu/iov.h"
>  #include "multifd.h"
>  #include "sysemu/runstate.h"
> +#include "kvm_arm.h"
>  
>  #if defined(__linux__)
>  #include "qemu/userfaultfd.h"
> @@ -80,6 +82,9 @@
>  #define RAM_SAVE_FLAG_XBZRLE   0x40
>  /* 0x80 is reserved in migration.h start with 0x100 next */
>  #define RAM_SAVE_FLAG_COMPRESS_PAGE    0x100
> +#define RAM_SAVE_FLAG_MTE              0x200

I think that's using the last free bit in the flags field, which is a
bit painful.

> +#define MTE_GRANULE_SIZE   (16)
>  
>  static inline bool is_zero_range(uint8_t *p, uint64_t size)
>  {
> @@ -317,6 +322,8 @@ struct RAMState {
>      bool ram_bulk_stage;
>      /* The free page optimization is enabled */
>      bool fpo_enabled;
> +    /* The RAM meta data(e.t memory tag) is enabled */
> +    bool metadata_enabled;
>      /* How many times we have dirty too many pages */
>      int dirty_rate_high_cnt;
>      /* these variables are used for bitmap sync */
> @@ -394,6 +401,15 @@ void precopy_enable_free_page_optimization(void)
>      ram_state->fpo_enabled = true;
>  }
>  
> +void precopy_enable_metadata_migration(void)
> +{
> +    if (!ram_state) {
> +        return;
> +    }
> +
> +    ram_state->metadata_enabled = true;
> +}
> +
>  uint64_t ram_bytes_remaining(void)
>  {
>      return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) :
> @@ -1134,6 +1150,61 @@ static bool control_save_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
>      return true;
>  }
>  
> +static int save_normal_page_mte_tags(QEMUFile *f, uint8_t *addr)
> +{
> +    uint8_t *tag_buf = NULL;
> +    uint64_t ipa;
> +    int size = TARGET_PAGE_SIZE / MTE_GRANULE_SIZE;

This function needs to be mostly somewhere aarch specific; or somehow
made more generic.
We shouldn't have RAM_SAVE_FLAG_MTE as well - we should be something
like RAM_SAVE_FLAG_ARCH_METADATA and that way other architectures with
something else glued onto pages can do something similar.
Try and keep migration/ architecture independent.

> +    if (kvm_physical_memory_addr_from_host(kvm_state, addr, &ipa)) {
> +        /* Buffer for the page tags(one byte per tag) */
> +        tag_buf = g_try_malloc0(size);

It feels like you want to allocate tag_buf in setup and free it at the
end rather than doing this in every page.

> +        if (!tag_buf) {
> +            error_report("%s: Error allocating MTE tag_buf", __func__);
> +            return 0;
> +        }
> +
> +        if (kvm_arm_mte_get_tags(ipa, TARGET_PAGE_SIZE, tag_buf) < 0) {
> +            error_report("%s: Can't get MTE tags from guest", __func__);

For any error like this you probably want to say the addresses to make
it easier to debug when it fails.

> +            g_free(tag_buf);
> +            return 0;
> +        }
> +
> +        qemu_put_buffer(f, tag_buf, size);
> +
> +        g_free(tag_buf);
> +
> +        return size;
> +    }
> +
> +    return 0;
> +}
> +
> +static void load_normal_page_mte_tags(QEMUFile *f, uint8_t *addr)
> +{
> +    uint8_t *tag_buf = NULL;
> +    uint64_t ipa;
> +    int size = TARGET_PAGE_SIZE / MTE_GRANULE_SIZE;
> +
> +    if (kvm_physical_memory_addr_from_host(kvm_state, addr, &ipa)) {
> +        /* Buffer for the page tags(one byte per tag) */
> +        tag_buf = g_try_malloc0(size);
> +        if (!tag_buf) {
> +            error_report("%s: Error allocating MTE tag_buf", __func__);
> +            return;
> +        }
> +
> +        qemu_get_buffer(f, tag_buf, size);
> +        if (kvm_arm_mte_set_tags(ipa, TARGET_PAGE_SIZE, tag_buf) < 0) {

what protections are there here to stop you setting the mte on something
useful, like part of the host kernel or qemu?

> +            error_report("%s: Can't set MTE tags to guest", __func__);
> +        }
> +
> +        g_free(tag_buf);
> +    }
> +
> +    return;
> +}
> +
>  /*
>   * directly send the page to the stream
>   *
> @@ -1148,6 +1219,10 @@ static bool control_save_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
>  static int save_normal_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
>                              uint8_t *buf, bool async)
>  {
> +    if (rs->metadata_enabled) {
> +        offset |= RAM_SAVE_FLAG_MTE;
> +    }
> +
>      ram_counters.transferred += save_page_header(rs, rs->f, block,
>                                                   offset | RAM_SAVE_FLAG_PAGE);
>      if (async) {
> @@ -1159,6 +1234,11 @@ static int save_normal_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
>      }
>      ram_counters.transferred += TARGET_PAGE_SIZE;
>      ram_counters.normal++;
> +
> +    if (rs->metadata_enabled) {
> +        ram_counters.transferred += save_normal_page_mte_tags(rs->f, buf);
> +    }
> +
>      return 1;
>  }
>  
> @@ -2189,6 +2269,7 @@ static void ram_state_reset(RAMState *rs)
>      rs->last_version = ram_list.version;
>      rs->ram_bulk_stage = true;
>      rs->fpo_enabled = false;
> +    rs->metadata_enabled = false;
>  }
>  
>  #define MAX_WAIT 50 /* ms, half buffered_file limit */
> @@ -3779,7 +3860,7 @@ static int ram_load_precopy(QEMUFile *f)
>              trace_ram_load_loop(block->idstr, (uint64_t)addr, flags, host);
>          }
>  
> -        switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
> +        switch (flags & ~(RAM_SAVE_FLAG_CONTINUE | RAM_SAVE_FLAG_MTE)) {
>          case RAM_SAVE_FLAG_MEM_SIZE:
>              /* Synchronize RAM block list */
>              total_ram_bytes = addr;
> @@ -3849,6 +3930,9 @@ static int ram_load_precopy(QEMUFile *f)
>  
>          case RAM_SAVE_FLAG_PAGE:
>              qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> +            if (flags & RAM_SAVE_FLAG_MTE) {
> +                load_normal_page_mte_tags(f, host);
> +            }
>              break;
>  
>          case RAM_SAVE_FLAG_COMPRESS_PAGE:
> -- 
> 2.17.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [RFC PATCH v2 4/5] Add migration support for KVM guest with MTE

[Haibo Xu]

On Thu, 18 Mar 2021 at 04:11, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Haibo Xu (haibo.xu@linaro.org) wrote:
> > To make it easier to keep the page tags sync with
> > the page data, tags for one page are appended to
> > the data during ram save iteration.
> >
> > This patch only add the pre-copy migration support.
> > Post-copy and compress as well as zero page saving
> > are not supported yet.
> >
> > Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
>
> My guess is that this doesn't work with a lot of other options; e.g.
> postcopy and probably compression and a bunch of other things.
> Postcopy I can see you'll need some interesting kernel changes for -
> you'd need to be able to atomically place a  page with it's tag data.
>
> You probably need to add stuff to migrate_caps_check  to disable
> features that you don't support.
>

Hi David,

Thanks so much for the comments!

You are right, this RFC patch only supports pre-copy mode, no
postcopy, no compression.
As a RFC, here just want to finalize the tag migration process, that is:
1. let the tag go with the page data(the current choice) which may be
a little complex to put
    them into the current migration process.
2. migrate them separately which is easy to implement with the current
migration(treat the tags
    as device status), but it would be hard to keep the page data and
tag to sync with each other.
3. Any other ways?

Once the tag migration process is finalized, a new formal patch series
with postcopy as well as
compression should be reworked.

What's more, you mentioned that "some interesting kernel changes are
needed to atomically
place a  page with it's tag data". You mean a single kernel API to
store page data and tag in
the migration load process?

Regards,
Haibo

> > ---
> >  include/hw/arm/virt.h    |  2 +
> >  include/migration/misc.h |  1 +
> >  migration/ram.c          | 86 +++++++++++++++++++++++++++++++++++++++-
> >  3 files changed, 88 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index 921416f918..8b28cde8bf 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -166,6 +166,8 @@ struct VirtMachineState {
> >      PCIBus *bus;
> >      char *oem_id;
> >      char *oem_table_id;
> > +    /* migrate memory tags */
> > +    NotifierWithReturn precopy_notifier;
> >  };
> >
> >  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
> > diff --git a/include/migration/misc.h b/include/migration/misc.h
> > index bccc1b6b44..005133f471 100644
> > --- a/include/migration/misc.h
> > +++ b/include/migration/misc.h
> > @@ -38,6 +38,7 @@ void precopy_add_notifier(NotifierWithReturn *n);
> >  void precopy_remove_notifier(NotifierWithReturn *n);
> >  int precopy_notify(PrecopyNotifyReason reason, Error **errp);
> >  void precopy_enable_free_page_optimization(void);
> > +void precopy_enable_metadata_migration(void);
> >
> >  void ram_mig_init(void);
> >  void qemu_guest_free_page_hint(void *addr, size_t len);
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 72143da0ac..e67b798c3b 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -53,10 +53,12 @@
> >  #include "block.h"
> >  #include "sysemu/sysemu.h"
> >  #include "sysemu/cpu-throttle.h"
> > +#include "sysemu/kvm.h"
> >  #include "savevm.h"
> >  #include "qemu/iov.h"
> >  #include "multifd.h"
> >  #include "sysemu/runstate.h"
> > +#include "kvm_arm.h"
> >
> >  #if defined(__linux__)
> >  #include "qemu/userfaultfd.h"
> > @@ -80,6 +82,9 @@
> >  #define RAM_SAVE_FLAG_XBZRLE   0x40
> >  /* 0x80 is reserved in migration.h start with 0x100 next */
> >  #define RAM_SAVE_FLAG_COMPRESS_PAGE    0x100
> > +#define RAM_SAVE_FLAG_MTE              0x200
>
> I think that's using the last free bit in the flags field, which is a
> bit painful.
>

Yes, only 0x80(reserved) and 0x200 are available.

> > +#define MTE_GRANULE_SIZE   (16)
> >
> >  static inline bool is_zero_range(uint8_t *p, uint64_t size)
> >  {
> > @@ -317,6 +322,8 @@ struct RAMState {
> >      bool ram_bulk_stage;
> >      /* The free page optimization is enabled */
> >      bool fpo_enabled;
> > +    /* The RAM meta data(e.t memory tag) is enabled */
> > +    bool metadata_enabled;
> >      /* How many times we have dirty too many pages */
> >      int dirty_rate_high_cnt;
> >      /* these variables are used for bitmap sync */
> > @@ -394,6 +401,15 @@ void precopy_enable_free_page_optimization(void)
> >      ram_state->fpo_enabled = true;
> >  }
> >
> > +void precopy_enable_metadata_migration(void)
> > +{
> > +    if (!ram_state) {
> > +        return;
> > +    }
> > +
> > +    ram_state->metadata_enabled = true;
> > +}
> > +
> >  uint64_t ram_bytes_remaining(void)
> >  {
> >      return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) :
> > @@ -1134,6 +1150,61 @@ static bool control_save_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
> >      return true;
> >  }
> >
> > +static int save_normal_page_mte_tags(QEMUFile *f, uint8_t *addr)
> > +{
> > +    uint8_t *tag_buf = NULL;
> > +    uint64_t ipa;
> > +    int size = TARGET_PAGE_SIZE / MTE_GRANULE_SIZE;
>
> This function needs to be mostly somewhere aarch specific; or somehow
> made more generic.
> We shouldn't have RAM_SAVE_FLAG_MTE as well - we should be something
> like RAM_SAVE_FLAG_ARCH_METADATA and that way other architectures with
> something else glued onto pages can do something similar.
> Try and keep migration/ architecture independent.

Yes, it should be arch independent and handled with a more abstract way.

>
> > +    if (kvm_physical_memory_addr_from_host(kvm_state, addr, &ipa)) {
> > +        /* Buffer for the page tags(one byte per tag) */
> > +        tag_buf = g_try_malloc0(size);
>
> It feels like you want to allocate tag_buf in setup and free it at the
> end rather than doing this in every page.

Yes, the tag buffer allocation could be moved to migration save/load
setup and cleanup function.

>
> > +        if (!tag_buf) {
> > +            error_report("%s: Error allocating MTE tag_buf", __func__);
> > +            return 0;
> > +        }
> > +
> > +        if (kvm_arm_mte_get_tags(ipa, TARGET_PAGE_SIZE, tag_buf) < 0) {
> > +            error_report("%s: Can't get MTE tags from guest", __func__);
>
> For any error like this you probably want to say the addresses to make
> it easier to debug when it fails.
>

Good suggestion! Will add the "address" info to the log.

> > +            g_free(tag_buf);
> > +            return 0;
> > +        }
> > +
> > +        qemu_put_buffer(f, tag_buf, size);
> > +
> > +        g_free(tag_buf);
> > +
> > +        return size;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static void load_normal_page_mte_tags(QEMUFile *f, uint8_t *addr)
> > +{
> > +    uint8_t *tag_buf = NULL;
> > +    uint64_t ipa;
> > +    int size = TARGET_PAGE_SIZE / MTE_GRANULE_SIZE;
> > +
> > +    if (kvm_physical_memory_addr_from_host(kvm_state, addr, &ipa)) {
> > +        /* Buffer for the page tags(one byte per tag) */
> > +        tag_buf = g_try_malloc0(size);
> > +        if (!tag_buf) {
> > +            error_report("%s: Error allocating MTE tag_buf", __func__);
> > +            return;
> > +        }
> > +
> > +        qemu_get_buffer(f, tag_buf, size);
> > +        if (kvm_arm_mte_set_tags(ipa, TARGET_PAGE_SIZE, tag_buf) < 0) {
>
> what protections are there here to stop you setting the mte on something
> useful, like part of the host kernel or qemu?

Kernel would do some parameter check(e.t ipa within a valid slot)
before setting the tag data.

>
> > +            error_report("%s: Can't set MTE tags to guest", __func__);
> > +        }
> > +
> > +        g_free(tag_buf);
> > +    }
> > +
> > +    return;
> > +}
> > +
> >  /*
> >   * directly send the page to the stream
> >   *
> > @@ -1148,6 +1219,10 @@ static bool control_save_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
> >  static int save_normal_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
> >                              uint8_t *buf, bool async)
> >  {
> > +    if (rs->metadata_enabled) {
> > +        offset |= RAM_SAVE_FLAG_MTE;
> > +    }
> > +
> >      ram_counters.transferred += save_page_header(rs, rs->f, block,
> >                                                   offset | RAM_SAVE_FLAG_PAGE);
> >      if (async) {
> > @@ -1159,6 +1234,11 @@ static int save_normal_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
> >      }
> >      ram_counters.transferred += TARGET_PAGE_SIZE;
> >      ram_counters.normal++;
> > +
> > +    if (rs->metadata_enabled) {
> > +        ram_counters.transferred += save_normal_page_mte_tags(rs->f, buf);
> > +    }
> > +
> >      return 1;
> >  }
> >
> > @@ -2189,6 +2269,7 @@ static void ram_state_reset(RAMState *rs)
> >      rs->last_version = ram_list.version;
> >      rs->ram_bulk_stage = true;
> >      rs->fpo_enabled = false;
> > +    rs->metadata_enabled = false;
> >  }
> >
> >  #define MAX_WAIT 50 /* ms, half buffered_file limit */
> > @@ -3779,7 +3860,7 @@ static int ram_load_precopy(QEMUFile *f)
> >              trace_ram_load_loop(block->idstr, (uint64_t)addr, flags, host);
> >          }
> >
> > -        switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
> > +        switch (flags & ~(RAM_SAVE_FLAG_CONTINUE | RAM_SAVE_FLAG_MTE)) {
> >          case RAM_SAVE_FLAG_MEM_SIZE:
> >              /* Synchronize RAM block list */
> >              total_ram_bytes = addr;
> > @@ -3849,6 +3930,9 @@ static int ram_load_precopy(QEMUFile *f)
> >
> >          case RAM_SAVE_FLAG_PAGE:
> >              qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> > +            if (flags & RAM_SAVE_FLAG_MTE) {
> > +                load_normal_page_mte_tags(f, host);
> > +            }
> >              break;
> >
> >          case RAM_SAVE_FLAG_COMPRESS_PAGE:
> > --
> > 2.17.1
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>

Re: [RFC PATCH v2 3/5] Add APIs to get/set MTE tags

[Juan Quintela]

Haibo Xu <haibo.xu@linaro.org> wrote:
> MTE spec provide instructions to retrieve the memory tags:
> (1) LDG, at 16 bytes granularity, and available in both user
>     and kernel space;
> (2) LDGM, at 256 bytes granularity in maximum, and only
>     available in kernel space
>
> To improve the performance, KVM has exposed the LDGM capability
> to user space by providing a new APIs. This patch is just a
> wrapper for the KVM APIs.
>
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  target/arm/kvm64.c   | 24 ++++++++++++++++++++++++
>  target/arm/kvm_arm.h |  2 ++
>  2 files changed, 26 insertions(+)
>
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 73a191f8e1..3157025316 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -1606,3 +1606,27 @@ bool kvm_arm_verify_ext_dabt_pending(CPUState *cs)
>      }
>      return false;
>  }
> +
> +int kvm_arm_mte_get_tags(uint64_t ipa, uint64_t len, uint8_t *buf)
> +{
> +    struct kvm_arm_copy_mte_tags args = {
> +        .guest_ipa = ipa,
> +        .length = len,
> +        .addr = buf,
> +        .flags = KVM_ARM_TAGS_FROM_GUEST,
> +    };
> +
> +    return kvm_vm_ioctl(kvm_state, KVM_ARM_MTE_COPY_TAGS, &args);

Just a question, how fast/slow are this calls?

My understanding is that we are making a kvm call for each page that we
want to migrate, right?

Each time that we want to send it.

Later, Juan.


> +}
> +
> +int kvm_arm_mte_set_tags(uint64_t ipa, uint64_t len, uint8_t *buf)
> +{
> +    struct kvm_arm_copy_mte_tags args = {
> +        .guest_ipa = ipa,
> +        .length = len,
> +        .addr = buf,
> +        .flags = KVM_ARM_TAGS_TO_GUEST,
> +    };
> +
> +    return kvm_vm_ioctl(kvm_state, KVM_ARM_MTE_COPY_TAGS, &args);
> +}
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index 34f8daa377..bbb833d6c6 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -360,6 +360,8 @@ int kvm_arm_vgic_probe(void);
>  
>  void kvm_arm_pmu_set_irq(CPUState *cs, int irq);
>  void kvm_arm_pmu_init(CPUState *cs);
> +int kvm_arm_mte_get_tags(uint64_t ipa, uint64_t len, uint8_t *buf);
> +int kvm_arm_mte_set_tags(uint64_t ipa, uint64_t len, uint8_t *buf);
>  
>  /**
>   * kvm_arm_pvtime_init:

Re: [RFC PATCH v2 5/5] Enable the MTE support for KVM guest

[Juan Quintela]

Haibo Xu <haibo.xu@linaro.org> wrote:
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> ---
>  hw/arm/virt.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 76658b93a3..36cfdb29e9 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -79,6 +79,7 @@
>  #include "hw/virtio/virtio-iommu.h"
>  #include "hw/char/pl011.h"
>  #include "qemu/guest-random.h"
> +#include "migration/misc.h"
>  
>  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>      static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> @@ -828,6 +829,21 @@ static void virt_powerdown_req(Notifier *n, void *opaque)
>      }
>  }
>  
> +static int virt_precopy_notify(NotifierWithReturn *n, void *data)
> +{
> +    PrecopyNotifyData *pnd = data;
> +
> +    switch (pnd->reason) {
> +    case PRECOPY_NOTIFY_SETUP:
> +        precopy_enable_metadata_migration();
> +        break;
> +    default:
> +        break;
> +    }
> +
> +    return 0;
> +}
> +
>  static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
>                               uint32_t phandle)
>  {
> @@ -1912,9 +1928,9 @@ static void machvirt_init(MachineState *machine)
>      }
>  
>      if (vms->mte && kvm_enabled()) {
> -        error_report("mach-virt: KVM does not support providing "
> -                     "MTE to the guest CPU");
> -        exit(1);
> +        /* connect migration precopy request */
> +        vms->precopy_notifier.notify = virt_precopy_notify;
> +        precopy_add_notifier(&vms->precopy_notifier);
>      }
>  
>      create_fdt(vms);

Why are you using a notifier here?
It is not enough for you just esetup at this point a variable somewhere
   "foo->mte_enabled = true"

And create a function that just does:

bool is_mte_enabled(vode)
{
        return foo->mte_enabled;
}

And just check that everywhere?

Later, Juan.

Re: [RFC PATCH v2 4/5] Add migration support for KVM guest with MTE

[Juan Quintela]

Haibo Xu <haibo.xu@linaro.org> wrote:
> To make it easier to keep the page tags sync with
> the page data, tags for one page are appended to
> the data during ram save iteration.
>
> This patch only add the pre-copy migration support.
> Post-copy and compress as well as zero page saving
> are not supported yet.
>
> Signed-off-by: Haibo Xu <haibo.xu@linaro.org>


>  #define RAM_SAVE_FLAG_XBZRLE   0x40
>  /* 0x80 is reserved in migration.h start with 0x100 next */
>  #define RAM_SAVE_FLAG_COMPRESS_PAGE    0x100
> +#define RAM_SAVE_FLAG_MTE              0x200

Flags are really a scarce resource.  You are using one here, when you
know that you will always have the feature enable (or not), so you can
do better during negotiation IMHO.



> +void precopy_enable_metadata_migration(void)
> +{
> +    if (!ram_state) {
> +        return;
> +    }
> +
> +    ram_state->metadata_enabled = true;
> +}

My understanding is that in your following patch, if mte is enabled, you
will always sent mte tags, for all pages needed, right?

> +static int save_normal_page_mte_tags(QEMUFile *f, uint8_t *addr)
> +{
> +    uint8_t *tag_buf = NULL;
> +    uint64_t ipa;
> +    int size = TARGET_PAGE_SIZE / MTE_GRANULE_SIZE;
> +
> +    if (kvm_physical_memory_addr_from_host(kvm_state, addr, &ipa)) {
> +        /* Buffer for the page tags(one byte per tag) */
> +        tag_buf = g_try_malloc0(size);

size of the buffer is known at start of migration.  Just get a buffer
and reuse it?

Do zero pages have mte tags?  From migration point of view, a zero page
is a page that is just full of zeros, i.e. nothing else special.
Because you are not sending any for them.



> @@ -1148,6 +1219,10 @@ static bool control_save_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
>  static int save_normal_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
>                              uint8_t *buf, bool async)
>  {
> +    if (rs->metadata_enabled) {
> +        offset |= RAM_SAVE_FLAG_MTE;

You don't really need the flag, for you normal pages are just
TARGET_PAGE_SIZE + (TARGET_PAGE_SIZE/MTE_)


> +    }
> +
>      ram_counters.transferred += save_page_header(rs, rs->f, block,
>                                                   offset | RAM_SAVE_FLAG_PAGE);
>      if (async) {
> @@ -1159,6 +1234,11 @@ static int save_normal_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
>      }
>      ram_counters.transferred += TARGET_PAGE_SIZE;
>      ram_counters.normal++;
> +
> +    if (rs->metadata_enabled) {

See?  You are not checking the flag, you are checking the bool setup at
the beggining of migration.

> +        ram_counters.transferred += save_normal_page_mte_tags(rs->f, buf);
> +    }
> +
>      return 1;
>  }
>  
> @@ -2189,6 +2269,7 @@ static void ram_state_reset(RAMState *rs)
>      rs->last_version = ram_list.version;
>      rs->ram_bulk_stage = true;
>      rs->fpo_enabled = false;
> +    rs->metadata_enabled = false;
>  }
>  
>  #define MAX_WAIT 50 /* ms, half buffered_file limit */
> @@ -3779,7 +3860,7 @@ static int ram_load_precopy(QEMUFile *f)
>              trace_ram_load_loop(block->idstr, (uint64_t)addr, flags, host);
>          }
>  
> -        switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
> +        switch (flags & ~(RAM_SAVE_FLAG_CONTINUE | RAM_SAVE_FLAG_MTE)) {

Creating the flag is hurting you here also.

>          case RAM_SAVE_FLAG_MEM_SIZE:
>              /* Synchronize RAM block list */
>              total_ram_bytes = addr;
> @@ -3849,6 +3930,9 @@ static int ram_load_precopy(QEMUFile *f)
>  
>          case RAM_SAVE_FLAG_PAGE:
>              qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> +            if (flags & RAM_SAVE_FLAG_MTE) {
> +                load_normal_page_mte_tags(f, host);
> +            }

I don't claim to understand the MTE, but my understanding is that if we
are using MTE, all pages have to have MTE flags, right?

So, somtehing like

is_mte_enabled()

that I told in the other thread looks like a good idea.

Later, Juan.

>              break;
>  
>          case RAM_SAVE_FLAG_COMPRESS_PAGE:

Re: [RFC PATCH v2 4/5] Add migration support for KVM guest with MTE

[Dr. David Alan Gilbert]

* Haibo Xu (haibo.xu@linaro.org) wrote:
> On Thu, 18 Mar 2021 at 04:11, Dr. David Alan Gilbert
> <dgilbert@redhat.com> wrote:
> >
> > * Haibo Xu (haibo.xu@linaro.org) wrote:
> > > To make it easier to keep the page tags sync with
> > > the page data, tags for one page are appended to
> > > the data during ram save iteration.
> > >
> > > This patch only add the pre-copy migration support.
> > > Post-copy and compress as well as zero page saving
> > > are not supported yet.
> > >
> > > Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> >
> > My guess is that this doesn't work with a lot of other options; e.g.
> > postcopy and probably compression and a bunch of other things.
> > Postcopy I can see you'll need some interesting kernel changes for -
> > you'd need to be able to atomically place a  page with it's tag data.
> >
> > You probably need to add stuff to migrate_caps_check  to disable
> > features that you don't support.
> >
> 
> Hi David,
> 
> Thanks so much for the comments!
> 
> You are right, this RFC patch only supports pre-copy mode, no
> postcopy, no compression.
> As a RFC, here just want to finalize the tag migration process, that is:
> 1. let the tag go with the page data(the current choice) which may be
> a little complex to put
>     them into the current migration process.

I think it's probably the easiest.

> 2. migrate them separately which is easy to implement with the current
> migration(treat the tags
>     as device status), but it would be hard to keep the page data and
> tag to sync with each other.
> 3. Any other ways?
> 
> Once the tag migration process is finalized, a new formal patch series
> with postcopy as well as
> compression should be reworked.

It's probably best to look what you would have to do to fit it in with
those before finalising the basic version; not necessary implement them
- just get an idea.

> What's more, you mentioned that "some interesting kernel changes are
> needed to atomically
> place a  page with it's tag data". You mean a single kernel API to
> store page data and tag in
> the migration load process?

Yes;  see migration/postcopy-ram.c postcopy_place_page  - it has the
problem of placing the whole page in memory before the guest starts
usign it; I think for MTE you're going to have to put the MTE data in at
the same time.

Dave

> 
> Regards,
> Haibo
> 
> > > ---
> > >  include/hw/arm/virt.h    |  2 +
> > >  include/migration/misc.h |  1 +
> > >  migration/ram.c          | 86 +++++++++++++++++++++++++++++++++++++++-
> > >  3 files changed, 88 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > > index 921416f918..8b28cde8bf 100644
> > > --- a/include/hw/arm/virt.h
> > > +++ b/include/hw/arm/virt.h
> > > @@ -166,6 +166,8 @@ struct VirtMachineState {
> > >      PCIBus *bus;
> > >      char *oem_id;
> > >      char *oem_table_id;
> > > +    /* migrate memory tags */
> > > +    NotifierWithReturn precopy_notifier;
> > >  };
> > >
> > >  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
> > > diff --git a/include/migration/misc.h b/include/migration/misc.h
> > > index bccc1b6b44..005133f471 100644
> > > --- a/include/migration/misc.h
> > > +++ b/include/migration/misc.h
> > > @@ -38,6 +38,7 @@ void precopy_add_notifier(NotifierWithReturn *n);
> > >  void precopy_remove_notifier(NotifierWithReturn *n);
> > >  int precopy_notify(PrecopyNotifyReason reason, Error **errp);
> > >  void precopy_enable_free_page_optimization(void);
> > > +void precopy_enable_metadata_migration(void);
> > >
> > >  void ram_mig_init(void);
> > >  void qemu_guest_free_page_hint(void *addr, size_t len);
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index 72143da0ac..e67b798c3b 100644
> > > --- a/migration/ram.c
> > > +++ b/migration/ram.c
> > > @@ -53,10 +53,12 @@
> > >  #include "block.h"
> > >  #include "sysemu/sysemu.h"
> > >  #include "sysemu/cpu-throttle.h"
> > > +#include "sysemu/kvm.h"
> > >  #include "savevm.h"
> > >  #include "qemu/iov.h"
> > >  #include "multifd.h"
> > >  #include "sysemu/runstate.h"
> > > +#include "kvm_arm.h"
> > >
> > >  #if defined(__linux__)
> > >  #include "qemu/userfaultfd.h"
> > > @@ -80,6 +82,9 @@
> > >  #define RAM_SAVE_FLAG_XBZRLE   0x40
> > >  /* 0x80 is reserved in migration.h start with 0x100 next */
> > >  #define RAM_SAVE_FLAG_COMPRESS_PAGE    0x100
> > > +#define RAM_SAVE_FLAG_MTE              0x200
> >
> > I think that's using the last free bit in the flags field, which is a
> > bit painful.
> >
> 
> Yes, only 0x80(reserved) and 0x200 are available.
> 
> > > +#define MTE_GRANULE_SIZE   (16)
> > >
> > >  static inline bool is_zero_range(uint8_t *p, uint64_t size)
> > >  {
> > > @@ -317,6 +322,8 @@ struct RAMState {
> > >      bool ram_bulk_stage;
> > >      /* The free page optimization is enabled */
> > >      bool fpo_enabled;
> > > +    /* The RAM meta data(e.t memory tag) is enabled */
> > > +    bool metadata_enabled;
> > >      /* How many times we have dirty too many pages */
> > >      int dirty_rate_high_cnt;
> > >      /* these variables are used for bitmap sync */
> > > @@ -394,6 +401,15 @@ void precopy_enable_free_page_optimization(void)
> > >      ram_state->fpo_enabled = true;
> > >  }
> > >
> > > +void precopy_enable_metadata_migration(void)
> > > +{
> > > +    if (!ram_state) {
> > > +        return;
> > > +    }
> > > +
> > > +    ram_state->metadata_enabled = true;
> > > +}
> > > +
> > >  uint64_t ram_bytes_remaining(void)
> > >  {
> > >      return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) :
> > > @@ -1134,6 +1150,61 @@ static bool control_save_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
> > >      return true;
> > >  }
> > >
> > > +static int save_normal_page_mte_tags(QEMUFile *f, uint8_t *addr)
> > > +{
> > > +    uint8_t *tag_buf = NULL;
> > > +    uint64_t ipa;
> > > +    int size = TARGET_PAGE_SIZE / MTE_GRANULE_SIZE;
> >
> > This function needs to be mostly somewhere aarch specific; or somehow
> > made more generic.
> > We shouldn't have RAM_SAVE_FLAG_MTE as well - we should be something
> > like RAM_SAVE_FLAG_ARCH_METADATA and that way other architectures with
> > something else glued onto pages can do something similar.
> > Try and keep migration/ architecture independent.
> 
> Yes, it should be arch independent and handled with a more abstract way.
> 
> >
> > > +    if (kvm_physical_memory_addr_from_host(kvm_state, addr, &ipa)) {
> > > +        /* Buffer for the page tags(one byte per tag) */
> > > +        tag_buf = g_try_malloc0(size);
> >
> > It feels like you want to allocate tag_buf in setup and free it at the
> > end rather than doing this in every page.
> 
> Yes, the tag buffer allocation could be moved to migration save/load
> setup and cleanup function.
> 
> >
> > > +        if (!tag_buf) {
> > > +            error_report("%s: Error allocating MTE tag_buf", __func__);
> > > +            return 0;
> > > +        }
> > > +
> > > +        if (kvm_arm_mte_get_tags(ipa, TARGET_PAGE_SIZE, tag_buf) < 0) {
> > > +            error_report("%s: Can't get MTE tags from guest", __func__);
> >
> > For any error like this you probably want to say the addresses to make
> > it easier to debug when it fails.
> >
> 
> Good suggestion! Will add the "address" info to the log.
> 
> > > +            g_free(tag_buf);
> > > +            return 0;
> > > +        }
> > > +
> > > +        qemu_put_buffer(f, tag_buf, size);
> > > +
> > > +        g_free(tag_buf);
> > > +
> > > +        return size;
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +static void load_normal_page_mte_tags(QEMUFile *f, uint8_t *addr)
> > > +{
> > > +    uint8_t *tag_buf = NULL;
> > > +    uint64_t ipa;
> > > +    int size = TARGET_PAGE_SIZE / MTE_GRANULE_SIZE;
> > > +
> > > +    if (kvm_physical_memory_addr_from_host(kvm_state, addr, &ipa)) {
> > > +        /* Buffer for the page tags(one byte per tag) */
> > > +        tag_buf = g_try_malloc0(size);
> > > +        if (!tag_buf) {
> > > +            error_report("%s: Error allocating MTE tag_buf", __func__);
> > > +            return;
> > > +        }
> > > +
> > > +        qemu_get_buffer(f, tag_buf, size);
> > > +        if (kvm_arm_mte_set_tags(ipa, TARGET_PAGE_SIZE, tag_buf) < 0) {
> >
> > what protections are there here to stop you setting the mte on something
> > useful, like part of the host kernel or qemu?
> 
> Kernel would do some parameter check(e.t ipa within a valid slot)
> before setting the tag data.
> 
> >
> > > +            error_report("%s: Can't set MTE tags to guest", __func__);
> > > +        }
> > > +
> > > +        g_free(tag_buf);
> > > +    }
> > > +
> > > +    return;
> > > +}
> > > +
> > >  /*
> > >   * directly send the page to the stream
> > >   *
> > > @@ -1148,6 +1219,10 @@ static bool control_save_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
> > >  static int save_normal_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
> > >                              uint8_t *buf, bool async)
> > >  {
> > > +    if (rs->metadata_enabled) {
> > > +        offset |= RAM_SAVE_FLAG_MTE;
> > > +    }
> > > +
> > >      ram_counters.transferred += save_page_header(rs, rs->f, block,
> > >                                                   offset | RAM_SAVE_FLAG_PAGE);
> > >      if (async) {
> > > @@ -1159,6 +1234,11 @@ static int save_normal_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
> > >      }
> > >      ram_counters.transferred += TARGET_PAGE_SIZE;
> > >      ram_counters.normal++;
> > > +
> > > +    if (rs->metadata_enabled) {
> > > +        ram_counters.transferred += save_normal_page_mte_tags(rs->f, buf);
> > > +    }
> > > +
> > >      return 1;
> > >  }
> > >
> > > @@ -2189,6 +2269,7 @@ static void ram_state_reset(RAMState *rs)
> > >      rs->last_version = ram_list.version;
> > >      rs->ram_bulk_stage = true;
> > >      rs->fpo_enabled = false;
> > > +    rs->metadata_enabled = false;
> > >  }
> > >
> > >  #define MAX_WAIT 50 /* ms, half buffered_file limit */
> > > @@ -3779,7 +3860,7 @@ static int ram_load_precopy(QEMUFile *f)
> > >              trace_ram_load_loop(block->idstr, (uint64_t)addr, flags, host);
> > >          }
> > >
> > > -        switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
> > > +        switch (flags & ~(RAM_SAVE_FLAG_CONTINUE | RAM_SAVE_FLAG_MTE)) {
> > >          case RAM_SAVE_FLAG_MEM_SIZE:
> > >              /* Synchronize RAM block list */
> > >              total_ram_bytes = addr;
> > > @@ -3849,6 +3930,9 @@ static int ram_load_precopy(QEMUFile *f)
> > >
> > >          case RAM_SAVE_FLAG_PAGE:
> > >              qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> > > +            if (flags & RAM_SAVE_FLAG_MTE) {
> > > +                load_normal_page_mte_tags(f, host);
> > > +            }
> > >              break;
> > >
> > >          case RAM_SAVE_FLAG_COMPRESS_PAGE:
> > > --
> > > 2.17.1
> > >
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> >
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [RFC PATCH v2 3/5] Add APIs to get/set MTE tags

[Haibo Xu]

On Thu, 25 Mar 2021 at 20:18, Juan Quintela <quintela@redhat.com> wrote:
>
> Haibo Xu <haibo.xu@linaro.org> wrote:
> > MTE spec provide instructions to retrieve the memory tags:
> > (1) LDG, at 16 bytes granularity, and available in both user
> >     and kernel space;
> > (2) LDGM, at 256 bytes granularity in maximum, and only
> >     available in kernel space
> >
> > To improve the performance, KVM has exposed the LDGM capability
> > to user space by providing a new APIs. This patch is just a
> > wrapper for the KVM APIs.
> >
> > Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> > ---
> >  target/arm/kvm64.c   | 24 ++++++++++++++++++++++++
> >  target/arm/kvm_arm.h |  2 ++
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > index 73a191f8e1..3157025316 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -1606,3 +1606,27 @@ bool kvm_arm_verify_ext_dabt_pending(CPUState *cs)
> >      }
> >      return false;
> >  }
> > +
> > +int kvm_arm_mte_get_tags(uint64_t ipa, uint64_t len, uint8_t *buf)
> > +{
> > +    struct kvm_arm_copy_mte_tags args = {
> > +        .guest_ipa = ipa,
> > +        .length = len,
> > +        .addr = buf,
> > +        .flags = KVM_ARM_TAGS_FROM_GUEST,
> > +    };
> > +
> > +    return kvm_vm_ioctl(kvm_state, KVM_ARM_MTE_COPY_TAGS, &args);
>
> Just a question, how fast/slow are this calls?
>

There is no performance data for this API yet, but at least it's more
efficient than that
to only be able to access a single tag by the EL0 "LDG" instruction.
We will try to
collect some performance data for this KVM API later.

> My understanding is that we are making a kvm call for each page that we
> want to migrate, right?
>

Yes, currently I chose to append the tag values to the page data
during the migration.

> Each time that we want to send it.
>
> Later, Juan.
>
>
> > +}
> > +
> > +int kvm_arm_mte_set_tags(uint64_t ipa, uint64_t len, uint8_t *buf)
> > +{
> > +    struct kvm_arm_copy_mte_tags args = {
> > +        .guest_ipa = ipa,
> > +        .length = len,
> > +        .addr = buf,
> > +        .flags = KVM_ARM_TAGS_TO_GUEST,
> > +    };
> > +
> > +    return kvm_vm_ioctl(kvm_state, KVM_ARM_MTE_COPY_TAGS, &args);
> > +}
> > diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> > index 34f8daa377..bbb833d6c6 100644
> > --- a/target/arm/kvm_arm.h
> > +++ b/target/arm/kvm_arm.h
> > @@ -360,6 +360,8 @@ int kvm_arm_vgic_probe(void);
> >
> >  void kvm_arm_pmu_set_irq(CPUState *cs, int irq);
> >  void kvm_arm_pmu_init(CPUState *cs);
> > +int kvm_arm_mte_get_tags(uint64_t ipa, uint64_t len, uint8_t *buf);
> > +int kvm_arm_mte_set_tags(uint64_t ipa, uint64_t len, uint8_t *buf);
> >
> >  /**
> >   * kvm_arm_pvtime_init:
>

Re: [RFC PATCH v2 4/5] Add migration support for KVM guest with MTE

[Haibo Xu]

On Thu, 25 Mar 2021 at 23:37, Juan Quintela <quintela@redhat.com> wrote:
>
> Haibo Xu <haibo.xu@linaro.org> wrote:
> > To make it easier to keep the page tags sync with
> > the page data, tags for one page are appended to
> > the data during ram save iteration.
> >
> > This patch only add the pre-copy migration support.
> > Post-copy and compress as well as zero page saving
> > are not supported yet.
> >
> > Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
>
>
> >  #define RAM_SAVE_FLAG_XBZRLE   0x40
> >  /* 0x80 is reserved in migration.h start with 0x100 next */
> >  #define RAM_SAVE_FLAG_COMPRESS_PAGE    0x100
> > +#define RAM_SAVE_FLAG_MTE              0x200
>
> Flags are really a scarce resource.  You are using one here, when you
> know that you will always have the feature enable (or not), so you can
> do better during negotiation IMHO.
>

Yes, any suggestions are welcomed to finalize the MTE migration support!

>
> > +void precopy_enable_metadata_migration(void)
> > +{
> > +    if (!ram_state) {
> > +        return;
> > +    }
> > +
> > +    ram_state->metadata_enabled = true;
> > +}
>
> My understanding is that in your following patch, if mte is enabled, you
> will always sent mte tags, for all pages needed, right?

Yes, for the current kernel support, we can't tell whether the MTE was enabled
for a specific page.

>
> > +static int save_normal_page_mte_tags(QEMUFile *f, uint8_t *addr)
> > +{
> > +    uint8_t *tag_buf = NULL;
> > +    uint64_t ipa;
> > +    int size = TARGET_PAGE_SIZE / MTE_GRANULE_SIZE;
> > +
> > +    if (kvm_physical_memory_addr_from_host(kvm_state, addr, &ipa)) {
> > +        /* Buffer for the page tags(one byte per tag) */
> > +        tag_buf = g_try_malloc0(size);
>
> size of the buffer is known at start of migration.  Just get a buffer
> and reuse it?

Yes, we can pre-allocate the buffer during the migration setup time

>
> Do zero pages have mte tags?  From migration point of view, a zero page
> is a page that is just full of zeros, i.e. nothing else special.
> Because you are not sending any for them.
>

Yes, I think we can do some optimization for the zero page migration support.

>
>
> > @@ -1148,6 +1219,10 @@ static bool control_save_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
> >  static int save_normal_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
> >                              uint8_t *buf, bool async)
> >  {
> > +    if (rs->metadata_enabled) {
> > +        offset |= RAM_SAVE_FLAG_MTE;
>
> You don't really need the flag, for you normal pages are just
> TARGET_PAGE_SIZE + (TARGET_PAGE_SIZE/MTE_)
>

So you suggest to use the size field to indicate whether tags are available?

>
> > +    }
> > +
> >      ram_counters.transferred += save_page_header(rs, rs->f, block,
> >                                                   offset | RAM_SAVE_FLAG_PAGE);
> >      if (async) {
> > @@ -1159,6 +1234,11 @@ static int save_normal_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
> >      }
> >      ram_counters.transferred += TARGET_PAGE_SIZE;
> >      ram_counters.normal++;
> > +
> > +    if (rs->metadata_enabled) {
>
> See?  You are not checking the flag, you are checking the bool setup at
> the beggining of migration.

The idea is to only migrate the memory tags when MTE was enabled for the VM.
Could you be more elaborate on "You are not checking the flag"?

>
> > +        ram_counters.transferred += save_normal_page_mte_tags(rs->f, buf);
> > +    }
> > +
> >      return 1;
> >  }
> >
> > @@ -2189,6 +2269,7 @@ static void ram_state_reset(RAMState *rs)
> >      rs->last_version = ram_list.version;
> >      rs->ram_bulk_stage = true;
> >      rs->fpo_enabled = false;
> > +    rs->metadata_enabled = false;
> >  }
> >
> >  #define MAX_WAIT 50 /* ms, half buffered_file limit */
> > @@ -3779,7 +3860,7 @@ static int ram_load_precopy(QEMUFile *f)
> >              trace_ram_load_loop(block->idstr, (uint64_t)addr, flags, host);
> >          }
> >
> > -        switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
> > +        switch (flags & ~(RAM_SAVE_FLAG_CONTINUE | RAM_SAVE_FLAG_MTE)) {
>
> Creating the flag is hurting you here also.
>
> >          case RAM_SAVE_FLAG_MEM_SIZE:
> >              /* Synchronize RAM block list */
> >              total_ram_bytes = addr;
> > @@ -3849,6 +3930,9 @@ static int ram_load_precopy(QEMUFile *f)
> >
> >          case RAM_SAVE_FLAG_PAGE:
> >              qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> > +            if (flags & RAM_SAVE_FLAG_MTE) {
> > +                load_normal_page_mte_tags(f, host);
> > +            }
>
> I don't claim to understand the MTE, but my understanding is that if we
> are using MTE, all pages have to have MTE flags, right?
>
> So, somtehing like
>
> is_mte_enabled()
>
> that I told in the other thread looks like a good idea.
>

Yes, we do need a function like this.

> Later, Juan.
>
> >              break;
> >
> >          case RAM_SAVE_FLAG_COMPRESS_PAGE:
>

Re: [RFC PATCH v2 4/5] Add migration support for KVM guest with MTE

[Haibo Xu]

On Fri, 26 Mar 2021 at 03:38, Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Haibo Xu (haibo.xu@linaro.org) wrote:
> > On Thu, 18 Mar 2021 at 04:11, Dr. David Alan Gilbert
> > <dgilbert@redhat.com> wrote:
> > >
> > > * Haibo Xu (haibo.xu@linaro.org) wrote:
> > > > To make it easier to keep the page tags sync with
> > > > the page data, tags for one page are appended to
> > > > the data during ram save iteration.
> > > >
> > > > This patch only add the pre-copy migration support.
> > > > Post-copy and compress as well as zero page saving
> > > > are not supported yet.
> > > >
> > > > Signed-off-by: Haibo Xu <haibo.xu@linaro.org>
> > >
> > > My guess is that this doesn't work with a lot of other options; e.g.
> > > postcopy and probably compression and a bunch of other things.
> > > Postcopy I can see you'll need some interesting kernel changes for -
> > > you'd need to be able to atomically place a  page with it's tag data.
> > >
> > > You probably need to add stuff to migrate_caps_check  to disable
> > > features that you don't support.
> > >
> >
> > Hi David,
> >
> > Thanks so much for the comments!
> >
> > You are right, this RFC patch only supports pre-copy mode, no
> > postcopy, no compression.
> > As a RFC, here just want to finalize the tag migration process, that is:
> > 1. let the tag go with the page data(the current choice) which may be
> > a little complex to put
> >     them into the current migration process.
>
> I think it's probably the easiest.
>

Agree!

> > 2. migrate them separately which is easy to implement with the current
> > migration(treat the tags
> >     as device status), but it would be hard to keep the page data and
> > tag to sync with each other.
> > 3. Any other ways?
> >
> > Once the tag migration process is finalized, a new formal patch series
> > with postcopy as well as
> > compression should be reworked.
>
> It's probably best to look what you would have to do to fit it in with
> those before finalising the basic version; not necessary implement them
> - just get an idea.
>

Thanks for the suggestion!

> > What's more, you mentioned that "some interesting kernel changes are
> > needed to atomically
> > place a  page with it's tag data". You mean a single kernel API to
> > store page data and tag in
> > the migration load process?
>
> Yes;  see migration/postcopy-ram.c postcopy_place_page  - it has the
> problem of placing the whole page in memory before the guest starts
> usign it; I think for MTE you're going to have to put the MTE data in at
> the same time.
>

Good point! Will check this with the kernel guys.

> Dave
>
> >
> > Regards,
> > Haibo
> >
> > > > ---
> > > >  include/hw/arm/virt.h    |  2 +
> > > >  include/migration/misc.h |  1 +
> > > >  migration/ram.c          | 86 +++++++++++++++++++++++++++++++++++++++-
> > > >  3 files changed, 88 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > > > index 921416f918..8b28cde8bf 100644
> > > > --- a/include/hw/arm/virt.h
> > > > +++ b/include/hw/arm/virt.h
> > > > @@ -166,6 +166,8 @@ struct VirtMachineState {
> > > >      PCIBus *bus;
> > > >      char *oem_id;
> > > >      char *oem_table_id;
> > > > +    /* migrate memory tags */
> > > > +    NotifierWithReturn precopy_notifier;
> > > >  };
> > > >
> > > >  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
> > > > diff --git a/include/migration/misc.h b/include/migration/misc.h
> > > > index bccc1b6b44..005133f471 100644
> > > > --- a/include/migration/misc.h
> > > > +++ b/include/migration/misc.h
> > > > @@ -38,6 +38,7 @@ void precopy_add_notifier(NotifierWithReturn *n);
> > > >  void precopy_remove_notifier(NotifierWithReturn *n);
> > > >  int precopy_notify(PrecopyNotifyReason reason, Error **errp);
> > > >  void precopy_enable_free_page_optimization(void);
> > > > +void precopy_enable_metadata_migration(void);
> > > >
> > > >  void ram_mig_init(void);
> > > >  void qemu_guest_free_page_hint(void *addr, size_t len);
> > > > diff --git a/migration/ram.c b/migration/ram.c
> > > > index 72143da0ac..e67b798c3b 100644
> > > > --- a/migration/ram.c
> > > > +++ b/migration/ram.c
> > > > @@ -53,10 +53,12 @@
> > > >  #include "block.h"
> > > >  #include "sysemu/sysemu.h"
> > > >  #include "sysemu/cpu-throttle.h"
> > > > +#include "sysemu/kvm.h"
> > > >  #include "savevm.h"
> > > >  #include "qemu/iov.h"
> > > >  #include "multifd.h"
> > > >  #include "sysemu/runstate.h"
> > > > +#include "kvm_arm.h"
> > > >
> > > >  #if defined(__linux__)
> > > >  #include "qemu/userfaultfd.h"
> > > > @@ -80,6 +82,9 @@
> > > >  #define RAM_SAVE_FLAG_XBZRLE   0x40
> > > >  /* 0x80 is reserved in migration.h start with 0x100 next */
> > > >  #define RAM_SAVE_FLAG_COMPRESS_PAGE    0x100
> > > > +#define RAM_SAVE_FLAG_MTE              0x200
> > >
> > > I think that's using the last free bit in the flags field, which is a
> > > bit painful.
> > >
> >
> > Yes, only 0x80(reserved) and 0x200 are available.
> >
> > > > +#define MTE_GRANULE_SIZE   (16)
> > > >
> > > >  static inline bool is_zero_range(uint8_t *p, uint64_t size)
> > > >  {
> > > > @@ -317,6 +322,8 @@ struct RAMState {
> > > >      bool ram_bulk_stage;
> > > >      /* The free page optimization is enabled */
> > > >      bool fpo_enabled;
> > > > +    /* The RAM meta data(e.t memory tag) is enabled */
> > > > +    bool metadata_enabled;
> > > >      /* How many times we have dirty too many pages */
> > > >      int dirty_rate_high_cnt;
> > > >      /* these variables are used for bitmap sync */
> > > > @@ -394,6 +401,15 @@ void precopy_enable_free_page_optimization(void)
> > > >      ram_state->fpo_enabled = true;
> > > >  }
> > > >
> > > > +void precopy_enable_metadata_migration(void)
> > > > +{
> > > > +    if (!ram_state) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    ram_state->metadata_enabled = true;
> > > > +}
> > > > +
> > > >  uint64_t ram_bytes_remaining(void)
> > > >  {
> > > >      return ram_state ? (ram_state->migration_dirty_pages * TARGET_PAGE_SIZE) :
> > > > @@ -1134,6 +1150,61 @@ static bool control_save_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
> > > >      return true;
> > > >  }
> > > >
> > > > +static int save_normal_page_mte_tags(QEMUFile *f, uint8_t *addr)
> > > > +{
> > > > +    uint8_t *tag_buf = NULL;
> > > > +    uint64_t ipa;
> > > > +    int size = TARGET_PAGE_SIZE / MTE_GRANULE_SIZE;
> > >
> > > This function needs to be mostly somewhere aarch specific; or somehow
> > > made more generic.
> > > We shouldn't have RAM_SAVE_FLAG_MTE as well - we should be something
> > > like RAM_SAVE_FLAG_ARCH_METADATA and that way other architectures with
> > > something else glued onto pages can do something similar.
> > > Try and keep migration/ architecture independent.
> >
> > Yes, it should be arch independent and handled with a more abstract way.
> >
> > >
> > > > +    if (kvm_physical_memory_addr_from_host(kvm_state, addr, &ipa)) {
> > > > +        /* Buffer for the page tags(one byte per tag) */
> > > > +        tag_buf = g_try_malloc0(size);
> > >
> > > It feels like you want to allocate tag_buf in setup and free it at the
> > > end rather than doing this in every page.
> >
> > Yes, the tag buffer allocation could be moved to migration save/load
> > setup and cleanup function.
> >
> > >
> > > > +        if (!tag_buf) {
> > > > +            error_report("%s: Error allocating MTE tag_buf", __func__);
> > > > +            return 0;
> > > > +        }
> > > > +
> > > > +        if (kvm_arm_mte_get_tags(ipa, TARGET_PAGE_SIZE, tag_buf) < 0) {
> > > > +            error_report("%s: Can't get MTE tags from guest", __func__);
> > >
> > > For any error like this you probably want to say the addresses to make
> > > it easier to debug when it fails.
> > >
> >
> > Good suggestion! Will add the "address" info to the log.
> >
> > > > +            g_free(tag_buf);
> > > > +            return 0;
> > > > +        }
> > > > +
> > > > +        qemu_put_buffer(f, tag_buf, size);
> > > > +
> > > > +        g_free(tag_buf);
> > > > +
> > > > +        return size;
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static void load_normal_page_mte_tags(QEMUFile *f, uint8_t *addr)
> > > > +{
> > > > +    uint8_t *tag_buf = NULL;
> > > > +    uint64_t ipa;
> > > > +    int size = TARGET_PAGE_SIZE / MTE_GRANULE_SIZE;
> > > > +
> > > > +    if (kvm_physical_memory_addr_from_host(kvm_state, addr, &ipa)) {
> > > > +        /* Buffer for the page tags(one byte per tag) */
> > > > +        tag_buf = g_try_malloc0(size);
> > > > +        if (!tag_buf) {
> > > > +            error_report("%s: Error allocating MTE tag_buf", __func__);
> > > > +            return;
> > > > +        }
> > > > +
> > > > +        qemu_get_buffer(f, tag_buf, size);
> > > > +        if (kvm_arm_mte_set_tags(ipa, TARGET_PAGE_SIZE, tag_buf) < 0) {
> > >
> > > what protections are there here to stop you setting the mte on something
> > > useful, like part of the host kernel or qemu?
> >
> > Kernel would do some parameter check(e.t ipa within a valid slot)
> > before setting the tag data.
> >
> > >
> > > > +            error_report("%s: Can't set MTE tags to guest", __func__);
> > > > +        }
> > > > +
> > > > +        g_free(tag_buf);
> > > > +    }
> > > > +
> > > > +    return;
> > > > +}
> > > > +
> > > >  /*
> > > >   * directly send the page to the stream
> > > >   *
> > > > @@ -1148,6 +1219,10 @@ static bool control_save_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
> > > >  static int save_normal_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
> > > >                              uint8_t *buf, bool async)
> > > >  {
> > > > +    if (rs->metadata_enabled) {
> > > > +        offset |= RAM_SAVE_FLAG_MTE;
> > > > +    }
> > > > +
> > > >      ram_counters.transferred += save_page_header(rs, rs->f, block,
> > > >                                                   offset | RAM_SAVE_FLAG_PAGE);
> > > >      if (async) {
> > > > @@ -1159,6 +1234,11 @@ static int save_normal_page(RAMState *rs, RAMBlock *block, ram_addr_t offset,
> > > >      }
> > > >      ram_counters.transferred += TARGET_PAGE_SIZE;
> > > >      ram_counters.normal++;
> > > > +
> > > > +    if (rs->metadata_enabled) {
> > > > +        ram_counters.transferred += save_normal_page_mte_tags(rs->f, buf);
> > > > +    }
> > > > +
> > > >      return 1;
> > > >  }
> > > >
> > > > @@ -2189,6 +2269,7 @@ static void ram_state_reset(RAMState *rs)
> > > >      rs->last_version = ram_list.version;
> > > >      rs->ram_bulk_stage = true;
> > > >      rs->fpo_enabled = false;
> > > > +    rs->metadata_enabled = false;
> > > >  }
> > > >
> > > >  #define MAX_WAIT 50 /* ms, half buffered_file limit */
> > > > @@ -3779,7 +3860,7 @@ static int ram_load_precopy(QEMUFile *f)
> > > >              trace_ram_load_loop(block->idstr, (uint64_t)addr, flags, host);
> > > >          }
> > > >
> > > > -        switch (flags & ~RAM_SAVE_FLAG_CONTINUE) {
> > > > +        switch (flags & ~(RAM_SAVE_FLAG_CONTINUE | RAM_SAVE_FLAG_MTE)) {
> > > >          case RAM_SAVE_FLAG_MEM_SIZE:
> > > >              /* Synchronize RAM block list */
> > > >              total_ram_bytes = addr;
> > > > @@ -3849,6 +3930,9 @@ static int ram_load_precopy(QEMUFile *f)
> > > >
> > > >          case RAM_SAVE_FLAG_PAGE:
> > > >              qemu_get_buffer(f, host, TARGET_PAGE_SIZE);
> > > > +            if (flags & RAM_SAVE_FLAG_MTE) {
> > > > +                load_normal_page_mte_tags(f, host);
> > > > +            }
> > > >              break;
> > > >
> > > >          case RAM_SAVE_FLAG_COMPRESS_PAGE:
> > > > --
> > > > 2.17.1
> > > >
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > >
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>