| Message ID | 20191009060818.29719-20-david@gibson.dropbear.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spapr: IRQ subsystem cleanup | expand |
On Wed, 9 Oct 2019 17:08:18 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > Traditional PCI INTx for vfio devices can only perform well if using > an in-kernel irqchip. Therefore, vfio_intx_update() issues a warning > if an in kernel irqchip is not available. > Can you elaborate on what doesn't "perform well" without an in-kernel irqchip ? Is it a matter of performance or is it actually broken or something else ? What is the impact on -machine accel=kvm,kernel-irqchip=off which always spit this warning ? > We usually do have an in-kernel irqchip available for pseries machines > on POWER hosts. However, because the platform allows feature > negotiation of what interrupt controller model to use, we don't > currently initialize it until machine reset. vfio_intx_update() is > called (first) from vfio_realize() before that, so it can issue a > spurious warning, even if we will have an in kernel irqchip by the > time we need it. > > To workaround this, make a call to spapr_irq_update_active_intc() from > spapr_irq_init() which is called at machine realize time, before the > vfio realize. This call will be pretty much obsoleted by the later > call at reset time, but it serves to suppress the spurious warning > from VFIO. > > Cc: Alex Williamson <alex.williamson@redhat.com> > Cc: Alexey Kardashevskiy <aik@ozlabs.ru> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr_irq.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index 7964e4a1b8..3aeb523f3e 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -274,6 +274,14 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > > spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, spapr, > smc->nr_xirqs + SPAPR_XIRQ_BASE); > + > + /* > + * Mostly we don't actually need this until reset, except that not > + * having this set up can cause VFIO devices to issue a > + * false-positive warning during realize(), because they don't yet > + * have an in-kernel irq chip. > + */ > + spapr_irq_update_active_intc(spapr); > } > > int spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp) > @@ -429,7 +437,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr) > * this. > */ > new_intc = SPAPR_INTC(spapr->xive); > - } else if (spapr_ovec_test(spapr->ov5_cas, OV5_XIVE_EXPLOIT)) { > + } else if (spapr->ov5_cas > + && spapr_ovec_test(spapr->ov5_cas, OV5_XIVE_EXPLOIT)) { > new_intc = SPAPR_INTC(spapr->xive); > } else { > new_intc = SPAPR_INTC(spapr->ics);
On Wed, Oct 09, 2019 at 10:37:38AM +0200, Greg Kurz wrote: > On Wed, 9 Oct 2019 17:08:18 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > Traditional PCI INTx for vfio devices can only perform well if using > > an in-kernel irqchip. Therefore, vfio_intx_update() issues a warning > > if an in kernel irqchip is not available. > > Can you elaborate on what doesn't "perform well" without an > in-kernel irqchip ? Not really, no, but Alex Williamson tells me it is soo. > Is it a matter of performance or is it > actually broken or something else ? I believe it's a matter of performance, but such a big one that it makes using it without kernel irqchip infeasible in many cases. > What is the impact on -machine accel=kvm,kernel-irqchip=off which > always spit this warning ? It should still spit that warning. > > We usually do have an in-kernel irqchip available for pseries machines > > on POWER hosts. However, because the platform allows feature > > negotiation of what interrupt controller model to use, we don't > > currently initialize it until machine reset. vfio_intx_update() is > > called (first) from vfio_realize() before that, so it can issue a > > spurious warning, even if we will have an in kernel irqchip by the > > time we need it. > > > > To workaround this, make a call to spapr_irq_update_active_intc() from > > spapr_irq_init() which is called at machine realize time, before the > > vfio realize. This call will be pretty much obsoleted by the later > > call at reset time, but it serves to suppress the spurious warning > > from VFIO. > > > > Cc: Alex Williamson <alex.williamson@redhat.com> > > Cc: Alexey Kardashevskiy <aik@ozlabs.ru> > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > hw/ppc/spapr_irq.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > > index 7964e4a1b8..3aeb523f3e 100644 > > --- a/hw/ppc/spapr_irq.c > > +++ b/hw/ppc/spapr_irq.c > > @@ -274,6 +274,14 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > > > > spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, spapr, > > smc->nr_xirqs + SPAPR_XIRQ_BASE); > > + > > + /* > > + * Mostly we don't actually need this until reset, except that not > > + * having this set up can cause VFIO devices to issue a > > + * false-positive warning during realize(), because they don't yet > > + * have an in-kernel irq chip. > > + */ > > + spapr_irq_update_active_intc(spapr); > > } > > > > int spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp) > > @@ -429,7 +437,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr) > > * this. > > */ > > new_intc = SPAPR_INTC(spapr->xive); > > - } else if (spapr_ovec_test(spapr->ov5_cas, OV5_XIVE_EXPLOIT)) { > > + } else if (spapr->ov5_cas > > + && spapr_ovec_test(spapr->ov5_cas, OV5_XIVE_EXPLOIT)) { > > new_intc = SPAPR_INTC(spapr->xive); > > } else { > > new_intc = SPAPR_INTC(spapr->ics); >
On Wed, 9 Oct 2019 19:52:59 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Oct 09, 2019 at 10:37:38AM +0200, Greg Kurz wrote: > > On Wed, 9 Oct 2019 17:08:18 +1100 > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > Traditional PCI INTx for vfio devices can only perform well if using > > > an in-kernel irqchip. Therefore, vfio_intx_update() issues a warning > > > if an in kernel irqchip is not available. > > > > Can you elaborate on what doesn't "perform well" without an > > in-kernel irqchip ? > > Not really, no, but Alex Williamson tells me it is soo. > > > Is it a matter of performance or is it > > actually broken or something else ? > > I believe it's a matter of performance, but such a big one that it > makes using it without kernel irqchip infeasible in many cases. > > > What is the impact on -machine accel=kvm,kernel-irqchip=off which > > always spit this warning ? > > It should still spit that warning. > Yeah of course it does, but it is expected that we cannot setup the irqfd since we deliberately don't have an in-kernel irqchip, isn't it ? Why spit a warning in this case ? Or is it just a not very user friendly way of saying "you will have poor performance if the VFIO device uses PCI INTx" ? > > > We usually do have an in-kernel irqchip available for pseries machines > > > on POWER hosts. However, because the platform allows feature > > > negotiation of what interrupt controller model to use, we don't > > > currently initialize it until machine reset. vfio_intx_update() is > > > called (first) from vfio_realize() before that, so it can issue a > > > spurious warning, even if we will have an in kernel irqchip by the > > > time we need it. > > > > > > To workaround this, make a call to spapr_irq_update_active_intc() from > > > spapr_irq_init() which is called at machine realize time, before the > > > vfio realize. This call will be pretty much obsoleted by the later > > > call at reset time, but it serves to suppress the spurious warning > > > from VFIO. > > > > > > Cc: Alex Williamson <alex.williamson@redhat.com> > > > Cc: Alexey Kardashevskiy <aik@ozlabs.ru> > > > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > > --- > > > hw/ppc/spapr_irq.c | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > > > index 7964e4a1b8..3aeb523f3e 100644 > > > --- a/hw/ppc/spapr_irq.c > > > +++ b/hw/ppc/spapr_irq.c > > > @@ -274,6 +274,14 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) > > > > > > spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, spapr, > > > smc->nr_xirqs + SPAPR_XIRQ_BASE); > > > + > > > + /* > > > + * Mostly we don't actually need this until reset, except that not > > > + * having this set up can cause VFIO devices to issue a > > > + * false-positive warning during realize(), because they don't yet > > > + * have an in-kernel irq chip. > > > + */ > > > + spapr_irq_update_active_intc(spapr); > > > } > > > > > > int spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp) > > > @@ -429,7 +437,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr) > > > * this. > > > */ > > > new_intc = SPAPR_INTC(spapr->xive); > > > - } else if (spapr_ovec_test(spapr->ov5_cas, OV5_XIVE_EXPLOIT)) { > > > + } else if (spapr->ov5_cas > > > + && spapr_ovec_test(spapr->ov5_cas, OV5_XIVE_EXPLOIT)) { > > > new_intc = SPAPR_INTC(spapr->xive); > > > } else { > > > new_intc = SPAPR_INTC(spapr->ics); > > >
On Wed, Oct 09, 2019 at 07:16:39PM +0200, Greg Kurz wrote: > On Wed, 9 Oct 2019 19:52:59 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Wed, Oct 09, 2019 at 10:37:38AM +0200, Greg Kurz wrote: > > > On Wed, 9 Oct 2019 17:08:18 +1100 > > > David Gibson <david@gibson.dropbear.id.au> wrote: > > > > > > > Traditional PCI INTx for vfio devices can only perform well if using > > > > an in-kernel irqchip. Therefore, vfio_intx_update() issues a warning > > > > if an in kernel irqchip is not available. > > > > > > Can you elaborate on what doesn't "perform well" without an > > > in-kernel irqchip ? > > > > Not really, no, but Alex Williamson tells me it is soo. > > > > > Is it a matter of performance or is it > > > actually broken or something else ? > > > > I believe it's a matter of performance, but such a big one that it > > makes using it without kernel irqchip infeasible in many cases. > > > > > What is the impact on -machine accel=kvm,kernel-irqchip=off which > > > always spit this warning ? > > > > It should still spit that warning. > > > > Yeah of course it does, but it is expected that we cannot setup > the irqfd since we deliberately don't have an in-kernel irqchip, > isn't it ? Why spit a warning in this case ? Or is it just a not > very user friendly way of saying "you will have poor performance > if the VFIO device uses PCI INTx" ? The last thing, AIUI.
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c index 7964e4a1b8..3aeb523f3e 100644 --- a/hw/ppc/spapr_irq.c +++ b/hw/ppc/spapr_irq.c @@ -274,6 +274,14 @@ void spapr_irq_init(SpaprMachineState *spapr, Error **errp) spapr->qirqs = qemu_allocate_irqs(spapr_set_irq, spapr, smc->nr_xirqs + SPAPR_XIRQ_BASE); + + /* + * Mostly we don't actually need this until reset, except that not + * having this set up can cause VFIO devices to issue a + * false-positive warning during realize(), because they don't yet + * have an in-kernel irq chip. + */ + spapr_irq_update_active_intc(spapr); } int spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error **errp) @@ -429,7 +437,8 @@ void spapr_irq_update_active_intc(SpaprMachineState *spapr) * this. */ new_intc = SPAPR_INTC(spapr->xive); - } else if (spapr_ovec_test(spapr->ov5_cas, OV5_XIVE_EXPLOIT)) { + } else if (spapr->ov5_cas + && spapr_ovec_test(spapr->ov5_cas, OV5_XIVE_EXPLOIT)) { new_intc = SPAPR_INTC(spapr->xive); } else { new_intc = SPAPR_INTC(spapr->ics);
Traditional PCI INTx for vfio devices can only perform well if using an in-kernel irqchip. Therefore, vfio_intx_update() issues a warning if an in kernel irqchip is not available. We usually do have an in-kernel irqchip available for pseries machines on POWER hosts. However, because the platform allows feature negotiation of what interrupt controller model to use, we don't currently initialize it until machine reset. vfio_intx_update() is called (first) from vfio_realize() before that, so it can issue a spurious warning, even if we will have an in kernel irqchip by the time we need it. To workaround this, make a call to spapr_irq_update_active_intc() from spapr_irq_init() which is called at machine realize time, before the vfio realize. This call will be pretty much obsoleted by the later call at reset time, but it serves to suppress the spurious warning from VFIO. Cc: Alex Williamson <alex.williamson@redhat.com> Cc: Alexey Kardashevskiy <aik@ozlabs.ru> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr_irq.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)