Re: [Xen-devel] [PATCH v8 09/11] vt-d: fix the IOMMU flush issue
> From: Xu, Quan
> Sent: Tuesday, June 14, 2016 5:04 PM
>
> On June 14, 2016 4:27 PM, Jan Beulich wrote:
> > >>> On 14.06.16 at 10:10, wrote:
> > > On June 13, 2016 11:52 PM, Jan Beulich wrote:
> > >> >>> "Xu, Quan" 06/13/16 5:22 PM >>>
> > >> >From: Quan Xu
> > >> >@@ -546,17 +550,37 @@ static int __must_check iommu_flush_all(void)
> > >> >struct acpi_drhd_unit *drhd; struct iommu *iommu; int
> > >> >flush_dev_iotlb;
> > >> >+int rc = 0;
> > >> >
> > >> >flush_all_cache();
> > >> >for_each_drhd_unit ( drhd )
> > >> >{
> > >> >+int iommu_rc, iommu_ret;
> > >> >+
> > >> >iommu = drhd->iommu;
> > >> >-iommu_flush_context_global(iommu, 0);
> > >> >+iommu_rc = iommu_flush_context_global(iommu, 0);
> > >> >flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
> > >> >-iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
> > >> >+iommu_ret = iommu_flush_iotlb_global(iommu, 0,
> > >> >+ flush_dev_iotlb);
> > >> >+
> > >> >+/*
> > >> >+ * The current logic for returns:
> > >> >+ * - positive invoke iommu_flush_write_buffer to flush
> > >> >cache.
> > >> >+ * - zero on success.
> > >> >+ * - negative on failure. Continue to flush IOMMU IOTLB on a
> > >> >+ * best effort basis.
> > >> >+ */
> > >> >+if ( iommu_rc > 0 || iommu_ret > 0 )
> > >> >+iommu_flush_write_buffer(iommu);
> > >> >+if ( rc >= 0 )
> > >> >+rc = iommu_rc;
> > >> >+if ( rc >= 0 )
> > >> >+rc = iommu_ret;
> > >>
> > >> First of all - is it correct to fold the two
> > >> iommu_flush_write_buffer() invocations?
> > >>
> > >
> > > Sure, it is correct..
> > >
> > > as:
> > > - For updates to remapping hardware structures that require
> > > context-cache, PASID-cache, IOTLB or IEC invalidation Operations to
> > > flush stale entries from the hardware caches, no additional action is
> > > required to make the modification Visible to hardware. This is
> > > because, hardware performs an implicit write-buffer-flushing as a
> > > pre-condition to context-cache, PASID-cache, IOTLB and IEC
> > > invalidation operations.
> > >
> > > - For updates to remapping hardware structures (such as modifying a
> > > currently not-present entry) that do not require Context-cache, IOTLB,
> > > or IEC invalidations, software must explicitly perform
> > > write-buffer-flushing to ensure the updated structures Are visible to
> > > hardware.
> >
> > But that's not the point. Instead my question was related to Kevin's concern
> > towards you making assumptions on the behavior of
> > iommu_flush_context_global() vs iommu_flush_iotlb_global(): What if the
> > first returned 1 but the second didn't?
> > It would seem to me that in such a
> > (theoretical) case iommu_flush_write_buffer() might need to be invoked prior
> > to the second flush function.
>
> In this case, it is correct too.
> As , hardware performs an __implicit__ write-buffer-flushing as a
> __pre-condition__ to
> IOTLB invalidation operation.
> So software is no need to call iommu_flush_write_buffer() to explicitly
> perform
> write-buffer-flushing for that the first returned 1.
>
Yes, there is no issue doing so based on spec.
Thanks
Kevin
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 09/11] vt-d: fix the IOMMU flush issue
On June 14, 2016 4:27 PM, Jan Beulich wrote:
> >>> On 14.06.16 at 10:10, wrote:
> > On June 13, 2016 11:52 PM, Jan Beulich wrote:
> >> >>> "Xu, Quan" 06/13/16 5:22 PM >>>
> >> >From: Quan Xu
> >> >@@ -546,17 +550,37 @@ static int __must_check iommu_flush_all(void)
> >> >struct acpi_drhd_unit *drhd; struct iommu *iommu; int
> >> >flush_dev_iotlb;
> >> >+int rc = 0;
> >> >
> >> >flush_all_cache();
> >> >for_each_drhd_unit ( drhd )
> >> >{
> >> >+int iommu_rc, iommu_ret;
> >> >+
> >> >iommu = drhd->iommu;
> >> >-iommu_flush_context_global(iommu, 0);
> >> >+iommu_rc = iommu_flush_context_global(iommu, 0);
> >> >flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
> >> >-iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
> >> >+iommu_ret = iommu_flush_iotlb_global(iommu, 0,
> >> >+ flush_dev_iotlb);
> >> >+
> >> >+/*
> >> >+ * The current logic for returns:
> >> >+ * - positive invoke iommu_flush_write_buffer to flush cache.
> >> >+ * - zero on success.
> >> >+ * - negative on failure. Continue to flush IOMMU IOTLB on a
> >> >+ * best effort basis.
> >> >+ */
> >> >+if ( iommu_rc > 0 || iommu_ret > 0 )
> >> >+iommu_flush_write_buffer(iommu);
> >> >+if ( rc >= 0 )
> >> >+rc = iommu_rc;
> >> >+if ( rc >= 0 )
> >> >+rc = iommu_ret;
> >>
> >> First of all - is it correct to fold the two
> >> iommu_flush_write_buffer() invocations?
> >>
> >
> > Sure, it is correct..
> >
> > as:
> > - For updates to remapping hardware structures that require
> > context-cache, PASID-cache, IOTLB or IEC invalidation Operations to
> > flush stale entries from the hardware caches, no additional action is
> > required to make the modification Visible to hardware. This is
> > because, hardware performs an implicit write-buffer-flushing as a
> > pre-condition to context-cache, PASID-cache, IOTLB and IEC
> > invalidation operations.
> >
> > - For updates to remapping hardware structures (such as modifying a
> > currently not-present entry) that do not require Context-cache, IOTLB,
> > or IEC invalidations, software must explicitly perform
> > write-buffer-flushing to ensure the updated structures Are visible to
> > hardware.
>
> But that's not the point. Instead my question was related to Kevin's concern
> towards you making assumptions on the behavior of
> iommu_flush_context_global() vs iommu_flush_iotlb_global(): What if the
> first returned 1 but the second didn't?
> It would seem to me that in such a
> (theoretical) case iommu_flush_write_buffer() might need to be invoked prior
> to the second flush function.
In this case, it is correct too.
As , hardware performs an __implicit__ write-buffer-flushing as a
__pre-condition__ to IOTLB invalidation operation.
So software is no need to call iommu_flush_write_buffer() to explicitly perform
write-buffer-flushing for that the first returned 1.
Quan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 09/11] vt-d: fix the IOMMU flush issue
>>> On 14.06.16 at 10:10, wrote:
> On June 13, 2016 11:52 PM, Jan Beulich wrote:
>> >>> "Xu, Quan" 06/13/16 5:22 PM >>>
>> >From: Quan Xu
>> >@@ -546,17 +550,37 @@ static int __must_check iommu_flush_all(void)
>> >struct acpi_drhd_unit *drhd; struct iommu *iommu; int flush_dev_iotlb;
>> >+int rc = 0;
>> >
>> >flush_all_cache();
>> >for_each_drhd_unit ( drhd )
>> >{
>> >+int iommu_rc, iommu_ret;
>> >+
>> >iommu = drhd->iommu;
>> >-iommu_flush_context_global(iommu, 0);
>> >+iommu_rc = iommu_flush_context_global(iommu, 0);
>> >flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
>> >-iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
>> >+iommu_ret = iommu_flush_iotlb_global(iommu, 0,
>> >+ flush_dev_iotlb);
>> >+
>> >+/*
>> >+ * The current logic for returns:
>> >+ * - positive invoke iommu_flush_write_buffer to flush cache.
>> >+ * - zero on success.
>> >+ * - negative on failure. Continue to flush IOMMU IOTLB on a
>> >+ * best effort basis.
>> >+ */
>> >+if ( iommu_rc > 0 || iommu_ret > 0 )
>> >+iommu_flush_write_buffer(iommu);
>> >+if ( rc >= 0 )
>> >+rc = iommu_rc;
>> >+if ( rc >= 0 )
>> >+rc = iommu_ret;
>>
>> First of all - is it correct to fold the two iommu_flush_write_buffer()
>> invocations?
>>
>
> Sure, it is correct..
>
> as:
> - For updates to remapping hardware structures that require context-cache,
> PASID-cache, IOTLB or IEC invalidation
> Operations to flush stale entries from the hardware caches, no additional
> action is required to make the modification
> Visible to hardware. This is because, hardware performs an implicit
> write-buffer-flushing as a pre-condition to context-cache,
> PASID-cache, IOTLB and IEC invalidation operations.
>
> - For updates to remapping hardware structures (such as modifying a currently
> not-present entry) that do not require
> Context-cache, IOTLB, or IEC invalidations, software must explicitly perform
> write-buffer-flushing to ensure the updated structures
> Are visible to hardware.
But that's not the point. Instead my question was related to Kevin's
concern towards you making assumptions on the behavior of
iommu_flush_context_global() vs iommu_flush_iotlb_global(): What
if the first returned 1 but the second didn't? It would seem to me
that in such a (theoretical) case iommu_flush_write_buffer() might
need to be invoked prior to the second flush function.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 09/11] vt-d: fix the IOMMU flush issue
On June 13, 2016 11:52 PM, Jan Beulich wrote:
> >>> "Xu, Quan" 06/13/16 5:22 PM >>>
> >From: Quan Xu
> >@@ -546,17 +550,37 @@ static int __must_check iommu_flush_all(void)
> >struct acpi_drhd_unit *drhd; struct iommu *iommu; int flush_dev_iotlb;
> >+int rc = 0;
> >
> >flush_all_cache();
> >for_each_drhd_unit ( drhd )
> >{
> >+int iommu_rc, iommu_ret;
> >+
> >iommu = drhd->iommu;
> >-iommu_flush_context_global(iommu, 0);
> >+iommu_rc = iommu_flush_context_global(iommu, 0);
> >flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
> >-iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
> >+iommu_ret = iommu_flush_iotlb_global(iommu, 0,
> >+ flush_dev_iotlb);
> >+
> >+/*
> >+ * The current logic for returns:
> >+ * - positive invoke iommu_flush_write_buffer to flush cache.
> >+ * - zero on success.
> >+ * - negative on failure. Continue to flush IOMMU IOTLB on a
> >+ * best effort basis.
> >+ */
> >+if ( iommu_rc > 0 || iommu_ret > 0 )
> >+iommu_flush_write_buffer(iommu);
> >+if ( rc >= 0 )
> >+rc = iommu_rc;
> >+if ( rc >= 0 )
> >+rc = iommu_ret;
>
> First of all - is it correct to fold the two iommu_flush_write_buffer()
> invocations?
>
Sure, it is correct..
as:
- For updates to remapping hardware structures that require context-cache,
PASID-cache, IOTLB or IEC invalidation
Operations to flush stale entries from the hardware caches, no additional
action is required to make the modification
Visible to hardware. This is because, hardware performs an implicit
write-buffer-flushing as a pre-condition to context-cache,
PASID-cache, IOTLB and IEC invalidation operations.
- For updates to remapping hardware structures (such as modifying a currently
not-present entry) that do not require
Context-cache, IOTLB, or IEC invalidations, software must explicitly perform
write-buffer-flushing to ensure the updated structures
Are visible to hardware.
> And then the variable naming is strange - both operations are IOMMU ones,
> so prefixing the variables with iommu_ doesn't help much here. How about
> ctxt_rc and iotlb_rc or some such?
>
To align to that file, I'm better using context_rc and iotlb_rc..
Quan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8 09/11] vt-d: fix the IOMMU flush issue
>>> "Xu, Quan" 06/13/16 5:22 PM >>>
>From: Quan Xu
>
>The propagation value from IOMMU flush interfaces may be positive, which
>indicates callers need to flush cache, not one of faliures.
>
>when the propagation value is positive, this patch fixes this flush issue
>as follows:
>- call iommu_flush_write_buffer() to flush cache.
>- return zero.
>
>Signed-off-by: Quan Xu
>Reviewed-by: Jan Beulich
>
>CC: Kevin Tian
>CC: Feng Wu
>CC: Keir Fraser
>CC: Jan Beulich
>CC: Andrew Cooper
>
>v8: drop assertion and check both flush_context and flush_iotlb
>whether the return values are positive.
>(note: this change is not under Jan's R-b).
In which case you should simply have dropped it.
>@@ -546,17 +550,37 @@ static int __must_check iommu_flush_all(void)
>struct acpi_drhd_unit *drhd;
>struct iommu *iommu;
>int flush_dev_iotlb;
>+int rc = 0;
>
>flush_all_cache();
>for_each_drhd_unit ( drhd )
>{
>+int iommu_rc, iommu_ret;
>+
>iommu = drhd->iommu;
>-iommu_flush_context_global(iommu, 0);
>+iommu_rc = iommu_flush_context_global(iommu, 0);
>flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
>-iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
>+iommu_ret = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
>+
>+/*
>+ * The current logic for returns:
>+ * - positive invoke iommu_flush_write_buffer to flush cache.
>+ * - zero on success.
>+ * - negative on failure. Continue to flush IOMMU IOTLB on a
>+ * best effort basis.
>+ */
>+if ( iommu_rc > 0 || iommu_ret > 0 )
>+iommu_flush_write_buffer(iommu);
>+if ( rc >= 0 )
>+rc = iommu_rc;
>+if ( rc >= 0 )
>+rc = iommu_ret;
First of all - is it correct to fold the two iommu_flush_write_buffer()
invocations?
And then the variable naming is strange - both operations are IOMMU ones, so
prefixing the variables with iommu_ doesn't help much here. How about ctxt_rc
and iotlb_rc or some such?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v8 09/11] vt-d: fix the IOMMU flush issue
From: Quan Xu
The propagation value from IOMMU flush interfaces may be positive, which
indicates callers need to flush cache, not one of faliures.
when the propagation value is positive, this patch fixes this flush issue
as follows:
- call iommu_flush_write_buffer() to flush cache.
- return zero.
Signed-off-by: Quan Xu
Reviewed-by: Jan Beulich
CC: Kevin Tian
CC: Feng Wu
CC: Keir Fraser
CC: Jan Beulich
CC: Andrew Cooper
v8: drop assertion and check both flush_context and flush_iotlb
whether the return values are positive.
(note: this change is not under Jan's R-b).
---
xen/drivers/passthrough/vtd/iommu.c | 150 +---
1 file changed, 103 insertions(+), 47 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/iommu.c
b/xen/drivers/passthrough/vtd/iommu.c
index 48edb67..2f046cb 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -388,17 +388,18 @@ static int flush_context_reg(
return 0;
}
-static int iommu_flush_context_global(
-struct iommu *iommu, int flush_non_present_entry)
+static int __must_check iommu_flush_context_global(struct iommu *iommu,
+ int flush_non_present_entry)
{
struct iommu_flush *flush = iommu_get_flush(iommu);
return flush->context(iommu, 0, 0, 0, DMA_CCMD_GLOBAL_INVL,
flush_non_present_entry);
}
-static int iommu_flush_context_device(
-struct iommu *iommu, u16 did, u16 source_id,
-u8 function_mask, int flush_non_present_entry)
+static int __must_check iommu_flush_context_device(struct iommu *iommu,
+ u16 did, u16 source_id,
+ u8 function_mask,
+ int flush_non_present_entry)
{
struct iommu_flush *flush = iommu_get_flush(iommu);
return flush->context(iommu, did, source_id, function_mask,
@@ -473,8 +474,9 @@ static int flush_iotlb_reg(void *_iommu, u16 did,
return 0;
}
-static int iommu_flush_iotlb_global(struct iommu *iommu,
-int flush_non_present_entry, int flush_dev_iotlb)
+static int __must_check iommu_flush_iotlb_global(struct iommu *iommu,
+ int flush_non_present_entry,
+ int flush_dev_iotlb)
{
struct iommu_flush *flush = iommu_get_flush(iommu);
int status;
@@ -491,8 +493,9 @@ static int iommu_flush_iotlb_global(struct iommu *iommu,
return status;
}
-static int iommu_flush_iotlb_dsi(struct iommu *iommu, u16 did,
-int flush_non_present_entry, int flush_dev_iotlb)
+static int __must_check iommu_flush_iotlb_dsi(struct iommu *iommu, u16 did,
+ int flush_non_present_entry,
+ int flush_dev_iotlb)
{
struct iommu_flush *flush = iommu_get_flush(iommu);
int status;
@@ -509,9 +512,10 @@ static int iommu_flush_iotlb_dsi(struct iommu *iommu, u16
did,
return status;
}
-static int iommu_flush_iotlb_psi(
-struct iommu *iommu, u16 did, u64 addr, unsigned int order,
-int flush_non_present_entry, int flush_dev_iotlb)
+static int __must_check iommu_flush_iotlb_psi(struct iommu *iommu, u16 did,
+ u64 addr, unsigned int order,
+ int flush_non_present_entry,
+ int flush_dev_iotlb)
{
struct iommu_flush *flush = iommu_get_flush(iommu);
int status;
@@ -546,17 +550,37 @@ static int __must_check iommu_flush_all(void)
struct acpi_drhd_unit *drhd;
struct iommu *iommu;
int flush_dev_iotlb;
+int rc = 0;
flush_all_cache();
for_each_drhd_unit ( drhd )
{
+int iommu_rc, iommu_ret;
+
iommu = drhd->iommu;
-iommu_flush_context_global(iommu, 0);
+iommu_rc = iommu_flush_context_global(iommu, 0);
flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
-iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
+iommu_ret = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
+
+/*
+ * The current logic for returns:
+ * - positive invoke iommu_flush_write_buffer to flush cache.
+ * - zero on success.
+ * - negative on failure. Continue to flush IOMMU IOTLB on a
+ * best effort basis.
+ */
+if ( iommu_rc > 0 || iommu_ret > 0 )
+iommu_flush_write_buffer(iommu);
+if ( rc >= 0 )
+rc = iommu_rc;
+if ( rc >= 0 )
+rc = iommu_ret;
}
-return 0;
+if ( rc > 0 )
+rc = 0;
+
+return rc;
}
static int __must_check iommu_flush_iotlb(struct domain *d,
@@ -569,6 +593,7 @@ static int __must_check iommu_flush_iotlb(struct domain *d,
