linux-kernel - Re: [RFC v3 06/10] iommu: iommu_get_group_resv

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c2a635f7-6b2b-d8f8-12c3-7759cbd7da43@redhat.com>
Date:   Wed, 7 Dec 2016 16:13:46 +0100
From:   Auger Eric <eric.auger@...hat.com>
To:     Robin Murphy <robin.murphy@....com>, eric.auger.pro@...il.com,
        christoffer.dall@...aro.org, marc.zyngier@....com,
        alex.williamson@...hat.com, will.deacon@....com, joro@...tes.org,
        tglx@...utronix.de, jason@...edaemon.net,
        linux-arm-kernel@...ts.infradead.org
Cc:     kvm@...r.kernel.org, drjones@...hat.com,
        linux-kernel@...r.kernel.org, pranav.sawargaonkar@...il.com,
        iommu@...ts.linux-foundation.org, punit.agrawal@....com,
        diana.craciun@....com
Subject: Re: [RFC v3 06/10] iommu: iommu_get_group_resv_regions

Hi Robin,

On 06/12/2016 19:13, Robin Murphy wrote:
> On 15/11/16 13:09, Eric Auger wrote:
>> Introduce iommu_get_group_resv_regions whose role consists in
>> enumerating all devices from the group and collecting their
>> reserved regions. It checks duplicates.
>>
>> Signed-off-by: Eric Auger <eric.auger@...hat.com>
>>
>> ---
>>
>> - we do not move list elements from device to group list since
>>   the iommu_put_resv_regions() could not be called.
>> - at the moment I did not introduce any iommu_put_group_resv_regions
>>   since it simply consists in voiding/freeing the list
>> ---
>>  drivers/iommu/iommu.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/iommu.h |  8 ++++++++
>>  2 files changed, 61 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index a4530ad..e0fbcc5 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -133,6 +133,59 @@ static ssize_t iommu_group_show_name(struct iommu_group *group, char *buf)
>>  	return sprintf(buf, "%s\n", group->name);
>>  }
>>  
>> +static bool iommu_resv_region_present(struct iommu_resv_region *region,
>> +				      struct list_head *head)
>> +{
>> +	struct iommu_resv_region *entry;
>> +
>> +	list_for_each_entry(entry, head, list) {
>> +		if ((region->start == entry->start) &&
>> +		    (region->length == entry->length) &&
>> +		    (region->prot == entry->prot))
>> +			return true;
>> +	}
>> +	return false;
>> +}
>> +
>> +static int
>> +iommu_insert_device_resv_regions(struct list_head *dev_resv_regions,
>> +				 struct list_head *group_resv_regions)
>> +{
>> +	struct iommu_resv_region *entry, *region;
>> +
>> +	list_for_each_entry(entry, dev_resv_regions, list) {
>> +		if (iommu_resv_region_present(entry, group_resv_regions))
>> +			continue;
> 
> In the case of overlapping regions which _aren't_ an exact match, would
> it be better to expand the existing one rather than leave the caller to
> sort it out? It seems a bit inconsistent to handle only the one case here.

Well this is mostly here to avoid inserting several times the same PCIe
host bridge windows (retrieved from several PCIe EP attached to the same
bridge). I don't know if it is worth making things over-complicated. Do
you have another situation in mind?
> 
>> +		region = iommu_alloc_resv_region(entry->start, entry->length,
>> +					       entry->prot);
>> +		if (!region)
>> +			return -ENOMEM;
>> +
>> +		list_add_tail(&region->list, group_resv_regions);
>> +	}
>> +	return 0;
>> +}
>> +
>> +int iommu_get_group_resv_regions(struct iommu_group *group,
>> +				 struct list_head *head)
>> +{
>> +	struct iommu_device *device;
>> +	int ret = 0;
>> +
>> +	list_for_each_entry(device, &group->devices, list) {
> 
> Should we not be taking the group mutex around this?
Yes you're right.

Thanks

Eric
> 
> Robin.
> 
>> +		struct list_head dev_resv_regions;
>> +
>> +		INIT_LIST_HEAD(&dev_resv_regions);
>> +		iommu_get_resv_regions(device->dev, &dev_resv_regions);
>> +		ret = iommu_insert_device_resv_regions(&dev_resv_regions, head);
>> +		iommu_put_resv_regions(device->dev, &dev_resv_regions);
>> +		if (ret)
>> +			break;
>> +	}
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_get_group_resv_regions);
>> +
>>  static IOMMU_GROUP_ATTR(name, S_IRUGO, iommu_group_show_name, NULL);
>>  
>>  static void iommu_group_release(struct kobject *kobj)
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 0aea877..0f7ae2c 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -243,6 +243,8 @@ extern void iommu_set_fault_handler(struct iommu_domain *domain,
>>  extern int iommu_request_dm_for_dev(struct device *dev);
>>  extern struct iommu_resv_region *
>>  iommu_alloc_resv_region(phys_addr_t start, size_t length, unsigned int prot);
>> +extern int iommu_get_group_resv_regions(struct iommu_group *group,
>> +					struct list_head *head);
>>  
>>  extern int iommu_attach_group(struct iommu_domain *domain,
>>  			      struct iommu_group *group);
>> @@ -462,6 +464,12 @@ static inline void iommu_put_resv_regions(struct device *dev,
>>  	return NULL;
>>  }
>>  
>> +static inline int iommu_get_group_resv_regions(struct iommu_group *group,
>> +					       struct list_head *head)
>> +{
>> +	return -ENODEV;
>> +}
>> +
>>  static inline int iommu_request_dm_for_dev(struct device *dev)
>>  {
>>  	return -ENODEV;
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>