Re: [Xen-devel] [PATCH] VT-d/ATS: correct and clean up dev_invalidate_iotlb()

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Jan Beulich wrote on 2014-06-24:
> While this was intended to only do cleanup (replace the two bogus "ret |= "
> constructs, and a simple formatting correction), this now also
> - fixes the bit manipulations for size_order > 0
>   a) correct an off-by-one in the use of size_order for shifting (till
>      now double the requested size got invalidated) b) in fact setting
>      bit 12 and up if necessary (without which too small a region might
>      have got invalidated)
>   c) making them capable of dealing with regions of 4Gb size and up -
>   corrects the return value handling, such that a later iteration's
>   success won't clear an earlier iteration's error indication
> - uses PCI_BDF2() instead of open coding it
> - bail immediately on bad passed in invalidation type, rather than
>   repeatedly printing the same message for each ATS-capable device, at
>   once also no longer hiding that failure from the caller
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Yang Zhang <yang.z.zhang@xxxxxxxxx>

> ---
> Note that despite not having ATS capable hardware and hence not being 
> able to actually test the changes, I'm still certain changed the code 
> gets closer to what the spec mandates than the original one.
> 
> --- a/xen/drivers/passthrough/vtd/x86/ats.c
> +++ b/xen/drivers/passthrough/vtd/x86/ats.c
> @@ -110,21 +110,23 @@ int dev_invalidate_iotlb(struct iommu *i
>      u64 addr, unsigned int size_order, u64 type)  {
>      struct pci_ats_dev *pdev;
> -    int sbit, ret = 0;
> -    u16 sid;
> +    int ret = 0;
> 
>      if ( !ecap_dev_iotlb(iommu->ecap) )
>          return ret;
>      list_for_each_entry( pdev, &ats_devices, list )
>      {
> -        sid = (pdev->bus << 8) | pdev->devfn;
> +        u16 sid = PCI_BDF2(pdev->bus, pdev->devfn);
> +        bool_t sbit;
> +        int rc = 0;
> 
>          /* Only invalidate devices that belong to this IOMMU */
>          if ( pdev->iommu != iommu )
>              continue;
> -        switch ( type ) {
> +        switch ( type )
> +        {
>          case DMA_TLB_DSI_FLUSH:
>              if ( !device_in_domain(iommu, pdev, did) )
>                  break;
> @@ -133,32 +135,37 @@ int dev_invalidate_iotlb(struct iommu *i
>              /* invalidate all translations: sbit=1,bit_63=0,bit[62:12]=1 */
>              sbit = 1;
>              addr = (~0 << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;
> -            ret |= qinval_device_iotlb(iommu, pdev->ats_queue_depth,
> -                                       sid, sbit, addr);
> +            rc = qinval_device_iotlb(iommu, pdev->ats_queue_depth,
> +                                     sid, sbit, addr);
>              break; case DMA_TLB_PSI_FLUSH: if (
>              !device_in_domain(iommu, pdev, did) )
>                  break;
> -            addr &= ~0 << (PAGE_SHIFT + size_order);
> -
>              /* if size <= 4K, set sbit = 0, else set sbit = 1 */
>              sbit = size_order ? 1 : 0;
>              
>              /* clear lower bits */
> -            addr &= (~0 << (PAGE_SHIFT + size_order));
> +            addr &= ~0 << PAGE_SHIFT_4K;
> 
>              /* if sbit == 1, zero out size_order bit and set lower bits to 1 
> */
>              if ( sbit )
> -                addr &= (~0  & ~(1 << (PAGE_SHIFT + size_order))); +   
>         { +                addr &= ~((u64)PAGE_SIZE_4K << (size_order -
> 1)); +                addr |= (((u64)1 << (size_order - 1)) - 1) <<
> PAGE_SHIFT_4K; +            }
> 
> -            ret |= qinval_device_iotlb(iommu, pdev->ats_queue_depth,
> -                                       sid, sbit, addr);
> +            rc = qinval_device_iotlb(iommu, pdev->ats_queue_depth,
> +                                     sid, sbit, addr);
>              break; default: dprintk(XENLOG_WARNING VTDPREFIX, "invalid
>              vt-d flush
> type\n");
> -            break;
> +            return -EOPNOTSUPP;
>          }
> +
> +        if ( !ret )
> +            ret = rc;
>      } + return ret;
>  }
>


Best regards,
Yang



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