| Message ID | 1474275250-26433-1-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
On Mon, Sep 19, 2016 at 10:54:10AM +0200, Cédric Le Goater wrote: > Today, the CPU index is used to index the icp array under xics. This > works correctly when the indexes are sync but that is an assumption > that could break future implementations. For instance, the PowerNV > platform CPUs use real HW ids and they are not contiguous. > > Let's introduce some helpers to hide the underlying structure of the > ICP array. The criteria to look for an ICPstate is still the CPU index > but it is decorrelated from the array index. > > This is an RFC to see what people think. It is used on the powernv > branch but it won't apply as it is on upstream. It should certainly be > optimised when a large number of CPUs are configured. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> Hm, so. First, I think the helpers to get the right icp state are a good idea, regardless of anything else. I'm happy to go ahead with a patch which introduces just that, because it will make future revisions easier. But, the rest of the changes seem to be predicated on allowing non-contiguous cpu_index values. Maybe we want to do that eventually, but we're a pretty long way off at present, doing so involves work in libvirt as well as qemu itself, and it's not clear we actually want it. I think for the time being you'd be better off keeping the simple array of icp states indexed by contiguous (barring cpu hotplug) cpu_index values, and dissociating the physical IDs (PIR == interrupt server number == DT id, IIUC) from the cpu_index. Obviously you'll need helpers to convert between cpu_index and physical ID at the machine level. > --- > hw/intc/xics.c | 59 ++++++++++++++++++++++++++++++++++++++++---------- > hw/intc/xics_kvm.c | 7 +---- > hw/intc/xics_spapr.c | 14 ++++++----- > include/hw/ppc/xics.h | 1 > 4 files changed, 59 insertions(+), 22 deletions(-) > > Index: qemu-dgibson-for-2.8.git/hw/intc/xics.c > =================================================================== > --- qemu-dgibson-for-2.8.git.orig/hw/intc/xics.c > +++ qemu-dgibson-for-2.8.git/hw/intc/xics.c > @@ -50,12 +50,41 @@ int xics_get_cpu_index_by_dt_id(int cpu_ > return -1; > } > > + > +static ICPState *xics_get_icp(XICSState *xics, CPUState *cs) > +{ > + int i; > + > + for (i = 0 ; i < xics->nr_servers; i++) { > + ICPState *ss = &xics->ss[i]; > + if (ss->cs == NULL) { > + ss->cs = cs; > + return ss; > + } > + } > + > + return NULL; > +} > + > +ICPState *xics_find_icp(XICSState *xics, int cpu_index) > +{ > + int i; > + > + for (i = 0 ; i < xics->nr_servers; i++) { > + ICPState *ss = &xics->ss[i]; > + if (ss->cs && ss->cs->cpu_index == cpu_index) > + return ss; > + } > + > + return NULL; > +} > + > void xics_cpu_destroy(XICSState *xics, PowerPCCPU *cpu) > { > CPUState *cs = CPU(cpu); > - ICPState *ss = &xics->ss[cs->cpu_index]; > + ICPState *ss = xics_find_icp(xics, cs->cpu_index); > > - assert(cs->cpu_index < xics->nr_servers); > + assert(ss); > assert(cs == ss->cs); > > ss->output = NULL; > @@ -66,12 +95,10 @@ void xics_cpu_setup(XICSState *xics, Pow > { > CPUState *cs = CPU(cpu); > CPUPPCState *env = &cpu->env; > - ICPState *ss = &xics->ss[cs->cpu_index]; > + ICPState *ss = xics_get_icp(xics, cs); > XICSStateClass *info = XICS_COMMON_GET_CLASS(xics); > > - assert(cs->cpu_index < xics->nr_servers); > - > - ss->cs = cs; > + assert(ss); > > if (info->cpu_setup) { > info->cpu_setup(xics, cpu); > @@ -276,9 +303,11 @@ static void icp_check_ipi(ICPState *ss) > > static void icp_resend(XICSState *xics, int server) > { > - ICPState *ss = xics->ss + server; > + ICPState *ss = xics_find_icp(xics, server); > ICSState *ics; > > + assert(ss); > + > if (ss->mfrr < CPPR(ss)) { > icp_check_ipi(ss); > } > @@ -289,10 +318,12 @@ static void icp_resend(XICSState *xics, > > void icp_set_cppr(XICSState *xics, int server, uint8_t cppr) > { > - ICPState *ss = xics->ss + server; > + ICPState *ss = xics_find_icp(xics, server); > uint8_t old_cppr; > uint32_t old_xisr; > > + assert(ss); > + > old_cppr = CPPR(ss); > ss->xirr = (ss->xirr & ~CPPR_MASK) | (cppr << 24); > > @@ -316,7 +347,9 @@ void icp_set_cppr(XICSState *xics, int s > > void icp_set_mfrr(XICSState *xics, int server, uint8_t mfrr) > { > - ICPState *ss = xics->ss + server; > + ICPState *ss = xics_find_icp(xics, server); > + > + assert(ss); > > ss->mfrr = mfrr; > if (mfrr < CPPR(ss)) { > @@ -348,10 +381,12 @@ uint32_t icp_ipoll(ICPState *ss, uint32_ > > void icp_eoi(XICSState *xics, int server, uint32_t xirr) > { > - ICPState *ss = xics->ss + server; > + ICPState *ss = xics_find_icp(xics, server); > ICSState *ics; > uint32_t irq; > > + assert(ss); > + > /* Send EOI -> ICS */ > ss->xirr = (ss->xirr & ~CPPR_MASK) | (xirr & CPPR_MASK); > trace_xics_icp_eoi(server, xirr, ss->xirr); > @@ -369,7 +404,9 @@ void icp_eoi(XICSState *xics, int server > static void icp_irq(ICSState *ics, int server, int nr, uint8_t priority) > { > XICSState *xics = ics->xics; > - ICPState *ss = xics->ss + server; > + ICPState *ss = xics_find_icp(xics, server); > + > + assert(ss); > > trace_xics_icp_irq(server, nr, priority); > > Index: qemu-dgibson-for-2.8.git/include/hw/ppc/xics.h > =================================================================== > --- qemu-dgibson-for-2.8.git.orig/include/hw/ppc/xics.h > +++ qemu-dgibson-for-2.8.git/include/hw/ppc/xics.h > @@ -207,5 +207,6 @@ ICSState *xics_find_source(XICSState *ic > void xics_add_ics(XICSState *xics); > > void xics_hmp_info_pic(Monitor *mon, const QDict *qdict); > +ICPState *xics_find_icp(XICSState *xics, int cpu_index); > > #endif /* XICS_H */ > Index: qemu-dgibson-for-2.8.git/hw/intc/xics_spapr.c > =================================================================== > --- qemu-dgibson-for-2.8.git.orig/hw/intc/xics_spapr.c > +++ qemu-dgibson-for-2.8.git/hw/intc/xics_spapr.c > @@ -67,9 +67,11 @@ static target_ulong h_xirr(PowerPCCPU *c > target_ulong opcode, target_ulong *args) > { > CPUState *cs = CPU(cpu); > - uint32_t xirr = icp_accept(spapr->xics->ss + cs->cpu_index); > + ICPState *ss = xics_find_icp(spapr->xics, cs->cpu_index); > > - args[0] = xirr; > + assert(ss); > + > + args[0] = icp_accept(ss); > return H_SUCCESS; > } > > @@ -77,10 +79,9 @@ static target_ulong h_xirr_x(PowerPCCPU > target_ulong opcode, target_ulong *args) > { > CPUState *cs = CPU(cpu); > - ICPState *ss = &spapr->xics->ss[cs->cpu_index]; > - uint32_t xirr = icp_accept(ss); > + ICPState *ss = xics_find_icp(spapr->xics, cs->cpu_index); > > - args[0] = xirr; > + args[0] = icp_accept(ss); > args[1] = cpu_get_host_ticks(); > return H_SUCCESS; > } > @@ -99,8 +100,9 @@ static target_ulong h_ipoll(PowerPCCPU * > target_ulong opcode, target_ulong *args) > { > CPUState *cs = CPU(cpu); > + ICPState *ss = xics_find_icp(spapr->xics, cs->cpu_index); > uint32_t mfrr; > - uint32_t xirr = icp_ipoll(spapr->xics->ss + cs->cpu_index, &mfrr); > + uint32_t xirr = icp_ipoll(ss, &mfrr); > > args[0] = xirr; > args[1] = mfrr; > Index: qemu-dgibson-for-2.8.git/hw/intc/xics_kvm.c > =================================================================== > --- qemu-dgibson-for-2.8.git.orig/hw/intc/xics_kvm.c > +++ qemu-dgibson-for-2.8.git/hw/intc/xics_kvm.c > @@ -326,14 +326,11 @@ static const TypeInfo ics_kvm_info = { > */ > static void xics_kvm_cpu_setup(XICSState *xics, PowerPCCPU *cpu) > { > - CPUState *cs; > - ICPState *ss; > + CPUState *cs = CPU(cpu); > + ICPState *ss = xics_find_icp(xics, cs->cpu_index); > KVMXICSState *xicskvm = XICS_SPAPR_KVM(xics); > int ret; > > - cs = CPU(cpu); > - ss = &xics->ss[cs->cpu_index]; > - > assert(cs->cpu_index < xics->nr_servers); > if (xicskvm->kernel_xics_fd == -1) { > abort(); >
On 09/20/2016 03:25 PM, David Gibson wrote: > On Mon, Sep 19, 2016 at 10:54:10AM +0200, Cédric Le Goater wrote: >> Today, the CPU index is used to index the icp array under xics. This >> works correctly when the indexes are sync but that is an assumption >> that could break future implementations. For instance, the PowerNV >> platform CPUs use real HW ids and they are not contiguous. >> >> Let's introduce some helpers to hide the underlying structure of the >> ICP array. The criteria to look for an ICPstate is still the CPU index >> but it is decorrelated from the array index. >> >> This is an RFC to see what people think. It is used on the powernv >> branch but it won't apply as it is on upstream. It should certainly be >> optimised when a large number of CPUs are configured. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > > Hm, so. > > First, I think the helpers to get the right icp state are a good idea, > regardless of anything else. I'm happy to go ahead with a patch which > introduces just that, because it will make future revisions easier. yes. that would be a first step, to hide the underlying implementation. > But, the rest of the changes seem to be predicated on allowing > non-contiguous cpu_index values. Maybe we want to do that eventually, > but we're a pretty long way off at present, doing so involves work in > libvirt as well as qemu itself, and it's not clear we actually want > it. OK. The pir being used in most places in the code, this is not a big problem, and even this part in the core object : + env->spr[SPR_PIR] = cs->cpu_index; could be easily worked out. I was just lazy. So the only area where we are using cpu_index is in xics for the ICPState indexing. > I think for the time being you'd be better off keeping the simple > array of icp states indexed by contiguous (barring cpu hotplug) > cpu_index values, and dissociating the physical IDs (PIR == interrupt > server number == DT id, IIUC) from the cpu_index. Obviously you'll > need helpers to convert between cpu_index and physical ID at the > machine level. If we keep the helper idea but transform them into XICSStateClass methods, we can define customs ops in xics_native and handle the conversion real-id <-> icp id there. I think a simple array under xics_native would do. I will try that in v4. Thanks, C.
Index: qemu-dgibson-for-2.8.git/hw/intc/xics.c =================================================================== --- qemu-dgibson-for-2.8.git.orig/hw/intc/xics.c +++ qemu-dgibson-for-2.8.git/hw/intc/xics.c @@ -50,12 +50,41 @@ int xics_get_cpu_index_by_dt_id(int cpu_ return -1; } + +static ICPState *xics_get_icp(XICSState *xics, CPUState *cs) +{ + int i; + + for (i = 0 ; i < xics->nr_servers; i++) { + ICPState *ss = &xics->ss[i]; + if (ss->cs == NULL) { + ss->cs = cs; + return ss; + } + } + + return NULL; +} + +ICPState *xics_find_icp(XICSState *xics, int cpu_index) +{ + int i; + + for (i = 0 ; i < xics->nr_servers; i++) { + ICPState *ss = &xics->ss[i]; + if (ss->cs && ss->cs->cpu_index == cpu_index) + return ss; + } + + return NULL; +} + void xics_cpu_destroy(XICSState *xics, PowerPCCPU *cpu) { CPUState *cs = CPU(cpu); - ICPState *ss = &xics->ss[cs->cpu_index]; + ICPState *ss = xics_find_icp(xics, cs->cpu_index); - assert(cs->cpu_index < xics->nr_servers); + assert(ss); assert(cs == ss->cs); ss->output = NULL; @@ -66,12 +95,10 @@ void xics_cpu_setup(XICSState *xics, Pow { CPUState *cs = CPU(cpu); CPUPPCState *env = &cpu->env; - ICPState *ss = &xics->ss[cs->cpu_index]; + ICPState *ss = xics_get_icp(xics, cs); XICSStateClass *info = XICS_COMMON_GET_CLASS(xics); - assert(cs->cpu_index < xics->nr_servers); - - ss->cs = cs; + assert(ss); if (info->cpu_setup) { info->cpu_setup(xics, cpu); @@ -276,9 +303,11 @@ static void icp_check_ipi(ICPState *ss) static void icp_resend(XICSState *xics, int server) { - ICPState *ss = xics->ss + server; + ICPState *ss = xics_find_icp(xics, server); ICSState *ics; + assert(ss); + if (ss->mfrr < CPPR(ss)) { icp_check_ipi(ss); } @@ -289,10 +318,12 @@ static void icp_resend(XICSState *xics, void icp_set_cppr(XICSState *xics, int server, uint8_t cppr) { - ICPState *ss = xics->ss + server; + ICPState *ss = xics_find_icp(xics, server); uint8_t old_cppr; uint32_t old_xisr; + assert(ss); + old_cppr = CPPR(ss); ss->xirr = (ss->xirr & ~CPPR_MASK) | (cppr << 24); @@ -316,7 +347,9 @@ void icp_set_cppr(XICSState *xics, int s void icp_set_mfrr(XICSState *xics, int server, uint8_t mfrr) { - ICPState *ss = xics->ss + server; + ICPState *ss = xics_find_icp(xics, server); + + assert(ss); ss->mfrr = mfrr; if (mfrr < CPPR(ss)) { @@ -348,10 +381,12 @@ uint32_t icp_ipoll(ICPState *ss, uint32_ void icp_eoi(XICSState *xics, int server, uint32_t xirr) { - ICPState *ss = xics->ss + server; + ICPState *ss = xics_find_icp(xics, server); ICSState *ics; uint32_t irq; + assert(ss); + /* Send EOI -> ICS */ ss->xirr = (ss->xirr & ~CPPR_MASK) | (xirr & CPPR_MASK); trace_xics_icp_eoi(server, xirr, ss->xirr); @@ -369,7 +404,9 @@ void icp_eoi(XICSState *xics, int server static void icp_irq(ICSState *ics, int server, int nr, uint8_t priority) { XICSState *xics = ics->xics; - ICPState *ss = xics->ss + server; + ICPState *ss = xics_find_icp(xics, server); + + assert(ss); trace_xics_icp_irq(server, nr, priority); Index: qemu-dgibson-for-2.8.git/include/hw/ppc/xics.h =================================================================== --- qemu-dgibson-for-2.8.git.orig/include/hw/ppc/xics.h +++ qemu-dgibson-for-2.8.git/include/hw/ppc/xics.h @@ -207,5 +207,6 @@ ICSState *xics_find_source(XICSState *ic void xics_add_ics(XICSState *xics); void xics_hmp_info_pic(Monitor *mon, const QDict *qdict); +ICPState *xics_find_icp(XICSState *xics, int cpu_index); #endif /* XICS_H */ Index: qemu-dgibson-for-2.8.git/hw/intc/xics_spapr.c =================================================================== --- qemu-dgibson-for-2.8.git.orig/hw/intc/xics_spapr.c +++ qemu-dgibson-for-2.8.git/hw/intc/xics_spapr.c @@ -67,9 +67,11 @@ static target_ulong h_xirr(PowerPCCPU *c target_ulong opcode, target_ulong *args) { CPUState *cs = CPU(cpu); - uint32_t xirr = icp_accept(spapr->xics->ss + cs->cpu_index); + ICPState *ss = xics_find_icp(spapr->xics, cs->cpu_index); - args[0] = xirr; + assert(ss); + + args[0] = icp_accept(ss); return H_SUCCESS; } @@ -77,10 +79,9 @@ static target_ulong h_xirr_x(PowerPCCPU target_ulong opcode, target_ulong *args) { CPUState *cs = CPU(cpu); - ICPState *ss = &spapr->xics->ss[cs->cpu_index]; - uint32_t xirr = icp_accept(ss); + ICPState *ss = xics_find_icp(spapr->xics, cs->cpu_index); - args[0] = xirr; + args[0] = icp_accept(ss); args[1] = cpu_get_host_ticks(); return H_SUCCESS; } @@ -99,8 +100,9 @@ static target_ulong h_ipoll(PowerPCCPU * target_ulong opcode, target_ulong *args) { CPUState *cs = CPU(cpu); + ICPState *ss = xics_find_icp(spapr->xics, cs->cpu_index); uint32_t mfrr; - uint32_t xirr = icp_ipoll(spapr->xics->ss + cs->cpu_index, &mfrr); + uint32_t xirr = icp_ipoll(ss, &mfrr); args[0] = xirr; args[1] = mfrr; Index: qemu-dgibson-for-2.8.git/hw/intc/xics_kvm.c =================================================================== --- qemu-dgibson-for-2.8.git.orig/hw/intc/xics_kvm.c +++ qemu-dgibson-for-2.8.git/hw/intc/xics_kvm.c @@ -326,14 +326,11 @@ static const TypeInfo ics_kvm_info = { */ static void xics_kvm_cpu_setup(XICSState *xics, PowerPCCPU *cpu) { - CPUState *cs; - ICPState *ss; + CPUState *cs = CPU(cpu); + ICPState *ss = xics_find_icp(xics, cs->cpu_index); KVMXICSState *xicskvm = XICS_SPAPR_KVM(xics); int ret; - cs = CPU(cpu); - ss = &xics->ss[cs->cpu_index]; - assert(cs->cpu_index < xics->nr_servers); if (xicskvm->kernel_xics_fd == -1) { abort();
Today, the CPU index is used to index the icp array under xics. This works correctly when the indexes are sync but that is an assumption that could break future implementations. For instance, the PowerNV platform CPUs use real HW ids and they are not contiguous. Let's introduce some helpers to hide the underlying structure of the ICP array. The criteria to look for an ICPstate is still the CPU index but it is decorrelated from the array index. This is an RFC to see what people think. It is used on the powernv branch but it won't apply as it is on upstream. It should certainly be optimised when a large number of CPUs are configured. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/intc/xics.c | 59 ++++++++++++++++++++++++++++++++++++++++---------- hw/intc/xics_kvm.c | 7 +---- hw/intc/xics_spapr.c | 14 ++++++----- include/hw/ppc/xics.h | 1 4 files changed, 59 insertions(+), 22 deletions(-)