Re: [PATCH v8 7/8] genirq/msi: map/unmap the MSI doorbells on msi_domain_alloc/free_irqs

[Eric Auger]

Hi Marc,
On 05/04/2016 03:21 PM, Marc Zyngier wrote:
> On 28/04/16 09:22, Eric Auger wrote:
>> This patch handles the iommu mapping of MSI doorbells that require to
>> be mapped in an iommu domain. This happens on msi_domain_alloc/free_irqs
>> since this is called in code that can sleep (pci_enable/disable_msi):
>> iommu_map/unmap is not stated as atomic. On msi_domain_(de)activate and
>> msi_domain_set_affinity, which must be atomic, we just lookup for this
>> pre-allocated/mapped IOVA.
>>
>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>>
>> ---
>> v7 -> v8:
>> - new percpu pointer type
>> - exit from the irq domain hierarchy parsing on first map/unmap success
>> - reset desc->irq to 0 on mapping failure
>>
>> v7: creation
>> ---
>>  kernel/irq/msi.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>>  1 file changed, 79 insertions(+), 8 deletions(-)
>>
>> diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c
>> index 72bf4d6..d5f95e6 100644
>> --- a/kernel/irq/msi.c
>> +++ b/kernel/irq/msi.c
>> @@ -14,6 +14,8 @@
>>  #include <linux/irq.h>
>>  #include <linux/irqdomain.h>
>>  #include <linux/msi.h>
>> +#include <linux/msi-iommu.h>
>> +#include <linux/iommu.h>
>>  
>>  /* Temparory solution for building, will be removed later */
>>  #include <linux/pci.h>
>> @@ -322,6 +324,56 @@ int msi_domain_populate_irqs(struct irq_domain *domain, struct device *dev,
>>  }
>>  
>>  /**
>> + * msi_handle_doorbell_mappings: in case the irq data corresponds to an
>> + * MSI that requires iommu mapping, traverse the irq domain hierarchy
>> + * to retrieve the doorbells to handle and iommu_map/unmap them according
>> + * to @map boolean.
>> + *
>> + * @data: irq data handle
>> + * @map: mapping if true, unmapping if false
>> + */
>> +static int msi_handle_doorbell_mappings(struct irq_data *data, bool map)
>> +{
>> +	for (; data; data = data->parent_data) {
>> +		struct device *dev =
>> +			msi_desc_to_dev(irq_data_get_msi_desc(data));
>> +		struct irq_chip *chip = irq_data_get_irq_chip(data);
>> +		const struct irq_chip_msi_doorbell_info *dbinfo;
>> +		struct iommu_domain *domain;
>> +		phys_addr_t __percpu *db_addr;
>> +		dma_addr_t iova;
>> +		int ret = 0, i;
>> +
>> +		domain = iommu_msi_domain(dev);
>> +		if (!domain)
>> +			continue;
>> +
>> +		if (!chip->msi_doorbell_info)
>> +			continue;
>> +
>> +		dbinfo = chip->msi_doorbell_info(data);
>> +		if (!dbinfo)
>> +			return -EINVAL;
>> +
>> +		for (i = 0; i < dbinfo->nb_doorbells; i++) {
>> +			db_addr = per_cpu_ptr(dbinfo->percpu_doorbells, i);
>> +			if (map) {
>> +				ret = iommu_msi_get_doorbell_iova(domain,
>> +								  *db_addr,
>> +								  dbinfo->size,
>> +								  dbinfo->prot,
>> +								  &iova);
>> +				if (ret)
>> +					return ret;
>> +			} else
>> +				iommu_msi_put_doorbell_iova(domain, *db_addr);
>> +		}
>> +		break;
>> +	}
>> +	return 0;
>> +}
> 
> I'm really not fond of this whole loop. Could you try to decouple the
> irq_data parsing (looking for a msi_doorbell_info method) from the
> actual mapping/unmapping? This would make it a lot more readable.
> Something along the lines of:
Just sent v9 where I addressed all your comments. Please let me know
whether this looks better.
> 
> 	struct device *dev;
> 	struct irq_chip *chip;
> 	struct iommu_domain *domain;
> 	const struct irq_chip_msi_doorbell_info *dbinfo;
> 
> 	while (data) {
> 		dev = msi_desc_to_dev(irq_data_get_msi_desc(data));
> 		domain = iommu_msi_domain(dev);
> 		if (!domain)
> 			continue;
> 
> 		chip = irq_data_get_irq_chip(data);
> 		if (chip->msi_doorbell_info)
> 			break;
> 
> 		data = data->parent;
> 	}
> 
> 	if (!data)
> 		return 0;
> 
> 	dbinfo = chip->msi_doorbell_info(data);
> 	if (!dbinfo)
> 		return -EINVAL;
> 
> 	[... handle mapping/unmapping here ...]
> 
>> +
>> +/**
>>   * msi_domain_alloc_irqs - Allocate interrupts from a MSI interrupt domain
>>   * @domain:	The domain to allocate from
>>   * @dev:	Pointer to device struct of the device for which the interrupts
>> @@ -352,17 +404,26 @@ int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
>>  
>>  		virq = __irq_domain_alloc_irqs(domain, virq, desc->nvec_used,
>>  					       dev_to_node(dev), &arg, false);
>> -		if (virq < 0) {
>> -			ret = -ENOSPC;
>> -			if (ops->handle_error)
>> -				ret = ops->handle_error(domain, desc, ret);
>> -			if (ops->msi_finish)
>> -				ops->msi_finish(&arg, ret);
>> -			return ret;
>> -		}
>> +		if (virq < 0)
>> +			goto error;
>>  
>>  		for (i = 0; i < desc->nvec_used; i++)
>>  			irq_set_msi_desc_off(virq, i, desc);
>> +
>> +		for (i = 0; i < desc->nvec_used; i++) {
>> +			ret = msi_handle_doorbell_mappings(
>> +				irq_get_irq_data(virq + i), true);
> 
> Do not be afraid of longer lines. Or if you are, create an intermediate
> variable. But this kind of construct makes my brain work harder, and I
> hate the feeling... ;-)
Yes I am afraid of checkpatch and I do my utmost to abide by its law ;-)
Well, let me know if the v9 is of any relief for your brain ;-)

Thanks for your time!

Eric
> 
>> +			if (ret)
>> +				break;
>> +		}
>> +		if (ret) {
>> +			for (; i >= 0; i--)
>> +				msi_handle_doorbell_mappings(
>> +					irq_get_irq_data(virq + i), false);
>> +			irq_domain_free_irqs(virq, desc->nvec_used);
>> +			desc->irq = 0;
>> +			goto error;
>> +		}
>>  	}
>>  
>>  	if (ops->msi_finish)
>> @@ -377,6 +438,13 @@ int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev,
>>  	}
>>  
>>  	return 0;
>> +error:
>> +	ret = -ENOSPC;
>> +	if (ops->handle_error)
>> +		ret = ops->handle_error(domain, desc, ret);
>> +	if (ops->msi_finish)
>> +		ops->msi_finish(&arg, ret);
>> +	return ret;
>>  }
>>  
>>  /**
>> @@ -396,6 +464,9 @@ void msi_domain_free_irqs(struct irq_domain *domain, struct device *dev)
>>  		 * entry. If that's the case, don't do anything.
>>  		 */
>>  		if (desc->irq) {
>> +			msi_handle_doorbell_mappings(
>> +				irq_get_irq_data(desc->irq),
>> +				false);
>>  			irq_domain_free_irqs(desc->irq, desc->nvec_used);
>>  			desc->irq = 0;
>>  		}
>>
> 
> Thanks,
> 
> 	M.
>