| Message ID | 20170911171235.29331-7-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | Guest exploitation of the XIVE interrupt controller (POWER9) | expand |
On Mon, Sep 11, 2017 at 07:12:20PM +0200, Cédric Le Goater wrote: > These are very similar to the XICS handlers in a simpler form. They > make use of the ICSIRQState array of the XICS interrupt source to > differentiate the MSI from the LSI interrupts. The spapr_xive_irq() > routine in charge of triggering the CPU interrupt line will be filled > later on. > > The next patch will introduce the MMIO handlers to interact with XIVE > interrupt sources. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/intc/spapr_xive.c | 46 +++++++++++++++++++++++++++++++++++++++++++++ > include/hw/ppc/spapr_xive.h | 1 + > 2 files changed, 47 insertions(+) > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > index 52c32f588d6d..1ed7b6a286e9 100644 > --- a/hw/intc/spapr_xive.c > +++ b/hw/intc/spapr_xive.c > @@ -27,6 +27,50 @@ > > #include "xive-internal.h" > > +static void spapr_xive_irq(sPAPRXive *xive, int srcno) > +{ > + > +} > + > +/* > + * XIVE Interrupt Source > + */ > +static void spapr_xive_source_set_irq_msi(sPAPRXive *xive, int srcno, int val) > +{ > + if (val) { > + spapr_xive_irq(xive, srcno); > + } > +} So in XICS "srcno" (vs "irq") indicates an offset within a single ICS object, as opposed to a global irq number. Does that concept even exist in XIVE? > + > +static void spapr_xive_source_set_irq_lsi(sPAPRXive *xive, int srcno, int val) > +{ > + ICSIRQState *irq = &xive->ics->irqs[srcno]; > + > + if (val) { > + irq->status |= XICS_STATUS_ASSERTED; > + } else { > + irq->status &= ~XICS_STATUS_ASSERTED; More mangling a XICS specific object for XIVE operations. Please stop. > + } > + > + if (irq->status & XICS_STATUS_ASSERTED > + && !(irq->status & XICS_STATUS_SENT)) { > + irq->status |= XICS_STATUS_SENT; > + spapr_xive_irq(xive, srcno); > + } > +} > + > +static void spapr_xive_source_set_irq(void *opaque, int srcno, int val) > +{ > + sPAPRXive *xive = SPAPR_XIVE(opaque); > + ICSIRQState *irq = &xive->ics->irqs[srcno]; > + > + if (irq->flags & XICS_FLAGS_IRQ_LSI) { > + spapr_xive_source_set_irq_lsi(xive, srcno, val); > + } else { > + spapr_xive_source_set_irq_msi(xive, srcno, val); > + } > +} > + > /* > * Main XIVE object > */ > @@ -80,6 +124,8 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) > } > > xive->ics = ICS_BASE(obj); > + xive->qirqs = qemu_allocate_irqs(spapr_xive_source_set_irq, xive, > + xive->nr_irqs); > > /* Allocate the last IRQ numbers for the IPIs */ > for (i = xive->nr_irqs - xive->nr_targets; i < xive->nr_irqs; i++) { > diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h > index 29112589b37f..eab92c4c1bb8 100644 > --- a/include/hw/ppc/spapr_xive.h > +++ b/include/hw/ppc/spapr_xive.h > @@ -38,6 +38,7 @@ struct sPAPRXive { > > /* IRQ */ > ICSState *ics; /* XICS source inherited from the SPAPR machine */ > + qemu_irq *qirqs; > > /* XIVE internal tables */ > uint8_t *sbe;
On 09/19/2017 04:48 AM, David Gibson wrote: > On Mon, Sep 11, 2017 at 07:12:20PM +0200, Cédric Le Goater wrote: >> These are very similar to the XICS handlers in a simpler form. They >> make use of the ICSIRQState array of the XICS interrupt source to >> differentiate the MSI from the LSI interrupts. The spapr_xive_irq() >> routine in charge of triggering the CPU interrupt line will be filled >> later on. >> >> The next patch will introduce the MMIO handlers to interact with XIVE >> interrupt sources. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/intc/spapr_xive.c | 46 +++++++++++++++++++++++++++++++++++++++++++++ >> include/hw/ppc/spapr_xive.h | 1 + >> 2 files changed, 47 insertions(+) >> >> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c >> index 52c32f588d6d..1ed7b6a286e9 100644 >> --- a/hw/intc/spapr_xive.c >> +++ b/hw/intc/spapr_xive.c >> @@ -27,6 +27,50 @@ >> >> #include "xive-internal.h" >> >> +static void spapr_xive_irq(sPAPRXive *xive, int srcno) >> +{ >> + >> +} >> + >> +/* >> + * XIVE Interrupt Source >> + */ >> +static void spapr_xive_source_set_irq_msi(sPAPRXive *xive, int srcno, int val) >> +{ >> + if (val) { >> + spapr_xive_irq(xive, srcno); >> + } >> +} > > So in XICS "srcno" (vs "irq") indicates an offset within a single ICS > object, as opposed to a global irq number. Does that concept even > exist in XIVE? We don't really care in the internals. 'srcno' is just an index in the tables, may be I should change the name. It could be the same in XICS but the xirr is manipulated at low level and so we need to propagate the source offset in a couple of places. This to say that the 'irq' number is a guest level information which in the patchset should only be used at the hcall level to identify a source. >> + >> +static void spapr_xive_source_set_irq_lsi(sPAPRXive *xive, int srcno, int val) >> +{ >> + ICSIRQState *irq = &xive->ics->irqs[srcno]; >> + >> + if (val) { >> + irq->status |= XICS_STATUS_ASSERTED; >> + } else { >> + irq->status &= ~XICS_STATUS_ASSERTED; > > More mangling a XICS specific object for XIVE operations. Please > stop. ah ! we will still need the same information and that means introducing a common source object. The patchset today just uses the XICS ICSIRQState array as a common object. >> + } >> + >> + if (irq->status & XICS_STATUS_ASSERTED >> + && !(irq->status & XICS_STATUS_SENT)) { >> + irq->status |= XICS_STATUS_SENT; >> + spapr_xive_irq(xive, srcno); >> + } >> +} >> + >> +static void spapr_xive_source_set_irq(void *opaque, int srcno, int val) >> +{ >> + sPAPRXive *xive = SPAPR_XIVE(opaque); >> + ICSIRQState *irq = &xive->ics->irqs[srcno]; >> + >> + if (irq->flags & XICS_FLAGS_IRQ_LSI) { >> + spapr_xive_source_set_irq_lsi(xive, srcno, val); >> + } else { >> + spapr_xive_source_set_irq_msi(xive, srcno, val); >> + } >> +} >> + >> /* >> * Main XIVE object >> */ >> @@ -80,6 +124,8 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) >> } >> >> xive->ics = ICS_BASE(obj); >> + xive->qirqs = qemu_allocate_irqs(spapr_xive_source_set_irq, xive, >> + xive->nr_irqs); >> >> /* Allocate the last IRQ numbers for the IPIs */ >> for (i = xive->nr_irqs - xive->nr_targets; i < xive->nr_irqs; i++) { >> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h >> index 29112589b37f..eab92c4c1bb8 100644 >> --- a/include/hw/ppc/spapr_xive.h >> +++ b/include/hw/ppc/spapr_xive.h >> @@ -38,6 +38,7 @@ struct sPAPRXive { >> >> /* IRQ */ >> ICSState *ics; /* XICS source inherited from the SPAPR machine */ >> + qemu_irq *qirqs; >> >> /* XIVE internal tables */ >> uint8_t *sbe; >
On Tue, Sep 19, 2017 at 05:08:21PM +0200, Cédric Le Goater wrote: > On 09/19/2017 04:48 AM, David Gibson wrote: > > On Mon, Sep 11, 2017 at 07:12:20PM +0200, Cédric Le Goater wrote: > >> These are very similar to the XICS handlers in a simpler form. They > >> make use of the ICSIRQState array of the XICS interrupt source to > >> differentiate the MSI from the LSI interrupts. The spapr_xive_irq() > >> routine in charge of triggering the CPU interrupt line will be filled > >> later on. > >> > >> The next patch will introduce the MMIO handlers to interact with XIVE > >> interrupt sources. > >> > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > >> --- > >> hw/intc/spapr_xive.c | 46 +++++++++++++++++++++++++++++++++++++++++++++ > >> include/hw/ppc/spapr_xive.h | 1 + > >> 2 files changed, 47 insertions(+) > >> > >> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > >> index 52c32f588d6d..1ed7b6a286e9 100644 > >> --- a/hw/intc/spapr_xive.c > >> +++ b/hw/intc/spapr_xive.c > >> @@ -27,6 +27,50 @@ > >> > >> #include "xive-internal.h" > >> > >> +static void spapr_xive_irq(sPAPRXive *xive, int srcno) > >> +{ > >> + > >> +} > >> + > >> +/* > >> + * XIVE Interrupt Source > >> + */ > >> +static void spapr_xive_source_set_irq_msi(sPAPRXive *xive, int srcno, int val) > >> +{ > >> + if (val) { > >> + spapr_xive_irq(xive, srcno); > >> + } > >> +} > > > > So in XICS "srcno" (vs "irq") indicates an offset within a single ICS > > object, as opposed to a global irq number. Does that concept even > > exist in XIVE? > > We don't really care in the internals. 'srcno' is just an index in the > tables, may be I should change the name. It could be the same in XICS > but the xirr is manipulated at low level and so we need to propagate > the source offset in a couple of places. Right. My point is that the XICS code deliberately uses srcno vs. irq names to identify which space we're talking about. If we re-use the srcno name in XIVE where it doesn't really apply that could be misleading. > This to say that the 'irq' number is a guest level information which > in the patchset should only be used at the hcall level to identify > a source. Right, and if there's no need to introduce a number space other than the guest one, we should keep using that everywhere - and give it a consistent name to avoid confusion. > > >> + > >> +static void spapr_xive_source_set_irq_lsi(sPAPRXive *xive, int srcno, int val) > >> +{ > >> + ICSIRQState *irq = &xive->ics->irqs[srcno]; > >> + > >> + if (val) { > >> + irq->status |= XICS_STATUS_ASSERTED; > >> + } else { > >> + irq->status &= ~XICS_STATUS_ASSERTED; > > > > More mangling a XICS specific object for XIVE operations. Please > > stop. > > ah ! we will still need the same information and that means introducing > a common source object. The patchset today just uses the XICS ICSIRQState > array as a common object. It's not really the same information though. For XICS irq->status is *all* the information about the line's state, for XIVE, most of that info is in the PQ bits which are elsewhere. That makes at least some of the information in ICSIRQState redundant, and therefore confusing and misleading. > >> + } > >> + > >> + if (irq->status & XICS_STATUS_ASSERTED > >> + && !(irq->status & XICS_STATUS_SENT)) { > >> + irq->status |= XICS_STATUS_SENT; > >> + spapr_xive_irq(xive, srcno); > >> + } > >> +} > >> + > >> +static void spapr_xive_source_set_irq(void *opaque, int srcno, int val) > >> +{ > >> + sPAPRXive *xive = SPAPR_XIVE(opaque); > >> + ICSIRQState *irq = &xive->ics->irqs[srcno]; > >> + > >> + if (irq->flags & XICS_FLAGS_IRQ_LSI) { > >> + spapr_xive_source_set_irq_lsi(xive, srcno, val); > >> + } else { > >> + spapr_xive_source_set_irq_msi(xive, srcno, val); > >> + } > >> +} > >> + > >> /* > >> * Main XIVE object > >> */ > >> @@ -80,6 +124,8 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) > >> } > >> > >> xive->ics = ICS_BASE(obj); > >> + xive->qirqs = qemu_allocate_irqs(spapr_xive_source_set_irq, xive, > >> + xive->nr_irqs); > >> > >> /* Allocate the last IRQ numbers for the IPIs */ > >> for (i = xive->nr_irqs - xive->nr_targets; i < xive->nr_irqs; i++) { > >> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h > >> index 29112589b37f..eab92c4c1bb8 100644 > >> --- a/include/hw/ppc/spapr_xive.h > >> +++ b/include/hw/ppc/spapr_xive.h > >> @@ -38,6 +38,7 @@ struct sPAPRXive { > >> > >> /* IRQ */ > >> ICSState *ics; /* XICS source inherited from the SPAPR machine */ > >> + qemu_irq *qirqs; > >> > >> /* XIVE internal tables */ > >> uint8_t *sbe; > > >
On 09/20/2017 06:38 AM, David Gibson wrote: > On Tue, Sep 19, 2017 at 05:08:21PM +0200, Cédric Le Goater wrote: >> On 09/19/2017 04:48 AM, David Gibson wrote: >>> On Mon, Sep 11, 2017 at 07:12:20PM +0200, Cédric Le Goater wrote: >>>> These are very similar to the XICS handlers in a simpler form. They >>>> make use of the ICSIRQState array of the XICS interrupt source to >>>> differentiate the MSI from the LSI interrupts. The spapr_xive_irq() >>>> routine in charge of triggering the CPU interrupt line will be filled >>>> later on. >>>> >>>> The next patch will introduce the MMIO handlers to interact with XIVE >>>> interrupt sources. >>>> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>> --- >>>> hw/intc/spapr_xive.c | 46 +++++++++++++++++++++++++++++++++++++++++++++ >>>> include/hw/ppc/spapr_xive.h | 1 + >>>> 2 files changed, 47 insertions(+) >>>> >>>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c >>>> index 52c32f588d6d..1ed7b6a286e9 100644 >>>> --- a/hw/intc/spapr_xive.c >>>> +++ b/hw/intc/spapr_xive.c >>>> @@ -27,6 +27,50 @@ >>>> >>>> #include "xive-internal.h" >>>> >>>> +static void spapr_xive_irq(sPAPRXive *xive, int srcno) >>>> +{ >>>> + >>>> +} >>>> + >>>> +/* >>>> + * XIVE Interrupt Source >>>> + */ >>>> +static void spapr_xive_source_set_irq_msi(sPAPRXive *xive, int srcno, int val) >>>> +{ >>>> + if (val) { >>>> + spapr_xive_irq(xive, srcno); >>>> + } >>>> +} >>> >>> So in XICS "srcno" (vs "irq") indicates an offset within a single ICS >>> object, as opposed to a global irq number. Does that concept even >>> exist in XIVE? >> >> We don't really care in the internals. 'srcno' is just an index in the >> tables, may be I should change the name. It could be the same in XICS >> but the xirr is manipulated at low level and so we need to propagate >> the source offset in a couple of places. > > Right. My point is that the XICS code deliberately uses srcno vs. irq > names to identify which space we're talking about. If we re-use the > srcno name in XIVE where it doesn't really apply that could be > misleading. yes. ok I will be careful with the naming. >> This to say that the 'irq' number is a guest level information which >> in the patchset should only be used at the hcall level to identify >> a source. > > Right, and if there's no need to introduce a number space other than > the guest one, we should keep using that everywhere - and give it a > consistent name to avoid confusion. yes. I agree. I think XICS could benefit from some cleanups. >>>> + >>>> +static void spapr_xive_source_set_irq_lsi(sPAPRXive *xive, int srcno, int val) >>>> +{ >>>> + ICSIRQState *irq = &xive->ics->irqs[srcno]; >>>> + >>>> + if (val) { >>>> + irq->status |= XICS_STATUS_ASSERTED; >>>> + } else { >>>> + irq->status &= ~XICS_STATUS_ASSERTED; >>> >>> More mangling a XICS specific object for XIVE operations. Please >>> stop. >> >> ah ! we will still need the same information and that means introducing >> a common source object. The patchset today just uses the XICS ICSIRQState >> array as a common object. > > It's not really the same information though. For XICS irq->status is > *all* the information about the line's state, for XIVE, most of that > info is in the PQ bits which are elsewhere. This is true. > That makes at least some of the information in ICSIRQState redundant,> and therefore confusing and misleading. I will respin the patchset in a different way to distinguish xive from xics clearly. I will keep CAS and migration for later. The source should not be too complex to handle but I don't know for the ICP. Thanks, C. >>>> + } >>>> + >>>> + if (irq->status & XICS_STATUS_ASSERTED >>>> + && !(irq->status & XICS_STATUS_SENT)) { >>>> + irq->status |= XICS_STATUS_SENT; >>>> + spapr_xive_irq(xive, srcno); >>>> + } >>>> +} >>>> + >>>> +static void spapr_xive_source_set_irq(void *opaque, int srcno, int val) >>>> +{ >>>> + sPAPRXive *xive = SPAPR_XIVE(opaque); >>>> + ICSIRQState *irq = &xive->ics->irqs[srcno]; >>>> + >>>> + if (irq->flags & XICS_FLAGS_IRQ_LSI) { >>>> + spapr_xive_source_set_irq_lsi(xive, srcno, val); >>>> + } else { >>>> + spapr_xive_source_set_irq_msi(xive, srcno, val); >>>> + } >>>> +} >>>> + >>>> /* >>>> * Main XIVE object >>>> */ >>>> @@ -80,6 +124,8 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) >>>> } >>>> >>>> xive->ics = ICS_BASE(obj); >>>> + xive->qirqs = qemu_allocate_irqs(spapr_xive_source_set_irq, xive, >>>> + xive->nr_irqs); >>>> >>>> /* Allocate the last IRQ numbers for the IPIs */ >>>> for (i = xive->nr_irqs - xive->nr_targets; i < xive->nr_irqs; i++) { >>>> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h >>>> index 29112589b37f..eab92c4c1bb8 100644 >>>> --- a/include/hw/ppc/spapr_xive.h >>>> +++ b/include/hw/ppc/spapr_xive.h >>>> @@ -38,6 +38,7 @@ struct sPAPRXive { >>>> >>>> /* IRQ */ >>>> ICSState *ics; /* XICS source inherited from the SPAPR machine */ >>>> + qemu_irq *qirqs; >>>> >>>> /* XIVE internal tables */ >>>> uint8_t *sbe; >>> >> >
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c index 52c32f588d6d..1ed7b6a286e9 100644 --- a/hw/intc/spapr_xive.c +++ b/hw/intc/spapr_xive.c @@ -27,6 +27,50 @@ #include "xive-internal.h" +static void spapr_xive_irq(sPAPRXive *xive, int srcno) +{ + +} + +/* + * XIVE Interrupt Source + */ +static void spapr_xive_source_set_irq_msi(sPAPRXive *xive, int srcno, int val) +{ + if (val) { + spapr_xive_irq(xive, srcno); + } +} + +static void spapr_xive_source_set_irq_lsi(sPAPRXive *xive, int srcno, int val) +{ + ICSIRQState *irq = &xive->ics->irqs[srcno]; + + if (val) { + irq->status |= XICS_STATUS_ASSERTED; + } else { + irq->status &= ~XICS_STATUS_ASSERTED; + } + + if (irq->status & XICS_STATUS_ASSERTED + && !(irq->status & XICS_STATUS_SENT)) { + irq->status |= XICS_STATUS_SENT; + spapr_xive_irq(xive, srcno); + } +} + +static void spapr_xive_source_set_irq(void *opaque, int srcno, int val) +{ + sPAPRXive *xive = SPAPR_XIVE(opaque); + ICSIRQState *irq = &xive->ics->irqs[srcno]; + + if (irq->flags & XICS_FLAGS_IRQ_LSI) { + spapr_xive_source_set_irq_lsi(xive, srcno, val); + } else { + spapr_xive_source_set_irq_msi(xive, srcno, val); + } +} + /* * Main XIVE object */ @@ -80,6 +124,8 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) } xive->ics = ICS_BASE(obj); + xive->qirqs = qemu_allocate_irqs(spapr_xive_source_set_irq, xive, + xive->nr_irqs); /* Allocate the last IRQ numbers for the IPIs */ for (i = xive->nr_irqs - xive->nr_targets; i < xive->nr_irqs; i++) { diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h index 29112589b37f..eab92c4c1bb8 100644 --- a/include/hw/ppc/spapr_xive.h +++ b/include/hw/ppc/spapr_xive.h @@ -38,6 +38,7 @@ struct sPAPRXive { /* IRQ */ ICSState *ics; /* XICS source inherited from the SPAPR machine */ + qemu_irq *qirqs; /* XIVE internal tables */ uint8_t *sbe;
These are very similar to the XICS handlers in a simpler form. They make use of the ICSIRQState array of the XICS interrupt source to differentiate the MSI from the LSI interrupts. The spapr_xive_irq() routine in charge of triggering the CPU interrupt line will be filled later on. The next patch will introduce the MMIO handlers to interact with XIVE interrupt sources. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/intc/spapr_xive.c | 46 +++++++++++++++++++++++++++++++++++++++++++++ include/hw/ppc/spapr_xive.h | 1 + 2 files changed, 47 insertions(+)