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

[David Gibson]

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.

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

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