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

Groups > linux.kernel > #1552249 > unrolled thread

[PATCH v6 07/18] iommu: Implement reserved_regions iommu-group sysfs file

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

  [PATCH v6 07/18] iommu: Implement reserved_regions iommu-group sysfs file Eric Auger <eric.auger@redhat.com> - 2017-01-05 20:10 +0100
    Re: [PATCH v6 07/18] iommu: Implement reserved_regions iommu-group  sysfs file Joerg Roedel <joro@8bytes.org> - 2017-01-06 12:10 +0100
      Re: [PATCH v6 07/18] iommu: Implement reserved_regions iommu-group  sysfs file Auger Eric <eric.auger@redhat.com> - 2017-01-06 13:00 +0100
        Re: [PATCH v6 07/18] iommu: Implement reserved_regions iommu-group  sysfs file Joerg Roedel <joro@8bytes.org> - 2017-01-06 13:50 +0100
          Re: [PATCH v6 07/18] iommu: Implement reserved_regions iommu-group  sysfs file Auger Eric <eric.auger@redhat.com> - 2017-01-06 14:10 +0100
          Re: [PATCH v6 07/18] iommu: Implement reserved_regions iommu-group  sysfs file Auger Eric <eric.auger@redhat.com> - 2017-01-06 18:20 +0100
            Re: [PATCH v6 07/18] iommu: Implement reserved_regions iommu-group  sysfs file Auger Eric <eric.auger@redhat.com> - 2017-01-08 17:30 +0100

#1552249 — [PATCH v6 07/18] iommu: Implement reserved_regions iommu-group sysfs file

A new iommu-group sysfs attribute file is introduced. It contains
the list of reserved regions for the iommu-group. Each reserved
region is described on a separate line:
- first field is the start IOVA address,
- second is the end IOVA address,

Signed-off-by: Eric Auger <eric.auger@redhat.com>

---

v3 -> v4:
- add cast to long long int when printing to avoid warning on
  i386
- change S_IRUGO into 0444
- remove sort. The list is natively sorted now.

The file layout is inspired of /sys/bus/pci/devices/BDF/resource.
I also read Documentation/filesystems/sysfs.txt so I expect this
to be frowned upon.
---
 drivers/iommu/iommu.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index a2d51b3..15756d8 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -211,8 +211,32 @@ int iommu_get_group_resv_regions(struct iommu_group *group,
 }
 EXPORT_SYMBOL_GPL(iommu_get_group_resv_regions);
 
+static ssize_t iommu_group_show_resv_regions(struct iommu_group *group,
+					     char *buf)
+{
+	struct iommu_resv_region *region, *next;
+	struct list_head group_resv_regions;
+	char *str = buf;
+
+	INIT_LIST_HEAD(&group_resv_regions);
+	iommu_get_group_resv_regions(group, &group_resv_regions);
+
+	list_for_each_entry_safe(region, next, &group_resv_regions, list) {
+		str += sprintf(str, "0x%016llx 0x%016llx\n",
+			       (long long int)region->start,
+			       (long long int)(region->start +
+						region->length - 1));
+		kfree(region);
+	}
+
+	return (str - buf);
+}
+
 static IOMMU_GROUP_ATTR(name, S_IRUGO, iommu_group_show_name, NULL);
 
+static IOMMU_GROUP_ATTR(reserved_regions, 0444,
+			iommu_group_show_resv_regions, NULL);
+
 static void iommu_group_release(struct kobject *kobj)
 {
 	struct iommu_group *group = to_iommu_group(kobj);
@@ -227,6 +251,8 @@ static void iommu_group_release(struct kobject *kobj)
 	if (group->default_domain)
 		iommu_domain_free(group->default_domain);
 
+	iommu_group_remove_file(group, &iommu_group_attr_reserved_regions);
+
 	kfree(group->name);
 	kfree(group);
 }
@@ -290,6 +316,11 @@ struct iommu_group *iommu_group_alloc(void)
 	 */
 	kobject_put(&group->kobj);
 
+	ret = iommu_group_create_file(group,
+				      &iommu_group_attr_reserved_regions);
+	if (ret)
+		return ERR_PTR(ret);
+
 	pr_debug("Allocated group %d\n", group->id);
 
 	return group;
-- 
1.9.1

[toc] | [next] | [standalone]

#1552696 — Re: [PATCH v6 07/18] iommu: Implement reserved_regions iommu-group sysfs file

On Thu, Jan 05, 2017 at 07:04:35PM +0000, Eric Auger wrote:
> +	list_for_each_entry_safe(region, next, &group_resv_regions, list) {
> +		str += sprintf(str, "0x%016llx 0x%016llx\n",
> +			       (long long int)region->start,
> +			       (long long int)(region->start +
> +						region->length - 1));
> +		kfree(region);
> +	}

I think it also makes sense to report the type of the reserved region.



	Joerg

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

#1552719 — Re: [PATCH v6 07/18] iommu: Implement reserved_regions iommu-group sysfs file

Hi Joerg,

On 06/01/2017 12:00, Joerg Roedel wrote:
> On Thu, Jan 05, 2017 at 07:04:35PM +0000, Eric Auger wrote:
>> +	list_for_each_entry_safe(region, next, &group_resv_regions, list) {
>> +		str += sprintf(str, "0x%016llx 0x%016llx\n",
>> +			       (long long int)region->start,
>> +			       (long long int)(region->start +
>> +						region->length - 1));
>> +		kfree(region);
>> +	}
> 
> I think it also makes sense to report the type of the reserved region.

What is the best practice in that case? Shall we put the type enum
values as strings such as:
- direct
- nomap
- msi

and document that in Documentation/ABI/testing/sysfs-kernel-iommu_groups

Thanks

Eric
> 
> 
> 
> 	Joerg
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

#1552754 — Re: [PATCH v6 07/18] iommu: Implement reserved_regions iommu-group sysfs file

On Fri, Jan 06, 2017 at 12:46:05PM +0100, Auger Eric wrote:
> On 06/01/2017 12:00, Joerg Roedel wrote:

> > I think it also makes sense to report the type of the reserved region.
> 
> What is the best practice in that case? Shall we put the type enum
> values as strings such as:
> - direct
> - nomap
> - msi
> 
> and document that in Documentation/ABI/testing/sysfs-kernel-iommu_groups

Yes, a string would be good. An probably 'reserved' is a better name
than nomap?


	Joerg

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

#1552764 — Re: [PATCH v6 07/18] iommu: Implement reserved_regions iommu-group sysfs file

Hi,

On 06/01/2017 13:48, Joerg Roedel wrote:
> On Fri, Jan 06, 2017 at 12:46:05PM +0100, Auger Eric wrote:
>> On 06/01/2017 12:00, Joerg Roedel wrote:
> 
>>> I think it also makes sense to report the type of the reserved region.
>>
>> What is the best practice in that case? Shall we put the type enum
>> values as strings such as:
>> - direct
>> - nomap
>> - msi
>>
>> and document that in Documentation/ABI/testing/sysfs-kernel-iommu_groups
> 
> Yes, a string would be good. An probably 'reserved' is a better name
> than nomap?

OK that's equal to me.

Thanks

Eric
> 
> 
> 	Joerg
> 

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

#1552974 — Re: [PATCH v6 07/18] iommu: Implement reserved_regions iommu-group sysfs file

Hi Joerg, Robin,

On 06/01/2017 13:48, Joerg Roedel wrote:
> On Fri, Jan 06, 2017 at 12:46:05PM +0100, Auger Eric wrote:
>> On 06/01/2017 12:00, Joerg Roedel wrote:
> 
>>> I think it also makes sense to report the type of the reserved region.
>>
>> What is the best practice in that case? Shall we put the type enum
>> values as strings such as:
>> - direct
>> - nomap
>> - msi
>>
>> and document that in Documentation/ABI/testing/sysfs-kernel-iommu_groups
> 
> Yes, a string would be good. An probably 'reserved' is a better name
> than nomap?
the iommu_insert_resv_region() function that builds the group reserved
region list sorts all regions and handles the case where there is an
overlap between regions. Current code does not care about the type of
regions. So in case a NOMAP region overlaps with a direct-mapped region,
what is reported to the user space is the superset and the type depends
on the overlap. This was suggested by Robin at some point to handle
overlaps.

I guess I should merge regions only in case the types equal?

I remember that Alex thought that user-space should not care so much
about the type of the regions so I tought it was better for the
user-space to have a minimal view of the regions.

On the other hand, this issue of merging regions of different types
should not happen often but I prefer to highlight the potential issue.

What is your guidance?

Thanks

Eric
> 
> 
> 	Joerg
> 

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

#1553902 — Re: [PATCH v6 07/18] iommu: Implement reserved_regions iommu-group sysfs file

Hi,

On 06/01/2017 18:18, Auger Eric wrote:
> Hi Joerg, Robin,
> 
> On 06/01/2017 13:48, Joerg Roedel wrote:
>> On Fri, Jan 06, 2017 at 12:46:05PM +0100, Auger Eric wrote:
>>> On 06/01/2017 12:00, Joerg Roedel wrote:
>>
>>>> I think it also makes sense to report the type of the reserved region.
>>>
>>> What is the best practice in that case? Shall we put the type enum
>>> values as strings such as:
>>> - direct
>>> - nomap
>>> - msi
>>>
>>> and document that in Documentation/ABI/testing/sysfs-kernel-iommu_groups
>>
>> Yes, a string would be good. An probably 'reserved' is a better name
>> than nomap?
> the iommu_insert_resv_region() function that builds the group reserved
> region list sorts all regions and handles the case where there is an
> overlap between regions. Current code does not care about the type of
> regions. So in case a NOMAP region overlaps with a direct-mapped region,
> what is reported to the user space is the superset and the type depends
> on the overlap. This was suggested by Robin at some point to handle
> overlaps.
> 
> I guess I should merge regions only in case the types equal?
> 
> I remember that Alex thought that user-space should not care so much
> about the type of the regions so I tought it was better for the
> user-space to have a minimal view of the regions.
> 
> On the other hand, this issue of merging regions of different types
> should not happen often but I prefer to highlight the potential issue.

> 
> What is your guidance?

Please forget the question. From an API point of view It does not make
sense that iommu_insert_resv_region() merges regions of a different
types since the type field becomes unreliable. I will fix this.

Thanks

Eric
> 
> Thanks
> 
> Eric
>>
>>
>> 	Joerg
>>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web