diff mbox series

[v3,1/4] iommu / x86: move call to scan_pci_devices() out of vendor code

Message ID 20190716101657.23327-2-paul.durrant@citrix.com (mailing list archive)
State New, archived
Headers show
Series iommu groups + cleanup | expand

Commit Message

Paul Durrant July 16, 2019, 10:16 a.m. UTC 
It's not vendor specific so it doesn't really belong there.

Scanning the PCI topology also really doesn't have much to do with IOMMU
initialization. It doesn't depend on there even being an IOMMU. This patch
moves to the call to the beginning of iommu_hardware_setup() but only
places it there because the topology information would be otherwise unused.

Subsequent patches will actually make use of the PCI topology during
(x86) IOMMU initialization.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Reviewed-by: "Roger Pau Monné" <roger.pau@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 <wl@xen.org>

v2:
 - Expanded commit comment.
 - Moved PCI scan to before IOMMU initialization, rather than after it.
---
 xen/drivers/passthrough/amd/pci_amd_iommu.c | 3 ++-
 xen/drivers/passthrough/vtd/iommu.c         | 4 ----
 xen/drivers/passthrough/x86/iommu.c         | 6 ++++++
 3 files changed, 8 insertions(+), 5 deletions(-)

Comments

Tian, Kevin July 24, 2019, 12:51 a.m. UTC | #1 
> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> Sent: Tuesday, July 16, 2019 6:17 PM
> 
> It's not vendor specific so it doesn't really belong there.
> 
> Scanning the PCI topology also really doesn't have much to do with IOMMU
> initialization. It doesn't depend on there even being an IOMMU. This patch
> moves to the call to the beginning of iommu_hardware_setup() but only
> places it there because the topology information would be otherwise unused.
> 
> Subsequent patches will actually make use of the PCI topology during
> (x86) IOMMU initialization.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Reviewed-by: "Roger Pau Monné" <roger.pau@citrix.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Jan Beulich July 24, 2019, 1:40 p.m. UTC | #2 
On 16.07.2019 12:16, Paul Durrant wrote:
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -28,9 +28,15 @@ struct iommu_ops __read_mostly iommu_ops;
>   
>   int __init iommu_hardware_setup(void)
>   {
> +    int rc;
> +
>       if ( !iommu_init_ops )
>           return -ENODEV;
>   
> +    rc = scan_pci_devices();
> +    if ( rc )
> +        return rc;

 From an abstract POV I'm not convinced failing IOMMU init because
a failed bus scan is appropriate. But the only currently possible
failure is -ENOMEM, in which case we'd be in bigger trouble anyway.
Therefore
Acked-by: Jan Beulich <jbeulich@suse.com>

The other question of course is in how far you can sensibly use
the results of this (incomplete) bus scan later during IOMMU init.
But hopefully that'll become clear from the subsequent patches.

Jan
Paul Durrant July 24, 2019, 1:59 p.m. UTC | #3 
> -----Original Message-----
> From: Jan Beulich <JBeulich@suse.com>
> Sent: 24 July 2019 14:41
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Kevin Tian
> <kevin.tian@intel.com>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v3 1/4] iommu / x86: move call to scan_pci_devices() out of vendor code
> 
> On 16.07.2019 12:16, Paul Durrant wrote:
> > --- a/xen/drivers/passthrough/x86/iommu.c
> > +++ b/xen/drivers/passthrough/x86/iommu.c
> > @@ -28,9 +28,15 @@ struct iommu_ops __read_mostly iommu_ops;
> >
> >   int __init iommu_hardware_setup(void)
> >   {
> > +    int rc;
> > +
> >       if ( !iommu_init_ops )
> >           return -ENODEV;
> >
> > +    rc = scan_pci_devices();
> > +    if ( rc )
> > +        return rc;
> 
>  From an abstract POV I'm not convinced failing IOMMU init because
> a failed bus scan is appropriate. But the only currently possible
> failure is -ENOMEM, in which case we'd be in bigger trouble anyway.
> Therefore
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> The other question of course is in how far you can sensibly use
> the results of this (incomplete) bus scan later during IOMMU init.
> But hopefully that'll become clear from the subsequent patches.

The only use, at the moment, is for group assignment... but I do need to check that I haven't missed doing group assignment for subsequently added devices. I have a feeling I did miss it.

  Paul
diff mbox series

Patch

diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 4afbcd1609..3338a8e0e8 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -180,7 +180,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 8b27d7e775..b0e3bf26b5 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 0fa6dcc3fd..a7438c9c25 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -28,9 +28,15 @@  struct iommu_ops __read_mostly iommu_ops;
 
 int __init iommu_hardware_setup(void)
 {
+    int rc;
+
     if ( !iommu_init_ops )
         return -ENODEV;
 
+    rc = scan_pci_devices();
+    if ( rc )
+        return rc;
+
     if ( !iommu_ops.init )
         iommu_ops = *iommu_init_ops->ops;
     else