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 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. > --- > 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; -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson