Re: [Qemu-devel] intel_iommu migration compatibiltiy for scalable mode emulation

[Peter Xu]

On Wed, Mar 27, 2019 at 06:17:08PM -0400, Michael S. Tsirkin wrote:
> On Wed, Mar 27, 2019 at 08:02:24PM +0000, Dr. David Alan Gilbert wrote:
> > (Oops, originally forgot to cc the list)
> > 
> > * Dr. David Alan Gilbert (address@hidden) wrote:
> > > Hi,
> > >   Commit fb43cf739 introduced a new iommu migration field called
> > > 'root_scalable' - but it doesn't do any machine type checks, which means
> > > it breaks migration between 3.1<->4.0 -  we should fix this for 4.0.
> > > 
> > > Can we make this conditional on the 4.0 machine type?
> > > 
> > > Dave
> > > --
> > > Dr. David Alan Gilbert / address@hidden / Manchester, UK
> > --
> > Dr. David Alan Gilbert / address@hidden / Manchester, UK
> 
> Yes we have to. Peter?

Oh yes... My fault to not have noticed it when reviewing.

A simple fix could be simply boosting the version:

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index b90de6c664..c1f4604143 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2950,7 +2950,7 @@ static int vtd_post_load(void *opaque, int version_id)
 
 static const VMStateDescription vtd_vmstate = {
     .name = "iommu-intel",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .priority = MIG_PRI_IOMMU,
     .post_load = vtd_post_load,
@@ -2966,7 +2966,7 @@ static const VMStateDescription vtd_vmstate = {
         VMSTATE_UINT8_ARRAY(csr, IntelIOMMUState, DMAR_REG_SIZE),
         VMSTATE_UINT8(iq_last_desc_type, IntelIOMMUState),
         VMSTATE_BOOL(root_extended, IntelIOMMUState),
-        VMSTATE_BOOL(root_scalable, IntelIOMMUState),
+        VMSTATE_BOOL_V(root_scalable, IntelIOMMUState, 2),
         VMSTATE_BOOL(dmar_enabled, IntelIOMMUState),
         VMSTATE_BOOL(qi_enabled, IntelIOMMUState),
         VMSTATE_BOOL(intr_enabled, IntelIOMMUState),

But maybe it won't satisfy 4.0->3.1 migrations (hmm Dave could you
hint me on why we need this backward migration again?  I probably
asked before but I must have forgotten!).

Another way is to calculate it during post load:

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index b90de6c664..8a68f7fac5 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -162,6 +162,15 @@ static inline void vtd_iommu_unlock(IntelIOMMUState *s)
     qemu_mutex_unlock(&s->iommu_lock);
 }

+static void vtd_update_scalable_state(IntelIOMMUState *s)
+{
+    uint64_t val = vtd_get_quad_raw(s, DMAR_RTADDR_REG);
+
+    if (s->scalable_mode) {
+        s->root_scalable = val & VTD_RTADDR_SMT;
+    }
+}
+
 /* Whether the address space needs to notify new mappings */
 static inline gboolean vtd_as_has_map_notifier(VTDAddressSpace *as)
 {
@@ -1710,11 +1719,10 @@ static void vtd_root_table_setup(IntelIOMMUState *s)
 {
     s->root = vtd_get_quad_raw(s, DMAR_RTADDR_REG);
     s->root_extended = s->root & VTD_RTADDR_RTT;
-    if (s->scalable_mode) {
-        s->root_scalable = s->root & VTD_RTADDR_SMT;
-    }
     s->root &= VTD_RTADDR_ADDR_MASK(s->aw_bits);

+    vtd_update_scalable_state(s);
+
     trace_vtd_reg_dmar_root(s->root, s->root_extended);
 }

@@ -2945,6 +2953,12 @@ static int vtd_post_load(void *opaque, int version_id)
      */
     vtd_switch_address_space_all(iommu);

+    /*
+     * We don't migrate the root_scalable to make sure we won't break
+     * migrations from/to old binaries.
+     */
+    vtd_update_scalable_state(iommu);
+
     return 0;
 }

@@ -2966,7 +2980,6 @@ static const VMStateDescription vtd_vmstate = {
         VMSTATE_UINT8_ARRAY(csr, IntelIOMMUState, DMAR_REG_SIZE),
         VMSTATE_UINT8(iq_last_desc_type, IntelIOMMUState),
         VMSTATE_BOOL(root_extended, IntelIOMMUState),
-        VMSTATE_BOOL(root_scalable, IntelIOMMUState),
         VMSTATE_BOOL(dmar_enabled, IntelIOMMUState),
         VMSTATE_BOOL(qi_enabled, IntelIOMMUState),
         VMSTATE_BOOL(intr_enabled, IntelIOMMUState),

This is a bit longer but I think it should fix both directions (3.1
<-> 4.0 migrations).

Regards,

-- 
Peter Xu