[PATCH for-5.0] xive/kvm: Trigger interrupts from userspace

[PATCH for-5.0] xive/kvm: Trigger interrupts from userspace

[Greg Kurz]

When using the XIVE KVM device, the trigger page is directly accessible
in QEMU. Unlike with XICS, no need to ask KVM to fire the interrupt. A
simple store on the trigger page does the job.

Just call xive_esb_trigger().

This may improve performance of emulated devices that go through
qemu_set_irq(), eg. virtio devices created with ioeventfd=off or
configured by the guest to use LSI interrupts, which aren't really
recommended setups.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/intc/spapr_xive_kvm.c |   16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
index 08012ac7cd76..69e73552f1ef 100644
--- a/hw/intc/spapr_xive_kvm.c
+++ b/hw/intc/spapr_xive_kvm.c
@@ -354,32 +354,20 @@ static void kvmppc_xive_source_get_state(XiveSource *xsrc)
 void kvmppc_xive_source_set_irq(void *opaque, int srcno, int val)
 {
     XiveSource *xsrc = opaque;
-    SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
-    struct kvm_irq_level args;
-    int rc;
-
-    /* The KVM XIVE device should be in use */
-    assert(xive->fd != -1);
 
-    args.irq = srcno;
     if (!xive_source_irq_is_lsi(xsrc, srcno)) {
         if (!val) {
             return;
         }
-        args.level = KVM_INTERRUPT_SET;
     } else {
         if (val) {
             xsrc->status[srcno] |= XIVE_STATUS_ASSERTED;
-            args.level = KVM_INTERRUPT_SET_LEVEL;
         } else {
             xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED;
-            args.level = KVM_INTERRUPT_UNSET;
         }
     }
-    rc = kvm_vm_ioctl(kvm_state, KVM_IRQ_LINE, &args);
-    if (rc < 0) {
-        error_report("XIVE: kvm_irq_line() failed : %s", strerror(errno));
-    }
+
+    xive_esb_trigger(xsrc, srcno);
 }
 
 /*

Re: [PATCH for-5.0] xive/kvm: Trigger interrupts from userspace

[Cédric Le Goater]

On 18/11/2019 16:12, Greg Kurz wrote:
> When using the XIVE KVM device, the trigger page is directly accessible
> in QEMU. Unlike with XICS, no need to ask KVM to fire the interrupt. A
> simple store on the trigger page does the job.
> 
> Just call xive_esb_trigger().

Yes but the KVM XIVE device does a few other checks. 

It checks that the interrupt was correctly initialized at the KVM device
level. We should be fine in QEMU which has similar checks.

It caches the LSI assertion level. We should be fine also because it is
useless in KVM when using the XIVE native exploitation mode.

It checks it is not a passthru interrupt. Any idea on how to check this 
condition under QEMU ? 
 
> This may improve performance of emulated devices that go through
> qemu_set_irq(), eg. virtio devices created with ioeventfd=off or
> configured by the guest to use LSI interrupts, which aren't really
> recommended setups.

LGTM.

Any figures to share ? 

C.

> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/intc/spapr_xive_kvm.c |   16 ++--------------
>  1 file changed, 2 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> index 08012ac7cd76..69e73552f1ef 100644
> --- a/hw/intc/spapr_xive_kvm.c
> +++ b/hw/intc/spapr_xive_kvm.c
> @@ -354,32 +354,20 @@ static void kvmppc_xive_source_get_state(XiveSource *xsrc)
>  void kvmppc_xive_source_set_irq(void *opaque, int srcno, int val)
>  {
>      XiveSource *xsrc = opaque;
> -    SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
> -    struct kvm_irq_level args;
> -    int rc;
> -
> -    /* The KVM XIVE device should be in use */
> -    assert(xive->fd != -1);
>  
> -    args.irq = srcno;
>      if (!xive_source_irq_is_lsi(xsrc, srcno)) {
>          if (!val) {
>              return;
>          }
> -        args.level = KVM_INTERRUPT_SET;
>      } else {
>          if (val) {
>              xsrc->status[srcno] |= XIVE_STATUS_ASSERTED;
> -            args.level = KVM_INTERRUPT_SET_LEVEL;
>          } else {
>              xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED;
> -            args.level = KVM_INTERRUPT_UNSET;
>          }
>      }
> -    rc = kvm_vm_ioctl(kvm_state, KVM_IRQ_LINE, &args);
> -    if (rc < 0) {
> -        error_report("XIVE: kvm_irq_line() failed : %s", strerror(errno));
> -    }
> +
> +    xive_esb_trigger(xsrc, srcno);
>  }

Re: [PATCH for-5.0] xive/kvm: Trigger interrupts from userspace

[Greg Kurz]

On Mon, 18 Nov 2019 16:37:16 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> On 18/11/2019 16:12, Greg Kurz wrote:
> > When using the XIVE KVM device, the trigger page is directly accessible
> > in QEMU. Unlike with XICS, no need to ask KVM to fire the interrupt. A
> > simple store on the trigger page does the job.
> > 
> > Just call xive_esb_trigger().
> 
> Yes but the KVM XIVE device does a few other checks. 
> 
> It checks that the interrupt was correctly initialized at the KVM device
> level. We should be fine in QEMU which has similar checks.
> 

Yeah, not sure we can do much more than to trust the QEMU irq code to
pass us a valid srcno.

> It caches the LSI assertion level. We should be fine also because it is
> useless in KVM when using the XIVE native exploitation mode.
> 

Yeah, I see it is set in kvmppc_xive_native_set_source() (why?) but never
used anywhere else in book3s_xive_native.c.

> It checks it is not a passthru interrupt. Any idea on how to check this 
> condition under QEMU ? 
> 

AFAICT passthru interrupts don't go through here either.
 
> > This may improve performance of emulated devices that go through
> > qemu_set_irq(), eg. virtio devices created with ioeventfd=off or
> > configured by the guest to use LSI interrupts, which aren't really
> > recommended setups.
> 
> LGTM.
> 
> Any figures to share ? 
> 

No. Since performance critical setups don't go through that path,
I didn't try too much.

> C.
> 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/intc/spapr_xive_kvm.c |   16 ++--------------
> >  1 file changed, 2 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> > index 08012ac7cd76..69e73552f1ef 100644
> > --- a/hw/intc/spapr_xive_kvm.c
> > +++ b/hw/intc/spapr_xive_kvm.c
> > @@ -354,32 +354,20 @@ static void kvmppc_xive_source_get_state(XiveSource *xsrc)
> >  void kvmppc_xive_source_set_irq(void *opaque, int srcno, int val)
> >  {
> >      XiveSource *xsrc = opaque;
> > -    SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
> > -    struct kvm_irq_level args;
> > -    int rc;
> > -
> > -    /* The KVM XIVE device should be in use */
> > -    assert(xive->fd != -1);
> >  
> > -    args.irq = srcno;
> >      if (!xive_source_irq_is_lsi(xsrc, srcno)) {
> >          if (!val) {
> >              return;
> >          }
> > -        args.level = KVM_INTERRUPT_SET;
> >      } else {
> >          if (val) {
> >              xsrc->status[srcno] |= XIVE_STATUS_ASSERTED;
> > -            args.level = KVM_INTERRUPT_SET_LEVEL;
> >          } else {
> >              xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED;
> > -            args.level = KVM_INTERRUPT_UNSET;
> >          }
> >      }
> > -    rc = kvm_vm_ioctl(kvm_state, KVM_IRQ_LINE, &args);
> > -    if (rc < 0) {
> > -        error_report("XIVE: kvm_irq_line() failed : %s", strerror(errno));
> > -    }
> > +
> > +    xive_esb_trigger(xsrc, srcno);
> >  }
> 
> 

Re: [PATCH for-5.0] xive/kvm: Trigger interrupts from userspace

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2935 bytes --]

On Mon, Nov 18, 2019 at 04:37:16PM +0100, Cédric Le Goater wrote:
> On 18/11/2019 16:12, Greg Kurz wrote:
> > When using the XIVE KVM device, the trigger page is directly accessible
> > in QEMU. Unlike with XICS, no need to ask KVM to fire the interrupt. A
> > simple store on the trigger page does the job.
> > 
> > Just call xive_esb_trigger().
> 
> Yes but the KVM XIVE device does a few other checks. 
> 
> It checks that the interrupt was correctly initialized at the KVM device
> level. We should be fine in QEMU which has similar checks.
> 
> It caches the LSI assertion level. We should be fine also because it is
> useless in KVM when using the XIVE native exploitation mode.
> 
> It checks it is not a passthru interrupt. Any idea on how to check this 
> condition under QEMU ? 
>  
> > This may improve performance of emulated devices that go through
> > qemu_set_irq(), eg. virtio devices created with ioeventfd=off or
> > configured by the guest to use LSI interrupts, which aren't really
> > recommended setups.
> 
> LGTM.

Ok, between the comments above and this, I'm not sure if this is ready
to merge or not.

> 
> Any figures to share ? 
> 
> C.
> 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/intc/spapr_xive_kvm.c |   16 ++--------------
> >  1 file changed, 2 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> > index 08012ac7cd76..69e73552f1ef 100644
> > --- a/hw/intc/spapr_xive_kvm.c
> > +++ b/hw/intc/spapr_xive_kvm.c
> > @@ -354,32 +354,20 @@ static void kvmppc_xive_source_get_state(XiveSource *xsrc)
> >  void kvmppc_xive_source_set_irq(void *opaque, int srcno, int val)
> >  {
> >      XiveSource *xsrc = opaque;
> > -    SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
> > -    struct kvm_irq_level args;
> > -    int rc;
> > -
> > -    /* The KVM XIVE device should be in use */
> > -    assert(xive->fd != -1);
> >  
> > -    args.irq = srcno;
> >      if (!xive_source_irq_is_lsi(xsrc, srcno)) {
> >          if (!val) {
> >              return;
> >          }
> > -        args.level = KVM_INTERRUPT_SET;
> >      } else {
> >          if (val) {
> >              xsrc->status[srcno] |= XIVE_STATUS_ASSERTED;
> > -            args.level = KVM_INTERRUPT_SET_LEVEL;
> >          } else {
> >              xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED;
> > -            args.level = KVM_INTERRUPT_UNSET;
> >          }
> >      }
> > -    rc = kvm_vm_ioctl(kvm_state, KVM_IRQ_LINE, &args);
> > -    if (rc < 0) {
> > -        error_report("XIVE: kvm_irq_line() failed : %s", strerror(errno));
> > -    }
> > +
> > +    xive_esb_trigger(xsrc, srcno);
> >  }
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH for-5.0] xive/kvm: Trigger interrupts from userspace

[Cédric Le Goater]

On 19/11/2019 01:47, David Gibson wrote:
> On Mon, Nov 18, 2019 at 04:37:16PM +0100, Cédric Le Goater wrote:
>> On 18/11/2019 16:12, Greg Kurz wrote:
>>> When using the XIVE KVM device, the trigger page is directly accessible
>>> in QEMU. Unlike with XICS, no need to ask KVM to fire the interrupt. A
>>> simple store on the trigger page does the job.
>>>
>>> Just call xive_esb_trigger().
>>
>> Yes but the KVM XIVE device does a few other checks. 
>>
>> It checks that the interrupt was correctly initialized at the KVM device
>> level. We should be fine in QEMU which has similar checks.
>>
>> It caches the LSI assertion level. We should be fine also because it is
>> useless in KVM when using the XIVE native exploitation mode.
>>
>> It checks it is not a passthru interrupt. Any idea on how to check this 
>> condition under QEMU ? 
>>  
>>> This may improve performance of emulated devices that go through
>>> qemu_set_irq(), eg. virtio devices created with ioeventfd=off or
>>> configured by the guest to use LSI interrupts, which aren't really
>>> recommended setups.
>>
>> LGTM.
> 
> Ok, between the comments above and this, I'm not sure if this is ready
> to merge or not.

I think it is. 

With this change, we are loosing a check on passthrough interrupts but 
I am not sure how critical this is given that QEMU can anyhow bypass 
KVM and trigger the interrupt using a store on the ESB page. 

>> Any figures to share ? 

I am torturing Greg to have numbers :) but he resisted well.

>> C.
>>
>>> Signed-off-by: Greg Kurz <groug@kaod.org>

Let's move on.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

C.

>>> ---
>>>  hw/intc/spapr_xive_kvm.c |   16 ++--------------
>>>  1 file changed, 2 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
>>> index 08012ac7cd76..69e73552f1ef 100644
>>> --- a/hw/intc/spapr_xive_kvm.c
>>> +++ b/hw/intc/spapr_xive_kvm.c
>>> @@ -354,32 +354,20 @@ static void kvmppc_xive_source_get_state(XiveSource *xsrc)
>>>  void kvmppc_xive_source_set_irq(void *opaque, int srcno, int val)
>>>  {
>>>      XiveSource *xsrc = opaque;
>>> -    SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
>>> -    struct kvm_irq_level args;
>>> -    int rc;
>>> -
>>> -    /* The KVM XIVE device should be in use */
>>> -    assert(xive->fd != -1);
>>>  
>>> -    args.irq = srcno;
>>>      if (!xive_source_irq_is_lsi(xsrc, srcno)) {
>>>          if (!val) {
>>>              return;
>>>          }
>>> -        args.level = KVM_INTERRUPT_SET;
>>>      } else {
>>>          if (val) {
>>>              xsrc->status[srcno] |= XIVE_STATUS_ASSERTED;
>>> -            args.level = KVM_INTERRUPT_SET_LEVEL;
>>>          } else {
>>>              xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED;
>>> -            args.level = KVM_INTERRUPT_UNSET;
>>>          }
>>>      }
>>> -    rc = kvm_vm_ioctl(kvm_state, KVM_IRQ_LINE, &args);
>>> -    if (rc < 0) {
>>> -        error_report("XIVE: kvm_irq_line() failed : %s", strerror(errno));
>>> -    }
>>> +
>>> +    xive_esb_trigger(xsrc, srcno);
>>>  }
>>
>>
> 

Re: [PATCH for-5.0] xive/kvm: Trigger interrupts from userspace

[Greg Kurz]

On Tue, 19 Nov 2019 09:15:52 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> On 19/11/2019 01:47, David Gibson wrote:
> > On Mon, Nov 18, 2019 at 04:37:16PM +0100, Cédric Le Goater wrote:
> >> On 18/11/2019 16:12, Greg Kurz wrote:
> >>> When using the XIVE KVM device, the trigger page is directly accessible
> >>> in QEMU. Unlike with XICS, no need to ask KVM to fire the interrupt. A
> >>> simple store on the trigger page does the job.
> >>>
> >>> Just call xive_esb_trigger().
> >>
> >> Yes but the KVM XIVE device does a few other checks. 
> >>
> >> It checks that the interrupt was correctly initialized at the KVM device
> >> level. We should be fine in QEMU which has similar checks.
> >>
> >> It caches the LSI assertion level. We should be fine also because it is
> >> useless in KVM when using the XIVE native exploitation mode.
> >>
> >> It checks it is not a passthru interrupt. Any idea on how to check this 
> >> condition under QEMU ? 
> >>  
> >>> This may improve performance of emulated devices that go through
> >>> qemu_set_irq(), eg. virtio devices created with ioeventfd=off or
> >>> configured by the guest to use LSI interrupts, which aren't really
> >>> recommended setups.
> >>
> >> LGTM.
> > 
> > Ok, between the comments above and this, I'm not sure if this is ready
> > to merge or not.
> 
> I think it is. 
> 
> With this change, we are loosing a check on passthrough interrupts but 
> I am not sure how critical this is given that QEMU can anyhow bypass 
> KVM and trigger the interrupt using a store on the ESB page. 
> 

True. Thinking a bit more about this: nothing prevents such a store to
be the result of a bug somewhere else in QEMU, eg. some dangling pointer
with the same value, in a much easier way than doing the KVM ioctl. Is
it a concern we should take into account ?

> >> Any figures to share ? 
> 
> I am torturing Greg to have numbers :) but he resisted well.
> 

Maybe a _liquid_ bribe or two can be convincing enough :-)

> >> C.
> >>
> >>> Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> Let's move on.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> C.
> 
> >>> ---
> >>>  hw/intc/spapr_xive_kvm.c |   16 ++--------------
> >>>  1 file changed, 2 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> >>> index 08012ac7cd76..69e73552f1ef 100644
> >>> --- a/hw/intc/spapr_xive_kvm.c
> >>> +++ b/hw/intc/spapr_xive_kvm.c
> >>> @@ -354,32 +354,20 @@ static void kvmppc_xive_source_get_state(XiveSource *xsrc)
> >>>  void kvmppc_xive_source_set_irq(void *opaque, int srcno, int val)
> >>>  {
> >>>      XiveSource *xsrc = opaque;
> >>> -    SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
> >>> -    struct kvm_irq_level args;
> >>> -    int rc;
> >>> -
> >>> -    /* The KVM XIVE device should be in use */
> >>> -    assert(xive->fd != -1);
> >>>  
> >>> -    args.irq = srcno;
> >>>      if (!xive_source_irq_is_lsi(xsrc, srcno)) {
> >>>          if (!val) {
> >>>              return;
> >>>          }
> >>> -        args.level = KVM_INTERRUPT_SET;
> >>>      } else {
> >>>          if (val) {
> >>>              xsrc->status[srcno] |= XIVE_STATUS_ASSERTED;
> >>> -            args.level = KVM_INTERRUPT_SET_LEVEL;
> >>>          } else {
> >>>              xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED;
> >>> -            args.level = KVM_INTERRUPT_UNSET;
> >>>          }
> >>>      }
> >>> -    rc = kvm_vm_ioctl(kvm_state, KVM_IRQ_LINE, &args);
> >>> -    if (rc < 0) {
> >>> -        error_report("XIVE: kvm_irq_line() failed : %s", strerror(errno));
> >>> -    }
> >>> +
> >>> +    xive_esb_trigger(xsrc, srcno);
> >>>  }
> >>
> >>
> > 
> 

Re: [PATCH for-5.0] xive/kvm: Trigger interrupts from userspace

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2045 bytes --]

On Tue, Nov 19, 2019 at 09:15:52AM +0100, Cédric Le Goater wrote:
> On 19/11/2019 01:47, David Gibson wrote:
> > On Mon, Nov 18, 2019 at 04:37:16PM +0100, Cédric Le Goater wrote:
> >> On 18/11/2019 16:12, Greg Kurz wrote:
> >>> When using the XIVE KVM device, the trigger page is directly accessible
> >>> in QEMU. Unlike with XICS, no need to ask KVM to fire the interrupt. A
> >>> simple store on the trigger page does the job.
> >>>
> >>> Just call xive_esb_trigger().
> >>
> >> Yes but the KVM XIVE device does a few other checks. 
> >>
> >> It checks that the interrupt was correctly initialized at the KVM device
> >> level. We should be fine in QEMU which has similar checks.
> >>
> >> It caches the LSI assertion level. We should be fine also because it is
> >> useless in KVM when using the XIVE native exploitation mode.
> >>
> >> It checks it is not a passthru interrupt. Any idea on how to check this 
> >> condition under QEMU ? 
> >>  
> >>> This may improve performance of emulated devices that go through
> >>> qemu_set_irq(), eg. virtio devices created with ioeventfd=off or
> >>> configured by the guest to use LSI interrupts, which aren't really
> >>> recommended setups.
> >>
> >> LGTM.
> > 
> > Ok, between the comments above and this, I'm not sure if this is ready
> > to merge or not.
> 
> I think it is. 
> 
> With this change, we are loosing a check on passthrough interrupts but 
> I am not sure how critical this is given that QEMU can anyhow bypass 
> KVM and trigger the interrupt using a store on the ESB page. 
> 
> >> Any figures to share ? 
> 
> I am torturing Greg to have numbers :) but he resisted well.
> 
> >> C.
> >>
> >>> Signed-off-by: Greg Kurz <groug@kaod.org>
> 
> Let's move on.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>

Works for me.  Applied.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]