Re: [Qemu-devel] [PATCH v3 14/15] spapr/irq: initialize the IRQ device only once

[Cédric Le Goater]

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 <address@hidden>
>> ---
>>  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);
>