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

[Song Gao]

在 2023/5/12 上午11:45, 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?
Nice,   thanks for you suggestion.

Thanks
Song Gao.
> -- >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