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) { }
We might want to do that for XICS also but it would break migratibility.
>>>> + object_property_set_int(obj, nr_irqs, "nr-irqs", &error_abort);
>>>> + object_property_set_int(obj, nr_ends, "nr-ends", &error_abort);
>>>
>>> This is still within the sPAPR code, and you have a pointer to the
>>> MachineState, so I don't see why you could't just derive nr_irqs and
>>> nr_servers from that, rather than having them passed in.
>>
>> for nr_servers I agree. nr_irqs comes from the machine class and it will
>> not make any sense using the machine class in the init routine of the
>> 'dual' sPAPR IRQ backend supporting both modes. See patch 34 which
>> initializes both backend for the 'dual' machine.
>
> Uh.. I guess I'll comment when I get to that patch, but I don't see
> why accessing the machine class would be a problem. If we have the
> MachineState we can get to the MachineClass.>
>>>> + object_property_set_bool(obj, true, "realized", &local_err);
>>>> + if (local_err) {
>>>> + error_propagate(errp, local_err);
>>>> + return NULL;
>>>> + }
>>>> + qdev_set_parent_bus(DEVICE(obj), sysbus_get_default());
>>>
>>> Whereas the XiveSource and XiveRouter I think make more sense as
>>> "device components" rather than SysBusDevice subclasses,
>>
>> Yes. I changed that.
>>
>>> I think it
>>> *does* make sense for the PAPR-XIVE object to be a full fledged
>>> SysBusDevice.
>>
>> Ah. That I didn't do but thinking of it, it makes sense as it is the
>> object managing the TIMA and ESB memory region mapping for the machine.
>>
>>> And for that reason, I think it makes more sense to create it with
>>> qdev_create(), which should avoid having to manually fiddle with the
>>> parent bus.
>>
>> OK. I will give it a try.
>>
>>>> + xive = SPAPR_XIVE(obj);
>>>> +
>>>> + /* Enable the CPU IPIs */
>>>> + for (i = 0; i < nr_servers; ++i) {
>>>> + spapr_xive_irq_enable(xive, SPAPR_IRQ_IPI + i, false);
>>>
>>> This comment possibly belonged on an earlier patch. I don't love the
>>> "..._enable" name - to me that suggests something runtime rather than
>>> configuration time. A better option isn't quickly occurring to me
>>> though :/.
>>
>> Instead, I could call the sPAPR IRQ claim method :
>>
>> for (i = 0; i < nr_servers; ++i) {
>> spapr_irq_xive.claim(spapr, SPAPR_IRQ_IPI + i, false, &local_err);
>> }
>>
>>
>> What it does is to set the EAS_VALID bit in the EAT (it also sets the
>> LSI bit). what about :
>>
>> spapr_xive_irq_validate()
>> spapr_xive_irq_invalidate()
>>
>> or to map the sPAPR IRQ backend names :
>>
>> spapr_xive_irq_claim()
>> spapr_xive_irq_free()
>
> Let's use claim/free to match the terms spapr already uses.
OK.
Thanks,
C.
>
>
>>
>>
>>>
>>>> + }
>>>> +
>>>> + return xive;
>>>> +}
>>>> +
>>>> +static void spapr_irq_init_xive(sPAPRMachineState *spapr, int nr_irqs,
>>>> + int nr_servers, Error **errp)
>>>> +{
>>>> + MachineState *machine = MACHINE(spapr);
>>>> + Error *local_err = NULL;
>>>> +
>>>> + /* KVM XIVE support */
>>>> + if (kvm_enabled()) {
>>>> + if (machine_kernel_irqchip_required(machine)) {
>>>> + error_setg(errp, "kernel_irqchip requested. no XIVE support");
>>>> + return;
>>>> + }
>>>> + }
>>>> +
>>>> + /* QEMU XIVE support */
>>>> + spapr->xive = spapr_xive_create(spapr, TYPE_SPAPR_XIVE, nr_irqs,
>>>> nr_servers,
>>>> + &local_err);
>>>> + if (local_err) {
>>>> + error_propagate(errp, local_err);
>>>> + return;
>>>> + }
>>>> +}
>>>> +
>>>> +static int spapr_irq_claim_xive(sPAPRMachineState *spapr, int irq, bool
>>>> lsi,
>>>> + Error **errp)
>>>> +{
>>>> + if (!spapr_xive_irq_enable(spapr->xive, irq, lsi)) {
>>>> + error_setg(errp, "IRQ %d is invalid", irq);
>>>> + return -1;
>>>> + }
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void spapr_irq_free_xive(sPAPRMachineState *spapr, int irq, int
>>>> num)
>>>> +{
>>>> + int i;
>>>> +
>>>> + for (i = irq; i < irq + num; ++i) {
>>>> + spapr_xive_irq_disable(spapr->xive, i);
>>>> + }
>>>> +}
>>>> +
>>>> +static qemu_irq spapr_qirq_xive(sPAPRMachineState *spapr, int irq)
>>>> +{
>>>> + return spapr_xive_qirq(spapr->xive, irq);
>>>> +}
>>>> +
>>>> +static void spapr_irq_print_info_xive(sPAPRMachineState *spapr,
>>>> + Monitor *mon)
>>>> +{
>>>> + CPUState *cs;
>>>> +
>>>> + CPU_FOREACH(cs) {
>>>> + PowerPCCPU *cpu = POWERPC_CPU(cs);
>>>> +
>>>> + xive_tctx_pic_print_info(XIVE_TCTX(cpu->intc), mon);
>>>> + }
>>>> +
>>>> + spapr_xive_pic_print_info(spapr->xive, mon);
>>>
>>> Any reason the info dumping routines are split into two?
>>
>> Not the same objects. Are you suggesting that we could print all the info
>> from the sPAPR XIVE model ? including the XiveTCTX. I thought of doing
>> that also. Fine for me if it's ok for you.
>
> Ah.. I think I got xive_pic_print_info() and
> xive_tctx_pic_print_info() mixed up. Never mind.
>
>>
>> Thanks,
>>
>> C.
>>
>>>
>>>> +}
>>>> +
>>>> +/*
>>>> + * XIVE uses the full IRQ number space. Set it to 8K to be compatible
>>>> + * with XICS.
>>>> + */
>>>> +
>>>> +#define SPAPR_IRQ_XIVE_NR_IRQS 0x2000
>>>> +#define SPAPR_IRQ_XIVE_NR_MSIS (SPAPR_IRQ_XIVE_NR_IRQS -
>>>> SPAPR_IRQ_MSI)
>>>> +
>>>> +sPAPRIrq spapr_irq_xive = {
>>>> + .nr_irqs = SPAPR_IRQ_XIVE_NR_IRQS,
>>>> + .nr_msis = SPAPR_IRQ_XIVE_NR_MSIS,
>>>> +
>>>> + .init = spapr_irq_init_xive,
>>>> + .claim = spapr_irq_claim_xive,
>>>> + .free = spapr_irq_free_xive,
>>>> + .qirq = spapr_qirq_xive,
>>>> + .print_info = spapr_irq_print_info_xive,
>>>> +};
>>>> +
>>>> /*
>>>> * sPAPR IRQ frontend routines for devices
>>>> */
>>>> -void spapr_irq_init(sPAPRMachineState *spapr, Error **errp)
>>>> +void spapr_irq_init(sPAPRMachineState *spapr, int nr_servers, Error
>>>> **errp)
>>>> {
>>>> sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>>>>
>>>> @@ -216,7 +329,7 @@ void spapr_irq_init(sPAPRMachineState *spapr, Error
>>>> **errp)
>>>> spapr_irq_msi_init(spapr, smc->irq->nr_msis);
>>>> }
>>>>
>>>> - smc->irq->init(spapr, smc->irq->nr_irqs, errp);
>>>> + smc->irq->init(spapr, smc->irq->nr_irqs, nr_servers, errp);
>>>> }
>>>>
>>>> int spapr_irq_claim(sPAPRMachineState *spapr, int irq, bool lsi, Error
>>>> **errp)
>>>
>>
>
