[Xen-devel] [PATCH 2/5] iommu / x86: move call to scan_pci_devices() out of vendor code

Paul Durrant posted 5 patches 1 year, 11 months ago

[Xen-devel] [PATCH 2/5] iommu / x86: move call to scan_pci_devices() out of vendor code

Posted by Paul Durrant 1 year, 11 months ago
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(-)

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)
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/5] iommu / x86: move call to scan_pci_devices() out of vendor code

Posted by Jan Beulich 1 year, 11 months ago
>>> 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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/5] iommu / x86: move call to scan_pci_devices() out of vendor code

Posted by Paul Durrant 1 year, 11 months ago
> -----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
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/5] iommu / x86: move call to scan_pci_devices() out of vendor code

Posted by Jan Beulich 1 year, 11 months ago
>>> 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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel