Re: [PATCH 1/7] spapr, xics: Get number of servers with a XICSFabricClass method

[Greg Kurz]

On Thu, 3 Oct 2019 15:59:29 +0200
Cédric Le Goater <address@hidden> wrote:

> On 03/10/2019 15:41, Greg Kurz wrote:
> > On Thu, 3 Oct 2019 15:19:22 +0200
> > Cédric Le Goater <address@hidden> wrote:
> > 
> >> On 03/10/2019 15:02, Greg Kurz wrote:
> >>> On Thu, 3 Oct 2019 14:58:45 +0200
> >>> Cédric Le Goater <address@hidden> wrote:
> >>>
> >>>> On 03/10/2019 14:49, Greg Kurz wrote:
> >>>>> On Thu, 3 Oct 2019 14:24:06 +0200
> >>>>> Cédric Le Goater <address@hidden> wrote:
> >>>>>
> >>>>>> On 03/10/2019 14:00, Greg Kurz wrote:
> >>>>>>> The number of servers, ie. upper bound of the highest VCPU id, is
> >>>>>>> currently only needed to generate the "interrupt-controller" node
> >>>>>>> in the DT. Soon it will be needed to inform the XICS-on-XIVE KVM
> >>>>>>> device that it can allocates less resources in the XIVE HW.
> >>>>>>>
> >>>>>>> Add a method to XICSFabricClass for this purpose. 
> >>>>>>
> >>>>>> This is sPAPR code and PowerNV does not care.
> >>>>>>
> >>>>>
> >>>>> Then PowerNV doesn't need to implement the method.
> >>>>>
> >>>>>> why can not we simply call spapr_max_server_number(spapr) ?
> >>>>>>
> >>>>>
> >>>>> Because the backend shouldn't reach out to sPAPR machine
> >>>>> internals. XICSFabric is the natural interface for ICS/ICP
> >>>>> if they need something from the machine.
> >>>>
> >>>> From what I can see, xics_nr_servers() is only called by : 
> >>>>
> >>>>   spapr_dt_xics(SpaprMachineState *spapr ...)
> >>>>   xics_kvm_connect(SpaprMachineState *spapr ...)
> >>>>
> >>>
> >>> Yes... and ?
> >>
> >> so it is spapr only and we can use spapr_max_server_number(spapr)
> >> without the need of a new XICSFabric handler.
> >>
> > 
> > Yet we did care to have an nr_server argument to spapr_dt_xics(), even
> > though we could have called spapr_max_server_number() since the
> > beginning. 
> 
> yes it is sometime good practice to pass only what a routine needs 
> and the whole state.
> 

Sure.

> > And we also care not to call spapr_max_server_number()
> > to setup nr_ends in XIVE. Right ?
> 
> yes. That's handled with a property.
>  

Yes and the same could be done if we had a derived type
for sPAPR XICS.

> > Ideally spapr_max_server_number() should even be local to spapr.c
> > IMHO. It happens to be extern because spapr_irq_init() needs it for
> > sPAPR XIVE, but I think it should rather be passed as an argument.
> 
> That will be the case again  when David has finished cleaning it up.
> 

I fully agree. I posted this series based on the current state of
ppc-for-4.2 in order to start the discussion, but I'm looking
forward to base it on the proper backend models that David is
cooking up for us :)

> > Anyway, if this patch doesn't reach consensus, I'll switch to using
> > spapr_max_server_number()... and do the same in sPAPR XIVE for
> > consistency ;-)
> 
> I don't see the problem sPAPR XIVE. There is a property "nr-server".
> 

Not before patch 2 in this series :) and it could be dropped and
replaced by a call to spapr_max_server_number() during realize.

> The XICS Fabric was introduced to handle differences between the
> PowerNV machine and spapr machine mostly. We can extend its use
> to abstract the interface between the machine and its device models

Well... this was suggested by David in some other mail...

> but, in that case, if we want consistency, we should then remove 
> the use of SpaprMachinestate from xics_kvm to begin with and use 
> only XICSFabric handlers. This is not the case today.
> 

I must admit I'm not a big fan of seeing SpaprMachinestate being
used in a lot of places like a jack-of-all-trade structure that
allows to access anything you want (even if you should not).

> Because this is a spapr model only. Why bother ? This would add 
> just extra and useless ops. 
> 
> You should wait for David to finish the cleanup. I think that 
> at end we will only do pass a nr_servers to a couple of routine.
> kvmppc_xive_connect() might need an extra argument.
> 

This is how I was doing when I started to work on this :)

> 
> C.
> 
> 
> > 
> >> C. 
> >>
> >>>> C. 
> >>>>
> >>>>>>
> >>>>>>> Implement it
> >>>>>>> for sPAPR and use it to generate the "interrupt-controller" node.
> >>>>>>>
> >>>>>>> Signed-off-by: Greg Kurz <address@hidden>
> >>>>>>> ---
> >>>>>>>  hw/intc/xics.c        |    7 +++++++
> >>>>>>>  hw/intc/xics_spapr.c  |    3 ++-
> >>>>>>>  hw/ppc/spapr.c        |    8 ++++++++
> >>>>>>>  include/hw/ppc/xics.h |    2 ++
> >>>>>>>  4 files changed, 19 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> >>>>>>> index dfe7dbd254ab..f82072935266 100644
> >>>>>>> --- a/hw/intc/xics.c
> >>>>>>> +++ b/hw/intc/xics.c
> >>>>>>> @@ -716,6 +716,13 @@ ICPState *xics_icp_get(XICSFabric *xi, int 
> >>>>>>> server)
> >>>>>>>      return xic->icp_get(xi, server);
> >>>>>>>  }
> >>>>>>>  
> >>>>>>> +uint32_t xics_nr_servers(XICSFabric *xi)
> >>>>>>> +{
> >>>>>>> +    XICSFabricClass *xic = XICS_FABRIC_GET_CLASS(xi);
> >>>>>>> +
> >>>>>>> +    return xic->nr_servers(xi);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>>  void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
> >>>>>>>  {
> >>>>>>>      assert(!(ics->irqs[srcno].flags & XICS_FLAGS_IRQ_MASK));
> >>>>>>> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> >>>>>>> index 6e5eb24b3cca..aa568ed0dc0d 100644
> >>>>>>> --- a/hw/intc/xics_spapr.c
> >>>>>>> +++ b/hw/intc/xics_spapr.c
> >>>>>>> @@ -311,8 +311,9 @@ static void ics_spapr_realize(DeviceState *dev, 
> >>>>>>> Error **errp)
> >>>>>>>  void spapr_dt_xics(SpaprMachineState *spapr, uint32_t nr_servers, 
> >>>>>>> void *fdt,
> >>>>>>>                     uint32_t phandle)
> >>>>>>>  {
> >>>>>>> +    ICSState *ics = spapr->ics;
> >>>>>>>      uint32_t interrupt_server_ranges_prop[] = {
> >>>>>>> -        0, cpu_to_be32(nr_servers),
> >>>>>>> +        0, cpu_to_be32(xics_nr_servers(ics->xics)),
> >>>>>>>      };
> >>>>>>>      int node;
> >>>>>>>  
> >>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>>>>> index 514a17ae74d6..b8b9796c88e4 100644
> >>>>>>> --- a/hw/ppc/spapr.c
> >>>>>>> +++ b/hw/ppc/spapr.c
> >>>>>>> @@ -4266,6 +4266,13 @@ static ICPState *spapr_icp_get(XICSFabric *xi, 
> >>>>>>> int vcpu_id)
> >>>>>>>      return cpu ? spapr_cpu_state(cpu)->icp : NULL;
> >>>>>>>  }
> >>>>>>>  
> >>>>>>> +static uint32_t spapr_nr_servers(XICSFabric *xi)
> >>>>>>> +{
> >>>>>>> +    SpaprMachineState *spapr = SPAPR_MACHINE(xi);
> >>>>>>> +
> >>>>>>> +    return spapr_max_server_number(spapr);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>>  static void spapr_pic_print_info(InterruptStatsProvider *obj,
> >>>>>>>                                   Monitor *mon)
> >>>>>>>  {
> >>>>>>> @@ -4423,6 +4430,7 @@ static void 
> >>>>>>> spapr_machine_class_init(ObjectClass *oc, void *data)
> >>>>>>>      xic->ics_get = spapr_ics_get;
> >>>>>>>      xic->ics_resend = spapr_ics_resend;
> >>>>>>>      xic->icp_get = spapr_icp_get;
> >>>>>>> +    xic->nr_servers = spapr_nr_servers;
> >>>>>>>      ispc->print_info = spapr_pic_print_info;
> >>>>>>>      /* Force NUMA node memory size to be a multiple of
> >>>>>>>       * SPAPR_MEMORY_BLOCK_SIZE (256M) since that's the granularity
> >>>>>>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> >>>>>>> index 1e6a9300eb2b..e6bb1239e8f8 100644
> >>>>>>> --- a/include/hw/ppc/xics.h
> >>>>>>> +++ b/include/hw/ppc/xics.h
> >>>>>>> @@ -151,9 +151,11 @@ typedef struct XICSFabricClass {
> >>>>>>>      ICSState *(*ics_get)(XICSFabric *xi, int irq);
> >>>>>>>      void (*ics_resend)(XICSFabric *xi);
> >>>>>>>      ICPState *(*icp_get)(XICSFabric *xi, int server);
> >>>>>>> +    uint32_t (*nr_servers)(XICSFabric *xi);
> >>>>>>>  } XICSFabricClass;
> >>>>>>>  
> >>>>>>>  ICPState *xics_icp_get(XICSFabric *xi, int server);
> >>>>>>> +uint32_t xics_nr_servers(XICSFabric *xi);
> >>>>>>>  
> >>>>>>>  /* Internal XICS interfaces */
> >>>>>>>  void icp_set_cppr(ICPState *icp, uint8_t cppr);
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> > 
>