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 <c...@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() > >> }, > >> }; > > > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature