Hi All,
This is a follow up to Matt's recent series[0] where he tackled a race that
turned out to be outside of the s390 IOMMU driver itself as well as duplicate
device attachments. After an internal discussion we came up with what I believe
is a cleaner fix. Instead of actively checking for duplicates we instead detach
from any previous domain on attach. From my cursory reading of the code this
seems to be what the Intel IOMMU driver is doing as well.
During development of this fix we realized that we can get rid of struct
s390_domain_device entirely if we instead thread the list through the attached
struct zpci_devs. This saves us from having to allocate during attach and gets
rid of one level of indirection during IOMMU operations. Coincidentally
I discovered that a previous list_head in struct zpci_dev is unused so this is
removed and then replaced.
The duplicate entry fix is the first patch of this series and the only one
which carries a Fixes tag. It may be applied alone or together with patches
2 and 3 which are followup clean ups.
Best regards,
Niklas
Changes since v1:
- After patch 3 we don't have to search in the devices list on detach as
we alreadz have hold of the zpci_dev (Jason)
- Add a WARN_ON() if somehow ended up detaching a device from a domain that
isn't the device's current domain.
- Removed the iteration and list delete from s390_domain_free() instead
just WARN_ON() when we're freeing without having detached
- The last two points should help catching sequencing errors much more
quickly in the future.
[0] https://lore.kernel.org/linux-iommu/[email protected]/
Niklas Schnelle (3):
iommu/s390: Fix duplicate domain attachments
s390/pci: remove unused bus_next field from struct zpci_dev
iommu/s390: Get rid of s390_domain_device
arch/s390/include/asm/pci.h | 2 +-
drivers/iommu/s390-iommu.c | 94 +++++++++++++++++--------------------
2 files changed, 43 insertions(+), 53 deletions(-)
--
2.34.1
This field was added in commit 44510d6fa0c0 ("s390/pci: Handling
multifunctions") but is an unused remnant of an earlier version where
the devices on the virtual bus were connected in a linked list instead
of a fixed 256 entry array of pointers.
It is also not used for the list of busses as that is threaded through
struct zpci_bus not through struct zpci_dev.
Signed-off-by: Niklas Schnelle <[email protected]>
---
arch/s390/include/asm/pci.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
index 7b4cdadbc023..108e732d7b14 100644
--- a/arch/s390/include/asm/pci.h
+++ b/arch/s390/include/asm/pci.h
@@ -117,7 +117,6 @@ struct zpci_bus {
struct zpci_dev {
struct zpci_bus *zbus;
struct list_head entry; /* list of all zpci_devices, needed for hotplug, etc. */
- struct list_head bus_next;
struct kref kref;
struct hotplug_slot hotplug_slot;
--
2.34.1
On 9/22/22 11:52, Niklas Schnelle wrote:
> This field was added in commit 44510d6fa0c0 ("s390/pci: Handling
> multifunctions") but is an unused remnant of an earlier version where
> the devices on the virtual bus were connected in a linked list instead
> of a fixed 256 entry array of pointers.
>
> It is also not used for the list of busses as that is threaded through
> struct zpci_bus not through struct zpci_dev.
>
> Signed-off-by: Niklas Schnelle <[email protected]>
Reviewed-by: Pierre Morel <[email protected]>
> ---
> arch/s390/include/asm/pci.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> index 7b4cdadbc023..108e732d7b14 100644
> --- a/arch/s390/include/asm/pci.h
> +++ b/arch/s390/include/asm/pci.h
> @@ -117,7 +117,6 @@ struct zpci_bus {
> struct zpci_dev {
> struct zpci_bus *zbus;
> struct list_head entry; /* list of all zpci_devices, needed for hotplug, etc. */
> - struct list_head bus_next;
> struct kref kref;
> struct hotplug_slot hotplug_slot;
>
--
Pierre Morel
IBM Lab Boeblingen
On 9/26/22 11:17, Pierre Morel wrote:
>
>
> On 9/22/22 11:52, Niklas Schnelle wrote:
>> This field was added in commit 44510d6fa0c0 ("s390/pci: Handling
>> multifunctions") but is an unused remnant of an earlier version where
>> the devices on the virtual bus were connected in a linked list instead
>> of a fixed 256 entry array of pointers.
>>
>> It is also not used for the list of busses as that is threaded through
>> struct zpci_bus not through struct zpci_dev.
>>
>> Signed-off-by: Niklas Schnelle <[email protected]>
>
> Reviewed-by: Pierre Morel <[email protected]>
>
>
Also couldn't it be detached of the series and posted on its own?
>> ---
>> arch/s390/include/asm/pci.h | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
>> index 7b4cdadbc023..108e732d7b14 100644
>> --- a/arch/s390/include/asm/pci.h
>> +++ b/arch/s390/include/asm/pci.h
>> @@ -117,7 +117,6 @@ struct zpci_bus {
>> struct zpci_dev {
>> struct zpci_bus *zbus;
>> struct list_head entry; /* list of all zpci_devices,
>> needed for hotplug, etc. */
>> - struct list_head bus_next;
>> struct kref kref;
>> struct hotplug_slot hotplug_slot;
>
--
Pierre Morel
IBM Lab Boeblingen
On Mon, 2022-09-26 at 11:23 +0200, Pierre Morel wrote:
>
> On 9/26/22 11:17, Pierre Morel wrote:
> >
> > On 9/22/22 11:52, Niklas Schnelle wrote:
> > > This field was added in commit 44510d6fa0c0 ("s390/pci: Handling
> > > multifunctions") but is an unused remnant of an earlier version where
> > > the devices on the virtual bus were connected in a linked list instead
> > > of a fixed 256 entry array of pointers.
> > >
> > > It is also not used for the list of busses as that is threaded through
> > > struct zpci_bus not through struct zpci_dev.
> > >
> > > Signed-off-by: Niklas Schnelle <[email protected]>
> >
> > Reviewed-by: Pierre Morel <[email protected]>
> >
> >
>
> Also couldn't it be detached of the series and posted on its own?
As this is entirely s390 specific this can go via the s390 tree without
re-posting. Since we're still figuring the rest of the series out it
might even make it upstream before that and then we can more easily
refer to it as a pre-requisite.
>
> > > ---
> > > arch/s390/include/asm/pci.h | 1 -
> > > 1 file changed, 1 deletion(-)
> > >
> > > diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> > > index 7b4cdadbc023..108e732d7b14 100644
> > > --- a/arch/s390/include/asm/pci.h
> > > +++ b/arch/s390/include/asm/pci.h
> > > @@ -117,7 +117,6 @@ struct zpci_bus {
> > > struct zpci_dev {
> > > struct zpci_bus *zbus;
> > > struct list_head entry; /* list of all zpci_devices,
> > > needed for hotplug, etc. */
> > > - struct list_head bus_next;
> > > struct kref kref;
> > > struct hotplug_slot hotplug_slot;