On 26/09/2019 03:03, David Gibson wrote:
> On Wed, Sep 25, 2019 at 09:05:34AM +0200, Cédric Le Goater wrote:
>> On 25/09/2019 08:45, David Gibson wrote:
>>> Both the XICS and XIVE interrupt backends have a "nr-irqs" property, but
>>> it means slightly different things. For XICS (or, strictly, the ICS) it
>>> indicates the number of "real" external IRQs. Those start at XICS_IRQ_BASE
>>> (0x1000) and don't include the special IPI vector. For XIVE, however, it
>>> includes the whole IRQ space, including XIVE's many IPI vectors.
>>>
>>> The spapr code currently doesn't handle this sensibly, with the nr_irqs
>>> value in SpaprIrq having different meanings depending on the backend.
>>> We fix this by renaming nr_irqs to nr_xirqs and making it always indicate
>>> just the number of external irqs, adjusting the value we pass to XIVE
>>> accordingly. We also use move to using common constants in most of the
>>> irq configurations, to make it clearer that the IRQ space looks the same
>>> to the guest (and emulated devices), even if the backend is different.
>>>
>>> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
>>
>> Reviewed-by: Cédric Le Goater <c...@kaod.org>
>>
>> one comment below,
>>
>>> ---
>>> hw/ppc/spapr_irq.c | 48 +++++++++++++++-----------------------
>>> include/hw/ppc/spapr_irq.h | 19 +++++++++------
>>> 2 files changed, 31 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
>>> index 8c26fa2d1e..5190a33e08 100644
>>> --- a/hw/ppc/spapr_irq.c
>>> +++ b/hw/ppc/spapr_irq.c
>>> @@ -92,7 +92,7 @@ static void spapr_irq_init_kvm(SpaprMachineState *spapr,
>>> * XICS IRQ backend.
>>> */
>>>
>>> -static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_irqs,
>>> +static void spapr_irq_init_xics(SpaprMachineState *spapr, int nr_xirqs,
>>> Error **errp)
>>> {
>>> Object *obj;
>>> @@ -102,7 +102,7 @@ static void spapr_irq_init_xics(SpaprMachineState
>>> *spapr, int nr_irqs,
>>> object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
>>> object_property_add_const_link(obj, ICS_PROP_XICS, OBJECT(spapr),
>>> &error_fatal);
>>> - object_property_set_int(obj, nr_irqs, "nr-irqs", &error_fatal);
>>> + object_property_set_int(obj, nr_xirqs, "nr-irqs", &error_fatal);
>>> object_property_set_bool(obj, true, "realized", &local_err);
>>> if (local_err) {
>>> error_propagate(errp, local_err);
>>> @@ -234,13 +234,9 @@ static void spapr_irq_init_kvm_xics(SpaprMachineState
>>> *spapr, Error **errp)
>>> }
>>> }
>>>
>>> -#define SPAPR_IRQ_XICS_NR_IRQS 0x1000
>>> -#define SPAPR_IRQ_XICS_NR_MSIS \
>>> - (XICS_IRQ_BASE + SPAPR_IRQ_XICS_NR_IRQS - SPAPR_IRQ_MSI)
>>> -
>>> SpaprIrq spapr_irq_xics = {
>>> - .nr_irqs = SPAPR_IRQ_XICS_NR_IRQS,
>>> - .nr_msis = SPAPR_IRQ_XICS_NR_MSIS,
>>> + .nr_xirqs = SPAPR_NR_XIRQS,
>>> + .nr_msis = SPAPR_NR_MSIS,
>>> .ov5 = SPAPR_OV5_XIVE_LEGACY,
>>>
>>> .init = spapr_irq_init_xics,
>>> @@ -260,7 +256,7 @@ SpaprIrq spapr_irq_xics = {
>>> /*
>>> * XIVE IRQ backend.
>>> */
>>> -static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_irqs,
>>> +static void spapr_irq_init_xive(SpaprMachineState *spapr, int nr_xirqs,
>>> Error **errp)
>>> {
>>> uint32_t nr_servers = spapr_max_server_number(spapr);
>>> @@ -268,7 +264,7 @@ static void spapr_irq_init_xive(SpaprMachineState
>>> *spapr, int nr_irqs,
>>> int i;
>>>
>>> dev = qdev_create(NULL, TYPE_SPAPR_XIVE);
>>> - qdev_prop_set_uint32(dev, "nr-irqs", nr_irqs);
>>> + qdev_prop_set_uint32(dev, "nr-irqs", nr_xirqs + SPAPR_XIRQ_BASE);
>>> /*
>>> * 8 XIVE END structures per CPU. One for each available priority
>>> */
>>> @@ -308,7 +304,7 @@ static qemu_irq spapr_qirq_xive(SpaprMachineState
>>> *spapr, int irq)
>>> {
>>> SpaprXive *xive = spapr->xive;
>>>
>>> - if (irq >= xive->nr_irqs) {
>>> + if ((irq < SPAPR_XIRQ_BASE) || (irq >= xive->nr_irqs)) {
>>
>> So IPIs cannot be pulsed ? I think that is OK in QEMU.
>
> They can be pulsed, they just can't be retrieved via the spapr_qirq()
> interface. Since that interface basically exists for the spapr root
> devices (VIO and PHBs) to find the qemu_irqs to wire themselves up to,
> I think that's fine.
>
> If we discover some reason we need to grab IPI qirqs by global number
> then we can revisit this.
Not from QEMU I think. This is fine.
C.
>
> I'll add a comment to clarify this in the later patch where I unify
> the qirq implementations.
>
>> XIVE unifies all the interrupts at the controller level. Any one can trigger
>> an interrupt with a store on the associate ESB page.
>
> Absolutely, and nothing's stopping them.
>>
>>> return NULL;
>>> }
>>>
>>> @@ -409,12 +405,9 @@ static void spapr_irq_init_kvm_xive(SpaprMachineState
>>> *spapr, Error **errp)
>>> * 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,
>>> + .nr_xirqs = SPAPR_NR_XIRQS,
>>> + .nr_msis = SPAPR_NR_MSIS,
>>> .ov5 = SPAPR_OV5_XIVE_EXPLOIT,
>>>
>>> .init = spapr_irq_init_xive,
>>> @@ -450,18 +443,18 @@ static SpaprIrq *spapr_irq_current(SpaprMachineState
>>> *spapr)
>>> &spapr_irq_xive : &spapr_irq_xics;
>>> }
>>>
>>> -static void spapr_irq_init_dual(SpaprMachineState *spapr, int nr_irqs,
>>> +static void spapr_irq_init_dual(SpaprMachineState *spapr, int nr_xirqs,
>>> Error **errp)
>>> {
>>> Error *local_err = NULL;
>>>
>>> - spapr_irq_xics.init(spapr, spapr_irq_xics.nr_irqs, &local_err);
>>> + spapr_irq_xics.init(spapr, spapr_irq_xics.nr_xirqs, &local_err);
>>> if (local_err) {
>>> error_propagate(errp, local_err);
>>> return;
>>> }
>>>
>>> - spapr_irq_xive.init(spapr, spapr_irq_xive.nr_irqs, &local_err);
>>> + spapr_irq_xive.init(spapr, spapr_irq_xive.nr_xirqs, &local_err);
>>> if (local_err) {
>>> error_propagate(errp, local_err);
>>> return;
>>> @@ -586,12 +579,9 @@ static const char
>>> *spapr_irq_get_nodename_dual(SpaprMachineState *spapr)
>>> /*
>>> * Define values in sync with the XIVE and XICS backend
>>> */
>>> -#define SPAPR_IRQ_DUAL_NR_IRQS 0x2000
>>> -#define SPAPR_IRQ_DUAL_NR_MSIS (SPAPR_IRQ_DUAL_NR_IRQS - SPAPR_IRQ_MSI)
>>> -
>>> SpaprIrq spapr_irq_dual = {
>>> - .nr_irqs = SPAPR_IRQ_DUAL_NR_IRQS,
>>> - .nr_msis = SPAPR_IRQ_DUAL_NR_MSIS,
>>> + .nr_xirqs = SPAPR_NR_XIRQS,
>>> + .nr_msis = SPAPR_NR_MSIS,
>>> .ov5 = SPAPR_OV5_XIVE_BOTH,
>>>
>>> .init = spapr_irq_init_dual,
>>> @@ -693,10 +683,10 @@ void spapr_irq_init(SpaprMachineState *spapr, Error
>>> **errp)
>>> spapr_irq_msi_init(spapr, spapr->irq->nr_msis);
>>> }
>>>
>>> - spapr->irq->init(spapr, spapr->irq->nr_irqs, errp);
>>> + spapr->irq->init(spapr, spapr->irq->nr_xirqs, errp);
>>>
>>> spapr->qirqs = qemu_allocate_irqs(spapr->irq->set_irq, spapr,
>>> - spapr->irq->nr_irqs);
>>> + spapr->irq->nr_xirqs +
>>> SPAPR_XIRQ_BASE);
>>> }
>>>
>>> int spapr_irq_claim(SpaprMachineState *spapr, int irq, bool lsi, Error
>>> **errp)
>>> @@ -804,11 +794,11 @@ int spapr_irq_find(SpaprMachineState *spapr, int num,
>>> bool align, Error **errp)
>>> return first + ics->offset;
>>> }
>>>
>>> -#define SPAPR_IRQ_XICS_LEGACY_NR_IRQS 0x400
>>> +#define SPAPR_IRQ_XICS_LEGACY_NR_XIRQS 0x400
>>>
>>> SpaprIrq spapr_irq_xics_legacy = {
>>> - .nr_irqs = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
>>> - .nr_msis = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
>>> + .nr_xirqs = SPAPR_IRQ_XICS_LEGACY_NR_XIRQS,
>>> + .nr_msis = SPAPR_IRQ_XICS_LEGACY_NR_XIRQS,
>>> .ov5 = SPAPR_OV5_XIVE_LEGACY,
>>>
>>> .init = spapr_irq_init_xics,
>>> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
>>> index 5db305165c..a8f9a2ab11 100644
>>> --- a/include/hw/ppc/spapr_irq.h
>>> +++ b/include/hw/ppc/spapr_irq.h
>>> @@ -16,13 +16,18 @@
>>> * 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 */
>>> -#define SPAPR_IRQ_PCI_LSI 0x1200 /* 32+ PHBs devices */
>>>
>>> -#define SPAPR_IRQ_MSI 0x1300 /* Offset of the dynamic range covered
>>> - * by the bitmap allocator */
>>> +#define SPAPR_XIRQ_BASE XICS_IRQ_BASE /* 0x1000 */
>>> +#define SPAPR_IRQ_EPOW (SPAPR_XIRQ_BASE + 0x0000)
>>> +#define SPAPR_IRQ_HOTPLUG (SPAPR_XIRQ_BASE + 0x0001)
>>> +#define SPAPR_IRQ_VIO (SPAPR_XIRQ_BASE + 0x0100) /* 256 VIO
>>> devices */
>>> +#define SPAPR_IRQ_PCI_LSI (SPAPR_XIRQ_BASE + 0x0200) /* 32+ PHBs
>>> devices */
>>> +
>>> +/* Offset of the dynamic range covered by the bitmap allocator */
>>> +#define SPAPR_IRQ_MSI (SPAPR_XIRQ_BASE + 0x0300)
>>> +
>>> +#define SPAPR_NR_XIRQS 0x1000
>>> +#define SPAPR_NR_MSIS (SPAPR_XIRQ_BASE + SPAPR_NR_XIRQS -
>>> SPAPR_IRQ_MSI)
>>>
>>> typedef struct SpaprMachineState SpaprMachineState;
>>>
>>> @@ -32,7 +37,7 @@ int spapr_irq_msi_alloc(SpaprMachineState *spapr,
>>> uint32_t num, bool align,
>>> void spapr_irq_msi_free(SpaprMachineState *spapr, int irq, uint32_t num);
>>>
>>> typedef struct SpaprIrq {
>>> - uint32_t nr_irqs;
>>> + uint32_t nr_xirqs;
>>> uint32_t nr_msis;
>>> uint8_t ov5;
>>>
>>>
>>
>
