Re: [Xen-devel] [PATCH v5 4/7] VT-d: Refactor iommu_ops .iotlb_flush() and iotlb_flush_all()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.02.16 at 11:18, <quan.xu@xxxxxxxxx> wrote:

(Side note: The VT-d: prefix of the subject seems inadequate here.)

> to pass down a flag indicating whether the lock is being held.

"the lock" being which one?

> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1100,7 +1100,7 @@ tlbflush:
>      if ( flush )
>      {
>          flush_tlb_domain(d);
> -        iommu_iotlb_flush(d, sgfn, egfn - sgfn);
> +        iommu_iotlb_flush(d, sgfn, egfn - sgfn, NONE_LOCK);
>      }
>  
>  out:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -631,9 +631,9 @@ static int xenmem_add_to_physmap(struct domain *d,
>      if ( need_iommu(d) )
>      {
>          this_cpu(iommu_dont_flush_iotlb) = 0;
> -        rc = iommu_iotlb_flush(d, xatp->idx - done, done);
> +        rc = iommu_iotlb_flush(d, xatp->idx - done, done, NONE_LOCK);
>          if ( !rc )
> -            rc = iommu_iotlb_flush(d, xatp->gpfn - done, done);
> +            rc = iommu_iotlb_flush(d, xatp->gpfn - done, done, NONE_LOCK);
>      }

Considering both of these changes, I think it would be better to
hide this implementation detail from callers outside of
xen/drivers/passthrough/, by making iommu_iotlb_flush() a
wrapper around the actual implementation. (This at once would
limit the amount of ack-s such a patch will require.)

> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -131,6 +131,13 @@ struct page_info;
>   * callback pair.
>   */
>  typedef int iommu_grdm_t(xen_pfn_t start, xen_ulong_t nr, u32 id, void 
> *ctxt);
> +/*
> + * A flag indicates whether the lock is being held.
> + * NONE_LOCK - no lock is being held.
> + * PCIDEVS_LOCK - pcidevs_lock is being held.
> + */
> +#define NONE_LOCK 0
> +#define PCIDEVS_LOCK 1

If you really assume further lock holding might need to be
communicated here, this should be made more obviously a bit
mask (by e.g. using hex constants, not having a NON_LOCKS
constant, and not making the first part of the comment refer to
just one lock). If, however, the pcidevs_lock is all you care
about, I think code readability would benefit from making the
respective function parameters bool_t, and adding (besides the
already mentioned wrapper) another wrapper named e.g.
iommu_iotlb_flush_all_locked().

Jan


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