* [PATCH] VT-d/ATS: correct and clean up dev_invalidate_iotlb()
@ 2014-06-24 14:25 Jan Beulich
2014-06-24 16:09 ` Andrew Cooper
2014-06-30 8:35 ` Zhang, Yang Z
0 siblings, 2 replies; 5+ messages in thread
From: Jan Beulich @ 2014-06-24 14:25 UTC (permalink / raw)
To: xen-devel; +Cc: Yang Z Zhang
[-- Attachment #1: Type: text/plain, Size: 3859 bytes --]
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@suse.com>
---
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;
}
[-- Attachment #2: VT-d-ATS-invalidate-cleanup.patch --]
[-- Type: text/plain, Size: 3910 bytes --]
VT-d/ATS: correct and clean up dev_invalidate_iotlb()
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@suse.com>
---
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;
}
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] VT-d/ATS: correct and clean up dev_invalidate_iotlb()
2014-06-24 14:25 [PATCH] VT-d/ATS: correct and clean up dev_invalidate_iotlb() Jan Beulich
@ 2014-06-24 16:09 ` Andrew Cooper
2014-06-25 10:49 ` Jan Beulich
2014-06-30 8:35 ` Zhang, Yang Z
1 sibling, 1 reply; 5+ messages in thread
From: Andrew Cooper @ 2014-06-24 16:09 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Yang Z Zhang
[-- Attachment #1.1: Type: text/plain, Size: 4190 bytes --]
On 24/06/14 15:25, Jan Beulich wrote:
> 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@suse.com>
> ---
> 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;
Doesn't this need to be ~0ULL as addr is u64?
~Andrew
>
> /* 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;
> }
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
[-- Attachment #1.2: Type: text/html, Size: 4971 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] VT-d/ATS: correct and clean up dev_invalidate_iotlb()
2014-06-24 16:09 ` Andrew Cooper
@ 2014-06-25 10:49 ` Jan Beulich
2014-06-25 12:50 ` Andrew Cooper
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2014-06-25 10:49 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Yang Z Zhang, xen-devel
>>> On 24.06.14 at 18:09, <andrew.cooper3@citrix.com> wrote:
> On 24/06/14 15:25, Jan Beulich wrote:
>> @@ -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;
>
> Doesn't this need to be ~0ULL as addr is u64?
No (or else further up in the code shown as context the same
would need to be done): Conversion here goes via sign-extension
(effectively int -> int64_t -> uint64_t).
Jan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] VT-d/ATS: correct and clean up dev_invalidate_iotlb()
2014-06-25 10:49 ` Jan Beulich
@ 2014-06-25 12:50 ` Andrew Cooper
0 siblings, 0 replies; 5+ messages in thread
From: Andrew Cooper @ 2014-06-25 12:50 UTC (permalink / raw)
To: Jan Beulich; +Cc: Yang Z Zhang, xen-devel
On 25/06/14 11:49, Jan Beulich wrote:
>>>> On 24.06.14 at 18:09, <andrew.cooper3@citrix.com> wrote:
>> On 24/06/14 15:25, Jan Beulich wrote:
>>> @@ -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;
>> Doesn't this need to be ~0ULL as addr is u64?
> No (or else further up in the code shown as context the same
> would need to be done): Conversion here goes via sign-extension
> (effectively int -> int64_t -> uint64_t).
>
> Jan
>
Ok, in which case Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] VT-d/ATS: correct and clean up dev_invalidate_iotlb()
2014-06-24 14:25 [PATCH] VT-d/ATS: correct and clean up dev_invalidate_iotlb() Jan Beulich
2014-06-24 16:09 ` Andrew Cooper
@ 2014-06-30 8:35 ` Zhang, Yang Z
1 sibling, 0 replies; 5+ messages in thread
From: Zhang, Yang Z @ 2014-06-30 8:35 UTC (permalink / raw)
To: Jan Beulich, xen-devel
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@suse.com>
Acked-by: Yang Zhang <yang.z.zhang@intel.com>
> ---
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-06-30 8:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-24 14:25 [PATCH] VT-d/ATS: correct and clean up dev_invalidate_iotlb() Jan Beulich
2014-06-24 16:09 ` Andrew Cooper
2014-06-25 10:49 ` Jan Beulich
2014-06-25 12:50 ` Andrew Cooper
2014-06-30 8:35 ` Zhang, Yang Z
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.