Re: [Qemu-devel] [PATCH v5 15/36] spapr: introdude a new machine IRQ backend for XIVE

[David Gibson]

On Thu, Nov 29, 2018 at 04:34:51PM +0100, Cédric Le Goater wrote:
> On 11/29/18 2:07 AM, David Gibson wrote:
> > On Wed, Nov 28, 2018 at 06:16:58PM +0100, Cédric Le Goater wrote:
> >> On 11/28/18 4:28 AM, David Gibson wrote:
> >>> On Fri, Nov 16, 2018 at 11:57:08AM +0100, Cédric Le Goater wrote:
> >>>> The XIVE IRQ backend uses the same layout as the new XICS backend but
> >>>> covers the full range of the IRQ number space. The IRQ numbers for the
> >>>> CPU IPIs are allocated at the bottom of this space, below 4K, to
> >>>> preserve compatibility with XICS which does not use that range.
> >>>>
> >>>> This should be enough given that the maximum number of CPUs is 1024
> >>>> for the sPAPR machine under QEMU. For the record, the biggest POWER8
> >>>> or POWER9 system has a maximum of 1536 HW threads (16 sockets, 192
> >>>> cores, SMT8).
> >>>>
> >>>> Signed-off-by: Cédric Le Goater <address@hidden>
> >>>> ---
> >>>>  include/hw/ppc/spapr.h     |   2 +
> >>>>  include/hw/ppc/spapr_irq.h |   7 ++-
> >>>>  hw/ppc/spapr.c             |   2 +-
> >>>>  hw/ppc/spapr_irq.c         | 119 ++++++++++++++++++++++++++++++++++++-
> >>>>  4 files changed, 124 insertions(+), 6 deletions(-)
> >>>>
> >>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >>>> index 6279711fe8f7..1fbc2663e06c 100644
> >>>> --- a/include/hw/ppc/spapr.h
> >>>> +++ b/include/hw/ppc/spapr.h
> >>>> @@ -16,6 +16,7 @@ typedef struct sPAPREventLogEntry sPAPREventLogEntry;
> >>>>  typedef struct sPAPREventSource sPAPREventSource;
> >>>>  typedef struct sPAPRPendingHPT sPAPRPendingHPT;
> >>>>  typedef struct ICSState ICSState;
> >>>> +typedef struct sPAPRXive sPAPRXive;
> >>>>  
> >>>>  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
> >>>>  #define SPAPR_ENTRY_POINT       0x100
> >>>> @@ -175,6 +176,7 @@ struct sPAPRMachineState {
> >>>>      const char *icp_type;
> >>>>      int32_t irq_map_nr;
> >>>>      unsigned long *irq_map;
> >>>> +    sPAPRXive  *xive;
> >>>>  
> >>>>      bool cmd_line_caps[SPAPR_CAP_NUM];
> >>>>      sPAPRCapabilities def, eff, mig;
> >>>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> >>>> index 0e9229bf219e..c854ae527808 100644
> >>>> --- a/include/hw/ppc/spapr_irq.h
> >>>> +++ b/include/hw/ppc/spapr_irq.h
> >>>> @@ -13,6 +13,7 @@
> >>>>  /*
> >>>>   * IRQ range offsets per device type
> >>>>   */
> >>>> +#define SPAPR_IRQ_IPI        0x0
> >>>>  #define SPAPR_IRQ_EPOW       0x1000  /* XICS_IRQ_BASE offset */
> >>>>  #define SPAPR_IRQ_HOTPLUG    0x1001
> >>>>  #define SPAPR_IRQ_VIO        0x1100  /* 256 VIO devices */
> >>>> @@ -33,7 +34,8 @@ typedef struct sPAPRIrq {
> >>>>      uint32_t    nr_irqs;
> >>>>      uint32_t    nr_msis;
> >>>>  
> >>>> -    void (*init)(sPAPRMachineState *spapr, int nr_irqs, Error **errp);
> >>>> +    void (*init)(sPAPRMachineState *spapr, int nr_irqs, int nr_servers,
> >>>> +                 Error **errp);
> >>>>      int (*claim)(sPAPRMachineState *spapr, int irq, bool lsi, Error 
> >>>> **errp);
> >>>>      void (*free)(sPAPRMachineState *spapr, int irq, int num);
> >>>>      qemu_irq (*qirq)(sPAPRMachineState *spapr, int irq);
> >>>> @@ -42,8 +44,9 @@ typedef struct sPAPRIrq {
> >>>>  
> >>>>  extern sPAPRIrq spapr_irq_xics;
> >>>>  extern sPAPRIrq spapr_irq_xics_legacy;
> >>>> +extern sPAPRIrq spapr_irq_xive;
> >>>>  
> >>>> -void spapr_irq_init(sPAPRMachineState *spapr, Error **errp);
> >>>> +void spapr_irq_init(sPAPRMachineState *spapr, int nr_servers, Error 
> >>>> **errp);
> >>>
> >>> I don't see why nr_servers needs to become a parameter, since it can
> >>> be derived from spapr within this routine.
> >>
> >> ok. This is true. We can use directly xics_max_server_number(spapr).
> >>
> >>>>  int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error 
> >>>> **errp);
> >>>>  void spapr_irq_free(sPAPRMachineState *spapr, int irq, int num);
> >>>>  qemu_irq spapr_qirq(sPAPRMachineState *spapr, int irq);
> >>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>>> index e470efe7993c..9f8c19e56e7a 100644
> >>>> --- a/hw/ppc/spapr.c
> >>>> +++ b/hw/ppc/spapr.c
> >>>> @@ -2594,7 +2594,7 @@ static void spapr_machine_init(MachineState 
> >>>> *machine)
> >>>>      spapr_set_vsmt_mode(spapr, &error_fatal);
> >>>>  
> >>>>      /* Set up Interrupt Controller before we create the VCPUs */
> >>>> -    spapr_irq_init(spapr, &error_fatal);
> >>>> +    spapr_irq_init(spapr, xics_max_server_number(spapr), &error_fatal);
> >>>
> >>> We should rename xics_max_server_number() since it's no longer xics
> >>> specific.
> >>
> >> yes.
> >>
> >>>>      /* Set up containers for ibm,client-architecture-support negotiated 
> >>>> options
> >>>>       */
> >>>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> >>>> index bac450ffff23..2569ae1bc7f8 100644
> >>>> --- a/hw/ppc/spapr_irq.c
> >>>> +++ b/hw/ppc/spapr_irq.c
> >>>> @@ -12,6 +12,7 @@
> >>>>  #include "qemu/error-report.h"
> >>>>  #include "qapi/error.h"
> >>>>  #include "hw/ppc/spapr.h"
> >>>> +#include "hw/ppc/spapr_xive.h"
> >>>>  #include "hw/ppc/xics.h"
> >>>>  #include "sysemu/kvm.h"
> >>>>  
> >>>> @@ -91,7 +92,7 @@ error:
> >>>>  }
> >>>>  
> >>>>  static void spapr_irq_init_xics(sPAPRMachineState *spapr, int nr_irqs,
> >>>> -                                Error **errp)
> >>>> +                                int nr_servers, Error **errp)
> >>>>  {
> >>>>      MachineState *machine = MACHINE(spapr);
> >>>>      Error *local_err = NULL;
> >>>> @@ -204,10 +205,122 @@ sPAPRIrq spapr_irq_xics = {
> >>>>      .print_info  = spapr_irq_print_info_xics,
> >>>>  };
> >>>>  
> >>>> + /*
> >>>> + * XIVE IRQ backend.
> >>>> + */
> >>>> +static sPAPRXive *spapr_xive_create(sPAPRMachineState *spapr,
> >>>> +                                    const char *type_xive, int nr_irqs,
> >>>> +                                    int nr_servers, Error **errp)
> >>>> +{
> >>>> +    sPAPRXive *xive;
> >>>> +    Error *local_err = NULL;
> >>>> +    Object *obj;
> >>>> +    uint32_t nr_ends = nr_servers << 3; /* 8 priority ENDs per CPU */
> >>>> +    int i;
> >>>> +
> >>>> +    obj = object_new(type_xive);
> >>>
> >>> What's the reason for making the type a parameter, rather than just
> >>> using the #define here.
> >>
> >> KVM.
> > 
> > Yeah, I realised that when I'd read a few patches further on.  As I
> > commented there, I don't think the separate KVM/TCG subclasses is
> > actually a good pattern to follow.
> 
> I will use the simple pattern in next spin: if (kvm) { } 

Great.

> We might want to do that for XICS also but it would break migratibility.  

Well, if that breaks migration, we already have a problem migrating
between KVM and non-KVM guests (or even KVM-with-irqchip and
KVM-without-irqchip guests).  I think we put the actual migratable
state in the base class to avoid that, but we should check.  If we
ever get time.

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