[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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@xxxxxxxxxx] > > Sent: 03 January 2019 18:20 > > To: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> > > Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxx; > > julien.grall@xxxxxxx; jbeulich@xxxxxxxx; Paul Durrant > > <Paul.Durrant@xxxxxxxxxx> > > 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@xxxxxxxxxx> > > > > > > > > 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. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |