| Message ID | 20181116105729.23240-3-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc: support for the XIVE interrupt controller (POWER9) | expand |
On Fri, Nov 16, 2018 at 11:56:55AM +0100, Cédric Le Goater wrote: > The 'sent' status of the LSI interrupt source is modeled with the 'P' > bit of the ESB and the assertion status of the source is maintained in > an array under the main sPAPRXive object. The type of the source is > stored in the same array for practical reasons. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> Looks good except for some minor details. > --- > include/hw/ppc/xive.h | 20 ++++++++++++- > hw/intc/xive.c | 68 +++++++++++++++++++++++++++++++++++++++---- > 2 files changed, 81 insertions(+), 7 deletions(-) > > diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h > index 5fec4b08705d..e118acd59f1e 100644 > --- a/include/hw/ppc/xive.h > +++ b/include/hw/ppc/xive.h > @@ -32,8 +32,10 @@ typedef struct XiveSource { > /* IRQs */ > uint32_t nr_irqs; > qemu_irq *qirqs; > + unsigned long *lsi_map; > + int32_t lsi_map_size; /* for VMSTATE_BITMAP */ At some point it's possible we'll want XiveSource subclasses that just know which irqs are LSI and which aren't without an explicit map. But this detail isn't exposed in the migration stream or the user interface, so we can tweak it later as ncessary. > - /* PQ bits */ > + /* PQ bits and LSI assertion bit */ > uint8_t *status; > > /* ESB memory region */ > @@ -89,6 +91,7 @@ static inline hwaddr xive_source_esb_mgmt(XiveSource *xsrc, int srcno) > * When doing an EOI, the Q bit will indicate if the interrupt > * needs to be re-triggered. > */ > +#define XIVE_STATUS_ASSERTED 0x4 /* Extra bit for LSI */ > #define XIVE_ESB_VAL_P 0x2 > #define XIVE_ESB_VAL_Q 0x1 > > @@ -127,4 +130,19 @@ static inline qemu_irq xive_source_qirq(XiveSource *xsrc, uint32_t srcno) > return xsrc->qirqs[srcno]; > } > > +static inline bool xive_source_irq_is_lsi(XiveSource *xsrc, uint32_t srcno) > +{ > + assert(srcno < xsrc->nr_irqs); > + return test_bit(srcno, xsrc->lsi_map); > +} > + > +static inline void xive_source_irq_set(XiveSource *xsrc, uint32_t srcno, > + bool lsi) The function name isn't obvious about this being controlling LSI configuration. '..._irq_set_lsi' maybe? > +{ > + assert(srcno < xsrc->nr_irqs); > + if (lsi) { > + bitmap_set(xsrc->lsi_map, srcno, 1); > + } > +} > + > #endif /* PPC_XIVE_H */ > diff --git a/hw/intc/xive.c b/hw/intc/xive.c > index f7621f84828c..ac4605fee8b7 100644 > --- a/hw/intc/xive.c > +++ b/hw/intc/xive.c > @@ -88,14 +88,40 @@ uint8_t xive_source_esb_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq) > return xive_esb_set(&xsrc->status[srcno], pq); > } > > +/* > + * Returns whether the event notification should be forwarded. > + */ > +static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t > srcno) What exactly "trigger" means isn't entirely obvious for an LSI. Might be clearer to have "lsi_assert" and "lsi_deassert" helpers instead. > +{ > + uint8_t old_pq = xive_source_esb_get(xsrc, srcno); > + > + switch (old_pq) { > + case XIVE_ESB_RESET: > + xive_source_esb_set(xsrc, srcno, XIVE_ESB_PENDING); > + return true; > + default: > + return false; > + } > +} > + > /* > * Returns whether the event notification should be forwarded. > */ > static bool xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno) > { > + bool ret; > + > assert(srcno < xsrc->nr_irqs); > > - return xive_esb_trigger(&xsrc->status[srcno]); > + ret = xive_esb_trigger(&xsrc->status[srcno]); > + > + if (xive_source_irq_is_lsi(xsrc, srcno) && > + xive_source_esb_get(xsrc, srcno) == XIVE_ESB_QUEUED) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "XIVE: queued an event on LSI IRQ %d\n", srcno); > + } > + > + return ret; > } > > /* > @@ -103,9 +129,22 @@ static bool xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno) > */ > static bool xive_source_esb_eoi(XiveSource *xsrc, uint32_t srcno) > { > + bool ret; > + > assert(srcno < xsrc->nr_irqs); > > - return xive_esb_eoi(&xsrc->status[srcno]); > + ret = xive_esb_eoi(&xsrc->status[srcno]); > + > + /* LSI sources do not set the Q bit but they can still be > + * asserted, in which case we should forward a new event > + * notification > + */ > + if (xive_source_irq_is_lsi(xsrc, srcno) && > + xsrc->status[srcno] & XIVE_STATUS_ASSERTED) { > + ret = xive_source_lsi_trigger(xsrc, srcno); > + } > + > + return ret; > } > > /* > @@ -268,8 +307,17 @@ static void xive_source_set_irq(void *opaque, int srcno, int val) > XiveSource *xsrc = XIVE_SOURCE(opaque); > bool notify = false; > > - if (val) { > - notify = xive_source_esb_trigger(xsrc, srcno); > + if (xive_source_irq_is_lsi(xsrc, srcno)) { > + if (val) { > + xsrc->status[srcno] |= XIVE_STATUS_ASSERTED; > + notify = xive_source_lsi_trigger(xsrc, srcno); > + } else { > + xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED; > + } > + } else { > + if (val) { > + notify = xive_source_esb_trigger(xsrc, srcno); > + } > } > > /* Forward the source event notification for routing */ > @@ -289,9 +337,11 @@ void xive_source_pic_print_info(XiveSource *xsrc, uint32_t offset, Monitor *mon) > continue; > } > > - monitor_printf(mon, " %08x %c%c\n", i + offset, > + monitor_printf(mon, " %08x %s %c%c%c\n", i + offset, > + xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI", > pq & XIVE_ESB_VAL_P ? 'P' : '-', > - pq & XIVE_ESB_VAL_Q ? 'Q' : '-'); > + pq & XIVE_ESB_VAL_Q ? 'Q' : '-', > + xsrc->status[i] & XIVE_STATUS_ASSERTED ? 'A' : ' '); > } > } > > @@ -299,6 +349,8 @@ static void xive_source_reset(DeviceState *dev) > { > XiveSource *xsrc = XIVE_SOURCE(dev); > > + /* Do not clear the LSI bitmap */ > + > /* PQs are initialized to 0b01 which corresponds to "ints off" */ > memset(xsrc->status, 0x1, xsrc->nr_irqs); > } > @@ -325,6 +377,9 @@ static void xive_source_realize(DeviceState *dev, Error **errp) > > xsrc->status = g_malloc0(xsrc->nr_irqs); > > + xsrc->lsi_map = bitmap_new(xsrc->nr_irqs); > + xsrc->lsi_map_size = xsrc->nr_irqs; > + > memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc), > &xive_source_esb_ops, xsrc, "xive.esb", > (1ull << xsrc->esb_shift) * xsrc->nr_irqs); > @@ -338,6 +393,7 @@ static const VMStateDescription vmstate_xive_source = { > .fields = (VMStateField[]) { > VMSTATE_UINT32_EQUAL(nr_irqs, XiveSource, NULL), > VMSTATE_VBUFFER_UINT32(status, XiveSource, 1, NULL, nr_irqs), > + VMSTATE_BITMAP(lsi_map, XiveSource, 1, lsi_map_size), This shouldn't be here. The lsi_map is all set up at machine configuration time and then static, so it doesn't need to be migrated. > VMSTATE_END_OF_LIST() > }, > };
On 11/22/18 4:19 AM, David Gibson wrote: > On Fri, Nov 16, 2018 at 11:56:55AM +0100, Cédric Le Goater wrote: >> The 'sent' status of the LSI interrupt source is modeled with the 'P' >> bit of the ESB and the assertion status of the source is maintained in >> an array under the main sPAPRXive object. The type of the source is >> stored in the same array for practical reasons. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > > Looks good except for some minor details. > >> --- >> include/hw/ppc/xive.h | 20 ++++++++++++- >> hw/intc/xive.c | 68 +++++++++++++++++++++++++++++++++++++++---- >> 2 files changed, 81 insertions(+), 7 deletions(-) >> >> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h >> index 5fec4b08705d..e118acd59f1e 100644 >> --- a/include/hw/ppc/xive.h >> +++ b/include/hw/ppc/xive.h >> @@ -32,8 +32,10 @@ typedef struct XiveSource { >> /* IRQs */ >> uint32_t nr_irqs; >> qemu_irq *qirqs; >> + unsigned long *lsi_map; >> + int32_t lsi_map_size; /* for VMSTATE_BITMAP */ > > At some point it's possible we'll want XiveSource subclasses that just > know which irqs are LSI and which aren't without an explicit map. But > this detail isn't exposed in the migration stream or the user > interface, so we can tweak it later as ncessary. > >> - /* PQ bits */ >> + /* PQ bits and LSI assertion bit */ >> uint8_t *status; >> >> /* ESB memory region */ >> @@ -89,6 +91,7 @@ static inline hwaddr xive_source_esb_mgmt(XiveSource *xsrc, int srcno) >> * When doing an EOI, the Q bit will indicate if the interrupt >> * needs to be re-triggered. >> */ >> +#define XIVE_STATUS_ASSERTED 0x4 /* Extra bit for LSI */ >> #define XIVE_ESB_VAL_P 0x2 >> #define XIVE_ESB_VAL_Q 0x1 >> >> @@ -127,4 +130,19 @@ static inline qemu_irq xive_source_qirq(XiveSource *xsrc, uint32_t srcno) >> return xsrc->qirqs[srcno]; >> } >> >> +static inline bool xive_source_irq_is_lsi(XiveSource *xsrc, uint32_t srcno) >> +{ >> + assert(srcno < xsrc->nr_irqs); >> + return test_bit(srcno, xsrc->lsi_map); >> +} >> + >> +static inline void xive_source_irq_set(XiveSource *xsrc, uint32_t srcno, >> + bool lsi) > > The function name isn't obvious about this being controlling LSI > configuration. '..._irq_set_lsi' maybe? yes. >> +{ >> + assert(srcno < xsrc->nr_irqs); >> + if (lsi) { >> + bitmap_set(xsrc->lsi_map, srcno, 1); >> + } >> +} >> + >> #endif /* PPC_XIVE_H */ >> diff --git a/hw/intc/xive.c b/hw/intc/xive.c >> index f7621f84828c..ac4605fee8b7 100644 >> --- a/hw/intc/xive.c >> +++ b/hw/intc/xive.c >> @@ -88,14 +88,40 @@ uint8_t xive_source_esb_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq) >> return xive_esb_set(&xsrc->status[srcno], pq); >> } >> >> +/* >> + * Returns whether the event notification should be forwarded. >> + */ >> +static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t >> srcno) > > What exactly "trigger" means isn't entirely obvious for an LSI. Might > be clearer to have "lsi_assert" and "lsi_deassert" helpers instead. This is called only when the interrupt is asserted. So it is a simplified LSI trigger depending only on the 'P' bit. > >> +{ >> + uint8_t old_pq = xive_source_esb_get(xsrc, srcno); >> + >> + switch (old_pq) { >> + case XIVE_ESB_RESET: >> + xive_source_esb_set(xsrc, srcno, XIVE_ESB_PENDING); >> + return true; >> + default: >> + return false; >> + } >> +} >> + >> /* >> * Returns whether the event notification should be forwarded. >> */ >> static bool xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno) >> { >> + bool ret; >> + >> assert(srcno < xsrc->nr_irqs); >> >> - return xive_esb_trigger(&xsrc->status[srcno]); >> + ret = xive_esb_trigger(&xsrc->status[srcno]); >> + >> + if (xive_source_irq_is_lsi(xsrc, srcno) && >> + xive_source_esb_get(xsrc, srcno) == XIVE_ESB_QUEUED) { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "XIVE: queued an event on LSI IRQ %d\n", srcno); >> + } >> + >> + return ret; >> } >> >> /* >> @@ -103,9 +129,22 @@ static bool xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno) >> */ >> static bool xive_source_esb_eoi(XiveSource *xsrc, uint32_t srcno) >> { >> + bool ret; >> + >> assert(srcno < xsrc->nr_irqs); >> >> - return xive_esb_eoi(&xsrc->status[srcno]); >> + ret = xive_esb_eoi(&xsrc->status[srcno]); >> + >> + /* LSI sources do not set the Q bit but they can still be >> + * asserted, in which case we should forward a new event >> + * notification >> + */ >> + if (xive_source_irq_is_lsi(xsrc, srcno) && >> + xsrc->status[srcno] & XIVE_STATUS_ASSERTED) { >> + ret = xive_source_lsi_trigger(xsrc, srcno); >> + } >> + >> + return ret; >> } >> >> /* >> @@ -268,8 +307,17 @@ static void xive_source_set_irq(void *opaque, int srcno, int val) >> XiveSource *xsrc = XIVE_SOURCE(opaque); >> bool notify = false; >> >> - if (val) { >> - notify = xive_source_esb_trigger(xsrc, srcno); >> + if (xive_source_irq_is_lsi(xsrc, srcno)) { >> + if (val) { >> + xsrc->status[srcno] |= XIVE_STATUS_ASSERTED; >> + notify = xive_source_lsi_trigger(xsrc, srcno); >> + } else { >> + xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED; >> + } >> + } else { >> + if (val) { >> + notify = xive_source_esb_trigger(xsrc, srcno); >> + } >> } >> >> /* Forward the source event notification for routing */ >> @@ -289,9 +337,11 @@ void xive_source_pic_print_info(XiveSource *xsrc, uint32_t offset, Monitor *mon) >> continue; >> } >> >> - monitor_printf(mon, " %08x %c%c\n", i + offset, >> + monitor_printf(mon, " %08x %s %c%c%c\n", i + offset, >> + xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI", >> pq & XIVE_ESB_VAL_P ? 'P' : '-', >> - pq & XIVE_ESB_VAL_Q ? 'Q' : '-'); >> + pq & XIVE_ESB_VAL_Q ? 'Q' : '-', >> + xsrc->status[i] & XIVE_STATUS_ASSERTED ? 'A' : ' '); >> } >> } >> >> @@ -299,6 +349,8 @@ static void xive_source_reset(DeviceState *dev) >> { >> XiveSource *xsrc = XIVE_SOURCE(dev); >> >> + /* Do not clear the LSI bitmap */ >> + >> /* PQs are initialized to 0b01 which corresponds to "ints off" */ >> memset(xsrc->status, 0x1, xsrc->nr_irqs); >> } >> @@ -325,6 +377,9 @@ static void xive_source_realize(DeviceState *dev, Error **errp) >> >> xsrc->status = g_malloc0(xsrc->nr_irqs); >> >> + xsrc->lsi_map = bitmap_new(xsrc->nr_irqs); >> + xsrc->lsi_map_size = xsrc->nr_irqs; >> + >> memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc), >> &xive_source_esb_ops, xsrc, "xive.esb", >> (1ull << xsrc->esb_shift) * xsrc->nr_irqs); >> @@ -338,6 +393,7 @@ static const VMStateDescription vmstate_xive_source = { >> .fields = (VMStateField[]) { >> VMSTATE_UINT32_EQUAL(nr_irqs, XiveSource, NULL), >> VMSTATE_VBUFFER_UINT32(status, XiveSource, 1, NULL, nr_irqs), >> + VMSTATE_BITMAP(lsi_map, XiveSource, 1, lsi_map_size), > > This shouldn't be here. The lsi_map is all set up at machine > configuration time and then static, so it doesn't need to be migrated. yes. of course ... I will get rid of it. Thanks, C. > >> VMSTATE_END_OF_LIST() >> }, >> }; >
On Thu, Nov 22, 2018 at 08:39:41AM +0100, Cédric Le Goater wrote: > On 11/22/18 4:19 AM, David Gibson wrote: > > On Fri, Nov 16, 2018 at 11:56:55AM +0100, Cédric Le Goater wrote: > >> The 'sent' status of the LSI interrupt source is modeled with the 'P' > >> bit of the ESB and the assertion status of the source is maintained in > >> an array under the main sPAPRXive object. The type of the source is > >> stored in the same array for practical reasons. > >> > >> Signed-off-by: Cédric Le Goater <clg@kaod.org> > > > > Looks good except for some minor details. > > > >> --- > >> include/hw/ppc/xive.h | 20 ++++++++++++- > >> hw/intc/xive.c | 68 +++++++++++++++++++++++++++++++++++++++---- > >> 2 files changed, 81 insertions(+), 7 deletions(-) > >> > >> diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h > >> index 5fec4b08705d..e118acd59f1e 100644 > >> --- a/include/hw/ppc/xive.h > >> +++ b/include/hw/ppc/xive.h > >> @@ -32,8 +32,10 @@ typedef struct XiveSource { > >> /* IRQs */ > >> uint32_t nr_irqs; > >> qemu_irq *qirqs; > >> + unsigned long *lsi_map; > >> + int32_t lsi_map_size; /* for VMSTATE_BITMAP */ > > > > At some point it's possible we'll want XiveSource subclasses that just > > know which irqs are LSI and which aren't without an explicit map. But > > this detail isn't exposed in the migration stream or the user > > interface, so we can tweak it later as ncessary. > > > >> - /* PQ bits */ > >> + /* PQ bits and LSI assertion bit */ > >> uint8_t *status; > >> > >> /* ESB memory region */ > >> @@ -89,6 +91,7 @@ static inline hwaddr xive_source_esb_mgmt(XiveSource *xsrc, int srcno) > >> * When doing an EOI, the Q bit will indicate if the interrupt > >> * needs to be re-triggered. > >> */ > >> +#define XIVE_STATUS_ASSERTED 0x4 /* Extra bit for LSI */ > >> #define XIVE_ESB_VAL_P 0x2 > >> #define XIVE_ESB_VAL_Q 0x1 > >> > >> @@ -127,4 +130,19 @@ static inline qemu_irq xive_source_qirq(XiveSource *xsrc, uint32_t srcno) > >> return xsrc->qirqs[srcno]; > >> } > >> > >> +static inline bool xive_source_irq_is_lsi(XiveSource *xsrc, uint32_t srcno) > >> +{ > >> + assert(srcno < xsrc->nr_irqs); > >> + return test_bit(srcno, xsrc->lsi_map); > >> +} > >> + > >> +static inline void xive_source_irq_set(XiveSource *xsrc, uint32_t srcno, > >> + bool lsi) > > > > The function name isn't obvious about this being controlling LSI > > configuration. '..._irq_set_lsi' maybe? > > yes. > > > >> +{ > >> + assert(srcno < xsrc->nr_irqs); > >> + if (lsi) { > >> + bitmap_set(xsrc->lsi_map, srcno, 1); > >> + } > >> +} > >> + > >> #endif /* PPC_XIVE_H */ > >> diff --git a/hw/intc/xive.c b/hw/intc/xive.c > >> index f7621f84828c..ac4605fee8b7 100644 > >> --- a/hw/intc/xive.c > >> +++ b/hw/intc/xive.c > >> @@ -88,14 +88,40 @@ uint8_t xive_source_esb_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq) > >> return xive_esb_set(&xsrc->status[srcno], pq); > >> } > >> > >> +/* > >> + * Returns whether the event notification should be forwarded. > >> + */ > >> +static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t > >> srcno) > > > > What exactly "trigger" means isn't entirely obvious for an LSI. Might > > be clearer to have "lsi_assert" and "lsi_deassert" helpers instead. > > This is called only when the interrupt is asserted. So it is a > simplified LSI trigger depending only on the 'P' bit. Yes, I see that. But the result is that while the MSI logic is encapsulated in the MSI trigger function, this leaves the LSI logic split across the trigger function and set_irq() itself. I think it would be better to have assert and deassert helpers instead, which handle both the trigger/notification and also the updating of the ASSERTED bit. > > > >> +{ > >> + uint8_t old_pq = xive_source_esb_get(xsrc, srcno); > >> + > >> + switch (old_pq) { > >> + case XIVE_ESB_RESET: > >> + xive_source_esb_set(xsrc, srcno, XIVE_ESB_PENDING); > >> + return true; > >> + default: > >> + return false; > >> + } > >> +} > >> + > >> /* > >> * Returns whether the event notification should be forwarded. > >> */ > >> static bool xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno) > >> { > >> + bool ret; > >> + > >> assert(srcno < xsrc->nr_irqs); > >> > >> - return xive_esb_trigger(&xsrc->status[srcno]); > >> + ret = xive_esb_trigger(&xsrc->status[srcno]); > >> + > >> + if (xive_source_irq_is_lsi(xsrc, srcno) && > >> + xive_source_esb_get(xsrc, srcno) == XIVE_ESB_QUEUED) { > >> + qemu_log_mask(LOG_GUEST_ERROR, > >> + "XIVE: queued an event on LSI IRQ %d\n", srcno); > >> + } > >> + > >> + return ret; > >> } > >> > >> /* > >> @@ -103,9 +129,22 @@ static bool xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno) > >> */ > >> static bool xive_source_esb_eoi(XiveSource *xsrc, uint32_t srcno) > >> { > >> + bool ret; > >> + > >> assert(srcno < xsrc->nr_irqs); > >> > >> - return xive_esb_eoi(&xsrc->status[srcno]); > >> + ret = xive_esb_eoi(&xsrc->status[srcno]); > >> + > >> + /* LSI sources do not set the Q bit but they can still be > >> + * asserted, in which case we should forward a new event > >> + * notification > >> + */ > >> + if (xive_source_irq_is_lsi(xsrc, srcno) && > >> + xsrc->status[srcno] & XIVE_STATUS_ASSERTED) { > >> + ret = xive_source_lsi_trigger(xsrc, srcno); > >> + } > >> + > >> + return ret; > >> } > >> > >> /* > >> @@ -268,8 +307,17 @@ static void xive_source_set_irq(void *opaque, int srcno, int val) > >> XiveSource *xsrc = XIVE_SOURCE(opaque); > >> bool notify = false; > >> > >> - if (val) { > >> - notify = xive_source_esb_trigger(xsrc, srcno); > >> + if (xive_source_irq_is_lsi(xsrc, srcno)) { > >> + if (val) { > >> + xsrc->status[srcno] |= XIVE_STATUS_ASSERTED; > >> + notify = xive_source_lsi_trigger(xsrc, srcno); > >> + } else { > >> + xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED; > >> + } > >> + } else { > >> + if (val) { > >> + notify = xive_source_esb_trigger(xsrc, srcno); > >> + } > >> } > >> > >> /* Forward the source event notification for routing */ > >> @@ -289,9 +337,11 @@ void xive_source_pic_print_info(XiveSource *xsrc, uint32_t offset, Monitor *mon) > >> continue; > >> } > >> > >> - monitor_printf(mon, " %08x %c%c\n", i + offset, > >> + monitor_printf(mon, " %08x %s %c%c%c\n", i + offset, > >> + xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI", > >> pq & XIVE_ESB_VAL_P ? 'P' : '-', > >> - pq & XIVE_ESB_VAL_Q ? 'Q' : '-'); > >> + pq & XIVE_ESB_VAL_Q ? 'Q' : '-', > >> + xsrc->status[i] & XIVE_STATUS_ASSERTED ? 'A' : ' '); > >> } > >> } > >> > >> @@ -299,6 +349,8 @@ static void xive_source_reset(DeviceState *dev) > >> { > >> XiveSource *xsrc = XIVE_SOURCE(dev); > >> > >> + /* Do not clear the LSI bitmap */ > >> + > >> /* PQs are initialized to 0b01 which corresponds to "ints off" */ > >> memset(xsrc->status, 0x1, xsrc->nr_irqs); > >> } > >> @@ -325,6 +377,9 @@ static void xive_source_realize(DeviceState *dev, Error **errp) > >> > >> xsrc->status = g_malloc0(xsrc->nr_irqs); > >> > >> + xsrc->lsi_map = bitmap_new(xsrc->nr_irqs); > >> + xsrc->lsi_map_size = xsrc->nr_irqs; > >> + > >> memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc), > >> &xive_source_esb_ops, xsrc, "xive.esb", > >> (1ull << xsrc->esb_shift) * xsrc->nr_irqs); > >> @@ -338,6 +393,7 @@ static const VMStateDescription vmstate_xive_source = { > >> .fields = (VMStateField[]) { > >> VMSTATE_UINT32_EQUAL(nr_irqs, XiveSource, NULL), > >> VMSTATE_VBUFFER_UINT32(status, XiveSource, 1, NULL, nr_irqs), > >> + VMSTATE_BITMAP(lsi_map, XiveSource, 1, lsi_map_size), > > > > This shouldn't be here. The lsi_map is all set up at machine > > configuration time and then static, so it doesn't need to be migrated. > > yes. of course ... I will get rid of it. > > Thanks, > > C. > > > >> VMSTATE_END_OF_LIST() > >> }, > >> }; > > >
>>>> +/* >>>> + * Returns whether the event notification should be forwarded. >>>> + */ >>>> +static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t >>>> srcno) >>> >>> What exactly "trigger" means isn't entirely obvious for an LSI. Might >>> be clearer to have "lsi_assert" and "lsi_deassert" helpers instead. >> >> This is called only when the interrupt is asserted. So it is a >> simplified LSI trigger depending only on the 'P' bit. > > Yes, I see that. But the result is that while the MSI logic is > encapsulated in the MSI trigger function, this leaves the LSI logic > split across the trigger function and set_irq() itself. I think it > would be better to have assert and deassert helpers instead, which > handle both the trigger/notification and also the updating of the > ASSERTED bit. Something like the xive_source_set_irq_lsi() below ? Thanks, C. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/intc/xive.c | 58 ++++++++++++++++++++++++++++++++++++++++++++------ include/hw/ppc/xive.h | 19 +++++++++++++++- 2 files changed, 70 insertions(+), 7 deletions(-) Index: qemu.git/include/hw/ppc/xive.h =================================================================== --- qemu.git.orig/include/hw/ppc/xive.h +++ qemu.git/include/hw/ppc/xive.h @@ -32,8 +32,9 @@ typedef struct XiveSource { /* IRQs */ uint32_t nr_irqs; qemu_irq *qirqs; + unsigned long *lsi_map; - /* PQ bits */ + /* PQ bits and LSI assertion bit */ uint8_t *status; /* ESB memory region */ @@ -89,6 +90,7 @@ static inline hwaddr xive_source_esb_mgm * When doing an EOI, the Q bit will indicate if the interrupt * needs to be re-triggered. */ +#define XIVE_STATUS_ASSERTED 0x4 /* Extra bit for LSI */ #define XIVE_ESB_VAL_P 0x2 #define XIVE_ESB_VAL_Q 0x1 @@ -127,4 +129,19 @@ static inline qemu_irq xive_source_qirq( return xsrc->qirqs[srcno]; } +static inline bool xive_source_irq_is_lsi(XiveSource *xsrc, uint32_t srcno) +{ + assert(srcno < xsrc->nr_irqs); + return test_bit(srcno, xsrc->lsi_map); +} + +static inline void xive_source_irq_set(XiveSource *xsrc, uint32_t srcno, + bool lsi) +{ + assert(srcno < xsrc->nr_irqs); + if (lsi) { + bitmap_set(xsrc->lsi_map, srcno, 1); + } +} + #endif /* PPC_XIVE_H */ Index: qemu.git/hw/intc/xive.c =================================================================== --- qemu.git.orig/hw/intc/xive.c +++ qemu.git/hw/intc/xive.c @@ -91,11 +91,35 @@ uint8_t xive_source_esb_set(XiveSource * /* * Returns whether the event notification should be forwarded. */ +static bool xive_source_set_irq_lsi(XiveSource *xsrc, uint32_t srcno, int val) +{ + if (!val) { + xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED; + return false; + } + + xsrc->status[srcno] |= XIVE_STATUS_ASSERTED; + return xive_source_esb_get(xsrc, srcno) == XIVE_ESB_RESET; +} + +/* + * Returns whether the event notification should be forwarded. + */ static bool xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno) { + bool notify; + assert(srcno < xsrc->nr_irqs); - return xive_esb_trigger(&xsrc->status[srcno]); + notify = xive_esb_trigger(&xsrc->status[srcno]); + + if (xive_source_irq_is_lsi(xsrc, srcno) && + xive_source_esb_get(xsrc, srcno) == XIVE_ESB_QUEUED) { + qemu_log_mask(LOG_GUEST_ERROR, + "XIVE: queued an event on LSI IRQ %d\n", srcno); + } + + return notify; } /* @@ -103,9 +127,22 @@ static bool xive_source_esb_trigger(Xive */ static bool xive_source_esb_eoi(XiveSource *xsrc, uint32_t srcno) { + bool notify; + assert(srcno < xsrc->nr_irqs); - return xive_esb_eoi(&xsrc->status[srcno]); + notify = xive_esb_eoi(&xsrc->status[srcno]); + + /* LSI sources do not set the Q bit but they can still be + * asserted, in which case we should forward a new event + * notification + */ + if (xive_source_irq_is_lsi(xsrc, srcno)) { + bool level = xsrc->status[srcno] & XIVE_STATUS_ASSERTED; + notify = xive_source_set_irq_lsi(xsrc, srcno, level); + } + + return notify; } /* @@ -268,8 +305,12 @@ static void xive_source_set_irq(void *op XiveSource *xsrc = XIVE_SOURCE(opaque); bool notify = false; - if (val) { - notify = xive_source_esb_trigger(xsrc, srcno); + if (xive_source_irq_is_lsi(xsrc, srcno)) { + notify = xive_source_set_irq_lsi(xsrc, srcno, val); + } else { + if (val) { + notify = xive_source_esb_trigger(xsrc, srcno); + } } /* Forward the source event notification for routing */ @@ -289,9 +330,11 @@ void xive_source_pic_print_info(XiveSour continue; } - monitor_printf(mon, " %08x %c%c\n", i + offset, + monitor_printf(mon, " %08x %s %c%c%c\n", i + offset, + xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI", pq & XIVE_ESB_VAL_P ? 'P' : '-', - pq & XIVE_ESB_VAL_Q ? 'Q' : '-'); + pq & XIVE_ESB_VAL_Q ? 'Q' : '-', + xsrc->status[i] & XIVE_STATUS_ASSERTED ? 'A' : ' '); } } @@ -299,6 +342,8 @@ static void xive_source_reset(DeviceStat { XiveSource *xsrc = XIVE_SOURCE(dev); + /* Do not clear the LSI bitmap */ + /* PQs are initialized to 0b01 which corresponds to "ints off" */ memset(xsrc->status, 0x1, xsrc->nr_irqs); } @@ -324,6 +369,7 @@ static void xive_source_realize(DeviceSt xsrc->nr_irqs); xsrc->status = g_malloc0(xsrc->nr_irqs); + xsrc->lsi_map = bitmap_new(xsrc->nr_irqs); memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc), &xive_source_esb_ops, xsrc, "xive.esb",
On Fri, Nov 23, 2018 at 02:28:35PM +0100, Cédric Le Goater wrote: > > >>>> +/* > >>>> + * Returns whether the event notification should be forwarded. > >>>> + */ > >>>> +static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t > >>>> srcno) > >>> > >>> What exactly "trigger" means isn't entirely obvious for an LSI. Might > >>> be clearer to have "lsi_assert" and "lsi_deassert" helpers instead. > >> > >> This is called only when the interrupt is asserted. So it is a > >> simplified LSI trigger depending only on the 'P' bit. > > > > Yes, I see that. But the result is that while the MSI logic is > > encapsulated in the MSI trigger function, this leaves the LSI logic > > split across the trigger function and set_irq() itself. I think it > > would be better to have assert and deassert helpers instead, which > > handle both the trigger/notification and also the updating of the > > ASSERTED bit. > > Something like the xive_source_set_irq_lsi() below ? Uh.. not exactly what I had in mind, but close enough. [snip] +/* > + * Returns whether the event notification should be forwarded. > + */ > static bool xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno) > { > + bool notify; > + > assert(srcno < xsrc->nr_irqs); > > - return xive_esb_trigger(&xsrc->status[srcno]); > + notify = xive_esb_trigger(&xsrc->status[srcno]); > + > + if (xive_source_irq_is_lsi(xsrc, srcno) && Except that this block can go, since this function is no longer called for LSIs. > + xive_source_esb_get(xsrc, srcno) == XIVE_ESB_QUEUED) { > + qemu_log_mask(LOG_GUEST_ERROR, > + "XIVE: queued an event on LSI IRQ %d\n", srcno); > + } > + > + return notify; > } > > /* > @@ -103,9 +127,22 @@ static bool xive_source_esb_trigger(Xive > */ > static bool xive_source_esb_eoi(XiveSource *xsrc, uint32_t srcno) > { > + bool notify; > + > assert(srcno < xsrc->nr_irqs); > > - return xive_esb_eoi(&xsrc->status[srcno]); > + notify = xive_esb_eoi(&xsrc->status[srcno]); > + > + /* LSI sources do not set the Q bit but they can still be > + * asserted, in which case we should forward a new event > + * notification > + */ > + if (xive_source_irq_is_lsi(xsrc, srcno)) { > + bool level = xsrc->status[srcno] & XIVE_STATUS_ASSERTED; > + notify = xive_source_set_irq_lsi(xsrc, srcno, level); > + } > + > + return notify; > } > > /* > @@ -268,8 +305,12 @@ static void xive_source_set_irq(void *op > XiveSource *xsrc = XIVE_SOURCE(opaque); > bool notify = false; > > - if (val) { > - notify = xive_source_esb_trigger(xsrc, srcno); > + if (xive_source_irq_is_lsi(xsrc, srcno)) { > + notify = xive_source_set_irq_lsi(xsrc, srcno, val); > + } else { > + if (val) { > + notify = xive_source_esb_trigger(xsrc, srcno); > + } > } > > /* Forward the source event notification for routing */ > @@ -289,9 +330,11 @@ void xive_source_pic_print_info(XiveSour > continue; > } > > - monitor_printf(mon, " %08x %c%c\n", i + offset, > + monitor_printf(mon, " %08x %s %c%c%c\n", i + offset, > + xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI", > pq & XIVE_ESB_VAL_P ? 'P' : '-', > - pq & XIVE_ESB_VAL_Q ? 'Q' : '-'); > + pq & XIVE_ESB_VAL_Q ? 'Q' : '-', > + xsrc->status[i] & XIVE_STATUS_ASSERTED ? 'A' : ' '); > } > } > > @@ -299,6 +342,8 @@ static void xive_source_reset(DeviceStat > { > XiveSource *xsrc = XIVE_SOURCE(dev); > > + /* Do not clear the LSI bitmap */ > + > /* PQs are initialized to 0b01 which corresponds to "ints off" */ > memset(xsrc->status, 0x1, xsrc->nr_irqs); > } > @@ -324,6 +369,7 @@ static void xive_source_realize(DeviceSt > xsrc->nr_irqs); > > xsrc->status = g_malloc0(xsrc->nr_irqs); > + xsrc->lsi_map = bitmap_new(xsrc->nr_irqs); > > memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc), > &xive_source_esb_ops, xsrc, "xive.esb", >
On 11/26/18 6:39 AM, David Gibson wrote: > On Fri, Nov 23, 2018 at 02:28:35PM +0100, Cédric Le Goater wrote: >> >>>>>> +/* >>>>>> + * Returns whether the event notification should be forwarded. >>>>>> + */ >>>>>> +static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t >>>>>> srcno) >>>>> >>>>> What exactly "trigger" means isn't entirely obvious for an LSI. Might >>>>> be clearer to have "lsi_assert" and "lsi_deassert" helpers instead. >>>> >>>> This is called only when the interrupt is asserted. So it is a >>>> simplified LSI trigger depending only on the 'P' bit. >>> >>> Yes, I see that. But the result is that while the MSI logic is >>> encapsulated in the MSI trigger function, this leaves the LSI logic >>> split across the trigger function and set_irq() itself. I think it >>> would be better to have assert and deassert helpers instead, which >>> handle both the trigger/notification and also the updating of the >>> ASSERTED bit. >> >> Something like the xive_source_set_irq_lsi() below ? > > Uh.. not exactly what I had in mind, but close enough. > > [snip] > +/* >> + * Returns whether the event notification should be forwarded. >> + */ >> static bool xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno) >> { >> + bool notify; >> + >> assert(srcno < xsrc->nr_irqs); >> >> - return xive_esb_trigger(&xsrc->status[srcno]); >> + notify = xive_esb_trigger(&xsrc->status[srcno]); >> + >> + if (xive_source_irq_is_lsi(xsrc, srcno) && > > Except that this block can go, since this function is no longer called > for LSIs. It still can be through the ESB MMIOs, if the guest does a load on the trigger page. C. > >> + xive_source_esb_get(xsrc, srcno) == XIVE_ESB_QUEUED) { >> + qemu_log_mask(LOG_GUEST_ERROR, >> + "XIVE: queued an event on LSI IRQ %d\n", srcno); >> + } >> + >> + return notify; >> } >> >> /* >> @@ -103,9 +127,22 @@ static bool xive_source_esb_trigger(Xive >> */ >> static bool xive_source_esb_eoi(XiveSource *xsrc, uint32_t srcno) >> { >> + bool notify; >> + >> assert(srcno < xsrc->nr_irqs); >> >> - return xive_esb_eoi(&xsrc->status[srcno]); >> + notify = xive_esb_eoi(&xsrc->status[srcno]); >> + >> + /* LSI sources do not set the Q bit but they can still be >> + * asserted, in which case we should forward a new event >> + * notification >> + */ >> + if (xive_source_irq_is_lsi(xsrc, srcno)) { >> + bool level = xsrc->status[srcno] & XIVE_STATUS_ASSERTED; >> + notify = xive_source_set_irq_lsi(xsrc, srcno, level); >> + } >> + >> + return notify; >> } >> >> /* >> @@ -268,8 +305,12 @@ static void xive_source_set_irq(void *op >> XiveSource *xsrc = XIVE_SOURCE(opaque); >> bool notify = false; >> >> - if (val) { >> - notify = xive_source_esb_trigger(xsrc, srcno); >> + if (xive_source_irq_is_lsi(xsrc, srcno)) { >> + notify = xive_source_set_irq_lsi(xsrc, srcno, val); >> + } else { >> + if (val) { >> + notify = xive_source_esb_trigger(xsrc, srcno); >> + } >> } >> >> /* Forward the source event notification for routing */ >> @@ -289,9 +330,11 @@ void xive_source_pic_print_info(XiveSour >> continue; >> } >> >> - monitor_printf(mon, " %08x %c%c\n", i + offset, >> + monitor_printf(mon, " %08x %s %c%c%c\n", i + offset, >> + xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI", >> pq & XIVE_ESB_VAL_P ? 'P' : '-', >> - pq & XIVE_ESB_VAL_Q ? 'Q' : '-'); >> + pq & XIVE_ESB_VAL_Q ? 'Q' : '-', >> + xsrc->status[i] & XIVE_STATUS_ASSERTED ? 'A' : ' '); >> } >> } >> >> @@ -299,6 +342,8 @@ static void xive_source_reset(DeviceStat >> { >> XiveSource *xsrc = XIVE_SOURCE(dev); >> >> + /* Do not clear the LSI bitmap */ >> + >> /* PQs are initialized to 0b01 which corresponds to "ints off" */ >> memset(xsrc->status, 0x1, xsrc->nr_irqs); >> } >> @@ -324,6 +369,7 @@ static void xive_source_realize(DeviceSt >> xsrc->nr_irqs); >> >> xsrc->status = g_malloc0(xsrc->nr_irqs); >> + xsrc->lsi_map = bitmap_new(xsrc->nr_irqs); >> >> memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc), >> &xive_source_esb_ops, xsrc, "xive.esb", >> >
On Mon, Nov 26, 2018 at 12:20:19PM +0100, Cédric Le Goater wrote: > On 11/26/18 6:39 AM, David Gibson wrote: > > On Fri, Nov 23, 2018 at 02:28:35PM +0100, Cédric Le Goater wrote: > >> > >>>>>> +/* > >>>>>> + * Returns whether the event notification should be forwarded. > >>>>>> + */ > >>>>>> +static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t > >>>>>> srcno) > >>>>> > >>>>> What exactly "trigger" means isn't entirely obvious for an LSI. Might > >>>>> be clearer to have "lsi_assert" and "lsi_deassert" helpers instead. > >>>> > >>>> This is called only when the interrupt is asserted. So it is a > >>>> simplified LSI trigger depending only on the 'P' bit. > >>> > >>> Yes, I see that. But the result is that while the MSI logic is > >>> encapsulated in the MSI trigger function, this leaves the LSI logic > >>> split across the trigger function and set_irq() itself. I think it > >>> would be better to have assert and deassert helpers instead, which > >>> handle both the trigger/notification and also the updating of the > >>> ASSERTED bit. > >> > >> Something like the xive_source_set_irq_lsi() below ? > > > > Uh.. not exactly what I had in mind, but close enough. > > > > [snip] > > +/* > >> + * Returns whether the event notification should be forwarded. > >> + */ > >> static bool xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno) > >> { > >> + bool notify; > >> + > >> assert(srcno < xsrc->nr_irqs); > >> > >> - return xive_esb_trigger(&xsrc->status[srcno]); > >> + notify = xive_esb_trigger(&xsrc->status[srcno]); > >> + > >> + if (xive_source_irq_is_lsi(xsrc, srcno) && > > > > Except that this block can go, since this function is no longer called > > for LSIs. > > It still can be through the ESB MMIOs, if the guest does a load on the > trigger page. Oh, good point. That makes me rethink all my comments on this matter. In that case I think your original code was fine, except that I'd prefer to see the setting of the ASSERTED bit inside the trigger function, instead of in the set_irq() caller. > > C. > > > > > >> + xive_source_esb_get(xsrc, srcno) == XIVE_ESB_QUEUED) { > >> + qemu_log_mask(LOG_GUEST_ERROR, > >> + "XIVE: queued an event on LSI IRQ %d\n", srcno); > >> + } > >> + > >> + return notify; > >> } > >> > >> /* > >> @@ -103,9 +127,22 @@ static bool xive_source_esb_trigger(Xive > >> */ > >> static bool xive_source_esb_eoi(XiveSource *xsrc, uint32_t srcno) > >> { > >> + bool notify; > >> + > >> assert(srcno < xsrc->nr_irqs); > >> > >> - return xive_esb_eoi(&xsrc->status[srcno]); > >> + notify = xive_esb_eoi(&xsrc->status[srcno]); > >> + > >> + /* LSI sources do not set the Q bit but they can still be > >> + * asserted, in which case we should forward a new event > >> + * notification > >> + */ > >> + if (xive_source_irq_is_lsi(xsrc, srcno)) { > >> + bool level = xsrc->status[srcno] & XIVE_STATUS_ASSERTED; > >> + notify = xive_source_set_irq_lsi(xsrc, srcno, level); > >> + } > >> + > >> + return notify; > >> } > >> > >> /* > >> @@ -268,8 +305,12 @@ static void xive_source_set_irq(void *op > >> XiveSource *xsrc = XIVE_SOURCE(opaque); > >> bool notify = false; > >> > >> - if (val) { > >> - notify = xive_source_esb_trigger(xsrc, srcno); > >> + if (xive_source_irq_is_lsi(xsrc, srcno)) { > >> + notify = xive_source_set_irq_lsi(xsrc, srcno, val); > >> + } else { > >> + if (val) { > >> + notify = xive_source_esb_trigger(xsrc, srcno); > >> + } > >> } > >> > >> /* Forward the source event notification for routing */ > >> @@ -289,9 +330,11 @@ void xive_source_pic_print_info(XiveSour > >> continue; > >> } > >> > >> - monitor_printf(mon, " %08x %c%c\n", i + offset, > >> + monitor_printf(mon, " %08x %s %c%c%c\n", i + offset, > >> + xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI", > >> pq & XIVE_ESB_VAL_P ? 'P' : '-', > >> - pq & XIVE_ESB_VAL_Q ? 'Q' : '-'); > >> + pq & XIVE_ESB_VAL_Q ? 'Q' : '-', > >> + xsrc->status[i] & XIVE_STATUS_ASSERTED ? 'A' : ' '); > >> } > >> } > >> > >> @@ -299,6 +342,8 @@ static void xive_source_reset(DeviceStat > >> { > >> XiveSource *xsrc = XIVE_SOURCE(dev); > >> > >> + /* Do not clear the LSI bitmap */ > >> + > >> /* PQs are initialized to 0b01 which corresponds to "ints off" */ > >> memset(xsrc->status, 0x1, xsrc->nr_irqs); > >> } > >> @@ -324,6 +369,7 @@ static void xive_source_realize(DeviceSt > >> xsrc->nr_irqs); > >> > >> xsrc->status = g_malloc0(xsrc->nr_irqs); > >> + xsrc->lsi_map = bitmap_new(xsrc->nr_irqs); > >> > >> memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc), > >> &xive_source_esb_ops, xsrc, "xive.esb", > >> > > >
On 11/27/18 12:48 AM, David Gibson wrote: > On Mon, Nov 26, 2018 at 12:20:19PM +0100, Cédric Le Goater wrote: >> On 11/26/18 6:39 AM, David Gibson wrote: >>> On Fri, Nov 23, 2018 at 02:28:35PM +0100, Cédric Le Goater wrote: >>>> >>>>>>>> +/* >>>>>>>> + * Returns whether the event notification should be forwarded. >>>>>>>> + */ >>>>>>>> +static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t >>>>>>>> srcno) >>>>>>> >>>>>>> What exactly "trigger" means isn't entirely obvious for an LSI. Might >>>>>>> be clearer to have "lsi_assert" and "lsi_deassert" helpers instead. >>>>>> >>>>>> This is called only when the interrupt is asserted. So it is a >>>>>> simplified LSI trigger depending only on the 'P' bit. >>>>> >>>>> Yes, I see that. But the result is that while the MSI logic is >>>>> encapsulated in the MSI trigger function, this leaves the LSI logic >>>>> split across the trigger function and set_irq() itself. I think it >>>>> would be better to have assert and deassert helpers instead, which >>>>> handle both the trigger/notification and also the updating of the >>>>> ASSERTED bit. >>>> >>>> Something like the xive_source_set_irq_lsi() below ? >>> >>> Uh.. not exactly what I had in mind, but close enough. >>> >>> [snip] >>> +/* >>>> + * Returns whether the event notification should be forwarded. >>>> + */ >>>> static bool xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno) >>>> { >>>> + bool notify; >>>> + >>>> assert(srcno < xsrc->nr_irqs); >>>> >>>> - return xive_esb_trigger(&xsrc->status[srcno]); >>>> + notify = xive_esb_trigger(&xsrc->status[srcno]); >>>> + >>>> + if (xive_source_irq_is_lsi(xsrc, srcno) && >>> >>> Except that this block can go, since this function is no longer called >>> for LSIs. >> >> It still can be through the ESB MMIOs, if the guest does a load on the >> trigger page. > > Oh, good point. That makes me rethink all my comments on this matter. > > In that case I think your original code was fine, except that I'd > prefer to see the setting of the ASSERTED bit inside the trigger > function, instead of in the set_irq() caller. ok. I will change that. Thanks, C. > > >> >> C. >> >> >>> >>>> + xive_source_esb_get(xsrc, srcno) == XIVE_ESB_QUEUED) { >>>> + qemu_log_mask(LOG_GUEST_ERROR, >>>> + "XIVE: queued an event on LSI IRQ %d\n", srcno); >>>> + } >>>> + >>>> + return notify; >>>> } >>>> >>>> /* >>>> @@ -103,9 +127,22 @@ static bool xive_source_esb_trigger(Xive >>>> */ >>>> static bool xive_source_esb_eoi(XiveSource *xsrc, uint32_t srcno) >>>> { >>>> + bool notify; >>>> + >>>> assert(srcno < xsrc->nr_irqs); >>>> >>>> - return xive_esb_eoi(&xsrc->status[srcno]); >>>> + notify = xive_esb_eoi(&xsrc->status[srcno]); >>>> + >>>> + /* LSI sources do not set the Q bit but they can still be >>>> + * asserted, in which case we should forward a new event >>>> + * notification >>>> + */ >>>> + if (xive_source_irq_is_lsi(xsrc, srcno)) { >>>> + bool level = xsrc->status[srcno] & XIVE_STATUS_ASSERTED; >>>> + notify = xive_source_set_irq_lsi(xsrc, srcno, level); >>>> + } >>>> + >>>> + return notify; >>>> } >>>> >>>> /* >>>> @@ -268,8 +305,12 @@ static void xive_source_set_irq(void *op >>>> XiveSource *xsrc = XIVE_SOURCE(opaque); >>>> bool notify = false; >>>> >>>> - if (val) { >>>> - notify = xive_source_esb_trigger(xsrc, srcno); >>>> + if (xive_source_irq_is_lsi(xsrc, srcno)) { >>>> + notify = xive_source_set_irq_lsi(xsrc, srcno, val); >>>> + } else { >>>> + if (val) { >>>> + notify = xive_source_esb_trigger(xsrc, srcno); >>>> + } >>>> } >>>> >>>> /* Forward the source event notification for routing */ >>>> @@ -289,9 +330,11 @@ void xive_source_pic_print_info(XiveSour >>>> continue; >>>> } >>>> >>>> - monitor_printf(mon, " %08x %c%c\n", i + offset, >>>> + monitor_printf(mon, " %08x %s %c%c%c\n", i + offset, >>>> + xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI", >>>> pq & XIVE_ESB_VAL_P ? 'P' : '-', >>>> - pq & XIVE_ESB_VAL_Q ? 'Q' : '-'); >>>> + pq & XIVE_ESB_VAL_Q ? 'Q' : '-', >>>> + xsrc->status[i] & XIVE_STATUS_ASSERTED ? 'A' : ' '); >>>> } >>>> } >>>> >>>> @@ -299,6 +342,8 @@ static void xive_source_reset(DeviceStat >>>> { >>>> XiveSource *xsrc = XIVE_SOURCE(dev); >>>> >>>> + /* Do not clear the LSI bitmap */ >>>> + >>>> /* PQs are initialized to 0b01 which corresponds to "ints off" */ >>>> memset(xsrc->status, 0x1, xsrc->nr_irqs); >>>> } >>>> @@ -324,6 +369,7 @@ static void xive_source_realize(DeviceSt >>>> xsrc->nr_irqs); >>>> >>>> xsrc->status = g_malloc0(xsrc->nr_irqs); >>>> + xsrc->lsi_map = bitmap_new(xsrc->nr_irqs); >>>> >>>> memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc), >>>> &xive_source_esb_ops, xsrc, "xive.esb", >>>> >>> >> >
diff --git a/include/hw/ppc/xive.h b/include/hw/ppc/xive.h index 5fec4b08705d..e118acd59f1e 100644 --- a/include/hw/ppc/xive.h +++ b/include/hw/ppc/xive.h @@ -32,8 +32,10 @@ typedef struct XiveSource { /* IRQs */ uint32_t nr_irqs; qemu_irq *qirqs; + unsigned long *lsi_map; + int32_t lsi_map_size; /* for VMSTATE_BITMAP */ - /* PQ bits */ + /* PQ bits and LSI assertion bit */ uint8_t *status; /* ESB memory region */ @@ -89,6 +91,7 @@ static inline hwaddr xive_source_esb_mgmt(XiveSource *xsrc, int srcno) * When doing an EOI, the Q bit will indicate if the interrupt * needs to be re-triggered. */ +#define XIVE_STATUS_ASSERTED 0x4 /* Extra bit for LSI */ #define XIVE_ESB_VAL_P 0x2 #define XIVE_ESB_VAL_Q 0x1 @@ -127,4 +130,19 @@ static inline qemu_irq xive_source_qirq(XiveSource *xsrc, uint32_t srcno) return xsrc->qirqs[srcno]; } +static inline bool xive_source_irq_is_lsi(XiveSource *xsrc, uint32_t srcno) +{ + assert(srcno < xsrc->nr_irqs); + return test_bit(srcno, xsrc->lsi_map); +} + +static inline void xive_source_irq_set(XiveSource *xsrc, uint32_t srcno, + bool lsi) +{ + assert(srcno < xsrc->nr_irqs); + if (lsi) { + bitmap_set(xsrc->lsi_map, srcno, 1); + } +} + #endif /* PPC_XIVE_H */ diff --git a/hw/intc/xive.c b/hw/intc/xive.c index f7621f84828c..ac4605fee8b7 100644 --- a/hw/intc/xive.c +++ b/hw/intc/xive.c @@ -88,14 +88,40 @@ uint8_t xive_source_esb_set(XiveSource *xsrc, uint32_t srcno, uint8_t pq) return xive_esb_set(&xsrc->status[srcno], pq); } +/* + * Returns whether the event notification should be forwarded. + */ +static bool xive_source_lsi_trigger(XiveSource *xsrc, uint32_t srcno) +{ + uint8_t old_pq = xive_source_esb_get(xsrc, srcno); + + switch (old_pq) { + case XIVE_ESB_RESET: + xive_source_esb_set(xsrc, srcno, XIVE_ESB_PENDING); + return true; + default: + return false; + } +} + /* * Returns whether the event notification should be forwarded. */ static bool xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno) { + bool ret; + assert(srcno < xsrc->nr_irqs); - return xive_esb_trigger(&xsrc->status[srcno]); + ret = xive_esb_trigger(&xsrc->status[srcno]); + + if (xive_source_irq_is_lsi(xsrc, srcno) && + xive_source_esb_get(xsrc, srcno) == XIVE_ESB_QUEUED) { + qemu_log_mask(LOG_GUEST_ERROR, + "XIVE: queued an event on LSI IRQ %d\n", srcno); + } + + return ret; } /* @@ -103,9 +129,22 @@ static bool xive_source_esb_trigger(XiveSource *xsrc, uint32_t srcno) */ static bool xive_source_esb_eoi(XiveSource *xsrc, uint32_t srcno) { + bool ret; + assert(srcno < xsrc->nr_irqs); - return xive_esb_eoi(&xsrc->status[srcno]); + ret = xive_esb_eoi(&xsrc->status[srcno]); + + /* LSI sources do not set the Q bit but they can still be + * asserted, in which case we should forward a new event + * notification + */ + if (xive_source_irq_is_lsi(xsrc, srcno) && + xsrc->status[srcno] & XIVE_STATUS_ASSERTED) { + ret = xive_source_lsi_trigger(xsrc, srcno); + } + + return ret; } /* @@ -268,8 +307,17 @@ static void xive_source_set_irq(void *opaque, int srcno, int val) XiveSource *xsrc = XIVE_SOURCE(opaque); bool notify = false; - if (val) { - notify = xive_source_esb_trigger(xsrc, srcno); + if (xive_source_irq_is_lsi(xsrc, srcno)) { + if (val) { + xsrc->status[srcno] |= XIVE_STATUS_ASSERTED; + notify = xive_source_lsi_trigger(xsrc, srcno); + } else { + xsrc->status[srcno] &= ~XIVE_STATUS_ASSERTED; + } + } else { + if (val) { + notify = xive_source_esb_trigger(xsrc, srcno); + } } /* Forward the source event notification for routing */ @@ -289,9 +337,11 @@ void xive_source_pic_print_info(XiveSource *xsrc, uint32_t offset, Monitor *mon) continue; } - monitor_printf(mon, " %08x %c%c\n", i + offset, + monitor_printf(mon, " %08x %s %c%c%c\n", i + offset, + xive_source_irq_is_lsi(xsrc, i) ? "LSI" : "MSI", pq & XIVE_ESB_VAL_P ? 'P' : '-', - pq & XIVE_ESB_VAL_Q ? 'Q' : '-'); + pq & XIVE_ESB_VAL_Q ? 'Q' : '-', + xsrc->status[i] & XIVE_STATUS_ASSERTED ? 'A' : ' '); } } @@ -299,6 +349,8 @@ static void xive_source_reset(DeviceState *dev) { XiveSource *xsrc = XIVE_SOURCE(dev); + /* Do not clear the LSI bitmap */ + /* PQs are initialized to 0b01 which corresponds to "ints off" */ memset(xsrc->status, 0x1, xsrc->nr_irqs); } @@ -325,6 +377,9 @@ static void xive_source_realize(DeviceState *dev, Error **errp) xsrc->status = g_malloc0(xsrc->nr_irqs); + xsrc->lsi_map = bitmap_new(xsrc->nr_irqs); + xsrc->lsi_map_size = xsrc->nr_irqs; + memory_region_init_io(&xsrc->esb_mmio, OBJECT(xsrc), &xive_source_esb_ops, xsrc, "xive.esb", (1ull << xsrc->esb_shift) * xsrc->nr_irqs); @@ -338,6 +393,7 @@ static const VMStateDescription vmstate_xive_source = { .fields = (VMStateField[]) { VMSTATE_UINT32_EQUAL(nr_irqs, XiveSource, NULL), VMSTATE_VBUFFER_UINT32(status, XiveSource, 1, NULL, nr_irqs), + VMSTATE_BITMAP(lsi_map, XiveSource, 1, lsi_map_size), VMSTATE_END_OF_LIST() }, };
The 'sent' status of the LSI interrupt source is modeled with the 'P' bit of the ESB and the assertion status of the source is maintained in an array under the main sPAPRXive object. The type of the source is stored in the same array for practical reasons. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- include/hw/ppc/xive.h | 20 ++++++++++++- hw/intc/xive.c | 68 +++++++++++++++++++++++++++++++++++++++---- 2 files changed, 81 insertions(+), 7 deletions(-)