| Message ID | 20230521102307.87081-2-jiaxun.yang@flygoat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/mips/loongson3_virt: Wire up loongarch_ipi device | expand |
Hi, Jiaxun, Rename loongarch_ipi to loongson_ipi? It will be shared by both MIPS and LoongArch in your series. Huacai On Sun, May 21, 2023 at 6:24 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote: > > As per "Loongson 3A5000/3B5000 Processor Reference Manual", > Loongson 3A5000's IPI implementation have 4 mailboxes per > core. > > However, in 78464f023b54 ("hw/loongarch/virt: Modify ipi as > percpu device"), the number of IPI mailboxes was reduced to > one, which mismatches actual hardware. > > It won't affect LoongArch based system as LoongArch boot code > only uses the first mailbox, however MIPS based Loongson boot > code uses all 4 mailboxes. > > Fixes: 78464f023b54 ("hw/loongarch/virt: Modify ipi as percpu device") > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > --- > hw/intc/loongarch_ipi.c | 6 +++--- > include/hw/intc/loongarch_ipi.h | 4 +++- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c > index d6ab91721ea1..3e453816524e 100644 > --- a/hw/intc/loongarch_ipi.c > +++ b/hw/intc/loongarch_ipi.c > @@ -238,14 +238,14 @@ static void loongarch_ipi_init(Object *obj) > > static const VMStateDescription vmstate_ipi_core = { > .name = "ipi-single", > - .version_id = 1, > - .minimum_version_id = 1, > + .version_id = 2, > + .minimum_version_id = 2, > .fields = (VMStateField[]) { > VMSTATE_UINT32(status, IPICore), > VMSTATE_UINT32(en, IPICore), > VMSTATE_UINT32(set, IPICore), > VMSTATE_UINT32(clear, IPICore), > - VMSTATE_UINT32_ARRAY(buf, IPICore, 2), > + VMSTATE_UINT32_ARRAY(buf, IPICore, IPI_MBX_NUM * 2), > VMSTATE_END_OF_LIST() > } > }; > diff --git a/include/hw/intc/loongarch_ipi.h b/include/hw/intc/loongarch_ipi.h > index 664e050b926e..6c6194786e80 100644 > --- a/include/hw/intc/loongarch_ipi.h > +++ b/include/hw/intc/loongarch_ipi.h > @@ -28,6 +28,8 @@ > #define MAIL_SEND_OFFSET 0 > #define ANY_SEND_OFFSET (IOCSR_ANY_SEND - IOCSR_MAIL_SEND) > > +#define IPI_MBX_NUM 4 > + > #define TYPE_LOONGARCH_IPI "loongarch_ipi" > OBJECT_DECLARE_SIMPLE_TYPE(LoongArchIPI, LOONGARCH_IPI) > > @@ -37,7 +39,7 @@ typedef struct IPICore { > uint32_t set; > uint32_t clear; > /* 64bit buf divide into 2 32bit buf */ > - uint32_t buf[2]; > + uint32_t buf[IPI_MBX_NUM * 2]; > qemu_irq irq; > } IPICore; > > -- > 2.39.2 (Apple Git-143) >
> 2023年5月22日 04:52，Huacai Chen <chenhuacai@kernel.org> 写道： > > Hi, Jiaxun, > > Rename loongarch_ipi to loongson_ipi? It will be shared by both MIPS > and LoongArch in your series. Hi Huacai, Thanks for the point, what’s the opinion from LoongArch mainatiners? Or perhaps rename it as loong_ipi to reflect the nature that it’s shared by MIPS based Loongson and LoongArch based Loongson? Thanks - Jiaxun > > > Huacai > > On Sun, May 21, 2023 at 6:24 PM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote: >> >> As per "Loongson 3A5000/3B5000 Processor Reference Manual", >> Loongson 3A5000's IPI implementation have 4 mailboxes per >> core. >> >> However, in 78464f023b54 ("hw/loongarch/virt: Modify ipi as >> percpu device"), the number of IPI mailboxes was reduced to >> one, which mismatches actual hardware. >> >> It won't affect LoongArch based system as LoongArch boot code >> only uses the first mailbox, however MIPS based Loongson boot >> code uses all 4 mailboxes. >> >> Fixes: 78464f023b54 ("hw/loongarch/virt: Modify ipi as percpu device") >> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> >> --- >> hw/intc/loongarch_ipi.c | 6 +++--- >> include/hw/intc/loongarch_ipi.h | 4 +++- >> 2 files changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c >> index d6ab91721ea1..3e453816524e 100644 >> --- a/hw/intc/loongarch_ipi.c >> +++ b/hw/intc/loongarch_ipi.c >> @@ -238,14 +238,14 @@ static void loongarch_ipi_init(Object *obj) >> >> static const VMStateDescription vmstate_ipi_core = { >> .name = "ipi-single", >> - .version_id = 1, >> - .minimum_version_id = 1, >> + .version_id = 2, >> + .minimum_version_id = 2, >> .fields = (VMStateField[]) { >> VMSTATE_UINT32(status, IPICore), >> VMSTATE_UINT32(en, IPICore), >> VMSTATE_UINT32(set, IPICore), >> VMSTATE_UINT32(clear, IPICore), >> - VMSTATE_UINT32_ARRAY(buf, IPICore, 2), >> + VMSTATE_UINT32_ARRAY(buf, IPICore, IPI_MBX_NUM * 2), >> VMSTATE_END_OF_LIST() >> } >> }; >> diff --git a/include/hw/intc/loongarch_ipi.h b/include/hw/intc/loongarch_ipi.h >> index 664e050b926e..6c6194786e80 100644 >> --- a/include/hw/intc/loongarch_ipi.h >> +++ b/include/hw/intc/loongarch_ipi.h >> @@ -28,6 +28,8 @@ >> #define MAIL_SEND_OFFSET 0 >> #define ANY_SEND_OFFSET (IOCSR_ANY_SEND - IOCSR_MAIL_SEND) >> >> +#define IPI_MBX_NUM 4 >> + >> #define TYPE_LOONGARCH_IPI "loongarch_ipi" >> OBJECT_DECLARE_SIMPLE_TYPE(LoongArchIPI, LOONGARCH_IPI) >> >> @@ -37,7 +39,7 @@ typedef struct IPICore { >> uint32_t set; >> uint32_t clear; >> /* 64bit buf divide into 2 32bit buf */ >> - uint32_t buf[2]; >> + uint32_t buf[IPI_MBX_NUM * 2]; >> qemu_irq irq; >> } IPICore; >> >> -- >> 2.39.2 (Apple Git-143) >>
On 22/5/23 13:47, Jiaxun Yang wrote: > > >> 2023年5月22日 04:52，Huacai Chen <chenhuacai@kernel.org> 写道： >> >> Hi, Jiaxun, >> >> Rename loongarch_ipi to loongson_ipi? It will be shared by both MIPS >> and LoongArch in your series. > > Hi Huacai, > > Thanks for the point, what’s the opinion from LoongArch mainatiners? > > Or perhaps rename it as loong_ipi to reflect the nature that it’s shared > by MIPS based Loongson and LoongArch based Loongson? I'm not a LoongArch maintainer, but a model named "loong_ipi" makes sense to me. Please add it to the two Virt machine sections in MAINTAINERS. > Thanks > - Jiaxun > >> >> >> Huacai
在 2023/5/22 下午9:44, Philippe Mathieu-Daudé 写道: > On 22/5/23 13:47, Jiaxun Yang wrote: >> >> >>> 2023年5月22日 04:52，Huacai Chen <chenhuacai@kernel.org> 写道： >>> >>> Hi, Jiaxun, >>> >>> Rename loongarch_ipi to loongson_ipi? It will be shared by both MIPS >>> and LoongArch in your series. >> >> Hi Huacai, >> >> Thanks for the point, what’s the opinion from LoongArch mainatiners? >> >> Or perhaps rename it as loong_ipi to reflect the nature that it’s shared >> by MIPS based Loongson and LoongArch based Loongson? > > I'm not a LoongArch maintainer, but a model named "loong_ipi" makes > sense to me. > > Please add it to the two Virt machine sections in MAINTAINERS. > 'loonggson_ipi' is better, qemu doesn't have naming with 'loong' as prefix. And patch2 should not use macros. Some attributes should be added to distinguish between MIPS and LongArch. All references to loongarch_ipi should also be changed. Thanks. Song Gao
> 2023年5月23日 02:25，Song Gao <gaosong@loongson.cn> 写道： > > > > 在 2023/5/22 下午9:44, Philippe Mathieu-Daudé 写道: >> On 22/5/23 13:47, Jiaxun Yang wrote: >>> >>> >>>> 2023年5月22日 04:52，Huacai Chen <chenhuacai@kernel.org> 写道： >>>> >>>> Hi, Jiaxun, >>>> >>>> Rename loongarch_ipi to loongson_ipi? It will be shared by both MIPS >>>> and LoongArch in your series. >>> >>> Hi Huacai, >>> >>> Thanks for the point, what’s the opinion from LoongArch mainatiners? >>> >>> Or perhaps rename it as loong_ipi to reflect the nature that it’s shared >>> by MIPS based Loongson and LoongArch based Loongson? >> >> I'm not a LoongArch maintainer, but a model named "loong_ipi" makes >> sense to me. >> >> Please add it to the two Virt machine sections in MAINTAINERS. Hi Song, >> > 'loonggson_ipi' is better, qemu doesn't have naming with 'loong' as prefix. Thanks, I’ll take looongson_ipi then. > > And patch2 should not use macros. Some attributes should be added to distinguish between MIPS and LongArch. By attribute do you mean property? If so I don’t see any necessity, the IP block Is totally the same on MIPS and LoongArch. I’m guarding them out because We have different way to get IOCSR address space on MIPS, which is due to be implemented. I can further abstract out a function to get IOCSR address space. But still, I think the best way to differ those two architecture is using TARGET_* macros, as it doesn’t make much sense to have unused code for another architecture compiled. > > All references to loongarch_ipi should also be changed. Sure. Thanks - Jiaxun > > Thanks. > Song Gao
在 2023/5/23 上午11:22, Jiaxun Yang 写道: > >> 2023年5月23日 02:25，Song Gao <gaosong@loongson.cn> 写道： >> >> >> >> 在 2023/5/22 下午9:44, Philippe Mathieu-Daudé 写道: >>> On 22/5/23 13:47, Jiaxun Yang wrote: >>>> >>>>> 2023年5月22日 04:52，Huacai Chen <chenhuacai@kernel.org> 写道： >>>>> >>>>> Hi, Jiaxun, >>>>> >>>>> Rename loongarch_ipi to loongson_ipi? It will be shared by both MIPS >>>>> and LoongArch in your series. >>>> Hi Huacai, >>>> >>>> Thanks for the point, what’s the opinion from LoongArch mainatiners? >>>> >>>> Or perhaps rename it as loong_ipi to reflect the nature that it’s shared >>>> by MIPS based Loongson and LoongArch based Loongson? >>> I'm not a LoongArch maintainer, but a model named "loong_ipi" makes >>> sense to me. >>> >>> Please add it to the two Virt machine sections in MAINTAINERS. > Hi Song, > >> 'loonggson_ipi' is better, qemu doesn't have naming with 'loong' as prefix. > Thanks, I’ll take looongson_ipi then. > >> And patch2 should not use macros. Some attributes should be added to distinguish between MIPS and LongArch. > By attribute do you mean property? Yes. > If so I don’t see any necessity, the IP block > Is totally the same on MIPS and LoongArch. I’m guarding them out because > We have different way to get IOCSR address space on MIPS, which is due > to be implemented. > > I can further abstract out a function to get IOCSR address space. But still, > I think the best way to differ those two architecture is using TARGET_* macros, > as it doesn’t make much sense to have unused code for another architecture > compiled. Most of the code in hw/intc or hw/ uses property to distinguish between different devices, not TARGE_* macro. I still think it is better to use property. Thanks. Song Gao >> All references to loongarch_ipi should also be changed. > Sure. > > Thanks > - Jiaxun
> 2023年5月23日 11:01，Song Gao <gaosong@loongson.cn> 写道： > > > > 在 2023/5/23 上午11:22, Jiaxun Yang 写道: [...] >> >>> >> Is totally the same on MIPS and LoongArch. I’m guarding them out because >> We have different way to get IOCSR address space on MIPS, which is due >> to be implemented. >> >> I can further abstract out a function to get IOCSR address space. But still, >> I think the best way to differ those two architecture is using TARGET_* macros, >> as it doesn’t make much sense to have unused code for another architecture >> compiled. > Most of the code in hw/intc or hw/ uses property to distinguish between different devices, not TARGE_* macro. They are the *same* device, with different way to handle IOCSR address space. Another problem is casting CPUState with LOONGARCH_CPU() is something invalid on MIPS, vice-versa. We are potentially introducing a security issue here. I know nobody have done something like this before, but not necessarily to be a bad idea. I’ll introduce something like: +#ifdef TARGET_LOONGARCH64 +static inline void *AddressSpace get_iocsr_as(int cpuid) +{ + CPUState *cs = qemu_get_cpu(cpuid); + LoongArchCPU *cpu = LOONGARCH_CPU(cs); + + return &cpu->env.address_space_iocsr; +} +#endif + +#ifdef TARGET_MIPS64 +static inline void *AddressSpace get_iocsr_as(int cpuid) +{ + CPUState *cs = qemu_get_cpu(cpuid); + MIPSCPU *cpu = MIPS_CPU(cs); + + return &cpu->env.iocsr.as; +} +#endif Thanks - Jiaxun > > I still think it is better to use property. > > Thanks. > Song Gao >>> All references to loongarch_ipi should also be changed. >> Sure. >> >> Thanks >> - Jiaxun >
On 23/5/23 13:18, Jiaxun Yang wrote: > > >> 2023年5月23日 11:01，Song Gao <gaosong@loongson.cn> 写道： >> >> >> >> 在 2023/5/23 上午11:22, Jiaxun Yang 写道: > [...] >>> >>>> >>> Is totally the same on MIPS and LoongArch. I’m guarding them out because >>> We have different way to get IOCSR address space on MIPS, which is due >>> to be implemented. >>> >>> I can further abstract out a function to get IOCSR address space. But still, >>> I think the best way to differ those two architecture is using TARGET_* macros, >>> as it doesn’t make much sense to have unused code for another architecture >>> compiled. >> Most of the code in hw/intc or hw/ uses property to distinguish between different devices, not TARGE_* macro. > > They are the *same* device, with different way to handle IOCSR address space. > > Another problem is casting CPUState with LOONGARCH_CPU() is something invalid on > MIPS, vice-versa. We are potentially introducing a security issue here. > > I know nobody have done something like this before, but not necessarily to be a bad idea. > > I’ll introduce something like: > > +#ifdef TARGET_LOONGARCH64 > +static inline void *AddressSpace get_iocsr_as(int cpuid) > +{ > + CPUState *cs = qemu_get_cpu(cpuid); > + LoongArchCPU *cpu = LOONGARCH_CPU(cs); > + > + return &cpu->env.address_space_iocsr; > +} > +#endif > + > +#ifdef TARGET_MIPS64 > +static inline void *AddressSpace get_iocsr_as(int cpuid) > +{ > + CPUState *cs = qemu_get_cpu(cpuid); > + MIPSCPU *cpu = MIPS_CPU(cs); > + > + return &cpu->env.iocsr.as; > +} > +#endif Introduce a QOM interface that provides a get_iocsr_as() implementation. See for example how TYPE_IDAU_INTERFACE works.
On Sun, 21 May 2023 at 11:24, Jiaxun Yang <jiaxun.yang@flygoat.com> wrote: > > As per "Loongson 3A5000/3B5000 Processor Reference Manual", > Loongson 3A5000's IPI implementation have 4 mailboxes per > core. > > However, in 78464f023b54 ("hw/loongarch/virt: Modify ipi as > percpu device"), the number of IPI mailboxes was reduced to > one, which mismatches actual hardware. > > It won't affect LoongArch based system as LoongArch boot code > only uses the first mailbox, however MIPS based Loongson boot > code uses all 4 mailboxes. > > Fixes: 78464f023b54 ("hw/loongarch/virt: Modify ipi as percpu device") > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > --- > hw/intc/loongarch_ipi.c | 6 +++--- > include/hw/intc/loongarch_ipi.h | 4 +++- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c > index d6ab91721ea1..3e453816524e 100644 > --- a/hw/intc/loongarch_ipi.c > +++ b/hw/intc/loongarch_ipi.c > @@ -238,14 +238,14 @@ static void loongarch_ipi_init(Object *obj) > > static const VMStateDescription vmstate_ipi_core = { > .name = "ipi-single", > - .version_id = 1, > - .minimum_version_id = 1, > + .version_id = 2, > + .minimum_version_id = 2, > .fields = (VMStateField[]) { > VMSTATE_UINT32(status, IPICore), > VMSTATE_UINT32(en, IPICore), > VMSTATE_UINT32(set, IPICore), > VMSTATE_UINT32(clear, IPICore), > - VMSTATE_UINT32_ARRAY(buf, IPICore, 2), > + VMSTATE_UINT32_ARRAY(buf, IPICore, IPI_MBX_NUM * 2), > VMSTATE_END_OF_LIST() > } > }; > diff --git a/include/hw/intc/loongarch_ipi.h b/include/hw/intc/loongarch_ipi.h > index 664e050b926e..6c6194786e80 100644 > --- a/include/hw/intc/loongarch_ipi.h > +++ b/include/hw/intc/loongarch_ipi.h > @@ -28,6 +28,8 @@ > #define MAIL_SEND_OFFSET 0 > #define ANY_SEND_OFFSET (IOCSR_ANY_SEND - IOCSR_MAIL_SEND) > > +#define IPI_MBX_NUM 4 > + > #define TYPE_LOONGARCH_IPI "loongarch_ipi" > OBJECT_DECLARE_SIMPLE_TYPE(LoongArchIPI, LOONGARCH_IPI) > > @@ -37,7 +39,7 @@ typedef struct IPICore { > uint32_t set; > uint32_t clear; > /* 64bit buf divide into 2 32bit buf */ > - uint32_t buf[2]; > + uint32_t buf[IPI_MBX_NUM * 2]; > qemu_irq irq; > } IPICore; In particular, this fixes Coverity issues CID 1512452 and 1512453, where Coverity notices that the code in loongarch_ipi_writel() and loongarch_ipi_readl() reads off the end of the too-short buf[]. thanks -- PMM
> 2023年6月3日 01:28，Peter Maydell <peter.maydell@linaro.org> 写道： > > On Sun, 21 May 2023 at 11:24, Jiaxun Yang <jiaxun.yang@flygoat.com> wrote: >> >> As per "Loongson 3A5000/3B5000 Processor Reference Manual", >> Loongson 3A5000's IPI implementation have 4 mailboxes per >> core. >> >> However, in 78464f023b54 ("hw/loongarch/virt: Modify ipi as >> percpu device"), the number of IPI mailboxes was reduced to >> one, which mismatches actual hardware. >> >> It won't affect LoongArch based system as LoongArch boot code >> only uses the first mailbox, however MIPS based Loongson boot >> code uses all 4 mailboxes. >> >> Fixes: 78464f023b54 ("hw/loongarch/virt: Modify ipi as percpu device") >> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> >> --- >> hw/intc/loongarch_ipi.c | 6 +++--- >> include/hw/intc/loongarch_ipi.h | 4 +++- >> 2 files changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c >> index d6ab91721ea1..3e453816524e 100644 >> --- a/hw/intc/loongarch_ipi.c >> +++ b/hw/intc/loongarch_ipi.c >> @@ -238,14 +238,14 @@ static void loongarch_ipi_init(Object *obj) >> >> static const VMStateDescription vmstate_ipi_core = { >> .name = "ipi-single", >> - .version_id = 1, >> - .minimum_version_id = 1, >> + .version_id = 2, >> + .minimum_version_id = 2, >> .fields = (VMStateField[]) { >> VMSTATE_UINT32(status, IPICore), >> VMSTATE_UINT32(en, IPICore), >> VMSTATE_UINT32(set, IPICore), >> VMSTATE_UINT32(clear, IPICore), >> - VMSTATE_UINT32_ARRAY(buf, IPICore, 2), >> + VMSTATE_UINT32_ARRAY(buf, IPICore, IPI_MBX_NUM * 2), >> VMSTATE_END_OF_LIST() >> } >> }; >> diff --git a/include/hw/intc/loongarch_ipi.h b/include/hw/intc/loongarch_ipi.h >> index 664e050b926e..6c6194786e80 100644 >> --- a/include/hw/intc/loongarch_ipi.h >> +++ b/include/hw/intc/loongarch_ipi.h >> @@ -28,6 +28,8 @@ >> #define MAIL_SEND_OFFSET 0 >> #define ANY_SEND_OFFSET (IOCSR_ANY_SEND - IOCSR_MAIL_SEND) >> >> +#define IPI_MBX_NUM 4 >> + >> #define TYPE_LOONGARCH_IPI "loongarch_ipi" >> OBJECT_DECLARE_SIMPLE_TYPE(LoongArchIPI, LOONGARCH_IPI) >> >> @@ -37,7 +39,7 @@ typedef struct IPICore { >> uint32_t set; >> uint32_t clear; >> /* 64bit buf divide into 2 32bit buf */ >> - uint32_t buf[2]; >> + uint32_t buf[IPI_MBX_NUM * 2]; >> qemu_irq irq; >> } IPICore; > > In particular, this fixes Coverity issues CID 1512452 and 1512453, > where Coverity notices that the code in loongarch_ipi_writel() and > loongarch_ipi_readl() reads off the end of the too-short buf[]. LoongArch maintainers, do you mind to take this patch while I’m refactoring rest of the series? Thanks Jiaxun > > thanks > -- PMM
在 2023/6/3 下午3:06, Jiaxun Yang 写道: > >> 2023年6月3日 01:28，Peter Maydell <peter.maydell@linaro.org> 写道： >> >> On Sun, 21 May 2023 at 11:24, Jiaxun Yang <jiaxun.yang@flygoat.com> wrote: >>> As per "Loongson 3A5000/3B5000 Processor Reference Manual", >>> Loongson 3A5000's IPI implementation have 4 mailboxes per >>> core. >>> >>> However, in 78464f023b54 ("hw/loongarch/virt: Modify ipi as >>> percpu device"), the number of IPI mailboxes was reduced to >>> one, which mismatches actual hardware. >>> >>> It won't affect LoongArch based system as LoongArch boot code >>> only uses the first mailbox, however MIPS based Loongson boot >>> code uses all 4 mailboxes. >>> >>> Fixes: 78464f023b54 ("hw/loongarch/virt: Modify ipi as percpu device") >>> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> >>> --- >>> hw/intc/loongarch_ipi.c | 6 +++--- >>> include/hw/intc/loongarch_ipi.h | 4 +++- >>> 2 files changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c >>> index d6ab91721ea1..3e453816524e 100644 >>> --- a/hw/intc/loongarch_ipi.c >>> +++ b/hw/intc/loongarch_ipi.c >>> @@ -238,14 +238,14 @@ static void loongarch_ipi_init(Object *obj) >>> >>> static const VMStateDescription vmstate_ipi_core = { >>> .name = "ipi-single", >>> - .version_id = 1, >>> - .minimum_version_id = 1, >>> + .version_id = 2, >>> + .minimum_version_id = 2, >>> .fields = (VMStateField[]) { >>> VMSTATE_UINT32(status, IPICore), >>> VMSTATE_UINT32(en, IPICore), >>> VMSTATE_UINT32(set, IPICore), >>> VMSTATE_UINT32(clear, IPICore), >>> - VMSTATE_UINT32_ARRAY(buf, IPICore, 2), >>> + VMSTATE_UINT32_ARRAY(buf, IPICore, IPI_MBX_NUM * 2), >>> VMSTATE_END_OF_LIST() >>> } >>> }; >>> diff --git a/include/hw/intc/loongarch_ipi.h b/include/hw/intc/loongarch_ipi.h >>> index 664e050b926e..6c6194786e80 100644 >>> --- a/include/hw/intc/loongarch_ipi.h >>> +++ b/include/hw/intc/loongarch_ipi.h >>> @@ -28,6 +28,8 @@ >>> #define MAIL_SEND_OFFSET 0 >>> #define ANY_SEND_OFFSET (IOCSR_ANY_SEND - IOCSR_MAIL_SEND) >>> >>> +#define IPI_MBX_NUM 4 >>> + >>> #define TYPE_LOONGARCH_IPI "loongarch_ipi" >>> OBJECT_DECLARE_SIMPLE_TYPE(LoongArchIPI, LOONGARCH_IPI) >>> >>> @@ -37,7 +39,7 @@ typedef struct IPICore { >>> uint32_t set; >>> uint32_t clear; >>> /* 64bit buf divide into 2 32bit buf */ >>> - uint32_t buf[2]; >>> + uint32_t buf[IPI_MBX_NUM * 2]; >>> qemu_irq irq; >>> } IPICore; >> In particular, this fixes Coverity issues CID 1512452 and 1512453, >> where Coverity notices that the code in loongarch_ipi_writel() and >> loongarch_ipi_readl() reads off the end of the too-short buf[]. > LoongArch maintainers, do you mind to take this patch while I’m refactoring > rest of the series? I don't mind. Thanks. Song Gao
在 2023/5/23 21:07, Philippe Mathieu-Daudé 写道: > On 23/5/23 13:18, Jiaxun Yang wrote: >> >> >>> 2023年5月23日 11:01，Song Gao <gaosong@loongson.cn> 写道： >>> >>> >>> >>> 在 2023/5/23 上午11:22, Jiaxun Yang 写道: >> [...] >>>> >>>>> >>>> Is totally the same on MIPS and LoongArch. I’m guarding them out because >>>> We have different way to get IOCSR address space on MIPS, which is due >>>> to be implemented. >>>> >>>> I can further abstract out a function to get IOCSR address space. But still, >>>> I think the best way to differ those two architecture is using TARGET_* macros, >>>> as it doesn’t make much sense to have unused code for another architecture >>>> compiled. >>> Most of the code in hw/intc or hw/ uses property to distinguish between different devices, not TARGE_* macro. >> >> They are the *same* device, with different way to handle IOCSR address space. >> >> Another problem is casting CPUState with LOONGARCH_CPU() is something invalid on >> MIPS, vice-versa. We are potentially introducing a security issue here. >> >> I know nobody have done something like this before, but not necessarily to be a bad idea. >> >> I’ll introduce something like: >> >> +#ifdef TARGET_LOONGARCH64 >> +static inline void *AddressSpace get_iocsr_as(int cpuid) >> +{ >> + CPUState *cs = qemu_get_cpu(cpuid); >> + LoongArchCPU *cpu = LOONGARCH_CPU(cs); >> + >> + return &cpu->env.address_space_iocsr; >> +} >> +#endif >> + >> +#ifdef TARGET_MIPS64 >> +static inline void *AddressSpace get_iocsr_as(int cpuid) >> +{ >> + CPUState *cs = qemu_get_cpu(cpuid); >> + MIPSCPU *cpu = MIPS_CPU(cs); >> + >> + return &cpu->env.iocsr.as; >> +} >> +#endif > > Introduce a QOM interface that provides a get_iocsr_as() implementation. > > See for example how TYPE_IDAU_INTERFACE works. another simple method, rename loongarch_ipi.c with loong_ipi_common.c, adding extra two files loongarch_ipi.c and loongson_ipi.c inheriting from loong_ipi_common.c Regards Bibo, Mao
在 2023/5/21 下午6:23, Jiaxun Yang 写道: > As per "Loongson 3A5000/3B5000 Processor Reference Manual", > Loongson 3A5000's IPI implementation have 4 mailboxes per > core. > > However, in 78464f023b54 ("hw/loongarch/virt: Modify ipi as > percpu device"), the number of IPI mailboxes was reduced to > one, which mismatches actual hardware. > > It won't affect LoongArch based system as LoongArch boot code > only uses the first mailbox, however MIPS based Loongson boot > code uses all 4 mailboxes. > > Fixes: 78464f023b54 ("hw/loongarch/virt: Modify ipi as percpu device") > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > --- > hw/intc/loongarch_ipi.c | 6 +++--- > include/hw/intc/loongarch_ipi.h | 4 +++- > 2 files changed, 6 insertions(+), 4 deletions(-) Reviewed-by: Song Gao <gaosong@loongson.cn> Thanks. Song Gao > diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c > index d6ab91721ea1..3e453816524e 100644 > --- a/hw/intc/loongarch_ipi.c > +++ b/hw/intc/loongarch_ipi.c > @@ -238,14 +238,14 @@ static void loongarch_ipi_init(Object *obj) > > static const VMStateDescription vmstate_ipi_core = { > .name = "ipi-single", > - .version_id = 1, > - .minimum_version_id = 1, > + .version_id = 2, > + .minimum_version_id = 2, > .fields = (VMStateField[]) { > VMSTATE_UINT32(status, IPICore), > VMSTATE_UINT32(en, IPICore), > VMSTATE_UINT32(set, IPICore), > VMSTATE_UINT32(clear, IPICore), > - VMSTATE_UINT32_ARRAY(buf, IPICore, 2), > + VMSTATE_UINT32_ARRAY(buf, IPICore, IPI_MBX_NUM * 2), > VMSTATE_END_OF_LIST() > } > }; > diff --git a/include/hw/intc/loongarch_ipi.h b/include/hw/intc/loongarch_ipi.h > index 664e050b926e..6c6194786e80 100644 > --- a/include/hw/intc/loongarch_ipi.h > +++ b/include/hw/intc/loongarch_ipi.h > @@ -28,6 +28,8 @@ > #define MAIL_SEND_OFFSET 0 > #define ANY_SEND_OFFSET (IOCSR_ANY_SEND - IOCSR_MAIL_SEND) > > +#define IPI_MBX_NUM 4 > + > #define TYPE_LOONGARCH_IPI "loongarch_ipi" > OBJECT_DECLARE_SIMPLE_TYPE(LoongArchIPI, LOONGARCH_IPI) > > @@ -37,7 +39,7 @@ typedef struct IPICore { > uint32_t set; > uint32_t clear; > /* 64bit buf divide into 2 32bit buf */ > - uint32_t buf[2]; > + uint32_t buf[IPI_MBX_NUM * 2]; > qemu_irq irq; > } IPICore; >
diff --git a/hw/intc/loongarch_ipi.c b/hw/intc/loongarch_ipi.c index d6ab91721ea1..3e453816524e 100644 --- a/hw/intc/loongarch_ipi.c +++ b/hw/intc/loongarch_ipi.c @@ -238,14 +238,14 @@ static void loongarch_ipi_init(Object *obj) static const VMStateDescription vmstate_ipi_core = { .name = "ipi-single", - .version_id = 1, - .minimum_version_id = 1, + .version_id = 2, + .minimum_version_id = 2, .fields = (VMStateField[]) { VMSTATE_UINT32(status, IPICore), VMSTATE_UINT32(en, IPICore), VMSTATE_UINT32(set, IPICore), VMSTATE_UINT32(clear, IPICore), - VMSTATE_UINT32_ARRAY(buf, IPICore, 2), + VMSTATE_UINT32_ARRAY(buf, IPICore, IPI_MBX_NUM * 2), VMSTATE_END_OF_LIST() } }; diff --git a/include/hw/intc/loongarch_ipi.h b/include/hw/intc/loongarch_ipi.h index 664e050b926e..6c6194786e80 100644 --- a/include/hw/intc/loongarch_ipi.h +++ b/include/hw/intc/loongarch_ipi.h @@ -28,6 +28,8 @@ #define MAIL_SEND_OFFSET 0 #define ANY_SEND_OFFSET (IOCSR_ANY_SEND - IOCSR_MAIL_SEND) +#define IPI_MBX_NUM 4 + #define TYPE_LOONGARCH_IPI "loongarch_ipi" OBJECT_DECLARE_SIMPLE_TYPE(LoongArchIPI, LOONGARCH_IPI) @@ -37,7 +39,7 @@ typedef struct IPICore { uint32_t set; uint32_t clear; /* 64bit buf divide into 2 32bit buf */ - uint32_t buf[2]; + uint32_t buf[IPI_MBX_NUM * 2]; qemu_irq irq; } IPICore;
As per "Loongson 3A5000/3B5000 Processor Reference Manual", Loongson 3A5000's IPI implementation have 4 mailboxes per core. However, in 78464f023b54 ("hw/loongarch/virt: Modify ipi as percpu device"), the number of IPI mailboxes was reduced to one, which mismatches actual hardware. It won't affect LoongArch based system as LoongArch boot code only uses the first mailbox, however MIPS based Loongson boot code uses all 4 mailboxes. Fixes: 78464f023b54 ("hw/loongarch/virt: Modify ipi as percpu device") Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> --- hw/intc/loongarch_ipi.c | 6 +++--- include/hw/intc/loongarch_ipi.h | 4 +++- 2 files changed, 6 insertions(+), 4 deletions(-)