Ben-Ami Yassour1 wrote:
> "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?
>
I don't think it's necessary to add kvm_intel_iommu_detach_dev() here.
If a device can be assigned to a guest, it should not be used by other
guest (assuming no hotplug support). And also
kvm_intel_iommu_detach_dev() is already called in
kvm_iommu_unmap_guest(). Normally context won't be present here.
Otherwise, there should be some wrong. I attach a patch to change it
back to original kernel VT-d code. I think it's correct and clean.
diff --git a/drivers/pci/intel-iommu.c b/drivers/pci/intel-iommu.c
index 930874f..04517a2 100644
--- a/drivers/pci/intel-iommu.c
+++ b/drivers/pci/intel-iommu.c
@@ -1264,8 +1264,11 @@ static int domain_context_mapping_one(struct
dmar_domain *domain,
if (!context)
return -ENOMEM;
spin_lock_irqsave(&iommu->lock, flags);
+ if (context_present(*context)) {
+ spin_unlock_irqrestore(&iommu->lock, flags);
+ return 0;
+ }
- context_clear_entry(*context);
context_set_domain_id(*context, domain->id);
context_set_address_width(*context, domain->agaw);
context_set_address_root(*context, virt_to_phys(domain->pgd));
Randy (Weidong)
--
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
