* [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.