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

[David Gibson]

On Tue, Jun 28, 2016 at 10:36:23AM +0530, Nikunj A Dadhania wrote:
> 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"

Yes, that would be ok with me.

> >> 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
> 

-- 
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