| Message ID | 159664893734.638781.14358602267403682394.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: Cleanups for XIVE and PHB | expand |
On Wed, Aug 05, 2020 at 07:35:37PM +0200, Greg Kurz wrote: > Calls to the KVM XIVE device are guarded by kvm_irqchip_in_kernel(). This > ensures that QEMU won't try to use the device if KVM is disabled or if > an in-kernel irqchip isn't required. > > When using ic-mode=dual with the pseries machine, we have two possible > interrupt controllers: XIVE and XICS. The kvm_irqchip_in_kernel() helper > will return true as soon as any of the KVM device is created. It might > lure QEMU to think that the other one is also around, while it is not. > This is exactly what happens with ic-mode=dual at machine init when > claiming IRQ numbers, which must be done on all possible IRQ backends, > eg. RTAS event sources or the PHB0 LSI table : only the KVM XICS device > is active but we end up calling kvmppc_xive_source_reset_one() anyway, > which fails. This doesn't cause any trouble because of another bug : > kvmppc_xive_source_reset_one() lacks an error_setg() and callers don't > see the failure. > > Most of the other kvmppc_xive_* functions have similar xive->fd > checks to filter out the case when KVM XIVE isn't active. It > might look safer to have idempotent functions but it doesn't > really help to understand what's going on when debugging. > > Since we already have all the kvm_irqchip_in_kernel() in place, > also have the callers to check xive->fd as well before calling > KVM XIVE specific code. Introduce helpers to access the underlying > fd for various XIVE types and call them with a kvm_irqchip_in_kernel() > guard : this allows the compiler to optimize the kvmppc_xive_* calls > out if CONFIG_KVM isn't defined, thus avoiding the need for stubs. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/intc/spapr_xive.c | 39 +++++++++++++++++++++++++-------------- > hw/intc/spapr_xive_kvm.c | 15 +++++++++++++++ > hw/intc/xive.c | 30 +++++++++++++++++++++++------- > include/hw/ppc/spapr_xive.h | 1 + > include/hw/ppc/xive.h | 2 ++ > 5 files changed, 66 insertions(+), 21 deletions(-) > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > index 89c8cd96670b..98e6489fb16d 100644 > --- a/hw/intc/spapr_xive.c > +++ b/hw/intc/spapr_xive.c > @@ -148,12 +148,19 @@ static void spapr_xive_end_pic_print_info(SpaprXive *xive, XiveEND *end, > xive_end_queue_pic_print_info(end, 6, mon); > } > > +/* > + * kvm_irqchip_in_kernel() will cause the compiler to turn this > + * info a nop if CONFIG_KVM isn't defined. > + */ > +#define kvmppc_xive_in_kernel(xive) \ > + (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip(xive)) This all seems like a good idea, my only concern is that the semantic difference between kvmppc_xive_in_kernel() and kvmppc_xive_kernel_irqchip() is pretty subtle and non-obvious (and likewise for the tctx and xsrc variations). We're right in the XIVE implementation, so I suggest you eliminate the kvmppc_xive_kernel_irqchip() etc. wrappers and just open code a check on the fd in kvmppc_xive_in_kernel() etc. > + > void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon) > { > XiveSource *xsrc = &xive->source; > int i; > > - if (kvm_irqchip_in_kernel()) { > + if (kvmppc_xive_in_kernel(xive)) { > Error *local_err = NULL; > > kvmppc_xive_synchronize_state(xive, &local_err); > @@ -507,8 +514,10 @@ static const VMStateDescription vmstate_spapr_xive_eas = { > > static int vmstate_spapr_xive_pre_save(void *opaque) > { > - if (kvm_irqchip_in_kernel()) { > - return kvmppc_xive_pre_save(SPAPR_XIVE(opaque)); > + SpaprXive *xive = SPAPR_XIVE(opaque); > + > + if (kvmppc_xive_in_kernel(xive)) { > + return kvmppc_xive_pre_save(xive); > } > > return 0; > @@ -520,8 +529,10 @@ static int vmstate_spapr_xive_pre_save(void *opaque) > */ > static int spapr_xive_post_load(SpaprInterruptController *intc, int version_id) > { > - if (kvm_irqchip_in_kernel()) { > - return kvmppc_xive_post_load(SPAPR_XIVE(intc), version_id); > + SpaprXive *xive = SPAPR_XIVE(intc); > + > + if (kvmppc_xive_in_kernel(xive)) { > + return kvmppc_xive_post_load(xive, version_id); > } > > return 0; > @@ -564,7 +575,7 @@ static int spapr_xive_claim_irq(SpaprInterruptController *intc, int lisn, > xive_source_irq_set_lsi(xsrc, lisn); > } > > - if (kvm_irqchip_in_kernel()) { > + if (kvmppc_xive_in_kernel(xive)) { > return kvmppc_xive_source_reset_one(xsrc, lisn, errp); > } > > @@ -641,7 +652,7 @@ static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val) > { > SpaprXive *xive = SPAPR_XIVE(intc); > > - if (kvm_irqchip_in_kernel()) { > + if (kvmppc_xive_in_kernel(xive)) { > kvmppc_xive_source_set_irq(&xive->source, irq, val); > } else { > xive_source_set_irq(&xive->source, irq, val); > @@ -749,7 +760,7 @@ static void spapr_xive_deactivate(SpaprInterruptController *intc) > > spapr_xive_mmio_set_enabled(xive, false); > > - if (kvm_irqchip_in_kernel()) { > + if (kvmppc_xive_in_kernel(xive)) { > kvmppc_xive_disconnect(intc); > } > } > @@ -1058,7 +1069,7 @@ static target_ulong h_int_set_source_config(PowerPCCPU *cpu, > new_eas.w = xive_set_field64(EAS_END_DATA, new_eas.w, eisn); > } > > - if (kvm_irqchip_in_kernel()) { > + if (kvmppc_xive_in_kernel(xive)) { > Error *local_err = NULL; > > kvmppc_xive_set_source_config(xive, lisn, &new_eas, &local_err); > @@ -1379,7 +1390,7 @@ static target_ulong h_int_set_queue_config(PowerPCCPU *cpu, > */ > > out: > - if (kvm_irqchip_in_kernel()) { > + if (kvmppc_xive_in_kernel(xive)) { > Error *local_err = NULL; > > kvmppc_xive_set_queue_config(xive, end_blk, end_idx, &end, &local_err); > @@ -1480,7 +1491,7 @@ static target_ulong h_int_get_queue_config(PowerPCCPU *cpu, > args[2] = 0; > } > > - if (kvm_irqchip_in_kernel()) { > + if (kvmppc_xive_in_kernel(xive)) { > Error *local_err = NULL; > > kvmppc_xive_get_queue_config(xive, end_blk, end_idx, end, &local_err); > @@ -1642,7 +1653,7 @@ static target_ulong h_int_esb(PowerPCCPU *cpu, > return H_P3; > } > > - if (kvm_irqchip_in_kernel()) { > + if (kvmppc_xive_in_kernel(xive)) { > args[0] = kvmppc_xive_esb_rw(xsrc, lisn, offset, data, > flags & SPAPR_XIVE_ESB_STORE); > } else { > @@ -1717,7 +1728,7 @@ static target_ulong h_int_sync(PowerPCCPU *cpu, > * under KVM > */ > > - if (kvm_irqchip_in_kernel()) { > + if (kvmppc_xive_in_kernel(xive)) { > Error *local_err = NULL; > > kvmppc_xive_sync_source(xive, lisn, &local_err); > @@ -1761,7 +1772,7 @@ static target_ulong h_int_reset(PowerPCCPU *cpu, > > device_legacy_reset(DEVICE(xive)); > > - if (kvm_irqchip_in_kernel()) { > + if (kvmppc_xive_in_kernel(xive)) { > Error *local_err = NULL; > > kvmppc_xive_reset(xive, &local_err); > diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c > index 893a1ee77e70..a9657e2b0cda 100644 > --- a/hw/intc/spapr_xive_kvm.c > +++ b/hw/intc/spapr_xive_kvm.c > @@ -889,3 +889,18 @@ void kvmppc_xive_disconnect(SpaprInterruptController *intc) > xive->change = NULL; > } > } > + > +bool kvmppc_xive_kernel_irqchip(SpaprXive *xive) > +{ > + return xive->fd != -1; > +} > + > +bool kvmppc_xive_kernel_irqchip_tctx(XiveTCTX *tctx) > +{ > + return kvmppc_xive_kernel_irqchip(SPAPR_XIVE(tctx->xptr)); > +} > + > +bool kvmppc_xive_kernel_irqchip_xsrc(XiveSource *xsrc) > +{ > + return kvmppc_xive_kernel_irqchip(SPAPR_XIVE(xsrc->xive)); > +} > diff --git a/hw/intc/xive.c b/hw/intc/xive.c > index 9b55e0356c62..2e1af4530dc5 100644 > --- a/hw/intc/xive.c > +++ b/hw/intc/xive.c > @@ -592,6 +592,13 @@ static const char * const xive_tctx_ring_names[] = { > "USER", "OS", "POOL", "PHYS", > }; > > +/* > + * kvm_irqchip_in_kernel() will cause the compiler to turn this > + * info a nop if CONFIG_KVM isn't defined. > + */ > +#define kvmppc_xive_in_kernel_tctx(tctx) \ > + (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip_tctx(tctx)) > + > void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon) > { > int cpu_index; > @@ -606,7 +613,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon) > > cpu_index = tctx->cs ? tctx->cs->cpu_index : -1; > > - if (kvm_irqchip_in_kernel()) { > + if (kvmppc_xive_in_kernel_tctx(tctx)) { > Error *local_err = NULL; > > kvmppc_xive_cpu_synchronize_state(tctx, &local_err); > @@ -671,7 +678,7 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp) > } > > /* Connect the presenter to the VCPU (required for CPU hotplug) */ > - if (kvm_irqchip_in_kernel()) { > + if (kvmppc_xive_in_kernel_tctx(tctx)) { > kvmppc_xive_cpu_connect(tctx, &local_err); > if (local_err) { > error_propagate(errp, local_err); > @@ -682,10 +689,11 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp) > > static int vmstate_xive_tctx_pre_save(void *opaque) > { > + XiveTCTX *tctx = XIVE_TCTX(opaque); > Error *local_err = NULL; > > - if (kvm_irqchip_in_kernel()) { > - kvmppc_xive_cpu_get_state(XIVE_TCTX(opaque), &local_err); > + if (kvmppc_xive_in_kernel_tctx(tctx)) { > + kvmppc_xive_cpu_get_state(tctx, &local_err); > if (local_err) { > error_report_err(local_err); > return -1; > @@ -697,14 +705,15 @@ static int vmstate_xive_tctx_pre_save(void *opaque) > > static int vmstate_xive_tctx_post_load(void *opaque, int version_id) > { > + XiveTCTX *tctx = XIVE_TCTX(opaque); > Error *local_err = NULL; > > - if (kvm_irqchip_in_kernel()) { > + if (kvmppc_xive_in_kernel_tctx(tctx)) { > /* > * Required for hotplugged CPU, for which the state comes > * after all states of the machine. > */ > - kvmppc_xive_cpu_set_state(XIVE_TCTX(opaque), &local_err); > + kvmppc_xive_cpu_set_state(tctx, &local_err); > if (local_err) { > error_report_err(local_err); > return -1; > @@ -1125,6 +1134,13 @@ static void xive_source_reset(void *dev) > memset(xsrc->status, XIVE_ESB_OFF, xsrc->nr_irqs); > } > > +/* > + * kvm_irqchip_in_kernel() will cause the compiler to turn this > + * info a nop if CONFIG_KVM isn't defined. > + */ > +#define kvmppc_xive_in_kernel_xsrc(xsrc) \ > + (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip_xsrc(xsrc)) > + > static void xive_source_realize(DeviceState *dev, Error **errp) > { > XiveSource *xsrc = XIVE_SOURCE(dev); > @@ -1147,7 +1163,7 @@ static void xive_source_realize(DeviceState *dev, Error **errp) > xsrc->status = g_malloc0(xsrc->nr_irqs); > xsrc->lsi_map = bitmap_new(xsrc->nr_irqs); > > - if (!kvm_irqchip_in_kernel()) { > + if (!kvmppc_xive_in_kernel_xsrc(xsrc)) { > memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc), > &xive_source_esb_ops, xsrc, "xive.esb", > (1ull << xsrc->esb_shift) * xsrc->nr_irqs); > diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h > index 93d09d68deb7..9fd00378ac1f 100644 > --- a/include/hw/ppc/spapr_xive.h > +++ b/include/hw/ppc/spapr_xive.h > @@ -94,5 +94,6 @@ void kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk, > void kvmppc_xive_synchronize_state(SpaprXive *xive, Error **errp); > int kvmppc_xive_pre_save(SpaprXive *xive); > int kvmppc_xive_post_load(SpaprXive *xive, int version_id); > +bool kvmppc_xive_kernel_irqchip(SpaprXive *xive); > > #endif /* PPC_SPAPR_XIVE_H */ > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h > index 705cf48176fc..ea66e9ea9c7b 100644 > --- a/include/hw/ppc/xive.h > +++ b/include/hw/ppc/xive.h > @@ -484,5 +484,7 @@ void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp); > void kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp); > void kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp); > void kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp); > +bool kvmppc_xive_kernel_irqchip_tctx(XiveTCTX *tctx); > +bool kvmppc_xive_kernel_irqchip_xsrc(XiveSource *xsrc); > > #endif /* PPC_XIVE_H */ > >
On Thu, 6 Aug 2020 15:08:35 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Wed, Aug 05, 2020 at 07:35:37PM +0200, Greg Kurz wrote: > > Calls to the KVM XIVE device are guarded by kvm_irqchip_in_kernel(). This > > ensures that QEMU won't try to use the device if KVM is disabled or if > > an in-kernel irqchip isn't required. > > > > When using ic-mode=dual with the pseries machine, we have two possible > > interrupt controllers: XIVE and XICS. The kvm_irqchip_in_kernel() helper > > will return true as soon as any of the KVM device is created. It might > > lure QEMU to think that the other one is also around, while it is not. > > This is exactly what happens with ic-mode=dual at machine init when > > claiming IRQ numbers, which must be done on all possible IRQ backends, > > eg. RTAS event sources or the PHB0 LSI table : only the KVM XICS device > > is active but we end up calling kvmppc_xive_source_reset_one() anyway, > > which fails. This doesn't cause any trouble because of another bug : > > kvmppc_xive_source_reset_one() lacks an error_setg() and callers don't > > see the failure. > > > > Most of the other kvmppc_xive_* functions have similar xive->fd > > checks to filter out the case when KVM XIVE isn't active. It > > might look safer to have idempotent functions but it doesn't > > really help to understand what's going on when debugging. > > > > Since we already have all the kvm_irqchip_in_kernel() in place, > > also have the callers to check xive->fd as well before calling > > KVM XIVE specific code. Introduce helpers to access the underlying > > fd for various XIVE types and call them with a kvm_irqchip_in_kernel() > > guard : this allows the compiler to optimize the kvmppc_xive_* calls > > out if CONFIG_KVM isn't defined, thus avoiding the need for stubs. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/intc/spapr_xive.c | 39 +++++++++++++++++++++++++-------------- > > hw/intc/spapr_xive_kvm.c | 15 +++++++++++++++ > > hw/intc/xive.c | 30 +++++++++++++++++++++++------- > > include/hw/ppc/spapr_xive.h | 1 + > > include/hw/ppc/xive.h | 2 ++ > > 5 files changed, 66 insertions(+), 21 deletions(-) > > > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > > index 89c8cd96670b..98e6489fb16d 100644 > > --- a/hw/intc/spapr_xive.c > > +++ b/hw/intc/spapr_xive.c > > @@ -148,12 +148,19 @@ static void spapr_xive_end_pic_print_info(SpaprXive *xive, XiveEND *end, > > xive_end_queue_pic_print_info(end, 6, mon); > > } > > > > +/* > > + * kvm_irqchip_in_kernel() will cause the compiler to turn this > > + * info a nop if CONFIG_KVM isn't defined. > > + */ > > +#define kvmppc_xive_in_kernel(xive) \ > > + (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip(xive)) > > This all seems like a good idea, my only concern is that the semantic > difference between kvmppc_xive_in_kernel() and > kvmppc_xive_kernel_irqchip() is pretty subtle and non-obvious (and > likewise for the tctx and xsrc variations). > Yeah, kvmppc_xive_in_kernel() and friends are essentially versions that also work when CONFIG_KVM is not set (needed to build qemu-system-ppc64 for non-POWER hosts). > We're right in the XIVE implementation, so I suggest you eliminate the > kvmppc_xive_kernel_irqchip() etc. wrappers and just open code a check > on the fd in kvmppc_xive_in_kernel() etc. > This is true for the kvmppc_xive_in_kernel() variant that takes a SpaprXive * arg. Things are different for the tctx and xsrc variants, because xive.c is platform agnostic (shared between spapr and powernv). Exposing the SpaprXive type to xive.c would look like a layering violation to me. Maybe introducing abstract class methods at the base XIVE level (eg. XivePresenterClass) and implement them in platform-specific code would make things clearer ? > > + > > void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon) > > { > > XiveSource *xsrc = &xive->source; > > int i; > > > > - if (kvm_irqchip_in_kernel()) { > > + if (kvmppc_xive_in_kernel(xive)) { > > Error *local_err = NULL; > > > > kvmppc_xive_synchronize_state(xive, &local_err); > > @@ -507,8 +514,10 @@ static const VMStateDescription vmstate_spapr_xive_eas = { > > > > static int vmstate_spapr_xive_pre_save(void *opaque) > > { > > - if (kvm_irqchip_in_kernel()) { > > - return kvmppc_xive_pre_save(SPAPR_XIVE(opaque)); > > + SpaprXive *xive = SPAPR_XIVE(opaque); > > + > > + if (kvmppc_xive_in_kernel(xive)) { > > + return kvmppc_xive_pre_save(xive); > > } > > > > return 0; > > @@ -520,8 +529,10 @@ static int vmstate_spapr_xive_pre_save(void *opaque) > > */ > > static int spapr_xive_post_load(SpaprInterruptController *intc, int version_id) > > { > > - if (kvm_irqchip_in_kernel()) { > > - return kvmppc_xive_post_load(SPAPR_XIVE(intc), version_id); > > + SpaprXive *xive = SPAPR_XIVE(intc); > > + > > + if (kvmppc_xive_in_kernel(xive)) { > > + return kvmppc_xive_post_load(xive, version_id); > > } > > > > return 0; > > @@ -564,7 +575,7 @@ static int spapr_xive_claim_irq(SpaprInterruptController *intc, int lisn, > > xive_source_irq_set_lsi(xsrc, lisn); > > } > > > > - if (kvm_irqchip_in_kernel()) { > > + if (kvmppc_xive_in_kernel(xive)) { > > return kvmppc_xive_source_reset_one(xsrc, lisn, errp); > > } > > > > @@ -641,7 +652,7 @@ static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val) > > { > > SpaprXive *xive = SPAPR_XIVE(intc); > > > > - if (kvm_irqchip_in_kernel()) { > > + if (kvmppc_xive_in_kernel(xive)) { > > kvmppc_xive_source_set_irq(&xive->source, irq, val); > > } else { > > xive_source_set_irq(&xive->source, irq, val); > > @@ -749,7 +760,7 @@ static void spapr_xive_deactivate(SpaprInterruptController *intc) > > > > spapr_xive_mmio_set_enabled(xive, false); > > > > - if (kvm_irqchip_in_kernel()) { > > + if (kvmppc_xive_in_kernel(xive)) { > > kvmppc_xive_disconnect(intc); > > } > > } > > @@ -1058,7 +1069,7 @@ static target_ulong h_int_set_source_config(PowerPCCPU *cpu, > > new_eas.w = xive_set_field64(EAS_END_DATA, new_eas.w, eisn); > > } > > > > - if (kvm_irqchip_in_kernel()) { > > + if (kvmppc_xive_in_kernel(xive)) { > > Error *local_err = NULL; > > > > kvmppc_xive_set_source_config(xive, lisn, &new_eas, &local_err); > > @@ -1379,7 +1390,7 @@ static target_ulong h_int_set_queue_config(PowerPCCPU *cpu, > > */ > > > > out: > > - if (kvm_irqchip_in_kernel()) { > > + if (kvmppc_xive_in_kernel(xive)) { > > Error *local_err = NULL; > > > > kvmppc_xive_set_queue_config(xive, end_blk, end_idx, &end, &local_err); > > @@ -1480,7 +1491,7 @@ static target_ulong h_int_get_queue_config(PowerPCCPU *cpu, > > args[2] = 0; > > } > > > > - if (kvm_irqchip_in_kernel()) { > > + if (kvmppc_xive_in_kernel(xive)) { > > Error *local_err = NULL; > > > > kvmppc_xive_get_queue_config(xive, end_blk, end_idx, end, &local_err); > > @@ -1642,7 +1653,7 @@ static target_ulong h_int_esb(PowerPCCPU *cpu, > > return H_P3; > > } > > > > - if (kvm_irqchip_in_kernel()) { > > + if (kvmppc_xive_in_kernel(xive)) { > > args[0] = kvmppc_xive_esb_rw(xsrc, lisn, offset, data, > > flags & SPAPR_XIVE_ESB_STORE); > > } else { > > @@ -1717,7 +1728,7 @@ static target_ulong h_int_sync(PowerPCCPU *cpu, > > * under KVM > > */ > > > > - if (kvm_irqchip_in_kernel()) { > > + if (kvmppc_xive_in_kernel(xive)) { > > Error *local_err = NULL; > > > > kvmppc_xive_sync_source(xive, lisn, &local_err); > > @@ -1761,7 +1772,7 @@ static target_ulong h_int_reset(PowerPCCPU *cpu, > > > > device_legacy_reset(DEVICE(xive)); > > > > - if (kvm_irqchip_in_kernel()) { > > + if (kvmppc_xive_in_kernel(xive)) { > > Error *local_err = NULL; > > > > kvmppc_xive_reset(xive, &local_err); > > diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c > > index 893a1ee77e70..a9657e2b0cda 100644 > > --- a/hw/intc/spapr_xive_kvm.c > > +++ b/hw/intc/spapr_xive_kvm.c > > @@ -889,3 +889,18 @@ void kvmppc_xive_disconnect(SpaprInterruptController *intc) > > xive->change = NULL; > > } > > } > > + > > +bool kvmppc_xive_kernel_irqchip(SpaprXive *xive) > > +{ > > + return xive->fd != -1; > > +} > > + > > +bool kvmppc_xive_kernel_irqchip_tctx(XiveTCTX *tctx) > > +{ > > + return kvmppc_xive_kernel_irqchip(SPAPR_XIVE(tctx->xptr)); > > +} > > + > > +bool kvmppc_xive_kernel_irqchip_xsrc(XiveSource *xsrc) > > +{ > > + return kvmppc_xive_kernel_irqchip(SPAPR_XIVE(xsrc->xive)); > > +} > > diff --git a/hw/intc/xive.c b/hw/intc/xive.c > > index 9b55e0356c62..2e1af4530dc5 100644 > > --- a/hw/intc/xive.c > > +++ b/hw/intc/xive.c > > @@ -592,6 +592,13 @@ static const char * const xive_tctx_ring_names[] = { > > "USER", "OS", "POOL", "PHYS", > > }; > > > > +/* > > + * kvm_irqchip_in_kernel() will cause the compiler to turn this > > + * info a nop if CONFIG_KVM isn't defined. > > + */ > > +#define kvmppc_xive_in_kernel_tctx(tctx) \ > > + (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip_tctx(tctx)) > > + > > void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon) > > { > > int cpu_index; > > @@ -606,7 +613,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon) > > > > cpu_index = tctx->cs ? tctx->cs->cpu_index : -1; > > > > - if (kvm_irqchip_in_kernel()) { > > + if (kvmppc_xive_in_kernel_tctx(tctx)) { > > Error *local_err = NULL; > > > > kvmppc_xive_cpu_synchronize_state(tctx, &local_err); > > @@ -671,7 +678,7 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp) > > } > > > > /* Connect the presenter to the VCPU (required for CPU hotplug) */ > > - if (kvm_irqchip_in_kernel()) { > > + if (kvmppc_xive_in_kernel_tctx(tctx)) { > > kvmppc_xive_cpu_connect(tctx, &local_err); > > if (local_err) { > > error_propagate(errp, local_err); > > @@ -682,10 +689,11 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp) > > > > static int vmstate_xive_tctx_pre_save(void *opaque) > > { > > + XiveTCTX *tctx = XIVE_TCTX(opaque); > > Error *local_err = NULL; > > > > - if (kvm_irqchip_in_kernel()) { > > - kvmppc_xive_cpu_get_state(XIVE_TCTX(opaque), &local_err); > > + if (kvmppc_xive_in_kernel_tctx(tctx)) { > > + kvmppc_xive_cpu_get_state(tctx, &local_err); > > if (local_err) { > > error_report_err(local_err); > > return -1; > > @@ -697,14 +705,15 @@ static int vmstate_xive_tctx_pre_save(void *opaque) > > > > static int vmstate_xive_tctx_post_load(void *opaque, int version_id) > > { > > + XiveTCTX *tctx = XIVE_TCTX(opaque); > > Error *local_err = NULL; > > > > - if (kvm_irqchip_in_kernel()) { > > + if (kvmppc_xive_in_kernel_tctx(tctx)) { > > /* > > * Required for hotplugged CPU, for which the state comes > > * after all states of the machine. > > */ > > - kvmppc_xive_cpu_set_state(XIVE_TCTX(opaque), &local_err); > > + kvmppc_xive_cpu_set_state(tctx, &local_err); > > if (local_err) { > > error_report_err(local_err); > > return -1; > > @@ -1125,6 +1134,13 @@ static void xive_source_reset(void *dev) > > memset(xsrc->status, XIVE_ESB_OFF, xsrc->nr_irqs); > > } > > > > +/* > > + * kvm_irqchip_in_kernel() will cause the compiler to turn this > > + * info a nop if CONFIG_KVM isn't defined. > > + */ > > +#define kvmppc_xive_in_kernel_xsrc(xsrc) \ > > + (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip_xsrc(xsrc)) > > + > > static void xive_source_realize(DeviceState *dev, Error **errp) > > { > > XiveSource *xsrc = XIVE_SOURCE(dev); > > @@ -1147,7 +1163,7 @@ static void xive_source_realize(DeviceState *dev, Error **errp) > > xsrc->status = g_malloc0(xsrc->nr_irqs); > > xsrc->lsi_map = bitmap_new(xsrc->nr_irqs); > > > > - if (!kvm_irqchip_in_kernel()) { > > + if (!kvmppc_xive_in_kernel_xsrc(xsrc)) { > > memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc), > > &xive_source_esb_ops, xsrc, "xive.esb", > > (1ull << xsrc->esb_shift) * xsrc->nr_irqs); > > diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h > > index 93d09d68deb7..9fd00378ac1f 100644 > > --- a/include/hw/ppc/spapr_xive.h > > +++ b/include/hw/ppc/spapr_xive.h > > @@ -94,5 +94,6 @@ void kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk, > > void kvmppc_xive_synchronize_state(SpaprXive *xive, Error **errp); > > int kvmppc_xive_pre_save(SpaprXive *xive); > > int kvmppc_xive_post_load(SpaprXive *xive, int version_id); > > +bool kvmppc_xive_kernel_irqchip(SpaprXive *xive); > > > > #endif /* PPC_SPAPR_XIVE_H */ > > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h > > index 705cf48176fc..ea66e9ea9c7b 100644 > > --- a/include/hw/ppc/xive.h > > +++ b/include/hw/ppc/xive.h > > @@ -484,5 +484,7 @@ void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp); > > void kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp); > > void kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp); > > void kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp); > > +bool kvmppc_xive_kernel_irqchip_tctx(XiveTCTX *tctx); > > +bool kvmppc_xive_kernel_irqchip_xsrc(XiveSource *xsrc); > > > > #endif /* PPC_XIVE_H */ > > > > >
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c index 89c8cd96670b..98e6489fb16d 100644 --- a/hw/intc/spapr_xive.c +++ b/hw/intc/spapr_xive.c @@ -148,12 +148,19 @@ static void spapr_xive_end_pic_print_info(SpaprXive *xive, XiveEND *end, xive_end_queue_pic_print_info(end, 6, mon); } +/* + * kvm_irqchip_in_kernel() will cause the compiler to turn this + * info a nop if CONFIG_KVM isn't defined. + */ +#define kvmppc_xive_in_kernel(xive) \ + (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip(xive)) + void spapr_xive_pic_print_info(SpaprXive *xive, Monitor *mon) { XiveSource *xsrc = &xive->source; int i; - if (kvm_irqchip_in_kernel()) { + if (kvmppc_xive_in_kernel(xive)) { Error *local_err = NULL; kvmppc_xive_synchronize_state(xive, &local_err); @@ -507,8 +514,10 @@ static const VMStateDescription vmstate_spapr_xive_eas = { static int vmstate_spapr_xive_pre_save(void *opaque) { - if (kvm_irqchip_in_kernel()) { - return kvmppc_xive_pre_save(SPAPR_XIVE(opaque)); + SpaprXive *xive = SPAPR_XIVE(opaque); + + if (kvmppc_xive_in_kernel(xive)) { + return kvmppc_xive_pre_save(xive); } return 0; @@ -520,8 +529,10 @@ static int vmstate_spapr_xive_pre_save(void *opaque) */ static int spapr_xive_post_load(SpaprInterruptController *intc, int version_id) { - if (kvm_irqchip_in_kernel()) { - return kvmppc_xive_post_load(SPAPR_XIVE(intc), version_id); + SpaprXive *xive = SPAPR_XIVE(intc); + + if (kvmppc_xive_in_kernel(xive)) { + return kvmppc_xive_post_load(xive, version_id); } return 0; @@ -564,7 +575,7 @@ static int spapr_xive_claim_irq(SpaprInterruptController *intc, int lisn, xive_source_irq_set_lsi(xsrc, lisn); } - if (kvm_irqchip_in_kernel()) { + if (kvmppc_xive_in_kernel(xive)) { return kvmppc_xive_source_reset_one(xsrc, lisn, errp); } @@ -641,7 +652,7 @@ static void spapr_xive_set_irq(SpaprInterruptController *intc, int irq, int val) { SpaprXive *xive = SPAPR_XIVE(intc); - if (kvm_irqchip_in_kernel()) { + if (kvmppc_xive_in_kernel(xive)) { kvmppc_xive_source_set_irq(&xive->source, irq, val); } else { xive_source_set_irq(&xive->source, irq, val); @@ -749,7 +760,7 @@ static void spapr_xive_deactivate(SpaprInterruptController *intc) spapr_xive_mmio_set_enabled(xive, false); - if (kvm_irqchip_in_kernel()) { + if (kvmppc_xive_in_kernel(xive)) { kvmppc_xive_disconnect(intc); } } @@ -1058,7 +1069,7 @@ static target_ulong h_int_set_source_config(PowerPCCPU *cpu, new_eas.w = xive_set_field64(EAS_END_DATA, new_eas.w, eisn); } - if (kvm_irqchip_in_kernel()) { + if (kvmppc_xive_in_kernel(xive)) { Error *local_err = NULL; kvmppc_xive_set_source_config(xive, lisn, &new_eas, &local_err); @@ -1379,7 +1390,7 @@ static target_ulong h_int_set_queue_config(PowerPCCPU *cpu, */ out: - if (kvm_irqchip_in_kernel()) { + if (kvmppc_xive_in_kernel(xive)) { Error *local_err = NULL; kvmppc_xive_set_queue_config(xive, end_blk, end_idx, &end, &local_err); @@ -1480,7 +1491,7 @@ static target_ulong h_int_get_queue_config(PowerPCCPU *cpu, args[2] = 0; } - if (kvm_irqchip_in_kernel()) { + if (kvmppc_xive_in_kernel(xive)) { Error *local_err = NULL; kvmppc_xive_get_queue_config(xive, end_blk, end_idx, end, &local_err); @@ -1642,7 +1653,7 @@ static target_ulong h_int_esb(PowerPCCPU *cpu, return H_P3; } - if (kvm_irqchip_in_kernel()) { + if (kvmppc_xive_in_kernel(xive)) { args[0] = kvmppc_xive_esb_rw(xsrc, lisn, offset, data, flags & SPAPR_XIVE_ESB_STORE); } else { @@ -1717,7 +1728,7 @@ static target_ulong h_int_sync(PowerPCCPU *cpu, * under KVM */ - if (kvm_irqchip_in_kernel()) { + if (kvmppc_xive_in_kernel(xive)) { Error *local_err = NULL; kvmppc_xive_sync_source(xive, lisn, &local_err); @@ -1761,7 +1772,7 @@ static target_ulong h_int_reset(PowerPCCPU *cpu, device_legacy_reset(DEVICE(xive)); - if (kvm_irqchip_in_kernel()) { + if (kvmppc_xive_in_kernel(xive)) { Error *local_err = NULL; kvmppc_xive_reset(xive, &local_err); diff --git a/hw/intc/spapr_xive_kvm.c b/hw/intc/spapr_xive_kvm.c index 893a1ee77e70..a9657e2b0cda 100644 --- a/hw/intc/spapr_xive_kvm.c +++ b/hw/intc/spapr_xive_kvm.c @@ -889,3 +889,18 @@ void kvmppc_xive_disconnect(SpaprInterruptController *intc) xive->change = NULL; } } + +bool kvmppc_xive_kernel_irqchip(SpaprXive *xive) +{ + return xive->fd != -1; +} + +bool kvmppc_xive_kernel_irqchip_tctx(XiveTCTX *tctx) +{ + return kvmppc_xive_kernel_irqchip(SPAPR_XIVE(tctx->xptr)); +} + +bool kvmppc_xive_kernel_irqchip_xsrc(XiveSource *xsrc) +{ + return kvmppc_xive_kernel_irqchip(SPAPR_XIVE(xsrc->xive)); +} diff --git a/hw/intc/xive.c b/hw/intc/xive.c index 9b55e0356c62..2e1af4530dc5 100644 --- a/hw/intc/xive.c +++ b/hw/intc/xive.c @@ -592,6 +592,13 @@ static const char * const xive_tctx_ring_names[] = { "USER", "OS", "POOL", "PHYS", }; +/* + * kvm_irqchip_in_kernel() will cause the compiler to turn this + * info a nop if CONFIG_KVM isn't defined. + */ +#define kvmppc_xive_in_kernel_tctx(tctx) \ + (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip_tctx(tctx)) + void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon) { int cpu_index; @@ -606,7 +613,7 @@ void xive_tctx_pic_print_info(XiveTCTX *tctx, Monitor *mon) cpu_index = tctx->cs ? tctx->cs->cpu_index : -1; - if (kvm_irqchip_in_kernel()) { + if (kvmppc_xive_in_kernel_tctx(tctx)) { Error *local_err = NULL; kvmppc_xive_cpu_synchronize_state(tctx, &local_err); @@ -671,7 +678,7 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp) } /* Connect the presenter to the VCPU (required for CPU hotplug) */ - if (kvm_irqchip_in_kernel()) { + if (kvmppc_xive_in_kernel_tctx(tctx)) { kvmppc_xive_cpu_connect(tctx, &local_err); if (local_err) { error_propagate(errp, local_err); @@ -682,10 +689,11 @@ static void xive_tctx_realize(DeviceState *dev, Error **errp) static int vmstate_xive_tctx_pre_save(void *opaque) { + XiveTCTX *tctx = XIVE_TCTX(opaque); Error *local_err = NULL; - if (kvm_irqchip_in_kernel()) { - kvmppc_xive_cpu_get_state(XIVE_TCTX(opaque), &local_err); + if (kvmppc_xive_in_kernel_tctx(tctx)) { + kvmppc_xive_cpu_get_state(tctx, &local_err); if (local_err) { error_report_err(local_err); return -1; @@ -697,14 +705,15 @@ static int vmstate_xive_tctx_pre_save(void *opaque) static int vmstate_xive_tctx_post_load(void *opaque, int version_id) { + XiveTCTX *tctx = XIVE_TCTX(opaque); Error *local_err = NULL; - if (kvm_irqchip_in_kernel()) { + if (kvmppc_xive_in_kernel_tctx(tctx)) { /* * Required for hotplugged CPU, for which the state comes * after all states of the machine. */ - kvmppc_xive_cpu_set_state(XIVE_TCTX(opaque), &local_err); + kvmppc_xive_cpu_set_state(tctx, &local_err); if (local_err) { error_report_err(local_err); return -1; @@ -1125,6 +1134,13 @@ static void xive_source_reset(void *dev) memset(xsrc->status, XIVE_ESB_OFF, xsrc->nr_irqs); } +/* + * kvm_irqchip_in_kernel() will cause the compiler to turn this + * info a nop if CONFIG_KVM isn't defined. + */ +#define kvmppc_xive_in_kernel_xsrc(xsrc) \ + (kvm_irqchip_in_kernel() && kvmppc_xive_kernel_irqchip_xsrc(xsrc)) + static void xive_source_realize(DeviceState *dev, Error **errp) { XiveSource *xsrc = XIVE_SOURCE(dev); @@ -1147,7 +1163,7 @@ static void xive_source_realize(DeviceState *dev, Error **errp) xsrc->status = g_malloc0(xsrc->nr_irqs); xsrc->lsi_map = bitmap_new(xsrc->nr_irqs); - if (!kvm_irqchip_in_kernel()) { + if (!kvmppc_xive_in_kernel_xsrc(xsrc)) { memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc), &xive_source_esb_ops, xsrc, "xive.esb", (1ull << xsrc->esb_shift) * xsrc->nr_irqs); diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h index 93d09d68deb7..9fd00378ac1f 100644 --- a/include/hw/ppc/spapr_xive.h +++ b/include/hw/ppc/spapr_xive.h @@ -94,5 +94,6 @@ void kvmppc_xive_get_queue_config(SpaprXive *xive, uint8_t end_blk, void kvmppc_xive_synchronize_state(SpaprXive *xive, Error **errp); int kvmppc_xive_pre_save(SpaprXive *xive); int kvmppc_xive_post_load(SpaprXive *xive, int version_id); +bool kvmppc_xive_kernel_irqchip(SpaprXive *xive); #endif /* PPC_SPAPR_XIVE_H */ diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h index 705cf48176fc..ea66e9ea9c7b 100644 --- a/include/hw/ppc/xive.h +++ b/include/hw/ppc/xive.h @@ -484,5 +484,7 @@ void kvmppc_xive_cpu_connect(XiveTCTX *tctx, Error **errp); void kvmppc_xive_cpu_synchronize_state(XiveTCTX *tctx, Error **errp); void kvmppc_xive_cpu_get_state(XiveTCTX *tctx, Error **errp); void kvmppc_xive_cpu_set_state(XiveTCTX *tctx, Error **errp); +bool kvmppc_xive_kernel_irqchip_tctx(XiveTCTX *tctx); +bool kvmppc_xive_kernel_irqchip_xsrc(XiveSource *xsrc); #endif /* PPC_XIVE_H */
Calls to the KVM XIVE device are guarded by kvm_irqchip_in_kernel(). This ensures that QEMU won't try to use the device if KVM is disabled or if an in-kernel irqchip isn't required. When using ic-mode=dual with the pseries machine, we have two possible interrupt controllers: XIVE and XICS. The kvm_irqchip_in_kernel() helper will return true as soon as any of the KVM device is created. It might lure QEMU to think that the other one is also around, while it is not. This is exactly what happens with ic-mode=dual at machine init when claiming IRQ numbers, which must be done on all possible IRQ backends, eg. RTAS event sources or the PHB0 LSI table : only the KVM XICS device is active but we end up calling kvmppc_xive_source_reset_one() anyway, which fails. This doesn't cause any trouble because of another bug : kvmppc_xive_source_reset_one() lacks an error_setg() and callers don't see the failure. Most of the other kvmppc_xive_* functions have similar xive->fd checks to filter out the case when KVM XIVE isn't active. It might look safer to have idempotent functions but it doesn't really help to understand what's going on when debugging. Since we already have all the kvm_irqchip_in_kernel() in place, also have the callers to check xive->fd as well before calling KVM XIVE specific code. Introduce helpers to access the underlying fd for various XIVE types and call them with a kvm_irqchip_in_kernel() guard : this allows the compiler to optimize the kvmppc_xive_* calls out if CONFIG_KVM isn't defined, thus avoiding the need for stubs. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/intc/spapr_xive.c | 39 +++++++++++++++++++++++++-------------- hw/intc/spapr_xive_kvm.c | 15 +++++++++++++++ hw/intc/xive.c | 30 +++++++++++++++++++++++------- include/hw/ppc/spapr_xive.h | 1 + include/hw/ppc/xive.h | 2 ++ 5 files changed, 66 insertions(+), 21 deletions(-)