| Message ID | 1489674912-21942-2-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
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 <clg@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. > --- > 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;
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 <clg@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; >
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;
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 <clg@kaod.org> --- 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(-)