Re: [RFC PATCH] spapr/xive: Allocate vCPU IPIs from local context

[Cédric Le Goater]

On 9/23/21 11:12, Greg Kurz wrote:
> On Wed, 22 Sep 2021 16:41:20 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
>
> > When QEMU switches to the XIVE interrupt mode, it creates all possible
> > guest interrupts at the level of the KVM device. These interrupts are
> > backed by real HW interrupts from the IPI interrupt pool of the XIVE
> > controller.
> >
> > Currently, this is done from the QEMU main thread, which results in
> > allocating all interrupts from the chip on which QEMU is running. IPIs
> > are not distributed across the system and the load is not well
> > balanced across the interrupt controllers.
> >
> > To improve distribution on the system, we should try to allocate the
> > underlying HW IPI from the vCPU context. This also benefits to
> > configurations using CPU pinning. In this case, the HW IPI is
> > allocated on the local chip, rerouting between interrupt controllers
> > is reduced and performance improved.
> >
> > This moves the initialization of the vCPU IPIs from reset time to the
> > H_INT_SET_SOURCE_CONFIG hcall which is called from the vCPU context.
> > But this needs some extra checks in the sequences getting and setting
> > the source states to make sure a valid HW IPI is backing the guest
> > interrupt. For that, we check if a target was configured in the END in
> > case of a vCPU IPI.
> >
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> >
> >   I have tested different SMT configurations, kernel_irqchip=off/on,
> >   did some migrations, CPU hotplug, etc. It's not enough and I would
> >   like more testing but, at least, it is not making anymore the bad
> >   assumption vCPU id = IPI number.
>
> Yeah, the IPI number is provided by the guest, so h_int_set_source_config()
> is really the only place where we can know the IPI number of a given vCPU.

The patch lacks a run_on_cpu() to perform the reset on the vCPU context
to be complete.

> >   Comments ?
>
> LGTM but I didn't check if more users of xive_end_is_valid() should
> be converted to using xive_source_is_initialized().

I think you mean xive_eas_is_valid() ?

The changes only impact KVM support since we are deferring the IRQ
initialization at the KVM level. What we have to be careful about is not
accessing an ESB page of an interrupt that would not have been initiliazed
in the KVM device, else QEMU gets a sigbus.

That only happens when QEMU gets/sets the ESB states.
 
> Any chance you have some perf numbers to share ?

I will try.

Thanks,

C.

 
> >   hw/intc/spapr_xive.c     | 17 +++++++++++++++++
> >   hw/intc/spapr_xive_kvm.c | 36 +++++++++++++++++++++++++++++++-----
> >   2 files changed, 48 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> > index 6f31ce74f198..2dc594a720b1 100644
> > --- a/hw/intc/spapr_xive.c
> > +++ b/hw/intc/spapr_xive.c
> > @@ -1089,6 +1089,23 @@ static target_ulong h_int_set_source_config(PowerPCCPU 
> > *cpu,
> >       if (spapr_xive_in_kernel(xive)) {
> >           Error *local_err = NULL;
> >   
> > +        /*
> > +         * Initialize the vCPU IPIs from the vCPU context to allocate
> > +         * the backing HW IPI on the local chip. This improves
> > +         * distribution of the IPIs in the system and when the vCPUs
> > +         * are pinned, it reduces rerouting between interrupt
> > +         * controllers for better performance.
> > +         */
> > +        if (lisn < SPAPR_XIRQ_BASE) {
> > +            XiveSource *xsrc = &xive->source;
> > +
> > +            kvmppc_xive_source_reset_one(xsrc, lisn, &local_err);
> > +            if (local_err) {
> > +                error_report_err(local_err);
> > +                return H_HARDWARE;
> > +            }
> > +        }
> > +
> >           kvmppc_xive_set_source_config(xive, lisn, &new_eas, &local_err);
> >           if (local_err) {
> >               error_report_err(local_err);
> > diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> > index 53731d158625..1607a59e9483 100644
> > --- a/hw/intc/spapr_xive_kvm.c
> > +++ b/hw/intc/spapr_xive_kvm.c
> > @@ -254,7 +254,12 @@ static int kvmppc_xive_source_reset(XiveSource *xsrc, 
> > Error **errp)
> >       SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
> >       int i;
> >   
> > -    for (i = 0; i < xsrc->nr_irqs; i++) {
> > +    /*
> > +     * vCPU IPIs are initialized at the KVM level when configured by
> > +     * H_INT_SET_SOURCE_CONFIG.
> > +     */
> > +
> > +    for (i = SPAPR_XIRQ_BASE; i < xsrc->nr_irqs; i++) {
> >           int ret;
> >   
> >           if (!xive_eas_is_valid(&xive->eat[i])) {
> > @@ -342,6 +347,27 @@ uint64_t kvmppc_xive_esb_rw(XiveSource *xsrc, int srcno, 
> > uint32_t offset,
> >       }
> >   }
> >   
> > +static bool xive_source_is_initialized(SpaprXive *xive, int lisn)
> > +{
> > +    assert(lisn < xive->nr_irqs);
> > +
> > +    if (!xive_eas_is_valid(&xive->eat[lisn])) {
> > +        return false;
> > +    }
> > +
> > +    /*
> > +     * vCPU IPIs are initialized at the KVM level when configured by
> > +     * H_INT_SET_SOURCE_CONFIG, in which case, we should have a valid
> > +     * target (server, priority) in the END.
> > +     */
> > +    if (lisn < SPAPR_XIRQ_BASE) {
> > +        return !!xive_get_field64(EAS_END_INDEX, xive->eat[lisn].w);
> > +    }
> > +
> > +    /* Device sources */
> > +    return true;
> > +}
> > +
> >   static void kvmppc_xive_source_get_state(XiveSource *xsrc)
> >   {
> >       SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
> > @@ -350,7 +376,7 @@ static void kvmppc_xive_source_get_state(XiveSource *xsrc)
> >       for (i = 0; i < xsrc->nr_irqs; i++) {
> >           uint8_t pq;
> >   
> > -        if (!xive_eas_is_valid(&xive->eat[i])) {
> > +        if (!xive_source_is_initialized(xive, i)) {
> >               continue;
> >           }
> >   
> > @@ -533,7 +559,7 @@ static void kvmppc_xive_change_state_handler(void *opaque, bool running,
> >               uint8_t pq;
> >               uint8_t old_pq;
> >   
> > -            if (!xive_eas_is_valid(&xive->eat[i])) {
> > +            if (!xive_source_is_initialized(xive, i)) {
> >                   continue;
> >               }
> >   
> > @@ -561,7 +587,7 @@ static void kvmppc_xive_change_state_handler(void *opaque, bool running,
> >       for (i = 0; i < xsrc->nr_irqs; i++) {
> >           uint8_t pq;
> >   
> > -        if (!xive_eas_is_valid(&xive->eat[i])) {
> > +        if (!xive_source_is_initialized(xive, i)) {
> >               continue;
> >           }
> >   
> > @@ -666,7 +692,7 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id)
> >   
> >       /* Restore the EAT */
> >       for (i = 0; i < xive->nr_irqs; i++) {
> > -        if (!xive_eas_is_valid(&xive->eat[i])) {
> > +        if (!xive_source_is_initialized(xive, i)) {
> >               continue;
> >           }
> >