On Mon, Oct 03, 2016 at 09:24:49AM +0200, Cédric Le Goater wrote: > Today, the Interrupt Presentation Controllers (ICP) are stored in an > array under the base class XICSState. They are simply indexed using > the CPU index field of CPUState. This made sense for the current > derived classes, spapr and kvm, as the CPU index are contiguous. > Nevertheless some problems have started to occur with CPU hotplug. > > With the PowerNV platform CPUs, this is not true anymore. Real HW ids > are being used and they are not contiguous. So we need a way to > customize the lookups in the array. Below is a proposal for this > purpose. A couple of helpers are introduced to hide the nature of the > underlying ICP array and also a new XICSStateClass method 'find_icp' > to let the derived classes customize the ICP lookups. > > A followup patch would be to let the derived class decide on the ICP > storage. They could use a hash table for instance. We would need to > introduce a new class method 'get_icp' for that. Or simply, change the > ICP array for a hash table and let the derived class decide on the key > to use. > > Signed-off-by: Cédric Le Goater <c...@kaod.org>
Uuuh.. IIRC this series has now been reworked (as suggested) to keep
cpu_index contiguous and have a separate hardware id. Doesn't that
mean this patch can go away (though the callers might need to use a
hwid->cpu_index helper).
> ---
> hw/intc/xics.c | 48 ++++++++++++++++++++++++++++++++++++------------
> hw/intc/xics_kvm.c | 7 ++-----
> hw/intc/xics_spapr.c | 12 ++++++------
> include/hw/ppc/xics.h | 2 ++
> 4 files changed, 46 insertions(+), 23 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 3bbbcc847791..876c472aaa69 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -48,12 +48,32 @@ int xics_get_cpu_index_by_dt_id(int cpu_dt_id)
> return -1;
> }
>
> +ICPState *xics_find_icp(XICSState *xics, int cpu_index)
> +{
> + XICSStateClass *xsc = XICS_COMMON_GET_CLASS(xics);
> + ICPState *icp = xsc->find_icp(xics, cpu_index);
> +
> + assert(icp);
> +
> + return icp;
> +}
> +
> +static ICPState *xics_get_icp(XICSState *xics, CPUState *cs)
> +{
> + ICPState *ss;
> +
> + assert(cs->cpu_index < xics->nr_servers);
> +
> + ss = &xics->ss[cs->cpu_index];
> + ss->cs = cs;
> + return ss;
> +}
> +
> 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(cs == ss->cs);
>
> ss->output = NULL;
> @@ -64,13 +84,9 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
> {
> 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;
> -
> if (info->cpu_setup) {
> info->cpu_setup(xics, cpu);
> }
> @@ -94,6 +110,12 @@ void xics_cpu_setup(XICSState *xics, PowerPCCPU *cpu)
> /*
> * XICS Common class - parent for emulated XICS and KVM-XICS
> */
> +
> +static ICPState *xics_common_find_icp(XICSState *xics, int cpu_index)
> +{
> + return &xics->ss[cpu_index];
> +}
> +
> static void xics_common_reset(DeviceState *d)
> {
> XICSState *xics = XICS_COMMON(d);
> @@ -191,8 +213,10 @@ static void xics_common_initfn(Object *obj)
> static void xics_common_class_init(ObjectClass *oc, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(oc);
> + XICSStateClass *xsc = XICS_COMMON_CLASS(oc);
>
> dc->reset = xics_common_reset;
> + xsc->find_icp = xics_common_find_icp;
> }
>
> static const TypeInfo xics_common_info = {
> @@ -261,7 +285,7 @@ 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;
>
> if (ss->mfrr < CPPR(ss)) {
> @@ -274,7 +298,7 @@ static void icp_resend(XICSState *xics, int server)
>
> 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;
>
> @@ -301,7 +325,7 @@ void icp_set_cppr(XICSState *xics, int server, uint8_t
> cppr)
>
> void icp_set_mfrr(XICSState *xics, int server, uint8_t mfrr)
> {
> - ICPState *ss = xics->ss + server;
> + ICPState *ss = xics_find_icp(xics, server);
>
> ss->mfrr = mfrr;
> if (mfrr < CPPR(ss)) {
> @@ -333,7 +357,7 @@ uint32_t icp_ipoll(ICPState *ss, uint32_t *mfrr)
>
> 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;
>
> @@ -354,7 +378,7 @@ void icp_eoi(XICSState *xics, int server, uint32_t xirr)
> 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);
>
> trace_xics_icp_irq(server, nr, priority);
>
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index 9c2f198fd142..b666bb59fc24 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/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();
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index e8d0623c2cb5..af29998b1255 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -67,9 +67,9 @@ static target_ulong h_xirr(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
> 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;
> + args[0] = icp_accept(ss);
> return H_SUCCESS;
> }
>
> @@ -77,10 +77,9 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
> 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 +98,9 @@ static target_ulong h_ipoll(PowerPCCPU *cpu,
> sPAPRMachineState *spapr,
> 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;
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index ca9f8da542e0..52c426d409c9 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -76,6 +76,7 @@ struct XICSStateClass {
> void (*cpu_setup)(XICSState *icp, PowerPCCPU *cpu);
> void (*set_nr_irqs)(XICSState *icp, uint32_t nr_irqs, Error **errp);
> void (*set_nr_servers)(XICSState *icp, uint32_t nr_servers, Error
> **errp);
> + ICPState *(*find_icp)(XICSState *xics, int cpu_index);
> };
>
> struct XICSState {
> @@ -206,5 +207,6 @@ void ics_set_irq_type(ICSState *ics, int srcno, bool lsi);
> ICSState *xics_find_source(XICSState *icp, int irq);
>
> void xics_hmp_info_pic(Monitor *mon, const QDict *qdict);
> +ICPState *xics_find_icp(XICSState *xics, int cpu_index);
>
> #endif /* XICS_H */
--
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
signature.asc
Description: PGP signature
