| Message ID | 20170911171235.29331-5-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
| Series | Guest exploitation of the XIVE interrupt controller (POWER9) | expand |
On Mon, 11 Sep 2017 19:12:18 +0200 Cédric Le Goater <clg@kaod.org> wrote: > The sPAPR machine first starts with a XICS interrupt model and > depending on the guest capabilities, the XIVE exploitation mode is > negotiated during CAS. A reset should then be performed to rebuild the > device tree but the same IRQ numbers which were allocated by the > devices prior to reset, when the XICS model was operating, are still > in use. > > For this purpose, we need a common IRQ number allocator for both the > interrupt models: XICS legacy or XIVE exploitation. This is what the > ICSIRQState array of the XICS interrupt source is used for. It also > contains the LSI/MSI flag of an interrupt which will we need later on. > > So, let's provide a link to the sPAPR ICS object under XIVE to make > use of it. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/intc/spapr_xive.c | 12 ++++++++++++ > include/hw/ppc/spapr_xive.h | 4 ++++ > 2 files changed, 16 insertions(+) > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > index 6d98528fae68..1681affb0848 100644 > --- a/hw/intc/spapr_xive.c > +++ b/hw/intc/spapr_xive.c > @@ -56,6 +56,8 @@ void spapr_xive_reset(void *dev) > static void spapr_xive_realize(DeviceState *dev, Error **errp) > { > sPAPRXive *xive = SPAPR_XIVE(dev); > + Object *obj; > + Error *err = NULL; > > if (!xive->nr_targets) { > error_setg(errp, "Number of interrupt targets needs to be greater 0"); > @@ -68,6 +70,16 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) > return; > } > > + /* Retrieve SPAPR ICS source to share the IRQ number allocator */ > + obj = object_property_get_link(OBJECT(dev), "ics", &err); > + if (!obj) { > + error_setg(errp, "%s: required link 'ics' not found: %s", > + __func__, error_get_pretty(err)); > + return; err is leaked if you do this way. Please do this instead: error_propagate(errp, err); error_prepend(errp, "required link 'ics' not found: "); Note: I've just sent a patch to fix the same error in XICS :) > + } > + > + xive->ics = ICS_BASE(obj); > + > /* Allocate SBEs (State Bit Entry). 2 bits, so 4 entries per byte */ > xive->sbe_size = DIV_ROUND_UP(xive->nr_irqs, 4); > xive->sbe = g_malloc0(xive->sbe_size); > diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h > index b17dd4f17b0b..29112589b37f 100644 > --- a/include/hw/ppc/spapr_xive.h > +++ b/include/hw/ppc/spapr_xive.h > @@ -24,6 +24,7 @@ > typedef struct sPAPRXive sPAPRXive; > typedef struct XiveIVE XiveIVE; > typedef struct XiveEQ XiveEQ; > +typedef struct ICSState ICSState; > > #define TYPE_SPAPR_XIVE "spapr-xive" > #define SPAPR_XIVE(obj) OBJECT_CHECK(sPAPRXive, (obj), TYPE_SPAPR_XIVE) > @@ -35,6 +36,9 @@ struct sPAPRXive { > uint32_t nr_targets; > uint32_t nr_irqs; > > + /* IRQ */ > + ICSState *ics; /* XICS source inherited from the SPAPR machine */ > + > /* XIVE internal tables */ > uint8_t *sbe; > uint32_t sbe_size;
On 09/12/2017 12:04 AM, Greg Kurz wrote: > On Mon, 11 Sep 2017 19:12:18 +0200 > Cédric Le Goater <clg@kaod.org> wrote: > >> The sPAPR machine first starts with a XICS interrupt model and >> depending on the guest capabilities, the XIVE exploitation mode is >> negotiated during CAS. A reset should then be performed to rebuild the >> device tree but the same IRQ numbers which were allocated by the >> devices prior to reset, when the XICS model was operating, are still >> in use. >> >> For this purpose, we need a common IRQ number allocator for both the >> interrupt models: XICS legacy or XIVE exploitation. This is what the >> ICSIRQState array of the XICS interrupt source is used for. It also >> contains the LSI/MSI flag of an interrupt which will we need later on. >> >> So, let's provide a link to the sPAPR ICS object under XIVE to make >> use of it. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/intc/spapr_xive.c | 12 ++++++++++++ >> include/hw/ppc/spapr_xive.h | 4 ++++ >> 2 files changed, 16 insertions(+) >> >> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c >> index 6d98528fae68..1681affb0848 100644 >> --- a/hw/intc/spapr_xive.c >> +++ b/hw/intc/spapr_xive.c >> @@ -56,6 +56,8 @@ void spapr_xive_reset(void *dev) >> static void spapr_xive_realize(DeviceState *dev, Error **errp) >> { >> sPAPRXive *xive = SPAPR_XIVE(dev); >> + Object *obj; >> + Error *err = NULL; >> >> if (!xive->nr_targets) { >> error_setg(errp, "Number of interrupt targets needs to be greater 0"); >> @@ -68,6 +70,16 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) >> return; >> } >> >> + /* Retrieve SPAPR ICS source to share the IRQ number allocator */ >> + obj = object_property_get_link(OBJECT(dev), "ics", &err); >> + if (!obj) { >> + error_setg(errp, "%s: required link 'ics' not found: %s", >> + __func__, error_get_pretty(err)); >> + return; > > err is leaked if you do this way. Please do this instead: > > error_propagate(errp, err); > error_prepend(errp, "required link 'ics' not found: "); ok. I will fix. C. > Note: I've just sent a patch to fix the same error in XICS :) > >> + } >> + >> + xive->ics = ICS_BASE(obj); >> + >> /* Allocate SBEs (State Bit Entry). 2 bits, so 4 entries per byte */ >> xive->sbe_size = DIV_ROUND_UP(xive->nr_irqs, 4); >> xive->sbe = g_malloc0(xive->sbe_size); >> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h >> index b17dd4f17b0b..29112589b37f 100644 >> --- a/include/hw/ppc/spapr_xive.h >> +++ b/include/hw/ppc/spapr_xive.h >> @@ -24,6 +24,7 @@ >> typedef struct sPAPRXive sPAPRXive; >> typedef struct XiveIVE XiveIVE; >> typedef struct XiveEQ XiveEQ; >> +typedef struct ICSState ICSState; >> >> #define TYPE_SPAPR_XIVE "spapr-xive" >> #define SPAPR_XIVE(obj) OBJECT_CHECK(sPAPRXive, (obj), TYPE_SPAPR_XIVE) >> @@ -35,6 +36,9 @@ struct sPAPRXive { >> uint32_t nr_targets; >> uint32_t nr_irqs; >> >> + /* IRQ */ >> + ICSState *ics; /* XICS source inherited from the SPAPR machine */ >> + >> /* XIVE internal tables */ >> uint8_t *sbe; >> uint32_t sbe_size; >
On Mon, Sep 11, 2017 at 07:12:18PM +0200, Cédric Le Goater wrote: > The sPAPR machine first starts with a XICS interrupt model and > depending on the guest capabilities, the XIVE exploitation mode is > negotiated during CAS. A reset should then be performed to rebuild the > device tree but the same IRQ numbers which were allocated by the > devices prior to reset, when the XICS model was operating, are still > in use. > > For this purpose, we need a common IRQ number allocator for both the > interrupt models: XICS legacy or XIVE exploitation. This is what the > ICSIRQState array of the XICS interrupt source is used for. It also > contains the LSI/MSI flag of an interrupt which will we need later on. > > So, let's provide a link to the sPAPR ICS object under XIVE to make > use of it. Blech, please don't. The XIVE code absolutely shouldn't be referencing XICS objects, it's a recipe for trouble down the line. If we have to have some sort of abstract "spapr interrupt source" object that could map to either an ICS irq, or a XIVE source then we can do that, but don't directly link XIVE and XICS. *Especially* not new-in-terms-of-old like this, rather than old-in-terms-of-new. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/intc/spapr_xive.c | 12 ++++++++++++ > include/hw/ppc/spapr_xive.h | 4 ++++ > 2 files changed, 16 insertions(+) > > diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c > index 6d98528fae68..1681affb0848 100644 > --- a/hw/intc/spapr_xive.c > +++ b/hw/intc/spapr_xive.c > @@ -56,6 +56,8 @@ void spapr_xive_reset(void *dev) > static void spapr_xive_realize(DeviceState *dev, Error **errp) > { > sPAPRXive *xive = SPAPR_XIVE(dev); > + Object *obj; > + Error *err = NULL; > > if (!xive->nr_targets) { > error_setg(errp, "Number of interrupt targets needs to be greater 0"); > @@ -68,6 +70,16 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) > return; > } > > + /* Retrieve SPAPR ICS source to share the IRQ number allocator */ This really suggests we need to move the irq number allocator out of XICS and into the general spapr code. Or get rid of it entirely (using a more static irq mapping) if possible. > + obj = object_property_get_link(OBJECT(dev), "ics", &err); > + if (!obj) { > + error_setg(errp, "%s: required link 'ics' not found: %s", > + __func__, error_get_pretty(err)); > + return; > + } > + > + xive->ics = ICS_BASE(obj); > + > /* Allocate SBEs (State Bit Entry). 2 bits, so 4 entries per byte */ > xive->sbe_size = DIV_ROUND_UP(xive->nr_irqs, 4); > xive->sbe = g_malloc0(xive->sbe_size); > diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h > index b17dd4f17b0b..29112589b37f 100644 > --- a/include/hw/ppc/spapr_xive.h > +++ b/include/hw/ppc/spapr_xive.h > @@ -24,6 +24,7 @@ > typedef struct sPAPRXive sPAPRXive; > typedef struct XiveIVE XiveIVE; > typedef struct XiveEQ XiveEQ; > +typedef struct ICSState ICSState; > > #define TYPE_SPAPR_XIVE "spapr-xive" > #define SPAPR_XIVE(obj) OBJECT_CHECK(sPAPRXive, (obj), TYPE_SPAPR_XIVE) > @@ -35,6 +36,9 @@ struct sPAPRXive { > uint32_t nr_targets; > uint32_t nr_irqs; > > + /* IRQ */ > + ICSState *ics; /* XICS source inherited from the SPAPR machine */ > + > /* XIVE internal tables */ > uint8_t *sbe; > uint32_t sbe_size;
On 09/19/2017 04:44 AM, David Gibson wrote: > On Mon, Sep 11, 2017 at 07:12:18PM +0200, Cédric Le Goater wrote: >> The sPAPR machine first starts with a XICS interrupt model and >> depending on the guest capabilities, the XIVE exploitation mode is >> negotiated during CAS. A reset should then be performed to rebuild the >> device tree but the same IRQ numbers which were allocated by the >> devices prior to reset, when the XICS model was operating, are still >> in use. >> >> For this purpose, we need a common IRQ number allocator for both the >> interrupt models: XICS legacy or XIVE exploitation. This is what the >> ICSIRQState array of the XICS interrupt source is used for. It also >> contains the LSI/MSI flag of an interrupt which will we need later on. >> >> So, let's provide a link to the sPAPR ICS object under XIVE to make >> use of it. > > Blech, please don't. The XIVE code absolutely shouldn't be > referencing XICS objects, it's a recipe for trouble down the line. Trouble I don't know. But it is a bit ugly and this is why this patchset is still an RFC. > If we have to have some sort of abstract "spapr interrupt source" > object that could map to either an ICS irq, or a XIVE source then we > can do that, but don't directly link XIVE and XICS. *Especially* not > new-in-terms-of-old like this, rather than old-in-terms-of-new. I agree with what you are saying on a common interrupt source but I just haven't found a way to do so yet. So I am trying to corner the ugliness in obvious shortcuts. The purpose is to identify what we need to support migration, hotplug, cas reset, etc. So the current solution is practical to support CAS reset and more important it does not break migration. We can't move an object or parts of an object around without breaking the migration as my recent changes on the xics icp have shown. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/intc/spapr_xive.c | 12 ++++++++++++ >> include/hw/ppc/spapr_xive.h | 4 ++++ >> 2 files changed, 16 insertions(+) >> >> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c >> index 6d98528fae68..1681affb0848 100644 >> --- a/hw/intc/spapr_xive.c >> +++ b/hw/intc/spapr_xive.c >> @@ -56,6 +56,8 @@ void spapr_xive_reset(void *dev) >> static void spapr_xive_realize(DeviceState *dev, Error **errp) >> { >> sPAPRXive *xive = SPAPR_XIVE(dev); >> + Object *obj; >> + Error *err = NULL; >> >> if (!xive->nr_targets) { >> error_setg(errp, "Number of interrupt targets needs to be greater 0"); >> @@ -68,6 +70,16 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) >> return; >> } >> >> + /* Retrieve SPAPR ICS source to share the IRQ number allocator */ > > This really suggests we need to move the irq number allocator out of > XICS and into the general spapr code. Or get rid of it entirely > (using a more static irq mapping) if possible. I have some out of tree changes introducing a bitmap at the spapr machine level. It is a nice cleanup of the spapr_ics_free() and spapr_ics_alloc*() routines. But it is not migration friendly and we still need to keep the ICSIRQState array, which also stores the IRQ state LSI/MSI. I need to check if such a change would bring some benefits for XIVE. C. >> + obj = object_property_get_link(OBJECT(dev), "ics", &err); >> + if (!obj) { >> + error_setg(errp, "%s: required link 'ics' not found: %s", >> + __func__, error_get_pretty(err)); >> + return; >> + } >> + >> + xive->ics = ICS_BASE(obj); >> + >> /* Allocate SBEs (State Bit Entry). 2 bits, so 4 entries per byte */ >> xive->sbe_size = DIV_ROUND_UP(xive->nr_irqs, 4); >> xive->sbe = g_malloc0(xive->sbe_size); >> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h >> index b17dd4f17b0b..29112589b37f 100644 >> --- a/include/hw/ppc/spapr_xive.h >> +++ b/include/hw/ppc/spapr_xive.h >> @@ -24,6 +24,7 @@ >> typedef struct sPAPRXive sPAPRXive; >> typedef struct XiveIVE XiveIVE; >> typedef struct XiveEQ XiveEQ; >> +typedef struct ICSState ICSState; >> >> #define TYPE_SPAPR_XIVE "spapr-xive" >> #define SPAPR_XIVE(obj) OBJECT_CHECK(sPAPRXive, (obj), TYPE_SPAPR_XIVE) >> @@ -35,6 +36,9 @@ struct sPAPRXive { >> uint32_t nr_targets; >> uint32_t nr_irqs; >> >> + /* IRQ */ >> + ICSState *ics; /* XICS source inherited from the SPAPR machine */ >> + >> /* XIVE internal tables */ >> uint8_t *sbe; >> uint32_t sbe_size; >
diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c index 6d98528fae68..1681affb0848 100644 --- a/hw/intc/spapr_xive.c +++ b/hw/intc/spapr_xive.c @@ -56,6 +56,8 @@ void spapr_xive_reset(void *dev) static void spapr_xive_realize(DeviceState *dev, Error **errp) { sPAPRXive *xive = SPAPR_XIVE(dev); + Object *obj; + Error *err = NULL; if (!xive->nr_targets) { error_setg(errp, "Number of interrupt targets needs to be greater 0"); @@ -68,6 +70,16 @@ static void spapr_xive_realize(DeviceState *dev, Error **errp) return; } + /* Retrieve SPAPR ICS source to share the IRQ number allocator */ + obj = object_property_get_link(OBJECT(dev), "ics", &err); + if (!obj) { + error_setg(errp, "%s: required link 'ics' not found: %s", + __func__, error_get_pretty(err)); + return; + } + + xive->ics = ICS_BASE(obj); + /* Allocate SBEs (State Bit Entry). 2 bits, so 4 entries per byte */ xive->sbe_size = DIV_ROUND_UP(xive->nr_irqs, 4); xive->sbe = g_malloc0(xive->sbe_size); diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h index b17dd4f17b0b..29112589b37f 100644 --- a/include/hw/ppc/spapr_xive.h +++ b/include/hw/ppc/spapr_xive.h @@ -24,6 +24,7 @@ typedef struct sPAPRXive sPAPRXive; typedef struct XiveIVE XiveIVE; typedef struct XiveEQ XiveEQ; +typedef struct ICSState ICSState; #define TYPE_SPAPR_XIVE "spapr-xive" #define SPAPR_XIVE(obj) OBJECT_CHECK(sPAPRXive, (obj), TYPE_SPAPR_XIVE) @@ -35,6 +36,9 @@ struct sPAPRXive { uint32_t nr_targets; uint32_t nr_irqs; + /* IRQ */ + ICSState *ics; /* XICS source inherited from the SPAPR machine */ + /* XIVE internal tables */ uint8_t *sbe; uint32_t sbe_size;
The sPAPR machine first starts with a XICS interrupt model and depending on the guest capabilities, the XIVE exploitation mode is negotiated during CAS. A reset should then be performed to rebuild the device tree but the same IRQ numbers which were allocated by the devices prior to reset, when the XICS model was operating, are still in use. For this purpose, we need a common IRQ number allocator for both the interrupt models: XICS legacy or XIVE exploitation. This is what the ICSIRQState array of the XICS interrupt source is used for. It also contains the LSI/MSI flag of an interrupt which will we need later on. So, let's provide a link to the sPAPR ICS object under XIVE to make use of it. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/intc/spapr_xive.c | 12 ++++++++++++ include/hw/ppc/spapr_xive.h | 4 ++++ 2 files changed, 16 insertions(+)