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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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