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