Re: [Qemu-devel] [PATCH v1 09/11] ppc/xics: Split ICS into ics-base and ics class

[Nikunj A Dadhania]

David Gibson <address@hidden> writes:

> [ Unknown signature status ]
> On Mon, Jun 27, 2016 at 03:41:06PM +0530, Nikunj A Dadhania wrote:
>> Nikunj A Dadhania <address@hidden> writes:
>> 
>> > David Gibson <address@hidden> writes:
>> >
>> >> [ Unknown signature status ]
>> >> On Thu, Jun 23, 2016 at 11:17:28PM +0530, Nikunj A Dadhania wrote:
>> >>> From: Benjamin Herrenschmidt <address@hidden>
>> >>> 
>> >>> The existing implementation remains same and ics-base is introduced.
>> >>> 
>> >>> 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.
>> >>> 
>> >>> Signed-off-by: Benjamin Herrenschmidt <address@hidden>
>> >>> Signed-off-by: Nikunj A Dadhania <address@hidden>
>> >>> ---
>> >>>  hw/intc/xics.c        | 101 
>> >>> +++++++++++++++++++++++++++++++++-----------------
>> >>>  hw/intc/xics_spapr.c  |  36 ++++++++++--------
>> >>>  include/hw/ppc/xics.h |  11 +++++-
>> >>>  3 files changed, 97 insertions(+), 51 deletions(-)
>> >>> 
>> >>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> >>> index 326d21f..e2aa48d 100644
>> >>> --- a/hw/intc/xics.c
>> >>> +++ b/hw/intc/xics.c
>> >>> @@ -220,9 +220,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);
>> >>> -static void ics_eoi(ICSState *ics, int nr);
>> >>> +static void ics_base_reject(ICSState *ics, uint32_t nr)
>> >>
>> >> AFICT these will actually work for any of the derived classes, since
>> >> they call the function pointer.  So I thin the original name was
>> >> better than ics_base_*().
>> >
>> > Sure, will change.
>> 
>> I had a look at this again, we will need to use ics_base_*(), same file
>> has the implementation of ics_reject() for TYPE_ICS.
>
> No, the ics_reject() plain names still work best for the generic
> versions which call via the function pointers.  Instead we should find
> a new name for the TYPE_ICE implementations.

Should we go back to call it as TYPE_ICS_SIMPLE retaining the "ics" for
migration compatibility. Rename all the related functions as
ics_simple_*

Similar to what we did for XICS

#define TYPE_XICS_COMMON "xics-common"
#define TYPE_XICS_SPAPR "xics"
#define TYPE_XICS_SPAPR_KVM "xics-spapr-kvm"
#define TYPE_XICS_NATIVE "xics-native"


Like this


#define TYPE_ICS_BASE   "ics-base"
#define TYPE_ICS_SIMPLE "ics"
#define TYPE_KVM_ICS    "icskvm"

>
>> BenH's patches had renamed the class implementation as ics_simple_*().
>> Since we moved to using ICS_BASE, ICS and KVM_ICS, IMHO this seems to
>> the appropriate names.
>
> No.  Using ics_base_*() for the generic versions is actively
> misleading.  Using good names for those is more important than what
> would usually be consistent naming practice for the TYPE_ICS
> implementation.

Regards
Nikunj