| Message ID | 1399442518-26303-2-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
> > for (i = 0; i < ics->nr_irqs; i++) { > /* FIXME: filter by server#? */ > - if (ics->islsi[i]) { > + if (ics->irqs[i].flags & XICS_FLAGS_LSI) { > resend_lsi(ics, i); Not part of your patch, but I'm curious about this FIXME (there's an identical FIXME in resend_msi). Has this proved to be a problem? With these patches you could have many unallocated interrupts in array AFTER the last allocated interrupt, correct? In that case, the loop would continue beyond the last allocated interrupt for no purpose. There are a couple ways to mitigate this type of situation by using alternative data structures to inform the loop traversal. I don't know if it is worth the effort, though. > +/* @flags == 0 measn the interrupt is not allocated */ > +#define XICS_FLAGS_LSI 0x1 > +#define XICS_FLAGS_MSI 0x2 (nit) typo in the above comment Mike
On 05/07/2014 08:01 AM, Alexey Kardashevskiy wrote: > The existing interrupt allocation scheme in SPAPR assumes that > interrupts are allocated at the start time, continously and the config > will not change. However, there are cases when this is not going to work > such as: > > 1. migration - we will have to have an ability to choose interrupt > numbers for devices in the command line and this will create gaps in > interrupt space. > > 2. PCI hotplug - interrupts from unplugged device need to be returned > back to interrupt pool, otherwise we will quickly run out of interrupts. > > This replaces a separate lslsi[] array with a byte in the ICSIRQState > struct and defines "LSI" and "MSI" flags. Neither of these flags set > signals that the descriptor is not allocated and not in use. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > hw/intc/xics.c | 21 ++++++++++++++------- > hw/intc/xics_kvm.c | 5 ++--- > include/hw/ppc/xics.h | 5 ++++- > 3 files changed, 20 insertions(+), 11 deletions(-) > > diff --git a/hw/intc/xics.c b/hw/intc/xics.c > index 64aabe7..1f89a00 100644 > --- a/hw/intc/xics.c > +++ b/hw/intc/xics.c > @@ -438,7 +438,7 @@ static void ics_set_irq(void *opaque, int srcno, int val) > { > ICSState *ics = (ICSState *)opaque; > > - if (ics->islsi[srcno]) { > + if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) { > set_irq_lsi(ics, srcno, val); > } else { > set_irq_msi(ics, srcno, val); > @@ -475,7 +475,7 @@ static void ics_write_xive(ICSState *ics, int nr, int server, > > trace_xics_ics_write_xive(nr, srcno, server, priority); > > - if (ics->islsi[srcno]) { > + if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) { > write_xive_lsi(ics, srcno); > } else { > write_xive_msi(ics, srcno); > @@ -497,7 +497,7 @@ static void ics_resend(ICSState *ics) > > for (i = 0; i < ics->nr_irqs; i++) { > /* FIXME: filter by server#? */ > - if (ics->islsi[i]) { > + if (ics->irqs[i].flags & XICS_FLAGS_LSI) { > resend_lsi(ics, i); > } else { > resend_msi(ics, i); > @@ -512,7 +512,7 @@ static void ics_eoi(ICSState *ics, int nr) > > trace_xics_ics_eoi(nr); > > - if (ics->islsi[srcno]) { > + if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) { > irq->status &= ~XICS_STATUS_SENT; > } > } > @@ -609,7 +609,6 @@ static void ics_realize(DeviceState *dev, Error **errp) > return; > } > ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState)); > - ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool)); > ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs); > } > > @@ -646,11 +645,19 @@ qemu_irq xics_get_qirq(XICSState *icp, int irq) > return icp->ics->qirqs[irq - icp->ics->offset]; > } > > +static void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) > +{ > + ics->irqs[srcno].flags |= > + lsi ? XICS_FLAGS_LSI : XICS_FLAGS_MSI; > +} > + > void xics_set_irq_type(XICSState *icp, int irq, bool lsi) > { > - assert(ics_valid_irq(icp->ics, irq)); > + ICSState *ics = icp->ics; > > - icp->ics->islsi[irq - icp->ics->offset] = lsi; > + assert(ics_valid_irq(ics, irq)); > + > + ics_set_irq_type(ics, irq - ics->offset, lsi); What if this gets called with MSI first, then LSI on the same irq number? > } > > /* > diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c > index 09476ae..456fc2c 100644 > --- a/hw/intc/xics_kvm.c > +++ b/hw/intc/xics_kvm.c > @@ -224,7 +224,7 @@ static int ics_set_kvm_state(ICSState *ics, int version_id) > state |= KVM_XICS_MASKED; > } > > - if (ics->islsi[i]) { > + if (ics->irqs[i].flags & XICS_FLAGS_LSI) { > state |= KVM_XICS_LEVEL_SENSITIVE; > if (irq->status & XICS_STATUS_ASSERTED) { > state |= KVM_XICS_PENDING; > @@ -253,7 +253,7 @@ static void ics_kvm_set_irq(void *opaque, int srcno, int val) > int rc; > > args.irq = srcno + ics->offset; > - if (!ics->islsi[srcno]) { > + if (ics->irqs[srcno].flags & XICS_FLAGS_MSI) { > if (!val) { > return; > } > @@ -290,7 +290,6 @@ static void ics_kvm_realize(DeviceState *dev, Error **errp) > return; > } > ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState)); > - ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool)); > ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs); > } > > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index 0d7673d..aad48cf 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -136,7 +136,6 @@ struct ICSState { > uint32_t nr_irqs; > uint32_t offset; > qemu_irq *qirqs; > - bool *islsi; > ICSIRQState *irqs; > XICSState *icp; > }; > @@ -150,6 +149,10 @@ struct ICSIRQState { > #define XICS_STATUS_REJECTED 0x4 > #define XICS_STATUS_MASKED_PENDING 0x8 > uint8_t status; > +/* @flags == 0 measn the interrupt is not allocated */ > +#define XICS_FLAGS_LSI 0x1 > +#define XICS_FLAGS_MSI 0x2 Please define a mask for the interrupt type. Alex > + uint8_t flags; > }; > > qemu_irq xics_get_qirq(XICSState *icp, int irq);
On 05/08/2014 01:52 PM, Alexander Graf wrote: > On 05/07/2014 08:01 AM, Alexey Kardashevskiy wrote: >> The existing interrupt allocation scheme in SPAPR assumes that >> interrupts are allocated at the start time, continously and the config >> will not change. However, there are cases when this is not going to work >> such as: >> >> 1. migration - we will have to have an ability to choose interrupt >> numbers for devices in the command line and this will create gaps in >> interrupt space. >> >> 2. PCI hotplug - interrupts from unplugged device need to be returned >> back to interrupt pool, otherwise we will quickly run out of interrupts. >> >> This replaces a separate lslsi[] array with a byte in the ICSIRQState >> struct and defines "LSI" and "MSI" flags. Neither of these flags set >> signals that the descriptor is not allocated and not in use. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> hw/intc/xics.c | 21 ++++++++++++++------- >> hw/intc/xics_kvm.c | 5 ++--- >> include/hw/ppc/xics.h | 5 ++++- >> 3 files changed, 20 insertions(+), 11 deletions(-) >> >> diff --git a/hw/intc/xics.c b/hw/intc/xics.c >> index 64aabe7..1f89a00 100644 >> --- a/hw/intc/xics.c >> +++ b/hw/intc/xics.c >> @@ -438,7 +438,7 @@ static void ics_set_irq(void *opaque, int srcno, >> int val) >> { >> ICSState *ics = (ICSState *)opaque; >> - if (ics->islsi[srcno]) { >> + if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) { >> set_irq_lsi(ics, srcno, val); >> } else { >> set_irq_msi(ics, srcno, val); >> @@ -475,7 +475,7 @@ static void ics_write_xive(ICSState *ics, int nr, >> int server, >> trace_xics_ics_write_xive(nr, srcno, server, priority); >> - if (ics->islsi[srcno]) { >> + if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) { >> write_xive_lsi(ics, srcno); >> } else { >> write_xive_msi(ics, srcno); >> @@ -497,7 +497,7 @@ static void ics_resend(ICSState *ics) >> for (i = 0; i < ics->nr_irqs; i++) { >> /* FIXME: filter by server#? */ >> - if (ics->islsi[i]) { >> + if (ics->irqs[i].flags & XICS_FLAGS_LSI) { >> resend_lsi(ics, i); >> } else { >> resend_msi(ics, i); >> @@ -512,7 +512,7 @@ static void ics_eoi(ICSState *ics, int nr) >> trace_xics_ics_eoi(nr); >> - if (ics->islsi[srcno]) { >> + if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) { >> irq->status &= ~XICS_STATUS_SENT; >> } >> } >> @@ -609,7 +609,6 @@ static void ics_realize(DeviceState *dev, Error >> **errp) >> return; >> } >> ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState)); >> - ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool)); >> ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs); >> } >> @@ -646,11 +645,19 @@ qemu_irq xics_get_qirq(XICSState *icp, int irq) >> return icp->ics->qirqs[irq - icp->ics->offset]; >> } >> +static void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) >> +{ >> + ics->irqs[srcno].flags |= >> + lsi ? XICS_FLAGS_LSI : XICS_FLAGS_MSI; >> +} >> + >> void xics_set_irq_type(XICSState *icp, int irq, bool lsi) >> { >> - assert(ics_valid_irq(icp->ics, irq)); >> + ICSState *ics = icp->ics; >> - icp->ics->islsi[irq - icp->ics->offset] = lsi; >> + assert(ics_valid_irq(ics, irq)); >> + >> + ics_set_irq_type(ics, irq - ics->offset, lsi); > > What if this gets called with MSI first, then LSI on the same irq number? Following your code I think this never happens. But to ensure it doesn't add an assert(!(ics->irqs[srcno].flags & XICS_FLAGS_IRQTYPE)) in ircs_set_irq_type() maybe? Alex
On 05/07/2014 11:09 PM, Mike Day wrote: > >> >> for (i = 0; i < ics->nr_irqs; i++) { >> /* FIXME: filter by server#? */ >> - if (ics->islsi[i]) { >> + if (ics->irqs[i].flags & XICS_FLAGS_LSI) { >> resend_lsi(ics, i); > > Not part of your patch, but I'm curious about this FIXME (there's an > identical FIXME in resend_msi). Has this proved to be a problem? ics_resend -> resend_lsi() -> icp_irq() and the last one delivers interrupt to the correct server. I should probably remove that FIXME or I am missing something here. > With > these patches you could have many unallocated interrupts in array AFTER > the last allocated interrupt, correct? I would have unallocated interrupts before this patch too. > In that case, the loop would > continue beyond the last allocated interrupt for no purpose. For LSI we check the type, if it is not, it is assumed MSI and then resend_msi() would check for XICS_STATUS_REJECTED which is not set for non-allocated interrupt so we are safe. But since I can tell now if it is allocated or not, I will fix it to call resend_msi() on only if irq is allocated as MSI. > There are > a couple ways to mitigate this type of situation by using alternative > data structures to inform the loop traversal. I don't know if it is > worth the effort, though. Here I lost you :) btw I just realized that in patch#2 it should be xics_find_source instead of xics_find_server. There are many interrupt servers already and just one interrupt source (we could have many like one per PHB or something like that but we are not there yet), this is what I meant. > > >> +/* @flags == 0 measn the interrupt is not allocated */ >> +#define XICS_FLAGS_LSI 0x1 >> +#define XICS_FLAGS_MSI 0x2 > > (nit) typo in the above comment > > Mike >
On Thu, May 8, 2014 at 11:12 PM, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >> There are >> a couple ways to mitigate this type of situation by using alternative >> data structures to inform the loop traversal. I don't know if it is >> worth the effort, though. > > Here I lost you :) If I read the code correctly, the problem I'm wondering about is that the loop will waste time traversing the array when there are only unallocated interrupts from the current index to the end. For example, if the interrupt array is 256 entries and the highest index that is allocated is 16, the outside loop will traverse all 256 entries while it should have exited after the 16th. To mitigate this you could keep a shadow index of the current highest allocated index and check for that in the outside loop. Or you could maintain a shadow linked list that only includes allocated array entries and just traverse that list. Each element on the list would be an allocated entry in the interrupt array. > btw I just realized that in patch#2 it should be xics_find_source instead > of xics_find_server. There are many interrupt servers already and just one > interrupt source (we could have many like one per PHB or something like > that but we are not there yet), this is what I meant.
diff --git a/hw/intc/xics.c b/hw/intc/xics.c index 64aabe7..1f89a00 100644 --- a/hw/intc/xics.c +++ b/hw/intc/xics.c @@ -438,7 +438,7 @@ static void ics_set_irq(void *opaque, int srcno, int val) { ICSState *ics = (ICSState *)opaque; - if (ics->islsi[srcno]) { + if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) { set_irq_lsi(ics, srcno, val); } else { set_irq_msi(ics, srcno, val); @@ -475,7 +475,7 @@ static void ics_write_xive(ICSState *ics, int nr, int server, trace_xics_ics_write_xive(nr, srcno, server, priority); - if (ics->islsi[srcno]) { + if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) { write_xive_lsi(ics, srcno); } else { write_xive_msi(ics, srcno); @@ -497,7 +497,7 @@ static void ics_resend(ICSState *ics) for (i = 0; i < ics->nr_irqs; i++) { /* FIXME: filter by server#? */ - if (ics->islsi[i]) { + if (ics->irqs[i].flags & XICS_FLAGS_LSI) { resend_lsi(ics, i); } else { resend_msi(ics, i); @@ -512,7 +512,7 @@ static void ics_eoi(ICSState *ics, int nr) trace_xics_ics_eoi(nr); - if (ics->islsi[srcno]) { + if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) { irq->status &= ~XICS_STATUS_SENT; } } @@ -609,7 +609,6 @@ static void ics_realize(DeviceState *dev, Error **errp) return; } ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState)); - ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool)); ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs); } @@ -646,11 +645,19 @@ qemu_irq xics_get_qirq(XICSState *icp, int irq) return icp->ics->qirqs[irq - icp->ics->offset]; } +static void ics_set_irq_type(ICSState *ics, int srcno, bool lsi) +{ + ics->irqs[srcno].flags |= + lsi ? XICS_FLAGS_LSI : XICS_FLAGS_MSI; +} + void xics_set_irq_type(XICSState *icp, int irq, bool lsi) { - assert(ics_valid_irq(icp->ics, irq)); + ICSState *ics = icp->ics; - icp->ics->islsi[irq - icp->ics->offset] = lsi; + assert(ics_valid_irq(ics, irq)); + + ics_set_irq_type(ics, irq - ics->offset, lsi); } /* diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c index 09476ae..456fc2c 100644 --- a/hw/intc/xics_kvm.c +++ b/hw/intc/xics_kvm.c @@ -224,7 +224,7 @@ static int ics_set_kvm_state(ICSState *ics, int version_id) state |= KVM_XICS_MASKED; } - if (ics->islsi[i]) { + if (ics->irqs[i].flags & XICS_FLAGS_LSI) { state |= KVM_XICS_LEVEL_SENSITIVE; if (irq->status & XICS_STATUS_ASSERTED) { state |= KVM_XICS_PENDING; @@ -253,7 +253,7 @@ static void ics_kvm_set_irq(void *opaque, int srcno, int val) int rc; args.irq = srcno + ics->offset; - if (!ics->islsi[srcno]) { + if (ics->irqs[srcno].flags & XICS_FLAGS_MSI) { if (!val) { return; } @@ -290,7 +290,6 @@ static void ics_kvm_realize(DeviceState *dev, Error **errp) return; } ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState)); - ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool)); ics->qirqs = qemu_allocate_irqs(ics_kvm_set_irq, ics, ics->nr_irqs); } diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h index 0d7673d..aad48cf 100644 --- a/include/hw/ppc/xics.h +++ b/include/hw/ppc/xics.h @@ -136,7 +136,6 @@ struct ICSState { uint32_t nr_irqs; uint32_t offset; qemu_irq *qirqs; - bool *islsi; ICSIRQState *irqs; XICSState *icp; }; @@ -150,6 +149,10 @@ struct ICSIRQState { #define XICS_STATUS_REJECTED 0x4 #define XICS_STATUS_MASKED_PENDING 0x8 uint8_t status; +/* @flags == 0 measn the interrupt is not allocated */ +#define XICS_FLAGS_LSI 0x1 +#define XICS_FLAGS_MSI 0x2 + uint8_t flags; }; qemu_irq xics_get_qirq(XICSState *icp, int irq);
The existing interrupt allocation scheme in SPAPR assumes that interrupts are allocated at the start time, continously and the config will not change. However, there are cases when this is not going to work such as: 1. migration - we will have to have an ability to choose interrupt numbers for devices in the command line and this will create gaps in interrupt space. 2. PCI hotplug - interrupts from unplugged device need to be returned back to interrupt pool, otherwise we will quickly run out of interrupts. This replaces a separate lslsi[] array with a byte in the ICSIRQState struct and defines "LSI" and "MSI" flags. Neither of these flags set signals that the descriptor is not allocated and not in use. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- hw/intc/xics.c | 21 ++++++++++++++------- hw/intc/xics_kvm.c | 5 ++--- include/hw/ppc/xics.h | 5 ++++- 3 files changed, 20 insertions(+), 11 deletions(-)