On 10/14/2016 07:34 AM, David Gibson wrote:
> 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).
Yes. This is how it is implemented.
The pnv machine only handles cpu hwid and it uses a custom find handler
to look for an ICP. But I see you made comments on the implementation.
Thanks,
C.
>> ---
>> 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 */
>
