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 <clg@kaod.org>
---
 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);
+}
+
+/* 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 ...
+ */
+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);
+        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);
 }
 
 /*
-- 
2.13.6

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 <clg@kaod.org>
> ---
>  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?

> +}
> +
> +/* 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..

> + */
> +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?

> +        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);
>  }
>  
>  /*

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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 <clg@kaod.org>
>> ---
>>  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);
>>  }
>>  
>>  /*
> 

On Thu, 2017-11-30 at 16:00 +1100, David Gibson wrote:
> 
> >  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?

It's the job of the OS to handle that case by consuming from the queue
until it's empty. There is an MMIO the guest can use if it wants to
that can set the IPB bits back to 1 for a given priority. Otherwise in
Linux we just have a SW way to force a replay.

> > +        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);
> >  }
> >  
> >  /*
> 
> 

On Sat, Dec 02, 2017 at 08:40:58AM -0600, Benjamin Herrenschmidt wrote:
> On Thu, 2017-11-30 at 16:00 +1100, David Gibson wrote:
> > 
> > >  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?
> 
> It's the job of the OS to handle that case by consuming from the queue
> until it's empty. There is an MMIO the guest can use if it wants to
> that can set the IPB bits back to 1 for a given priority. Otherwise in
> Linux we just have a SW way to force a replay.

Ok, so "accept" is effectively saying the OS is accepting all
interrupts from that queue, right?

> 
> > > +        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);
> > >  }
> > >  
> > >  /*
> > 
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

On Mon, 2017-12-04 at 12:17 +1100, David Gibson wrote:
> On Sat, Dec 02, 2017 at 08:40:58AM -0600, Benjamin Herrenschmidt wrote:
> > On Thu, 2017-11-30 at 16:00 +1100, David Gibson wrote:
> > > 
> > > >  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?
> > 
> > It's the job of the OS to handle that case by consuming from the queue
> > until it's empty. There is an MMIO the guest can use if it wants to
> > that can set the IPB bits back to 1 for a given priority. Otherwise in
> > Linux we just have a SW way to force a replay.
> 
> Ok, so "accept" is effectively saying the OS is accepting all
> interrupts from that queue, right?

It's whatever you want it to mean. It's simply a test & clear on the
prio bit. From a HW standpoint, you could have multiple queues or just
set an internal SW flag to go chck again later etc...

> 
> > 
> > > > +        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);
> > > >  }
> > > >  
> > > >  /*
> > > 
> > > 
> 
> 

>> +/* 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?

The priority can be above in the TM_SPC_SET_OS_PENDING command. 

>> +}
>> +
>> +/* 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..

The 'accept' sequence does :

  - store the PIPR in the CPPR
  - reset the pending buffer bit in the IPB
  - recompute the PIPR value from the new IPB
  - drop the exception bit for OS

Typical values are : 

  before  IBP=02 PIPR=06 CPPR=ff NSR=80
   after  IBP=00 PIPR=ff CPPR=06 NSR=00

So today, if the IPB becomes zero, the PIPR is adjusted to 0xFF.

But, if the PIPR is clamped to the CPPR (6), there is a disconnect
between the IBP and the PIPR values. Is that OK ? 

Then, the second effect is that as soon as the OS sets back the CPPR 
to 0xFF, we notify the CPU again and enter a loop. 

I think this is because a test on the EO bit of the NSR is missing 
in the routine setting the CPPR. this bit notifies the presence of 
an exception for the O/S. I will add that. I misunderstood the specs 
on the topic.

C.