Re: [Xen-devel] [PATCH v7 09/11] vt-d: fix the IOMMU flush issue
On June 12, 2016 5:27 PM, Xu, Quan wrote:
> On June 12, 2016 3:33 PM, Tian, Kevin wrote:
> > > From: Xu, Quan
> > > Sent: Wednesday, June 08, 2016 4:59 PM @@ -545,18 +549,42 @@
> 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 )
> > > {
> > > iommu = drhd->iommu;
> > > -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);
> > > +/*
> > > + * The current logic for rc 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.
> > > + *
> > > + * Moreover, IOMMU flush handlers flush_context_qi and
> > flush_iotlb_qi
> > > + * (or flush_context_reg and flush_iotlb_reg, deep functions in
> > > the
> > > + * call trees of iommu_flush_context_global and
> > iommu_flush_iotlb_global)
> > > + * are with the same logic to bubble up positive return value.
> > > + */
> > > +rc = iommu_flush_context_global(iommu, 0);
> > > +if ( rc <= 0 )
> > > +{
> > > +int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
> > > +int ret = iommu_flush_iotlb_global(iommu, 0,
> > > + flush_dev_iotlb);
> > > +
> > > +ASSERT(ret <= 0);
> > > +if ( !rc )
> > > +rc = ret;
> >
> > I'm dubious about the assertion here. Why can't above call return 1
> > upon error on earlier flush? I digged back your earlier reply like:
> >
> > > Yes, the iommu_flush_iotlb_dsi() can also return 1.
> > > Look at the call tree, at the beginning of
> > > flush_context_qi()/flush_iotlb_qi(), or
> > > flush_context_reg()/flush_iotlb_reg()..
> > >
> > > If rc was negative when we call iommu_flush_context_device(), it is
> > > impossible to return 1 for iommu_flush_iotlb_dsi().
> >
> > But I don't think it a good idea of making so much assumptions about
> > internal implementations of those low level interfaces.
> > Also flush_context may fail for one specific reason which doesn't
> > block flush_iotlb which could get 1 returned when caching mode is
> > disabled. We'd better have return-1 case correctly handled here.
> >
>
> Your comment looks reasonable here. Could I change it as below:
>
> -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,35 @@ 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 ret;
> +
> iommu = drhd->iommu;
> -iommu_flush_context_global(iommu, 0);
> +/*
> + * The current logic for rc 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.
> + */
> +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);
> +ret = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
> +if ( !rc )
> +rc = ret;
> +
> +if ( rc > 0 || ret > 0 )
> +iommu_flush_write_buffer(iommu);
> }
>
> -return 0;
> +if ( rc > 0 )
> +rc = 0;
> +
> +return rc;
> }
>
>
Ah, this change is not correct, as the previous error return
value may be erased by the later positive / zero value.
I'll highlight this change is not under Jan's R-b in next v8.
.. I think the below is correct.
+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,
Re: [Xen-devel] [PATCH v7 09/11] vt-d: fix the IOMMU flush issue
On June 12, 2016 3:33 PM, Tian, Kevin wrote:
> > From: Xu, Quan
> > Sent: Wednesday, June 08, 2016 4:59 PM @@ -545,18 +549,42 @@ 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 )
> > {
> > iommu = drhd->iommu;
> > -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);
> > +/*
> > + * The current logic for rc 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.
> > + *
> > + * Moreover, IOMMU flush handlers flush_context_qi and
> flush_iotlb_qi
> > + * (or flush_context_reg and flush_iotlb_reg, deep functions in the
> > + * call trees of iommu_flush_context_global and
> iommu_flush_iotlb_global)
> > + * are with the same logic to bubble up positive return value.
> > + */
> > +rc = iommu_flush_context_global(iommu, 0);
> > +if ( rc <= 0 )
> > +{
> > +int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
> > +int ret = iommu_flush_iotlb_global(iommu, 0,
> > + flush_dev_iotlb);
> > +
> > +ASSERT(ret <= 0);
> > +if ( !rc )
> > +rc = ret;
>
> I'm dubious about the assertion here. Why can't above call return 1 upon error
> on earlier flush? I digged back your earlier reply like:
>
> > Yes, the iommu_flush_iotlb_dsi() can also return 1.
> > Look at the call tree, at the beginning of
> > flush_context_qi()/flush_iotlb_qi(), or
> > flush_context_reg()/flush_iotlb_reg()..
> >
> > If rc was negative when we call iommu_flush_context_device(), it is
> > impossible to return 1 for iommu_flush_iotlb_dsi().
>
> But I don't think it a good idea of making so much assumptions about internal
> implementations of those low level interfaces.
> Also flush_context may fail for one specific reason which doesn't block
> flush_iotlb which could get 1 returned when caching mode is disabled. We'd
> better have return-1 case correctly handled here.
>
Your comment looks reasonable here. Could I change it as below:
-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,35 @@ 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 ret;
+
iommu = drhd->iommu;
-iommu_flush_context_global(iommu, 0);
+/*
+ * The current logic for rc 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.
+ */
+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);
+ret = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
+if ( !rc )
+rc = ret;
+
+if ( rc > 0 || ret > 0 )
+iommu_flush_write_buffer(iommu);
}
-return 0;
+if ( rc > 0 )
+rc = 0;
+
+return rc;
}
Also, Jan, what's your opinion?
Quan
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v7 09/11] vt-d: fix the IOMMU flush issue
> From: Xu, Quan
> Sent: Wednesday, June 08, 2016 4:59 PM
> @@ -545,18 +549,42 @@ 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 )
> {
> iommu = drhd->iommu;
> -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);
> +/*
> + * The current logic for rc 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.
> + *
> + * Moreover, IOMMU flush handlers flush_context_qi and flush_iotlb_qi
> + * (or flush_context_reg and flush_iotlb_reg, deep functions in the
> + * call trees of iommu_flush_context_global and
> iommu_flush_iotlb_global)
> + * are with the same logic to bubble up positive return value.
> + */
> +rc = iommu_flush_context_global(iommu, 0);
> +if ( rc <= 0 )
> +{
> +int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
> +int ret = iommu_flush_iotlb_global(iommu, 0, flush_dev_iotlb);
> +
> +ASSERT(ret <= 0);
> +if ( !rc )
> +rc = ret;
I'm dubious about the assertion here. Why can't above call
return 1 upon error on earlier flush? I digged back your
earlier reply like:
> Yes, the iommu_flush_iotlb_dsi() can also return 1.
> Look at the call tree, at the beginning of
> flush_context_qi()/flush_iotlb_qi(), or
> flush_context_reg()/flush_iotlb_reg()..
>
> If rc was negative when we call iommu_flush_context_device(), it is
> impossible to return 1 for iommu_flush_iotlb_dsi().
But I don't think it a good idea of making so much assumptions
about internal implementations of those low level interfaces.
Also flush_context may fail for one specific reason which doesn't
block flush_iotlb which could get 1 returned when caching mode
is disabled. We'd better have return-1 case correctly handled here.
Thanks
Kevin
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
[Xen-devel] [PATCH v7 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
v7:
1. Drop blank lines.
2. make the assignment be replaced by its right side becoming
the variable's initializer.
3. leave the comments as are, no reply from VT-d maintainers yet.
---
xen/drivers/passthrough/vtd/iommu.c | 167 ++--
1 file changed, 124 insertions(+), 43 deletions(-)
diff --git a/xen/drivers/passthrough/vtd/iommu.c
b/xen/drivers/passthrough/vtd/iommu.c
index 48edb67..2a55985 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;
@@ -545,18 +549,42 @@ 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 )
{
iommu = drhd->iommu;
-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);
+/*
+ * The current logic for rc 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.
+ *
+ * Moreover, IOMMU flush handlers flush_context_qi and flush_iotlb_qi
+ * (or flush_context_reg and flush_iotlb_reg, deep functions in the
+ * call trees of iommu_flush_context_global and
iommu_flush_iotlb_global)
+ * are with the same logic to bubble up positive return value.
+ */
+rc = iommu_flush_context_global(iommu, 0);
+if ( rc <= 0 )
+{
+int flush_dev_iotlb = find_ats_dev_drhd(iommu) ? 1 : 0;
+int ret = iommu_flush_iotlb_global(iommu, 0, flush_dev_i
