If a triggered event is let through, the Event Queue data defined in the
associated IVE is pushed in the in-memory event queue. The latter is a
circular buffer provided by the OS using the H_INT_SET_QUEUE_CONFIG hcall,
one per server and priority couple. It is composed of Event Queue entries
which are 4 bytes long, the first bit being a 'generation' bit and the 31
following bits the EQ Data field.

The EQ Data field provides a way to set an invariant logical event source
number for an IRQ. It is set with the H_INT_SET_SOURCE_CONFIG hcall.

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

diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
index 983317a6b3f6..df14c5a88275 100644
--- a/hw/intc/spapr_xive.c
+++ b/hw/intc/spapr_xive.c
@@ -193,9 +193,76 @@ static sPAPRXiveICP *spapr_xive_icp_get(sPAPRXive *xive, int server)
     return cpu ? SPAPR_XIVE_ICP(cpu->intc) : NULL;
 }
 
+static void spapr_xive_eq_push(XiveEQ *eq, uint32_t data)
+{
+    uint64_t qaddr_base = (((uint64_t)(eq->w2 & 0x0fffffff)) << 32) | eq->w3;
+    uint32_t qsize = GETFIELD(EQ_W0_QSIZE, eq->w0);
+    uint32_t qindex = GETFIELD(EQ_W1_PAGE_OFF, eq->w1);
+    uint32_t qgen = GETFIELD(EQ_W1_GENERATION, eq->w1);
+
+    uint64_t qaddr = qaddr_base + (qindex << 2);
+    uint32_t qdata = cpu_to_be32((qgen << 31) | (data & 0x7fffffff));
+    uint32_t qentries = 1 << (qsize + 10);
+
+    if (dma_memory_write(&address_space_memory, qaddr, &qdata, sizeof(qdata))) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to write EQ data @0x%"
+                      HWADDR_PRIx "\n", __func__, qaddr);
+        return;
+    }
+
+    qindex = (qindex + 1) % qentries;
+    if (qindex == 0) {
+        qgen ^= 1;
+        eq->w1 = SETFIELD(EQ_W1_GENERATION, eq->w1, qgen);
+    }
+    eq->w1 = SETFIELD(EQ_W1_PAGE_OFF, eq->w1, qindex);
+}
+
 static void spapr_xive_irq(sPAPRXive *xive, int lisn)
 {
+    XiveIVE *ive;
+    XiveEQ *eq;
+    uint32_t eq_idx;
+    uint8_t priority;
+
+    ive = spapr_xive_get_ive(xive, lisn);
+    if (!ive || !(ive->w & IVE_VALID)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid LISN %d\n", lisn);
+        return;
+    }
 
+    if (ive->w & IVE_MASKED) {
+        return;
+    }
+
+    /* Find our XiveEQ */
+    eq_idx = GETFIELD(IVE_EQ_INDEX, ive->w);
+    eq = spapr_xive_get_eq(xive, eq_idx);
+    if (!eq) {
+        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: No EQ for LISN %d\n", lisn);
+        return;
+    }
+
+    if (eq->w0 & EQ_W0_ENQUEUE) {
+        spapr_xive_eq_push(eq, GETFIELD(IVE_EQ_DATA, ive->w));
+    } else {
+        qemu_log_mask(LOG_UNIMP, "XIVE: !ENQUEUE not implemented\n");
+    }
+
+    if (!(eq->w0 & EQ_W0_UCOND_NOTIFY)) {
+        qemu_log_mask(LOG_UNIMP, "XIVE: !UCOND_NOTIFY not implemented\n");
+    }
+
+    if (GETFIELD(EQ_W6_FORMAT_BIT, eq->w6) == 0) {
+        priority = GETFIELD(EQ_W7_F0_PRIORITY, eq->w7);
+
+        /* The EQ is masked. Can this happen ?  */
+        if (priority == 0xff) {
+            g_assert_not_reached();
+        }
+    } else {
+        qemu_log_mask(LOG_UNIMP, "XIVE: w7 format1 not implemented\n");
+    }
 }
 
 /*
-- 
2.13.6

On Thu, Nov 23, 2017 at 02:29:44PM +0100, Cédric Le Goater wrote:
> If a triggered event is let through, the Event Queue data defined in the
> associated IVE is pushed in the in-memory event queue. The latter is a
> circular buffer provided by the OS using the H_INT_SET_QUEUE_CONFIG hcall,
> one per server and priority couple. It is composed of Event Queue entries
> which are 4 bytes long, the first bit being a 'generation' bit and the 31
> following bits the EQ Data field.
> 
> The EQ Data field provides a way to set an invariant logical event source
> number for an IRQ. It is set with the H_INT_SET_SOURCE_CONFIG hcall.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/intc/spapr_xive.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> index 983317a6b3f6..df14c5a88275 100644
> --- a/hw/intc/spapr_xive.c
> +++ b/hw/intc/spapr_xive.c
> @@ -193,9 +193,76 @@ static sPAPRXiveICP *spapr_xive_icp_get(sPAPRXive *xive, int server)
>      return cpu ? SPAPR_XIVE_ICP(cpu->intc) : NULL;
>  }
>  
> +static void spapr_xive_eq_push(XiveEQ *eq, uint32_t data)
> +{
> +    uint64_t qaddr_base = (((uint64_t)(eq->w2 & 0x0fffffff)) << 32) | eq->w3;
> +    uint32_t qsize = GETFIELD(EQ_W0_QSIZE, eq->w0);
> +    uint32_t qindex = GETFIELD(EQ_W1_PAGE_OFF, eq->w1);
> +    uint32_t qgen = GETFIELD(EQ_W1_GENERATION, eq->w1);
> +
> +    uint64_t qaddr = qaddr_base + (qindex << 2);
> +    uint32_t qdata = cpu_to_be32((qgen << 31) | (data & 0x7fffffff));
> +    uint32_t qentries = 1 << (qsize + 10);
> +
> +    if (dma_memory_write(&address_space_memory, qaddr, &qdata, sizeof(qdata))) {

This suggests that uint32_t data contains guest endian data, which it
generally shouldn't.  Better to use stl_be_dma() (or whatever is
appropriate for the endianness of the data field.

> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to write EQ data @0x%"
> +                      HWADDR_PRIx "\n", __func__, qaddr);
> +        return;
> +    }
> +
> +    qindex = (qindex + 1) % qentries;
> +    if (qindex == 0) {
> +        qgen ^= 1;
> +        eq->w1 = SETFIELD(EQ_W1_GENERATION, eq->w1, qgen);
> +    }
> +    eq->w1 = SETFIELD(EQ_W1_PAGE_OFF, eq->w1, qindex);
> +}
> +
>  static void spapr_xive_irq(sPAPRXive *xive, int lisn)
>  {
> +    XiveIVE *ive;
> +    XiveEQ *eq;
> +    uint32_t eq_idx;
> +    uint8_t priority;
> +
> +    ive = spapr_xive_get_ive(xive, lisn);
> +    if (!ive || !(ive->w & IVE_VALID)) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid LISN %d\n", lisn);

As mentioned on other patches, I'm a little concerned by these
guest-triggerable logs.  I guess the LOG_GUEST_ERROR mask will save
us, though.

> +        return;
> +    }
>  
> +    if (ive->w & IVE_MASKED) {
> +        return;
> +    }
> +
> +    /* Find our XiveEQ */
> +    eq_idx = GETFIELD(IVE_EQ_INDEX, ive->w);
> +    eq = spapr_xive_get_eq(xive, eq_idx);
> +    if (!eq) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: No EQ for LISN %d\n", lisn);
> +        return;
> +    }
> +
> +    if (eq->w0 & EQ_W0_ENQUEUE) {
> +        spapr_xive_eq_push(eq, GETFIELD(IVE_EQ_DATA, ive->w));
> +    } else {
> +        qemu_log_mask(LOG_UNIMP, "XIVE: !ENQUEUE not implemented\n");
> +    }
> +
> +    if (!(eq->w0 & EQ_W0_UCOND_NOTIFY)) {
> +        qemu_log_mask(LOG_UNIMP, "XIVE: !UCOND_NOTIFY not implemented\n");
> +    }
> +
> +    if (GETFIELD(EQ_W6_FORMAT_BIT, eq->w6) == 0) {
> +        priority = GETFIELD(EQ_W7_F0_PRIORITY, eq->w7);
> +
> +        /* The EQ is masked. Can this happen ?  */
> +        if (priority == 0xff) {
> +            g_assert_not_reached();
> +        }
> +    } else {
> +        qemu_log_mask(LOG_UNIMP, "XIVE: w7 format1 not implemented\n");
> +    }
>  }
>  
>  /*

-- 
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 04:49 AM, David Gibson wrote:
> On Thu, Nov 23, 2017 at 02:29:44PM +0100, Cédric Le Goater wrote:
>> If a triggered event is let through, the Event Queue data defined in the
>> associated IVE is pushed in the in-memory event queue. The latter is a
>> circular buffer provided by the OS using the H_INT_SET_QUEUE_CONFIG hcall,
>> one per server and priority couple. It is composed of Event Queue entries
>> which are 4 bytes long, the first bit being a 'generation' bit and the 31
>> following bits the EQ Data field.
>>
>> The EQ Data field provides a way to set an invariant logical event source
>> number for an IRQ. It is set with the H_INT_SET_SOURCE_CONFIG hcall.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/intc/spapr_xive.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 67 insertions(+)
>>
>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>> index 983317a6b3f6..df14c5a88275 100644
>> --- a/hw/intc/spapr_xive.c
>> +++ b/hw/intc/spapr_xive.c
>> @@ -193,9 +193,76 @@ static sPAPRXiveICP *spapr_xive_icp_get(sPAPRXive *xive, int server)
>>      return cpu ? SPAPR_XIVE_ICP(cpu->intc) : NULL;
>>  }
>>  
>> +static void spapr_xive_eq_push(XiveEQ *eq, uint32_t data)
>> +{
>> +    uint64_t qaddr_base = (((uint64_t)(eq->w2 & 0x0fffffff)) << 32) | eq->w3;
>> +    uint32_t qsize = GETFIELD(EQ_W0_QSIZE, eq->w0);
>> +    uint32_t qindex = GETFIELD(EQ_W1_PAGE_OFF, eq->w1);
>> +    uint32_t qgen = GETFIELD(EQ_W1_GENERATION, eq->w1);
>> +
>> +    uint64_t qaddr = qaddr_base + (qindex << 2);
>> +    uint32_t qdata = cpu_to_be32((qgen << 31) | (data & 0x7fffffff));
>> +    uint32_t qentries = 1 << (qsize + 10);
>> +
>> +    if (dma_memory_write(&address_space_memory, qaddr, &qdata, sizeof(qdata))) {
> 
> This suggests that uint32_t data contains guest endian data, which it
> generally shouldn't.  Better to use stl_be_dma() (or whatever is
> appropriate for the endianness of the data field.

There are no requirement on the endianness of the data field and 
it is just stored in the IVE in the hcall H_INT_SET_SOURCE_CONFIG. 
So the guest can pass whatever it likes.  

>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to write EQ data @0x%"
>> +                      HWADDR_PRIx "\n", __func__, qaddr);
>> +        return;
>> +    }
>> +
>> +    qindex = (qindex + 1) % qentries;
>> +    if (qindex == 0) {
>> +        qgen ^= 1;
>> +        eq->w1 = SETFIELD(EQ_W1_GENERATION, eq->w1, qgen);
>> +    }
>> +    eq->w1 = SETFIELD(EQ_W1_PAGE_OFF, eq->w1, qindex);
>> +}
>> +
>>  static void spapr_xive_irq(sPAPRXive *xive, int lisn)
>>  {
>> +    XiveIVE *ive;
>> +    XiveEQ *eq;
>> +    uint32_t eq_idx;
>> +    uint8_t priority;
>> +
>> +    ive = spapr_xive_get_ive(xive, lisn);
>> +    if (!ive || !(ive->w & IVE_VALID)) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid LISN %d\n", lisn);
> 
> As mentioned on other patches, I'm a little concerned by these
> guest-triggerable logs.  I guess the LOG_GUEST_ERROR mask will save
> us, though.

I want to track 'invalid' interrupts but I haven't seen these show up 
in my tests. I agree there are a little too much and some could just 
be asserts.

Thanks,

C.

> 
>> +        return;
>> +    }
>>  
>> +    if (ive->w & IVE_MASKED) {
>> +        return;
>> +    }
>> +
>> +    /* Find our XiveEQ */
>> +    eq_idx = GETFIELD(IVE_EQ_INDEX, ive->w);
>> +    eq = spapr_xive_get_eq(xive, eq_idx);
>> +    if (!eq) {
>> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: No EQ for LISN %d\n", lisn);
>> +        return;
>> +    }
>> +
>> +    if (eq->w0 & EQ_W0_ENQUEUE) {
>> +        spapr_xive_eq_push(eq, GETFIELD(IVE_EQ_DATA, ive->w));
>> +    } else {
>> +        qemu_log_mask(LOG_UNIMP, "XIVE: !ENQUEUE not implemented\n");
>> +    }
>> +
>> +    if (!(eq->w0 & EQ_W0_UCOND_NOTIFY)) {
>> +        qemu_log_mask(LOG_UNIMP, "XIVE: !UCOND_NOTIFY not implemented\n");
>> +    }
>> +
>> +    if (GETFIELD(EQ_W6_FORMAT_BIT, eq->w6) == 0) {
>> +        priority = GETFIELD(EQ_W7_F0_PRIORITY, eq->w7);
>> +
>> +        /* The EQ is masked. Can this happen ?  */
>> +        if (priority == 0xff) {
>> +            g_assert_not_reached();
>> +        }
>> +    } else {
>> +        qemu_log_mask(LOG_UNIMP, "XIVE: w7 format1 not implemented\n");
>> +    }
>>  }
>>  
>>  /*
> 

On Thu, Nov 30, 2017 at 02:16:30PM +0000, Cédric Le Goater wrote:
> On 11/30/2017 04:49 AM, David Gibson wrote:
> > On Thu, Nov 23, 2017 at 02:29:44PM +0100, Cédric Le Goater wrote:
> >> If a triggered event is let through, the Event Queue data defined in the
> >> associated IVE is pushed in the in-memory event queue. The latter is a
> >> circular buffer provided by the OS using the H_INT_SET_QUEUE_CONFIG hcall,
> >> one per server and priority couple. It is composed of Event Queue entries
> >> which are 4 bytes long, the first bit being a 'generation' bit and the 31
> >> following bits the EQ Data field.
> >>
> >> The EQ Data field provides a way to set an invariant logical event source
> >> number for an IRQ. It is set with the H_INT_SET_SOURCE_CONFIG hcall.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  hw/intc/spapr_xive.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 67 insertions(+)
> >>
> >> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
> >> index 983317a6b3f6..df14c5a88275 100644
> >> --- a/hw/intc/spapr_xive.c
> >> +++ b/hw/intc/spapr_xive.c
> >> @@ -193,9 +193,76 @@ static sPAPRXiveICP *spapr_xive_icp_get(sPAPRXive *xive, int server)
> >>      return cpu ? SPAPR_XIVE_ICP(cpu->intc) : NULL;
> >>  }
> >>  
> >> +static void spapr_xive_eq_push(XiveEQ *eq, uint32_t data)
> >> +{
> >> +    uint64_t qaddr_base = (((uint64_t)(eq->w2 & 0x0fffffff)) << 32) | eq->w3;
> >> +    uint32_t qsize = GETFIELD(EQ_W0_QSIZE, eq->w0);
> >> +    uint32_t qindex = GETFIELD(EQ_W1_PAGE_OFF, eq->w1);
> >> +    uint32_t qgen = GETFIELD(EQ_W1_GENERATION, eq->w1);
> >> +
> >> +    uint64_t qaddr = qaddr_base + (qindex << 2);
> >> +    uint32_t qdata = cpu_to_be32((qgen << 31) | (data & 0x7fffffff));
> >> +    uint32_t qentries = 1 << (qsize + 10);
> >> +
> >> +    if (dma_memory_write(&address_space_memory, qaddr, &qdata, sizeof(qdata))) {
> > 
> > This suggests that uint32_t data contains guest endian data, which it
> > generally shouldn't.  Better to use stl_be_dma() (or whatever is
> > appropriate for the endianness of the data field.
> 
> There are no requirement on the endianness of the data field and 
> it is just stored in the IVE in the hcall H_INT_SET_SOURCE_CONFIG. 
> So the guest can pass whatever it likes.  

Hm, ok.  Guest endian (or at least, not definitively host-endian) data
in a plain uint32_t makes me uncomfortable.  Could we use char data[4]
instead, to make it clear it's a byte-ordered buffer, rather than a
number as far as the XIVE is concerned.

Hm.. except that doesn't quite work, because the hardware must define
which end that generation bit ends up in...

> >> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to write EQ data @0x%"
> >> +                      HWADDR_PRIx "\n", __func__, qaddr);
> >> +        return;
> >> +    }
> >> +
> >> +    qindex = (qindex + 1) % qentries;
> >> +    if (qindex == 0) {
> >> +        qgen ^= 1;
> >> +        eq->w1 = SETFIELD(EQ_W1_GENERATION, eq->w1, qgen);
> >> +    }
> >> +    eq->w1 = SETFIELD(EQ_W1_PAGE_OFF, eq->w1, qindex);
> >> +}
> >> +
> >>  static void spapr_xive_irq(sPAPRXive *xive, int lisn)
> >>  {
> >> +    XiveIVE *ive;
> >> +    XiveEQ *eq;
> >> +    uint32_t eq_idx;
> >> +    uint8_t priority;
> >> +
> >> +    ive = spapr_xive_get_ive(xive, lisn);
> >> +    if (!ive || !(ive->w & IVE_VALID)) {
> >> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid LISN %d\n", lisn);
> > 
> > As mentioned on other patches, I'm a little concerned by these
> > guest-triggerable logs.  I guess the LOG_GUEST_ERROR mask will save
> > us, though.
> 
> I want to track 'invalid' interrupts but I haven't seen these show up 
> in my tests. I agree there are a little too much and some could just 
> be asserts.

Uh.. I don't think many can be assert()s.  assert() is only
appropriate if it being tripped definitely indicates a bug in qemu.
Nearly all these qemu_log()s I've seen can be tripped by the guest
doing something bad, which absolutely should not assert() qemu.

-- 
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 12/01/2017 05:10 AM, David Gibson wrote:
> On Thu, Nov 30, 2017 at 02:16:30PM +0000, Cédric Le Goater wrote:
>> On 11/30/2017 04:49 AM, David Gibson wrote:
>>> On Thu, Nov 23, 2017 at 02:29:44PM +0100, Cédric Le Goater wrote:
>>>> If a triggered event is let through, the Event Queue data defined in the
>>>> associated IVE is pushed in the in-memory event queue. The latter is a
>>>> circular buffer provided by the OS using the H_INT_SET_QUEUE_CONFIG hcall,
>>>> one per server and priority couple. It is composed of Event Queue entries
>>>> which are 4 bytes long, the first bit being a 'generation' bit and the 31
>>>> following bits the EQ Data field.
>>>>
>>>> The EQ Data field provides a way to set an invariant logical event source
>>>> number for an IRQ. It is set with the H_INT_SET_SOURCE_CONFIG hcall.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>>  hw/intc/spapr_xive.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 67 insertions(+)
>>>>
>>>> diff --git a/hw/intc/spapr_xive.c b/hw/intc/spapr_xive.c
>>>> index 983317a6b3f6..df14c5a88275 100644
>>>> --- a/hw/intc/spapr_xive.c
>>>> +++ b/hw/intc/spapr_xive.c
>>>> @@ -193,9 +193,76 @@ static sPAPRXiveICP *spapr_xive_icp_get(sPAPRXive *xive, int server)
>>>>      return cpu ? SPAPR_XIVE_ICP(cpu->intc) : NULL;
>>>>  }
>>>>  
>>>> +static void spapr_xive_eq_push(XiveEQ *eq, uint32_t data)
>>>> +{
>>>> +    uint64_t qaddr_base = (((uint64_t)(eq->w2 & 0x0fffffff)) << 32) | eq->w3;
>>>> +    uint32_t qsize = GETFIELD(EQ_W0_QSIZE, eq->w0);
>>>> +    uint32_t qindex = GETFIELD(EQ_W1_PAGE_OFF, eq->w1);
>>>> +    uint32_t qgen = GETFIELD(EQ_W1_GENERATION, eq->w1);
>>>> +
>>>> +    uint64_t qaddr = qaddr_base + (qindex << 2);
>>>> +    uint32_t qdata = cpu_to_be32((qgen << 31) | (data & 0x7fffffff));
>>>> +    uint32_t qentries = 1 << (qsize + 10);
>>>> +
>>>> +    if (dma_memory_write(&address_space_memory, qaddr, &qdata, sizeof(qdata))) {
>>>
>>> This suggests that uint32_t data contains guest endian data, which it
>>> generally shouldn't.  Better to use stl_be_dma() (or whatever is
>>> appropriate for the endianness of the data field.
>>
>> There are no requirement on the endianness of the data field and 
>> it is just stored in the IVE in the hcall H_INT_SET_SOURCE_CONFIG. 
>> So the guest can pass whatever it likes.  
> 
> Hm, ok.  Guest endian (or at least, not definitively host-endian) data
> in a plain uint32_t makes me uncomfortable.  Could we use char data[4]
> instead, to make it clear it's a byte-ordered buffer, rather than a
> number as far as the XIVE is concerned.
> 
> Hm.. except that doesn't quite work, because the hardware must define
> which end that generation bit ends up in...

Sorry, this is is BE. My bad.

C.
 
>>>> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to write EQ data @0x%"
>>>> +                      HWADDR_PRIx "\n", __func__, qaddr);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    qindex = (qindex + 1) % qentries;
>>>> +    if (qindex == 0) {
>>>> +        qgen ^= 1;
>>>> +        eq->w1 = SETFIELD(EQ_W1_GENERATION, eq->w1, qgen);
>>>> +    }
>>>> +    eq->w1 = SETFIELD(EQ_W1_PAGE_OFF, eq->w1, qindex);
>>>> +}
>>>> +
>>>>  static void spapr_xive_irq(sPAPRXive *xive, int lisn)
>>>>  {
>>>> +    XiveIVE *ive;
>>>> +    XiveEQ *eq;
>>>> +    uint32_t eq_idx;
>>>> +    uint8_t priority;
>>>> +
>>>> +    ive = spapr_xive_get_ive(xive, lisn);
>>>> +    if (!ive || !(ive->w & IVE_VALID)) {
>>>> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid LISN %d\n", lisn);
>>>
>>> As mentioned on other patches, I'm a little concerned by these
>>> guest-triggerable logs.  I guess the LOG_GUEST_ERROR mask will save
>>> us, though.
>>
>> I want to track 'invalid' interrupts but I haven't seen these show up 
>> in my tests. I agree there are a little too much and some could just 
>> be asserts.
> 
> Uh.. I don't think many can be assert()s.  assert() is only
> appropriate if it being tripped definitely indicates a bug in qemu.
> Nearly all these qemu_log()s I've seen can be tripped by the guest
> doing something bad, which absolutely should not assert() qemu.
> 

On Fri, 2017-12-01 at 15:10 +1100, David Gibson wrote:
> 
> Hm, ok.  Guest endian (or at least, not definitively host-endian) data
> in a plain uint32_t makes me uncomfortable.  Could we use char data[4]
> instead, to make it clear it's a byte-ordered buffer, rather than a
> number as far as the XIVE is concerned.
> 
> Hm.. except that doesn't quite work, because the hardware must define
> which end that generation bit ends up in...

It also needs to be written atomically. Just say it's big endian.

Cheers,
Ben.

> > >> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to write EQ data @0x%"
> > >> +                      HWADDR_PRIx "\n", __func__, qaddr);
> > >> +        return;
> > >> +    }
> > >> +
> > >> +    qindex = (qindex + 1) % qentries;
> > >> +    if (qindex == 0) {
> > >> +        qgen ^= 1;
> > >> +        eq->w1 = SETFIELD(EQ_W1_GENERATION, eq->w1, qgen);
> > >> +    }
> > >> +    eq->w1 = SETFIELD(EQ_W1_PAGE_OFF, eq->w1, qindex);
> > >> +}
> > >> +
> > >>  static void spapr_xive_irq(sPAPRXive *xive, int lisn)
> > >>  {
> > >> +    XiveIVE *ive;
> > >> +    XiveEQ *eq;
> > >> +    uint32_t eq_idx;
> > >> +    uint8_t priority;
> > >> +
> > >> +    ive = spapr_xive_get_ive(xive, lisn);
> > >> +    if (!ive || !(ive->w & IVE_VALID)) {
> > >> +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid LISN %d\n", lisn);
> > > 
> > > As mentioned on other patches, I'm a little concerned by these
> > > guest-triggerable logs.  I guess the LOG_GUEST_ERROR mask will save
> > > us, though.
> > 
> > I want to track 'invalid' interrupts but I haven't seen these show up 
> > in my tests. I agree there are a little too much and some could just 
> > be asserts.
> 
> Uh.. I don't think many can be assert()s.  assert() is only
> appropriate if it being tripped definitely indicates a bug in qemu.
> Nearly all these qemu_log()s I've seen can be tripped by the guest
> doing something bad, which absolutely should not assert() qemu.

On Sat, 2017-12-02 at 08:45 -0600, Benjamin Herrenschmidt wrote:
> On Fri, 2017-12-01 at 15:10 +1100, David Gibson wrote:
> > 
> > Hm, ok.  Guest endian (or at least, not definitively host-endian) data
> > in a plain uint32_t makes me uncomfortable.  Could we use char data[4]
> > instead, to make it clear it's a byte-ordered buffer, rather than a
> > number as far as the XIVE is concerned.
> > 
> > Hm.. except that doesn't quite work, because the hardware must define
> > which end that generation bit ends up in...
> 
> It also needs to be written atomically. Just say it's big endian.

Also the guest reads it using be32_to_cpup...

> 
> Cheers,
> Ben.
> 
> > > > > +        qemu_log_mask(LOG_GUEST_ERROR, "%s: failed to write EQ data @0x%"
> > > > > +                      HWADDR_PRIx "\n", __func__, qaddr);
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    qindex = (qindex + 1) % qentries;
> > > > > +    if (qindex == 0) {
> > > > > +        qgen ^= 1;
> > > > > +        eq->w1 = SETFIELD(EQ_W1_GENERATION, eq->w1, qgen);
> > > > > +    }
> > > > > +    eq->w1 = SETFIELD(EQ_W1_PAGE_OFF, eq->w1, qindex);
> > > > > +}
> > > > > +
> > > > >  static void spapr_xive_irq(sPAPRXive *xive, int lisn)
> > > > >  {
> > > > > +    XiveIVE *ive;
> > > > > +    XiveEQ *eq;
> > > > > +    uint32_t eq_idx;
> > > > > +    uint8_t priority;
> > > > > +
> > > > > +    ive = spapr_xive_get_ive(xive, lisn);
> > > > > +    if (!ive || !(ive->w & IVE_VALID)) {
> > > > > +        qemu_log_mask(LOG_GUEST_ERROR, "XIVE: invalid LISN %d\n", lisn);
> > > > 
> > > > As mentioned on other patches, I'm a little concerned by these
> > > > guest-triggerable logs.  I guess the LOG_GUEST_ERROR mask will save
> > > > us, though.
> > > 
> > > I want to track 'invalid' interrupts but I haven't seen these show up 
> > > in my tests. I agree there are a little too much and some could just 
> > > be asserts.
> > 
> > Uh.. I don't think many can be assert()s.  assert() is only
> > appropriate if it being tripped definitely indicates a bug in qemu.
> > Nearly all these qemu_log()s I've seen can be tripped by the guest
> > doing something bad, which absolutely should not assert() qemu.

On Sat, Dec 02, 2017 at 08:46:19AM -0600, Benjamin Herrenschmidt wrote:
> On Sat, 2017-12-02 at 08:45 -0600, Benjamin Herrenschmidt wrote:
> > On Fri, 2017-12-01 at 15:10 +1100, David Gibson wrote:
> > > 
> > > Hm, ok.  Guest endian (or at least, not definitively host-endian) data
> > > in a plain uint32_t makes me uncomfortable.  Could we use char data[4]
> > > instead, to make it clear it's a byte-ordered buffer, rather than a
> > > number as far as the XIVE is concerned.
> > > 
> > > Hm.. except that doesn't quite work, because the hardware must define
> > > which end that generation bit ends up in...
> > 
> > It also needs to be written atomically. Just say it's big endian.
> 
> Also the guest reads it using be32_to_cpup...

Ok.  Definitely should be treated as BE and read/written with the be32
DMA helper functions.

-- 
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 12/04/2017 01:20 AM, David Gibson wrote:
> On Sat, Dec 02, 2017 at 08:46:19AM -0600, Benjamin Herrenschmidt wrote:
>> On Sat, 2017-12-02 at 08:45 -0600, Benjamin Herrenschmidt wrote:
>>> On Fri, 2017-12-01 at 15:10 +1100, David Gibson wrote:
>>>>
>>>> Hm, ok.  Guest endian (or at least, not definitively host-endian) data
>>>> in a plain uint32_t makes me uncomfortable.  Could we use char data[4]
>>>> instead, to make it clear it's a byte-ordered buffer, rather than a
>>>> number as far as the XIVE is concerned.
>>>>
>>>> Hm.. except that doesn't quite work, because the hardware must define
>>>> which end that generation bit ends up in...
>>>
>>> It also needs to be written atomically. Just say it's big endian.
>>
>> Also the guest reads it using be32_to_cpup...
> 
> Ok.  Definitely should be treated as BE and read/written with the be32
> DMA helper functions.
> 

hmm, the stl_be_dma does not return errors but dma_memory_write()
does. 

    static inline void st##_sname##_##_end##_dma(AddressSpace *as,      \
                                                 dma_addr_t addr,       \
                                                 uint##_bits##_t val)   \
    {                                                                   \
        val = cpu_to_##_end##_bits(val);                                \
        dma_memory_write(as, addr, &val, (_bits) / 8);                  \
    }

These macros seem to be only used by spapr_vio and nvram. I can probably 
change them.

C.