[RFC,v2,13/21] ppc/xive: handle interrupt acknowledgment by the O/S

When an O/S Exception is raised, 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), which contains the priority of the most
favored pending notification. The bit number corresponding to the
priority of the pending interrupt is reseted in the Interrupt Pending
Buffer (IPB) and so is the EO bit of the NSR.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/intc/spapr_xive.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

On Mon, Sep 11, 2017 at 07:12:27PM +0200, Cédric Le Goater wrote:
> When an O/S Exception is raised, 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), which contains the priority of the most
> favored pending notification. The bit number corresponding to the
> priority of the pending interrupt is reseted in the Interrupt Pending
> Buffer (IPB) and so is the EO bit of the NSR.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/intc/spapr_xive.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index e5d4b723b7e0..ad3ff91b13ea 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -50,7 +50,24 @@ static uint8_t ipb_to_pipr(uint8_t ibp)
>  
>  static uint64_t spapr_xive_icp_accept(ICPState *icp)
>  {
> -    return 0;
> +    uint8_t nsr = icp->tima_os[TM_NSR];
> +
> +    qemu_irq_lower(icp->output);

Ah, here's the lower.  This should not be in a different patch from
the matching raise.  Plus, this doesn't seem right.  Shouldn't this
recheck the CPPR against the PIPR, in case a higher priority irq has
been delivered since the one the cpu is acking.

> +    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);
> +        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(ICPState *icp)

On 09/19/2017 09:53 AM, David Gibson wrote:
> On Mon, Sep 11, 2017 at 07:12:27PM +0200, Cédric Le Goater wrote:
>> When an O/S Exception is raised, 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), which contains the priority of the most
>> favored pending notification. The bit number corresponding to the
>> priority of the pending interrupt is reseted in the Interrupt Pending
>> Buffer (IPB) and so is the EO bit of the NSR.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/intc/spapr_xive.c | 19 ++++++++++++++++++-
>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>> index e5d4b723b7e0..ad3ff91b13ea 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -50,7 +50,24 @@ static uint8_t ipb_to_pipr(uint8_t ibp)
>>  
>>  static uint64_t spapr_xive_icp_accept(ICPState *icp)
>>  {
>> -    return 0;
>> +    uint8_t nsr = icp->tima_os[TM_NSR];
>> +
>> +    qemu_irq_lower(icp->output);
> 
> Ah, here's the lower.  This should not be in a different patch from
> the matching raise.  

ok I can merge these.

> Plus, this doesn't seem right.  Shouldn't this
> recheck the CPPR against the PIPR, in case a higher priority irq has
> been delivered since the one the cpu is acking.

If a higher priority is delivered, it means that the CPPR was more 
privileged and that we have now two bits set in the IPB by the time 
the interrupt is acked. The high priority PIPR will become the new 
CPPR and the IBP will be modified keeping only the lower priority. 

if the CPPR is modified to the lower priority level, then the 
first interrupt will be delivered again. 

I think this is fine.

C.


> 
>> +    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);
>> +        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(ICPState *icp)
>

On Wed, 2017-09-20 at 11:40 +0200, Cédric Le Goater wrote:
> > Plus, this doesn't seem right.  Shouldn't this
> > recheck the CPPR against the PIPR, in case a higher priority irq has
> > been delivered since the one the cpu is acking.
> 
> If a higher priority is delivered, it means that the CPPR was more 
> privileged and that we have now two bits set in the IPB by the time 
> the interrupt is acked. The high priority PIPR will become the new 
> CPPR and the IBP will be modified keeping only the lower priority. 
> 
> if the CPPR is modified to the lower priority level, then the 
> first interrupt will be delivered again. 
> 
> I think this is fine.

Also remember the HW PIPR behaviour, its a bit odd, it will be clamped
by the CPPR. So if CPPR is 0 PIPR will be 0.

If CPPR is 7, PIPR will be <= 7, etc...

Cheers,
Ben.