* [PATCH] VT-d: make flush-all actually flush all
@ 2015-12-09 14:52 Jan Beulich
2015-12-09 16:00 ` Andrew Cooper
2015-12-10 3:06 ` Wu, Feng
0 siblings, 2 replies; 6+ messages in thread
From: Jan Beulich @ 2015-12-09 14:52 UTC (permalink / raw)
To: xen-devel; +Cc: Kevin Tian, Feng Wu
[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]
VT-d: make flush-all actually flush all
Passing gfn=0 and page_count=0 actually avoids the
iommu_flush_iotlb_dsi() and results in page-specific invalidation
instead.
Reported-by: "张智" <zhangzhi2014@caep.cn>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -583,7 +583,7 @@ static void __intel_iommu_iotlb_flush(st
if ( iommu_domid == -1 )
continue;
- if ( page_count > 1 || gfn == -1 )
+ if ( page_count != 1 || gfn == INVALID_GFN )
{
if ( iommu_flush_iotlb_dsi(iommu, iommu_domid,
0, flush_dev_iotlb) )
@@ -592,7 +592,7 @@ static void __intel_iommu_iotlb_flush(st
else
{
if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
- (paddr_t)gfn << PAGE_SHIFT_4K, 0,
+ (paddr_t)gfn << PAGE_SHIFT_4K, PAGE_ORDER_4K,
!dma_old_pte_present, flush_dev_iotlb) )
iommu_flush_write_buffer(iommu);
}
@@ -606,7 +606,7 @@ static void intel_iommu_iotlb_flush(stru
static void intel_iommu_iotlb_flush_all(struct domain *d)
{
- __intel_iommu_iotlb_flush(d, 0, 0, 0);
+ __intel_iommu_iotlb_flush(d, INVALID_GFN, 0, 0);
}
/* clear one page's page table */
[-- Attachment #2: VT-d-flush-all.patch --]
[-- Type: text/plain, Size: 1385 bytes --]
VT-d: make flush-all actually flush all
Passing gfn=0 and page_count=0 actually avoids the
iommu_flush_iotlb_dsi() and results in page-specific invalidation
instead.
Reported-by: "å¼ æ™º" <zhangzhi2014@caep.cn>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -583,7 +583,7 @@ static void __intel_iommu_iotlb_flush(st
if ( iommu_domid == -1 )
continue;
- if ( page_count > 1 || gfn == -1 )
+ if ( page_count != 1 || gfn == INVALID_GFN )
{
if ( iommu_flush_iotlb_dsi(iommu, iommu_domid,
0, flush_dev_iotlb) )
@@ -592,7 +592,7 @@ static void __intel_iommu_iotlb_flush(st
else
{
if ( iommu_flush_iotlb_psi(iommu, iommu_domid,
- (paddr_t)gfn << PAGE_SHIFT_4K, 0,
+ (paddr_t)gfn << PAGE_SHIFT_4K, PAGE_ORDER_4K,
!dma_old_pte_present, flush_dev_iotlb) )
iommu_flush_write_buffer(iommu);
}
@@ -606,7 +606,7 @@ static void intel_iommu_iotlb_flush(stru
static void intel_iommu_iotlb_flush_all(struct domain *d)
{
- __intel_iommu_iotlb_flush(d, 0, 0, 0);
+ __intel_iommu_iotlb_flush(d, INVALID_GFN, 0, 0);
}
/* clear one page's page table */
[-- 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] 6+ messages in thread* Re: [PATCH] VT-d: make flush-all actually flush all
2015-12-09 14:52 [PATCH] VT-d: make flush-all actually flush all Jan Beulich
@ 2015-12-09 16:00 ` Andrew Cooper
2015-12-10 3:06 ` Wu, Feng
1 sibling, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2015-12-09 16:00 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Kevin Tian, Feng Wu
On 09/12/15 14:52, Jan Beulich wrote:
> VT-d: make flush-all actually flush all
>
> Passing gfn=0 and page_count=0 actually avoids the
> iommu_flush_iotlb_dsi() and results in page-specific invalidation
> instead.
>
> Reported-by: "张智" <zhangzhi2014@caep.cn>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] VT-d: make flush-all actually flush all
2015-12-09 14:52 [PATCH] VT-d: make flush-all actually flush all Jan Beulich
2015-12-09 16:00 ` Andrew Cooper
@ 2015-12-10 3:06 ` Wu, Feng
2015-12-10 7:39 ` Jan Beulich
1 sibling, 1 reply; 6+ messages in thread
From: Wu, Feng @ 2015-12-10 3:06 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Tian, Kevin, Wu, Feng
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, December 9, 2015 10:53 PM
> To: xen-devel <xen-devel@lists.xenproject.org>
> Cc: Wu, Feng <feng.wu@intel.com>; Tian, Kevin <kevin.tian@intel.com>
> Subject: [PATCH] VT-d: make flush-all actually flush all
>
> VT-d: make flush-all actually flush all
>
> Passing gfn=0 and page_count=0 actually avoids the
> iommu_flush_iotlb_dsi() and results in page-specific invalidation
> instead.
>
> Reported-by: "张智" <zhangzhi2014@caep.cn>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -583,7 +583,7 @@ static void __intel_iommu_iotlb_flush(st
> if ( iommu_domid == -1 )
> continue;
>
> - if ( page_count > 1 || gfn == -1 )
> + if ( page_count != 1 || gfn == INVALID_GFN )
This patch looks good me, but I think using 'page_count' to decide
whether using dsi or psi is not a good idea, since psi should also support
invalidate multiple pages from VT-d Spec. (Seems no support in Xen?)
Thanks,
Feng
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] VT-d: make flush-all actually flush all
2015-12-10 3:06 ` Wu, Feng
@ 2015-12-10 7:39 ` Jan Beulich
2015-12-10 8:29 ` Wu, Feng
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2015-12-10 7:39 UTC (permalink / raw)
To: Feng Wu; +Cc: xen-devel, Kevin Tian
>>> On 10.12.15 at 04:06, <feng.wu@intel.com> wrote:
>
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Wednesday, December 9, 2015 10:53 PM
>> To: xen-devel <xen-devel@lists.xenproject.org>
>> Cc: Wu, Feng <feng.wu@intel.com>; Tian, Kevin <kevin.tian@intel.com>
>> Subject: [PATCH] VT-d: make flush-all actually flush all
>>
>> VT-d: make flush-all actually flush all
>>
>> Passing gfn=0 and page_count=0 actually avoids the
>> iommu_flush_iotlb_dsi() and results in page-specific invalidation
>> instead.
>>
>> Reported-by: "张智" <zhangzhi2014@caep.cn>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -583,7 +583,7 @@ static void __intel_iommu_iotlb_flush(st
>> if ( iommu_domid == -1 )
>> continue;
>>
>> - if ( page_count > 1 || gfn == -1 )
>> + if ( page_count != 1 || gfn == INVALID_GFN )
>
> This patch looks good me, but I think using 'page_count' to decide
> whether using dsi or psi is not a good idea, since psi should also support
> invalidate multiple pages from VT-d Spec. (Seems no support in Xen?)
I'm fine with this getting improved in a subsequent patch, but I
don't see this to be done here - what you propose is an
enhancement, while here I'm fixing a latent bug (which originally
got reported to security@, and we were able to discard security
concerns merely because the sole intel_iommu_iotlb_flush_all()
caller sits on a code path reachable only through an XSA-77
covered domctl). The more that there currently is no caller
passing in other than 0 or 1.
In an ideal world, my expectation really would have been for
you to ack this change (of course unless you see anything
actively wrong with it) and immediately follow it up with the
described improvement (with the caveat that - see above -
you'd have difficulty actually testing such a change).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] VT-d: make flush-all actually flush all
2015-12-10 7:39 ` Jan Beulich
@ 2015-12-10 8:29 ` Wu, Feng
2015-12-10 9:24 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Wu, Feng @ 2015-12-10 8:29 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Tian, Kevin, Wu, Feng
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, December 10, 2015 3:39 PM
> To: Wu, Feng <feng.wu@intel.com>
> Cc: Tian, Kevin <kevin.tian@intel.com>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: RE: [PATCH] VT-d: make flush-all actually flush all
>
> >>> On 10.12.15 at 04:06, <feng.wu@intel.com> wrote:
>
> >
> >> -----Original Message-----
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Wednesday, December 9, 2015 10:53 PM
> >> To: xen-devel <xen-devel@lists.xenproject.org>
> >> Cc: Wu, Feng <feng.wu@intel.com>; Tian, Kevin <kevin.tian@intel.com>
> >> Subject: [PATCH] VT-d: make flush-all actually flush all
> >>
> >> VT-d: make flush-all actually flush all
> >>
> >> Passing gfn=0 and page_count=0 actually avoids the
> >> iommu_flush_iotlb_dsi() and results in page-specific invalidation
> >> instead.
> >>
> >> Reported-by: "张智" <zhangzhi2014@caep.cn>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> --- a/xen/drivers/passthrough/vtd/iommu.c
> >> +++ b/xen/drivers/passthrough/vtd/iommu.c
> >> @@ -583,7 +583,7 @@ static void __intel_iommu_iotlb_flush(st
> >> if ( iommu_domid == -1 )
> >> continue;
> >>
> >> - if ( page_count > 1 || gfn == -1 )
> >> + if ( page_count != 1 || gfn == INVALID_GFN )
> >
> > This patch looks good me, but I think using 'page_count' to decide
> > whether using dsi or psi is not a good idea, since psi should also support
> > invalidate multiple pages from VT-d Spec. (Seems no support in Xen?)
>
> I'm fine with this getting improved in a subsequent patch, but I
> don't see this to be done here - what you propose is an
> enhancement, while here I'm fixing a latent bug (which originally
> got reported to security@, and we were able to discard security
> concerns merely because the sole intel_iommu_iotlb_flush_all()
> caller sits on a code path reachable only through an XSA-77
> covered domctl). The more that there currently is no caller
> passing in other than 0 or 1.
In intel_iommu_iotlb_flush_all(), 0 is passed in as the 'page_count',
but intel_iommu_iotlb_flush() can pass in a value more than 1
for 'page_count', right?
>
> In an ideal world, my expectation really would have been for
> you to ack this change (of course unless you see anything
> actively wrong with it) and immediately follow it up with the
> described improvement (with the caveat that - see above -
> you'd have difficulty actually testing such a change).
Acked-by: Feng Wu <feng.wu@intel.com>
Thanks,
Feng
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] VT-d: make flush-all actually flush all
2015-12-10 8:29 ` Wu, Feng
@ 2015-12-10 9:24 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2015-12-10 9:24 UTC (permalink / raw)
To: Feng Wu; +Cc: xen-devel, Kevin Tian
>>> On 10.12.15 at 09:29, <feng.wu@intel.com> wrote:
>
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Thursday, December 10, 2015 3:39 PM
>> To: Wu, Feng <feng.wu@intel.com>
>> Cc: Tian, Kevin <kevin.tian@intel.com>; xen-devel <xen-
>> devel@lists.xenproject.org>
>> Subject: RE: [PATCH] VT-d: make flush-all actually flush all
>>
>> >>> On 10.12.15 at 04:06, <feng.wu@intel.com> wrote:
>> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> Sent: Wednesday, December 9, 2015 10:53 PM
>> >> --- a/xen/drivers/passthrough/vtd/iommu.c
>> >> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> >> @@ -583,7 +583,7 @@ static void __intel_iommu_iotlb_flush(st
>> >> if ( iommu_domid == -1 )
>> >> continue;
>> >>
>> >> - if ( page_count > 1 || gfn == -1 )
>> >> + if ( page_count != 1 || gfn == INVALID_GFN )
>> >
>> > This patch looks good me, but I think using 'page_count' to decide
>> > whether using dsi or psi is not a good idea, since psi should also support
>> > invalidate multiple pages from VT-d Spec. (Seems no support in Xen?)
>>
>> I'm fine with this getting improved in a subsequent patch, but I
>> don't see this to be done here - what you propose is an
>> enhancement, while here I'm fixing a latent bug (which originally
>> got reported to security@, and we were able to discard security
>> concerns merely because the sole intel_iommu_iotlb_flush_all()
>> caller sits on a code path reachable only through an XSA-77
>> covered domctl). The more that there currently is no caller
>> passing in other than 0 or 1.
>
> In intel_iommu_iotlb_flush_all(), 0 is passed in as the 'page_count',
> but intel_iommu_iotlb_flush() can pass in a value more than 1
> for 'page_count', right?
Ah, yes, it being the .iotlb_flush handler.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-12-10 9:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-09 14:52 [PATCH] VT-d: make flush-all actually flush all Jan Beulich
2015-12-09 16:00 ` Andrew Cooper
2015-12-10 3:06 ` Wu, Feng
2015-12-10 7:39 ` Jan Beulich
2015-12-10 8:29 ` Wu, Feng
2015-12-10 9:24 ` Jan Beulich
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.