| Message ID | 20190321144914.19934-15-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: add KVM support to the XIVE interrupt mode | expand |
On Thu, Mar 21, 2019 at 03:49:13PM +0100, Cédric Le Goater wrote: > Add a check to make sure that the routine initializing the emulated > IRQ device is called once. We don't have much to test on the XICS > side, so we introduce a 'static bool'. Not very elegant but works well > enough. What's the rationale for this? > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/intc/spapr_xive.c | 9 +++++++++ > hw/intc/xics_spapr.c | 10 ++++++++++ > 2 files changed, 19 insertions(+) > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > index f889c89dc2e9..15d41d9cd36d 100644 > --- a/hw/intc/spapr_xive.c > +++ b/hw/intc/spapr_xive.c > @@ -337,6 +337,15 @@ void spapr_xive_init(SpaprXive *xive, Error **errp) > XiveSource *xsrc = &xive->source; > XiveENDSource *end_xsrc = &xive->end_source; > > + /* > + * The emulated XIVE device can only be initialized once. If the > + * ESB memory region has been already mapped, it means we have been > + * through there. > + */ > + if (memory_region_is_mapped(&xsrc->esb_mmio)) { > + return; > + } > + > /* TIMA initialization */ > memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops, xive, > "xive.tima", 4ull << TM_SHIFT); > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c > index 9d2b8adef7c5..67e82f3720b0 100644 > --- a/hw/intc/xics_spapr.c > +++ b/hw/intc/xics_spapr.c > @@ -239,6 +239,16 @@ static void rtas_int_on(PowerPCCPU *cpu, SpaprMachineState *spapr, > > void xics_spapr_init(SpaprMachineState *spapr) > { > + static bool init_done; > + > + /* > + * Emulated mode can only be initialized once. > + */ > + if (init_done) { > + return; > + } > + init_done = true; > + > /* Registration of global state belongs into realize */ > spapr_rtas_register(RTAS_IBM_SET_XIVE, "ibm,set-xive", rtas_set_xive); > spapr_rtas_register(RTAS_IBM_GET_XIVE, "ibm,get-xive", rtas_get_xive);
On 3/22/19 3:15 AM, David Gibson wrote: > On Thu, Mar 21, 2019 at 03:49:13PM +0100, Cédric Le Goater wrote: >> Add a check to make sure that the routine initializing the emulated >> IRQ device is called once. We don't have much to test on the XICS >> side, so we introduce a 'static bool'. Not very elegant but works well >> enough. > > What's the rationale for this? The rationale for initializing the IRQ device only once is that reset will do the initialization when KVM support is added for the dual interrupt mode. When using the emulated device, some inits can only be done once. I should have probably added this patch at the end of the patchset. The rationale for the 'static bool' is pragmatism. Others solutions could be to : 1. add a new 'init' attribute under ICSState 2. modify spapr_register_hypercall() and spapr_rtas_register() to allow multiple definitions 3. add a spapr_is_registered_hypercall() helper 4. add a device fini routine (but it doesn't work for XIVE) I picked the most obvious and less intrusive one. solution 1 was my next favored one. C. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/intc/spapr_xive.c | 9 +++++++++ >> hw/intc/xics_spapr.c | 10 ++++++++++ >> 2 files changed, 19 insertions(+) >> >> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c >> index f889c89dc2e9..15d41d9cd36d 100644 >> --- a/hw/intc/spapr_xive.c >> +++ b/hw/intc/spapr_xive.c >> @@ -337,6 +337,15 @@ void spapr_xive_init(SpaprXive *xive, Error **errp) >> XiveSource *xsrc = &xive->source; >> XiveENDSource *end_xsrc = &xive->end_source; >> >> + /* >> + * The emulated XIVE device can only be initialized once. If the >> + * ESB memory region has been already mapped, it means we have been >> + * through there. >> + */ >> + if (memory_region_is_mapped(&xsrc->esb_mmio)) { >> + return; >> + } >> + >> /* TIMA initialization */ >> memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops, xive, >> "xive.tima", 4ull << TM_SHIFT); >> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c >> index 9d2b8adef7c5..67e82f3720b0 100644 >> --- a/hw/intc/xics_spapr.c >> +++ b/hw/intc/xics_spapr.c >> @@ -239,6 +239,16 @@ static void rtas_int_on(PowerPCCPU *cpu, SpaprMachineState *spapr, >> >> void xics_spapr_init(SpaprMachineState *spapr) >> { >> + static bool init_done; >> + >> + /* >> + * Emulated mode can only be initialized once. >> + */ >> + if (init_done) { >> + return; >> + } >> + init_done = true; >> + >> /* Registration of global state belongs into realize */ >> spapr_rtas_register(RTAS_IBM_SET_XIVE, "ibm,set-xive", rtas_set_xive); >> spapr_rtas_register(RTAS_IBM_GET_XIVE, "ibm,get-xive", rtas_get_xive); >
On Fri, Mar 22, 2019 at 08:07:23AM +0100, Cédric Le Goater wrote: > On 3/22/19 3:15 AM, David Gibson wrote: > > On Thu, Mar 21, 2019 at 03:49:13PM +0100, Cédric Le Goater wrote: > >> Add a check to make sure that the routine initializing the emulated > >> IRQ device is called once. We don't have much to test on the XICS > >> side, so we introduce a 'static bool'. Not very elegant but works well > >> enough. > > > > What's the rationale for this? > > The rationale for initializing the IRQ device only once is that reset > will do the initialization when KVM support is added for the dual > interrupt mode. When using the emulated device, some inits can only be > done once. I should have probably added this patch at the end of the > patchset. > > The rationale for the 'static bool' is pragmatism. Others solutions > could be to : > > 1. add a new 'init' attribute under ICSState > 2. modify spapr_register_hypercall() and spapr_rtas_register() > to allow multiple definitions > 3. add a spapr_is_registered_hypercall() helper > 4. add a device fini routine (but it doesn't work for XIVE) > > I picked the most obvious and less intrusive one. solution 1 was > my next favored one. Hm, I see. I _really_ dislike static locals because they're effectively global state that's very hard to spot. So, yeah, I'd go with option (1) above. It doesn't need to be exposed to the user or migrated, so it should be very straightforward.
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c index f889c89dc2e9..15d41d9cd36d 100644 --- a/hw/intc/spapr_xive.c +++ b/hw/intc/spapr_xive.c @@ -337,6 +337,15 @@ void spapr_xive_init(SpaprXive *xive, Error **errp) XiveSource *xsrc = &xive->source; XiveENDSource *end_xsrc = &xive->end_source; + /* + * The emulated XIVE device can only be initialized once. If the + * ESB memory region has been already mapped, it means we have been + * through there. + */ + if (memory_region_is_mapped(&xsrc->esb_mmio)) { + return; + } + /* TIMA initialization */ memory_region_init_io(&xive->tm_mmio, OBJECT(xive), &xive_tm_ops, xive, "xive.tima", 4ull << TM_SHIFT); diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c index 9d2b8adef7c5..67e82f3720b0 100644 --- a/hw/intc/xics_spapr.c +++ b/hw/intc/xics_spapr.c @@ -239,6 +239,16 @@ static void rtas_int_on(PowerPCCPU *cpu, SpaprMachineState *spapr, void xics_spapr_init(SpaprMachineState *spapr) { + static bool init_done; + + /* + * Emulated mode can only be initialized once. + */ + if (init_done) { + return; + } + init_done = true; + /* Registration of global state belongs into realize */ spapr_rtas_register(RTAS_IBM_SET_XIVE, "ibm,set-xive", rtas_set_xive); spapr_rtas_register(RTAS_IBM_GET_XIVE, "ibm,get-xive", rtas_get_xive);
Add a check to make sure that the routine initializing the emulated IRQ device is called once. We don't have much to test on the XICS side, so we introduce a 'static bool'. Not very elegant but works well enough. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/intc/spapr_xive.c | 9 +++++++++ hw/intc/xics_spapr.c | 10 ++++++++++ 2 files changed, 19 insertions(+)