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",
>>>>
>>>
>>
>
