* [PATCH for-4.12] xen/iommu: fix dev assignment on ARM after 91d4eca7
@ 2019-01-03 0:28 Stefano Stabellini
2019-01-03 0:50 ` Andrew Cooper
0 siblings, 1 reply; 10+ messages in thread
From: Stefano Stabellini @ 2019-01-03 0:28 UTC (permalink / raw)
To: xen-devel; +Cc: julien.grall, sstabellini, jbeulich, paul.durrant
Fix device assignment on ARM after 91d4eca7 "mm / iommu: split
need_iommu() into has_iommu_pt() and need_iommu_pt_sync()".
arch_iommu_populate_page_table returns -ENOSYS which causes
iommu_construct to return early, although it is not an error.
Interestingly, the if ( rc ) was present even before 91d4eca7, but it was
still working before. Now, with the new hd->status field it won't
complete the initialization successfully.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index ac62d7f..a63329b 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -269,7 +269,7 @@ int iommu_construct(struct domain *d)
hd->need_sync = true;
rc = arch_iommu_populate_page_table(d);
- if ( rc )
+ if ( rc != 0 && rc != -ENOSYS )
{
if ( rc != -ERESTART )
{
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH for-4.12] xen/iommu: fix dev assignment on ARM after 91d4eca7
2019-01-03 0:28 [PATCH for-4.12] xen/iommu: fix dev assignment on ARM after 91d4eca7 Stefano Stabellini
@ 2019-01-03 0:50 ` Andrew Cooper
2019-01-03 18:19 ` Stefano Stabellini
0 siblings, 1 reply; 10+ messages in thread
From: Andrew Cooper @ 2019-01-03 0:50 UTC (permalink / raw)
To: Stefano Stabellini, xen-devel; +Cc: julien.grall, paul.durrant, jbeulich
On 03/01/2019 00:28, Stefano Stabellini wrote:
> Fix device assignment on ARM after 91d4eca7 "mm / iommu: split
> need_iommu() into has_iommu_pt() and need_iommu_pt_sync()".
>
> arch_iommu_populate_page_table returns -ENOSYS which causes
> iommu_construct to return early, although it is not an error.
>
> Interestingly, the if ( rc ) was present even before 91d4eca7, but it was
> still working before. Now, with the new hd->status field it won't
> complete the initialization successfully.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>
> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index ac62d7f..a63329b 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -269,7 +269,7 @@ int iommu_construct(struct domain *d)
> hd->need_sync = true;
>
> rc = arch_iommu_populate_page_table(d);
The comment in ARM's arch_iommu_populate_page_table() says
/* The IOMMU shares the p2m with the CPU */
Which means that iommu_use_hap_pt() (just out of context above this
hunk) is wrong. It should return true, which will prevent us entering
this path during initialisation.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-4.12] xen/iommu: fix dev assignment on ARM after 91d4eca7
2019-01-03 0:50 ` Andrew Cooper
@ 2019-01-03 18:19 ` Stefano Stabellini
2019-01-04 9:03 ` Paul Durrant
0 siblings, 1 reply; 10+ messages in thread
From: Stefano Stabellini @ 2019-01-03 18:19 UTC (permalink / raw)
To: Andrew Cooper
Cc: julien.grall, Stefano Stabellini, paul.durrant, jbeulich, xen-devel
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2574 bytes --]
On Thu, 3 Jan 2019, Andrew Cooper wrote:
> On 03/01/2019 00:28, Stefano Stabellini wrote:
> > Fix device assignment on ARM after 91d4eca7 "mm / iommu: split
> > need_iommu() into has_iommu_pt() and need_iommu_pt_sync()".
> >
> > arch_iommu_populate_page_table returns -ENOSYS which causes
> > iommu_construct to return early, although it is not an error.
> >
> > Interestingly, the if ( rc ) was present even before 91d4eca7, but it was
> > still working before. Now, with the new hd->status field it won't
> > complete the initialization successfully.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> >
> > diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> > index ac62d7f..a63329b 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -269,7 +269,7 @@ int iommu_construct(struct domain *d)
> > hd->need_sync = true;
> >
> > rc = arch_iommu_populate_page_table(d);
>
> The comment in ARM's arch_iommu_populate_page_table() says
>
> /* The IOMMU shares the p2m with the CPU */
>
> Which means that iommu_use_hap_pt() (just out of context above this
> hunk) is wrong. It should return true, which will prevent us entering
> this path during initialisation.
iommu_use_hap_pt is implemented by calling has_iommu_pt(d) (also on x86
is part of the implementation), which is implemented as:
(dom_iommu(d)->status != IOMMU_STATUS_disabled)
IOMMU_STATUS_disabled is zero in the enum, so status starts as
IOMMU_STATUS_disabled. It is set to IOMMU_STATUS_initializing for dom0
in iommu_hwdom_init, but it is not set to IOMMU_STATUS_initializing for
other domains anywhere, leading to this error.
It looks like we want to move the initialization of status from
iommu_hwdom_init to iommu_domain_init, which is called for everybody.
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index ac62d7f..7cf6aaf 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -180,6 +180,7 @@ int iommu_domain_init(struct domain *d)
if ( !iommu_enabled )
return 0;
+ hd->status = IOMMU_STATUS_initializing;
hd->platform_ops = iommu_get_ops();
return hd->platform_ops->init(d);
}
@@ -206,7 +207,6 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
register_keyhandler('o', &iommu_dump_p2m_table, "dump iommu p2m table", 0);
- hd->status = IOMMU_STATUS_initializing;
hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
if ( need_iommu_pt_sync(d) )
{
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH for-4.12] xen/iommu: fix dev assignment on ARM after 91d4eca7
2019-01-03 18:19 ` Stefano Stabellini
@ 2019-01-04 9:03 ` Paul Durrant
2019-01-04 17:46 ` Stefano Stabellini
0 siblings, 1 reply; 10+ messages in thread
From: Paul Durrant @ 2019-01-04 9:03 UTC (permalink / raw)
To: 'Stefano Stabellini', Andrew Cooper
Cc: julien.grall, jbeulich, xen-devel
> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> Sent: 03 January 2019 18:20
> To: Andrew Cooper <Andrew.Cooper3@citrix.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>; xen-devel@lists.xen.org;
> julien.grall@arm.com; jbeulich@suse.com; Paul Durrant
> <Paul.Durrant@citrix.com>
> Subject: Re: [Xen-devel] [PATCH for-4.12] xen/iommu: fix dev assignment on
> ARM after 91d4eca7
>
> On Thu, 3 Jan 2019, Andrew Cooper wrote:
> > On 03/01/2019 00:28, Stefano Stabellini wrote:
> > > Fix device assignment on ARM after 91d4eca7 "mm / iommu: split
> > > need_iommu() into has_iommu_pt() and need_iommu_pt_sync()".
> > >
> > > arch_iommu_populate_page_table returns -ENOSYS which causes
> > > iommu_construct to return early, although it is not an error.
> > >
> > > Interestingly, the if ( rc ) was present even before 91d4eca7, but it
> was
> > > still working before. Now, with the new hd->status field it won't
> > > complete the initialization successfully.
> > >
> > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > >
> > > diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> > > index ac62d7f..a63329b 100644
> > > --- a/xen/drivers/passthrough/iommu.c
> > > +++ b/xen/drivers/passthrough/iommu.c
> > > @@ -269,7 +269,7 @@ int iommu_construct(struct domain *d)
> > > hd->need_sync = true;
> > >
> > > rc = arch_iommu_populate_page_table(d);
> >
> > The comment in ARM's arch_iommu_populate_page_table() says
> >
> > /* The IOMMU shares the p2m with the CPU */
> >
> > Which means that iommu_use_hap_pt() (just out of context above this
> > hunk) is wrong. It should return true, which will prevent us entering
> > this path during initialisation.
>
> iommu_use_hap_pt is implemented by calling has_iommu_pt(d) (also on x86
> is part of the implementation), which is implemented as:
>
> (dom_iommu(d)->status != IOMMU_STATUS_disabled)
>
> IOMMU_STATUS_disabled is zero in the enum, so status starts as
> IOMMU_STATUS_disabled. It is set to IOMMU_STATUS_initializing for dom0
> in iommu_hwdom_init, but it is not set to IOMMU_STATUS_initializing for
> other domains anywhere, leading to this error.
It is set for other domains... see iommu_construct().
Paul
>
> It looks like we want to move the initialization of status from
> iommu_hwdom_init to iommu_domain_init, which is called for everybody.
>
>
> diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> index ac62d7f..7cf6aaf 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -180,6 +180,7 @@ int iommu_domain_init(struct domain *d)
> if ( !iommu_enabled )
> return 0;
>
> + hd->status = IOMMU_STATUS_initializing;
> hd->platform_ops = iommu_get_ops();
> return hd->platform_ops->init(d);
> }
> @@ -206,7 +207,6 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>
> register_keyhandler('o', &iommu_dump_p2m_table, "dump iommu p2m
> table", 0);
>
> - hd->status = IOMMU_STATUS_initializing;
> hd->need_sync = iommu_hwdom_strict && !iommu_use_hap_pt(d);
> if ( need_iommu_pt_sync(d) )
> {
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-4.12] xen/iommu: fix dev assignment on ARM after 91d4eca7
2019-01-04 9:03 ` Paul Durrant
@ 2019-01-04 17:46 ` Stefano Stabellini
2019-01-07 8:56 ` Paul Durrant
0 siblings, 1 reply; 10+ messages in thread
From: Stefano Stabellini @ 2019-01-04 17:46 UTC (permalink / raw)
To: Paul Durrant
Cc: Andrew Cooper, julien.grall, 'Stefano Stabellini',
jbeulich, xen-devel
[-- Attachment #1: Type: TEXT/PLAIN, Size: 3564 bytes --]
On Fri, 4 Jan 2019, Paul Durrant wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> > Sent: 03 January 2019 18:20
> > To: Andrew Cooper <Andrew.Cooper3@citrix.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>; xen-devel@lists.xen.org;
> > julien.grall@arm.com; jbeulich@suse.com; Paul Durrant
> > <Paul.Durrant@citrix.com>
> > Subject: Re: [Xen-devel] [PATCH for-4.12] xen/iommu: fix dev assignment on
> > ARM after 91d4eca7
> >
> > On Thu, 3 Jan 2019, Andrew Cooper wrote:
> > > On 03/01/2019 00:28, Stefano Stabellini wrote:
> > > > Fix device assignment on ARM after 91d4eca7 "mm / iommu: split
> > > > need_iommu() into has_iommu_pt() and need_iommu_pt_sync()".
> > > >
> > > > arch_iommu_populate_page_table returns -ENOSYS which causes
> > > > iommu_construct to return early, although it is not an error.
> > > >
> > > > Interestingly, the if ( rc ) was present even before 91d4eca7, but it
> > was
> > > > still working before. Now, with the new hd->status field it won't
> > > > complete the initialization successfully.
> > > >
> > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > > >
> > > > diff --git a/xen/drivers/passthrough/iommu.c
> > b/xen/drivers/passthrough/iommu.c
> > > > index ac62d7f..a63329b 100644
> > > > --- a/xen/drivers/passthrough/iommu.c
> > > > +++ b/xen/drivers/passthrough/iommu.c
> > > > @@ -269,7 +269,7 @@ int iommu_construct(struct domain *d)
> > > > hd->need_sync = true;
> > > >
> > > > rc = arch_iommu_populate_page_table(d);
> > >
> > > The comment in ARM's arch_iommu_populate_page_table() says
> > >
> > > /* The IOMMU shares the p2m with the CPU */
> > >
> > > Which means that iommu_use_hap_pt() (just out of context above this
> > > hunk) is wrong. It should return true, which will prevent us entering
> > > this path during initialisation.
> >
> > iommu_use_hap_pt is implemented by calling has_iommu_pt(d) (also on x86
> > is part of the implementation), which is implemented as:
> >
> > (dom_iommu(d)->status != IOMMU_STATUS_disabled)
> >
> > IOMMU_STATUS_disabled is zero in the enum, so status starts as
> > IOMMU_STATUS_disabled. It is set to IOMMU_STATUS_initializing for dom0
> > in iommu_hwdom_init, but it is not set to IOMMU_STATUS_initializing for
> > other domains anywhere, leading to this error.
>
> It is set for other domains... see iommu_construct().
Hi Paul, unfortunately it doesn't work that way. Let me explain:
iommu_construct() sets hd->status to IOMMU_STATUS_initializing *after*
calling iommu_use_hap_pt(d), and specifically inside the if statement:
if ( !iommu_use_hap_pt(d) )
{
int rc;
hd->status = IOMMU_STATUS_initializing;
hd->need_sync = true;
rc = arch_iommu_populate_page_table(d);
if ( rc )
{
if ( rc != -ERESTART )
{
hd->need_sync = false;
hd->status = IOMMU_STATUS_disabled;
}
return rc;
}
}
However, for iommu_use_hap_pt(d) to return the correct value, which
should be always true on ARM, status needs to be already set to
IOMMU_STATUS_initializing.
Because when we enter iommu_construct status is IOMMU_STATUS_disabled,
iommu_use_hap_pt(d) calls has_iommu_pt(d) which wrongly returns false
because it is implemented as:
#define has_iommu_pt(d) (dom_iommu(d)->status != IOMMU_STATUS_disabled)
I think we need to move the initialization of status to
IOMMU_STATUS_initializing earlier, specifically to iommu_domain_init.
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-4.12] xen/iommu: fix dev assignment on ARM after 91d4eca7
2019-01-04 17:46 ` Stefano Stabellini
@ 2019-01-07 8:56 ` Paul Durrant
2019-01-07 18:40 ` Stefano Stabellini
0 siblings, 1 reply; 10+ messages in thread
From: Paul Durrant @ 2019-01-07 8:56 UTC (permalink / raw)
To: 'Stefano Stabellini'
Cc: Andrew Cooper, julien.grall, jbeulich, xen-devel
> -----Original Message-----
> From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> Sent: 04 January 2019 17:46
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: 'Stefano Stabellini' <sstabellini@kernel.org>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; xen-devel@lists.xen.org;
> julien.grall@arm.com; jbeulich@suse.com
> Subject: RE: [Xen-devel] [PATCH for-4.12] xen/iommu: fix dev assignment on
> ARM after 91d4eca7
>
> On Fri, 4 Jan 2019, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> > > Sent: 03 January 2019 18:20
> > > To: Andrew Cooper <Andrew.Cooper3@citrix.com>
> > > Cc: Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel@lists.xen.org;
> > > julien.grall@arm.com; jbeulich@suse.com; Paul Durrant
> > > <Paul.Durrant@citrix.com>
> > > Subject: Re: [Xen-devel] [PATCH for-4.12] xen/iommu: fix dev
> assignment on
> > > ARM after 91d4eca7
> > >
> > > On Thu, 3 Jan 2019, Andrew Cooper wrote:
> > > > On 03/01/2019 00:28, Stefano Stabellini wrote:
> > > > > Fix device assignment on ARM after 91d4eca7 "mm / iommu: split
> > > > > need_iommu() into has_iommu_pt() and need_iommu_pt_sync()".
> > > > >
> > > > > arch_iommu_populate_page_table returns -ENOSYS which causes
> > > > > iommu_construct to return early, although it is not an error.
> > > > >
> > > > > Interestingly, the if ( rc ) was present even before 91d4eca7, but
> it
> > > was
> > > > > still working before. Now, with the new hd->status field it won't
> > > > > complete the initialization successfully.
> > > > >
> > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > > > >
> > > > > diff --git a/xen/drivers/passthrough/iommu.c
> > > b/xen/drivers/passthrough/iommu.c
> > > > > index ac62d7f..a63329b 100644
> > > > > --- a/xen/drivers/passthrough/iommu.c
> > > > > +++ b/xen/drivers/passthrough/iommu.c
> > > > > @@ -269,7 +269,7 @@ int iommu_construct(struct domain *d)
> > > > > hd->need_sync = true;
> > > > >
> > > > > rc = arch_iommu_populate_page_table(d);
> > > >
> > > > The comment in ARM's arch_iommu_populate_page_table() says
> > > >
> > > > /* The IOMMU shares the p2m with the CPU */
> > > >
> > > > Which means that iommu_use_hap_pt() (just out of context above this
> > > > hunk) is wrong. It should return true, which will prevent us
> entering
> > > > this path during initialisation.
> > >
> > > iommu_use_hap_pt is implemented by calling has_iommu_pt(d) (also on
> x86
> > > is part of the implementation), which is implemented as:
> > >
> > > (dom_iommu(d)->status != IOMMU_STATUS_disabled)
> > >
> > > IOMMU_STATUS_disabled is zero in the enum, so status starts as
> > > IOMMU_STATUS_disabled. It is set to IOMMU_STATUS_initializing for dom0
> > > in iommu_hwdom_init, but it is not set to IOMMU_STATUS_initializing
> for
> > > other domains anywhere, leading to this error.
> >
> > It is set for other domains... see iommu_construct().
>
> Hi Paul, unfortunately it doesn't work that way. Let me explain:
>
>
> iommu_construct() sets hd->status to IOMMU_STATUS_initializing *after*
> calling iommu_use_hap_pt(d), and specifically inside the if statement:
>
> if ( !iommu_use_hap_pt(d) )
> {
> int rc;
>
> hd->status = IOMMU_STATUS_initializing;
> hd->need_sync = true;
>
> rc = arch_iommu_populate_page_table(d);
> if ( rc )
> {
> if ( rc != -ERESTART )
> {
> hd->need_sync = false;
> hd->status = IOMMU_STATUS_disabled;
> }
>
> return rc;
> }
> }
>
> However, for iommu_use_hap_pt(d) to return the correct value, which
> should be always true on ARM, status needs to be already set to
> IOMMU_STATUS_initializing.
>
> Because when we enter iommu_construct status is IOMMU_STATUS_disabled,
> iommu_use_hap_pt(d) calls has_iommu_pt(d) which wrongly returns false
> because it is implemented as:
>
> #define has_iommu_pt(d) (dom_iommu(d)->status != IOMMU_STATUS_disabled)
>
>
> I think we need to move the initialization of status to
> IOMMU_STATUS_initializing earlier, specifically to iommu_domain_init.
The use of iommu_use_hap_pt() here is indeed a problem, but I think it would be sufficient to move the line "hd->status = IOMMU_STATUS_initializing" to just before the if rather than to a completely separate function.
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-4.12] xen/iommu: fix dev assignment on ARM after 91d4eca7
2019-01-07 8:56 ` Paul Durrant
@ 2019-01-07 18:40 ` Stefano Stabellini
2019-01-08 8:30 ` Paul Durrant
0 siblings, 1 reply; 10+ messages in thread
From: Stefano Stabellini @ 2019-01-07 18:40 UTC (permalink / raw)
To: Paul Durrant
Cc: Andrew Cooper, julien.grall, 'Stefano Stabellini',
jbeulich, xen-devel
[-- Attachment #1: Type: TEXT/PLAIN, Size: 5759 bytes --]
On Mon, 7 Jan 2019, Paul Durrant wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> > Sent: 04 January 2019 17:46
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: 'Stefano Stabellini' <sstabellini@kernel.org>; Andrew Cooper
> > <Andrew.Cooper3@citrix.com>; xen-devel@lists.xen.org;
> > julien.grall@arm.com; jbeulich@suse.com
> > Subject: RE: [Xen-devel] [PATCH for-4.12] xen/iommu: fix dev assignment on
> > ARM after 91d4eca7
> >
> > On Fri, 4 Jan 2019, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: Stefano Stabellini [mailto:sstabellini@kernel.org]
> > > > Sent: 03 January 2019 18:20
> > > > To: Andrew Cooper <Andrew.Cooper3@citrix.com>
> > > > Cc: Stefano Stabellini <sstabellini@kernel.org>; xen-
> > devel@lists.xen.org;
> > > > julien.grall@arm.com; jbeulich@suse.com; Paul Durrant
> > > > <Paul.Durrant@citrix.com>
> > > > Subject: Re: [Xen-devel] [PATCH for-4.12] xen/iommu: fix dev
> > assignment on
> > > > ARM after 91d4eca7
> > > >
> > > > On Thu, 3 Jan 2019, Andrew Cooper wrote:
> > > > > On 03/01/2019 00:28, Stefano Stabellini wrote:
> > > > > > Fix device assignment on ARM after 91d4eca7 "mm / iommu: split
> > > > > > need_iommu() into has_iommu_pt() and need_iommu_pt_sync()".
> > > > > >
> > > > > > arch_iommu_populate_page_table returns -ENOSYS which causes
> > > > > > iommu_construct to return early, although it is not an error.
> > > > > >
> > > > > > Interestingly, the if ( rc ) was present even before 91d4eca7, but
> > it
> > > > was
> > > > > > still working before. Now, with the new hd->status field it won't
> > > > > > complete the initialization successfully.
> > > > > >
> > > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > > > > >
> > > > > > diff --git a/xen/drivers/passthrough/iommu.c
> > > > b/xen/drivers/passthrough/iommu.c
> > > > > > index ac62d7f..a63329b 100644
> > > > > > --- a/xen/drivers/passthrough/iommu.c
> > > > > > +++ b/xen/drivers/passthrough/iommu.c
> > > > > > @@ -269,7 +269,7 @@ int iommu_construct(struct domain *d)
> > > > > > hd->need_sync = true;
> > > > > >
> > > > > > rc = arch_iommu_populate_page_table(d);
> > > > >
> > > > > The comment in ARM's arch_iommu_populate_page_table() says
> > > > >
> > > > > /* The IOMMU shares the p2m with the CPU */
> > > > >
> > > > > Which means that iommu_use_hap_pt() (just out of context above this
> > > > > hunk) is wrong. It should return true, which will prevent us
> > entering
> > > > > this path during initialisation.
> > > >
> > > > iommu_use_hap_pt is implemented by calling has_iommu_pt(d) (also on
> > x86
> > > > is part of the implementation), which is implemented as:
> > > >
> > > > (dom_iommu(d)->status != IOMMU_STATUS_disabled)
> > > >
> > > > IOMMU_STATUS_disabled is zero in the enum, so status starts as
> > > > IOMMU_STATUS_disabled. It is set to IOMMU_STATUS_initializing for dom0
> > > > in iommu_hwdom_init, but it is not set to IOMMU_STATUS_initializing
> > for
> > > > other domains anywhere, leading to this error.
> > >
> > > It is set for other domains... see iommu_construct().
> >
> > Hi Paul, unfortunately it doesn't work that way. Let me explain:
> >
> >
> > iommu_construct() sets hd->status to IOMMU_STATUS_initializing *after*
> > calling iommu_use_hap_pt(d), and specifically inside the if statement:
> >
> > if ( !iommu_use_hap_pt(d) )
> > {
> > int rc;
> >
> > hd->status = IOMMU_STATUS_initializing;
> > hd->need_sync = true;
> >
> > rc = arch_iommu_populate_page_table(d);
> > if ( rc )
> > {
> > if ( rc != -ERESTART )
> > {
> > hd->need_sync = false;
> > hd->status = IOMMU_STATUS_disabled;
> > }
> >
> > return rc;
> > }
> > }
> >
> > However, for iommu_use_hap_pt(d) to return the correct value, which
> > should be always true on ARM, status needs to be already set to
> > IOMMU_STATUS_initializing.
> >
> > Because when we enter iommu_construct status is IOMMU_STATUS_disabled,
> > iommu_use_hap_pt(d) calls has_iommu_pt(d) which wrongly returns false
> > because it is implemented as:
> >
> > #define has_iommu_pt(d) (dom_iommu(d)->status != IOMMU_STATUS_disabled)
> >
> >
> > I think we need to move the initialization of status to
> > IOMMU_STATUS_initializing earlier, specifically to iommu_domain_init.
>
> The use of iommu_use_hap_pt() here is indeed a problem, but I think it would be sufficient to move the line "hd->status = IOMMU_STATUS_initializing" to just before the if rather than to a completely separate function.
Yes, that works too.
---
xen/iommu: fix dev assignment on ARM after 91d4eca7
Fix device assignment on ARM after 91d4eca7 "mm / iommu: split
need_iommu() into has_iommu_pt() and need_iommu_pt_sync()".
arch_iommu_populate_page_table returns -ENOSYS which causes
iommu_construct to return early, although it is not an error.
hd->status needs to be set to IOMMU_STATUS_initializing before calling
iommu_use_hap_pt, otherwise iommu_use_hap_pt will return the wrong
value.
Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index ac62d7f..a6f69f4 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -261,11 +261,11 @@ int iommu_construct(struct domain *d)
if ( hd->status == IOMMU_STATUS_initialized )
return 0;
+ hd->status = IOMMU_STATUS_initializing;
if ( !iommu_use_hap_pt(d) )
{
int rc;
- hd->status = IOMMU_STATUS_initializing;
hd->need_sync = true;
rc = arch_iommu_populate_page_table(d);
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH for-4.12] xen/iommu: fix dev assignment on ARM after 91d4eca7
2019-01-07 18:40 ` Stefano Stabellini
@ 2019-01-08 8:30 ` Paul Durrant
2019-01-08 8:47 ` Jan Beulich
0 siblings, 1 reply; 10+ messages in thread
From: Paul Durrant @ 2019-01-08 8:30 UTC (permalink / raw)
To: 'Stefano Stabellini'
Cc: Andrew Cooper, julien.grall, jbeulich, xen-devel
> -----Original Message-----
[snip]
> >
> > The use of iommu_use_hap_pt() here is indeed a problem, but I think it
> would be sufficient to move the line "hd->status =
> IOMMU_STATUS_initializing" to just before the if rather than to a
> completely separate function.
>
> Yes, that works too.
>
> ---
> xen/iommu: fix dev assignment on ARM after 91d4eca7
>
> Fix device assignment on ARM after 91d4eca7 "mm / iommu: split
> need_iommu() into has_iommu_pt() and need_iommu_pt_sync()".
>
> arch_iommu_populate_page_table returns -ENOSYS which causes
> iommu_construct to return early, although it is not an error.
>
> hd->status needs to be set to IOMMU_STATUS_initializing before calling
> iommu_use_hap_pt, otherwise iommu_use_hap_pt will return the wrong
> value.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
LGTM so you can add...
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
>
> diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> index ac62d7f..a6f69f4 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -261,11 +261,11 @@ int iommu_construct(struct domain *d)
> if ( hd->status == IOMMU_STATUS_initialized )
> return 0;
>
> + hd->status = IOMMU_STATUS_initializing;
> if ( !iommu_use_hap_pt(d) )
> {
> int rc;
>
> - hd->status = IOMMU_STATUS_initializing;
> hd->need_sync = true;
>
> rc = arch_iommu_populate_page_table(d);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-4.12] xen/iommu: fix dev assignment on ARM after 91d4eca7
2019-01-08 8:30 ` Paul Durrant
@ 2019-01-08 8:47 ` Jan Beulich
2019-01-08 18:50 ` Stefano Stabellini
0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2019-01-08 8:47 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: Andrew Cooper, Julien Grall, Paul Durrant, xen-devel
>>> On 08.01.19 at 09:30, <Paul.Durrant@citrix.com> wrote:
>> -----Original Message-----
> [snip]
>> >
>> > The use of iommu_use_hap_pt() here is indeed a problem, but I think it
>> would be sufficient to move the line "hd->status =
>> IOMMU_STATUS_initializing" to just before the if rather than to a
>> completely separate function.
>>
>> Yes, that works too.
>>
>> ---
>> xen/iommu: fix dev assignment on ARM after 91d4eca7
>>
>> Fix device assignment on ARM after 91d4eca7 "mm / iommu: split
>> need_iommu() into has_iommu_pt() and need_iommu_pt_sync()".
>>
>> arch_iommu_populate_page_table returns -ENOSYS which causes
>> iommu_construct to return early, although it is not an error.
>>
>> hd->status needs to be set to IOMMU_STATUS_initializing before calling
>> iommu_use_hap_pt, otherwise iommu_use_hap_pt will return the wrong
>> value.
>>
>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>
> LGTM so you can add...
>
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
>
>>
>> diff --git a/xen/drivers/passthrough/iommu.c
>> b/xen/drivers/passthrough/iommu.c
>> index ac62d7f..a6f69f4 100644
>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -261,11 +261,11 @@ int iommu_construct(struct domain *d)
>> if ( hd->status == IOMMU_STATUS_initialized )
>> return 0;
>>
>> + hd->status = IOMMU_STATUS_initializing;
>> if ( !iommu_use_hap_pt(d) )
With a blank line inserted above here also
Acked-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH for-4.12] xen/iommu: fix dev assignment on ARM after 91d4eca7
2019-01-08 8:47 ` Jan Beulich
@ 2019-01-08 18:50 ` Stefano Stabellini
0 siblings, 0 replies; 10+ messages in thread
From: Stefano Stabellini @ 2019-01-08 18:50 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Julien Grall, Stefano Stabellini, Paul Durrant, xen-devel
On Tue, 8 Jan 2019, Jan Beulich wrote:
> >>> On 08.01.19 at 09:30, <Paul.Durrant@citrix.com> wrote:
> >> -----Original Message-----
> > [snip]
> >> >
> >> > The use of iommu_use_hap_pt() here is indeed a problem, but I think it
> >> would be sufficient to move the line "hd->status =
> >> IOMMU_STATUS_initializing" to just before the if rather than to a
> >> completely separate function.
> >>
> >> Yes, that works too.
> >>
> >> ---
> >> xen/iommu: fix dev assignment on ARM after 91d4eca7
> >>
> >> Fix device assignment on ARM after 91d4eca7 "mm / iommu: split
> >> need_iommu() into has_iommu_pt() and need_iommu_pt_sync()".
> >>
> >> arch_iommu_populate_page_table returns -ENOSYS which causes
> >> iommu_construct to return early, although it is not an error.
> >>
> >> hd->status needs to be set to IOMMU_STATUS_initializing before calling
> >> iommu_use_hap_pt, otherwise iommu_use_hap_pt will return the wrong
> >> value.
> >>
> >> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> >
> > LGTM so you can add...
> >
> > Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> >
> >>
> >> diff --git a/xen/drivers/passthrough/iommu.c
> >> b/xen/drivers/passthrough/iommu.c
> >> index ac62d7f..a6f69f4 100644
> >> --- a/xen/drivers/passthrough/iommu.c
> >> +++ b/xen/drivers/passthrough/iommu.c
> >> @@ -261,11 +261,11 @@ int iommu_construct(struct domain *d)
> >> if ( hd->status == IOMMU_STATUS_initialized )
> >> return 0;
> >>
> >> + hd->status = IOMMU_STATUS_initializing;
> >> if ( !iommu_use_hap_pt(d) )
>
> With a blank line inserted above here also
> Acked-by: Jan Beulich <jbeulich@suse.com>
Added and committed. Thanks!
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-01-08 18:50 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-03 0:28 [PATCH for-4.12] xen/iommu: fix dev assignment on ARM after 91d4eca7 Stefano Stabellini
2019-01-03 0:50 ` Andrew Cooper
2019-01-03 18:19 ` Stefano Stabellini
2019-01-04 9:03 ` Paul Durrant
2019-01-04 17:46 ` Stefano Stabellini
2019-01-07 8:56 ` Paul Durrant
2019-01-07 18:40 ` Stefano Stabellini
2019-01-08 8:30 ` Paul Durrant
2019-01-08 8:47 ` Jan Beulich
2019-01-08 18:50 ` Stefano Stabellini
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.