[PATCH v2] x86/vtd: fix IOMMU share PT destruction path

[PATCH v2] x86/vtd: fix IOMMU share PT destruction path

[Wei Liu]

Commit 2916951c1 ("mm / iommu: include need_iommu() test in
iommu_use_hap_pt()") included need_iommu() in iommu_use_hap_pt and
91d4eca7add ("mm / iommu: split need_iommu() into has_iommu_pt() and
need_iommu_pt_sync()") made things finer grain by spliting need_iommu
into three states.

The destruction path can't use iommu_use_hap_pt because at the point
platform op is called, IOMMU is either already switched to or has
always been in disabled state, and the shared PT test would always be
false.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
v2: fix bug and update comment.
---
 xen/drivers/passthrough/vtd/iommu.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index f94b522c73..4d1ff10817 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1752,7 +1752,17 @@ static void iommu_domain_teardown(struct domain *d)
         xfree(mrmrr);
     }
 
-    if ( iommu_use_hap_pt(d) )
+    ASSERT(iommu_enabled);
+
+    /*
+     * We can't use iommu_use_hap_pt here because either IOMMU state
+     * is already changed to IOMMU_STATUS_disabled at this point or
+     * has always been IOMMU_STATUS_disabled.
+     *
+     * We also need to test if HAP is enabled because PV guests can
+     * enter this path too.
+     */
+    if ( hap_enabled(d) && iommu_hap_pt_share )
         return;
 
     spin_lock(&hd->arch.mapping_lock);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86/vtd: fix IOMMU share PT destruction path

[Paul Durrant]

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 09 October 2018 15:57
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>
> Subject: [PATCH v2] x86/vtd: fix IOMMU share PT destruction path
> 
> Commit 2916951c1 ("mm / iommu: include need_iommu() test in
> iommu_use_hap_pt()") included need_iommu() in iommu_use_hap_pt and
> 91d4eca7add ("mm / iommu: split need_iommu() into has_iommu_pt() and
> need_iommu_pt_sync()") made things finer grain by spliting need_iommu
> into three states.
> 
> The destruction path can't use iommu_use_hap_pt because at the point
> platform op is called, IOMMU is either already switched to or has
> always been in disabled state, and the shared PT test would always be
> false.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
> v2: fix bug and update comment.
> ---
>  xen/drivers/passthrough/vtd/iommu.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index f94b522c73..4d1ff10817 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1752,7 +1752,17 @@ static void iommu_domain_teardown(struct domain *d)
>          xfree(mrmrr);
>      }
> 
> -    if ( iommu_use_hap_pt(d) )
> +    ASSERT(iommu_enabled);
> +
> +    /*
> +     * We can't use iommu_use_hap_pt here because either IOMMU state
> +     * is already changed to IOMMU_STATUS_disabled at this point or
> +     * has always been IOMMU_STATUS_disabled.
> +     *
> +     * We also need to test if HAP is enabled because PV guests can
> +     * enter this path too.
> +     */
> +    if ( hap_enabled(d) && iommu_hap_pt_share )
>          return;
> 
>      spin_lock(&hd->arch.mapping_lock);
> --
> 2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86/vtd: fix IOMMU share PT destruction path

[Roger Pau Monné]

On Tue, Oct 09, 2018 at 03:57:08PM +0100, Wei Liu wrote:
> Commit 2916951c1 ("mm / iommu: include need_iommu() test in
> iommu_use_hap_pt()") included need_iommu() in iommu_use_hap_pt and
> 91d4eca7add ("mm / iommu: split need_iommu() into has_iommu_pt() and
> need_iommu_pt_sync()") made things finer grain by spliting need_iommu
> into three states.
> 
> The destruction path can't use iommu_use_hap_pt because at the point
> platform op is called, IOMMU is either already switched to or has
> always been in disabled state, and the shared PT test would always be
> false.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

This is fine (and will fix destroying HVM/PVH guests), but I would
still like deallocate_iommu_page_tables (from pci_amd_iommu.c) to be
fixed in order to avoid the usage of iommu_use_hap_pt there also.

IMO the check itself could be converted into
ASSERT(!iommu_hap_pt_share) since it's unlikely that AMD will have
support for share page tables anytime soon.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86/vtd: fix IOMMU share PT destruction path

[Wei Liu]

On Tue, Oct 09, 2018 at 03:57:08PM +0100, Wei Liu wrote:
> Commit 2916951c1 ("mm / iommu: include need_iommu() test in
> iommu_use_hap_pt()") included need_iommu() in iommu_use_hap_pt and
> 91d4eca7add ("mm / iommu: split need_iommu() into has_iommu_pt() and
> need_iommu_pt_sync()") made things finer grain by spliting need_iommu
> into three states.
> 
> The destruction path can't use iommu_use_hap_pt because at the point
> platform op is called, IOMMU is either already switched to or has
> always been in disabled state, and the shared PT test would always be
> false.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Kevin, can you ack/nack this patch? Staging is currently blocked by a
bug which is fixed by this patch.

Thanks,
Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86/vtd: fix IOMMU share PT destruction path

[Wei Liu]

Add Kevin in the To: field

On Thu, Oct 11, 2018 at 09:27:31AM +0100, Wei Liu wrote:
> On Tue, Oct 09, 2018 at 03:57:08PM +0100, Wei Liu wrote:
> > Commit 2916951c1 ("mm / iommu: include need_iommu() test in
> > iommu_use_hap_pt()") included need_iommu() in iommu_use_hap_pt and
> > 91d4eca7add ("mm / iommu: split need_iommu() into has_iommu_pt() and
> > need_iommu_pt_sync()") made things finer grain by spliting need_iommu
> > into three states.
> > 
> > The destruction path can't use iommu_use_hap_pt because at the point
> > platform op is called, IOMMU is either already switched to or has
> > always been in disabled state, and the shared PT test would always be
> > false.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Kevin, can you ack/nack this patch? Staging is currently blocked by a
> bug which is fixed by this patch.
> 
> Thanks,
> Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86/vtd: fix IOMMU share PT destruction path

[Tian, Kevin]

> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: Thursday, October 11, 2018 4:28 PM
> 
> On Tue, Oct 09, 2018 at 03:57:08PM +0100, Wei Liu wrote:
> > Commit 2916951c1 ("mm / iommu: include need_iommu() test in
> > iommu_use_hap_pt()") included need_iommu() in iommu_use_hap_pt
> and
> > 91d4eca7add ("mm / iommu: split need_iommu() into has_iommu_pt()
> and
> > need_iommu_pt_sync()") made things finer grain by spliting need_iommu
> > into three states.
> >
> > The destruction path can't use iommu_use_hap_pt because at the point
> > platform op is called, IOMMU is either already switched to or has
> > always been in disabled state, and the shared PT test would always be
> > false.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Kevin, can you ack/nack this patch? Staging is currently blocked by a
> bug which is fixed by this patch.
> 

Sorry I thought I've sent the ack but looks not:

Acked-by: Kevin Tian <kevin.tian@intel.com>

Thanks
Kevin

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] x86/vtd: fix IOMMU share PT destruction path

[Wei Liu]

On Thu, Oct 11, 2018 at 08:30:59AM +0000, Tian, Kevin wrote:
> > From: Wei Liu [mailto:wei.liu2@citrix.com]
> > Sent: Thursday, October 11, 2018 4:28 PM
> > 
> > On Tue, Oct 09, 2018 at 03:57:08PM +0100, Wei Liu wrote:
> > > Commit 2916951c1 ("mm / iommu: include need_iommu() test in
> > > iommu_use_hap_pt()") included need_iommu() in iommu_use_hap_pt
> > and
> > > 91d4eca7add ("mm / iommu: split need_iommu() into has_iommu_pt()
> > and
> > > need_iommu_pt_sync()") made things finer grain by spliting need_iommu
> > > into three states.
> > >
> > > The destruction path can't use iommu_use_hap_pt because at the point
> > > platform op is called, IOMMU is either already switched to or has
> > > always been in disabled state, and the shared PT test would always be
> > > false.
> > >
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > 
> > Kevin, can you ack/nack this patch? Staging is currently blocked by a
> > bug which is fixed by this patch.
> > 
> 
> Sorry I thought I've sent the ack but looks not:
> 
> Acked-by: Kevin Tian <kevin.tian@intel.com>

Thank you!

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel