Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 6/9] ppc/xics: Split ICS into ics-base and ics class

[Cédric Le Goater]

On 09/20/2016 08:02 AM, Nikunj A Dadhania wrote:
> Cédric Le Goater <address@hidden> writes:
> 
>> On 09/19/2016 08:29 AM, Nikunj A Dadhania wrote:
>>> From: Benjamin Herrenschmidt <address@hidden>
>>>
>>> The existing implementation remains same and ics-base is introduced. The
>>> type name "ics" is retained, and all the related functions renamed as
>>> ics_simple_*
>>>
>>> This will allow different implementations for the source controllers
>>> such as the MSI support of PHB3 on Power8 which uses in-memory state
>>> tables for example.
>>
>> A couple of issues below regarding the class helpers,
>>
>>> Signed-off-by: Benjamin Herrenschmidt <address@hidden>
>>> Signed-off-by: Nikunj A Dadhania <address@hidden>
>>> ---
>>>  hw/intc/trace-events  |  10 ++--
>>>  hw/intc/xics.c        | 143 
>>> +++++++++++++++++++++++++++++++-------------------
>>>  hw/intc/xics_kvm.c    |  10 ++--
>>>  hw/intc/xics_spapr.c  |  28 +++++-----
>>>  include/hw/ppc/xics.h |  23 +++++---
>>>  5 files changed, 131 insertions(+), 83 deletions(-)
>>>
>>> diff --git a/hw/intc/trace-events b/hw/intc/trace-events
>>> index aa342a8..a367b46 100644
>>> --- a/hw/intc/trace-events
>>> +++ b/hw/intc/trace-events
>>> @@ -50,12 +50,12 @@ xics_icp_accept(uint32_t old_xirr, uint32_t new_xirr) 
>>> "icp_accept: XIRR %#"PRIx3
>>>  xics_icp_eoi(int server, uint32_t xirr, uint32_t new_xirr) "icp_eoi: 
>>> server %d given XIRR %#"PRIx32" new XIRR %#"PRIx32
>>>  xics_icp_irq(int server, int nr, uint8_t priority) "cpu %d trying to 
>>> deliver irq %#"PRIx32" priority %#x"
>>>  xics_icp_raise(uint32_t xirr, uint8_t pending_priority) "raising IRQ new 
>>> XIRR=%#x new pending priority=%#x"
>>> -xics_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq %#x]"
>>> +xics_ics_simple_set_irq_msi(int srcno, int nr) "set_irq_msi: srcno %d [irq 
>>> %#x]"
>>>  xics_masked_pending(void) "set_irq_msi: masked pending"
>>> -xics_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq %#x]"
>>> -xics_ics_write_xive(int nr, int srcno, int server, uint8_t priority) 
>>> "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
>>> -xics_ics_reject(int nr, int srcno) "reject irq %#x [src %d]"
>>> -xics_ics_eoi(int nr) "ics_eoi: irq %#x"
>>> +xics_ics_simple_set_irq_lsi(int srcno, int nr) "set_irq_lsi: srcno %d [irq 
>>> %#x]"
>>> +xics_ics_simple_write_xive(int nr, int srcno, int server, uint8_t 
>>> priority) "ics_write_xive: irq %#x [src %d] server %#x prio %#x"
>>> +xics_ics_simple_reject(int nr, int srcno) "reject irq %#x [src %d]"
>>> +xics_ics_simple_eoi(int nr) "ics_eoi: irq %#x"
>>>  xics_alloc(int irq) "irq %d"
>>>  xics_alloc_block(int first, int num, bool lsi, int align) "first irq %d, 
>>> %d irqs, lsi=%d, alignnum %d"
>>>  xics_ics_free(int src, int irq, int num) "Source#%d, first irq %d, %d irqs"
>>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>>> index c7901c4..b15751e 100644
>>> --- a/hw/intc/xics.c
>>> +++ b/hw/intc/xics.c
>>> @@ -113,7 +113,7 @@ void xics_add_ics(XICSState *xics)
>>>  {
>>>      ICSState *ics;
>>>  
>>> -    ics = ICS(object_new(TYPE_ICS));
>>> +    ics = ICS_SIMPLE(object_new(TYPE_ICS_SIMPLE));
>>>      object_property_add_child(OBJECT(xics), "ics", OBJECT(ics), NULL);
>>>      ics->xics = xics;
>>>      QLIST_INSERT_HEAD(&xics->ics, ics, list);
>>> @@ -224,9 +224,32 @@ static const TypeInfo xics_common_info = {
>>>  #define XISR(ss)   (((ss)->xirr) & XISR_MASK)
>>>  #define CPPR(ss)   (((ss)->xirr) >> 24)
>>>  
>>> -static void ics_reject(ICSState *ics, int nr);
>>> -static void ics_resend(ICSState *ics, int server);
>>> -static void ics_eoi(ICSState *ics, int nr);
>>> +static void ics_reject(ICSState *ics, uint32_t nr)
>>> +{
>>> +    ICSStateClass *k = ICS_GET_CLASS(ics);
>>
>> Shouldn't that be ICS_BASE_GET_CLASS()
> 
> The class hierarchy is something like this:
> 
> ICS_BASE -> ICS_SIMPLE -> ICS_KVM

yes. but if we called  ics_* with an instance of an ics class which is 
not a ICS_SIMPLE class that will break.

ICSStateClass is the baseclass, whenever we call methods on a ICSState* 
object, we should use the method defines in ICSStateClass.


> We have an instance init for ICS_SIMPLE where we set these pointers.
> 
>>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
>>> index a7a1e54..2231f2a 100644
>>> --- a/include/hw/ppc/xics.h
>>> +++ b/include/hw/ppc/xics.h
>>> @@ -119,22 +119,29 @@ struct ICPState {
>>>      bool cap_irq_xics_enabled;
>>>  };
>>>  
>>> -#define TYPE_ICS "ics"
>>> -#define ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS)
>>> +#define TYPE_ICS_BASE "ics-base"
>>> +#define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE)
>>
>> #define ICS_BASE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_BASE)
> 
> Yes, that is a bug. Will correct.
> 
>>
>>> -#define TYPE_KVM_ICS "icskvm"
>>> -#define KVM_ICS(obj) OBJECT_CHECK(ICSState, (obj), TYPE_KVM_ICS)
>>> +/* Retain ics for sPAPR for migration from existing sPAPR guests */
>>> +#define TYPE_ICS_SIMPLE "ics"
>>> +#define ICS_SIMPLE(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_SIMPLE)
>>> +
>>> +#define TYPE_ICS_KVM "icskvm"
>>> +#define ICS_KVM(obj) OBJECT_CHECK(ICSState, (obj), TYPE_ICS_KVM)
>>>  
>>>  #define ICS_CLASS(klass) \
>>> -     OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS)
>>> +     OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_SIMPLE)
>>>  #define ICS_GET_CLASS(obj) \
>>> -     OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS)
>>> +     OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS_SIMPLE)
>>
>> #define ICS_BASE_CLASS(klass) \
>>      OBJECT_CLASS_CHECK(ICSStateClass, (klass), TYPE_ICS_BASE)
>> #define ICS_BASE_GET_CLASS(obj) \
>>      OBJECT_GET_CLASS(ICSStateClass, (obj), TYPE_ICS_BASE)
> 
> As explained above, not needed.

We will with powernv which defines a new ics type for the phb3 msis.

Thanks,

C.