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 <c...@kaod.org>
> >>>> ---
> >>>> 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
