| Message ID | 20190508132403.1454-3-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iommu groups + cleanup | expand |
>>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote: > It's not vendor specific so it shouldn't really be there. Perhaps, but this needs better justification: > --- a/xen/drivers/passthrough/vtd/iommu.c > +++ b/xen/drivers/passthrough/vtd/iommu.c > @@ -2372,10 +2372,6 @@ static int __init vtd_setup(void) > P(iommu_hap_pt_share, "Shared EPT tables"); > #undef P > > - ret = scan_pci_devices(); > - if ( ret ) > - goto error; > - > ret = init_vtd_hw(); Even after some looking around, it's not obvious to me that the latter call doesn't depend on PCI devices being known, more specifically segment 0's bus2bridge[] having been filled. Nor can I tell whether there would be some noticeable misbehavior (prior to any guests starting) if there was a dependency and it got broken by the re- ordering. Jan
> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 13 May 2019 08:36 > To: Paul Durrant <Paul.Durrant@citrix.com> > Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Andrew > Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; Wei Liu > <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>; xen-devel <xen-devel@lists.xenproject.org> > Subject: Re: [PATCH 2/5] iommu / x86: move call to scan_pci_devices() out of vendor code > > >>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote: > > It's not vendor specific so it shouldn't really be there. > > Perhaps, but this needs better justification: > > > --- a/xen/drivers/passthrough/vtd/iommu.c > > +++ b/xen/drivers/passthrough/vtd/iommu.c > > @@ -2372,10 +2372,6 @@ static int __init vtd_setup(void) > > P(iommu_hap_pt_share, "Shared EPT tables"); > > #undef P > > > > - ret = scan_pci_devices(); > > - if ( ret ) > > - goto error; > > - > > ret = init_vtd_hw(); > > Even after some looking around, it's not obvious to me that the latter > call doesn't depend on PCI devices being known, more specifically > segment 0's bus2bridge[] having been filled. Nor can I tell whether > there would be some noticeable misbehavior (prior to any guests > starting) if there was a dependency and it got broken by the re- > ordering. I don't see any dependency but the code is somewhat tangled. Perhaps it would be better to build the PCI topology *before* IOMMU init and then iterate over the the devices after init to do the group assignment. I certainly can't see anything in the scan as it stands that would need the IOMMU to have been initialized. Paul > > Jan >
>>> On 14.05.19 at 18:13, <Paul.Durrant@citrix.com> wrote: >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: 13 May 2019 08:36 >> To: Paul Durrant <Paul.Durrant@citrix.com> >> Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit > <suravee.suthikulpanit@amd.com>; Andrew >> Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>; Wei > Liu >> <wei.liu2@citrix.com>; Kevin Tian <kevin.tian@intel.com>; xen-devel > <xen-devel@lists.xenproject.org> >> Subject: Re: [PATCH 2/5] iommu / x86: move call to scan_pci_devices() out of > vendor code >> >> >>> On 08.05.19 at 15:24, <paul.durrant@citrix.com> wrote: >> > It's not vendor specific so it shouldn't really be there. >> >> Perhaps, but this needs better justification: >> >> > --- a/xen/drivers/passthrough/vtd/iommu.c >> > +++ b/xen/drivers/passthrough/vtd/iommu.c >> > @@ -2372,10 +2372,6 @@ static int __init vtd_setup(void) >> > P(iommu_hap_pt_share, "Shared EPT tables"); >> > #undef P >> > >> > - ret = scan_pci_devices(); >> > - if ( ret ) >> > - goto error; >> > - >> > ret = init_vtd_hw(); >> >> Even after some looking around, it's not obvious to me that the latter >> call doesn't depend on PCI devices being known, more specifically >> segment 0's bus2bridge[] having been filled. Nor can I tell whether >> there would be some noticeable misbehavior (prior to any guests >> starting) if there was a dependency and it got broken by the re- >> ordering. > > I don't see any dependency but the code is somewhat tangled. Perhaps it > would be better to build the PCI topology *before* IOMMU init and then > iterate over the the devices after init to do the group assignment. I > certainly can't see anything in the scan as it stands that would need the > IOMMU to have been initialized. Ah, yes, that's likely a better model. As to the dependency aspect: pci_add_device() calls iommu_add_device(). But oddly enough _scan_pci_devices() calls alloc_pdev(), not pci_add_device(). So indeed there doesn't look to be any dependency at present. Jan
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c index dbc71ca7d5..872bbe21c2 100644 --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c @@ -182,7 +182,8 @@ static int __init iov_detect(void) if ( !amd_iommu_perdev_intremap ) printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table is not recommended (see XSA-36)!\n"); - return scan_pci_devices(); + + return 0; } int amd_iommu_alloc_root(struct domain_iommu *hd) diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index 7b9e09a084..f9c76f594c 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -2372,10 +2372,6 @@ static int __init vtd_setup(void) P(iommu_hap_pt_share, "Shared EPT tables"); #undef P - ret = scan_pci_devices(); - if ( ret ) - goto error; - ret = init_vtd_hw(); if ( ret ) goto error; diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c index 034ac903dd..895c7fb564 100644 --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -28,6 +28,8 @@ struct iommu_ops __read_mostly iommu_ops; int __init iommu_hardware_setup(void) { + int rc; + if ( !iommu_init_ops ) return -ENODEV; @@ -37,7 +39,12 @@ int __init iommu_hardware_setup(void) /* x2apic setup may have previously initialised the struct. */ ASSERT(iommu_ops.init == iommu_init_ops->ops->init); - return iommu_init_ops->setup(); + rc = iommu_init_ops->setup(); + + if ( !rc ) + rc = scan_pci_devices(); + + return rc; } int iommu_enable_x2apic(void)
It's not vendor specific so it shouldn't really be there. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> --- Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> Cc: Brian Woods <brian.woods@amd.com> Cc: Kevin Tian <kevin.tian@intel.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> Cc: "Roger Pau Monné" <roger.pau@citrix.com> --- xen/drivers/passthrough/amd/pci_amd_iommu.c | 3 ++- xen/drivers/passthrough/vtd/iommu.c | 4 ---- xen/drivers/passthrough/x86/iommu.c | 9 ++++++++- 3 files changed, 10 insertions(+), 6 deletions(-)