Re: [Qemu-devel] [PATCH v6 04/37] ppc/xive: introduce the XiveRouter model

[David Gibson]

On Fri, Dec 07, 2018 at 08:49:21AM +0100, Cédric Le Goater wrote:
> On 12/7/18 2:57 AM, David Gibson wrote:
> > On Thu, Dec 06, 2018 at 07:22:54AM +0100, Cédric Le Goater wrote:
> >> On 12/6/18 4:41 AM, David Gibson wrote:
> >>> On Thu, Dec 06, 2018 at 12:22:18AM +0100, Cédric Le Goater wrote:
> >>>> The XiveRouter models the second sub-engine of the XIVE architecture :
> >>>> the Interrupt Virtualization Routing Engine (IVRE).
> >>>>
> >>>> The IVRE handles event notifications of the IVSE and performs the
> >>>> interrupt routing process. For this purpose, it uses a set of tables
> >>>> stored in system memory, the first of which being the Event Assignment
> >>>> Structure (EAS) table.
> >>>>
> >>>> The EAT associates an interrupt source number with an Event Notification
> >>>> Descriptor (END) which will be used in a second phase of the routing
> >>>> process to identify a Notification Virtual Target.
> >>>>
> >>>> The XiveRouter is an abstract class which needs to be inherited from
> >>>> to define a storage for the EAT, and other upcoming tables.
> >>>>
> >>>> Signed-off-by: Cédric Le Goater <address@hidden>
> >>>> ---
> >>>>  include/hw/ppc/xive.h      | 31 ++++++++++++++++
> >>>>  include/hw/ppc/xive_regs.h | 50 +++++++++++++++++++++++++
> >>>>  hw/intc/xive.c             | 76 ++++++++++++++++++++++++++++++++++++++
> >>>>  3 files changed, 157 insertions(+)
> >>>>  create mode 100644 include/hw/ppc/xive_regs.h
> >>>>
> >>>> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h
> >>>> index 6770cffec67d..57ec9f84f527 100644
> >>>> --- a/include/hw/ppc/xive.h
> >>>> +++ b/include/hw/ppc/xive.h
> >>>> @@ -141,6 +141,8 @@
> >>>>  #define PPC_XIVE_H
> >>>>  
> >>>>  #include "hw/qdev-core.h"
> >>>> +#include "hw/sysbus.h"
> >>>> +#include "hw/ppc/xive_regs.h"
> >>>>  
> >>>>  /*
> >>>>   * XIVE Fabric (Interface between Source and Router)
> >>>> @@ -297,4 +299,33 @@ static inline void xive_source_irq_set(XiveSource 
> >>>> *xsrc, uint32_t srcno,
> >>>>      }
> >>>>  }
> >>>>  
> >>>> +/*
> >>>> + * XIVE Router
> >>>> + */
> >>>> +
> >>>> +typedef struct XiveRouter {
> >>>> +    SysBusDevice    parent;
> >>>
> >>> I thought the plan was to make XiveRouter as well as XiveSource a
> >>> TYPE_DEVICE descendent rather than a SysBusDevice?
> >>
> >> We start talking about that, indeed, but then :
> >>
> >>    https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg06407.html
> >>
> >> I thought we concluded that it was going to get too complex.
> >>
> >> Also, sPAPRXive is a direct descendant of XiveRouter and we want sPAPRXive 
> >> on SysBus.
> > 
> > Ah, good point.  So, to clarify my thinking here - I think from a
> > theoretical point of view, having XiveRouter not be sysbus and
> > including it by composition is probably the "correct" approach.
> 
> One possible solution would be to transform the XiveRouter in a QOM 
> interface, this will be possible when I have removed the chip_id field,
> and define the VST accessors as we do today. I am not sure how QOM 
> interfaces are considered, but I think they are more in the composition 
> pattern than inheritance. That way, we could have sPAPRXive directly 
> inherit from SysBusDevice.
> 
> I can give it a try for v7, and you could merge the small XiveRouter 
> changes in the current XiveRouter patch.
> 
> > But I can also see that that will be a bit of a pain in practice.  So
> > yes, keeping it as a SysBusDevice is ok, at least as long as any
> > migration stuff is in the "outermost" / most specific type, which I
> > believe it is.
> 
> By this sentence, you mean that we don't rely on the XiveRouter model 
> to capture the sPAPRXive state ?

Yes.  Basically we should only have VMStateDecriptions registered by
the spapr specific objects, not the internal parts / superclasses
they're composed of.

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