Re: [Qemu-devel] [PATCH RFC v3 13/14] intel_iommu: allow dynamic switch of IOMMU region

[Peter Xu <peterx@redhat.com>]

On Mon, Jan 16, 2017 at 02:20:31PM +0800, Jason Wang wrote:

[...]

> >diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >index fd75112..2596f11 100644
> >--- a/hw/i386/intel_iommu.c
> >+++ b/hw/i386/intel_iommu.c
> >@@ -1343,9 +1343,49 @@ static void vtd_handle_gcmd_sirtp(IntelIOMMUState *s)
> >      vtd_set_clear_mask_long(s, DMAR_GSTS_REG, 0, VTD_GSTS_IRTPS);
> >  }
> >+static void vtd_switch_address_space(VTDAddressSpace *as, bool iommu_enabled)
> 
> Looks like you can check s->dmar_enabled here?

Yes, we need to check old state in case we don't need a switch at all.
Actually I checked it...

> 
> >+{
> >+    assert(as);
> >+
> >+    trace_vtd_switch_address_space(pci_bus_num(as->bus),
> >+                                   VTD_PCI_SLOT(as->devfn),
> >+                                   VTD_PCI_FUNC(as->devfn),
> >+                                   iommu_enabled);
> >+
> >+    /* Turn off first then on the other */
> >+    if (iommu_enabled) {
> >+        memory_region_set_enabled(&as->sys_alias, false);
> >+        memory_region_set_enabled(&as->iommu, true);
> >+    } else {
> >+        memory_region_set_enabled(&as->iommu, false);
> >+        memory_region_set_enabled(&as->sys_alias, true);
> >+    }
> >+}

[...]

> >  /* Handle Translation Enable/Disable */
> >  static void vtd_handle_gcmd_te(IntelIOMMUState *s, bool en)
> >  {
> >+    if (s->dmar_enabled == en) {
> >+        return;
> >+    }
> >+

... here :) ... and ...

[...]

> >+        memory_region_init_alias(&vtd_dev_as->sys_alias, OBJECT(s),
> >+                                 "vtd_sys_alias", get_system_memory(),
> >+                                 0, memory_region_size(get_system_memory()));
> >          memory_region_init_io(&vtd_dev_as->iommu_ir, OBJECT(s),
> >                                &vtd_mem_ir_ops, s, "intel_iommu_ir",
> >                                VTD_INTERRUPT_ADDR_SIZE);
> >-        memory_region_add_subregion(&vtd_dev_as->iommu, VTD_INTERRUPT_ADDR_FIRST,
> >-                                    &vtd_dev_as->iommu_ir);
> >-        address_space_init(&vtd_dev_as->as,
> >-                           &vtd_dev_as->iommu, name);
> >+        memory_region_init(&vtd_dev_as->root, OBJECT(s),
> >+                           "vtd_root", UINT64_MAX);
> >+        memory_region_add_subregion_overlap(&vtd_dev_as->root,
> >+                                            VTD_INTERRUPT_ADDR_FIRST,
> >+                                            &vtd_dev_as->iommu_ir, 64);
> >+        address_space_init(&vtd_dev_as->as, &vtd_dev_as->root, name);
> >+        memory_region_add_subregion_overlap(&vtd_dev_as->root, 0,
> >+                                            &vtd_dev_as->sys_alias, 1);
> >+        memory_region_add_subregion_overlap(&vtd_dev_as->root, 0,
> >+                                            &vtd_dev_as->iommu, 1);
> >+        vtd_switch_address_space(vtd_dev_as, s->dmar_enabled);

... here I also used vtd_switch_address_space() to setup the init
state of the regions (in order to share the codes). So how about I
rename vtd_switch_address_space() into something like
vtd_setup_address_space(), to avoid misunderstanding?

Thanks,

-- peterx