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
