On 8/20/20 3:45 PM, Cédric Le Goater wrote: > The vCPU IPIs are now allocated in kvmppc_xive_cpu_connect() when the > vCPU connects to the KVM device and not when all the sources are reset > in kvmppc_xive_source_reset()
This patch is introducing a regression when vsmt is in used. https://bugs.launchpad.net/qemu/+bug/1900241 when the OS boots, H_INT_SET_SOURCE_CONFIG fails with EINVAL, which should mean that the IPI is not created at the host/KVM level. > This requires extra care for hotplug vCPUs and VM restore. > > Signed-off-by: Cédric Le Goater <c...@kaod.org> > --- > hw/intc/spapr_xive_kvm.c | 47 +++++++++++++++++++++++++++++++++++----- > 1 file changed, 42 insertions(+), 5 deletions(-) > > diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c > index 4ea687c93ecd..f838c208b559 100644 > --- a/hw/intc/spapr_xive_kvm.c > +++ b/hw/intc/spapr_xive_kvm.c > @@ -146,6 +146,15 @@ int kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, > Error **errp) > return s.ret; > } > > +static int kvmppc_xive_reset_ipi(SpaprXive *xive, CPUState *cs, Error **errp) > +{ > + unsigned long ipi = kvm_arch_vcpu_id(cs); ( I am wondering if this is the correct id to use ? ) > + uint64_t state = 0; > + > + return kvm_device_access(xive->fd, KVM_DEV_XIVE_GRP_SOURCE, ipi, > + &state, true, errp); > +} > + > int kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp) > { > ERRP_GUARD(); > @@ -175,6 +184,12 @@ int kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp) > return ret; > } > > + /* Create/reset the vCPU IPI */ > + ret = kvmppc_xive_reset_ipi(xive, tctx->cs, errp); > + if (ret < 0) { > + return ret; > + } > + > kvm_cpu_enable(tctx->cs); > return 0; > } > @@ -260,6 +275,12 @@ int kvmppc_xive_source_reset_one(XiveSource *xsrc, int > srcno, Error **errp) > > assert(xive->fd != -1); > > + /* > + * The vCPU IPIs are now allocated in kvmppc_xive_cpu_connect() > + * and not with all sources in kvmppc_xive_source_reset() > + */ > + assert(srcno >= SPAPR_XIRQ_BASE); > + > if (xive_source_irq_is_lsi(xsrc, srcno)) { > state |= KVM_XIVE_LEVEL_SENSITIVE; > if (xsrc->status[srcno] & XIVE_STATUS_ASSERTED) { > @@ -271,12 +292,28 @@ int kvmppc_xive_source_reset_one(XiveSource *xsrc, int > srcno, Error **errp) > true, errp); > } > > +/* > + * To be valid, a source must have been claimed by the machine (valid > + * entry in the EAS table) and if it is a vCPU IPI, the vCPU should > + * have been enabled, which means the IPI has been allocated in > + * kvmppc_xive_cpu_connect(). > + */ > +static bool xive_source_is_valid(SpaprXive *xive, int i) > +{ > + return xive_eas_is_valid(&xive->eat[i]) && > + (i >= SPAPR_XIRQ_BASE || kvm_cpu_is_enabled(i)); > +} > + > 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++) { > + /* > + * Skip the vCPU IPIs. These are created/reset when the vCPUs are > + * connected in kvmppc_xive_cpu_connect() > + */ > + for (i = SPAPR_XIRQ_BASE; i < xsrc->nr_irqs; i++) { This change skips the range [ 0x0 ... 0x1000 ] and relies on the presenter to create the vCPU IPIs at the KVM level. But spapr_irq_init() could have claimed more in : /* Enable the CPU IPIs */ for (i = 0; i < nr_servers; ++i) { SpaprInterruptControllerClass *sicc = SPAPR_INTC_GET_CLASS(spapr->xive); if (sicc->claim_irq(SPAPR_INTC(spapr->xive), SPAPR_IRQ_IPI + i, false, errp) < 0) { return; } } I think this is what is happening with vsmt. However, I don't know how to fix it :/ Thanks, C. > int ret; > > if (!xive_eas_is_valid(&xive->eat[i])) { > @@ -370,7 +407,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_valid(xive, i)) { > continue; > } > > @@ -553,7 +590,7 @@ static void kvmppc_xive_change_state_handler(void > *opaque, int running, > uint8_t pq; > uint8_t old_pq; > > - if (!xive_eas_is_valid(&xive->eat[i])) { > + if (!xive_source_is_valid(xive, i)) { > continue; > } > > @@ -581,7 +618,7 @@ static void kvmppc_xive_change_state_handler(void > *opaque, int running, > for (i = 0; i < xsrc->nr_irqs; i++) { > uint8_t pq; > > - if (!xive_eas_is_valid(&xive->eat[i])) { > + if (!xive_source_is_valid(xive, i)) { > continue; > } > > @@ -696,7 +733,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_valid(xive, i)) { > continue; > } > >
