RE: [PATCH] KVM: PCIPT: VT-d: fix context mapping

[Ben-Ami Yassour <benami@il.ibm.com>]

On Thu, 2008-06-19 at 16:59 +0800, Han, Weidong wrote:
> benami@il.ibm.com wrote:
> > From: Ben-Ami Yassour <benami@il.ibm.com>
> > 
> > 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 <benami@il.ibm.com>
> > ---
> >  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?

Regards,
Ben

> 
> > +
> > +	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);