[PATCH] iommu-groups: Add list_head for iommu groups

[PATCH] iommu-groups: Add list_head for iommu groups

[Ritesh Harjani]

Hi All,

Please find the below patch and let me know your suggestions on this.

With this patch iommu groups can be linked and iommu driver will
get to know which all iommu groups belongs to a particular iommu device.



Each iommu device might have multiple iommu groups,
with each group populated with many devices. With
this configuration, iommu driver may require a list
of iommu groups belonging to each iommu device.

Add list_head struct entry in iommu_group.

Signed-off-by: Ritesh Harjani <ritesh.harjani-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/iommu/iommu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e5555fc..70588dc 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -36,6 +36,10 @@ static struct ida iommu_group_ida;
 static struct mutex iommu_group_mutex;

 struct iommu_group {
+       struct list_head list;  /* lower level iommu driver structure
+                                * may require list of iommu groups
+                                * belonging to that iommu device.
+                                */
        struct kobject kobj;
        struct kobject *devices_kobj;
        struct list_head devices;
--
1.8.1.3

Re: [PATCH] iommu-groups: Add list_head for iommu groups

[Alex Williamson]

On Fri, 2014-04-04 at 11:12 +0530, Ritesh Harjani wrote:
> Hi All,
> 
> Please find the below patch and let me know your suggestions on this.
> 
> With this patch iommu groups can be linked and iommu driver will
> get to know which all iommu groups belongs to a particular iommu device.
> 
> 
> 
> Each iommu device might have multiple iommu groups,
> with each group populated with many devices. With
> this configuration, iommu driver may require a list
> of iommu groups belonging to each iommu device.
> 
> Add list_head struct entry in iommu_group.
> 
> Signed-off-by: Ritesh Harjani <ritesh.harjani-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/iommu/iommu.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index e5555fc..70588dc 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -36,6 +36,10 @@ static struct ida iommu_group_ida;
>  static struct mutex iommu_group_mutex;
> 
>  struct iommu_group {
> +       struct list_head list;  /* lower level iommu driver structure
> +                                * may require list of iommu groups
> +                                * belonging to that iommu device.
> +                                */


To have a list would require locking to manage that list, but you seem
to imply that the iommu driver would be managing this list... but there
might be multiple iommu drivers in a single system.  Groups are
currently independent of each other by design, trying to manage them in
a list defeats that.  There is also no example provided here of why we
would need or even want to do this.  Thanks,

Alex

>         struct kobject kobj;
>         struct kobject *devices_kobj;
>         struct list_head devices;
> --
> 1.8.1.3

Re: [PATCH] iommu-groups: Add list_head for iommu groups

[Ritesh Harjani]

On Fri, Apr 4, 2014 at 7:33 PM, Alex Williamson
<alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, 2014-04-04 at 11:12 +0530, Ritesh Harjani wrote:
>> Hi All,
>>
>> Please find the below patch and let me know your suggestions on this.
>>
>> With this patch iommu groups can be linked and iommu driver will
>> get to know which all iommu groups belongs to a particular iommu device.
>>
>>
>>
>> Each iommu device might have multiple iommu groups,
>> with each group populated with many devices. With
>> this configuration, iommu driver may require a list
>> of iommu groups belonging to each iommu device.
>>
>> Add list_head struct entry in iommu_group.
>>
>> Signed-off-by: Ritesh Harjani <ritesh.harjani-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/iommu/iommu.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index e5555fc..70588dc 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -36,6 +36,10 @@ static struct ida iommu_group_ida;
>>  static struct mutex iommu_group_mutex;
>>
>>  struct iommu_group {
>> +       struct list_head list;  /* lower level iommu driver structure
>> +                                * may require list of iommu groups
>> +                                * belonging to that iommu device.
>> +                                */
>
>
> To have a list would require locking to manage that list, but you seem
> to imply that the iommu driver would be managing this list... but there
> might be multiple iommu drivers in a single system. Groups are
> currently independent of each other by design, trying to manage them in
> a list defeats that.

HOW ??

> There is also no example provided here of why we
> would need or even want to do this.  Thanks,

Say multiple groups are linked to one iommu device. Each iommu_group maintains
one iommu_data specific to their group. Now, if iommu driver of a
iommu device wants
to query all the iommu groups and thus all devices which belongs to
this iommu hardware,
then in that it will be nice if iommu_group struct provides a list_entry ?

Thanks
Ritesh







>
> Alex
>
>>         struct kobject kobj;
>>         struct kobject *devices_kobj;
>>         struct list_head devices;
>> --
>> 1.8.1.3
>
>
>

Re: [PATCH] iommu-groups: Add list_head for iommu groups

[Alex Williamson]

On Mon, 2014-04-07 at 15:23 +0530, Ritesh Harjani wrote:
> On Fri, Apr 4, 2014 at 7:33 PM, Alex Williamson
> <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Fri, 2014-04-04 at 11:12 +0530, Ritesh Harjani wrote:
> >> Hi All,
> >>
> >> Please find the below patch and let me know your suggestions on this.
> >>
> >> With this patch iommu groups can be linked and iommu driver will
> >> get to know which all iommu groups belongs to a particular iommu device.
> >>
> >>
> >>
> >> Each iommu device might have multiple iommu groups,
> >> with each group populated with many devices. With
> >> this configuration, iommu driver may require a list
> >> of iommu groups belonging to each iommu device.
> >>
> >> Add list_head struct entry in iommu_group.
> >>
> >> Signed-off-by: Ritesh Harjani <ritesh.harjani-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> ---
> >>  drivers/iommu/iommu.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >> index e5555fc..70588dc 100644
> >> --- a/drivers/iommu/iommu.c
> >> +++ b/drivers/iommu/iommu.c
> >> @@ -36,6 +36,10 @@ static struct ida iommu_group_ida;
> >>  static struct mutex iommu_group_mutex;
> >>
> >>  struct iommu_group {
> >> +       struct list_head list;  /* lower level iommu driver structure
> >> +                                * may require list of iommu groups
> >> +                                * belonging to that iommu device.
> >> +                                */
> >
> >
> > To have a list would require locking to manage that list, but you seem
> > to imply that the iommu driver would be managing this list... but there
> > might be multiple iommu drivers in a single system. Groups are
> > currently independent of each other by design, trying to manage them in
> > a list defeats that.
> 
> HOW ??
> 
> > There is also no example provided here of why we
> > would need or even want to do this.  Thanks,
> 
> Say multiple groups are linked to one iommu device. Each iommu_group maintains
> one iommu_data specific to their group. Now, if iommu driver of a
> iommu device wants
> to query all the iommu groups and thus all devices which belongs to
> this iommu hardware,
> then in that it will be nice if iommu_group struct provides a list_entry ?

iommu_data is owned by the iommu driver, so if a driver wants to create
such a list, why would it not do so there?  What's being requested here
is an extension to a private data structure with no concrete example of
why this is needed.  Current users don't seem to have any need for this.
Furthermore struct iommu_group is intentionally opaque to everything
outside of iommu.c, so without either relocating this structure to a
header or adding a list service to the iommu group interfaces, the
change is completely useless.  Thanks,

Alex

Re: [PATCH] iommu-groups: Add list_head for iommu groups

[Ritesh Harjani]

Yes, Alex.

Thanks for the suggestions.
Ritesh


On Mon, Apr 7, 2014 at 7:44 PM, Alex Williamson
<alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Mon, 2014-04-07 at 15:23 +0530, Ritesh Harjani wrote:
>> On Fri, Apr 4, 2014 at 7:33 PM, Alex Williamson
>> <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > On Fri, 2014-04-04 at 11:12 +0530, Ritesh Harjani wrote:
>> >> Hi All,
>> >>
>> >> Please find the below patch and let me know your suggestions on this.
>> >>
>> >> With this patch iommu groups can be linked and iommu driver will
>> >> get to know which all iommu groups belongs to a particular iommu device.
>> >>
>> >>
>> >>
>> >> Each iommu device might have multiple iommu groups,
>> >> with each group populated with many devices. With
>> >> this configuration, iommu driver may require a list
>> >> of iommu groups belonging to each iommu device.
>> >>
>> >> Add list_head struct entry in iommu_group.
>> >>
>> >> Signed-off-by: Ritesh Harjani <ritesh.harjani-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >> ---
>> >>  drivers/iommu/iommu.c | 4 ++++
>> >>  1 file changed, 4 insertions(+)
>> >>
>> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> >> index e5555fc..70588dc 100644
>> >> --- a/drivers/iommu/iommu.c
>> >> +++ b/drivers/iommu/iommu.c
>> >> @@ -36,6 +36,10 @@ static struct ida iommu_group_ida;
>> >>  static struct mutex iommu_group_mutex;
>> >>
>> >>  struct iommu_group {
>> >> +       struct list_head list;  /* lower level iommu driver structure
>> >> +                                * may require list of iommu groups
>> >> +                                * belonging to that iommu device.
>> >> +                                */
>> >
>> >
>> > To have a list would require locking to manage that list, but you seem
>> > to imply that the iommu driver would be managing this list... but there
>> > might be multiple iommu drivers in a single system. Groups are
>> > currently independent of each other by design, trying to manage them in
>> > a list defeats that.
>>
>> HOW ??
>>
>> > There is also no example provided here of why we
>> > would need or even want to do this.  Thanks,
>>
>> Say multiple groups are linked to one iommu device. Each iommu_group maintains
>> one iommu_data specific to their group. Now, if iommu driver of a
>> iommu device wants
>> to query all the iommu groups and thus all devices which belongs to
>> this iommu hardware,
>> then in that it will be nice if iommu_group struct provides a list_entry ?
>
> iommu_data is owned by the iommu driver, so if a driver wants to create
> such a list, why would it not do so there?  What's being requested here
> is an extension to a private data structure with no concrete example of
> why this is needed.  Current users don't seem to have any need for this.
> Furthermore struct iommu_group is intentionally opaque to everything
> outside of iommu.c, so without either relocating this structure to a
> header or adding a list service to the iommu group interfaces, the
> change is completely useless.  Thanks,
>
> Alex
>

Re: [PATCH] iommu-groups: Add list_head for iommu groups

[Joerg Roedel]

On Mon, Apr 07, 2014 at 09:54:54PM +0530, Ritesh Harjani wrote:
> On Mon, Apr 7, 2014 at 7:44 PM, Alex Williamson
> <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Mon, 2014-04-07 at 15:23 +0530, Ritesh Harjani wrote:
> >> Say multiple groups are linked to one iommu device. Each iommu_group maintains
> >> one iommu_data specific to their group. Now, if iommu driver of a
> >> iommu device wants
> >> to query all the iommu groups and thus all devices which belongs to
> >> this iommu hardware,
> >> then in that it will be nice if iommu_group struct provides a list_entry ?
> >
> > iommu_data is owned by the iommu driver, so if a driver wants to create
> > such a list, why would it not do so there?  What's being requested here
> > is an extension to a private data structure with no concrete example of
> > why this is needed.  Current users don't seem to have any need for this.
> > Furthermore struct iommu_group is intentionally opaque to everything
> > outside of iommu.c, so without either relocating this structure to a
> > header or adding a list service to the iommu group interfaces, the
> > change is completely useless.  Thanks,

I agree with Alex. Besides, when submitting such a change please also
include the code that makes use of it. Only then we can see if the
change is really necessary.


	Joerg

Re: [PATCH] iommu-groups: Add list_head for iommu groups

[Ritesh Harjani]

Sure Joerg,

Thanks
Ritesh


On Tue, Apr 8, 2014 at 3:48 PM, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Apr 07, 2014 at 09:54:54PM +0530, Ritesh Harjani wrote:
>> On Mon, Apr 7, 2014 at 7:44 PM, Alex Williamson
>> <alex.williamson-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > On Mon, 2014-04-07 at 15:23 +0530, Ritesh Harjani wrote:
>> >> Say multiple groups are linked to one iommu device. Each iommu_group maintains
>> >> one iommu_data specific to their group. Now, if iommu driver of a
>> >> iommu device wants
>> >> to query all the iommu groups and thus all devices which belongs to
>> >> this iommu hardware,
>> >> then in that it will be nice if iommu_group struct provides a list_entry ?
>> >
>> > iommu_data is owned by the iommu driver, so if a driver wants to create
>> > such a list, why would it not do so there?  What's being requested here
>> > is an extension to a private data structure with no concrete example of
>> > why this is needed.  Current users don't seem to have any need for this.
>> > Furthermore struct iommu_group is intentionally opaque to everything
>> > outside of iommu.c, so without either relocating this structure to a
>> > header or adding a list service to the iommu group interfaces, the
>> > change is completely useless.  Thanks,
>
> I agree with Alex. Besides, when submitting such a change please also
> include the code that makes use of it. Only then we can see if the
> change is really necessary.
>
>
>         Joerg
>
>