linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] irqdomain: add empty irq_domain_check_msi_remap
@ 2017-03-02 10:01 Mian Yousaf Kaukab
  2017-03-02 10:01 ` [PATCH 2/2] vfio: type1: conditionally check MSI remapping at irq domain level Mian Yousaf Kaukab
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Mian Yousaf Kaukab @ 2017-03-02 10:01 UTC (permalink / raw)
  To: linux-kernel, kvm, marc.zyngier, alex.williamson
  Cc: eric.auger, will.deacon, Mian Yousaf Kaukab

Fix following build error for s390:
drivers/vfio/vfio_iommu_type1.c: In function 'vfio_iommu_type1_attach_group':
drivers/vfio/vfio_iommu_type1.c:1290:25: error: implicit declaration of function 'irq_domain_check_msi_remap'

Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@suse.com>
---
 include/linux/irqdomain.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 188eced6813e..137817b08cdc 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -524,6 +524,10 @@ static inline struct irq_domain *irq_find_matching_fwnode(
 {
 	return NULL;
 }
+static inline bool irq_domain_check_msi_remap(void)
+{
+	return true;
+}
 #endif /* !CONFIG_IRQ_DOMAIN */
 
 #endif /* _LINUX_IRQDOMAIN_H */
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/2] vfio: type1: conditionally check MSI remapping at irq domain level
  2017-03-02 10:01 [PATCH 1/2] irqdomain: add empty irq_domain_check_msi_remap Mian Yousaf Kaukab
@ 2017-03-02 10:01 ` Mian Yousaf Kaukab
  2017-03-02 10:19   ` Marc Zyngier
  2017-03-02 10:24   ` Auger Eric
  2017-03-02 10:16 ` [PATCH 1/2] irqdomain: add empty irq_domain_check_msi_remap Marc Zyngier
  2017-03-02 10:24 ` Auger Eric
  2 siblings, 2 replies; 13+ messages in thread
From: Mian Yousaf Kaukab @ 2017-03-02 10:01 UTC (permalink / raw)
  To: linux-kernel, kvm, marc.zyngier, alex.williamson
  Cc: eric.auger, will.deacon, Mian Yousaf Kaukab

Check only if irq domains are available.

Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@suse.com>
---
 drivers/vfio/vfio_iommu_type1.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index bd6f293c4ebd..e3ed50e40ead 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1287,8 +1287,9 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	INIT_LIST_HEAD(&domain->group_list);
 	list_add(&group->next, &domain->group_list);
 
-	msi_remap = resv_msi ? irq_domain_check_msi_remap() :
-				iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
+	msi_remap = resv_msi && IS_ENABLED(CONFIG_IRQ_DOMAIN) ?
+			irq_domain_check_msi_remap() :
+			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",
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] irqdomain: add empty irq_domain_check_msi_remap
  2017-03-02 10:01 [PATCH 1/2] irqdomain: add empty irq_domain_check_msi_remap Mian Yousaf Kaukab
  2017-03-02 10:01 ` [PATCH 2/2] vfio: type1: conditionally check MSI remapping at irq domain level Mian Yousaf Kaukab
@ 2017-03-02 10:16 ` Marc Zyngier
  2017-03-02 10:29   ` Auger Eric
  2017-03-02 10:24 ` Auger Eric
  2 siblings, 1 reply; 13+ messages in thread
From: Marc Zyngier @ 2017-03-02 10:16 UTC (permalink / raw)
  To: Mian Yousaf Kaukab, linux-kernel, kvm, alex.williamson
  Cc: eric.auger, will.deacon

On 02/03/17 10:01, Mian Yousaf Kaukab wrote:
> Fix following build error for s390:
> drivers/vfio/vfio_iommu_type1.c: In function 'vfio_iommu_type1_attach_group':
> drivers/vfio/vfio_iommu_type1.c:1290:25: error: implicit declaration of function 'irq_domain_check_msi_remap'
> 
> Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@suse.com>
> ---
>  include/linux/irqdomain.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 188eced6813e..137817b08cdc 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -524,6 +524,10 @@ static inline struct irq_domain *irq_find_matching_fwnode(
>  {
>  	return NULL;
>  }
> +static inline bool irq_domain_check_msi_remap(void)
> +{
> +	return true;

I'm not sure about that one. If we don't support reserved regions for
MSI, why should we return "true" here? My gut feeling is that it should
be false (because we lack the infrastructure to deal with it).

It is a bit of a moot point since the only calling site will *not* call
this in that case, but I believe that we should be consistent.

Eric, what do you think?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] vfio: type1: conditionally check MSI remapping at irq domain level
  2017-03-02 10:01 ` [PATCH 2/2] vfio: type1: conditionally check MSI remapping at irq domain level Mian Yousaf Kaukab
@ 2017-03-02 10:19   ` Marc Zyngier
  2017-03-02 10:24   ` Auger Eric
  1 sibling, 0 replies; 13+ messages in thread
From: Marc Zyngier @ 2017-03-02 10:19 UTC (permalink / raw)
  To: Mian Yousaf Kaukab, linux-kernel, kvm, alex.williamson
  Cc: eric.auger, will.deacon

On 02/03/17 10:01, Mian Yousaf Kaukab wrote:
> Check only if irq domains are available.
> 
> Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@suse.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index bd6f293c4ebd..e3ed50e40ead 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1287,8 +1287,9 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	INIT_LIST_HEAD(&domain->group_list);
>  	list_add(&group->next, &domain->group_list);
>  
> -	msi_remap = resv_msi ? irq_domain_check_msi_remap() :
> -				iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
> +	msi_remap = resv_msi && IS_ENABLED(CONFIG_IRQ_DOMAIN) ?

Now, that seems completely overkill in the light of your previous patch.
Why do we need this at all?

> +			irq_domain_check_msi_remap() :
> +			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",
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] irqdomain: add empty irq_domain_check_msi_remap
  2017-03-02 10:01 [PATCH 1/2] irqdomain: add empty irq_domain_check_msi_remap Mian Yousaf Kaukab
  2017-03-02 10:01 ` [PATCH 2/2] vfio: type1: conditionally check MSI remapping at irq domain level Mian Yousaf Kaukab
  2017-03-02 10:16 ` [PATCH 1/2] irqdomain: add empty irq_domain_check_msi_remap Marc Zyngier
@ 2017-03-02 10:24 ` Auger Eric
  2017-03-02 12:23   ` Mian Yousaf Kaukab
  2 siblings, 1 reply; 13+ messages in thread
From: Auger Eric @ 2017-03-02 10:24 UTC (permalink / raw)
  To: Mian Yousaf Kaukab, linux-kernel, kvm, marc.zyngier, alex.williamson
  Cc: will.deacon

Hi Mian Yousaf,

On 02/03/2017 11:01, Mian Yousaf Kaukab wrote:
> Fix following build error for s390:
> drivers/vfio/vfio_iommu_type1.c: In function 'vfio_iommu_type1_attach_group':
> drivers/vfio/vfio_iommu_type1.c:1290:25: error: implicit declaration of function 'irq_domain_check_msi_remap'
> 
> Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@suse.com>
> ---
>  include/linux/irqdomain.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
> index 188eced6813e..137817b08cdc 100644
> --- a/include/linux/irqdomain.h
> +++ b/include/linux/irqdomain.h
> @@ -524,6 +524,10 @@ static inline struct irq_domain *irq_find_matching_fwnode(
>  {
>  	return NULL;
>  }
> +static inline bool irq_domain_check_msi_remap(void)
> +{
> +	return true;
By default you should rather return false, reporting there is no MSI
remapping capability on irq domain side. Besides thank you for the fix.

Best Regards

Eric
> +}
>  #endif /* !CONFIG_IRQ_DOMAIN */
>  
>  #endif /* _LINUX_IRQDOMAIN_H */
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] vfio: type1: conditionally check MSI remapping at irq domain level
  2017-03-02 10:01 ` [PATCH 2/2] vfio: type1: conditionally check MSI remapping at irq domain level Mian Yousaf Kaukab
  2017-03-02 10:19   ` Marc Zyngier
@ 2017-03-02 10:24   ` Auger Eric
  2017-03-02 12:38     ` Mian Yousaf Kaukab
  1 sibling, 1 reply; 13+ messages in thread
From: Auger Eric @ 2017-03-02 10:24 UTC (permalink / raw)
  To: Mian Yousaf Kaukab, linux-kernel, kvm, marc.zyngier, alex.williamson
  Cc: will.deacon

Hi,

On 02/03/2017 11:01, Mian Yousaf Kaukab wrote:
> Check only if irq domains are available.
> 
> Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@suse.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index bd6f293c4ebd..e3ed50e40ead 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1287,8 +1287,9 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	INIT_LIST_HEAD(&domain->group_list);
>  	list_add(&group->next, &domain->group_list);
>  
> -	msi_remap = resv_msi ? irq_domain_check_msi_remap() :
> -				iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
> +	msi_remap = resv_msi && IS_ENABLED(CONFIG_IRQ_DOMAIN) ?
> +			irq_domain_check_msi_remap() :
> +			iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
Is that patch actually needed after [PATCH 1/2] irqdomain: add empty
irq_domain_check_msi_remap. irq_domain_check_msi_remap() should be
defined and if you follow my suggestion, would return false. Anyway in
your case resv_msi should be false.

Thanks

Eric
>  
>  	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",
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] irqdomain: add empty irq_domain_check_msi_remap
  2017-03-02 10:16 ` [PATCH 1/2] irqdomain: add empty irq_domain_check_msi_remap Marc Zyngier
@ 2017-03-02 10:29   ` Auger Eric
  0 siblings, 0 replies; 13+ messages in thread
From: Auger Eric @ 2017-03-02 10:29 UTC (permalink / raw)
  To: Marc Zyngier, Mian Yousaf Kaukab, linux-kernel, kvm, alex.williamson
  Cc: will.deacon

Hi Marc,

On 02/03/2017 11:16, Marc Zyngier wrote:
> On 02/03/17 10:01, Mian Yousaf Kaukab wrote:
>> Fix following build error for s390:
>> drivers/vfio/vfio_iommu_type1.c: In function 'vfio_iommu_type1_attach_group':
>> drivers/vfio/vfio_iommu_type1.c:1290:25: error: implicit declaration of function 'irq_domain_check_msi_remap'
>>
>> Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@suse.com>
>> ---
>>  include/linux/irqdomain.h | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>> index 188eced6813e..137817b08cdc 100644
>> --- a/include/linux/irqdomain.h
>> +++ b/include/linux/irqdomain.h
>> @@ -524,6 +524,10 @@ static inline struct irq_domain *irq_find_matching_fwnode(
>>  {
>>  	return NULL;
>>  }
>> +static inline bool irq_domain_check_msi_remap(void)
>> +{
>> +	return true;
> 
> I'm not sure about that one. If we don't support reserved regions for
> MSI, why should we return "true" here? My gut feeling is that it should
> be false (because we lack the infrastructure to deal with it).
> 
> It is a bit of a moot point since the only calling site will *not* call
> this in that case, but I believe that we should be consistent.
> 
> Eric, what do you think?

I agree with you. I Would return false here as just commented and I
don't think subsequent patch is needed.

Thanks

Eric
> 
> Thanks,
> 
> 	M.
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] irqdomain: add empty irq_domain_check_msi_remap
  2017-03-02 10:24 ` Auger Eric
@ 2017-03-02 12:23   ` Mian Yousaf Kaukab
  2017-03-02 13:12     ` Auger Eric
  0 siblings, 1 reply; 13+ messages in thread
From: Mian Yousaf Kaukab @ 2017-03-02 12:23 UTC (permalink / raw)
  To: Auger Eric, linux-kernel, kvm, marc.zyngier, alex.williamson; +Cc: will.deacon

On 03/02/2017 11:24 AM, Auger Eric wrote:
> Hi Mian Yousaf,
> 
> On 02/03/2017 11:01, Mian Yousaf Kaukab wrote:
>> Fix following build error for s390:
>> drivers/vfio/vfio_iommu_type1.c: In function 'vfio_iommu_type1_attach_group':
>> drivers/vfio/vfio_iommu_type1.c:1290:25: error: implicit declaration of function 'irq_domain_check_msi_remap'
>>
>> Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@suse.com>
>> ---
>>   include/linux/irqdomain.h | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>> index 188eced6813e..137817b08cdc 100644
>> --- a/include/linux/irqdomain.h
>> +++ b/include/linux/irqdomain.h
>> @@ -524,6 +524,10 @@ static inline struct irq_domain *irq_find_matching_fwnode(
>>   {
>>   	return NULL;
>>   }
>> +static inline bool irq_domain_check_msi_remap(void)
>> +{
>> +	return true;
> By default you should rather return false, reporting there is no MSI
> remapping capability on irq domain side. Besides thank you for the fix.
I choose to return true based on the function header comments of 
irq_domain_check_msi_remap. It says

"Return: false if any MSI irq domain does not support IRQ remapping, 
true otherwise (including if there is no MSI irq domain)"

So function should return true in case of no MSI irq domain. Have I miss 
understood this?

BR,
Yousaf

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] vfio: type1: conditionally check MSI remapping at irq domain level
  2017-03-02 10:24   ` Auger Eric
@ 2017-03-02 12:38     ` Mian Yousaf Kaukab
  2017-03-02 13:46       ` Auger Eric
  0 siblings, 1 reply; 13+ messages in thread
From: Mian Yousaf Kaukab @ 2017-03-02 12:38 UTC (permalink / raw)
  To: Auger Eric, linux-kernel, kvm, marc.zyngier, alex.williamson; +Cc: will.deacon

On 03/02/2017 11:24 AM, Auger Eric wrote:
> Hi,
> 
> On 02/03/2017 11:01, Mian Yousaf Kaukab wrote:
>> Check only if irq domains are available.
>>
>> Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@suse.com>
>> ---
>>   drivers/vfio/vfio_iommu_type1.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index bd6f293c4ebd..e3ed50e40ead 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -1287,8 +1287,9 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>>   	INIT_LIST_HEAD(&domain->group_list);
>>   	list_add(&group->next, &domain->group_list);
>>   
>> -	msi_remap = resv_msi ? irq_domain_check_msi_remap() :
>> -				iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
>> +	msi_remap = resv_msi && IS_ENABLED(CONFIG_IRQ_DOMAIN) ?
>> +			irq_domain_check_msi_remap() :
>> +			iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
> Is that patch actually needed after [PATCH 1/2] irqdomain: add empty
> irq_domain_check_msi_remap. irq_domain_check_msi_remap() should be
> defined and if you follow my suggestion, would return false. Anyway in
> your case resv_msi should be false.
I agree its an overkill if resv_msi is guaranteed to be false. What I am 
unsure about is that, if iommu have IOMMU_RESV_MSI regions that would 
mean that irq domains are selected in the build. If this is not 
guaranteed, then we need to add this check.

> 
> Thanks
> 
> Eric

BR,
Yousaf

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] irqdomain: add empty irq_domain_check_msi_remap
  2017-03-02 12:23   ` Mian Yousaf Kaukab
@ 2017-03-02 13:12     ` Auger Eric
  2017-03-02 13:31       ` Mian Yousaf Kaukab
  0 siblings, 1 reply; 13+ messages in thread
From: Auger Eric @ 2017-03-02 13:12 UTC (permalink / raw)
  To: Mian Yousaf Kaukab, linux-kernel, kvm, marc.zyngier, alex.williamson
  Cc: will.deacon

Hi Yousaf,

On 02/03/2017 13:23, Mian Yousaf Kaukab wrote:
> On 03/02/2017 11:24 AM, Auger Eric wrote:
>> Hi Mian Yousaf,
>>
>> On 02/03/2017 11:01, Mian Yousaf Kaukab wrote:
>>> Fix following build error for s390:
>>> drivers/vfio/vfio_iommu_type1.c: In function
>>> 'vfio_iommu_type1_attach_group':
>>> drivers/vfio/vfio_iommu_type1.c:1290:25: error: implicit declaration
>>> of function 'irq_domain_check_msi_remap'
>>>
>>> Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@suse.com>
>>> ---
>>>   include/linux/irqdomain.h | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>>> index 188eced6813e..137817b08cdc 100644
>>> --- a/include/linux/irqdomain.h
>>> +++ b/include/linux/irqdomain.h
>>> @@ -524,6 +524,10 @@ static inline struct irq_domain
>>> *irq_find_matching_fwnode(
>>>   {
>>>       return NULL;
>>>   }
>>> +static inline bool irq_domain_check_msi_remap(void)
>>> +{
>>> +    return true;
>> By default you should rather return false, reporting there is no MSI
>> remapping capability on irq domain side. Besides thank you for the fix.
> I choose to return true based on the function header comments of
> irq_domain_check_msi_remap. It says
> 
> "Return: false if any MSI irq domain does not support IRQ remapping,
> true otherwise (including if there is no MSI irq domain)"
> 
> So function should return true in case of no MSI irq domain. Have I miss
> understood this?
This behavior is indeed mandated on ARM - where MSI are translated by
the smmu - to allow safe device assignment if there is no MSI domain,
ie. in this situation there is no risk an assigned device writes into an
MSI doorbell.

As the function is not implemented at all in your case, personally I
would rather be defensive though and return false. You were not able to
check the capability.

Thanks

Eric
> 
> BR,
> Yousaf

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] irqdomain: add empty irq_domain_check_msi_remap
  2017-03-02 13:12     ` Auger Eric
@ 2017-03-02 13:31       ` Mian Yousaf Kaukab
  0 siblings, 0 replies; 13+ messages in thread
From: Mian Yousaf Kaukab @ 2017-03-02 13:31 UTC (permalink / raw)
  To: Auger Eric, linux-kernel, kvm, marc.zyngier, alex.williamson; +Cc: will.deacon

On 03/02/2017 02:12 PM, Auger Eric wrote:
> Hi Yousaf,
> 
> On 02/03/2017 13:23, Mian Yousaf Kaukab wrote:
>> On 03/02/2017 11:24 AM, Auger Eric wrote:
>>> Hi Mian Yousaf,
>>>
>>> On 02/03/2017 11:01, Mian Yousaf Kaukab wrote:
>>>> Fix following build error for s390:
>>>> drivers/vfio/vfio_iommu_type1.c: In function
>>>> 'vfio_iommu_type1_attach_group':
>>>> drivers/vfio/vfio_iommu_type1.c:1290:25: error: implicit declaration
>>>> of function 'irq_domain_check_msi_remap'
>>>>
>>>> Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@suse.com>
>>>> ---
>>>>    include/linux/irqdomain.h | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
>>>> index 188eced6813e..137817b08cdc 100644
>>>> --- a/include/linux/irqdomain.h
>>>> +++ b/include/linux/irqdomain.h
>>>> @@ -524,6 +524,10 @@ static inline struct irq_domain
>>>> *irq_find_matching_fwnode(
>>>>    {
>>>>        return NULL;
>>>>    }
>>>> +static inline bool irq_domain_check_msi_remap(void)
>>>> +{
>>>> +    return true;
>>> By default you should rather return false, reporting there is no MSI
>>> remapping capability on irq domain side. Besides thank you for the fix.
>> I choose to return true based on the function header comments of
>> irq_domain_check_msi_remap. It says
>>
>> "Return: false if any MSI irq domain does not support IRQ remapping,
>> true otherwise (including if there is no MSI irq domain)"
>>
>> So function should return true in case of no MSI irq domain. Have I miss
>> understood this?
> This behavior is indeed mandated on ARM - where MSI are translated by
> the smmu - to allow safe device assignment if there is no MSI domain,
> ie. in this situation there is no risk an assigned device writes into an
> MSI doorbell.
> 
> As the function is not implemented at all in your case, personally I
> would rather be defensive though and return false. You were not able to
> check the capability.
OK Agree. I will send an update as soon as a decision is made on 2/2.

BR,
Yousaf

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] vfio: type1: conditionally check MSI remapping at irq domain level
  2017-03-02 12:38     ` Mian Yousaf Kaukab
@ 2017-03-02 13:46       ` Auger Eric
  2017-03-02 15:01         ` Mian Yousaf Kaukab
  0 siblings, 1 reply; 13+ messages in thread
From: Auger Eric @ 2017-03-02 13:46 UTC (permalink / raw)
  To: Mian Yousaf Kaukab, linux-kernel, kvm, marc.zyngier, alex.williamson
  Cc: will.deacon

Hi,

On 02/03/2017 13:38, Mian Yousaf Kaukab wrote:
> On 03/02/2017 11:24 AM, Auger Eric wrote:
>> Hi,
>>
>> On 02/03/2017 11:01, Mian Yousaf Kaukab wrote:
>>> Check only if irq domains are available.
>>>
>>> Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@suse.com>
>>> ---
>>>   drivers/vfio/vfio_iommu_type1.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/vfio/vfio_iommu_type1.c
>>> b/drivers/vfio/vfio_iommu_type1.c
>>> index bd6f293c4ebd..e3ed50e40ead 100644
>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>> @@ -1287,8 +1287,9 @@ static int vfio_iommu_type1_attach_group(void
>>> *iommu_data,
>>>       INIT_LIST_HEAD(&domain->group_list);
>>>       list_add(&group->next, &domain->group_list);
>>>   -    msi_remap = resv_msi ? irq_domain_check_msi_remap() :
>>> -                iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
>>> +    msi_remap = resv_msi && IS_ENABLED(CONFIG_IRQ_DOMAIN) ?
>>> +            irq_domain_check_msi_remap() :
>>> +            iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
>> Is that patch actually needed after [PATCH 1/2] irqdomain: add empty
>> irq_domain_check_msi_remap. irq_domain_check_msi_remap() should be
>> defined and if you follow my suggestion, would return false. Anyway in
>> your case resv_msi should be false.
> I agree its an overkill if resv_msi is guaranteed to be false. What I am
> unsure about is that, if iommu have IOMMU_RESV_MSI regions that would
> mean that irq domains are selected in the build. If this is not
> guaranteed, then we need to add this check.

Currently only ARM SMMUs advertise IOMMU_RESV_MSI regions. If attempting
to do passthrough on an ARM platform not implementing IRQ_DOMAIN the
unsafe IRQ assignment mode would need to be chosen (if
irq_domain_check_msi_remap() returns false as discussed before). Anyway
checking the  interrupt remapping capability on IOMMU side would report
false as well since the capability is not exposed by ARM SMMU anymore.

Thanks

Eric
> 
>>
>> Thanks
>>
>> Eric
> 
> BR,
> Yousaf

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] vfio: type1: conditionally check MSI remapping at irq domain level
  2017-03-02 13:46       ` Auger Eric
@ 2017-03-02 15:01         ` Mian Yousaf Kaukab
  0 siblings, 0 replies; 13+ messages in thread
From: Mian Yousaf Kaukab @ 2017-03-02 15:01 UTC (permalink / raw)
  To: Auger Eric, linux-kernel, kvm, marc.zyngier, alex.williamson; +Cc: will.deacon

On 03/02/2017 02:46 PM, Auger Eric wrote:
> Hi,
> 
> On 02/03/2017 13:38, Mian Yousaf Kaukab wrote:
>> On 03/02/2017 11:24 AM, Auger Eric wrote:
>>> Hi,
>>>
>>> On 02/03/2017 11:01, Mian Yousaf Kaukab wrote:
>>>> Check only if irq domains are available.
>>>>
>>>> Signed-off-by: Mian Yousaf Kaukab <yousaf.kaukab@suse.com>
>>>> ---
>>>>    drivers/vfio/vfio_iommu_type1.c | 5 +++--
>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/vfio/vfio_iommu_type1.c
>>>> b/drivers/vfio/vfio_iommu_type1.c
>>>> index bd6f293c4ebd..e3ed50e40ead 100644
>>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>>> @@ -1287,8 +1287,9 @@ static int vfio_iommu_type1_attach_group(void
>>>> *iommu_data,
>>>>        INIT_LIST_HEAD(&domain->group_list);
>>>>        list_add(&group->next, &domain->group_list);
>>>>    -    msi_remap = resv_msi ? irq_domain_check_msi_remap() :
>>>> -                iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
>>>> +    msi_remap = resv_msi && IS_ENABLED(CONFIG_IRQ_DOMAIN) ?
>>>> +            irq_domain_check_msi_remap() :
>>>> +            iommu_capable(bus, IOMMU_CAP_INTR_REMAP);
>>> Is that patch actually needed after [PATCH 1/2] irqdomain: add empty
>>> irq_domain_check_msi_remap. irq_domain_check_msi_remap() should be
>>> defined and if you follow my suggestion, would return false. Anyway in
>>> your case resv_msi should be false.
>> I agree its an overkill if resv_msi is guaranteed to be false. What I am
>> unsure about is that, if iommu have IOMMU_RESV_MSI regions that would
>> mean that irq domains are selected in the build. If this is not
>> guaranteed, then we need to add this check.
> 
> Currently only ARM SMMUs advertise IOMMU_RESV_MSI regions. If attempting
> to do passthrough on an ARM platform not implementing IRQ_DOMAIN the
> unsafe IRQ assignment mode would need to be chosen (if
> irq_domain_check_msi_remap() returns false as discussed before). Anyway
> checking the  interrupt remapping capability on IOMMU side would report
> false as well since the capability is not exposed by ARM SMMU anymore.OK I will drop this patch. We get a warning anyway in case the 
capability is checked from the wrong place.

BR,
Yousaf

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2017-03-02 15:49 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-02 10:01 [PATCH 1/2] irqdomain: add empty irq_domain_check_msi_remap Mian Yousaf Kaukab
2017-03-02 10:01 ` [PATCH 2/2] vfio: type1: conditionally check MSI remapping at irq domain level Mian Yousaf Kaukab
2017-03-02 10:19   ` Marc Zyngier
2017-03-02 10:24   ` Auger Eric
2017-03-02 12:38     ` Mian Yousaf Kaukab
2017-03-02 13:46       ` Auger Eric
2017-03-02 15:01         ` Mian Yousaf Kaukab
2017-03-02 10:16 ` [PATCH 1/2] irqdomain: add empty irq_domain_check_msi_remap Marc Zyngier
2017-03-02 10:29   ` Auger Eric
2017-03-02 10:24 ` Auger Eric
2017-03-02 12:23   ` Mian Yousaf Kaukab
2017-03-02 13:12     ` Auger Eric
2017-03-02 13:31       ` Mian Yousaf Kaukab

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).