On 2/12/19 7:24 PM, Greg Kurz wrote:
> Only pseries machines, either recent ones started with ic-mode=xics
> or older ones using the legacy irq allocation scheme, need to set the
> @offset of the ICS to XICS_IRQ_BASE. Recent pseries started with
> ic-mode=dual set it to 0 and powernv machines set it to some other
> value at runtime.
>
> It thus doesn't really help to set the default value of the ICS offset
> to XICS_IRQ_BASE in ics_base_instance_init().
>
> Drop that code from XICS and let the pseries code set the offset
> explicitely for clarity.
Looks OK to me. I would have call it 'offset' and not 'xics_offset'
though, because we might want to create an sPAPR IRQ XIVE device with
some offset one day. There is still some work to be done before that
is possible. Anyhow,
Reviewed-by: Cédric Le Goater <c...@kaod.org>
Thanks,
C.
>
> Signed-off-by: Greg Kurz <gr...@kaod.org>
> ---
> hw/intc/xics.c | 8 --------
> hw/ppc/spapr_irq.c | 33 ++++++++++++++++++++-------------
> include/hw/ppc/spapr_irq.h | 1 +
> 3 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index 16e8ffa2aaf7..7cac138067e2 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -638,13 +638,6 @@ static void ics_base_realize(DeviceState *dev, Error
> **errp)
> ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
> }
>
> -static void ics_base_instance_init(Object *obj)
> -{
> - ICSState *ics = ICS_BASE(obj);
> -
> - ics->offset = XICS_IRQ_BASE;
> -}
> -
> static int ics_base_dispatch_pre_save(void *opaque)
> {
> ICSState *ics = opaque;
> @@ -720,7 +713,6 @@ static const TypeInfo ics_base_info = {
> .parent = TYPE_DEVICE,
> .abstract = true,
> .instance_size = sizeof(ICSState),
> - .instance_init = ics_base_instance_init,
> .class_init = ics_base_class_init,
> .class_size = sizeof(ICSStateClass),
> };
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 80b0083b8e38..8217e0215411 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -68,10 +68,11 @@ void spapr_irq_msi_reset(sPAPRMachineState *spapr)
>
> static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
> const char *type_ics,
> - int nr_irqs, Error **errp)
> + int nr_irqs, int offset, Error **errp)
> {
> Error *local_err = NULL;
> Object *obj;
> + ICSState *ics;
>
> obj = object_new(type_ics);
> object_property_add_child(OBJECT(spapr), "ics", obj, &error_abort);
> @@ -86,7 +87,10 @@ static ICSState *spapr_ics_create(sPAPRMachineState *spapr,
> goto error;
> }
>
> - return ICS_BASE(obj);
> + ics = ICS_BASE(obj);
> + ics->offset = offset;
> +
> + return ics;
>
> error:
> error_propagate(errp, local_err);
> @@ -104,6 +108,7 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr,
> Error **errp)
> !xics_kvm_init(spapr, &local_err)) {
> spapr->icp_type = TYPE_KVM_ICP;
> spapr->ics = spapr_ics_create(spapr, TYPE_ICS_KVM, nr_irqs,
> + spapr->irq->xics_offset,
> &local_err);
> }
> if (machine_kernel_irqchip_required(machine) && !spapr->ics) {
> @@ -119,6 +124,7 @@ static void spapr_irq_init_xics(sPAPRMachineState *spapr,
> Error **errp)
> xics_spapr_init(spapr);
> spapr->icp_type = TYPE_ICP;
> spapr->ics = spapr_ics_create(spapr, TYPE_ICS_SIMPLE, nr_irqs,
> + spapr->irq->xics_offset,
> &local_err);
> }
>
> @@ -246,6 +252,7 @@ sPAPRIrq spapr_irq_xics = {
> .nr_irqs = SPAPR_IRQ_XICS_NR_IRQS,
> .nr_msis = SPAPR_IRQ_XICS_NR_MSIS,
> .ov5 = SPAPR_OV5_XIVE_LEGACY,
> + .xics_offset = XICS_IRQ_BASE,
>
> .init = spapr_irq_init_xics,
> .claim = spapr_irq_claim_xics,
> @@ -451,17 +458,6 @@ static void spapr_irq_init_dual(sPAPRMachineState
> *spapr, Error **errp)
> return;
> }
>
> - /*
> - * Align the XICS and the XIVE IRQ number space under QEMU.
> - *
> - * However, the XICS KVM device still considers that the IRQ
> - * numbers should start at XICS_IRQ_BASE (0x1000). Either we
> - * should introduce a KVM device ioctl to set the offset or ignore
> - * the lower 4K numbers when using the get/set ioctl of the XICS
> - * KVM device. The second option seems the least intrusive.
> - */
> - spapr->ics->offset = 0;
> -
> spapr_irq_xive.init(spapr, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> @@ -582,6 +578,16 @@ sPAPRIrq spapr_irq_dual = {
> .nr_irqs = SPAPR_IRQ_DUAL_NR_IRQS,
> .nr_msis = SPAPR_IRQ_DUAL_NR_MSIS,
> .ov5 = SPAPR_OV5_XIVE_BOTH,
> + /*
> + * Align the XICS and the XIVE IRQ number space under QEMU.
> + *
> + * However, the XICS KVM device still considers that the IRQ
> + * numbers should start at XICS_IRQ_BASE (0x1000). Either we
> + * should introduce a KVM device ioctl to set the offset or ignore
> + * the lower 4K numbers when using the get/set ioctl of the XICS
> + * KVM device. The second option seems the least intrusive.
> + */
> + .xics_offset = 0,
>
> .init = spapr_irq_init_dual,
> .claim = spapr_irq_claim_dual,
> @@ -712,6 +718,7 @@ sPAPRIrq spapr_irq_xics_legacy = {
> .nr_irqs = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
> .nr_msis = SPAPR_IRQ_XICS_LEGACY_NR_IRQS,
> .ov5 = SPAPR_OV5_XIVE_LEGACY,
> + .xics_offset = XICS_IRQ_BASE,
>
> .init = spapr_irq_init_xics,
> .claim = spapr_irq_claim_xics,
> diff --git a/include/hw/ppc/spapr_irq.h b/include/hw/ppc/spapr_irq.h
> index 14b02c3aca33..5e30858dc22a 100644
> --- a/include/hw/ppc/spapr_irq.h
> +++ b/include/hw/ppc/spapr_irq.h
> @@ -34,6 +34,7 @@ typedef struct sPAPRIrq {
> uint32_t nr_irqs;
> uint32_t nr_msis;
> uint8_t ov5;
> + uint32_t xics_offset;
>
> void (*init)(sPAPRMachineState *spapr, Error **errp);
> int (*claim)(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp);
>
