Re: [Xen-devel] [PATCH v8 2/3] VT-d: Wrap a _sync version for all VT-d flush interfaces

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Thu, 2016-03-24 at 13:57 +0800, Quan Xu wrote:
> For consistency, we wrap a _sync version for all VT-d flush
> interfaces.
>
I'm sorry, maybe it's me, but "for consistency" with what?

I see from where this comes, if I look at v7. But when this patch will
be committed, what it is doing and why we decided to do it should be
evident by just reading the changelog, without having to google for the
review history.

So, please, try to describe the situation a little bit better (e.g., do
we have inconsistencies, right now? Is this needed for avoiding
introducing inconsistencies by means of this series? Etc.).

> --- a/xen/drivers/passthrough/vtd/qinval.c
> +++ b/xen/drivers/passthrough/vtd/qinval.c
> @@ -72,6 +72,70 @@ static void qinval_update_qtail(struct iommu
> *iommu, unsigned int index)
>      dmar_writeq(iommu->reg, DMAR_IQT_REG, (val <<
> QINVAL_INDEX_SHIFT));
>  }
>  
> +static int __must_check queue_invalidate_wait(struct iommu *iommu,
> +    u8 iflag, u8 sw, u8 fn)
> +{
>
It looks like you are "just" moving this function, without making any
change to the code, is this the case?

Assuming it is, mixing pure code movement and functional changes in the
same patch makes reviewing the patch itself harder. And pure code motio
is also bad for archaeologists (`git blame' would point at this commit
for all of this function!).

So, I'd say either isolate the code movement in a pre-patch, or try
using forward declarations. Given how moving messes up history, my
personal preference would be for the latter.

> +static int invalidate_sync(struct iommu *iommu)
> +{
> +    struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
> +
> +    if ( qi_ctrl->qinval_maddr )
> +        return queue_invalidate_wait(iommu, 0, 1, 1);
> +
> +    return 0;
> +}

Same for this, even worse, in terms of how hard this makes to review
this patch, as can be seen...

> @@ -135,65 +208,12 @@ static void queue_invalidate_iotlb(struct iommu
> *iommu,
>
> -static int invalidate_sync(struct iommu *iommu)
> +static int queue_invalidate_iotlb_sync(struct iommu *iommu,
> +    u8 granu, u8 dr, u8 dw, u16 did, u8 am, u8 ih, u64 addr)
>  {
> -    struct qi_ctrl *qi_ctrl = iommu_qi_ctrl(iommu);
> +    queue_invalidate_iotlb(iommu, granu, dr, dw, did, am, ih, addr);
>  
> -    if ( qi_ctrl->qinval_maddr )
> -        return queue_invalidate_wait(iommu, 0, 1, 1);
> -    return 0;
> +    return invalidate_sync(iommu);
>  }
>  
...here!

The rest of this patch looks fine to me, with only one more doubt.
Here:

> --- a/xen/drivers/passthrough/vtd/x86/ats.c
> +++ b/xen/drivers/passthrough/vtd/x86/ats.c
> @@ -118,7 +118,6 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16
> did,
>      {
>          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 )
> @@ -134,8 +133,8 @@ int dev_invalidate_iotlb(struct iommu *iommu, u16
> did,
>              /* invalidate all translations:
> sbit=1,bit_63=0,bit[62:12]=1 */
>              sbit = 1;
>              addr = (~0UL << PAGE_SHIFT_4K) & 0x7FFFFFFFFFFFFFFF;
> -            rc = qinval_device_iotlb(iommu, pdev->ats_queue_depth,
> -                                     sid, sbit, addr);
> +            ret = qinval_device_iotlb_sync(iommu, pdev-
> >ats_queue_depth,
> +                                           sid, sbit, addr);
>              break;
>          case DMA_TLB_PSI_FLUSH:
>              if ( !device_in_domain(iommu, pdev, did) )
> @@ -154,16 +153,13 @@ int dev_invalidate_iotlb(struct iommu *iommu,
> u16 did,
>                  addr |= (((u64)1 << (size_order - 1)) - 1) <<
> PAGE_SHIFT_4K;
>              }
>  
> -            rc = qinval_device_iotlb(iommu, pdev->ats_queue_depth,
> -                                     sid, sbit, addr);
> +            ret = qinval_device_iotlb_sync(iommu, pdev-
> >ats_queue_depth,
> +                                           sid, sbit, addr);
>              break;
>          default:
>              dprintk(XENLOG_WARNING VTDPREFIX, "invalid vt-d flush
> type\n");
>              return -EOPNOTSUPP;
>          }
> -
> -        if ( !ret )
> -            ret = rc;
>      }
>  
>      return ret;
>
Am I misreading something or we are introducing synchronous handling,
which was not there before?

If yes, is it ok to do this in this patch?

And if yes again, I think that it at least should be noted in the
changelog, as it would mean that the patch is not only introducing some
wrappers.

Regads,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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