Re: [RFC PATCH 0/2] iommu/s390: Fix iommu-groups and add sysfs support

[Joerg Roedel]

Hi Gerald,

On Fri, Apr 28, 2017 at 08:06:12PM +0200, Gerald Schaefer wrote:
> On Fri, 28 Apr 2017 16:55:13 +0200
> Joerg Roedel  wrote:

> Also, IIRC, add_device will get called before attach_dev. Currently we
> allow to attach more than one device (apparently from different buses) to
> one domain (one shared DMA table) in attach_dev. But then it would be too
> late to also add all devices to the same iommu-group. That would have had
> to be done earlier in add_device, but there we don't know yet if a shared
> DMA table would be set up later in attach_dev.

I think there is some misunderstanding here about what iommu-groups are.
An iommu-group is a group of devices that are not isolated from each
other wrt. DMA and/or IRQs. This means that the devices can influence
each other, e.g. directly DMA to each other without IOMMU control. So
the grouping relies on how the hardware is built.

Domains on the other side are a software controled concept. A domain is
basically an abstraction of a DMA address space. Multiple devices can
share on domain/address-space, just as multiple threads can share a
cpu address space.

The point of iommu-grouping is to make sure that we don't assign
different domains to devices in the same group, as that could break or
cause security issues without proper isolation.


Regards,

Joerg
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 0/2] iommu/s390: Fix iommu-groups and add sysfs support

[Joerg Roedel]

On Fri, Apr 28, 2017 at 05:25:20PM +0200, Sebastian Ott wrote:
> On Fri, 28 Apr 2017, Joerg Roedel wrote:
> > That sounds special :) So will every function of a single device end up
> > as a seperate device on a seperate root-bus?
> 
> Yes. That's true even for multi-function and SRIOV.

Okay, so its truly special :) Thanks for your confirmation.


Joerg

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 0/2] iommu/s390: Fix iommu-groups and add sysfs support

[Gerald Schaefer]

On Fri, 28 Apr 2017 16:55:13 +0200
Joerg Roedel  wrote:

> > I am however a bit confused now, about how we would have allowed group
> > sharing with the current s390 IOMMU code, or IOW in which scenario would
> > iommu_group_get() in the add_device callback find a shareable iommu-group?  
> 
> The usual way to do this is to use the iommu_group_get_for_dev()
> function, which invokes the iommu_ops->device_group call-back of the
> driver to find a matching group or allocating a new one.
> 
> There are ready-to-use functions for this call-back already:
> 
>   1) generic_device_group() - which just allocates a new group for
>  the device. This is usually used outside of PCI
> 
>   2) pci_device_group() - Which walks the PCI hierarchy to find
>  devices that are not isolated and uses the matching group for
>  its isolation domain.
> 
> A few drivers have their own versions of this call-back, but those are
> IOMMU drivers supporting multiple bus-types and need to find the right
> way to determine the group first.
> 
> > So, I guess we may have an issue with not sharing iommu-groups when
> > it could make sense to do so. But your patch would not fix this, as
> > we still would allocate separate iommu-groups for all functions.  
> 
> Yes, but the above approach won't help when each function ends up on a
> seperate bus because the code looks for different functions that are
> enumerated as such. Anyway, some more insight into how this enumeration
> works on s390 would be great :)

Since Sebastian confirmed this, it looks like we do not really have any
enumeration when there is a separate bus for each function.

Also, IIRC, add_device will get called before attach_dev. Currently we
allow to attach more than one device (apparently from different buses) to
one domain (one shared DMA table) in attach_dev. But then it would be too
late to also add all devices to the same iommu-group. That would have had
to be done earlier in add_device, but there we don't know yet if a shared
DMA table would be set up later in attach_dev.

So, it looks to me that we cannot provide correct iommu-group sharing
on s390, even though we allow iommu-domain sharing, which sounds odd. Since
this "shared domain / DMA table" option in attach_dev was only added
because at that time I thought that was a hard requirement for any arch-
specific IOMMU API implementation, maybe there was some misunderstanding.

It would make the code easier (and more consistent with the s390 hardware)
if I would just remove that option from attach_dev, and allow only one
device/function per iommu-domain. What do you think, could this be removed
for s390, or is there any common code requirement for providing that option
(and is it OK that we have separate iommu-groups in this case)?

Regards,
Gerald

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 0/2] iommu/s390: Fix iommu-groups and add sysfs support

[Sebastian Ott]

On Fri, 28 Apr 2017, Joerg Roedel wrote:
> On Fri, Apr 28, 2017 at 02:46:34PM +0200, Gerald Schaefer wrote:
> > On Thu, 27 Apr 2017 23:03:25 +0200
> > Joerg Roedel  wrote:
> > 
> > > > Well, there is a separate zpci_dev for each pci_dev on s390,
> > > > and each of those has its own separate dma-table (thus not shared).  
> > > 
> > > Is that true for all functions of a PCIe card, so does every function of
> > > a device has its own zpci_dev structure and thus its own DMA-table?
> > 
> > Yes, clp_add_pci_device() is called for every function, which in turn calls
> > zpci_create_device() with a freshly allocated zdev. zpci_enable_device()
> > then sets up a new DMA address space for each function.
> 
> That sounds special :) So will every function of a single device end up
> as a seperate device on a seperate root-bus?

Yes. That's true even for multi-function and SRIOV.

> > > My assumption came from the fact that the zpci_dev is read from
> > > pci_dev->sysdata, which is propagated there from the pci_bridge
> > > through the pci_root_bus structures.
> > 
> > The zdev gets there via zpci_create_device() -> zpci_scan_bus() ->
> > pci_scan_root_bus(), which is done for every single function.
> > 
> > Not sure if I understand this right, but it looks like we set up a new PCI
> > bus for each function.
> 
> Yeah, it sounds like this. Maybe Sebastian can confirm that?

Yes. Confirmed.

Regards,
Sebastian

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 0/2] iommu/s390: Fix iommu-groups and add sysfs support

[Joerg Roedel]

Hi Gerald,

On Fri, Apr 28, 2017 at 02:46:34PM +0200, Gerald Schaefer wrote:
> On Thu, 27 Apr 2017 23:03:25 +0200
> Joerg Roedel  wrote:
> 
> > > Well, there is a separate zpci_dev for each pci_dev on s390,
> > > and each of those has its own separate dma-table (thus not shared).  
> > 
> > Is that true for all functions of a PCIe card, so does every function of
> > a device has its own zpci_dev structure and thus its own DMA-table?
> 
> Yes, clp_add_pci_device() is called for every function, which in turn calls
> zpci_create_device() with a freshly allocated zdev. zpci_enable_device()
> then sets up a new DMA address space for each function.

That sounds special :) So will every function of a single device end up
as a seperate device on a seperate root-bus?

> > My assumption came from the fact that the zpci_dev is read from
> > pci_dev->sysdata, which is propagated there from the pci_bridge
> > through the pci_root_bus structures.
> 
> The zdev gets there via zpci_create_device() -> zpci_scan_bus() ->
> pci_scan_root_bus(), which is done for every single function.
> 
> Not sure if I understand this right, but it looks like we set up a new PCI
> bus for each function.

Yeah, it sounds like this. Maybe Sebastian can confirm that?

> I am however a bit confused now, about how we would have allowed group
> sharing with the current s390 IOMMU code, or IOW in which scenario would
> iommu_group_get() in the add_device callback find a shareable iommu-group?

The usual way to do this is to use the iommu_group_get_for_dev()
function, which invokes the iommu_ops->device_group call-back of the
driver to find a matching group or allocating a new one.

There are ready-to-use functions for this call-back already:

1) generic_device_group() - which just allocates a new group for
   the device. This is usually used outside of PCI

2) pci_device_group() - Which walks the PCI hierarchy to find
   devices that are not isolated and uses the matching group for
   its isolation domain.

A few drivers have their own versions of this call-back, but those are
IOMMU drivers supporting multiple bus-types and need to find the right
way to determine the group first.

> So, I guess we may have an issue with not sharing iommu-groups when
> it could make sense to do so. But your patch would not fix this, as
> we still would allocate separate iommu-groups for all functions.

Yes, but the above approach won't help when each function ends up on a
seperate bus because the code looks for different functions that are
enumerated as such. Anyway, some more insight into how this enumeration
works on s390 would be great :)


Regards,

Joerg

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 0/2] iommu/s390: Fix iommu-groups and add sysfs support

[Gerald Schaefer]

Hi Joerg,

I guess we are a bit special on s390 (again), see below. Sebastian is more
familiar with the base s390 PCI code, he may correct me if I'm wrong.

On Thu, 27 Apr 2017 23:03:25 +0200
Joerg Roedel  wrote:

> > Well, there is a separate zpci_dev for each pci_dev on s390,
> > and each of those has its own separate dma-table (thus not shared).  
> 
> Is that true for all functions of a PCIe card, so does every function of
> a device has its own zpci_dev structure and thus its own DMA-table?

Yes, clp_add_pci_device() is called for every function, which in turn calls
zpci_create_device() with a freshly allocated zdev. zpci_enable_device()
then sets up a new DMA address space for each function.

> My assumption came from the fact that the zpci_dev is read from
> pci_dev->sysdata, which is propagated there from the pci_bridge
> through the pci_root_bus structures.

The zdev gets there via zpci_create_device() -> zpci_scan_bus() ->
pci_scan_root_bus(), which is done for every single function.

Not sure if I understand this right, but it looks like we set up a new PCI
bus for each function.

> > Given this "separate zpci_dev for each pci_dev" situation, I don't
> > see what this update actually changes, compared to the previous code,
> > see also my comments to that patch.  
> 
> The add_device call-back is invoked for every function of a pci-device,
> because each function gets its own pci_dev structure. Also we usually
> group all functions of a PCI-device together into one iommu-group,
> because we don't trust that the device isolates its functions from each
> other.

OK, but similar to the add_device callback, zpci_create_device() is
also invoked for every function. So, allocating a new iommu-group in
zpci_create_device() will never lead to any group sharing.

I am however a bit confused now, about how we would have allowed group
sharing with the current s390 IOMMU code, or IOW in which scenario would
iommu_group_get() in the add_device callback find a shareable iommu-group?

In the attach_dev callback, we provide the option to "force" multiple
functions using the same iommu-domain / DMA address space, by de-registering
the per-function DMA address space and registering a common space. But
such functions would only be in the same iommu "domain" and not "group",
if I get this right.

So, I guess we may have an issue with not sharing iommu-groups when
it could make sense to do so. But your patch would not fix this, as
we still would allocate separate iommu-groups for all functions.

Regards,
Gerald

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 0/2] iommu/s390: Fix iommu-groups and add sysfs support

[Joerg Roedel]

Hi Gerald,

thanks for your reply. I have some more questions, please see below.

On Thu, Apr 27, 2017 at 08:10:18PM +0200, Gerald Schaefer wrote:

> Well, there is a separate zpci_dev for each pci_dev on s390,
> and each of those has its own separate dma-table (thus not shared).

Is that true for all functions of a PCIe card, so does every function of
a device has its own zpci_dev structure and thus its own DMA-table?

My assumption came from the fact that the zpci_dev is read from
pci_dev->sysdata, which is propagated there from the pci_bridge
through the pci_root_bus structures.

> Given this "separate zpci_dev for each pci_dev" situation, I don't
> see what this update actually changes, compared to the previous code,
> see also my comments to that patch.

The add_device call-back is invoked for every function of a pci-device,
because each function gets its own pci_dev structure. Also we usually
group all functions of a PCI-device together into one iommu-group,
because we don't trust that the device isolates its functions from each
other.

Regards,

Joerg

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [RFC PATCH 0/2] iommu/s390: Fix iommu-groups and add sysfs support

[Gerald Schaefer]

On Thu, 27 Apr 2017 17:28:23 +0200
Joerg Roedel  wrote:

> Hey,
> 
> here are two patches for the s390 PCI and IOMMU code. It is
> based on the assumption that every pci_dev that points to
> the same zpci_dev shares a single dma-table (and thus a
> single address space).

Well, there is a separate zpci_dev for each pci_dev on s390,
and each of those has its own separate dma-table (thus not shared).

> 
> If this assupmtion is true (as it looks to me from reading
> the code) then the iommu-group setup code in the s390 iommu
> driver needs to be updated.

Given this "separate zpci_dev for each pci_dev" situation, I don't
see what this update actually changes, compared to the previous code,
see also my comments to that patch.

> 
> These patches do this and also add support for the
> iommu_device_register interface to the s390 iommu driver.
> 
> Any comments and testing appreciated.
> 
> Thanks,
> 
>   Joerg
> 
> Joerg Roedel (2):
>   iommu/s390: Fix IOMMU groups
>   iommu/s390: Add support for iommu_device handling
> 
>  arch/s390/include/asm/pci.h |  8 +
>  arch/s390/pci/pci.c | 10 ++-
>  drivers/iommu/s390-iommu.c  | 71 
> ++---
>  3 files changed, 78 insertions(+), 11 deletions(-)
> 

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RFC PATCH 0/2] iommu/s390: Fix iommu-groups and add sysfs support

[Joerg Roedel]

Hey,

here are two patches for the s390 PCI and IOMMU code. It is
based on the assumption that every pci_dev that points to
the same zpci_dev shares a single dma-table (and thus a
single address space).

If this assupmtion is true (as it looks to me from reading
the code) then the iommu-group setup code in the s390 iommu
driver needs to be updated.

These patches do this and also add support for the
iommu_device_register interface to the s390 iommu driver.

Any comments and testing appreciated.

Thanks,

Joerg

Joerg Roedel (2):
  iommu/s390: Fix IOMMU groups
  iommu/s390: Add support for iommu_device handling

 arch/s390/include/asm/pci.h |  8 +
 arch/s390/pci/pci.c | 10 ++-
 drivers/iommu/s390-iommu.c  | 71 ++---
 3 files changed, 78 insertions(+), 11 deletions(-)

-- 
1.9.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu