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

[Philippe Mathieu-Daudé]

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