Re: [Qemu-ppc] [PATCH v5 02/36] ppc/xive: add support for the LSI interrupt sources

[Cédric Le Goater]

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