Re: [Qemu-devel] [PATCH 15/25] spapr: notify the CPU when the XIVE interrupt priority is more privileged

[Cédric Le Goater]

On 11/30/2017 05:00 AM, David Gibson wrote:
> On Thu, Nov 23, 2017 at 02:29:45PM +0100, Cédric Le Goater wrote:
>> The Pending Interrupt Priority Register (PIPR) contains the priority
>> of the most favored pending notification. It is calculated from the
>> Interrupt Pending Buffer (IPB) which indicates a pending interrupt at
>> the priority corresponding to the bit number.
>>
>> If the PIPR is more favored (1) than the Current Processor Priority
>> Register (CPPR), the CPU interrupt line is raised and the EO bit of
>> the Notification Source Register is updated to notify the presence of
>> an exception for the O/S. The check needs to be done whenever the PIPR
>> or the CPPR is changed.
>>
>> Then, the O/S Exception is raised and the O/S acknowledges the
>> interrupt with a special read in the TIMA. If the EO bit of the
>> Notification Source Register (NSR) is set (and it should), the Current
>> Processor Priority Register (CPPR) takes the value of the Pending
>> Interrupt Priority Register (PIPR). The bit number in the Interrupt
>> Pending Buffer (IPB) corresponding to the priority of the pending
>> interrupt is reseted and so is the EO bit of the NSR.
>>
>> (1) numerically less than
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  hw/intc/spapr_xive.c | 77 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 76 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>> index df14c5a88275..fead9c7031f3 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -39,9 +39,63 @@ struct sPAPRXiveICP {
>>      XiveEQ    eqt[XIVE_PRIORITY_MAX + 1];
>>  };
>>  
>> +/* Convert a priority number to an Interrupt Pending Buffer (IPB)
>> + * register, which indicates a pending interrupt at the priority
>> + * corresponding to the bit number
>> + */
>> +static uint8_t priority_to_ipb(uint8_t priority)
>> +{
>> +    return priority > XIVE_PRIORITY_MAX ?
>> +        0 : 1 << (XIVE_PRIORITY_MAX - priority);
> 
> Does handling out of bounds values here make sense, or should you just
> assert() they're not passed in?

Looking at the code, I think we could assert, yes. I need to 
check the SET_OS_PENDING command first.

>> +}
>> +
>> +/* Convert an Interrupt Pending Buffer (IPB) register to a Pending
>> + * Interrupt Priority Register (PIPR), which contains the priority of
>> + * the most favored pending notification.
>> + *
>> + * TODO:
>> + *
>> + *   PIPR is clamped to CPPR. So the value in the PIPR is:
>> + *
>> + *     v = leftmost_bit_of(ipb) (or 0xff);
>> + *     pipr = v < cppr ? v : cppr;
>> + *
>> + * Ben says: "which means it's never actually 0xff ... surprise !".
>> + * But, the CPPR can be set to 0xFF ... I am confused ...
> 
> A resolution to this would be nice..

That's on my TODO list. Not a big issue. 
 
>> + */
>> +static uint8_t ipb_to_pipr(uint8_t ibp)
>> +{
>> +    return ibp ? clz32((uint32_t)ibp << 24) : 0xff;
>> +}
>> +
>>  static uint64_t spapr_xive_icp_accept(sPAPRXiveICP *icp)
>>  {
>> -    return 0;
>> +    uint8_t nsr = icp->tima_os[TM_NSR];
>> +
>> +    qemu_irq_lower(icp->output);
>> +
>> +    if (icp->tima_os[TM_NSR] & TM_QW1_NSR_EO) {
>> +        uint8_t cppr = icp->tima_os[TM_PIPR];
>> +
>> +        icp->tima_os[TM_CPPR] = cppr;
>> +
>> +        /* Reset the pending buffer bit */
>> +        icp->tima_os[TM_IPB] &= ~priority_to_ipb(cppr);
> 
> What if multiple irqs of the same priority were queued?

When an interrupt is EOI'ed, the queue is scanned to check 
for any pending interrupts. If so, a replay is forced with 
a call to force_external_irq_replay()

C. 

> 
>> +        icp->tima_os[TM_PIPR] = ipb_to_pipr(icp->tima_os[TM_IPB]);
>> +
>> +        /* Drop Exception bit for OS */
>> +        icp->tima_os[TM_NSR] &= ~TM_QW1_NSR_EO;
>> +    }
>> +
>> +    return (nsr << 8) | icp->tima_os[TM_CPPR];
>> +}
>> +
>> +static void spapr_xive_icp_notify(sPAPRXiveICP *icp)
>> +{
>> +    if (icp->tima_os[TM_PIPR] < icp->tima_os[TM_CPPR]) {
>> +        icp->tima_os[TM_NSR] |= TM_QW1_NSR_EO;
>> +        qemu_irq_raise(icp->output);
>> +    }
>>  }
>>  
>>  static void spapr_xive_icp_set_cppr(sPAPRXiveICP *icp, uint8_t cppr)
>> @@ -51,6 +105,9 @@ static void spapr_xive_icp_set_cppr(sPAPRXiveICP *icp, 
>> uint8_t cppr)
>>      }
>>  
>>      icp->tima_os[TM_CPPR] = cppr;
>> +
>> +    /* CPPR has changed, inform the ICP which might raise an exception */
>> +    spapr_xive_icp_notify(icp);
>>  }
>>  
>>  /*
>> @@ -224,6 +281,8 @@ static void spapr_xive_irq(sPAPRXive *xive, int lisn)
>>      XiveEQ *eq;
>>      uint32_t eq_idx;
>>      uint8_t priority;
>> +    uint32_t server;
>> +    sPAPRXiveICP *icp;
>>  
>>      ive = spapr_xive_get_ive(xive, lisn);
>>      if (!ive || !(ive->w & IVE_VALID)) {
>> @@ -253,6 +312,13 @@ static void spapr_xive_irq(sPAPRXive *xive, int lisn)
>>          qemu_log_mask(LOG_UNIMP, "XIVE: !UCOND_NOTIFY not implemented\n");
>>      }
>>  
>> +    server = GETFIELD(EQ_W6_NVT_INDEX, eq->w6);
>> +    icp = spapr_xive_icp_get(xive, server);
>> +    if (!icp) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: No ICP for server %d\n", 
>> server);
>> +        return;
>> +    }
>> +
>>      if (GETFIELD(EQ_W6_FORMAT_BIT, eq->w6) == 0) {
>>          priority = GETFIELD(EQ_W7_F0_PRIORITY, eq->w7);
>>  
>> @@ -260,9 +326,18 @@ static void spapr_xive_irq(sPAPRXive *xive, int lisn)
>>          if (priority == 0xff) {
>>              g_assert_not_reached();
>>          }
>> +
>> +        /* Update the IPB (Interrupt Pending Buffer) with the priority
>> +         * of the new notification and inform the ICP, which will
>> +         * decide to raise the exception, or not, depending the CPPR.
>> +         */
>> +        icp->tima_os[TM_IPB] |= priority_to_ipb(priority);
>> +        icp->tima_os[TM_PIPR] = ipb_to_pipr(icp->tima_os[TM_IPB]);
>>      } else {
>>          qemu_log_mask(LOG_UNIMP, "XIVE: w7 format1 not implemented\n");
>>      }
>> +
>> +    spapr_xive_icp_notify(icp);
>>  }
>>  
>>  /*
>