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?
Randy (Weidong)
>
>>
>>> +
>>> + spin_unlock_irqrestore(&iommu->lock, flags);
>>> +
>>> + detach_domain_for_dev(domain, bus, devfn);
>>>
>>> + spin_lock_irqsave(&iommu->lock, flags);
>>> +
>>> context_clear_entry(*context);
>>> context_set_domain_id(*context, domain->id);
>>> context_set_address_width(*context, domain->agaw);
