diff mbox

[v3,06/10] VFIO: platform: add irq bypass producer management

Message ID 1439212864-12954-7-git-send-email-eric.auger@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Auger Aug. 10, 2015, 1:21 p.m. UTC
This patch populates the IRQ bypass callacks:
- stop/start producer simply consist in disabling/enabling the host irq
- add/del consumer: basically set the automasked flag to false/true

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---
v2 -> v3:
- vfio_platform_irq_bypass_add_consumer now returns an error in case
  the IRQ is recognized as active
- active boolean not set anymore
- do not VFIO mask the IRQ anymore on unforward

v1 -> v2:
- device handle caching in vfio_platform_device is introduced in a
  separate patch
- use container_of
---
 drivers/vfio/platform/vfio_platform_irq.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

Comments

Alex Williamson Aug. 12, 2015, 6:56 p.m. UTC | #1
On Mon, 2015-08-10 at 15:21 +0200, Eric Auger wrote:
> This patch populates the IRQ bypass callacks:
> - stop/start producer simply consist in disabling/enabling the host irq
> - add/del consumer: basically set the automasked flag to false/true
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> v2 -> v3:
> - vfio_platform_irq_bypass_add_consumer now returns an error in case
>   the IRQ is recognized as active
> - active boolean not set anymore
> - do not VFIO mask the IRQ anymore on unforward
> 
> v1 -> v2:
> - device handle caching in vfio_platform_device is introduced in a
>   separate patch
> - use container_of
> ---
>  drivers/vfio/platform/vfio_platform_irq.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
> index efaee58..400a188 100644
> --- a/drivers/vfio/platform/vfio_platform_irq.c
> +++ b/drivers/vfio/platform/vfio_platform_irq.c
> @@ -224,23 +224,44 @@ static int vfio_platform_is_active(struct vfio_platform_irq *irq)
>  
>  static void vfio_platform_irq_bypass_stop(struct irq_bypass_producer *prod)
>  {
> +	disable_irq(prod->irq);
>  }
>  
>  static void vfio_platform_irq_bypass_start(struct irq_bypass_producer *prod)
>  {
> +	enable_irq(prod->irq);
>  }
>  
>  static int vfio_platform_irq_bypass_add_consumer(
>  			struct irq_bypass_producer *prod,
>  			struct irq_bypass_consumer *cons)
>  {
> -	return 0;
> +	struct vfio_platform_irq *irq =
> +		container_of(prod, struct vfio_platform_irq, producer);
> +
> +	/*
> +	 * if the IRQ is active at irqchip level or VFIO (auto)masked
> +	 * this means the host IRQ is already under injection in the
> +	 * guest and this not safe to change the forwarding state at
> +	 * that stage.
> +	 * It is not possible to differentiate user-space masking
> +	 * from auto-masking, leading to possible false detection of
> +	 * active state.
> +	 */
> +	if (vfio_platform_is_active(irq))
> +		return -EAGAIN;

Here's an example of why we don't want WARN_ON if a registration fails,
this is effectively random.  When and how is a re-try going to happen?

> +
> +	return vfio_platform_set_automasked(irq, false);

set_forwarded just seems so much more logical here.

>  }
>  
>  static void vfio_platform_irq_bypass_del_consumer(
>  			struct irq_bypass_producer *prod,
>  			struct irq_bypass_consumer *cons)
>  {
> +	struct vfio_platform_irq *irq =
> +		container_of(prod, struct vfio_platform_irq, producer);
> +
> +	vfio_platform_set_automasked(irq, true);
>  }
>  
>  static int vfio_set_trigger(struct vfio_platform_device *vdev, int index,



--
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
Eric Auger Aug. 17, 2015, 3:51 p.m. UTC | #2
Hi Alex,
On 08/12/2015 08:56 PM, Alex Williamson wrote:
> On Mon, 2015-08-10 at 15:21 +0200, Eric Auger wrote:
>> This patch populates the IRQ bypass callacks:
>> - stop/start producer simply consist in disabling/enabling the host irq
>> - add/del consumer: basically set the automasked flag to false/true
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>> v2 -> v3:
>> - vfio_platform_irq_bypass_add_consumer now returns an error in case
>>   the IRQ is recognized as active
>> - active boolean not set anymore
>> - do not VFIO mask the IRQ anymore on unforward
>>
>> v1 -> v2:
>> - device handle caching in vfio_platform_device is introduced in a
>>   separate patch
>> - use container_of
>> ---
>>  drivers/vfio/platform/vfio_platform_irq.c | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
>> index efaee58..400a188 100644
>> --- a/drivers/vfio/platform/vfio_platform_irq.c
>> +++ b/drivers/vfio/platform/vfio_platform_irq.c
>> @@ -224,23 +224,44 @@ static int vfio_platform_is_active(struct vfio_platform_irq *irq)
>>  
>>  static void vfio_platform_irq_bypass_stop(struct irq_bypass_producer *prod)
>>  {
>> +	disable_irq(prod->irq);
>>  }
>>  
>>  static void vfio_platform_irq_bypass_start(struct irq_bypass_producer *prod)
>>  {
>> +	enable_irq(prod->irq);
>>  }
>>  
>>  static int vfio_platform_irq_bypass_add_consumer(
>>  			struct irq_bypass_producer *prod,
>>  			struct irq_bypass_consumer *cons)
>>  {
>> -	return 0;
>> +	struct vfio_platform_irq *irq =
>> +		container_of(prod, struct vfio_platform_irq, producer);
>> +
>> +	/*
>> +	 * if the IRQ is active at irqchip level or VFIO (auto)masked
>> +	 * this means the host IRQ is already under injection in the
>> +	 * guest and this not safe to change the forwarding state at
>> +	 * that stage.
>> +	 * It is not possible to differentiate user-space masking
>> +	 * from auto-masking, leading to possible false detection of
>> +	 * active state.
>> +	 */
>> +	if (vfio_platform_is_active(irq))
>> +		return -EAGAIN;
> 
> Here's an example of why we don't want WARN_ON if a registration fails,
> this is effectively random.  When and how is a re-try going to happen?
To be honest I did not intend to implement any retry mechanism. I rather
intended to change the user-side which currently is not adapted to that
start-up. Typically with current QEMU code we have this 2 phase IRQ
signal startup, first set up eventfd signaling, then VFIO mask, then
turn irqfd on. With such sequence forwarding setup will fail because the
IRQ is seen as masked (false detection of activity).

Do you mandate such a retry mechanism? If forwarding fails we resort on
standard irqfd ...

I think forwarding setup should be as much static as possible (I think
this opinion also is shared by Marc?).

Please let me know your opinion on this.

Best Regards

Eric

> 
>> +
>> +	return vfio_platform_set_automasked(irq, false);
> 
> set_forwarded just seems so much more logical here.
> 
>>  }
>>  
>>  static void vfio_platform_irq_bypass_del_consumer(
>>  			struct irq_bypass_producer *prod,
>>  			struct irq_bypass_consumer *cons)
>>  {
>> +	struct vfio_platform_irq *irq =
>> +		container_of(prod, struct vfio_platform_irq, producer);
>> +
>> +	vfio_platform_set_automasked(irq, true);
>>  }
>>  
>>  static int vfio_set_trigger(struct vfio_platform_device *vdev, int index,
> 
> 
> 

--
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
Christoffer Dall Sept. 2, 2015, 7:32 p.m. UTC | #3
On Mon, Aug 10, 2015 at 03:21:00PM +0200, Eric Auger wrote:
> This patch populates the IRQ bypass callacks:
> - stop/start producer simply consist in disabling/enabling the host irq
> - add/del consumer: basically set the automasked flag to false/true
> 
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
> 
> ---
> v2 -> v3:
> - vfio_platform_irq_bypass_add_consumer now returns an error in case
>   the IRQ is recognized as active
> - active boolean not set anymore
> - do not VFIO mask the IRQ anymore on unforward
> 
> v1 -> v2:
> - device handle caching in vfio_platform_device is introduced in a
>   separate patch
> - use container_of
> ---
>  drivers/vfio/platform/vfio_platform_irq.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
> index efaee58..400a188 100644
> --- a/drivers/vfio/platform/vfio_platform_irq.c
> +++ b/drivers/vfio/platform/vfio_platform_irq.c
> @@ -224,23 +224,44 @@ static int vfio_platform_is_active(struct vfio_platform_irq *irq)
>  
>  static void vfio_platform_irq_bypass_stop(struct irq_bypass_producer *prod)
>  {
> +	disable_irq(prod->irq);
>  }
>  
>  static void vfio_platform_irq_bypass_start(struct irq_bypass_producer *prod)
>  {
> +	enable_irq(prod->irq);
>  }
>  
>  static int vfio_platform_irq_bypass_add_consumer(
>  			struct irq_bypass_producer *prod,
>  			struct irq_bypass_consumer *cons)
>  {
> -	return 0;
> +	struct vfio_platform_irq *irq =
> +		container_of(prod, struct vfio_platform_irq, producer);
> +
> +	/*
> +	 * if the IRQ is active at irqchip level or VFIO (auto)masked
           If                                       masked by VFIO?
> +	 * this means the host IRQ is already under injection in the
> +	 * guest and this not safe to change the forwarding state at
> +	 * that stage.

is this really specifically bound to guests?

> +	 * It is not possible to differentiate user-space masking
> +	 * from auto-masking, leading to possible false detection of
> +	 * active state.

ok, what is the consequence of that?

> +	 */
> +	if (vfio_platform_is_active(irq))
> +		return -EAGAIN;
> +
> +	return vfio_platform_set_automasked(irq, false);
>  }
>  
>  static void vfio_platform_irq_bypass_del_consumer(
>  			struct irq_bypass_producer *prod,
>  			struct irq_bypass_consumer *cons)
>  {
> +	struct vfio_platform_irq *irq =
> +		container_of(prod, struct vfio_platform_irq, producer);
> +
> +	vfio_platform_set_automasked(irq, true);
>  }
>  
>  static int vfio_set_trigger(struct vfio_platform_device *vdev, int index,
> -- 
> 1.9.1
> 
--
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
diff mbox

Patch

diff --git a/drivers/vfio/platform/vfio_platform_irq.c b/drivers/vfio/platform/vfio_platform_irq.c
index efaee58..400a188 100644
--- a/drivers/vfio/platform/vfio_platform_irq.c
+++ b/drivers/vfio/platform/vfio_platform_irq.c
@@ -224,23 +224,44 @@  static int vfio_platform_is_active(struct vfio_platform_irq *irq)
 
 static void vfio_platform_irq_bypass_stop(struct irq_bypass_producer *prod)
 {
+	disable_irq(prod->irq);
 }
 
 static void vfio_platform_irq_bypass_start(struct irq_bypass_producer *prod)
 {
+	enable_irq(prod->irq);
 }
 
 static int vfio_platform_irq_bypass_add_consumer(
 			struct irq_bypass_producer *prod,
 			struct irq_bypass_consumer *cons)
 {
-	return 0;
+	struct vfio_platform_irq *irq =
+		container_of(prod, struct vfio_platform_irq, producer);
+
+	/*
+	 * if the IRQ is active at irqchip level or VFIO (auto)masked
+	 * this means the host IRQ is already under injection in the
+	 * guest and this not safe to change the forwarding state at
+	 * that stage.
+	 * It is not possible to differentiate user-space masking
+	 * from auto-masking, leading to possible false detection of
+	 * active state.
+	 */
+	if (vfio_platform_is_active(irq))
+		return -EAGAIN;
+
+	return vfio_platform_set_automasked(irq, false);
 }
 
 static void vfio_platform_irq_bypass_del_consumer(
 			struct irq_bypass_producer *prod,
 			struct irq_bypass_consumer *cons)
 {
+	struct vfio_platform_irq *irq =
+		container_of(prod, struct vfio_platform_irq, producer);
+
+	vfio_platform_set_automasked(irq, true);
 }
 
 static int vfio_set_trigger(struct vfio_platform_device *vdev, int index,