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.
>
>> +{
>> + 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()
>> },
>> };
>
