Re: [Qemu-devel] [PATCH 02/10] ppc/xive: introduce a XiveTCTX pointer under PowerPCCPU

[Cédric Le Goater]

On 1/3/19 4:57 AM, David Gibson wrote:
> On Wed, Jan 02, 2019 at 06:57:35AM +0100, Cédric Le Goater wrote:
>> which will be used by the machine only when the XIVE interrupt mode is
>> in use.
> 
> I don't love the idea of putting a hook this specific into the
> PowerPCCPU structure, though it might be the easiest path in the short
> term.
> 
> A couple of approaches: 1) revisit my changes to allow for a pointer
> to machine-defined per-cpu data.  

ok but we still need two different presenters objects, one for each
mode.

> or 2) do we actually need a cpu to tctx pointer.
> 
> Expanding on (2) - here you use the pointer to find the right TIMA
> state to access,

yes. 

> but that could also be handled by having different TIMA IO instances 
> and mapping those individually to cpu_as.  

It might work for QEMU but I am not sure how to implement the KVM 
backend with such a design. We only have a single ram ptr mapping 
for the machine in the KVM case.

There are a couple of places where we need to loop on the XiveTCTX 
(presenter, KVM) and we use the QEMU CPU list and the cpu->tctx for 
that. One of the reasons we introduced the cpu->intc pointer some 
time ago was to get rid of the ICP array. 

I don't think we want to introduce a XiveTCTX array again.

> On the interrupt delivery side I think a tctx to cpu link will suffice.  

yes. that we already have. It is mostly used by KVM in fact. 

> For sPAPR there might be complications with translating cpu numbers in
> hcalls to the right tctx.

The XiveTCTX is not used by the hcalls. We are fine on that side.


The double pointer solution is what I prefer today, but if you are 
uncomfortable with it, we can come back to the previous where I was 
allocating a XiveTCTX child under the CPU and switching the presenter 
objects at reset. The only issue remaining was the child unparenting 
in the unrealize() method.    

Thanks, 

C.

 
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  target/ppc/cpu.h        | 2 ++
>>  hw/intc/xive.c          | 6 +++---
>>  hw/ppc/spapr_cpu_core.c | 7 ++++++-
>>  hw/ppc/spapr_irq.c      | 8 ++++----
>>  4 files changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index d5f99f1fc7b9..c76036985623 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -1177,6 +1177,7 @@ do {                                            \
>>  
>>  typedef struct PPCVirtualHypervisor PPCVirtualHypervisor;
>>  typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
>> +typedef struct XiveTCTX XiveTCTX;
>>  
>>  /**
>>   * PowerPCCPU:
>> @@ -1196,6 +1197,7 @@ struct PowerPCCPU {
>>      uint32_t compat_pvr;
>>      PPCVirtualHypervisor *vhyp;
>>      Object *intc;
>> +    XiveTCTX *tctx;
>>      void *machine_data;
>>      int32_t node_id; /* NUMA node this CPU belongs to */
>>      PPCHash64Options *hash64_opts;
>> diff --git a/hw/intc/xive.c b/hw/intc/xive.c
>> index ea33494338dc..410c53278a11 100644
>> --- a/hw/intc/xive.c
>> +++ b/hw/intc/xive.c
>> @@ -321,7 +321,7 @@ static void xive_tm_write(void *opaque, hwaddr offset,
>>                            uint64_t value, unsigned size)
>>  {
>>      PowerPCCPU *cpu = POWERPC_CPU(current_cpu);
>> -    XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
>> +    XiveTCTX *tctx = cpu->tctx;
>>      const XiveTmOp *xto;
>>  
>>      /*
>> @@ -360,7 +360,7 @@ static void xive_tm_write(void *opaque, hwaddr offset,
>>  static uint64_t xive_tm_read(void *opaque, hwaddr offset, unsigned size)
>>  {
>>      PowerPCCPU *cpu = POWERPC_CPU(current_cpu);
>> -    XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
>> +    XiveTCTX *tctx = cpu->tctx;
>>      const XiveTmOp *xto;
>>  
>>      /*
>> @@ -1186,7 +1186,7 @@ static bool xive_presenter_match(XiveRouter *xrtr, 
>> uint8_t format,
>>  
>>      CPU_FOREACH(cs) {
>>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>> -        XiveTCTX *tctx = XIVE_TCTX(cpu->intc);
>> +        XiveTCTX *tctx = cpu->tctx;
>>          int ring;
>>  
>>          /*
>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>> index 2739b2a4b818..1473ef853336 100644
>> --- a/hw/ppc/spapr_cpu_core.c
>> +++ b/hw/ppc/spapr_cpu_core.c
>> @@ -194,7 +194,12 @@ static void spapr_unrealize_vcpu(PowerPCCPU *cpu, 
>> sPAPRCPUCore *sc)
>>          vmstate_unregister(NULL, &vmstate_spapr_cpu_state, 
>> cpu->machine_data);
>>      }
>>      qemu_unregister_reset(spapr_cpu_reset, cpu);
>> -    object_unparent(cpu->intc);
>> +    if (cpu->intc) {
>> +        object_unparent(cpu->intc);
>> +    }
>> +    if (cpu->tctx) {
>> +        object_unparent(OBJECT(cpu->tctx));
>> +    }
>>      cpu_remove_sync(CPU(cpu));
>>      object_unparent(OBJECT(cpu));
>>  }
>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>> index 2d2e17b66533..8d028db44ff4 100644
>> --- a/hw/ppc/spapr_irq.c
>> +++ b/hw/ppc/spapr_irq.c
>> @@ -305,7 +305,7 @@ static void spapr_irq_print_info_xive(sPAPRMachineState 
>> *spapr,
>>      CPU_FOREACH(cs) {
>>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>>  
>> -        xive_tctx_pic_print_info(XIVE_TCTX(cpu->intc), mon);
>> +        xive_tctx_pic_print_info(cpu->tctx, mon);
>>      }
>>  
>>      spapr_xive_pic_print_info(spapr->xive, mon);
>> @@ -323,13 +323,13 @@ static void 
>> spapr_irq_cpu_intc_create_xive(sPAPRMachineState *spapr,
>>          return;
>>      }
>>  
>> -    cpu->intc = obj;
>> +    cpu->tctx = XIVE_TCTX(obj);
>>  
>>      /*
>>       * (TCG) Early setting the OS CAM line for hotplugged CPUs as they
>>       * don't beneficiate from the reset of the XIVE IRQ backend
>>       */
>> -    spapr_xive_set_tctx_os_cam(XIVE_TCTX(obj));
>> +    spapr_xive_set_tctx_os_cam(cpu->tctx);
>>  }
>>  
>>  static int spapr_irq_post_load_xive(sPAPRMachineState *spapr, int 
>> version_id)
>> @@ -345,7 +345,7 @@ static void spapr_irq_reset_xive(sPAPRMachineState 
>> *spapr, Error **errp)
>>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>>  
>>          /* (TCG) Set the OS CAM line of the thread interrupt context. */
>> -        spapr_xive_set_tctx_os_cam(XIVE_TCTX(cpu->intc));
>> +        spapr_xive_set_tctx_os_cam(cpu->tctx);
>>      }
>>  }
>>  
>