| Message ID | 154774528595.1208625.15523908858345133758.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: Add support for PHB hotplug | expand |
On Thu, 17 Jan 2019 18:14:46 +0100 Greg Kurz <groug@kaod.org> wrote: > The current code assumes that an interrupt is allocated as soon as its > type is set to MSI or LSI. PHB hotplug will require to be able to set > the type of an interrupt before actually allocating it. > > Disintricate type setting from allocation by using another flag bit for > the latter. Introduce a new ics_claim_irq() function for allocation. The > behavior of aborting if the same irq gets allocated twice is kept. > > ics_set_irq_type() now only sets the type to MSI or LSI. It doesn't bring > anything to abort if the type was already set before. Drop the assert > and XICS_FLAGS_IRQ_MASK on the way. > With some more testing (and thinking :) I now realize that the type of the irq should survive PHB unplug, which is not the case with this patch. This calls for a v4. > Older QEMUs don't know about XICS_FLAGS_IRQ_CLAIMED. In order to safely > handle incoming migration, we must fix the irq flags. This is done at > post load thanks to a compat property. We don't need to do anything for > backward migration since older machines only care for the irq type. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/core/machine.c | 4 ++++ > hw/intc/xics.c | 28 +++++++++++++++++++++++++--- > hw/ppc/pnv_psi.c | 1 + > hw/ppc/spapr_irq.c | 4 +--- > include/hw/ppc/xics.h | 8 ++++++-- > 5 files changed, 37 insertions(+), 8 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 1a0a9ab1117a..536a34092367 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -25,6 +25,10 @@ > > GlobalProperty hw_compat_3_1[] = { > { > + .driver = "ics-base", > + .property = "has-claimed-flag", > + .value = "off", > + },{ > .driver = "pcie-root-port", > .property = "x-speed", > .value = "2_5", > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index 16e8ffa2aaf7..82cf04548907 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -75,7 +75,7 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon) > for (i = 0; i < ics->nr_irqs; i++) { > ICSIRQState *irq = ics->irqs + i; > > - if (!(irq->flags & XICS_FLAGS_IRQ_MASK)) { > + if (!(irq->flags & XICS_FLAGS_IRQ_CLAIMED)) { > continue; > } > monitor_printf(mon, " %4x %s %02x %02x\n", > @@ -662,6 +662,22 @@ static int ics_base_dispatch_post_load(void *opaque, int version_id) > ICSState *ics = opaque; > ICSStateClass *info = ICS_BASE_GET_CLASS(ics); > > + if (!ics->has_claimed_flag) { > + int i; > + > + for (i = 0; i < ics->nr_irqs; i++) { > + ICSIRQState *irq = ics->irqs + i; > + > + /* > + * For older machines, allocating the irq and setting its type is > + * the same thing. > + */ > + if (irq->flags & (XICS_FLAGS_IRQ_LSI| XICS_FLAGS_IRQ_MSI)) { > + irq->flags |= XICS_FLAGS_IRQ_CLAIMED; > + } > + } > + } > + > if (info->post_load) { > return info->post_load(ics, version_id); > } > @@ -702,6 +718,7 @@ static const VMStateDescription vmstate_ics_base = { > > static Property ics_base_properties[] = { > DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0), > + DEFINE_PROP_BOOL("has-claimed-flag", ICSState, has_claimed_flag, true), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -741,10 +758,15 @@ ICPState *xics_icp_get(XICSFabric *xi, int server) > return xic->icp_get(xi, server); > } > > -void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) > +void ics_claim_irq(ICSState *ics, int srcno) > { > - assert(!(ics->irqs[srcno].flags & XICS_FLAGS_IRQ_MASK)); > + assert(ICS_IRQ_FREE(ics, srcno)); > + > + ics->irqs[srcno].flags |= XICS_FLAGS_IRQ_CLAIMED; > +} > > +void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) > +{ > ics->irqs[srcno].flags |= > lsi ? XICS_FLAGS_IRQ_LSI : XICS_FLAGS_IRQ_MSI; > } > diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c > index 8ced09506321..ced34e1119dc 100644 > --- a/hw/ppc/pnv_psi.c > +++ b/hw/ppc/pnv_psi.c > @@ -488,6 +488,7 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp) > > for (i = 0; i < ics->nr_irqs; i++) { > ics_set_irq_type(ics, i, true); > + ics_claim_irq(ics, i); > } > > psi->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs); > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index 1da7a32348fc..86c712d15382 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -125,9 +125,6 @@ error: > error_propagate(errp, local_err); > } > > -#define ICS_IRQ_FREE(ics, srcno) \ > - (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_IRQ_MASK))) > - > static int spapr_irq_claim_xics(sPAPRMachineState *spapr, int irq, bool lsi, > Error **errp) > { > @@ -146,6 +143,7 @@ static int spapr_irq_claim_xics(sPAPRMachineState *spapr, int irq, bool lsi, > } > > ics_set_irq_type(ics, irq - ics->offset, lsi); > + ics_claim_irq(ics, irq - ics->offset); > return 0; > } > > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index fad786e8b22d..e3be5cc663c5 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -131,6 +131,7 @@ struct ICSState { > /*< public >*/ > uint32_t nr_irqs; > uint32_t offset; > + bool has_claimed_flag; > ICSIRQState *irqs; > XICSFabric *xics; > }; > @@ -153,13 +154,15 @@ struct ICSIRQState { > #define XICS_STATUS_PRESENTED 0x10 > #define XICS_STATUS_QUEUED 0x20 > uint8_t status; > -/* (flags & XICS_FLAGS_IRQ_MASK) == 0 means the interrupt is not allocated */ > #define XICS_FLAGS_IRQ_LSI 0x1 > #define XICS_FLAGS_IRQ_MSI 0x2 > -#define XICS_FLAGS_IRQ_MASK 0x3 > +#define XICS_FLAGS_IRQ_CLAIMED 0x4 > uint8_t flags; > }; > > +#define ICS_IRQ_FREE(ics, srcno) \ > + (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_IRQ_CLAIMED))) > + > struct XICSFabric { > Object parent; > }; > @@ -193,6 +196,7 @@ void ics_simple_write_xive(ICSState *ics, int nr, int server, > void ics_simple_set_irq(void *opaque, int srcno, int val); > void ics_kvm_set_irq(void *opaque, int srcno, int val); > > +void ics_claim_irq(ICSState *ics, int srcno); > void ics_set_irq_type(ICSState *ics, int srcno, bool lsi); > void icp_pic_print_info(ICPState *icp, Monitor *mon); > void ics_pic_print_info(ICSState *ics, Monitor *mon); >
On 1/17/19 6:14 PM, Greg Kurz wrote: > The current code assumes that an interrupt is allocated as soon as its > type is set to MSI or LSI. PHB hotplug will require to be able to set > the type of an interrupt before actually allocating it. > > Disintricate type setting from allocation by using another flag bit for > the latter. Introduce a new ics_claim_irq() function for allocation. The > behavior of aborting if the same irq gets allocated twice is kept. > > ics_set_irq_type() now only sets the type to MSI or LSI. It doesn't bring > anything to abort if the type was already set before. Drop the assert > and XICS_FLAGS_IRQ_MASK on the way. > > Older QEMUs don't know about XICS_FLAGS_IRQ_CLAIMED. In order to safely > handle incoming migration, we must fix the irq flags. This is done at > post load thanks to a compat property. We don't need to do anything for > backward migration since older machines only care for the irq type. > > Signed-off-by: Greg Kurz <groug@kaod.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> In case of a respin, may be transform macro ICS_IRQ_FREE() in a static inline helper. Thanks, C. > --- > hw/core/machine.c | 4 ++++ > hw/intc/xics.c | 28 +++++++++++++++++++++++++--- > hw/ppc/pnv_psi.c | 1 + > hw/ppc/spapr_irq.c | 4 +--- > include/hw/ppc/xics.h | 8 ++++++-- > 5 files changed, 37 insertions(+), 8 deletions(-) > > diff --git a/hw/core/machine.c b/hw/core/machine.c > index 1a0a9ab1117a..536a34092367 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -25,6 +25,10 @@ > > GlobalProperty hw_compat_3_1[] = { > { > + .driver = "ics-base", > + .property = "has-claimed-flag", > + .value = "off", > + },{ > .driver = "pcie-root-port", > .property = "x-speed", > .value = "2_5", > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index 16e8ffa2aaf7..82cf04548907 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -75,7 +75,7 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon) > for (i = 0; i < ics->nr_irqs; i++) { > ICSIRQState *irq = ics->irqs + i; > > - if (!(irq->flags & XICS_FLAGS_IRQ_MASK)) { > + if (!(irq->flags & XICS_FLAGS_IRQ_CLAIMED)) { > continue; > } > monitor_printf(mon, " %4x %s %02x %02x\n", > @@ -662,6 +662,22 @@ static int ics_base_dispatch_post_load(void *opaque, int version_id) > ICSState *ics = opaque; > ICSStateClass *info = ICS_BASE_GET_CLASS(ics); > > + if (!ics->has_claimed_flag) { > + int i; > + > + for (i = 0; i < ics->nr_irqs; i++) { > + ICSIRQState *irq = ics->irqs + i; > + > + /* > + * For older machines, allocating the irq and setting its type is > + * the same thing. > + */ > + if (irq->flags & (XICS_FLAGS_IRQ_LSI| XICS_FLAGS_IRQ_MSI)) { > + irq->flags |= XICS_FLAGS_IRQ_CLAIMED; > + } > + } > + } > + > if (info->post_load) { > return info->post_load(ics, version_id); > } > @@ -702,6 +718,7 @@ static const VMStateDescription vmstate_ics_base = { > > static Property ics_base_properties[] = { > DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0), > + DEFINE_PROP_BOOL("has-claimed-flag", ICSState, has_claimed_flag, true), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -741,10 +758,15 @@ ICPState *xics_icp_get(XICSFabric *xi, int server) > return xic->icp_get(xi, server); > } > > -void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) > +void ics_claim_irq(ICSState *ics, int srcno) > { > - assert(!(ics->irqs[srcno].flags & XICS_FLAGS_IRQ_MASK)); > + assert(ICS_IRQ_FREE(ics, srcno)); > + > + ics->irqs[srcno].flags |= XICS_FLAGS_IRQ_CLAIMED; > +} > > +void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) > +{ > ics->irqs[srcno].flags |= > lsi ? XICS_FLAGS_IRQ_LSI : XICS_FLAGS_IRQ_MSI; > } > diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c > index 8ced09506321..ced34e1119dc 100644 > --- a/hw/ppc/pnv_psi.c > +++ b/hw/ppc/pnv_psi.c > @@ -488,6 +488,7 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp) > > for (i = 0; i < ics->nr_irqs; i++) { > ics_set_irq_type(ics, i, true); > + ics_claim_irq(ics, i); > } > > psi->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs); > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > index 1da7a32348fc..86c712d15382 100644 > --- a/hw/ppc/spapr_irq.c > +++ b/hw/ppc/spapr_irq.c > @@ -125,9 +125,6 @@ error: > error_propagate(errp, local_err); > } > > -#define ICS_IRQ_FREE(ics, srcno) \ > - (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_IRQ_MASK))) > - > static int spapr_irq_claim_xics(sPAPRMachineState *spapr, int irq, bool lsi, > Error **errp) > { > @@ -146,6 +143,7 @@ static int spapr_irq_claim_xics(sPAPRMachineState *spapr, int irq, bool lsi, > } > > ics_set_irq_type(ics, irq - ics->offset, lsi); > + ics_claim_irq(ics, irq - ics->offset); > return 0; > } > > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index fad786e8b22d..e3be5cc663c5 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -131,6 +131,7 @@ struct ICSState { > /*< public >*/ > uint32_t nr_irqs; > uint32_t offset; > + bool has_claimed_flag; > ICSIRQState *irqs; > XICSFabric *xics; > }; > @@ -153,13 +154,15 @@ struct ICSIRQState { > #define XICS_STATUS_PRESENTED 0x10 > #define XICS_STATUS_QUEUED 0x20 > uint8_t status; > -/* (flags & XICS_FLAGS_IRQ_MASK) == 0 means the interrupt is not allocated */ > #define XICS_FLAGS_IRQ_LSI 0x1 > #define XICS_FLAGS_IRQ_MSI 0x2 > -#define XICS_FLAGS_IRQ_MASK 0x3 > +#define XICS_FLAGS_IRQ_CLAIMED 0x4 > uint8_t flags; > }; > > +#define ICS_IRQ_FREE(ics, srcno) \ > + (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_IRQ_CLAIMED))) > + > struct XICSFabric { > Object parent; > }; > @@ -193,6 +196,7 @@ void ics_simple_write_xive(ICSState *ics, int nr, int server, > void ics_simple_set_irq(void *opaque, int srcno, int val); > void ics_kvm_set_irq(void *opaque, int srcno, int val); > > +void ics_claim_irq(ICSState *ics, int srcno); > void ics_set_irq_type(ICSState *ics, int srcno, bool lsi); > void icp_pic_print_info(ICPState *icp, Monitor *mon); > void ics_pic_print_info(ICSState *ics, Monitor *mon); >
On Fri, 18 Jan 2019 13:26:29 +0100 Cédric Le Goater <clg@kaod.org> wrote: > On 1/17/19 6:14 PM, Greg Kurz wrote: > > The current code assumes that an interrupt is allocated as soon as its > > type is set to MSI or LSI. PHB hotplug will require to be able to set > > the type of an interrupt before actually allocating it. > > > > Disintricate type setting from allocation by using another flag bit for > > the latter. Introduce a new ics_claim_irq() function for allocation. The > > behavior of aborting if the same irq gets allocated twice is kept. > > > > ics_set_irq_type() now only sets the type to MSI or LSI. It doesn't bring > > anything to abort if the type was already set before. Drop the assert > > and XICS_FLAGS_IRQ_MASK on the way. > > > > Older QEMUs don't know about XICS_FLAGS_IRQ_CLAIMED. In order to safely > > handle incoming migration, we must fix the irq flags. This is done at > > post load thanks to a compat property. We don't need to do anything for > > backward migration since older machines only care for the irq type. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > In case of a respin, may be transform macro ICS_IRQ_FREE() in a static > inline helper. > Yeah, there will be a respin (see my other mail in this thread) and I'll just do that. > Thanks, > > C. > > > --- > > hw/core/machine.c | 4 ++++ > > hw/intc/xics.c | 28 +++++++++++++++++++++++++--- > > hw/ppc/pnv_psi.c | 1 + > > hw/ppc/spapr_irq.c | 4 +--- > > include/hw/ppc/xics.h | 8 ++++++-- > > 5 files changed, 37 insertions(+), 8 deletions(-) > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c > > index 1a0a9ab1117a..536a34092367 100644 > > --- a/hw/core/machine.c > > +++ b/hw/core/machine.c > > @@ -25,6 +25,10 @@ > > > > GlobalProperty hw_compat_3_1[] = { > > { > > + .driver = "ics-base", > > + .property = "has-claimed-flag", > > + .value = "off", > > + },{ > > .driver = "pcie-root-port", > > .property = "x-speed", > > .value = "2_5", > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > > index 16e8ffa2aaf7..82cf04548907 100644 > > --- a/hw/intc/xics.c > > +++ b/hw/intc/xics.c > > @@ -75,7 +75,7 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon) > > for (i = 0; i < ics->nr_irqs; i++) { > > ICSIRQState *irq = ics->irqs + i; > > > > - if (!(irq->flags & XICS_FLAGS_IRQ_MASK)) { > > + if (!(irq->flags & XICS_FLAGS_IRQ_CLAIMED)) { > > continue; > > } > > monitor_printf(mon, " %4x %s %02x %02x\n", > > @@ -662,6 +662,22 @@ static int ics_base_dispatch_post_load(void *opaque, int version_id) > > ICSState *ics = opaque; > > ICSStateClass *info = ICS_BASE_GET_CLASS(ics); > > > > + if (!ics->has_claimed_flag) { > > + int i; > > + > > + for (i = 0; i < ics->nr_irqs; i++) { > > + ICSIRQState *irq = ics->irqs + i; > > + > > + /* > > + * For older machines, allocating the irq and setting its type is > > + * the same thing. > > + */ > > + if (irq->flags & (XICS_FLAGS_IRQ_LSI| XICS_FLAGS_IRQ_MSI)) { > > + irq->flags |= XICS_FLAGS_IRQ_CLAIMED; > > + } > > + } > > + } > > + > > if (info->post_load) { > > return info->post_load(ics, version_id); > > } > > @@ -702,6 +718,7 @@ static const VMStateDescription vmstate_ics_base = { > > > > static Property ics_base_properties[] = { > > DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0), > > + DEFINE_PROP_BOOL("has-claimed-flag", ICSState, has_claimed_flag, true), > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > @@ -741,10 +758,15 @@ ICPState *xics_icp_get(XICSFabric *xi, int server) > > return xic->icp_get(xi, server); > > } > > > > -void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) > > +void ics_claim_irq(ICSState *ics, int srcno) > > { > > - assert(!(ics->irqs[srcno].flags & XICS_FLAGS_IRQ_MASK)); > > + assert(ICS_IRQ_FREE(ics, srcno)); > > + > > + ics->irqs[srcno].flags |= XICS_FLAGS_IRQ_CLAIMED; > > +} > > > > +void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) > > +{ > > ics->irqs[srcno].flags |= > > lsi ? XICS_FLAGS_IRQ_LSI : XICS_FLAGS_IRQ_MSI; > > } > > diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c > > index 8ced09506321..ced34e1119dc 100644 > > --- a/hw/ppc/pnv_psi.c > > +++ b/hw/ppc/pnv_psi.c > > @@ -488,6 +488,7 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp) > > > > for (i = 0; i < ics->nr_irqs; i++) { > > ics_set_irq_type(ics, i, true); > > + ics_claim_irq(ics, i); > > } > > > > psi->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs); > > diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c > > index 1da7a32348fc..86c712d15382 100644 > > --- a/hw/ppc/spapr_irq.c > > +++ b/hw/ppc/spapr_irq.c > > @@ -125,9 +125,6 @@ error: > > error_propagate(errp, local_err); > > } > > > > -#define ICS_IRQ_FREE(ics, srcno) \ > > - (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_IRQ_MASK))) > > - > > static int spapr_irq_claim_xics(sPAPRMachineState *spapr, int irq, bool lsi, > > Error **errp) > > { > > @@ -146,6 +143,7 @@ static int spapr_irq_claim_xics(sPAPRMachineState *spapr, int irq, bool lsi, > > } > > > > ics_set_irq_type(ics, irq - ics->offset, lsi); > > + ics_claim_irq(ics, irq - ics->offset); > > return 0; > > } > > > > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > > index fad786e8b22d..e3be5cc663c5 100644 > > --- a/include/hw/ppc/xics.h > > +++ b/include/hw/ppc/xics.h > > @@ -131,6 +131,7 @@ struct ICSState { > > /*< public >*/ > > uint32_t nr_irqs; > > uint32_t offset; > > + bool has_claimed_flag; > > ICSIRQState *irqs; > > XICSFabric *xics; > > }; > > @@ -153,13 +154,15 @@ struct ICSIRQState { > > #define XICS_STATUS_PRESENTED 0x10 > > #define XICS_STATUS_QUEUED 0x20 > > uint8_t status; > > -/* (flags & XICS_FLAGS_IRQ_MASK) == 0 means the interrupt is not allocated */ > > #define XICS_FLAGS_IRQ_LSI 0x1 > > #define XICS_FLAGS_IRQ_MSI 0x2 > > -#define XICS_FLAGS_IRQ_MASK 0x3 > > +#define XICS_FLAGS_IRQ_CLAIMED 0x4 > > uint8_t flags; > > }; > > > > +#define ICS_IRQ_FREE(ics, srcno) \ > > + (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_IRQ_CLAIMED))) > > + > > struct XICSFabric { > > Object parent; > > }; > > @@ -193,6 +196,7 @@ void ics_simple_write_xive(ICSState *ics, int nr, int server, > > void ics_simple_set_irq(void *opaque, int srcno, int val); > > void ics_kvm_set_irq(void *opaque, int srcno, int val); > > > > +void ics_claim_irq(ICSState *ics, int srcno); > > void ics_set_irq_type(ICSState *ics, int srcno, bool lsi); > > void icp_pic_print_info(ICPState *icp, Monitor *mon); > > void ics_pic_print_info(ICSState *ics, Monitor *mon); > > >
On Thu, Jan 17, 2019 at 06:14:46PM +0100, Greg Kurz wrote: > The current code assumes that an interrupt is allocated as soon as its > type is set to MSI or LSI. PHB hotplug will require to be able to set > the type of an interrupt before actually allocating it. Hm.. why?
On Tue, 5 Feb 2019 17:13:46 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Thu, Jan 17, 2019 at 06:14:46PM +0100, Greg Kurz wrote: > > The current code assumes that an interrupt is allocated as soon as its > > type is set to MSI or LSI. PHB hotplug will require to be able to set > > the type of an interrupt before actually allocating it. > > Hm.. why? > The justification for that is given in patch 6 actually: Every PHB needs to claim 4 LSIs to support legacy PCI devices. This is currently done at PHB realize. When using in-kernel XICS (or upcoming in-kernel XIVE), QEMU synchronizes the state of all irqs, including these LSIs, later on at machine reset. In order to support PHB hotplug, we need a way to tell KVM about the LSIs that doesn't require a machine reset. Since these irq numbers are fixed values derived from the PHB index, let's identify them all at machine init. Older machines that don't have fixed irq numbers cannot support PHB hotplug and keep the existing behavior. FYI, I'm currently reworking that part entirely. Maybe not worth wasting to much time on reviewing this v3.
On Tue, Feb 05, 2019 at 03:59:15PM +0100, Greg Kurz wrote: > On Tue, 5 Feb 2019 17:13:46 +1100 > David Gibson <david@gibson.dropbear.id.au> wrote: > > > On Thu, Jan 17, 2019 at 06:14:46PM +0100, Greg Kurz wrote: > > > The current code assumes that an interrupt is allocated as soon as its > > > type is set to MSI or LSI. PHB hotplug will require to be able to set > > > the type of an interrupt before actually allocating it. > > > > Hm.. why? > > > > The justification for that is given in patch 6 actually: > > Every PHB needs to claim 4 LSIs to support legacy PCI devices. This is > currently done at PHB realize. When using in-kernel XICS (or upcoming > in-kernel XIVE), QEMU synchronizes the state of all irqs, including > these LSIs, later on at machine reset. > > In order to support PHB hotplug, we need a way to tell KVM about the > LSIs that doesn't require a machine reset. Since these irq numbers are > fixed values derived from the PHB index, let's identify them all at > machine init. Older machines that don't have fixed irq numbers cannot > support PHB hotplug and keep the existing behavior. Sounds good. > FYI, I'm currently reworking that part entirely. Maybe not worth wasting to > much time on reviewing this v3. Ok, I have plenty of other stuff to review, so I'll wait for the next spin.
diff --git a/hw/core/machine.c b/hw/core/machine.c index 1a0a9ab1117a..536a34092367 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -25,6 +25,10 @@ GlobalProperty hw_compat_3_1[] = { { + .driver = "ics-base", + .property = "has-claimed-flag", + .value = "off", + },{ .driver = "pcie-root-port", .property = "x-speed", .value = "2_5", diff --git a/hw/intc/xics.c b/hw/intc/xics.c index 16e8ffa2aaf7..82cf04548907 100644 --- a/hw/intc/xics.c +++ b/hw/intc/xics.c @@ -75,7 +75,7 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon) for (i = 0; i < ics->nr_irqs; i++) { ICSIRQState *irq = ics->irqs + i; - if (!(irq->flags & XICS_FLAGS_IRQ_MASK)) { + if (!(irq->flags & XICS_FLAGS_IRQ_CLAIMED)) { continue; } monitor_printf(mon, " %4x %s %02x %02x\n", @@ -662,6 +662,22 @@ static int ics_base_dispatch_post_load(void *opaque, int version_id) ICSState *ics = opaque; ICSStateClass *info = ICS_BASE_GET_CLASS(ics); + if (!ics->has_claimed_flag) { + int i; + + for (i = 0; i < ics->nr_irqs; i++) { + ICSIRQState *irq = ics->irqs + i; + + /* + * For older machines, allocating the irq and setting its type is + * the same thing. + */ + if (irq->flags & (XICS_FLAGS_IRQ_LSI| XICS_FLAGS_IRQ_MSI)) { + irq->flags |= XICS_FLAGS_IRQ_CLAIMED; + } + } + } + if (info->post_load) { return info->post_load(ics, version_id); } @@ -702,6 +718,7 @@ static const VMStateDescription vmstate_ics_base = { static Property ics_base_properties[] = { DEFINE_PROP_UINT32("nr-irqs", ICSState, nr_irqs, 0), + DEFINE_PROP_BOOL("has-claimed-flag", ICSState, has_claimed_flag, true), DEFINE_PROP_END_OF_LIST(), }; @@ -741,10 +758,15 @@ ICPState *xics_icp_get(XICSFabric *xi, int server) return xic->icp_get(xi, server); } -void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) +void ics_claim_irq(ICSState *ics, int srcno) { - assert(!(ics->irqs[srcno].flags & XICS_FLAGS_IRQ_MASK)); + assert(ICS_IRQ_FREE(ics, srcno)); + + ics->irqs[srcno].flags |= XICS_FLAGS_IRQ_CLAIMED; +} +void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) +{ ics->irqs[srcno].flags |= lsi ? XICS_FLAGS_IRQ_LSI : XICS_FLAGS_IRQ_MSI; } diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c index 8ced09506321..ced34e1119dc 100644 --- a/hw/ppc/pnv_psi.c +++ b/hw/ppc/pnv_psi.c @@ -488,6 +488,7 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp) for (i = 0; i < ics->nr_irqs; i++) { ics_set_irq_type(ics, i, true); + ics_claim_irq(ics, i); } psi->qirqs = qemu_allocate_irqs(ics_simple_set_irq, ics, ics->nr_irqs); diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c index 1da7a32348fc..86c712d15382 100644 --- a/hw/ppc/spapr_irq.c +++ b/hw/ppc/spapr_irq.c @@ -125,9 +125,6 @@ error: error_propagate(errp, local_err); } -#define ICS_IRQ_FREE(ics, srcno) \ - (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_IRQ_MASK))) - static int spapr_irq_claim_xics(sPAPRMachineState *spapr, int irq, bool lsi, Error **errp) { @@ -146,6 +143,7 @@ static int spapr_irq_claim_xics(sPAPRMachineState *spapr, int irq, bool lsi, } ics_set_irq_type(ics, irq - ics->offset, lsi); + ics_claim_irq(ics, irq - ics->offset); return 0; } diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h index fad786e8b22d..e3be5cc663c5 100644 --- a/include/hw/ppc/xics.h +++ b/include/hw/ppc/xics.h @@ -131,6 +131,7 @@ struct ICSState { /*< public >*/ uint32_t nr_irqs; uint32_t offset; + bool has_claimed_flag; ICSIRQState *irqs; XICSFabric *xics; }; @@ -153,13 +154,15 @@ struct ICSIRQState { #define XICS_STATUS_PRESENTED 0x10 #define XICS_STATUS_QUEUED 0x20 uint8_t status; -/* (flags & XICS_FLAGS_IRQ_MASK) == 0 means the interrupt is not allocated */ #define XICS_FLAGS_IRQ_LSI 0x1 #define XICS_FLAGS_IRQ_MSI 0x2 -#define XICS_FLAGS_IRQ_MASK 0x3 +#define XICS_FLAGS_IRQ_CLAIMED 0x4 uint8_t flags; }; +#define ICS_IRQ_FREE(ics, srcno) \ + (!((ics)->irqs[(srcno)].flags & (XICS_FLAGS_IRQ_CLAIMED))) + struct XICSFabric { Object parent; }; @@ -193,6 +196,7 @@ void ics_simple_write_xive(ICSState *ics, int nr, int server, void ics_simple_set_irq(void *opaque, int srcno, int val); void ics_kvm_set_irq(void *opaque, int srcno, int val); +void ics_claim_irq(ICSState *ics, int srcno); void ics_set_irq_type(ICSState *ics, int srcno, bool lsi); void icp_pic_print_info(ICPState *icp, Monitor *mon); void ics_pic_print_info(ICSState *ics, Monitor *mon);
The current code assumes that an interrupt is allocated as soon as its type is set to MSI or LSI. PHB hotplug will require to be able to set the type of an interrupt before actually allocating it. Disintricate type setting from allocation by using another flag bit for the latter. Introduce a new ics_claim_irq() function for allocation. The behavior of aborting if the same irq gets allocated twice is kept. ics_set_irq_type() now only sets the type to MSI or LSI. It doesn't bring anything to abort if the type was already set before. Drop the assert and XICS_FLAGS_IRQ_MASK on the way. Older QEMUs don't know about XICS_FLAGS_IRQ_CLAIMED. In order to safely handle incoming migration, we must fix the irq flags. This is done at post load thanks to a compat property. We don't need to do anything for backward migration since older machines only care for the irq type. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/core/machine.c | 4 ++++ hw/intc/xics.c | 28 +++++++++++++++++++++++++--- hw/ppc/pnv_psi.c | 1 + hw/ppc/spapr_irq.c | 4 +--- include/hw/ppc/xics.h | 8 ++++++-- 5 files changed, 37 insertions(+), 8 deletions(-)