Xen devel - Dec 2012 - Ping: [PATCH] IOMMU/ATS: fix maximum queue depth calculation

Anyone? (I''d really want an ack from both Intel - who originally
contributed the ATS code - and AMD - due to the adjustment of
their later re-arrangements.)

Jan
>>> On 28.11.12 at 12:32, Jan Beulich wrote:
> The capabilities register field is a 5-bit value, and the 5 bits all
> being zero actually means 32 entries.
> 
> Under the assumption that amd_iommu_flush_iotlb() really just tried
> to correct for the miscalculation above when adding 32 to the value,
> that adjustment is also being removed.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
> @@ -321,7 +321,7 @@ void amd_iommu_flush_iotlb(struct pci_de
>  
>      req_id = get_dma_requestor_id(iommu->seg, bdf);
>      queueid = req_id;
> -    maxpend = (ats_pdev->ats_queue_depth + 32) & 0xff;
> +    maxpend = ats_pdev->ats_queue_depth & 0xff;
>  
>      /* send INVALIDATE_IOTLB_PAGES command */
>      spin_lock_irqsave(&iommu->lock, flags);
> --- a/xen/drivers/passthrough/ats.h
> +++ b/xen/drivers/passthrough/ats.h
> @@ -28,7 +28,7 @@ struct pci_ats_dev {
>  
>  #define ATS_REG_CAP    4
>  #define ATS_REG_CTL    6
> -#define ATS_QUEUE_DEPTH_MASK     0xF
> +#define ATS_QUEUE_DEPTH_MASK     0x1f
>  #define ATS_ENABLE               (1<<15)
>  
>  extern struct list_head ats_devices;
> --- a/xen/drivers/passthrough/x86/ats.c
> +++ b/xen/drivers/passthrough/x86/ats.c
> @@ -94,6 +94,8 @@ int enable_ats_device(int seg, int bus, 
>          value = pci_conf_read16(seg, bus, PCI_SLOT(devfn),
>                                  PCI_FUNC(devfn), pos + ATS_REG_CAP);
>          pdev->ats_queue_depth = value & ATS_QUEUE_DEPTH_MASK;
> +        if ( !pdev->ats_queue_depth )
> +            pdev->ats_queue_depth = ATS_QUEUE_DEPTH_MASK + 1;
>          list_add(&pdev->list, &ats_devices);
>      }
>  
> 
> 
>

Sorry for the late reply.   Originally it should be caused by mis-read about the
field''s width.   Thanks!
Acked-by Xiantao Zhang <xiantao.zhang@intel.com>
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, December 04, 2012 6:09 PM
> To: Wei Huang; Wei Wang; Zhang, Xiantao
> Cc: Boris Ostrovsky; xen-devel
> Subject: Ping: [PATCH] IOMMU/ATS: fix maximum queue depth calculation
> 
> Anyone? (I''d really want an ack from both Intel - who originally
contributed
> the ATS code - and AMD - due to the adjustment of their later re-
> arrangements.)
> 
> Jan
> 
> >>> On 28.11.12 at 12:32, Jan Beulich wrote:
> > The capabilities register field is a 5-bit value, and the 5 bits all
> > being zero actually means 32 entries.
> >
> > Under the assumption that amd_iommu_flush_iotlb() really just tried to
> > correct for the miscalculation above when adding 32 to the value, that
> > adjustment is also being removed.
> >
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> > --- a/xen/drivers/passthrough/amd/iommu_cmd.c
> > +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
> > @@ -321,7 +321,7 @@ void amd_iommu_flush_iotlb(struct pci_de
> >
> >      req_id = get_dma_requestor_id(iommu->seg, bdf);
> >      queueid = req_id;
> > -    maxpend = (ats_pdev->ats_queue_depth + 32) & 0xff;
> > +    maxpend = ats_pdev->ats_queue_depth & 0xff;
> >
> >      /* send INVALIDATE_IOTLB_PAGES command */
> >      spin_lock_irqsave(&iommu->lock, flags);
> > --- a/xen/drivers/passthrough/ats.h
> > +++ b/xen/drivers/passthrough/ats.h
> > @@ -28,7 +28,7 @@ struct pci_ats_dev {
> >
> >  #define ATS_REG_CAP    4
> >  #define ATS_REG_CTL    6
> > -#define ATS_QUEUE_DEPTH_MASK     0xF
> > +#define ATS_QUEUE_DEPTH_MASK     0x1f
> >  #define ATS_ENABLE               (1<<15)
> >
> >  extern struct list_head ats_devices;
> > --- a/xen/drivers/passthrough/x86/ats.c
> > +++ b/xen/drivers/passthrough/x86/ats.c
> > @@ -94,6 +94,8 @@ int enable_ats_device(int seg, int bus,
> >          value = pci_conf_read16(seg, bus, PCI_SLOT(devfn),
> >                                  PCI_FUNC(devfn), pos + ATS_REG_CAP);
> >          pdev->ats_queue_depth = value & ATS_QUEUE_DEPTH_MASK;
> > +        if ( !pdev->ats_queue_depth )
> > +            pdev->ats_queue_depth = ATS_QUEUE_DEPTH_MASK + 1;
> >          list_add(&pdev->list, &ats_devices);
> >      }
> >
> >
> >
> >
> 
>

Sorry for being late. Here it is.

Acked-by: Wei Huang <wei.huang2@amd.com>

Thanks,
-Wei
-----Original Message-----
From: Jan Beulich [mailto:JBeulich@suse.com] 
Sent: Tuesday, December 04, 2012 4:09 AM
To: Huang2, Wei; Wei Wang; xiantao.zhang@intel.com
Cc: Ostrovsky, Boris; xen-devel
Subject: Ping: [PATCH] IOMMU/ATS: fix maximum queue depth calculation

Anyone? (I''d really want an ack from both Intel - who originally
contributed the ATS code - and AMD - due to the adjustment of their later
re-arrangements.)

Jan
>>> On 28.11.12 at 12:32, Jan Beulich wrote:
> The capabilities register field is a 5-bit value, and the 5 bits all 
> being zero actually means 32 entries.
> 
> Under the assumption that amd_iommu_flush_iotlb() really just tried to 
> correct for the miscalculation above when adding 32 to the value, that 
> adjustment is also being removed.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
> @@ -321,7 +321,7 @@ void amd_iommu_flush_iotlb(struct pci_de
>  
>      req_id = get_dma_requestor_id(iommu->seg, bdf);
>      queueid = req_id;
> -    maxpend = (ats_pdev->ats_queue_depth + 32) & 0xff;
> +    maxpend = ats_pdev->ats_queue_depth & 0xff;
>  
>      /* send INVALIDATE_IOTLB_PAGES command */
>      spin_lock_irqsave(&iommu->lock, flags);
> --- a/xen/drivers/passthrough/ats.h
> +++ b/xen/drivers/passthrough/ats.h
> @@ -28,7 +28,7 @@ struct pci_ats_dev {
>  
>  #define ATS_REG_CAP    4
>  #define ATS_REG_CTL    6
> -#define ATS_QUEUE_DEPTH_MASK     0xF
> +#define ATS_QUEUE_DEPTH_MASK     0x1f
>  #define ATS_ENABLE               (1<<15)
>  
>  extern struct list_head ats_devices;
> --- a/xen/drivers/passthrough/x86/ats.c
> +++ b/xen/drivers/passthrough/x86/ats.c
> @@ -94,6 +94,8 @@ int enable_ats_device(int seg, int bus, 
>          value = pci_conf_read16(seg, bus, PCI_SLOT(devfn),
>                                  PCI_FUNC(devfn), pos + ATS_REG_CAP);
>          pdev->ats_queue_depth = value & ATS_QUEUE_DEPTH_MASK;
> +        if ( !pdev->ats_queue_depth )
> +            pdev->ats_queue_depth = ATS_QUEUE_DEPTH_MASK + 1;
>          list_add(&pdev->list, &ats_devices);
>      }
>  
> 
> 
>