On Wed, 13 Feb 2019 14:26:01 +1100 David Gibson <da...@gibson.dropbear.id.au> wrote:
> On Tue, Feb 12, 2019 at 07:24:00PM +0100, 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. > > > > Signed-off-by: Greg Kurz <gr...@kaod.org> > > So this actually relates to a discussion I've had on some of Cédric's > more recent patches. Changing the ics offset in ic-mode=dual doesn't > make sense to me. The global (guest) interrupt numbers need to match > between XICS and XIVE, but the global interrupt numbers don't have to > match the ICS source numbers, which is what ics->offset is about. > Yeah. We'll see what comes out of the discussion at: https://patchwork.ozlabs.org/patch/1021496/ > > --- > > 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); > > >
pgpSyZy3LvwVC.pgp
Description: OpenPGP digital signature