On 03/22/2017 07:33 AM, David Gibson wrote:
> On Thu, Mar 16, 2017 at 03:35:05PM +0100, Cédric Le Goater wrote:
>> Today, the ICPState array of the sPAPR machine is indexed with
>> 'cpu_index' of the CPUState. This numbering of CPUs is internal to
>> QEMU and the guest only knows about what is exposed in the device
>> tree, that is the 'cpu_dt_id'. This is why sPAPR uses the helper
>> xics_get_cpu_index_by_dt_id() to do the mapping in a couple of places.
>>
>> To provide a more generic XICS layer, we need to abstract the IRQ
>> 'server' number and remove any assumption made on its nature. It
>> should not be used as a 'cpu_index' for lookups like xics_cpu_setup()
>> and xics_cpu_destroy() do.
>>
>> To reach that goal, we choose to introduce an ICPState backlink under
>> PowerPCCPU, and let the machine core init routine do the ICPState
>> lookup. The resulting object is stored under PowerPCCPU which is
>> passed on to xics_cpu_setup(). The IRQ 'server' number in XICS is now
>> generic. sPAPR uses 'cpu_dt_id' and PowerNV will use 'PIR' number.
>>
>> This also has the benefit of simplifying the sPAPR hcall routines
>> which do not need to do any ICPState lookups anymore.
>>
>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>
> Having a direct link from the cpu to the interrupt state is a good
> idea. However, I'm not so fond of having a field that's specific to a
> particular platforms intc in the CPU. I'd suggest making it instead
> an Object *. We can use it for ICP now, but other platforms can use
> it for pointers to per-cpu interrupt state if they need to.
Yes. You are right. I made the change and the consequences are small
on the patchset. I will wait a bit before resending.
Thanks,
C.
>
>> ---
>> hw/intc/xics.c | 4 ++--
>> hw/intc/xics_spapr.c | 20 +++++---------------
>> hw/ppc/spapr_cpu_core.c | 4 +++-
>> target/ppc/cpu.h | 2 ++
>> 4 files changed, 12 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index e740989a1162..5cde86ceb3bc 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -52,7 +52,7 @@ int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
>> void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu)
>> {
>> CPUState *cs = CPU(cpu);
>> - ICPState *icp = xics_icp_get(xi, cs->cpu_index);
>> + ICPState *icp = cpu->icp;
>>
>> assert(icp);
>> assert(cs == icp->cs);
>> @@ -65,7 +65,7 @@ void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu)
>> {
>> CPUState *cs = CPU(cpu);
>> CPUPPCState *env = &cpu->env;
>> - ICPState *icp = xics_icp_get(xi, cs->cpu_index);
>> + ICPState *icp = cpu->icp;
>> ICPStateClass *icpc;
>>
>> assert(icp);
>> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
>> index 84d24b2837a7..178b3adc8af7 100644
>> --- a/hw/intc/xics_spapr.c
>> +++ b/hw/intc/xics_spapr.c
>> @@ -43,11 +43,9 @@
>> static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>> target_ulong opcode, target_ulong *args)
>> {
>> - CPUState *cs = CPU(cpu);
>> - ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
>> target_ulong cppr = args[0];
>>
>> - icp_set_cppr(icp, cppr);
>> + icp_set_cppr(cpu->icp, cppr);
>> return H_SUCCESS;
>> }
>>
>> @@ -69,9 +67,7 @@ static target_ulong h_ipi(PowerPCCPU *cpu,
>> sPAPRMachineState *spapr,
>> static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>> target_ulong opcode, target_ulong *args)
>> {
>> - CPUState *cs = CPU(cpu);
>> - ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
>> - uint32_t xirr = icp_accept(icp);
>> + uint32_t xirr = icp_accept(cpu->icp);
>>
>> args[0] = xirr;
>> return H_SUCCESS;
>> @@ -80,9 +76,7 @@ static target_ulong h_xirr(PowerPCCPU *cpu,
>> sPAPRMachineState *spapr,
>> static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>> target_ulong opcode, target_ulong *args)
>> {
>> - CPUState *cs = CPU(cpu);
>> - ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
>> - uint32_t xirr = icp_accept(icp);
>> + uint32_t xirr = icp_accept(cpu->icp);
>>
>> args[0] = xirr;
>> args[1] = cpu_get_host_ticks();
>> @@ -92,21 +86,17 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu,
>> sPAPRMachineState *spapr,
>> static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>> target_ulong opcode, target_ulong *args)
>> {
>> - CPUState *cs = CPU(cpu);
>> - ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
>> target_ulong xirr = args[0];
>>
>> - icp_eoi(icp, xirr);
>> + icp_eoi(cpu->icp, xirr);
>> return H_SUCCESS;
>> }
>>
>> static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>> target_ulong opcode, target_ulong *args)
>> {
>> - CPUState *cs = CPU(cpu);
>> - ICPState *icp = xics_icp_get(XICS_FABRIC(spapr), cs->cpu_index);
>> uint32_t mfrr;
>> - uint32_t xirr = icp_ipoll(icp, &mfrr);
>> + uint32_t xirr = icp_ipoll(cpu->icp, &mfrr);
>>
>> args[0] = xirr;
>> args[1] = mfrr;
>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>> index 6883f0991ae9..59f1cba6fba5 100644
>> --- a/hw/ppc/spapr_cpu_core.c
>> +++ b/hw/ppc/spapr_cpu_core.c
>> @@ -63,6 +63,7 @@ static void spapr_cpu_init(sPAPRMachineState *spapr,
>> PowerPCCPU *cpu,
>> Error **errp)
>> {
>> CPUPPCState *env = &cpu->env;
>> + XICSFabric *xi = XICS_FABRIC(spapr);
>>
>> /* Set time-base frequency to 512 MHz */
>> cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
>> @@ -80,7 +81,8 @@ static void spapr_cpu_init(sPAPRMachineState *spapr,
>> PowerPCCPU *cpu,
>> }
>> }
>>
>> - xics_cpu_setup(XICS_FABRIC(spapr), cpu);
>> + cpu->icp = xics_icp_get(xi, CPU(cpu)->cpu_index);
>> + xics_cpu_setup(xi, cpu);
>>
>> qemu_register_reset(spapr_cpu_reset, cpu);
>> spapr_cpu_reset(cpu);
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 5ee33b3fd315..b1626d0a6607 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -1176,6 +1176,7 @@ do { \
>>
>> typedef struct PPCVirtualHypervisor PPCVirtualHypervisor;
>> typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
>> +typedef struct ICPState ICPState;
>>
>> /**
>> * PowerPCCPU:
>> @@ -1196,6 +1197,7 @@ struct PowerPCCPU {
>> uint32_t max_compat;
>> uint32_t compat_pvr;
>> PPCVirtualHypervisor *vhyp;
>> + ICPState *icp;
>>
>> /* Fields related to migration compatibility hacks */
>> bool pre_2_8_migration;
>
