[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [Patch RFC 03/13] vt-d: Track the Device-TLB invalidation status in an invalidation table.
>>> 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 ++. > + 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. > + } else { Coding style. > --- 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? Also the qi_table_ prefixes seem rather pointless. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |