Re: [PULL 23/35] hw/intc: Rework Loongson LIOINTC
在 2021/1/11 下午6:35, Peter Maydell 写道: On Mon, 11 Jan 2021 at 10:20, BALATON Zoltan wrote: On Mon, 11 Jan 2021, Jiaxun Yang wrote: On Mon, Jan 11, 2021, at 8:36 AM, Huacai Chen wrote: I think R_END should be 0x60, Jiaxun, what do you think? U r right. The manual is misleading. The R_END constant is also used in loongson_liointc_init() for the length of the memory region so you might want to revise that. If this is a 32 bit register then you should decide what R_END means? Is it the end of the memory region in which case the reg starts at R_END - 4 or is it the address of the last reg in which case the memory region ends at R_END + 4. From the above I think it's the address of the last reg so you'll probably need to add 4 in loongson_liointc_init() when creating the memory region. Mmm, or check (addr >= R_START && addr < (R_START + R_ISR_SIZE * NUM_CORES)) Side note: R_ISR_SIZE is 8, but the code makes both the 32-bit addresses you can read/write in that 8-byte range behave the same way. Is that really what the hardware does ? Or does it actually have 1 32-bit register per core, spaced 8 bytes apart ? Yes, the hardware was designed like that. It have 1 32-bit register per core but spaced 8 bytes apart. I assume they were planing to add more ISRs in the future but as the product line have been ceased I'm not going to handle that. I'll send a patch for fix. Thanks. - Jiaxun thanks -- PMM
Re: [PULL 23/35] hw/intc: Rework Loongson LIOINTC
On Mon, 11 Jan 2021, Peter Maydell wrote: On Mon, 11 Jan 2021 at 10:20, BALATON Zoltan wrote: On Mon, 11 Jan 2021, Jiaxun Yang wrote: On Mon, Jan 11, 2021, at 8:36 AM, Huacai Chen wrote: I think R_END should be 0x60, Jiaxun, what do you think? U r right. The manual is misleading. The R_END constant is also used in loongson_liointc_init() for the length of the memory region so you might want to revise that. If this is a 32 bit register then you should decide what R_END means? Is it the end of the memory region in which case the reg starts at R_END - 4 or is it the address of the last reg in which case the memory region ends at R_END + 4. From the above I think it's the address of the last reg so you'll probably need to add 4 in loongson_liointc_init() when creating the memory region. Mmm, or check (addr >= R_START && addr < (R_START + R_ISR_SIZE * NUM_CORES)) That was basically the original version just hiding the calculation in a macro so we could also just drop this part of the patch and be happy with it :-) "If it ain't broke, don't fix it" Side note: R_ISR_SIZE is 8, but the code makes both the 32-bit addresses you can read/write in that 8-byte range behave the same way. Is that really what the hardware does ? Or does it actually have 1 32-bit register per core, spaced 8 bytes apart ? This might turn into a bike shed thread but I just thought: would range_covers_byte() or ranges_overlap() be useful here to test if addr is within the regs area? I've used that in vt82c686.c::pm_write_config(). That might actually be simpler if that worked. Regards, BALATON Zoltan
Re: [PULL 23/35] hw/intc: Rework Loongson LIOINTC
On Mon, 11 Jan 2021 at 10:20, BALATON Zoltan wrote: > > On Mon, 11 Jan 2021, Jiaxun Yang wrote: > > On Mon, Jan 11, 2021, at 8:36 AM, Huacai Chen wrote: > >> I think R_END should be 0x60, Jiaxun, what do you think? > > > > U r right. > > The manual is misleading. > > The R_END constant is also used in loongson_liointc_init() for the length > of the memory region so you might want to revise that. If this is a 32 bit > register then you should decide what R_END means? Is it the end of the > memory region in which case the reg starts at R_END - 4 or is it the > address of the last reg in which case the memory region ends at R_END + 4. > From the above I think it's the address of the last reg so you'll probably > need to add 4 in loongson_liointc_init() when creating the memory region. Mmm, or check (addr >= R_START && addr < (R_START + R_ISR_SIZE * NUM_CORES)) Side note: R_ISR_SIZE is 8, but the code makes both the 32-bit addresses you can read/write in that 8-byte range behave the same way. Is that really what the hardware does ? Or does it actually have 1 32-bit register per core, spaced 8 bytes apart ? thanks -- PMM
Re: [PULL 23/35] hw/intc: Rework Loongson LIOINTC
On Mon, 11 Jan 2021, Jiaxun Yang wrote: On Mon, Jan 11, 2021, at 8:36 AM, Huacai Chen wrote: I think R_END should be 0x60, Jiaxun, what do you think? U r right. The manual is misleading. The R_END constant is also used in loongson_liointc_init() for the length of the memory region so you might want to revise that. If this is a 32 bit register then you should decide what R_END means? Is it the end of the memory region in which case the reg starts at R_END - 4 or is it the address of the last reg in which case the memory region ends at R_END + 4. From the above I think it's the address of the last reg so you'll probably need to add 4 in loongson_liointc_init() when creating the memory region. Regards, BALATON Zoltan Thanks. - Jiaxun Huacai On Mon, Jan 11, 2021 at 5:51 AM BALATON Zoltan wrote: On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: Hi Peter, Huacai, On 1/10/21 8:49 PM, Peter Maydell wrote: On Sun, 3 Jan 2021 at 21:11, Philippe Mathieu-Daudé wrote: From: Huacai Chen As suggested by Philippe Mathieu-Daudé, rework Loongson's liointc: 1, Move macro definitions to loongson_liointc.h; 2, Remove magic values and use macros instead; 3, Replace dead D() code by trace events. Suggested-by: Philippe Mathieu-Daudé Signed-off-by: Huacai Chen Tested-by: Philippe Mathieu-Daudé Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20201221110538.3186646-2-chenhua...@kernel.org> Signed-off-by: Philippe Mathieu-Daudé --- include/hw/intc/loongson_liointc.h | 22 ++ hw/intc/loongson_liointc.c | 36 +- 2 files changed, 38 insertions(+), 20 deletions(-) create mode 100644 include/hw/intc/loongson_liointc.h Hi; Coverity complains about a possible array overrun in this commit: @@ -40,13 +39,10 @@ #define R_IEN 0x24 #define R_IEN_SET 0x28 #define R_IEN_CLR 0x2c -#define R_PERCORE_ISR(x)(0x40 + 0x8 * x) +#define R_ISR_SIZE 0x8 +#define R_START 0x40 #define R_END 0x64 -#define TYPE_LOONGSON_LIOINTC "loongson.liointc" -DECLARE_INSTANCE_CHECKER(struct loongson_liointc, LOONGSON_LIOINTC, - TYPE_LOONGSON_LIOINTC) - struct loongson_liointc { SysBusDevice parent_obj; @@ -123,14 +119,13 @@ liointc_read(void *opaque, hwaddr addr, unsigned int size) goto out; } -/* Rest is 4 byte */ +/* Rest are 4 bytes */ if (size != 4 || (addr % 4)) { goto out; } Expanding macros in the following: -if (addr >= R_PERCORE_ISR(0) && -addr < R_PERCORE_ISR(NUM_CORES)) { -int core = (addr - R_PERCORE_ISR(0)) / 8; if (addr >= (0x40 + 0x8 * 0) && addr < (0x40 + 0x8 * 4)) -> if (addr >= 0x40 && addr < 0x60) int core = (addr - 0x40) / 8; +if (addr >= R_START && addr < R_END) { +int core = (addr - R_START) / R_ISR_SIZE; if (addr >= 0x40 && addr < 0x64) int core = (addr - 0x40) / 0x8; R_END seems to be off by 4 in the above. Should it be 0x60? Regards, BALATON Zoltan R_END is 0x64 and R_START is 0x40, so if addr is 0x60 then addr - R_START is 0x32 and so core here is 4. However p->per_core_isr[] only has 4 entries, so this will be off the end of the array. This is CID 1438965. r = p->per_core_isr[core]; goto out; } -if (addr >= R_PERCORE_ISR(0) && -addr < R_PERCORE_ISR(NUM_CORES)) { -int core = (addr - R_PERCORE_ISR(0)) / 8; +if (addr >= R_START && addr < R_END) { +int core = (addr - R_START) / R_ISR_SIZE; p->per_core_isr[core] = value; goto out; } Same thing here, CID 1438967. Thanks Peter. Huacai, can you have a look please? Thanks, Phil.
Re: [PULL 23/35] hw/intc: Rework Loongson LIOINTC
On Mon, Jan 11, 2021, at 8:36 AM, Huacai Chen wrote: > I think R_END should be 0x60, Jiaxun, what do you think? U r right. The manual is misleading. Thanks. - Jiaxun > > Huacai > > On Mon, Jan 11, 2021 at 5:51 AM BALATON Zoltan wrote: > > > > On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: > > > Hi Peter, Huacai, > > > > > > On 1/10/21 8:49 PM, Peter Maydell wrote: > > >> On Sun, 3 Jan 2021 at 21:11, Philippe Mathieu-Daudé > > >> wrote: > > >>> > > >>> From: Huacai Chen > > >>> > > >>> As suggested by Philippe Mathieu-Daudé, rework Loongson's liointc: > > >>> 1, Move macro definitions to loongson_liointc.h; > > >>> 2, Remove magic values and use macros instead; > > >>> 3, Replace dead D() code by trace events. > > >>> > > >>> Suggested-by: Philippe Mathieu-Daudé > > >>> Signed-off-by: Huacai Chen > > >>> Tested-by: Philippe Mathieu-Daudé > > >>> Reviewed-by: Philippe Mathieu-Daudé > > >>> Message-Id: <20201221110538.3186646-2-chenhua...@kernel.org> > > >>> Signed-off-by: Philippe Mathieu-Daudé > > >>> --- > > >>> include/hw/intc/loongson_liointc.h | 22 ++ > > >>> hw/intc/loongson_liointc.c | 36 +- > > >>> 2 files changed, 38 insertions(+), 20 deletions(-) > > >>> create mode 100644 include/hw/intc/loongson_liointc.h > > >> > > >> Hi; Coverity complains about a possible array overrun > > >> in this commit: > > >> > > >> > > >>> @@ -40,13 +39,10 @@ > > >>> #define R_IEN 0x24 > > >>> #define R_IEN_SET 0x28 > > >>> #define R_IEN_CLR 0x2c > > >>> -#define R_PERCORE_ISR(x)(0x40 + 0x8 * x) > > >>> +#define R_ISR_SIZE 0x8 > > >>> +#define R_START 0x40 > > >>> #define R_END 0x64 > > >>> > > >>> -#define TYPE_LOONGSON_LIOINTC "loongson.liointc" > > >>> -DECLARE_INSTANCE_CHECKER(struct loongson_liointc, LOONGSON_LIOINTC, > > >>> - TYPE_LOONGSON_LIOINTC) > > >>> - > > >>> struct loongson_liointc { > > >>> SysBusDevice parent_obj; > > >>> > > >>> @@ -123,14 +119,13 @@ liointc_read(void *opaque, hwaddr addr, unsigned > > >>> int size) > > >>> goto out; > > >>> } > > >>> > > >>> -/* Rest is 4 byte */ > > >>> +/* Rest are 4 bytes */ > > >>> if (size != 4 || (addr % 4)) { > > >>> goto out; > > >>> } > > >>> > > > > Expanding macros in the following: > > > > >>> -if (addr >= R_PERCORE_ISR(0) && > > >>> -addr < R_PERCORE_ISR(NUM_CORES)) { > > >>> -int core = (addr - R_PERCORE_ISR(0)) / 8; > > > > if (addr >= (0x40 + 0x8 * 0) && addr < (0x40 + 0x8 * 4)) > > -> > > if (addr >= 0x40 && addr < 0x60) > > int core = (addr - 0x40) / 8; > > > > > > >>> +if (addr >= R_START && addr < R_END) { > > >>> +int core = (addr - R_START) / R_ISR_SIZE; > > > > if (addr >= 0x40 && addr < 0x64) > > int core = (addr - 0x40) / 0x8; > > > > R_END seems to be off by 4 in the above. Should it be 0x60? > > > > Regards, > > BALATON Zoltan > > > > >> R_END is 0x64 and R_START is 0x40, so if addr is 0x60 > > >> then addr - R_START is 0x32 and so core here is 4. > > >> However p->per_core_isr[] only has 4 entries, so this will > > >> be off the end of the array. > > >> > > >> This is CID 1438965. > > >> > > >>> r = p->per_core_isr[core]; > > >>> goto out; > > >>> } > > >> > > >>> -if (addr >= R_PERCORE_ISR(0) && > > >>> -addr < R_PERCORE_ISR(NUM_CORES)) { > > >>> -int core = (addr - R_PERCORE_ISR(0)) / 8; > > >>> +if (addr >= R_START && addr < R_END) { > > >>> +int core = (addr - R_START) / R_ISR_SIZE; > > >>> p->per_core_isr[core] = value; > > >>> goto out; > > >>> } > > >> > > >> Same thing here, CID 1438967. > > > > > > Thanks Peter. > > > > > > Huacai, can you have a look please? > > > > > > Thanks, > > > > > > Phil. > > > > > > > -- - Jiaxun
Re: [PULL 23/35] hw/intc: Rework Loongson LIOINTC
I think R_END should be 0x60, Jiaxun, what do you think? Huacai On Mon, Jan 11, 2021 at 5:51 AM BALATON Zoltan wrote: > > On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: > > Hi Peter, Huacai, > > > > On 1/10/21 8:49 PM, Peter Maydell wrote: > >> On Sun, 3 Jan 2021 at 21:11, Philippe Mathieu-Daudé > >> wrote: > >>> > >>> From: Huacai Chen > >>> > >>> As suggested by Philippe Mathieu-Daudé, rework Loongson's liointc: > >>> 1, Move macro definitions to loongson_liointc.h; > >>> 2, Remove magic values and use macros instead; > >>> 3, Replace dead D() code by trace events. > >>> > >>> Suggested-by: Philippe Mathieu-Daudé > >>> Signed-off-by: Huacai Chen > >>> Tested-by: Philippe Mathieu-Daudé > >>> Reviewed-by: Philippe Mathieu-Daudé > >>> Message-Id: <20201221110538.3186646-2-chenhua...@kernel.org> > >>> Signed-off-by: Philippe Mathieu-Daudé > >>> --- > >>> include/hw/intc/loongson_liointc.h | 22 ++ > >>> hw/intc/loongson_liointc.c | 36 +- > >>> 2 files changed, 38 insertions(+), 20 deletions(-) > >>> create mode 100644 include/hw/intc/loongson_liointc.h > >> > >> Hi; Coverity complains about a possible array overrun > >> in this commit: > >> > >> > >>> @@ -40,13 +39,10 @@ > >>> #define R_IEN 0x24 > >>> #define R_IEN_SET 0x28 > >>> #define R_IEN_CLR 0x2c > >>> -#define R_PERCORE_ISR(x)(0x40 + 0x8 * x) > >>> +#define R_ISR_SIZE 0x8 > >>> +#define R_START 0x40 > >>> #define R_END 0x64 > >>> > >>> -#define TYPE_LOONGSON_LIOINTC "loongson.liointc" > >>> -DECLARE_INSTANCE_CHECKER(struct loongson_liointc, LOONGSON_LIOINTC, > >>> - TYPE_LOONGSON_LIOINTC) > >>> - > >>> struct loongson_liointc { > >>> SysBusDevice parent_obj; > >>> > >>> @@ -123,14 +119,13 @@ liointc_read(void *opaque, hwaddr addr, unsigned > >>> int size) > >>> goto out; > >>> } > >>> > >>> -/* Rest is 4 byte */ > >>> +/* Rest are 4 bytes */ > >>> if (size != 4 || (addr % 4)) { > >>> goto out; > >>> } > >>> > > Expanding macros in the following: > > >>> -if (addr >= R_PERCORE_ISR(0) && > >>> -addr < R_PERCORE_ISR(NUM_CORES)) { > >>> -int core = (addr - R_PERCORE_ISR(0)) / 8; > > if (addr >= (0x40 + 0x8 * 0) && addr < (0x40 + 0x8 * 4)) > -> > if (addr >= 0x40 && addr < 0x60) > int core = (addr - 0x40) / 8; > > > >>> +if (addr >= R_START && addr < R_END) { > >>> +int core = (addr - R_START) / R_ISR_SIZE; > > if (addr >= 0x40 && addr < 0x64) > int core = (addr - 0x40) / 0x8; > > R_END seems to be off by 4 in the above. Should it be 0x60? > > Regards, > BALATON Zoltan > > >> R_END is 0x64 and R_START is 0x40, so if addr is 0x60 > >> then addr - R_START is 0x32 and so core here is 4. > >> However p->per_core_isr[] only has 4 entries, so this will > >> be off the end of the array. > >> > >> This is CID 1438965. > >> > >>> r = p->per_core_isr[core]; > >>> goto out; > >>> } > >> > >>> -if (addr >= R_PERCORE_ISR(0) && > >>> -addr < R_PERCORE_ISR(NUM_CORES)) { > >>> -int core = (addr - R_PERCORE_ISR(0)) / 8; > >>> +if (addr >= R_START && addr < R_END) { > >>> +int core = (addr - R_START) / R_ISR_SIZE; > >>> p->per_core_isr[core] = value; > >>> goto out; > >>> } > >> > >> Same thing here, CID 1438967. > > > > Thanks Peter. > > > > Huacai, can you have a look please? > > > > Thanks, > > > > Phil. > > > >
Re: [PULL 23/35] hw/intc: Rework Loongson LIOINTC
On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote: Hi Peter, Huacai, On 1/10/21 8:49 PM, Peter Maydell wrote: On Sun, 3 Jan 2021 at 21:11, Philippe Mathieu-Daudé wrote: From: Huacai Chen As suggested by Philippe Mathieu-Daudé, rework Loongson's liointc: 1, Move macro definitions to loongson_liointc.h; 2, Remove magic values and use macros instead; 3, Replace dead D() code by trace events. Suggested-by: Philippe Mathieu-Daudé Signed-off-by: Huacai Chen Tested-by: Philippe Mathieu-Daudé Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20201221110538.3186646-2-chenhua...@kernel.org> Signed-off-by: Philippe Mathieu-Daudé --- include/hw/intc/loongson_liointc.h | 22 ++ hw/intc/loongson_liointc.c | 36 +- 2 files changed, 38 insertions(+), 20 deletions(-) create mode 100644 include/hw/intc/loongson_liointc.h Hi; Coverity complains about a possible array overrun in this commit: @@ -40,13 +39,10 @@ #define R_IEN 0x24 #define R_IEN_SET 0x28 #define R_IEN_CLR 0x2c -#define R_PERCORE_ISR(x)(0x40 + 0x8 * x) +#define R_ISR_SIZE 0x8 +#define R_START 0x40 #define R_END 0x64 -#define TYPE_LOONGSON_LIOINTC "loongson.liointc" -DECLARE_INSTANCE_CHECKER(struct loongson_liointc, LOONGSON_LIOINTC, - TYPE_LOONGSON_LIOINTC) - struct loongson_liointc { SysBusDevice parent_obj; @@ -123,14 +119,13 @@ liointc_read(void *opaque, hwaddr addr, unsigned int size) goto out; } -/* Rest is 4 byte */ +/* Rest are 4 bytes */ if (size != 4 || (addr % 4)) { goto out; } Expanding macros in the following: -if (addr >= R_PERCORE_ISR(0) && -addr < R_PERCORE_ISR(NUM_CORES)) { -int core = (addr - R_PERCORE_ISR(0)) / 8; if (addr >= (0x40 + 0x8 * 0) && addr < (0x40 + 0x8 * 4)) -> if (addr >= 0x40 && addr < 0x60) int core = (addr - 0x40) / 8; +if (addr >= R_START && addr < R_END) { +int core = (addr - R_START) / R_ISR_SIZE; if (addr >= 0x40 && addr < 0x64) int core = (addr - 0x40) / 0x8; R_END seems to be off by 4 in the above. Should it be 0x60? Regards, BALATON Zoltan R_END is 0x64 and R_START is 0x40, so if addr is 0x60 then addr - R_START is 0x32 and so core here is 4. However p->per_core_isr[] only has 4 entries, so this will be off the end of the array. This is CID 1438965. r = p->per_core_isr[core]; goto out; } -if (addr >= R_PERCORE_ISR(0) && -addr < R_PERCORE_ISR(NUM_CORES)) { -int core = (addr - R_PERCORE_ISR(0)) / 8; +if (addr >= R_START && addr < R_END) { +int core = (addr - R_START) / R_ISR_SIZE; p->per_core_isr[core] = value; goto out; } Same thing here, CID 1438967. Thanks Peter. Huacai, can you have a look please? Thanks, Phil.
Re: [PULL 23/35] hw/intc: Rework Loongson LIOINTC
Hi Peter, Huacai, On 1/10/21 8:49 PM, Peter Maydell wrote: > On Sun, 3 Jan 2021 at 21:11, Philippe Mathieu-Daudé wrote: >> >> From: Huacai Chen >> >> As suggested by Philippe Mathieu-Daudé, rework Loongson's liointc: >> 1, Move macro definitions to loongson_liointc.h; >> 2, Remove magic values and use macros instead; >> 3, Replace dead D() code by trace events. >> >> Suggested-by: Philippe Mathieu-Daudé >> Signed-off-by: Huacai Chen >> Tested-by: Philippe Mathieu-Daudé >> Reviewed-by: Philippe Mathieu-Daudé >> Message-Id: <20201221110538.3186646-2-chenhua...@kernel.org> >> Signed-off-by: Philippe Mathieu-Daudé >> --- >> include/hw/intc/loongson_liointc.h | 22 ++ >> hw/intc/loongson_liointc.c | 36 +- >> 2 files changed, 38 insertions(+), 20 deletions(-) >> create mode 100644 include/hw/intc/loongson_liointc.h > > Hi; Coverity complains about a possible array overrun > in this commit: > > >> @@ -40,13 +39,10 @@ >> #define R_IEN 0x24 >> #define R_IEN_SET 0x28 >> #define R_IEN_CLR 0x2c >> -#define R_PERCORE_ISR(x)(0x40 + 0x8 * x) >> +#define R_ISR_SIZE 0x8 >> +#define R_START 0x40 >> #define R_END 0x64 >> >> -#define TYPE_LOONGSON_LIOINTC "loongson.liointc" >> -DECLARE_INSTANCE_CHECKER(struct loongson_liointc, LOONGSON_LIOINTC, >> - TYPE_LOONGSON_LIOINTC) >> - >> struct loongson_liointc { >> SysBusDevice parent_obj; >> >> @@ -123,14 +119,13 @@ liointc_read(void *opaque, hwaddr addr, unsigned int >> size) >> goto out; >> } >> >> -/* Rest is 4 byte */ >> +/* Rest are 4 bytes */ >> if (size != 4 || (addr % 4)) { >> goto out; >> } >> >> -if (addr >= R_PERCORE_ISR(0) && >> -addr < R_PERCORE_ISR(NUM_CORES)) { >> -int core = (addr - R_PERCORE_ISR(0)) / 8; >> +if (addr >= R_START && addr < R_END) { >> +int core = (addr - R_START) / R_ISR_SIZE; > > R_END is 0x64 and R_START is 0x40, so if addr is 0x60 > then addr - R_START is 0x32 and so core here is 4. > However p->per_core_isr[] only has 4 entries, so this will > be off the end of the array. > > This is CID 1438965. > >> r = p->per_core_isr[core]; >> goto out; >> } > >> -if (addr >= R_PERCORE_ISR(0) && >> -addr < R_PERCORE_ISR(NUM_CORES)) { >> -int core = (addr - R_PERCORE_ISR(0)) / 8; >> +if (addr >= R_START && addr < R_END) { >> +int core = (addr - R_START) / R_ISR_SIZE; >> p->per_core_isr[core] = value; >> goto out; >> } > > Same thing here, CID 1438967. Thanks Peter. Huacai, can you have a look please? Thanks, Phil.
Re: [PULL 23/35] hw/intc: Rework Loongson LIOINTC
On Sun, 3 Jan 2021 at 21:11, Philippe Mathieu-Daudé wrote: > > From: Huacai Chen > > As suggested by Philippe Mathieu-Daudé, rework Loongson's liointc: > 1, Move macro definitions to loongson_liointc.h; > 2, Remove magic values and use macros instead; > 3, Replace dead D() code by trace events. > > Suggested-by: Philippe Mathieu-Daudé > Signed-off-by: Huacai Chen > Tested-by: Philippe Mathieu-Daudé > Reviewed-by: Philippe Mathieu-Daudé > Message-Id: <20201221110538.3186646-2-chenhua...@kernel.org> > Signed-off-by: Philippe Mathieu-Daudé > --- > include/hw/intc/loongson_liointc.h | 22 ++ > hw/intc/loongson_liointc.c | 36 +- > 2 files changed, 38 insertions(+), 20 deletions(-) > create mode 100644 include/hw/intc/loongson_liointc.h Hi; Coverity complains about a possible array overrun in this commit: > @@ -40,13 +39,10 @@ > #define R_IEN 0x24 > #define R_IEN_SET 0x28 > #define R_IEN_CLR 0x2c > -#define R_PERCORE_ISR(x)(0x40 + 0x8 * x) > +#define R_ISR_SIZE 0x8 > +#define R_START 0x40 > #define R_END 0x64 > > -#define TYPE_LOONGSON_LIOINTC "loongson.liointc" > -DECLARE_INSTANCE_CHECKER(struct loongson_liointc, LOONGSON_LIOINTC, > - TYPE_LOONGSON_LIOINTC) > - > struct loongson_liointc { > SysBusDevice parent_obj; > > @@ -123,14 +119,13 @@ liointc_read(void *opaque, hwaddr addr, unsigned int > size) > goto out; > } > > -/* Rest is 4 byte */ > +/* Rest are 4 bytes */ > if (size != 4 || (addr % 4)) { > goto out; > } > > -if (addr >= R_PERCORE_ISR(0) && > -addr < R_PERCORE_ISR(NUM_CORES)) { > -int core = (addr - R_PERCORE_ISR(0)) / 8; > +if (addr >= R_START && addr < R_END) { > +int core = (addr - R_START) / R_ISR_SIZE; R_END is 0x64 and R_START is 0x40, so if addr is 0x60 then addr - R_START is 0x32 and so core here is 4. However p->per_core_isr[] only has 4 entries, so this will be off the end of the array. This is CID 1438965. > r = p->per_core_isr[core]; > goto out; > } > -if (addr >= R_PERCORE_ISR(0) && > -addr < R_PERCORE_ISR(NUM_CORES)) { > -int core = (addr - R_PERCORE_ISR(0)) / 8; > +if (addr >= R_START && addr < R_END) { > +int core = (addr - R_START) / R_ISR_SIZE; > p->per_core_isr[core] = value; > goto out; > } Same thing here, CID 1438967. thanks -- PMM
[PULL 23/35] hw/intc: Rework Loongson LIOINTC
From: Huacai Chen As suggested by Philippe Mathieu-Daudé, rework Loongson's liointc: 1, Move macro definitions to loongson_liointc.h; 2, Remove magic values and use macros instead; 3, Replace dead D() code by trace events. Suggested-by: Philippe Mathieu-Daudé Signed-off-by: Huacai Chen Tested-by: Philippe Mathieu-Daudé Reviewed-by: Philippe Mathieu-Daudé Message-Id: <20201221110538.3186646-2-chenhua...@kernel.org> Signed-off-by: Philippe Mathieu-Daudé --- include/hw/intc/loongson_liointc.h | 22 ++ hw/intc/loongson_liointc.c | 36 +- 2 files changed, 38 insertions(+), 20 deletions(-) create mode 100644 include/hw/intc/loongson_liointc.h diff --git a/include/hw/intc/loongson_liointc.h b/include/hw/intc/loongson_liointc.h new file mode 100644 index 000..848e65eb359 --- /dev/null +++ b/include/hw/intc/loongson_liointc.h @@ -0,0 +1,22 @@ +/* + * This file is subject to the terms and conditions of the GNU General Public + * License. See the file "COPYING" in the main directory of this archive + * for more details. + * + * Copyright (c) 2020 Huacai Chen + * Copyright (c) 2020 Jiaxun Yang + * + */ + +#ifndef LOONGSON_LIOINTC_H +#define LOONGSON_LIOINTC_H + +#include "qemu/units.h" +#include "hw/sysbus.h" +#include "qom/object.h" + +#define TYPE_LOONGSON_LIOINTC "loongson.liointc" +DECLARE_INSTANCE_CHECKER(struct loongson_liointc, LOONGSON_LIOINTC, + TYPE_LOONGSON_LIOINTC) + +#endif /* LOONGSON_LIOINTC_H */ diff --git a/hw/intc/loongson_liointc.c b/hw/intc/loongson_liointc.c index fbbfb57ee9c..f823d484e08 100644 --- a/hw/intc/loongson_liointc.c +++ b/hw/intc/loongson_liointc.c @@ -1,6 +1,7 @@ /* * QEMU Loongson Local I/O interrupt controler. * + * Copyright (c) 2020 Huacai Chen * Copyright (c) 2020 Jiaxun Yang * * This program is free software: you can redistribute it and/or modify @@ -19,13 +20,11 @@ */ #include "qemu/osdep.h" -#include "hw/sysbus.h" #include "qemu/module.h" +#include "qemu/log.h" #include "hw/irq.h" #include "hw/qdev-properties.h" -#include "qom/object.h" - -#define D(x) +#include "hw/intc/loongson_liointc.h" #define NUM_IRQS32 @@ -40,13 +39,10 @@ #define R_IEN 0x24 #define R_IEN_SET 0x28 #define R_IEN_CLR 0x2c -#define R_PERCORE_ISR(x)(0x40 + 0x8 * x) +#define R_ISR_SIZE 0x8 +#define R_START 0x40 #define R_END 0x64 -#define TYPE_LOONGSON_LIOINTC "loongson.liointc" -DECLARE_INSTANCE_CHECKER(struct loongson_liointc, LOONGSON_LIOINTC, - TYPE_LOONGSON_LIOINTC) - struct loongson_liointc { SysBusDevice parent_obj; @@ -123,14 +119,13 @@ liointc_read(void *opaque, hwaddr addr, unsigned int size) goto out; } -/* Rest is 4 byte */ +/* Rest are 4 bytes */ if (size != 4 || (addr % 4)) { goto out; } -if (addr >= R_PERCORE_ISR(0) && -addr < R_PERCORE_ISR(NUM_CORES)) { -int core = (addr - R_PERCORE_ISR(0)) / 8; +if (addr >= R_START && addr < R_END) { +int core = (addr - R_START) / R_ISR_SIZE; r = p->per_core_isr[core]; goto out; } @@ -147,7 +142,8 @@ liointc_read(void *opaque, hwaddr addr, unsigned int size) } out: -D(qemu_log("%s: size=%d addr=%lx val=%x\n", __func__, size, addr, r)); +qemu_log_mask(CPU_LOG_INT, "%s: size=%d, addr=%"HWADDR_PRIx", val=%x\n", + __func__, size, addr, r); return r; } @@ -158,7 +154,8 @@ liointc_write(void *opaque, hwaddr addr, struct loongson_liointc *p = opaque; uint32_t value = val64; -D(qemu_log("%s: size=%d, addr=%lx val=%x\n", __func__, size, addr, value)); +qemu_log_mask(CPU_LOG_INT, "%s: size=%d, addr=%"HWADDR_PRIx", val=%x\n", + __func__, size, addr, value); /* Mapper is 1 byte */ if (size == 1 && addr < R_MAPPER_END) { @@ -166,14 +163,13 @@ liointc_write(void *opaque, hwaddr addr, goto out; } -/* Rest is 4 byte */ +/* Rest are 4 bytes */ if (size != 4 || (addr % 4)) { goto out; } -if (addr >= R_PERCORE_ISR(0) && -addr < R_PERCORE_ISR(NUM_CORES)) { -int core = (addr - R_PERCORE_ISR(0)) / 8; +if (addr >= R_START && addr < R_END) { +int core = (addr - R_START) / R_ISR_SIZE; p->per_core_isr[core] = value; goto out; } @@ -224,7 +220,7 @@ static void loongson_liointc_init(Object *obj) } memory_region_init_io(>mmio, obj, _ops, p, - "loongson.liointc", R_END); + TYPE_LOONGSON_LIOINTC, R_END); sysbus_init_mmio(SYS_BUS_DEVICE(obj), >mmio); } -- 2.26.2