Re: [PATCH 2/3] hw/intc: Add NULL pointer check on LoongArch ipi device

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 12/5/23 05:01, Song Gao wrote:
> Hi,  Philippe
> 
> 在 2023/5/12 上午3:03, Philippe Mathieu-Daudé 写道:
>> On 6/4/23 12:00, Song Gao wrote:
>>> When ipi mailbox is used, cpu index is decoded from iocsr register.
>>> cpu maybe does not exist. This patch adss NULL pointer check on
>>> ipi device.
>>
>> How can that happens from a guest vcpu context?
>>
> cpuid(cs->cpu_index)  is decoded from iocsr register.
> 
>      cpuid = (val >> 16) & 0x3ff;   // ipi_sned [25:16]
> 
> The value maybe invalid.  qemu only support 4 vcpu.

What about something like this?

-- >8 --
-static void ipi_send(uint64_t val)
+static void ipi_send(uint32_t val)
  {
-    int cpuid, data;
+    uint32_t cpuid;
+    uint8_t vector;
      CPULoongArchState *env;
      CPUState *cs;
      LoongArchCPU *cpu;

-    cpuid = (val >> 16) & 0x3ff;
+    cpuid = extract32(val, 16, 10);
+    if (cpuid >= MAX_IPI_CORE_NUM) {
+        trace_loongarch_ipi_unsupported_cpuid("IOCSR_IPI_SEND", cpuid);
+        return;
+    }
      /* IPI status vector */
-    data = 1 << (val & 0x1f);
+    vector = extract8(val, 0, 5);
+
      cs = qemu_get_cpu(cpuid);
      cpu = LOONGARCH_CPU(cs);
      env = &cpu->env;
      address_space_stl(&env->address_space_iocsr, 0x1008,
-                      data, MEMTXATTRS_UNSPECIFIED, NULL);
+                      BIT(vector), MEMTXATTRS_UNSPECIFIED, NULL);

  }
---

> you can find more about ipi_send registers at:
> https://github.com/loongson/LoongArch-Documentation/releases/download/2023.04.20/Loongson-3A5000-usermanual-v1.03-EN.pdf
> Table 63. Processor core inter-processor communication registers
> 
>>> Signed-off-by: Song Gao <gaosong@loongson.cn>
>>> ---
>>>   hw/intc/loongarch_ipi.c | 31 +++++++++++++++++++------------
>>>   1 file changed, 19 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c
>>> index 0563d83a35..39e899df46 100644
>>> --- a/hw/intc/loongarch_ipi.c
>>> +++ b/hw/intc/loongarch_ipi.c
>>> @@ -86,11 +86,12 @@ static void ipi_send(uint64_t val)
>>>       /* IPI status vector */
>>>       data = 1 << (val & 0x1f);
>>>       cs = qemu_get_cpu(cpuid);
>>> -    cpu = LOONGARCH_CPU(cs);
>>> -    env = &cpu->env;
>>> -    address_space_stl(&env->address_space_iocsr, 0x1008,
>>> -                      data, MEMTXATTRS_UNSPECIFIED, NULL);
>>> -
>>> +    if (cs) {
>>> +        cpu = LOONGARCH_CPU(cs);
>>> +        env = &cpu->env;
>>> +        address_space_stl(&env->address_space_iocsr, 0x1008,
>>> +                          data, MEMTXATTRS_UNSPECIFIED, NULL);
>>> +    }
>>
>> Is that the hardware behavior?
>>
> Yes.
>> Could logging the invalid cpuid request be useful?
>>
> Sure.
> 
> Thanks.
> Song Gao
>