* [PATCH] x86/vtd: fix IOMMU share PT destruction path
@ 2018-10-09 10:42 Wei Liu
2018-10-09 11:22 ` Paul Durrant
2018-10-09 14:26 ` Wei Liu
0 siblings, 2 replies; 12+ messages in thread
From: Wei Liu @ 2018-10-09 10:42 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Paul Durrant, Wei Liu, Jan Beulich, Roger Pau Monné
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 already switched to disabled state and
the shared PT test would always be false.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
xen/drivers/passthrough/vtd/iommu.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index f94b522c73..d66d9e8ad0 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1752,7 +1752,13 @@ static void iommu_domain_teardown(struct domain *d)
xfree(mrmrr);
}
- if ( iommu_use_hap_pt(d) )
+ ASSERT(hap_enabled(d));
+
+ /*
+ * We can't use iommu_use_hap_pt here because the IOMMU state is already
+ * changed to IOMMU_STATUS_disabled at this point.
+ */
+ if ( 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
2018-10-09 10:42 [PATCH] x86/vtd: fix IOMMU share PT destruction path Wei Liu
@ 2018-10-09 11:22 ` Paul Durrant
2018-10-09 11:38 ` Paul Durrant
2018-10-09 12:57 ` Roger Pau Monné
2018-10-09 14:26 ` Wei Liu
1 sibling, 2 replies; 12+ messages in thread
From: Paul Durrant @ 2018-10-09 11:22 UTC (permalink / raw)
To: xen-devel; +Cc: Kevin Tian, Wei Liu, Jan Beulich, Roger Pau Monne
> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 09 October 2018 11:42
> To: xen-devel@lists.xenproject.org
> Cc: Jan Beulich <JBeulich@suse.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>
> Subject: [PATCH] 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 already switched to disabled state and
> the shared PT test would always be false.
>
This is VT-d specific and the issue could affect AMD too. Can't you just switch round the call to teardown and setting IOMMU_status_disabled in xen/drivers/passthrough/iommu.c:iommu_teardown()?
Paul
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> xen/drivers/passthrough/vtd/iommu.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> index f94b522c73..d66d9e8ad0 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1752,7 +1752,13 @@ static void iommu_domain_teardown(struct domain *d)
> xfree(mrmrr);
> }
>
> - if ( iommu_use_hap_pt(d) )
> + ASSERT(hap_enabled(d));
> +
> + /*
> + * We can't use iommu_use_hap_pt here because the IOMMU state is
> already
> + * changed to IOMMU_STATUS_disabled at this point.
> + */
> + if ( 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
2018-10-09 11:22 ` Paul Durrant
@ 2018-10-09 11:38 ` Paul Durrant
2018-10-09 12:58 ` Wei Liu
2018-10-09 12:57 ` Roger Pau Monné
1 sibling, 1 reply; 12+ messages in thread
From: Paul Durrant @ 2018-10-09 11:38 UTC (permalink / raw)
To: Paul Durrant, xen-devel; +Cc: Kevin Tian, Wei Liu, Jan Beulich, Roger Pau Monne
> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Paul Durrant
> Sent: 09 October 2018 12:23
> To: Wei Liu <wei.liu2@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Kevin Tian <kevin.tian@intel.com>; Wei Liu <wei.liu2@citrix.com>; Jan
> Beulich <JBeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH] x86/vtd: fix IOMMU share PT destruction
> path
>
> > -----Original Message-----
> > From: Wei Liu [mailto:wei.liu2@citrix.com]
> > Sent: 09 October 2018 11:42
> > To: xen-devel@lists.xenproject.org
> > Cc: Jan Beulich <JBeulich@suse.com>; Roger Pau Monne
> > <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu
> > <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>
> > Subject: [PATCH] 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 already switched to disabled state and
> > the shared PT test would always be false.
> >
>
> This is VT-d specific and the issue could affect AMD too.
Yes, it looks like deallocate_iommu_page_tables() will then try to deallocate page tables that would not have been allocated. This probably happens to be safe because the root table is allocated on demand, but that might change in future so I think it better to fix as I suggest below.
Paul
> Can't you just
> switch round the call to teardown and setting IOMMU_status_disabled in
> xen/drivers/passthrough/iommu.c:iommu_teardown()?
>
> Paul
>
>
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > xen/drivers/passthrough/vtd/iommu.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/drivers/passthrough/vtd/iommu.c
> > b/xen/drivers/passthrough/vtd/iommu.c
> > index f94b522c73..d66d9e8ad0 100644
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -1752,7 +1752,13 @@ static void iommu_domain_teardown(struct domain
> *d)
> > xfree(mrmrr);
> > }
> >
> > - if ( iommu_use_hap_pt(d) )
> > + ASSERT(hap_enabled(d));
> > +
> > + /*
> > + * We can't use iommu_use_hap_pt here because the IOMMU state is
> > already
> > + * changed to IOMMU_STATUS_disabled at this point.
> > + */
> > + if ( 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
2018-10-09 11:22 ` Paul Durrant
2018-10-09 11:38 ` Paul Durrant
@ 2018-10-09 12:57 ` Roger Pau Monné
2018-10-09 13:11 ` Jan Beulich
1 sibling, 1 reply; 12+ messages in thread
From: Roger Pau Monné @ 2018-10-09 12:57 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Kevin Tian, Wei Liu, Jan Beulich
On Tue, Oct 09, 2018 at 12:22:34PM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Wei Liu [mailto:wei.liu2@citrix.com]
> > Sent: 09 October 2018 11:42
> > To: xen-devel@lists.xenproject.org
> > Cc: Jan Beulich <JBeulich@suse.com>; Roger Pau Monne
> > <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu
> > <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>
> > Subject: [PATCH] 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 already switched to disabled state and
> > the shared PT test would always be false.
> >
>
> This is VT-d specific and the issue could affect AMD too. Can't you just switch round the call to teardown and setting IOMMU_status_disabled in xen/drivers/passthrough/iommu.c:iommu_teardown()?
AMD doesn't support shared page tables so far, but in order to avoid
having to debug this again if AMD ever gains support for shared page
tables I think switching the order in iommu_teardown might be better.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
2018-10-09 11:38 ` Paul Durrant
@ 2018-10-09 12:58 ` Wei Liu
2018-10-09 13:37 ` Wei Liu
0 siblings, 1 reply; 12+ messages in thread
From: Wei Liu @ 2018-10-09 12:58 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Kevin Tian, Wei Liu, Jan Beulich, Roger Pau Monne
On Tue, Oct 09, 2018 at 12:38:41PM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> > Of Paul Durrant
> > Sent: 09 October 2018 12:23
> > To: Wei Liu <wei.liu2@citrix.com>; xen-devel@lists.xenproject.org
> > Cc: Kevin Tian <kevin.tian@intel.com>; Wei Liu <wei.liu2@citrix.com>; Jan
> > Beulich <JBeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
> > Subject: Re: [Xen-devel] [PATCH] x86/vtd: fix IOMMU share PT destruction
> > path
> >
> > > -----Original Message-----
> > > From: Wei Liu [mailto:wei.liu2@citrix.com]
> > > Sent: 09 October 2018 11:42
> > > To: xen-devel@lists.xenproject.org
> > > Cc: Jan Beulich <JBeulich@suse.com>; Roger Pau Monne
> > > <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu
> > > <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>
> > > Subject: [PATCH] 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 already switched to disabled state and
> > > the shared PT test would always be false.
> > >
> >
> > This is VT-d specific and the issue could affect AMD too.
>
> Yes, it looks like deallocate_iommu_page_tables() will then try to deallocate page tables that would not have been allocated. This probably happens to be safe because the root table is allocated on demand, but that might change in future so I think it better to fix as I suggest below.
>
> Paul
>
> > Can't you just
> > switch round the call to teardown and setting IOMMU_status_disabled in
> > xen/drivers/passthrough/iommu.c:iommu_teardown()?
OK. I will swap those lines instead.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
2018-10-09 12:57 ` Roger Pau Monné
@ 2018-10-09 13:11 ` Jan Beulich
0 siblings, 0 replies; 12+ messages in thread
From: Jan Beulich @ 2018-10-09 13:11 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel, Kevin Tian, Paul Durrant, Wei Liu
>>> On 09.10.18 at 14:57, <roger.pau@citrix.com> wrote:
> On Tue, Oct 09, 2018 at 12:22:34PM +0100, Paul Durrant wrote:
>> > -----Original Message-----
>> > From: Wei Liu [mailto:wei.liu2@citrix.com]
>> > Sent: 09 October 2018 11:42
>> > To: xen-devel@lists.xenproject.org
>> > Cc: Jan Beulich <JBeulich@suse.com>; Roger Pau Monne
>> > <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu
>> > <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>
>> > Subject: [PATCH] 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 already switched to disabled state and
>> > the shared PT test would always be false.
>> >
>>
>> This is VT-d specific and the issue could affect AMD too. Can't you just
> switch round the call to teardown and setting IOMMU_status_disabled in
> xen/drivers/passthrough/iommu.c:iommu_teardown()?
>
> AMD doesn't support shared page tables so far, but in order to avoid
> having to debug this again if AMD ever gains support for shared page
> tables I think switching the order in iommu_teardown might be better.
I think we've pretty much ruled out AMD ever re-gaining such
sharing functionality. In fact I thought we had settled that the
dead code relating to this functionality could be ripped out, if
anyone could carve out the time needed for doing so.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
2018-10-09 12:58 ` Wei Liu
@ 2018-10-09 13:37 ` Wei Liu
2018-10-09 14:29 ` Paul Durrant
0 siblings, 1 reply; 12+ messages in thread
From: Wei Liu @ 2018-10-09 13:37 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Kevin Tian, Wei Liu, Jan Beulich, Roger Pau Monne
On Tue, Oct 09, 2018 at 01:58:14PM +0100, Wei Liu wrote:
> On Tue, Oct 09, 2018 at 12:38:41PM +0100, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> > > Of Paul Durrant
> > > Sent: 09 October 2018 12:23
> > > To: Wei Liu <wei.liu2@citrix.com>; xen-devel@lists.xenproject.org
> > > Cc: Kevin Tian <kevin.tian@intel.com>; Wei Liu <wei.liu2@citrix.com>; Jan
> > > Beulich <JBeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
> > > Subject: Re: [Xen-devel] [PATCH] x86/vtd: fix IOMMU share PT destruction
> > > path
> > >
> > > > -----Original Message-----
> > > > From: Wei Liu [mailto:wei.liu2@citrix.com]
> > > > Sent: 09 October 2018 11:42
> > > > To: xen-devel@lists.xenproject.org
> > > > Cc: Jan Beulich <JBeulich@suse.com>; Roger Pau Monne
> > > > <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu
> > > > <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>
> > > > Subject: [PATCH] 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 already switched to disabled state and
> > > > the shared PT test would always be false.
> > > >
> > >
> > > This is VT-d specific and the issue could affect AMD too.
> >
> > Yes, it looks like deallocate_iommu_page_tables() will then try to deallocate page tables that would not have been allocated. This probably happens to be safe because the root table is allocated on demand, but that might change in future so I think it better to fix as I suggest below.
> >
> > Paul
> >
> > > Can't you just
> > > switch round the call to teardown and setting IOMMU_status_disabled in
> > > xen/drivers/passthrough/iommu.c:iommu_teardown()?
>
> OK. I will swap those lines instead.
Unfortunately swapping those two lines won't work, because for a DomU
that doesn't have passthrough device, IOMMU is always disabled during
its life cycle. Yet mm/p2m.c invokes iommu_share_p2m_table based solely
on if hap is enabled.
Do you have other suggestions? It seems that we'd better make the
predicates for enabling and disabling match.
Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
2018-10-09 10:42 [PATCH] x86/vtd: fix IOMMU share PT destruction path Wei Liu
2018-10-09 11:22 ` Paul Durrant
@ 2018-10-09 14:26 ` Wei Liu
2018-10-09 14:32 ` Paul Durrant
1 sibling, 1 reply; 12+ messages in thread
From: Wei Liu @ 2018-10-09 14:26 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Paul Durrant, Wei Liu, Jan Beulich, Roger Pau Monné
On Tue, Oct 09, 2018 at 11:42:17AM +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 already switched to disabled state and
> the shared PT test would always be false.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> xen/drivers/passthrough/vtd/iommu.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> index f94b522c73..d66d9e8ad0 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1752,7 +1752,13 @@ static void iommu_domain_teardown(struct domain *d)
> xfree(mrmrr);
> }
>
> - if ( iommu_use_hap_pt(d) )
> + ASSERT(hap_enabled(d));
This needs to be moved into the predicate below because PV domain can enter this path too.
> +
> + /*
> + * We can't use iommu_use_hap_pt here because the IOMMU state is already
> + * changed to IOMMU_STATUS_disabled at this point.
> + */
> + if ( 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
2018-10-09 13:37 ` Wei Liu
@ 2018-10-09 14:29 ` Paul Durrant
0 siblings, 0 replies; 12+ messages in thread
From: Paul Durrant @ 2018-10-09 14:29 UTC (permalink / raw)
Cc: xen-devel, Kevin Tian, Wei Liu, Jan Beulich, Roger Pau Monne
> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 09 October 2018 14:38
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>; xen-devel@lists.xenproject.org; Kevin
> Tian <kevin.tian@intel.com>; Jan Beulich <JBeulich@suse.com>; Roger Pau
> Monne <roger.pau@citrix.com>
> Subject: Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
>
> On Tue, Oct 09, 2018 at 01:58:14PM +0100, Wei Liu wrote:
> > On Tue, Oct 09, 2018 at 12:38:41PM +0100, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On
> Behalf
> > > > Of Paul Durrant
> > > > Sent: 09 October 2018 12:23
> > > > To: Wei Liu <wei.liu2@citrix.com>; xen-devel@lists.xenproject.org
> > > > Cc: Kevin Tian <kevin.tian@intel.com>; Wei Liu
> <wei.liu2@citrix.com>; Jan
> > > > Beulich <JBeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>
> > > > Subject: Re: [Xen-devel] [PATCH] x86/vtd: fix IOMMU share PT
> destruction
> > > > path
> > > >
> > > > > -----Original Message-----
> > > > > From: Wei Liu [mailto:wei.liu2@citrix.com]
> > > > > Sent: 09 October 2018 11:42
> > > > > To: xen-devel@lists.xenproject.org
> > > > > Cc: Jan Beulich <JBeulich@suse.com>; Roger Pau Monne
> > > > > <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>;
> Wei Liu
> > > > > <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>
> > > > > Subject: [PATCH] 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 already switched to disabled state
> and
> > > > > the shared PT test would always be false.
> > > > >
> > > >
> > > > This is VT-d specific and the issue could affect AMD too.
> > >
> > > Yes, it looks like deallocate_iommu_page_tables() will then try to
> deallocate page tables that would not have been allocated. This probably
> happens to be safe because the root table is allocated on demand, but that
> might change in future so I think it better to fix as I suggest below.
> > >
> > > Paul
> > >
> > > > Can't you just
> > > > switch round the call to teardown and setting IOMMU_status_disabled
> in
> > > > xen/drivers/passthrough/iommu.c:iommu_teardown()?
> >
> > OK. I will swap those lines instead.
>
> Unfortunately swapping those two lines won't work, because for a DomU
> that doesn't have passthrough device, IOMMU is always disabled during
> its life cycle. Yet mm/p2m.c invokes iommu_share_p2m_table based solely
> on if hap is enabled.
OK. This disparity between dom0 and domU set-up is annoying.
>
> Do you have other suggestions? It seems that we'd better make the
> predicates for enabling and disabling match.
>
Since your patch to the VT-d code works, let's go with it then.
Paul
> Wei.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
2018-10-09 14:26 ` Wei Liu
@ 2018-10-09 14:32 ` Paul Durrant
2018-10-09 14:40 ` Wei Liu
0 siblings, 1 reply; 12+ messages in thread
From: Paul Durrant @ 2018-10-09 14:32 UTC (permalink / raw)
To: xen-devel; +Cc: Kevin Tian, Wei Liu, Jan Beulich, Roger Pau Monne
> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 09 October 2018 15:26
> To: xen-devel@lists.xenproject.org
> Cc: Jan Beulich <JBeulich@suse.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>
> Subject: Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
>
> On Tue, Oct 09, 2018 at 11:42:17AM +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 already switched to disabled state and
> > the shared PT test would always be false.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > xen/drivers/passthrough/vtd/iommu.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/drivers/passthrough/vtd/iommu.c
> b/xen/drivers/passthrough/vtd/iommu.c
> > index f94b522c73..d66d9e8ad0 100644
> > --- a/xen/drivers/passthrough/vtd/iommu.c
> > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > @@ -1752,7 +1752,13 @@ static void iommu_domain_teardown(struct domain
> *d)
> > xfree(mrmrr);
> > }
> >
> > - if ( iommu_use_hap_pt(d) )
> > + ASSERT(hap_enabled(d));
>
> This needs to be moved into the predicate below because PV domain can
> enter this path too.
>
> > +
> > + /*
> > + * We can't use iommu_use_hap_pt here because the IOMMU state is
> already
> > + * changed to IOMMU_STATUS_disabled at this point.
> > + */
> > + if ( iommu_hap_pt_share )
> > return;
I guess the other option is to track whether there is anything to tear down in the iommu struct and test that. We have status and need_sync now, but maybe that's too simplistic.
Paul
> >
> > 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
2018-10-09 14:32 ` Paul Durrant
@ 2018-10-09 14:40 ` Wei Liu
2018-10-09 14:45 ` Paul Durrant
0 siblings, 1 reply; 12+ messages in thread
From: Wei Liu @ 2018-10-09 14:40 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Kevin Tian, Wei Liu, Jan Beulich, Roger Pau Monne
On Tue, Oct 09, 2018 at 03:32:51PM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Wei Liu [mailto:wei.liu2@citrix.com]
> > Sent: 09 October 2018 15:26
> > To: xen-devel@lists.xenproject.org
> > Cc: Jan Beulich <JBeulich@suse.com>; Roger Pau Monne
> > <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu
> > <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>
> > Subject: Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
> >
> > On Tue, Oct 09, 2018 at 11:42:17AM +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 already switched to disabled state and
> > > the shared PT test would always be false.
> > >
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > > xen/drivers/passthrough/vtd/iommu.c | 8 +++++++-
> > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/xen/drivers/passthrough/vtd/iommu.c
> > b/xen/drivers/passthrough/vtd/iommu.c
> > > index f94b522c73..d66d9e8ad0 100644
> > > --- a/xen/drivers/passthrough/vtd/iommu.c
> > > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > > @@ -1752,7 +1752,13 @@ static void iommu_domain_teardown(struct domain
> > *d)
> > > xfree(mrmrr);
> > > }
> > >
> > > - if ( iommu_use_hap_pt(d) )
> > > + ASSERT(hap_enabled(d));
> >
> > This needs to be moved into the predicate below because PV domain can
> > enter this path too.
> >
> > > +
> > > + /*
> > > + * We can't use iommu_use_hap_pt here because the IOMMU state is
> > already
> > > + * changed to IOMMU_STATUS_disabled at this point.
> > > + */
> > > + if ( iommu_hap_pt_share )
> > > return;
>
> I guess the other option is to track whether there is anything to tear down in the iommu struct and test that. We have status and need_sync now, but maybe that's too simplistic.
Using hap_enabled(d) && iommu_hap_pt_share should be good enough to me.
Unless you feel strongly about this I'm not going to introduce more code
churn than necessary.
Wei.
>
> Paul
>
> > >
> > > 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
2018-10-09 14:40 ` Wei Liu
@ 2018-10-09 14:45 ` Paul Durrant
0 siblings, 0 replies; 12+ messages in thread
From: Paul Durrant @ 2018-10-09 14:45 UTC (permalink / raw)
Cc: xen-devel, Kevin Tian, Wei Liu, Jan Beulich, Roger Pau Monne
> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 09 October 2018 15:41
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>; xen-devel@lists.xenproject.org; Jan
> Beulich <JBeulich@suse.com>; Roger Pau Monne <roger.pau@citrix.com>; Kevin
> Tian <kevin.tian@intel.com>
> Subject: Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
>
> On Tue, Oct 09, 2018 at 03:32:51PM +0100, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Wei Liu [mailto:wei.liu2@citrix.com]
> > > Sent: 09 October 2018 15:26
> > > To: xen-devel@lists.xenproject.org
> > > Cc: Jan Beulich <JBeulich@suse.com>; Roger Pau Monne
> > > <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Wei
> Liu
> > > <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>
> > > Subject: Re: [PATCH] x86/vtd: fix IOMMU share PT destruction path
> > >
> > > On Tue, Oct 09, 2018 at 11:42:17AM +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 already switched to disabled state
> and
> > > > the shared PT test would always be false.
> > > >
> > > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > > ---
> > > > xen/drivers/passthrough/vtd/iommu.c | 8 +++++++-
> > > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/xen/drivers/passthrough/vtd/iommu.c
> > > b/xen/drivers/passthrough/vtd/iommu.c
> > > > index f94b522c73..d66d9e8ad0 100644
> > > > --- a/xen/drivers/passthrough/vtd/iommu.c
> > > > +++ b/xen/drivers/passthrough/vtd/iommu.c
> > > > @@ -1752,7 +1752,13 @@ static void iommu_domain_teardown(struct
> domain
> > > *d)
> > > > xfree(mrmrr);
> > > > }
> > > >
> > > > - if ( iommu_use_hap_pt(d) )
> > > > + ASSERT(hap_enabled(d));
> > >
> > > This needs to be moved into the predicate below because PV domain can
> > > enter this path too.
> > >
> > > > +
> > > > + /*
> > > > + * We can't use iommu_use_hap_pt here because the IOMMU state
> is
> > > already
> > > > + * changed to IOMMU_STATUS_disabled at this point.
> > > > + */
> > > > + if ( iommu_hap_pt_share )
> > > > return;
> >
> > I guess the other option is to track whether there is anything to tear
> down in the iommu struct and test that. We have status and need_sync now,
> but maybe that's too simplistic.
>
> Using hap_enabled(d) && iommu_hap_pt_share should be good enough to me.
> Unless you feel strongly about this I'm not going to introduce more code
> churn than necessary.
>
Ok. Fair enough. Let's keep the patch small.
Paul
> Wei.
>
> >
> > Paul
> >
> > > >
> > > > 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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-10-09 14:54 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-09 10:42 [PATCH] x86/vtd: fix IOMMU share PT destruction path Wei Liu
2018-10-09 11:22 ` Paul Durrant
2018-10-09 11:38 ` Paul Durrant
2018-10-09 12:58 ` Wei Liu
2018-10-09 13:37 ` Wei Liu
2018-10-09 14:29 ` Paul Durrant
2018-10-09 12:57 ` Roger Pau Monné
2018-10-09 13:11 ` Jan Beulich
2018-10-09 14:26 ` Wei Liu
2018-10-09 14:32 ` Paul Durrant
2018-10-09 14:40 ` Wei Liu
2018-10-09 14:45 ` Paul Durrant
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.