Re: [Xen-devel] [Patch RFC 03/13] vt-d: Track the Device-TLB invalidation status in an invalidation table.

["Xu, Quan" <quan.xu@xxxxxxxxx>]

>> >>> On 29.09.2015 at 17:24 <JBeulich@xxxxxxxx> wrote:
> >>> On 16.09.15 at 15:23, <quan.xu@xxxxxxxxx> wrote:
> > @@ -139,6 +140,7 @@ static int queue_invalidate_wait(struct iommu *iommu,
> >      unsigned long flags;
> >      u64 entry_base;
> >      struct qinval_entry *qinval_entry, *qinval_entries;
> > +    struct domain *d;
> >
> >      spin_lock_irqsave(&iommu->register_lock, flags);
> >      index = qinval_next_index(iommu); @@ -152,9 +154,22 @@ static int
> > queue_invalidate_wait(struct iommu *iommu,
> >      qinval_entry->q.inv_wait_dsc.lo.sw = sw;
> >      qinval_entry->q.inv_wait_dsc.lo.fn = fn;
> >      qinval_entry->q.inv_wait_dsc.lo.res_1 = 0;
> > -    qinval_entry->q.inv_wait_dsc.lo.sdata = QINVAL_STAT_DONE;
> >      qinval_entry->q.inv_wait_dsc.hi.res_1 = 0;
> > -    qinval_entry->q.inv_wait_dsc.hi.saddr = virt_to_maddr(&poll_slot) >> 2;
> > +
> > +    if ( iflag )
> > +    {
> > +        d = rcu_lock_domain_by_id(iommu->domid_map[device_id]);
> > +        if ( d == NULL )
> > +            return -ENODATA;
> > +
> > +        qinval_entry->q.inv_wait_dsc.lo.sdata = ++ qi_table_data(d);
> 
> Stray blank following the ++.
> 

I will modify it in next version.

> > +        qinval_entry->q.inv_wait_dsc.hi.saddr = virt_to_maddr(
> > +
> &qi_table_pollslot(d)) >> 2;
> > +        rcu_unlock_domain(d);
> 
> If you don't hold a reference to the domain, what prevents it from going away,
> struct domain getting freed, and the write to the poll slot corrupting data 
> if the
> memory gets re-used? Plus, if you obtain a domain reference at the time you
> enter it into domid_map[], you wouldn't have to be worried about failure above
> (i.e. you could simply ASSERT() success of rcu_lock_domain_by_id()).
> 
> Considering the implementation of rcu_lock_domain_by_id() I also wonder
> whether it wouldn't be more efficient to make domid_map[] an array of struct
> domain pointers.
> 
Good catch!!
Patch #11 can prevents it from going away.
patch 11: 
##
If the Device-TLB flush is still not completed, schedule and wait on a 
waitqueue until the Device-TLB flush is 
Completed, before schedule RCU asynchronous completion of domain destroy.
##


And,
 If it has got domain by rcu_lock_domain_by_id(), the domain structure is 
protected by RCU lock from rcu_lock_domain_by_id() to rcu_lock_domain_by_id().




> > +    } else {
> 
> Coding style.
> 

I will modify it in next version.



> > --- a/xen/include/xen/hvm/iommu.h
> > +++ b/xen/include/xen/hvm/iommu.h
> > @@ -23,6 +23,21 @@
> >  #include <xen/list.h>
> >  #include <asm/hvm/iommu.h>
> >
> > +/*
> > + * Status Address and Data: Status address and data is used by
> > +hardware to perform
> > + * wait descriptor completion status write when the Status Write(SW) field 
> > is
> Set.
> > + *
> > + * Track the Device-TLB invalidation status in an invalidation table.
> > +Update
> > + * invalidation table's count of in-flight Device-TLB invalidation
> > +request and
> > + * assign the address of global polling parameter per domain in the
> > +Status Address
> > + * of each invalidation wait descriptor, when submit Device-TLB
> > +invalidation
> > + * requests.
> > + */
> > +struct qi_talbe {
> > +    u64 qi_table_poll_slot;
> 
> Why is this a 64-bit field when the respective write stores 32 bits only?
> 
Yes, it should be u32. It is a DWORD.
Invalidation Wait Descriptor's Status Address[63:2] would be very likely to 
mislead me.


> Also the qi_table_ prefixes seem rather pointless.
> 

I will modify it in next version.



Quan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel