Re: [PATCH v2 2/2] spapr/irq: Only claim VALID interrupts at the KVM level

[Greg Kurz <groug@kaod.org>]

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

On Mon, 16 Sep 2019 10:44:32 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Sep 11, 2019 at 03:39:37PM +0200, Cédric Le Goater wrote:
> > A typical pseries VM with 16 vCPUs, one disk, one network adapater
> > uses less than 100 interrupts but the whole IRQ number space of the
> > QEMU machine is allocated at reset time and it is 8K wide. This is
> > wasting a considerable amount of interrupt numbers in the global IRQ
> > space which has 1M interrupts per socket on a POWER9.
> > 
> > To optimise the HW resources, only request at the KVM level interrupts
> > which have been claimed by the guest. This will help to increase the
> > maximum number of VMs per system and also help supporting nested guests
> > using the XIVE interrupt mode.
> > 
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> Applied to ppc-for-4.2, thanks.
> 

While experimenting 4.1->4.2 migration with your irq cleanup series,
I've hit this:

qemu-system-ppc64: KVM_SET_DEVICE_ATTR failed: Group 3 attr 0x0000000000001300: Invalid argument
qemu-system-ppc64: error while loading state for instance 0x0 of device 'spapr'
qemu-system-ppc64: load of migration failed: Operation not permitted

Failing to restore the source config with EINVAL seems to come from the
following check in kvmppc_xive_native_set_source_config():

	if (!state->valid)
		return -EINVAL;

which makes sense since we haven't requested any interrupt yet.

We should hence do it at post load before restoring the source
config.

I'll send a patch ASAP.

> > ---
> >  hw/intc/spapr_xive_kvm.c | 29 ++++++++++++++++++++++++++---
> >  hw/intc/xics_kvm.c       |  8 ++++++++
> >  2 files changed, 34 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c
> > index 17af4d19f54e..71b88d7797bc 100644
> > --- a/hw/intc/spapr_xive_kvm.c
> > +++ b/hw/intc/spapr_xive_kvm.c
> > @@ -255,11 +255,16 @@ void kvmppc_xive_source_reset_one(XiveSource *xsrc, int srcno, Error **errp)
> >  
> >  static void kvmppc_xive_source_reset(XiveSource *xsrc, Error **errp)
> >  {
> > +    SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
> >      int i;
> >  
> >      for (i = 0; i < xsrc->nr_irqs; i++) {
> >          Error *local_err = NULL;
> >  
> > +        if (!xive_eas_is_valid(&xive->eat[i])) {
> > +            continue;
> > +        }
> > +
> >          kvmppc_xive_source_reset_one(xsrc, i, &local_err);
> >          if (local_err) {
> >              error_propagate(errp, local_err);
> > @@ -328,11 +333,18 @@ uint64_t kvmppc_xive_esb_rw(XiveSource *xsrc, int srcno, uint32_t offset,
> >  
> >  static void kvmppc_xive_source_get_state(XiveSource *xsrc)
> >  {
> > +    SpaprXive *xive = SPAPR_XIVE(xsrc->xive);
> >      int i;
> >  
> >      for (i = 0; i < xsrc->nr_irqs; i++) {
> > +        uint8_t pq;
> > +
> > +        if (!xive_eas_is_valid(&xive->eat[i])) {
> > +            continue;
> > +        }
> > +
> >          /* Perform a load without side effect to retrieve the PQ bits */
> > -        uint8_t pq = xive_esb_read(xsrc, i, XIVE_ESB_GET);
> > +        pq = xive_esb_read(xsrc, i, XIVE_ESB_GET);
> >  
> >          /* and save PQ locally */
> >          xive_source_esb_set(xsrc, i, pq);
> > @@ -521,9 +533,14 @@ static void kvmppc_xive_change_state_handler(void *opaque, int running,
> >       */
> >      if (running) {
> >          for (i = 0; i < xsrc->nr_irqs; i++) {
> > -            uint8_t pq = xive_source_esb_get(xsrc, i);
> > +            uint8_t pq;
> >              uint8_t old_pq;
> >  
> > +            if (!xive_eas_is_valid(&xive->eat[i])) {
> > +                continue;
> > +            }
> > +
> > +            pq = xive_source_esb_get(xsrc, i);
> >              old_pq = xive_esb_read(xsrc, i, XIVE_ESB_SET_PQ_00 + (pq << 8));
> >  
> >              /*
> > @@ -545,7 +562,13 @@ static void kvmppc_xive_change_state_handler(void *opaque, int running,
> >       * migration is in progress.
> >       */
> >      for (i = 0; i < xsrc->nr_irqs; i++) {
> > -        uint8_t pq = xive_esb_read(xsrc, i, XIVE_ESB_GET);
> > +        uint8_t pq;
> > +
> > +        if (!xive_eas_is_valid(&xive->eat[i])) {
> > +            continue;
> > +        }
> > +
> > +        pq = xive_esb_read(xsrc, i, XIVE_ESB_GET);
> >  
> >          /*
> >           * PQ is set to PENDING to possibly catch a triggered
> > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> > index a4d2e876cc5f..ba90d6dc966c 100644
> > --- a/hw/intc/xics_kvm.c
> > +++ b/hw/intc/xics_kvm.c
> > @@ -190,6 +190,10 @@ void ics_get_kvm_state(ICSState *ics)
> >      for (i = 0; i < ics->nr_irqs; i++) {
> >          ICSIRQState *irq = &ics->irqs[i];
> >  
> > +        if (ics_irq_free(ics, i)) {
> > +            continue;
> > +        }
> > +
> >          kvm_device_access(kernel_xics_fd, KVM_DEV_XICS_GRP_SOURCES,
> >                            i + ics->offset, &state, false, &error_fatal);
> >  
> > @@ -301,6 +305,10 @@ int ics_set_kvm_state(ICSState *ics, Error **errp)
> >          Error *local_err = NULL;
> >          int ret;
> >  
> > +        if (ics_irq_free(ics, i)) {
> > +            continue;
> > +        }
> > +
> >          ret = ics_set_kvm_state_one(ics, i, &local_err);
> >          if (ret < 0) {
> >              error_propagate(errp, local_err);
> 


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