| Message ID | 160528045027.804522.6161091782230763832.stgit@bahia.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-5.2] spapr/xive: Create IPIs in KVM on demand | expand |
On 11/13/20 4:14 PM, Greg Kurz wrote: > Recent commit acbdb9956fe9 introduced a dedicated path to create > IPIs in KVM. This is done from under kvmppc_xive_cpu_connect() with > the assumption that the IPI number is equal to the vCPU id. The > latter is wrong: the guest chooses an arbitrary LISN from the > "ibm,xive-lisn-ranges" and assigns it to a target vCPU with the > H_INT_SET_SOURCE_CONFIG hcall. This went unnoticed so far because > IPI numbers and vCPU ids happen to match by default. This is no > longer the case though when setting the VSMT machine property to > a value that is different from (ie. bigger than) the number of > vCPUs per core (ie. -smp threads). Wrong IPIs end up being created > in KVM but the guest still uses the ones it has allocated and gets > very confused (see BugLink below). > > Fix this by creating the IPI at the only place where we have > its appropriate number : when the guest allocates it with the > H_INT_SET_SOURCE_CONFIG hcall. We detect this is an IPI because > it is < SPAPR_XIRQ_BASE and we get the vCPU id from the hcall > arguments. The EAS of the IPI is tracked in the kvm_enabled_cpus > list. It is now used instead of vcpu_id to filter unallocated IPIs > out in xive_source_is_valid(). It also allows to only reset the > IPI on the first call to H_INT_SET_SOURCE_CONFIG. > > Restore unmasked IPIs from the vCPU contexts in kvmppc_xive_post_load(). > Masked ones will be created when the guests eventually unmask them > with H_INT_SET_SOURCE_CONFIG. > > Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com> > Fixes: acbdb9956fe9 ("spapr/xive: Allocate IPIs independently from the other sources") > BugLink: https://bugs.launchpad.net/qemu/+bug/1900241 > Cc: clg@kaod.org > Signed-off-by: Greg Kurz <groug@kaod.org> I am quite certain this is correct but the complexity looks like a potential source of bugs. So I think it is simpler to revert the optimization introduced by acbdb9956fe9 and find a better solution covering SMT also. Thanks, C. > --- > hw/intc/spapr_xive_kvm.c | 141 +++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 127 insertions(+), 14 deletions(-) > > diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c > index 66bf4c06fe55..4e5871c3aac2 100644 > --- a/hw/intc/spapr_xive_kvm.c > +++ b/hw/intc/spapr_xive_kvm.c > @@ -30,6 +30,7 @@ > */ > typedef struct KVMEnabledCPU { > unsigned long vcpu_id; > + XiveEAS *ipi_eas; > QLIST_ENTRY(KVMEnabledCPU) node; > } KVMEnabledCPU; > > @@ -55,6 +56,7 @@ static void kvm_cpu_enable(CPUState *cs) > > enabled_cpu = g_malloc(sizeof(*enabled_cpu)); > enabled_cpu->vcpu_id = vcpu_id; > + enabled_cpu->ipi_eas = NULL; > QLIST_INSERT_HEAD(&kvm_enabled_cpus, enabled_cpu, node); > } > > @@ -156,26 +158,29 @@ int kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp) > */ > typedef struct { > SpaprXive *xive; > + uint32_t lisn; > Error *err; > int rc; > } XiveInitIPI; > > static void kvmppc_xive_reset_ipi_on_cpu(CPUState *cs, run_on_cpu_data arg) > { > - unsigned long ipi = kvm_arch_vcpu_id(cs); > XiveInitIPI *s = arg.host_ptr; > + unsigned long ipi = s->lisn; > uint64_t state = 0; > > s->rc = kvm_device_access(s->xive->fd, KVM_DEV_XIVE_GRP_SOURCE, ipi, > &state, true, &s->err); > } > > -static int kvmppc_xive_reset_ipi(SpaprXive *xive, CPUState *cs, Error **errp) > +static int kvmppc_xive_reset_ipi(SpaprXive *xive, CPUState *cs, uint32_t lisn, > + Error **errp) > { > XiveInitIPI s = { > .xive = xive, > .err = NULL, > .rc = 0, > + .lisn = lisn, > }; > > run_on_cpu(cs, kvmppc_xive_reset_ipi_on_cpu, RUN_ON_CPU_HOST_PTR(&s)); > @@ -214,12 +219,6 @@ 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; > } > @@ -228,6 +227,62 @@ int kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp) > * XIVE Interrupt Source (KVM) > */ > > +static bool spapr_xive_is_ipi(uint32_t lisn) > +{ > + return lisn < SPAPR_XIRQ_BASE; > +} > + > +static bool kvm_ipi_is_enabled(SpaprXive *xive, uint32_t lisn) > +{ > + KVMEnabledCPU *enabled_cpu; > + > + g_assert(spapr_xive_is_ipi(lisn)); > + > + QLIST_FOREACH(enabled_cpu, &kvm_enabled_cpus, node) { > + if (enabled_cpu->ipi_eas == &xive->eat[lisn]) { > + return true; > + } > + } > + return false; > +} > + > +static int kvm_ipi_enable(SpaprXive *xive, uint32_t lisn, uint32_t vcpu_id, > + Error **errp) > +{ > + KVMEnabledCPU *enabled_cpu; > + > + g_assert(spapr_xive_is_ipi(lisn)); > + > + QLIST_FOREACH(enabled_cpu, &kvm_enabled_cpus, node) { > + if (enabled_cpu->vcpu_id == vcpu_id) { > + CPUState *cs = CPU(spapr_find_cpu(vcpu_id)); > + XiveEAS *eas = &xive->eat[lisn]; > + > + /* No change ? */ > + if (enabled_cpu->ipi_eas && enabled_cpu->ipi_eas == eas) { > + return 0; > + } > + > + /* XXX: abort ? */ > + if (!cs) { > + break; > + } > + > + /* Create/reset the vCPU IPI */ > + int ret = kvmppc_xive_reset_ipi(xive, cs, lisn, errp); > + if (ret < 0) { > + return ret; > + } > + > + enabled_cpu->ipi_eas = eas; > + return 0; > + } > + } > + > + error_setg(errp, "vCPU #%d not found", vcpu_id); > + return -ESRCH; > +} > + > int kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas, > Error **errp) > { > @@ -248,6 +303,14 @@ int kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas, > > spapr_xive_end_to_target(end_blk, end_idx, &server, &priority); > > + if (spapr_xive_is_ipi(lisn)) { > + /* Create the vCPU IPI */ > + int ret = kvm_ipi_enable(xive, lisn, server, errp); > + if (ret < 0) { > + return ret; > + } > + } > + > kvm_src = priority << KVM_XIVE_SOURCE_PRIORITY_SHIFT & > KVM_XIVE_SOURCE_PRIORITY_MASK; > kvm_src |= server << KVM_XIVE_SOURCE_SERVER_SHIFT & > @@ -280,7 +343,7 @@ 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() > + * The vCPU IPIs are now allocated in kvmppc_xive_set_source_config() > * and not with all sources in kvmppc_xive_source_reset() > */ > assert(srcno >= SPAPR_XIRQ_BASE); > @@ -300,12 +363,12 @@ int kvmppc_xive_source_reset_one(XiveSource *xsrc, int srcno, Error **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(). > + * kvmppc_xive_set_source_config(). > */ > 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)); > + (!spapr_xive_is_ipi(i) || kvm_ipi_is_enabled(xive, i)); > } > > static int kvmppc_xive_source_reset(XiveSource *xsrc, Error **errp) > @@ -314,8 +377,8 @@ static int kvmppc_xive_source_reset(XiveSource *xsrc, Error **errp) > int i; > > /* > - * Skip the vCPU IPIs. These are created/reset when the vCPUs are > - * connected in kvmppc_xive_cpu_connect() > + * Skip the vCPU IPIs. These are created/reset on-demand in > + * kvmppc_xive_set_source_config(). > */ > for (i = SPAPR_XIRQ_BASE; i < xsrc->nr_irqs; i++) { > int ret; > @@ -724,7 +787,57 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id) > } > > /* Restore the EAT */ > - for (i = 0; i < xive->nr_irqs; i++) { > + > + /* IPIs are restored from the appropriate vCPU context */ > + CPU_FOREACH(cs) { > + /* > + * The EAT has valid entries to accomodate all possible vCPUs, > + * but we only want to allocate in KVM the IPIs that were > + * actually allocated before migration. Let's consider the full > + * list of IPIs to find an EAS that matches the vCPU id. > + * > + * If an IPI appears unmasked in the EAT, it is a proof that the > + * guest did successfully call H_INT_SET_SOURCE_CONFIG and we > + * should thus create the IPI at the KVM level if the END index > + * matches the vCPU id. > + * > + * If an IPI appears masked in the EAT, then we don't know exactly > + * what happened before migration but we don't care. The IPI will > + * be created when the guest eventually unmasks it with a subsequent > + * call to H_INT_SET_SOURCE_CONFIG. > + */ > + for (i = 0; i < SPAPR_XIRQ_BASE; i++) { > + XiveEAS *eas = &xive->eat[i]; > + uint32_t end_idx; > + uint32_t end_blk; > + uint8_t priority; > + uint32_t server; > + > + if (!xive_eas_is_valid(eas)) { > + continue; > + } > + > + if (xive_eas_is_masked(eas)) { > + continue; > + } > + > + end_idx = xive_get_field64(EAS_END_INDEX, eas->w); > + end_blk = xive_get_field64(EAS_END_BLOCK, eas->w); > + spapr_xive_end_to_target(end_blk, end_idx, &server, &priority); > + if (server != kvm_arch_vcpu_id(cs)) { > + continue; > + } > + > + ret = kvmppc_xive_set_source_config(xive, i, eas, &local_err); > + if (ret < 0) { > + goto fail; > + } > + break; > + } > + } > + > + /* Now restore non-IPIs */ > + for (i = SPAPR_XIRQ_BASE; i < xive->nr_irqs; i++) { > if (!xive_source_is_valid(xive, i)) { > continue; > } > >
On Mon, 16 Nov 2020 14:43:12 +0100 Cédric Le Goater <clg@kaod.org> wrote: > On 11/13/20 4:14 PM, Greg Kurz wrote: > > Recent commit acbdb9956fe9 introduced a dedicated path to create > > IPIs in KVM. This is done from under kvmppc_xive_cpu_connect() with > > the assumption that the IPI number is equal to the vCPU id. The > > latter is wrong: the guest chooses an arbitrary LISN from the > > "ibm,xive-lisn-ranges" and assigns it to a target vCPU with the > > H_INT_SET_SOURCE_CONFIG hcall. This went unnoticed so far because > > IPI numbers and vCPU ids happen to match by default. This is no > > longer the case though when setting the VSMT machine property to > > a value that is different from (ie. bigger than) the number of > > vCPUs per core (ie. -smp threads). Wrong IPIs end up being created > > in KVM but the guest still uses the ones it has allocated and gets > > very confused (see BugLink below). > > > > Fix this by creating the IPI at the only place where we have > > its appropriate number : when the guest allocates it with the > > H_INT_SET_SOURCE_CONFIG hcall. We detect this is an IPI because > > it is < SPAPR_XIRQ_BASE and we get the vCPU id from the hcall > > arguments. The EAS of the IPI is tracked in the kvm_enabled_cpus > > list. It is now used instead of vcpu_id to filter unallocated IPIs > > out in xive_source_is_valid(). It also allows to only reset the > > IPI on the first call to H_INT_SET_SOURCE_CONFIG. > > > > Restore unmasked IPIs from the vCPU contexts in kvmppc_xive_post_load(). > > Masked ones will be created when the guests eventually unmask them > > with H_INT_SET_SOURCE_CONFIG. > > > > Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com> > > Fixes: acbdb9956fe9 ("spapr/xive: Allocate IPIs independently from the other sources") > > BugLink: https://bugs.launchpad.net/qemu/+bug/1900241 > > Cc: clg@kaod.org > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > I am quite certain this is correct but the complexity looks like a > potential source of bugs. So I think it is simpler to revert the > optimization introduced by acbdb9956fe9 and find a better solution > covering SMT also. > I tend to agree. Even if I could successfully test various scenarios around hotplug and migration, I'm really not super confident to merge this in an RC. I'll post a series that reverts acbdb9956fe9 ASAP. > Thanks, > > C. > > > > > --- > > hw/intc/spapr_xive_kvm.c | 141 +++++++++++++++++++++++++++++++++++++++++----- > > 1 file changed, 127 insertions(+), 14 deletions(-) > > > > diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c > > index 66bf4c06fe55..4e5871c3aac2 100644 > > --- a/hw/intc/spapr_xive_kvm.c > > +++ b/hw/intc/spapr_xive_kvm.c > > @@ -30,6 +30,7 @@ > > */ > > typedef struct KVMEnabledCPU { > > unsigned long vcpu_id; > > + XiveEAS *ipi_eas; > > QLIST_ENTRY(KVMEnabledCPU) node; > > } KVMEnabledCPU; > > > > @@ -55,6 +56,7 @@ static void kvm_cpu_enable(CPUState *cs) > > > > enabled_cpu = g_malloc(sizeof(*enabled_cpu)); > > enabled_cpu->vcpu_id = vcpu_id; > > + enabled_cpu->ipi_eas = NULL; > > QLIST_INSERT_HEAD(&kvm_enabled_cpus, enabled_cpu, node); > > } > > > > @@ -156,26 +158,29 @@ int kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp) > > */ > > typedef struct { > > SpaprXive *xive; > > + uint32_t lisn; > > Error *err; > > int rc; > > } XiveInitIPI; > > > > static void kvmppc_xive_reset_ipi_on_cpu(CPUState *cs, run_on_cpu_data arg) > > { > > - unsigned long ipi = kvm_arch_vcpu_id(cs); > > XiveInitIPI *s = arg.host_ptr; > > + unsigned long ipi = s->lisn; > > uint64_t state = 0; > > > > s->rc = kvm_device_access(s->xive->fd, KVM_DEV_XIVE_GRP_SOURCE, ipi, > > &state, true, &s->err); > > } > > > > -static int kvmppc_xive_reset_ipi(SpaprXive *xive, CPUState *cs, Error **errp) > > +static int kvmppc_xive_reset_ipi(SpaprXive *xive, CPUState *cs, uint32_t lisn, > > + Error **errp) > > { > > XiveInitIPI s = { > > .xive = xive, > > .err = NULL, > > .rc = 0, > > + .lisn = lisn, > > }; > > > > run_on_cpu(cs, kvmppc_xive_reset_ipi_on_cpu, RUN_ON_CPU_HOST_PTR(&s)); > > @@ -214,12 +219,6 @@ 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; > > } > > @@ -228,6 +227,62 @@ int kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp) > > * XIVE Interrupt Source (KVM) > > */ > > > > +static bool spapr_xive_is_ipi(uint32_t lisn) > > +{ > > + return lisn < SPAPR_XIRQ_BASE; > > +} > > + > > +static bool kvm_ipi_is_enabled(SpaprXive *xive, uint32_t lisn) > > +{ > > + KVMEnabledCPU *enabled_cpu; > > + > > + g_assert(spapr_xive_is_ipi(lisn)); > > + > > + QLIST_FOREACH(enabled_cpu, &kvm_enabled_cpus, node) { > > + if (enabled_cpu->ipi_eas == &xive->eat[lisn]) { > > + return true; > > + } > > + } > > + return false; > > +} > > + > > +static int kvm_ipi_enable(SpaprXive *xive, uint32_t lisn, uint32_t vcpu_id, > > + Error **errp) > > +{ > > + KVMEnabledCPU *enabled_cpu; > > + > > + g_assert(spapr_xive_is_ipi(lisn)); > > + > > + QLIST_FOREACH(enabled_cpu, &kvm_enabled_cpus, node) { > > + if (enabled_cpu->vcpu_id == vcpu_id) { > > + CPUState *cs = CPU(spapr_find_cpu(vcpu_id)); > > + XiveEAS *eas = &xive->eat[lisn]; > > + > > + /* No change ? */ > > + if (enabled_cpu->ipi_eas && enabled_cpu->ipi_eas == eas) { > > + return 0; > > + } > > + > > + /* XXX: abort ? */ > > + if (!cs) { > > + break; > > + } > > + > > + /* Create/reset the vCPU IPI */ > > + int ret = kvmppc_xive_reset_ipi(xive, cs, lisn, errp); > > + if (ret < 0) { > > + return ret; > > + } > > + > > + enabled_cpu->ipi_eas = eas; > > + return 0; > > + } > > + } > > + > > + error_setg(errp, "vCPU #%d not found", vcpu_id); > > + return -ESRCH; > > +} > > + > > int kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas, > > Error **errp) > > { > > @@ -248,6 +303,14 @@ int kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas, > > > > spapr_xive_end_to_target(end_blk, end_idx, &server, &priority); > > > > + if (spapr_xive_is_ipi(lisn)) { > > + /* Create the vCPU IPI */ > > + int ret = kvm_ipi_enable(xive, lisn, server, errp); > > + if (ret < 0) { > > + return ret; > > + } > > + } > > + > > kvm_src = priority << KVM_XIVE_SOURCE_PRIORITY_SHIFT & > > KVM_XIVE_SOURCE_PRIORITY_MASK; > > kvm_src |= server << KVM_XIVE_SOURCE_SERVER_SHIFT & > > @@ -280,7 +343,7 @@ 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() > > + * The vCPU IPIs are now allocated in kvmppc_xive_set_source_config() > > * and not with all sources in kvmppc_xive_source_reset() > > */ > > assert(srcno >= SPAPR_XIRQ_BASE); > > @@ -300,12 +363,12 @@ int kvmppc_xive_source_reset_one(XiveSource *xsrc, int srcno, Error **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(). > > + * kvmppc_xive_set_source_config(). > > */ > > 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)); > > + (!spapr_xive_is_ipi(i) || kvm_ipi_is_enabled(xive, i)); > > } > > > > static int kvmppc_xive_source_reset(XiveSource *xsrc, Error **errp) > > @@ -314,8 +377,8 @@ static int kvmppc_xive_source_reset(XiveSource *xsrc, Error **errp) > > int i; > > > > /* > > - * Skip the vCPU IPIs. These are created/reset when the vCPUs are > > - * connected in kvmppc_xive_cpu_connect() > > + * Skip the vCPU IPIs. These are created/reset on-demand in > > + * kvmppc_xive_set_source_config(). > > */ > > for (i = SPAPR_XIRQ_BASE; i < xsrc->nr_irqs; i++) { > > int ret; > > @@ -724,7 +787,57 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id) > > } > > > > /* Restore the EAT */ > > - for (i = 0; i < xive->nr_irqs; i++) { > > + > > + /* IPIs are restored from the appropriate vCPU context */ > > + CPU_FOREACH(cs) { > > + /* > > + * The EAT has valid entries to accomodate all possible vCPUs, > > + * but we only want to allocate in KVM the IPIs that were > > + * actually allocated before migration. Let's consider the full > > + * list of IPIs to find an EAS that matches the vCPU id. > > + * > > + * If an IPI appears unmasked in the EAT, it is a proof that the > > + * guest did successfully call H_INT_SET_SOURCE_CONFIG and we > > + * should thus create the IPI at the KVM level if the END index > > + * matches the vCPU id. > > + * > > + * If an IPI appears masked in the EAT, then we don't know exactly > > + * what happened before migration but we don't care. The IPI will > > + * be created when the guest eventually unmasks it with a subsequent > > + * call to H_INT_SET_SOURCE_CONFIG. > > + */ > > + for (i = 0; i < SPAPR_XIRQ_BASE; i++) { > > + XiveEAS *eas = &xive->eat[i]; > > + uint32_t end_idx; > > + uint32_t end_blk; > > + uint8_t priority; > > + uint32_t server; > > + > > + if (!xive_eas_is_valid(eas)) { > > + continue; > > + } > > + > > + if (xive_eas_is_masked(eas)) { > > + continue; > > + } > > + > > + end_idx = xive_get_field64(EAS_END_INDEX, eas->w); > > + end_blk = xive_get_field64(EAS_END_BLOCK, eas->w); > > + spapr_xive_end_to_target(end_blk, end_idx, &server, &priority); > > + if (server != kvm_arch_vcpu_id(cs)) { > > + continue; > > + } > > + > > + ret = kvmppc_xive_set_source_config(xive, i, eas, &local_err); > > + if (ret < 0) { > > + goto fail; > > + } > > + break; > > + } > > + } > > + > > + /* Now restore non-IPIs */ > > + for (i = SPAPR_XIRQ_BASE; i < xive->nr_irqs; i++) { > > if (!xive_source_is_valid(xive, i)) { > > continue; > > } > > > > >
diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c index 66bf4c06fe55..4e5871c3aac2 100644 --- a/hw/intc/spapr_xive_kvm.c +++ b/hw/intc/spapr_xive_kvm.c @@ -30,6 +30,7 @@ */ typedef struct KVMEnabledCPU { unsigned long vcpu_id; + XiveEAS *ipi_eas; QLIST_ENTRY(KVMEnabledCPU) node; } KVMEnabledCPU; @@ -55,6 +56,7 @@ static void kvm_cpu_enable(CPUState *cs) enabled_cpu = g_malloc(sizeof(*enabled_cpu)); enabled_cpu->vcpu_id = vcpu_id; + enabled_cpu->ipi_eas = NULL; QLIST_INSERT_HEAD(&kvm_enabled_cpus, enabled_cpu, node); } @@ -156,26 +158,29 @@ int kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp) */ typedef struct { SpaprXive *xive; + uint32_t lisn; Error *err; int rc; } XiveInitIPI; static void kvmppc_xive_reset_ipi_on_cpu(CPUState *cs, run_on_cpu_data arg) { - unsigned long ipi = kvm_arch_vcpu_id(cs); XiveInitIPI *s = arg.host_ptr; + unsigned long ipi = s->lisn; uint64_t state = 0; s->rc = kvm_device_access(s->xive->fd, KVM_DEV_XIVE_GRP_SOURCE, ipi, &state, true, &s->err); } -static int kvmppc_xive_reset_ipi(SpaprXive *xive, CPUState *cs, Error **errp) +static int kvmppc_xive_reset_ipi(SpaprXive *xive, CPUState *cs, uint32_t lisn, + Error **errp) { XiveInitIPI s = { .xive = xive, .err = NULL, .rc = 0, + .lisn = lisn, }; run_on_cpu(cs, kvmppc_xive_reset_ipi_on_cpu, RUN_ON_CPU_HOST_PTR(&s)); @@ -214,12 +219,6 @@ 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; } @@ -228,6 +227,62 @@ int kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp) * XIVE Interrupt Source (KVM) */ +static bool spapr_xive_is_ipi(uint32_t lisn) +{ + return lisn < SPAPR_XIRQ_BASE; +} + +static bool kvm_ipi_is_enabled(SpaprXive *xive, uint32_t lisn) +{ + KVMEnabledCPU *enabled_cpu; + + g_assert(spapr_xive_is_ipi(lisn)); + + QLIST_FOREACH(enabled_cpu, &kvm_enabled_cpus, node) { + if (enabled_cpu->ipi_eas == &xive->eat[lisn]) { + return true; + } + } + return false; +} + +static int kvm_ipi_enable(SpaprXive *xive, uint32_t lisn, uint32_t vcpu_id, + Error **errp) +{ + KVMEnabledCPU *enabled_cpu; + + g_assert(spapr_xive_is_ipi(lisn)); + + QLIST_FOREACH(enabled_cpu, &kvm_enabled_cpus, node) { + if (enabled_cpu->vcpu_id == vcpu_id) { + CPUState *cs = CPU(spapr_find_cpu(vcpu_id)); + XiveEAS *eas = &xive->eat[lisn]; + + /* No change ? */ + if (enabled_cpu->ipi_eas && enabled_cpu->ipi_eas == eas) { + return 0; + } + + /* XXX: abort ? */ + if (!cs) { + break; + } + + /* Create/reset the vCPU IPI */ + int ret = kvmppc_xive_reset_ipi(xive, cs, lisn, errp); + if (ret < 0) { + return ret; + } + + enabled_cpu->ipi_eas = eas; + return 0; + } + } + + error_setg(errp, "vCPU #%d not found", vcpu_id); + return -ESRCH; +} + int kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas, Error **errp) { @@ -248,6 +303,14 @@ int kvmppc_xive_set_source_config(SpaprXive *xive, uint32_t lisn, XiveEAS *eas, spapr_xive_end_to_target(end_blk, end_idx, &server, &priority); + if (spapr_xive_is_ipi(lisn)) { + /* Create the vCPU IPI */ + int ret = kvm_ipi_enable(xive, lisn, server, errp); + if (ret < 0) { + return ret; + } + } + kvm_src = priority << KVM_XIVE_SOURCE_PRIORITY_SHIFT & KVM_XIVE_SOURCE_PRIORITY_MASK; kvm_src |= server << KVM_XIVE_SOURCE_SERVER_SHIFT & @@ -280,7 +343,7 @@ 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() + * The vCPU IPIs are now allocated in kvmppc_xive_set_source_config() * and not with all sources in kvmppc_xive_source_reset() */ assert(srcno >= SPAPR_XIRQ_BASE); @@ -300,12 +363,12 @@ int kvmppc_xive_source_reset_one(XiveSource *xsrc, int srcno, Error **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(). + * kvmppc_xive_set_source_config(). */ 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)); + (!spapr_xive_is_ipi(i) || kvm_ipi_is_enabled(xive, i)); } static int kvmppc_xive_source_reset(XiveSource *xsrc, Error **errp) @@ -314,8 +377,8 @@ static int kvmppc_xive_source_reset(XiveSource *xsrc, Error **errp) int i; /* - * Skip the vCPU IPIs. These are created/reset when the vCPUs are - * connected in kvmppc_xive_cpu_connect() + * Skip the vCPU IPIs. These are created/reset on-demand in + * kvmppc_xive_set_source_config(). */ for (i = SPAPR_XIRQ_BASE; i < xsrc->nr_irqs; i++) { int ret; @@ -724,7 +787,57 @@ int kvmppc_xive_post_load(SpaprXive *xive, int version_id) } /* Restore the EAT */ - for (i = 0; i < xive->nr_irqs; i++) { + + /* IPIs are restored from the appropriate vCPU context */ + CPU_FOREACH(cs) { + /* + * The EAT has valid entries to accomodate all possible vCPUs, + * but we only want to allocate in KVM the IPIs that were + * actually allocated before migration. Let's consider the full + * list of IPIs to find an EAS that matches the vCPU id. + * + * If an IPI appears unmasked in the EAT, it is a proof that the + * guest did successfully call H_INT_SET_SOURCE_CONFIG and we + * should thus create the IPI at the KVM level if the END index + * matches the vCPU id. + * + * If an IPI appears masked in the EAT, then we don't know exactly + * what happened before migration but we don't care. The IPI will + * be created when the guest eventually unmasks it with a subsequent + * call to H_INT_SET_SOURCE_CONFIG. + */ + for (i = 0; i < SPAPR_XIRQ_BASE; i++) { + XiveEAS *eas = &xive->eat[i]; + uint32_t end_idx; + uint32_t end_blk; + uint8_t priority; + uint32_t server; + + if (!xive_eas_is_valid(eas)) { + continue; + } + + if (xive_eas_is_masked(eas)) { + continue; + } + + end_idx = xive_get_field64(EAS_END_INDEX, eas->w); + end_blk = xive_get_field64(EAS_END_BLOCK, eas->w); + spapr_xive_end_to_target(end_blk, end_idx, &server, &priority); + if (server != kvm_arch_vcpu_id(cs)) { + continue; + } + + ret = kvmppc_xive_set_source_config(xive, i, eas, &local_err); + if (ret < 0) { + goto fail; + } + break; + } + } + + /* Now restore non-IPIs */ + for (i = SPAPR_XIRQ_BASE; i < xive->nr_irqs; i++) { if (!xive_source_is_valid(xive, i)) { continue; }
Recent commit acbdb9956fe9 introduced a dedicated path to create IPIs in KVM. This is done from under kvmppc_xive_cpu_connect() with the assumption that the IPI number is equal to the vCPU id. The latter is wrong: the guest chooses an arbitrary LISN from the "ibm,xive-lisn-ranges" and assigns it to a target vCPU with the H_INT_SET_SOURCE_CONFIG hcall. This went unnoticed so far because IPI numbers and vCPU ids happen to match by default. This is no longer the case though when setting the VSMT machine property to a value that is different from (ie. bigger than) the number of vCPUs per core (ie. -smp threads). Wrong IPIs end up being created in KVM but the guest still uses the ones it has allocated and gets very confused (see BugLink below). Fix this by creating the IPI at the only place where we have its appropriate number : when the guest allocates it with the H_INT_SET_SOURCE_CONFIG hcall. We detect this is an IPI because it is < SPAPR_XIRQ_BASE and we get the vCPU id from the hcall arguments. The EAS of the IPI is tracked in the kvm_enabled_cpus list. It is now used instead of vcpu_id to filter unallocated IPIs out in xive_source_is_valid(). It also allows to only reset the IPI on the first call to H_INT_SET_SOURCE_CONFIG. Restore unmasked IPIs from the vCPU contexts in kvmppc_xive_post_load(). Masked ones will be created when the guests eventually unmask them with H_INT_SET_SOURCE_CONFIG. Reported-by: Satheesh Rajendran <sathnaga@linux.vnet.ibm.com> Fixes: acbdb9956fe9 ("spapr/xive: Allocate IPIs independently from the other sources") BugLink: https://bugs.launchpad.net/qemu/+bug/1900241 Cc: clg@kaod.org Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/intc/spapr_xive_kvm.c | 141 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 127 insertions(+), 14 deletions(-)