Re: [PATCH v6 17/18] vfio/type1: Check MSI remapping at irq domain level

[Auger Eric]

Hi Bharat

On 06/01/2017 09:50, Bharat Bhushan wrote:
> Hi Eric,
> 
>> -----Original Message-----
>> From: Eric Auger [mailto:eric.auger@xxxxxxxxxx]
>> Sent: Friday, January 06, 2017 12:35 AM
>> To: eric.auger@xxxxxxxxxx; eric.auger.pro@xxxxxxxxx;
>> christoffer.dall@xxxxxxxxxx; marc.zyngier@xxxxxxx;
>> robin.murphy@xxxxxxx; alex.williamson@xxxxxxxxxx;
>> will.deacon@xxxxxxx; joro@xxxxxxxxxx; tglx@xxxxxxxxxxxxx;
>> jason@xxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: kvm@xxxxxxxxxxxxxxx; drjones@xxxxxxxxxx; linux-
>> kernel@xxxxxxxxxxxxxxx; pranav.sawargaonkar@xxxxxxxxx;
>> iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx; punit.agrawal@xxxxxxx; Diana Madalina
>> Craciun <diana.craciun@xxxxxxx>; gpkulkarni@xxxxxxxxx;
>> shankerd@xxxxxxxxxxxxxx; Bharat Bhushan <bharat.bhushan@xxxxxxx>;
>> geethasowjanya.akula@xxxxxxxxx
>> Subject: [PATCH v6 17/18] vfio/type1: Check MSI remapping at irq domain
>> level
>>
>> In case the IOMMU translates MSI transactions (typical case on ARM), we
>> check MSI remapping capability at IRQ domain level. Otherwise it is checked
>> at IOMMU level.
>>
>> At this stage the arm-smmu-(v3) still advertise the
>> IOMMU_CAP_INTR_REMAP capability at IOMMU level. This will be removed
>> in subsequent patches.
>>
>> Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
>>
>> ---
>>
>> v6: rewrite test
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c
>> b/drivers/vfio/vfio_iommu_type1.c index b473ef80..fa0b5c4 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -40,6 +40,7 @@
>>  #include <linux/mdev.h>
>>  #include <linux/notifier.h>
>>  #include <linux/dma-iommu.h>
>> +#include <linux/irqdomain.h>
>>
>>  #define DRIVER_VERSION  "0.2"
>>  #define DRIVER_AUTHOR   "Alex Williamson
>> <alex.williamson@xxxxxxxxxx>"
>> @@ -1208,7 +1209,7 @@ static int vfio_iommu_type1_attach_group(void
>> *iommu_data,
>>  	struct vfio_domain *domain, *d;
>>  	struct bus_type *bus = NULL, *mdev_bus;
>>  	int ret;
>> -	bool resv_msi;
>> +	bool resv_msi, msi_remap;
>>  	phys_addr_t resv_msi_base;
>>
>>  	mutex_lock(&iommu->lock);
>> @@ -1284,8 +1285,10 @@ static int vfio_iommu_type1_attach_group(void
>> *iommu_data,
>>  	INIT_LIST_HEAD(&domain->group_list);
>>  	list_add(&group->next, &domain->group_list);
>>
>> -	if (!allow_unsafe_interrupts &&
>> -	    !iommu_capable(bus, IOMMU_CAP_INTR_REMAP)) {
>> +	msi_remap = resv_msi ? irq_domain_check_msi_remap() :
> 
> There can be multiple interrupt-controller, at-least theoretically it is possible and not sure practically it exists and supported, where not all may support IRQ_REMAP. If that is the case be then should we check for IRQ-REMAP for that device-bus irq-domain?
> 
I mentioned in the cover letter that the approach was defensive and
rough today. As soon as we detect an MSI controller in the platform that
has no support for MSI remapping we flag the assignment as unsafe. I
think this approach was agreed on the ML. Such rough assessment was used
in the past on x86.

I am reluctant to add more complexity at that stage. This can be
improved latter I think when such platforms show up.

Best Regards

Eric
> Thanks
> -Bharat
> 
>> +				iommu_capable(bus,
>> IOMMU_CAP_INTR_REMAP);
>> +
>> +	if (!allow_unsafe_interrupts && !msi_remap) {
>>  		pr_warn("%s: No interrupt remapping support.  Use the
>> module param \"allow_unsafe_interrupts\" to enable VFIO IOMMU support
>> on this platform\n",
>>  		       __func__);
>>  		ret = -EPERM;
>> --
>> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>