Re: [Xen-devel] [Patch v11 1/3] IOMMU: add a timeout parameter for device IOTLB invalidation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.06.16 at 11:05, <quan.xu@xxxxxxxxx> wrote:
> From: Quan Xu <quan.xu@xxxxxxxxx>
> 
> The parameter 'iommu_dev_iotlb_timeout' specifies the timeout of
> the device IOTLB invalidation in milliseconds. By default, the
> timeout is 1ms, which can be boot-time changed.
> 
> Add a __must_check annotation. The followup patch titled
> 'VT-d IOTLB/Context/IEC flush issue' addresses the __mustcheck.
> That is the other callers of this routine (two or three levels up)
> ignore the return code. This patch does not address this but the
> other does.

The description really needs to mention that the dropping of
the panic() is intentional, and why.

> v11: Change the timeout parameter from 'vtd_qi_timeout' to
>     'iommu_dev_iotlb_timeout', which is not only for VT-d device
>     IOTLB invalidation, but also for other IOMMU implementations.

This goes after the first --- separator.

> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -996,6 +996,15 @@ debug hypervisor only).
>  
>  >> Enable IOMMU debugging code (implies `verbose`).
>  
> +### iommu\_dev\_iotlb\_timeout
> +> `= <integer>`
> +
> +> Default: `1`

So on v10 I had made clear that any timeout reduction from its
current value is, for the ATS case, not acceptable, unless you have
proof that this lower value will fit all past, present, and future
devices. Otherwise we're risking a regression here.

> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -28,6 +28,8 @@
>  #include "vtd.h"
>  #include "extern.h"
>  
> +#define IOMMU_QI_TIMEOUT MILLISECS(1)

May I suggest VTD_QI_TIMEOUT (but see also below)?

> @@ -163,14 +165,21 @@ static int queue_invalidate_wait(struct iommu *iommu,
>      /* Now we don't support interrupt method */
>      if ( sw )
>      {
> +        s_time_t timeout;
> +
>          /* In case all wait descriptor writes to same addr with same data */
> -        start_time = NOW();
> +        timeout = flush_dev_iotlb ?
> +                  (NOW() + iommu_dev_iotlb_timeout * MILLISECS(1)) :

MILLISECS(iommu_dev_iotlb_timeout)

> +                  (NOW() + IOMMU_QI_TIMEOUT);

Or really the whole expression should probably simply become

        timeout = NOW() + MILLISECS(flush_dev_iotlb ? iommu_dev_iotlb_timeout : 
VTD_QI_TIMEOUT);

(of course with VTD_QI_TIMEOUT having its MILLISECS() dropped,
and suitably line wrapped).

> @@ -344,10 +355,11 @@ static int __must_check flush_iotlb_qi(void *_iommu, 
> u16 did, u64 addr,
>                                 dw, did, size_order, 0, addr);
>          if ( flush_dev_iotlb )
>              ret = dev_invalidate_iotlb(iommu, did, addr, size_order, type);
> -        rc = invalidate_sync(iommu);
> +        rc = invalidate_sync(iommu, flush_dev_iotlb);
>          if ( !ret )
>              ret = rc;
>      }
> +
>      return ret;

Once again an addition of a blank line that doesn't belong here.

Jan


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