Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/6] xics: add flags for interrupts

[Alexander Graf <agraf@suse.de>]

On 05/08/2014 01:52 PM, Alexander Graf wrote:
> On 05/07/2014 08:01 AM, Alexey Kardashevskiy wrote:
>> The existing interrupt allocation scheme in SPAPR assumes that
>> interrupts are allocated at the start time, continously and the config
>> will not change. However, there are cases when this is not going to work
>> such as:
>>
>> 1. migration - we will have to have an ability to choose interrupt
>> numbers for devices in the command line and this will create gaps in
>> interrupt space.
>>
>> 2. PCI hotplug - interrupts from unplugged device need to be returned
>> back to interrupt pool, otherwise we will quickly run out of interrupts.
>>
>> This replaces a separate lslsi[] array with a byte in the ICSIRQState
>> struct and defines "LSI" and "MSI" flags. Neither of these flags set
>> signals that the descriptor is not allocated and not in use.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>   hw/intc/xics.c        | 21 ++++++++++++++-------
>>   hw/intc/xics_kvm.c    |  5 ++---
>>   include/hw/ppc/xics.h |  5 ++++-
>>   3 files changed, 20 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
>> index 64aabe7..1f89a00 100644
>> --- a/hw/intc/xics.c
>> +++ b/hw/intc/xics.c
>> @@ -438,7 +438,7 @@ static void ics_set_irq(void *opaque, int srcno, 
>> int val)
>>   {
>>       ICSState *ics = (ICSState *)opaque;
>>   -    if (ics->islsi[srcno]) {
>> +    if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) {
>>           set_irq_lsi(ics, srcno, val);
>>       } else {
>>           set_irq_msi(ics, srcno, val);
>> @@ -475,7 +475,7 @@ static void ics_write_xive(ICSState *ics, int nr, 
>> int server,
>>         trace_xics_ics_write_xive(nr, srcno, server, priority);
>>   -    if (ics->islsi[srcno]) {
>> +    if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) {
>>           write_xive_lsi(ics, srcno);
>>       } else {
>>           write_xive_msi(ics, srcno);
>> @@ -497,7 +497,7 @@ static void ics_resend(ICSState *ics)
>>         for (i = 0; i < ics->nr_irqs; i++) {
>>           /* FIXME: filter by server#? */
>> -        if (ics->islsi[i]) {
>> +        if (ics->irqs[i].flags & XICS_FLAGS_LSI) {
>>               resend_lsi(ics, i);
>>           } else {
>>               resend_msi(ics, i);
>> @@ -512,7 +512,7 @@ static void ics_eoi(ICSState *ics, int nr)
>>         trace_xics_ics_eoi(nr);
>>   -    if (ics->islsi[srcno]) {
>> +    if (ics->irqs[srcno].flags & XICS_FLAGS_LSI) {
>>           irq->status &= ~XICS_STATUS_SENT;
>>       }
>>   }
>> @@ -609,7 +609,6 @@ static void ics_realize(DeviceState *dev, Error 
>> **errp)
>>           return;
>>       }
>>       ics->irqs = g_malloc0(ics->nr_irqs * sizeof(ICSIRQState));
>> -    ics->islsi = g_malloc0(ics->nr_irqs * sizeof(bool));
>>       ics->qirqs = qemu_allocate_irqs(ics_set_irq, ics, ics->nr_irqs);
>>   }
>>   @@ -646,11 +645,19 @@ qemu_irq xics_get_qirq(XICSState *icp, int irq)
>>       return icp->ics->qirqs[irq - icp->ics->offset];
>>   }
>>   +static void ics_set_irq_type(ICSState *ics, int srcno, bool lsi)
>> +{
>> +    ics->irqs[srcno].flags |=
>> +        lsi ? XICS_FLAGS_LSI : XICS_FLAGS_MSI;
>> +}
>> +
>>   void xics_set_irq_type(XICSState *icp, int irq, bool lsi)
>>   {
>> -    assert(ics_valid_irq(icp->ics, irq));
>> +    ICSState *ics = icp->ics;
>>   -    icp->ics->islsi[irq - icp->ics->offset] = lsi;
>> +    assert(ics_valid_irq(ics, irq));
>> +
>> +    ics_set_irq_type(ics, irq - ics->offset, lsi);
>
> What if this gets called with MSI first, then LSI on the same irq number?

Following your code I think this never happens. But to ensure it doesn't 
add an assert(!(ics->irqs[srcno].flags & XICS_FLAGS_IRQTYPE)) in 
ircs_set_irq_type() maybe?


Alex