"Han, Weidong" <[EMAIL PROTECTED]> wrote on 19/06/2008 17:18:00:
> Ben-Ami Yassour wrote:
> > On Thu, 2008-06-19 at 16:59 +0800, Han, Weidong wrote:
> >> [EMAIL PROTECTED] wrote:
> >>> From: Ben-Ami Yassour <[EMAIL PROTECTED]>
> >>>
> >>> When changing the VT-d context mapping, according to the spec, it
> >>> is required to first set the context to not present, flush and only
> >>> then apply the new context.
> >>>
> >>> Signed-off-by: Ben-Ami Yassour <[EMAIL PROTECTED]>
> >>> ---
> >>> drivers/pci/intel-iommu.c | 17 +++++++++++++++++
> >>> 1 files changed, 17 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
> >>> index 930874f..dcdfa97 100644 --- a/drivers/pci/intel-iommu.c
> >>> +++ b/drivers/pci/intel-iommu.c
> >>> @@ -56,6 +56,7 @@
> >>>
> >>>
> >>> static void flush_unmaps_timeout(unsigned long data);
> >>> +static void detach_domain_for_dev(struct dmar_domain *domain, u8
> >>> bus, u8 devfn);
> >>>
> >>> DEFINE_TIMER(unmap_timer, flush_unmaps_timeout, 0, 0);
> >>>
> >>> @@ -1264,7 +1265,23 @@ static int domain_context_mapping_one(struct
> >>> dmar_domain *domain, if (!context)
> >>> return -ENOMEM;
> >>> spin_lock_irqsave(&iommu->lock, flags);
> >>> +
> >>> + if (context_present(*context) &&
> >>> + (context_domain_id(*context) == domain->id) &&
> >>> + (context_address_width(*context) == domain->agaw) &&
> >>> + (context_address_root(*context) == virt_to_phys(domain->pgd))
> >>> && + (context_translation_type(*context) ==
> >>> CONTEXT_TT_MULTI_LEVEL) && +
> >>> (!context_fault_disable(*context))) {
> >>> + spin_unlock_irqrestore(&iommu->lock, flags); + return 0;
> >>> + }
> >>
> >> Only need to check context_present(*context) according to VT-d spec,
> >> which says "software must not modify fields other than the Present
> >> (P) field of currently present root-entries or context-entries."
> >>
> >> Randy (Weidong)
> >
> > The logic here is that, if no change is made to the context then just
> > return ok (0). Otherwise, according to the spec, we need to first
> > invalidate the context, flush it, and only then apply the changes to
> > the context.
> >
> > The other option is to make sure that before every call to this
> > function the context is invalidated, but disabling it inside the
> > function seems safer. do you agree with that?
> >
>
> After a device can be assigned to guest with VT-d, it needs a
> context unmap function. When a device is assigned to a guest, map
> context for it, while when it is detached from a guest, should unmap
> its context. With the context unmap function, I think we needn't to
> implement your logic in domain_context_mapping_one(), instead just
> check its present. What's your opinion?
Sure, that's fine, this is the other option I mentioned.
But we need to add the context unmap function.
Something like:
diff --git a/arch/x86/kvm/vtd.c b/arch/x86/kvm/vtd.c
index be775cd..4b96fbb 100644
--- a/arch/x86/kvm/vtd.c
+++ b/arch/x86/kvm/vtd.c
@@ -109,11 +109,17 @@ found:
kvm_iommu_unmap_memslots(kvm);
return -EFAULT;
}
+
+ kvm_intel_iommu_detach_dev(kvm->arch.domain,
+ pdev->bus->number, pdev->devfn);
+
kvm_intel_iommu_context_mapping(kvm->arch.domain, pdev);
return 0;
}
EXPORT_SYMBOL_GPL(kvm_iommu_map_guest);
Agree?
Thanks,
Ben
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
