By the document of ipi mailsend device, byte is written only when the mask bit
is 0. The original code discards mask bit and overwrite the data always, this
patch fixes the issue.

Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
---
 hw/intc/loongarch_ipi.c | 45 ++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c
index 553e88703d..e4b1fb5366 100644
--- a/hw/intc/loongarch_ipi.c
+++ b/hw/intc/loongarch_ipi.c
@@ -50,35 +50,40 @@ static uint64_t loongarch_ipi_readl(void *opaque, hwaddr addr, unsigned size)
     return ret;
 }
 
-static int get_ipi_data(target_ulong val)
+static void send_ipi_data(CPULoongArchState *env, target_ulong val, target_ulong addr)
 {
     int i, mask, data;
 
-    data = val >> 32;
-    mask = (val >> 27) & 0xf;
-
+    data = address_space_ldl(&env->address_space_iocsr, addr,
+                             MEMTXATTRS_UNSPECIFIED, NULL);
+    mask  = 0;
     for (i = 0; i < 4; i++) {
-        if ((mask >> i) & 1) {
-            data &= ~(0xff << (i * 8));
+        /* bit 27 - 30 is mask for byte write */
+        if (val & (0x1UL << (27 + i))) {
+            mask |= 0xff << (i * 8);
         }
     }
-    return data;
+
+    data &= mask;
+    data |= (val >> 32) & ~mask;
+    address_space_stl(&env->address_space_iocsr, addr,
+                      data, MEMTXATTRS_UNSPECIFIED, NULL);
 }
 
 static void ipi_send(uint64_t val)
 {
     int cpuid, data;
     CPULoongArchState *env;
+    CPUState *cs;
+    LoongArchCPU *cpu;
 
     cpuid = (val >> 16) & 0x3ff;
     /* IPI status vector */
     data = 1 << (val & 0x1f);
-    qemu_mutex_lock_iothread();
-    CPUState *cs = qemu_get_cpu(cpuid);
-    LoongArchCPU *cpu = LOONGARCH_CPU(cs);
+    cs = qemu_get_cpu(cpuid);
+    cpu = LOONGARCH_CPU(cs);
     env = &cpu->env;
     loongarch_cpu_set_irq(cpu, IRQ_IPI, 1);
-    qemu_mutex_unlock_iothread();
     address_space_stl(&env->address_space_iocsr, 0x1008,
                       data, MEMTXATTRS_UNSPECIFIED, NULL);
 
@@ -86,23 +91,23 @@ static void ipi_send(uint64_t val)
 
 static void mail_send(uint64_t val)
 {
-    int cpuid, data;
+    int cpuid;
     hwaddr addr;
     CPULoongArchState *env;
+    CPUState *cs;
+    LoongArchCPU *cpu;
 
     cpuid = (val >> 16) & 0x3ff;
     addr = 0x1020 + (val & 0x1c);
-    CPUState *cs = qemu_get_cpu(cpuid);
-    LoongArchCPU *cpu = LOONGARCH_CPU(cs);
+    cs = qemu_get_cpu(cpuid);
+    cpu = LOONGARCH_CPU(cs);
     env = &cpu->env;
-    data = get_ipi_data(val);
-    address_space_stl(&env->address_space_iocsr, addr,
-                      data, MEMTXATTRS_UNSPECIFIED, NULL);
+    send_ipi_data(env, val, addr);
 }
 
 static void any_send(uint64_t val)
 {
-    int cpuid, data;
+    int cpuid;
     hwaddr addr;
     CPULoongArchState *env;
 
@@ -111,9 +116,7 @@ static void any_send(uint64_t val)
     CPUState *cs = qemu_get_cpu(cpuid);
     LoongArchCPU *cpu = LOONGARCH_CPU(cs);
     env = &cpu->env;
-    data = get_ipi_data(val);
-    address_space_stl(&env->address_space_iocsr, addr,
-                      data, MEMTXATTRS_UNSPECIFIED, NULL);
+    send_ipi_data(env, val, addr);
 }
 
 static void loongarch_ipi_writel(void *opaque, hwaddr addr, uint64_t val,
-- 
2.31.1

On 7/1/22 15:04, Xiaojuan Yang wrote:
> By the document of ipi mailsend device, byte is written only when the mask bit
> is 0. The original code discards mask bit and overwrite the data always, this
> patch fixes the issue.
> 
> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> ---
>   hw/intc/loongarch_ipi.c | 45 ++++++++++++++++++++++-------------------
>   1 file changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c
> index 553e88703d..e4b1fb5366 100644
> --- a/hw/intc/loongarch_ipi.c
> +++ b/hw/intc/loongarch_ipi.c
> @@ -50,35 +50,40 @@ static uint64_t loongarch_ipi_readl(void *opaque, hwaddr addr, unsigned size)
>       return ret;
>   }
>   
> -static int get_ipi_data(target_ulong val)
> +static void send_ipi_data(CPULoongArchState *env, target_ulong val, target_ulong addr)
>   {
>       int i, mask, data;
>   
> -    data = val >> 32;
> -    mask = (val >> 27) & 0xf;
> -
> +    data = address_space_ldl(&env->address_space_iocsr, addr,
> +                             MEMTXATTRS_UNSPECIFIED, NULL);
> +    mask  = 0;
>       for (i = 0; i < 4; i++) {
> -        if ((mask >> i) & 1) {
> -            data &= ~(0xff << (i * 8));
> +        /* bit 27 - 30 is mask for byte write */
> +        if (val & (0x1UL << (27 + i))) {

UL suffix is never correct, since it means different things on different hosts.
Anyway, you don't any suffix here.

How often does mask == 0, so that all of val is written?  In which case you could skip the 
load.

r~

On 2022/7/4 下午1:37, Richard Henderson wrote:
> On 7/1/22 15:04, Xiaojuan Yang wrote:
>> By the document of ipi mailsend device, byte is written only when the 
>> mask bit
>> is 0. The original code discards mask bit and overwrite the data 
>> always, this
>> patch fixes the issue.
>>
>> Signed-off-by: Xiaojuan Yang <yangxiaojuan@loongson.cn>
>> ---
>>   hw/intc/loongarch_ipi.c | 45 ++++++++++++++++++++++-------------------
>>   1 file changed, 24 insertions(+), 21 deletions(-)
>>
>> diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c
>> index 553e88703d..e4b1fb5366 100644
>> --- a/hw/intc/loongarch_ipi.c
>> +++ b/hw/intc/loongarch_ipi.c
>> @@ -50,35 +50,40 @@ static uint64_t loongarch_ipi_readl(void *opaque, 
>> hwaddr addr, unsigned size)
>>       return ret;
>>   }
>>   -static int get_ipi_data(target_ulong val)
>> +static void send_ipi_data(CPULoongArchState *env, target_ulong val, 
>> target_ulong addr)
>>   {
>>       int i, mask, data;
>>   -    data = val >> 32;
>> -    mask = (val >> 27) & 0xf;
>> -
>> +    data = address_space_ldl(&env->address_space_iocsr, addr,
>> +                             MEMTXATTRS_UNSPECIFIED, NULL);
>> +    mask  = 0;
>>       for (i = 0; i < 4; i++) {
>> -        if ((mask >> i) & 1) {
>> -            data &= ~(0xff << (i * 8));
>> +        /* bit 27 - 30 is mask for byte write */
>> +        if (val & (0x1UL << (27 + i))) {
>
> UL suffix is never correct, since it means different things on 
> different hosts.
> Anyway, you don't any suffix here.
>
OK, we will remove the suffix there.

> How often does mask == 0, so that all of val is written?  In which 
> case you could skip the load.
>
At most time the mask is always 0, so  we add a condition to skip the load.
like this:
+    int i, mask = 0, data = 0;
...

+   /*
+    * bit 27-30 is mask for byte writing,
+    * if the mask is 0, we should do nothing.
+    */
+   if ((val >> 27) & 0xf) {
+       data = address_space_ldl(&env->address_space_iocsr, addr,
+                                MEMTXATTRS_UNSPECIFIED, NULL);
...
}

After fix these problem, should we only send these two patches?

Thanks.
Song Gao
> r~

On 7/4/22 14:40, gaosong wrote:
> After fix these problem, should we only send these two patches?

Correct.  I will merge the other loongarch patches today, so you should be able to rebase 
on master.


r~