Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1521511 > unrolled thread

Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT agnostic

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT  agnostic Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-11-14 11:30 +0100
    Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT  agnostic Robin Murphy <robin.murphy@arm.com> - 2016-11-14 13:10 +0100
      Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT  agnostic Joerg Roedel <joro@8bytes.org> - 2016-11-14 17:00 +0100
        Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT  agnostic Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-11-14 17:50 +0100
        Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT  agnostic Robin Murphy <robin.murphy@arm.com> - 2016-11-14 19:30 +0100
          Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT  agnostic Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-11-15 11:10 +0100
          Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT  agnostic Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-11-16 11:00 +0100

#1521511 — Re: [PATCH v7 04/16] drivers: iommu: make of_iommu_set/get_ops() DT agnostic

Hi Robin, Joerg,

On Fri, Nov 11, 2016 at 05:43:39PM +0000, Robin Murphy wrote:
> On 11/11/16 16:27, Joerg Roedel wrote:
> > On Fri, Nov 11, 2016 at 04:17:37PM +0000, Robin Murphy wrote:
> >> In the original of_iommu_configure design, the thought was that an ops
> >> structure could be IOMMU-instance-specific (hence the later-removed
> >> "priv" member), so I suppose right now it is mostly a hangover from
> >> that. However, it's also what we initialise a device's fwspec with, so
> >> becomes important again if we're ever going to get past the limitations
> >> of buses-which-are-not-actually-buses[1].
> > 
> > Yeah, I discussed this with a few others at LPC. My current idea is to
> > tell the iommu-core which hardware-iommus exist in the system and a
> > seperate iommu_ops ptr for each of them. Then every struct device can
> > link to the iommu-instance it is translated by.
> 
> Er, that sounds very much like a description of what we already have in
> 4.9-rc. Every struct device now has an iommu_fwspec which encapsulates
> both an iommu_ops pointer (which can perfectly well be per-instance if
> the IOMMU driver wants) and a place for the IOMMU-private data to
> replace the mess of archdata.iommu and driver-internal globals.
> 
> > We are not there yet, but this will give you the same per-device
> > iommu-ops as implemented here.
> 
> With those two patches I linked to, which make the bulk of the IOMMU
> core code per-device-ops-aware off the bat, I'd say we *are* already
> pretty much there. It's only iommu_domain_alloc() which needs a
> device-based alternative, and the non-of_xlate-based IOMMU drivers to
> either call iommu_fwspec_init() for themselves, or perhaps for x86
> plumbing in DMAR/IVRS equivalents of the IORT parsing to the
> infrastructure provided by this series.

I think it all boils down to how we end up implementing the per-device
iommu_ops look-up/binding, question is what do you want me to do with
this patch, it should be fine to drop it and use dev->bus->iommu_ops
for the look-up but I should know sooner rather than later to make
sure the series get another good round of testing.

Please let me know, thank you very much.

Lorenzo

[toc] | [next] | [standalone]

#1521589

On 14/11/16 10:26, Lorenzo Pieralisi wrote:
> Hi Robin, Joerg,
> 
> On Fri, Nov 11, 2016 at 05:43:39PM +0000, Robin Murphy wrote:
>> On 11/11/16 16:27, Joerg Roedel wrote:
>>> On Fri, Nov 11, 2016 at 04:17:37PM +0000, Robin Murphy wrote:
>>>> In the original of_iommu_configure design, the thought was that an ops
>>>> structure could be IOMMU-instance-specific (hence the later-removed
>>>> "priv" member), so I suppose right now it is mostly a hangover from
>>>> that. However, it's also what we initialise a device's fwspec with, so
>>>> becomes important again if we're ever going to get past the limitations
>>>> of buses-which-are-not-actually-buses[1].
>>>
>>> Yeah, I discussed this with a few others at LPC. My current idea is to
>>> tell the iommu-core which hardware-iommus exist in the system and a
>>> seperate iommu_ops ptr for each of them. Then every struct device can
>>> link to the iommu-instance it is translated by.
>>
>> Er, that sounds very much like a description of what we already have in
>> 4.9-rc. Every struct device now has an iommu_fwspec which encapsulates
>> both an iommu_ops pointer (which can perfectly well be per-instance if
>> the IOMMU driver wants) and a place for the IOMMU-private data to
>> replace the mess of archdata.iommu and driver-internal globals.
>>
>>> We are not there yet, but this will give you the same per-device
>>> iommu-ops as implemented here.
>>
>> With those two patches I linked to, which make the bulk of the IOMMU
>> core code per-device-ops-aware off the bat, I'd say we *are* already
>> pretty much there. It's only iommu_domain_alloc() which needs a
>> device-based alternative, and the non-of_xlate-based IOMMU drivers to
>> either call iommu_fwspec_init() for themselves, or perhaps for x86
>> plumbing in DMAR/IVRS equivalents of the IORT parsing to the
>> infrastructure provided by this series.
> 
> I think it all boils down to how we end up implementing the per-device
> iommu_ops look-up/binding, question is what do you want me to do with
> this patch, it should be fine to drop it and use dev->bus->iommu_ops
> for the look-up but I should know sooner rather than later to make
> sure the series get another good round of testing.

If we've already made the decision to move away from bus ops, I don't
see that it makes sense to deliberately introduce new dependencies on
them. Besides, as it stands, this patch literally implements "tell the
iommu-core which hardware-iommus exist in the system and a seperate
iommu_ops ptr for each of them" straight off.

Robin.

> 
> Please let me know, thank you very much.
> 
> Lorenzo
> 

[toc] | [prev] | [next] | [standalone]

#1521793

On Mon, Nov 14, 2016 at 12:00:47PM +0000, Robin Murphy wrote:
> If we've already made the decision to move away from bus ops, I don't
> see that it makes sense to deliberately introduce new dependencies on
> them. Besides, as it stands, this patch literally implements "tell the
> iommu-core which hardware-iommus exist in the system and a seperate
> iommu_ops ptr for each of them" straight off.

Not sure which code you are looking at, but as I see it we have only
per-device iommu-ops now (with this patch). That is different from
having core-visible hardware-iommu instances where devices could link
to.

Also the rest of iommu-core code still makes use of the per-bus ops. The
per-device ops are only used for the of_xlate fn-ptr.



	Joerg

[toc] | [prev] | [next] | [standalone]

#1521843

On Mon, Nov 14, 2016 at 04:52:23PM +0100, Joerg Roedel wrote:
> On Mon, Nov 14, 2016 at 12:00:47PM +0000, Robin Murphy wrote:
> > If we've already made the decision to move away from bus ops, I don't
> > see that it makes sense to deliberately introduce new dependencies on
> > them. Besides, as it stands, this patch literally implements "tell the
> > iommu-core which hardware-iommus exist in the system and a seperate
> > iommu_ops ptr for each of them" straight off.
> 
> Not sure which code you are looking at, but as I see it we have only
> per-device iommu-ops now (with this patch). That is different from
> having core-visible hardware-iommu instances where devices could link
> to.

This patch enables the IOMMU-OF-node<->device look-up on non-OF (ie
ACPI) systems by "converting" the of_node to a generic fwnode_handle,
that's all it does (and move the related look-up code from
drivers/iommu/of_iommu.c to drivers/iommu/iommu.c so that it does
not depend on OF_IOMMU any longer).

> Also the rest of iommu-core code still makes use of the per-bus ops. The
> per-device ops are only used for the of_xlate fn-ptr.

I can put this patch on the backburner and retrieve the iommu_ops
through the dev->bus path in the IORT xlate function (iort_iommu_xlate()
introduced in the last patch), the change is trivial and should work
just fine but it deserves a v8 to give everyone a chance to test it.

We would end-up handling the device->iommu_ops look-up differently in DT
and ACPI for streamid translations though, I am not sure I see a reason
why.

Thanks,
Lorenzo

[toc] | [prev] | [next] | [standalone]

#1521969

On 14/11/16 15:52, Joerg Roedel wrote:
> On Mon, Nov 14, 2016 at 12:00:47PM +0000, Robin Murphy wrote:
>> If we've already made the decision to move away from bus ops, I don't
>> see that it makes sense to deliberately introduce new dependencies on
>> them. Besides, as it stands, this patch literally implements "tell the
>> iommu-core which hardware-iommus exist in the system and a seperate
>> iommu_ops ptr for each of them" straight off.
> 
> Not sure which code you are looking at, but as I see it we have only
> per-device iommu-ops now (with this patch). That is different from
> having core-visible hardware-iommu instances where devices could link
> to.

The per-device IOMMU ops are already there since 57f98d2f61e1. This
patch generalises the other end, moving the "registering an IOMMU
instance" (i.e. iommu_fwentry) bit into the IOMMU core, from being
OF-specific. I'd be perfectly happy if we rename iommu_fwentry to
iommu_instance, fwnode_iommu_set_ops() to iommu_register_instance(), and
such if that makes the design intent clearer.

If you'd also prefer to replace iommu_fwspec::ops with an opaque
iommu_fwspec::iommu_instance pointer so that things are a bit more
centralised (and users are forced to go through the API rather then call
ops directly), I'd have no major objection either. My main point is that
we've been deliberately putting the relevant building blocks in place -
the of_iommu_{get,set}_ops stuff was designed from the start to
accommodate per-instance ops, via the ops pointer *being* the instance
token; the iommu_fwspec stuff is deliberately intended to provide
per-device ops on top of that. The raw functionality is either there in
iommu.c already, or moving there in patches already written, so if it
doesn't look right all we need to focus on is making it look right.

> Also the rest of iommu-core code still makes use of the per-bus ops. The
> per-device ops are only used for the of_xlate fn-ptr.

Hence my aforementioned patches intended for 4.10, directly following on
from introducing iommu_fwspec in 4.9:

http://www.mail-archive.com/iommu@lists.linux-foundation.org/msg14576.html

...the purpose being to provide a smooth transition from per-bus ops to
per-device, per-instance ops. Apply those and we're 90% of the way there
for OF-based IOMMU drivers (not that any of those actually need
per-instance ops, admittedly; I did prototype it for the ARM SMMU ages
ago, but it didn't seem worth the bother). Lorenzo's series broadens the
scope to ACPI-based systems and moves the generically-useful parts into
the core where we can easily build on them further if necessary. The
major remaining work is to convert external callers of the current
bus-dependent functions like iommu_domain_alloc(), iommu_present(), etc.
to device-based alternatives.

Robin.

[toc] | [prev] | [next] | [standalone]

#1522511

On Mon, Nov 14, 2016 at 06:25:16PM +0000, Robin Murphy wrote:
> On 14/11/16 15:52, Joerg Roedel wrote:
> > On Mon, Nov 14, 2016 at 12:00:47PM +0000, Robin Murphy wrote:
> >> If we've already made the decision to move away from bus ops, I don't
> >> see that it makes sense to deliberately introduce new dependencies on
> >> them. Besides, as it stands, this patch literally implements "tell the
> >> iommu-core which hardware-iommus exist in the system and a seperate
> >> iommu_ops ptr for each of them" straight off.
> > 
> > Not sure which code you are looking at, but as I see it we have only
> > per-device iommu-ops now (with this patch). That is different from
> > having core-visible hardware-iommu instances where devices could link
> > to.
> 
> The per-device IOMMU ops are already there since 57f98d2f61e1. This
> patch generalises the other end, moving the "registering an IOMMU
> instance" (i.e. iommu_fwentry) bit into the IOMMU core, from being
> OF-specific. I'd be perfectly happy if we rename iommu_fwentry to
> iommu_instance, fwnode_iommu_set_ops() to iommu_register_instance(), and
> such if that makes the design intent clearer.

I second that and I need to know what to do with this patch sooner
rather than later so it is time we make a decision please.

Joerg, what's your opinion ?

Thanks,
Lorenzo

> If you'd also prefer to replace iommu_fwspec::ops with an opaque
> iommu_fwspec::iommu_instance pointer so that things are a bit more
> centralised (and users are forced to go through the API rather then call
> ops directly), I'd have no major objection either. My main point is that
> we've been deliberately putting the relevant building blocks in place -
> the of_iommu_{get,set}_ops stuff was designed from the start to
> accommodate per-instance ops, via the ops pointer *being* the instance
> token; the iommu_fwspec stuff is deliberately intended to provide
> per-device ops on top of that. The raw functionality is either there in
> iommu.c already, or moving there in patches already written, so if it
> doesn't look right all we need to focus on is making it look right.
> 
> > Also the rest of iommu-core code still makes use of the per-bus ops. The
> > per-device ops are only used for the of_xlate fn-ptr.
> 
> Hence my aforementioned patches intended for 4.10, directly following on
> from introducing iommu_fwspec in 4.9:
> 
> http://www.mail-archive.com/iommu@lists.linux-foundation.org/msg14576.html
> 
> ...the purpose being to provide a smooth transition from per-bus ops to
> per-device, per-instance ops. Apply those and we're 90% of the way there
> for OF-based IOMMU drivers (not that any of those actually need
> per-instance ops, admittedly; I did prototype it for the ARM SMMU ages
> ago, but it didn't seem worth the bother). Lorenzo's series broadens the
> scope to ACPI-based systems and moves the generically-useful parts into
> the core where we can easily build on them further if necessary. The
> major remaining work is to convert external callers of the current
> bus-dependent functions like iommu_domain_alloc(), iommu_present(), etc.
> to device-based alternatives.
> 
> Robin.

[toc] | [prev] | [next] | [standalone]

#1523347

Hi Joerg,

On Mon, Nov 14, 2016 at 06:25:16PM +0000, Robin Murphy wrote:
> On 14/11/16 15:52, Joerg Roedel wrote:
> > On Mon, Nov 14, 2016 at 12:00:47PM +0000, Robin Murphy wrote:
> >> If we've already made the decision to move away from bus ops, I don't
> >> see that it makes sense to deliberately introduce new dependencies on
> >> them. Besides, as it stands, this patch literally implements "tell the
> >> iommu-core which hardware-iommus exist in the system and a seperate
> >> iommu_ops ptr for each of them" straight off.
> > 
> > Not sure which code you are looking at, but as I see it we have only
> > per-device iommu-ops now (with this patch). That is different from
> > having core-visible hardware-iommu instances where devices could link
> > to.
> 
> The per-device IOMMU ops are already there since 57f98d2f61e1. This
> patch generalises the other end, moving the "registering an IOMMU
> instance" (i.e. iommu_fwentry) bit into the IOMMU core, from being
> OF-specific. I'd be perfectly happy if we rename iommu_fwentry to
> iommu_instance, fwnode_iommu_set_ops() to iommu_register_instance(), and
> such if that makes the design intent clearer.

I can easily make the changes Robin suggests above, I need to know
what to do with this patch it is the last blocking point for this
series and time is running out I can revert to using dev->bus to
retrieve iommu_ops (even though I do not think it makes sense given
what Robin outlines below) but I need to know please, we can't gate
an entire series for this patch that is just syntactic sugar.

Thanks !
Lorenzo

> If you'd also prefer to replace iommu_fwspec::ops with an opaque
> iommu_fwspec::iommu_instance pointer so that things are a bit more
> centralised (and users are forced to go through the API rather then call
> ops directly), I'd have no major objection either. My main point is that
> we've been deliberately putting the relevant building blocks in place -
> the of_iommu_{get,set}_ops stuff was designed from the start to
> accommodate per-instance ops, via the ops pointer *being* the instance
> token; the iommu_fwspec stuff is deliberately intended to provide
> per-device ops on top of that. The raw functionality is either there in
> iommu.c already, or moving there in patches already written, so if it
> doesn't look right all we need to focus on is making it look right.
> 
> > Also the rest of iommu-core code still makes use of the per-bus ops. The
> > per-device ops are only used for the of_xlate fn-ptr.
> 
> Hence my aforementioned patches intended for 4.10, directly following on
> from introducing iommu_fwspec in 4.9:
> 
> http://www.mail-archive.com/iommu@lists.linux-foundation.org/msg14576.html
> 
> ...the purpose being to provide a smooth transition from per-bus ops to
> per-device, per-instance ops. Apply those and we're 90% of the way there
> for OF-based IOMMU drivers (not that any of those actually need
> per-instance ops, admittedly; I did prototype it for the ARM SMMU ages
> ago, but it didn't seem worth the bother). Lorenzo's series broadens the
> scope to ACPI-based systems and moves the generically-useful parts into
> the core where we can easily build on them further if necessary. The
> major remaining work is to convert external callers of the current
> bus-dependent functions like iommu_domain_alloc(), iommu_present(), etc.
> to device-based alternatives.
> 
> Robin.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web