| Message ID | 20210107092652.3438696-4-atish.patra@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Assorted fixes for RV32 | expand |
On Thu, Jan 7, 2021 at 10:28 AM Atish Patra <atish.patra@wdc.com> wrote: > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock > allocation if it is requested to be aligned with SMP_CACHE_BYTES. > > Signed-off-by: Atish Patra <atish.patra@wdc.com> Tested-by: Geert Uytterhoeven <geert@linux-m68k.org> (on vexriscv) Gr{oetje,eeting}s, Geert
On Thu, Jan 7, 2021 at 2:57 PM Atish Patra <atish.patra@wdc.com> wrote: > > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock > allocation if it is requested to be aligned with SMP_CACHE_BYTES. > > Signed-off-by: Atish Patra <atish.patra@wdc.com> Looks good to me. Reviewed-by: Anup Patel <anup@brainfault.org> > --- > arch/riscv/include/asm/cache.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h > index 9b58b104559e..c9c669ea2fe6 100644 > --- a/arch/riscv/include/asm/cache.h > +++ b/arch/riscv/include/asm/cache.h > @@ -7,7 +7,11 @@ > #ifndef _ASM_RISCV_CACHE_H > #define _ASM_RISCV_CACHE_H > > +#ifdef CONFIG_64BIT > #define L1_CACHE_SHIFT 6 > +#else > +#define L1_CACHE_SHIFT 5 > +#endif > > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT) > > -- > 2.25.1 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv Regards, Anup
On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote: > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock > allocation if it is requested to be aligned with SMP_CACHE_BYTES. > > Signed-off-by: Atish Patra <atish.patra@wdc.com> > --- > arch/riscv/include/asm/cache.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h > index 9b58b104559e..c9c669ea2fe6 100644 > --- a/arch/riscv/include/asm/cache.h > +++ b/arch/riscv/include/asm/cache.h > @@ -7,7 +7,11 @@ > #ifndef _ASM_RISCV_CACHE_H > #define _ASM_RISCV_CACHE_H > > +#ifdef CONFIG_64BIT > #define L1_CACHE_SHIFT 6 > +#else > +#define L1_CACHE_SHIFT 5 > +#endif > > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT) Should we not instead just #define SMP_CACHE_BYTES L1_CACHE_BYTES like a handful of architectures do? The cache size is sort of fake here, as we don't have any non-coherent mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte cache lines in RISC-V implementations as software may assume that for performance reasons. Not really a strong reason, but I'd prefer to just make these match.
On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote: > > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of > > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock > > allocation if it is requested to be aligned with SMP_CACHE_BYTES. > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com> > > --- > > arch/riscv/include/asm/cache.h | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h > > index 9b58b104559e..c9c669ea2fe6 100644 > > --- a/arch/riscv/include/asm/cache.h > > +++ b/arch/riscv/include/asm/cache.h > > @@ -7,7 +7,11 @@ > > #ifndef _ASM_RISCV_CACHE_H > > #define _ASM_RISCV_CACHE_H > > > > +#ifdef CONFIG_64BIT > > #define L1_CACHE_SHIFT 6 > > +#else > > +#define L1_CACHE_SHIFT 5 > > +#endif > > > > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT) > > Should we not instead just > > #define SMP_CACHE_BYTES L1_CACHE_BYTES > > like a handful of architectures do? > The generic code already defines it that way in include/linux/cache.h > The cache size is sort of fake here, as we don't have any non-coherent > mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte > cache lines in RISC-V implementations as software may assume that for > performance reasons. Not really a strong reason, but I'd prefer to just make > these match. > If it is documented somewhere in the kernel, we should update that. I think SMP_CACHE_BYTES being 64 actually degrades the performance as there will be a fragmented memory blocks with 32 bit bytes gap wherever SMP_CACHE_BYTES is used as an alignment requirement. In addition to that, Geert Uytterhoeven mentioned some panic on vex32 without this patch. I didn't see anything in Qemu though. > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote: > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: >> >> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote: >> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of >> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock >> > allocation if it is requested to be aligned with SMP_CACHE_BYTES. >> > >> > Signed-off-by: Atish Patra <atish.patra@wdc.com> >> > --- >> > arch/riscv/include/asm/cache.h | 4 ++++ >> > 1 file changed, 4 insertions(+) >> > >> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h >> > index 9b58b104559e..c9c669ea2fe6 100644 >> > --- a/arch/riscv/include/asm/cache.h >> > +++ b/arch/riscv/include/asm/cache.h >> > @@ -7,7 +7,11 @@ >> > #ifndef _ASM_RISCV_CACHE_H >> > #define _ASM_RISCV_CACHE_H >> > >> > +#ifdef CONFIG_64BIT >> > #define L1_CACHE_SHIFT 6 >> > +#else >> > +#define L1_CACHE_SHIFT 5 >> > +#endif >> > >> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT) >> >> Should we not instead just >> >> #define SMP_CACHE_BYTES L1_CACHE_BYTES >> >> like a handful of architectures do? >> > > The generic code already defines it that way in include/linux/cache.h > >> The cache size is sort of fake here, as we don't have any non-coherent >> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte >> cache lines in RISC-V implementations as software may assume that for >> performance reasons. Not really a strong reason, but I'd prefer to just make >> these match. >> > > If it is documented somewhere in the kernel, we should update that. I > think SMP_CACHE_BYTES being 64 > actually degrades the performance as there will be a fragmented memory > blocks with 32 bit bytes gap wherever > SMP_CACHE_BYTES is used as an alignment requirement. I don't buy that: if you're trying to align to the cache size then the gaps are the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so there's really no reason for these to be different between the base ISAs. > In addition to that, Geert Uytterhoeven mentioned some panic on vex32 > without this patch. > I didn't see anything in Qemu though. Something like that is probably only going to show up on real hardware, QEMU doesn't really do anything with the cache line size. That said, as there's nothing in our kernel now related to non-coherent memory there really should only be performance issue (at least until we have non-coherent systems). I'd bet that the change is just masking some other bug, either in the software or the hardware. I'd prefer to root cause this rather than just working around it, as it'll probably come back later and in a more difficult way to find. >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote: > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > >> > >> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote: > >> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of > >> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock > >> > allocation if it is requested to be aligned with SMP_CACHE_BYTES. > >> > > >> > Signed-off-by: Atish Patra <atish.patra@wdc.com> > >> > --- > >> > arch/riscv/include/asm/cache.h | 4 ++++ > >> > 1 file changed, 4 insertions(+) > >> > > >> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h > >> > index 9b58b104559e..c9c669ea2fe6 100644 > >> > --- a/arch/riscv/include/asm/cache.h > >> > +++ b/arch/riscv/include/asm/cache.h > >> > @@ -7,7 +7,11 @@ > >> > #ifndef _ASM_RISCV_CACHE_H > >> > #define _ASM_RISCV_CACHE_H > >> > > >> > +#ifdef CONFIG_64BIT > >> > #define L1_CACHE_SHIFT 6 > >> > +#else > >> > +#define L1_CACHE_SHIFT 5 > >> > +#endif > >> > > >> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT) > >> > >> Should we not instead just > >> > >> #define SMP_CACHE_BYTES L1_CACHE_BYTES > >> > >> like a handful of architectures do? > >> > > > > The generic code already defines it that way in include/linux/cache.h > > > >> The cache size is sort of fake here, as we don't have any non-coherent > >> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte > >> cache lines in RISC-V implementations as software may assume that for > >> performance reasons. Not really a strong reason, but I'd prefer to just make > >> these match. > >> > > > > If it is documented somewhere in the kernel, we should update that. I > > think SMP_CACHE_BYTES being 64 > > actually degrades the performance as there will be a fragmented memory > > blocks with 32 bit bytes gap wherever > > SMP_CACHE_BYTES is used as an alignment requirement. > > I don't buy that: if you're trying to align to the cache size then the gaps are > the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so > there's really no reason for these to be different between the base ISAs. > Got your point. I noticed this when fixing the resource tree issue where the SMP_CACHE_BYTES alignment was not intentional but causing the issue. The real issue was solved via another patch in this series though. Just to clarify, if the allocation function intends to allocate consecutive memory, it should use 32 instead of SMP_CACHE_BYTES. This will lead to a #ifdef macro in the code. > > In addition to that, Geert Uytterhoeven mentioned some panic on vex32 > > without this patch. > > I didn't see anything in Qemu though. > > Something like that is probably only going to show up on real hardware, QEMU > doesn't really do anything with the cache line size. That said, as there's > nothing in our kernel now related to non-coherent memory there really should > only be performance issue (at least until we have non-coherent systems). > > I'd bet that the change is just masking some other bug, either in the software > or the hardware. I'd prefer to root cause this rather than just working around > it, as it'll probably come back later and in a more difficult way to find. > Agreed. @Geert Uytterhoeven Can you do a further analysis of the panic you were saying ? We may need to change an alignment requirement to 32 for RV32 manually at some place in code. > >> _______________________________________________ > >> linux-riscv mailing list > >> linux-riscv@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Thu, 14 Jan 2021 13:11:14 PST (-0800), atishp@atishpatra.org wrote: > On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: >> >> On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote: >> > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: >> >> >> >> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote: >> >> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of >> >> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock >> >> > allocation if it is requested to be aligned with SMP_CACHE_BYTES. >> >> > >> >> > Signed-off-by: Atish Patra <atish.patra@wdc.com> >> >> > --- >> >> > arch/riscv/include/asm/cache.h | 4 ++++ >> >> > 1 file changed, 4 insertions(+) >> >> > >> >> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h >> >> > index 9b58b104559e..c9c669ea2fe6 100644 >> >> > --- a/arch/riscv/include/asm/cache.h >> >> > +++ b/arch/riscv/include/asm/cache.h >> >> > @@ -7,7 +7,11 @@ >> >> > #ifndef _ASM_RISCV_CACHE_H >> >> > #define _ASM_RISCV_CACHE_H >> >> > >> >> > +#ifdef CONFIG_64BIT >> >> > #define L1_CACHE_SHIFT 6 >> >> > +#else >> >> > +#define L1_CACHE_SHIFT 5 >> >> > +#endif >> >> > >> >> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT) >> >> >> >> Should we not instead just >> >> >> >> #define SMP_CACHE_BYTES L1_CACHE_BYTES >> >> >> >> like a handful of architectures do? >> >> >> > >> > The generic code already defines it that way in include/linux/cache.h >> > >> >> The cache size is sort of fake here, as we don't have any non-coherent >> >> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte >> >> cache lines in RISC-V implementations as software may assume that for >> >> performance reasons. Not really a strong reason, but I'd prefer to just make >> >> these match. >> >> >> > >> > If it is documented somewhere in the kernel, we should update that. I >> > think SMP_CACHE_BYTES being 64 >> > actually degrades the performance as there will be a fragmented memory >> > blocks with 32 bit bytes gap wherever >> > SMP_CACHE_BYTES is used as an alignment requirement. >> >> I don't buy that: if you're trying to align to the cache size then the gaps are >> the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so >> there's really no reason for these to be different between the base ISAs. >> > > Got your point. I noticed this when fixing the resource tree issue > where the SMP_CACHE_BYTES > alignment was not intentional but causing the issue. The real issue > was solved via another patch in this series though. > > Just to clarify, if the allocation function intends to allocate > consecutive memory, it should use 32 instead of SMP_CACHE_BYTES. > This will lead to a #ifdef macro in the code. Well, if you want to be allocating XLEN-byte sized chunks then you should use an XLEN-based define and if you want to allocate cache-line-sized chunks then you should use some cache-line-based define. I guess I'd have to see the code to know if an ifdef is the right way to go, but the right thing is probably to just change over to something that already exists. >> > In addition to that, Geert Uytterhoeven mentioned some panic on vex32 >> > without this patch. >> > I didn't see anything in Qemu though. >> >> Something like that is probably only going to show up on real hardware, QEMU >> doesn't really do anything with the cache line size. That said, as there's >> nothing in our kernel now related to non-coherent memory there really should >> only be performance issue (at least until we have non-coherent systems). >> >> I'd bet that the change is just masking some other bug, either in the software >> or the hardware. I'd prefer to root cause this rather than just working around >> it, as it'll probably come back later and in a more difficult way to find. >> > > Agreed. @Geert Uytterhoeven Can you do a further analysis of the panic > you were saying ? > We may need to change an alignment requirement to 32 for RV32 manually > at some place in code. > >> >> _______________________________________________ >> >> linux-riscv mailing list >> >> linux-riscv@lists.infradead.org >> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi Atish, On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <atishp@atishpatra.org> wrote: > On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote: > > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > >> > > >> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote: > > >> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of > > >> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock > > >> > allocation if it is requested to be aligned with SMP_CACHE_BYTES. > > >> > > > >> > Signed-off-by: Atish Patra <atish.patra@wdc.com> > > >> > --- > > >> > arch/riscv/include/asm/cache.h | 4 ++++ > > >> > 1 file changed, 4 insertions(+) > > >> > > > >> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h > > >> > index 9b58b104559e..c9c669ea2fe6 100644 > > >> > --- a/arch/riscv/include/asm/cache.h > > >> > +++ b/arch/riscv/include/asm/cache.h > > >> > @@ -7,7 +7,11 @@ > > >> > #ifndef _ASM_RISCV_CACHE_H > > >> > #define _ASM_RISCV_CACHE_H > > >> > > > >> > +#ifdef CONFIG_64BIT > > >> > #define L1_CACHE_SHIFT 6 > > >> > +#else > > >> > +#define L1_CACHE_SHIFT 5 > > >> > +#endif > > >> > > > >> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT) > > >> > > >> Should we not instead just > > >> > > >> #define SMP_CACHE_BYTES L1_CACHE_BYTES > > >> > > >> like a handful of architectures do? > > >> > > > > > > The generic code already defines it that way in include/linux/cache.h > > > > > >> The cache size is sort of fake here, as we don't have any non-coherent > > >> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte > > >> cache lines in RISC-V implementations as software may assume that for > > >> performance reasons. Not really a strong reason, but I'd prefer to just make > > >> these match. > > >> > > > > > > If it is documented somewhere in the kernel, we should update that. I > > > think SMP_CACHE_BYTES being 64 > > > actually degrades the performance as there will be a fragmented memory > > > blocks with 32 bit bytes gap wherever > > > SMP_CACHE_BYTES is used as an alignment requirement. > > > > I don't buy that: if you're trying to align to the cache size then the gaps are > > the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so > > there's really no reason for these to be different between the base ISAs. > > > > Got your point. I noticed this when fixing the resource tree issue > where the SMP_CACHE_BYTES > alignment was not intentional but causing the issue. The real issue > was solved via another patch in this series though. > > Just to clarify, if the allocation function intends to allocate > consecutive memory, it should use 32 instead of SMP_CACHE_BYTES. > This will lead to a #ifdef macro in the code. > > > > In addition to that, Geert Uytterhoeven mentioned some panic on vex32 > > > without this patch. > > > I didn't see anything in Qemu though. > > > > Something like that is probably only going to show up on real hardware, QEMU > > doesn't really do anything with the cache line size. That said, as there's > > nothing in our kernel now related to non-coherent memory there really should > > only be performance issue (at least until we have non-coherent systems). > > > > I'd bet that the change is just masking some other bug, either in the software > > or the hardware. I'd prefer to root cause this rather than just working around > > it, as it'll probably come back later and in a more difficult way to find. > > > > Agreed. @Geert Uytterhoeven Can you do a further analysis of the panic > you were saying ? > We may need to change an alignment requirement to 32 for RV32 manually > at some place in code. My findings were in https://lore.kernel.org/linux-riscv/CAMuHMdWf6K-5y02+WJ6Khu1cD6P0n5x1wYQikrECkuNtAA1pgg@mail.gmail.com/ Note that when the memblock.reserved list kept increasing, it kept on adding the same entry to the list. But that was fixed by "[PATCH 1/4] RISC-V: Do not allocate memblock while iterating reserved memblocks". After that, only the (reproducible) "Unable to handle kernel paging request at virtual address 61636473" was left, always at the same place. No idea where the actual corruption happened. Gr{oetje,eeting}s, Geert
On Thu, 14 Jan 2021 23:59:04 PST (-0800), geert@linux-m68k.org wrote: > Hi Atish, > > On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <atishp@atishpatra.org> wrote: >> On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: >> > On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote: >> > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: >> > >> >> > >> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote: >> > >> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of >> > >> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock >> > >> > allocation if it is requested to be aligned with SMP_CACHE_BYTES. >> > >> > >> > >> > Signed-off-by: Atish Patra <atish.patra@wdc.com> >> > >> > --- >> > >> > arch/riscv/include/asm/cache.h | 4 ++++ >> > >> > 1 file changed, 4 insertions(+) >> > >> > >> > >> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h >> > >> > index 9b58b104559e..c9c669ea2fe6 100644 >> > >> > --- a/arch/riscv/include/asm/cache.h >> > >> > +++ b/arch/riscv/include/asm/cache.h >> > >> > @@ -7,7 +7,11 @@ >> > >> > #ifndef _ASM_RISCV_CACHE_H >> > >> > #define _ASM_RISCV_CACHE_H >> > >> > >> > >> > +#ifdef CONFIG_64BIT >> > >> > #define L1_CACHE_SHIFT 6 >> > >> > +#else >> > >> > +#define L1_CACHE_SHIFT 5 >> > >> > +#endif >> > >> > >> > >> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT) >> > >> >> > >> Should we not instead just >> > >> >> > >> #define SMP_CACHE_BYTES L1_CACHE_BYTES >> > >> >> > >> like a handful of architectures do? >> > >> >> > > >> > > The generic code already defines it that way in include/linux/cache.h >> > > >> > >> The cache size is sort of fake here, as we don't have any non-coherent >> > >> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte >> > >> cache lines in RISC-V implementations as software may assume that for >> > >> performance reasons. Not really a strong reason, but I'd prefer to just make >> > >> these match. >> > >> >> > > >> > > If it is documented somewhere in the kernel, we should update that. I >> > > think SMP_CACHE_BYTES being 64 >> > > actually degrades the performance as there will be a fragmented memory >> > > blocks with 32 bit bytes gap wherever >> > > SMP_CACHE_BYTES is used as an alignment requirement. >> > >> > I don't buy that: if you're trying to align to the cache size then the gaps are >> > the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so >> > there's really no reason for these to be different between the base ISAs. >> > >> >> Got your point. I noticed this when fixing the resource tree issue >> where the SMP_CACHE_BYTES >> alignment was not intentional but causing the issue. The real issue >> was solved via another patch in this series though. >> >> Just to clarify, if the allocation function intends to allocate >> consecutive memory, it should use 32 instead of SMP_CACHE_BYTES. >> This will lead to a #ifdef macro in the code. >> >> > > In addition to that, Geert Uytterhoeven mentioned some panic on vex32 >> > > without this patch. >> > > I didn't see anything in Qemu though. >> > >> > Something like that is probably only going to show up on real hardware, QEMU >> > doesn't really do anything with the cache line size. That said, as there's >> > nothing in our kernel now related to non-coherent memory there really should >> > only be performance issue (at least until we have non-coherent systems). >> > >> > I'd bet that the change is just masking some other bug, either in the software >> > or the hardware. I'd prefer to root cause this rather than just working around >> > it, as it'll probably come back later and in a more difficult way to find. >> > >> >> Agreed. @Geert Uytterhoeven Can you do a further analysis of the panic >> you were saying ? >> We may need to change an alignment requirement to 32 for RV32 manually >> at some place in code. > > My findings were in > https://lore.kernel.org/linux-riscv/CAMuHMdWf6K-5y02+WJ6Khu1cD6P0n5x1wYQikrECkuNtAA1pgg@mail.gmail.com/ > > Note that when the memblock.reserved list kept increasing, it kept on > adding the same entry to the list. But that was fixed by "[PATCH 1/4] > RISC-V: Do not allocate memblock while iterating reserved memblocks". > > After that, only the (reproducible) "Unable to handle kernel paging > request at virtual address 61636473" was left, always at the same place. > No idea where the actual corruption happened. Thanks. Presumably I need an FPGA to run this? That will make it tricky to find anything here on my end. > > Gr{oetje,eeting}s, > > Geert
On Thu, Jan 14, 2021 at 11:59 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Atish, > > On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <atishp@atishpatra.org> wrote: > > On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > > On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote: > > > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > > >> > > > >> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote: > > > >> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of > > > >> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock > > > >> > allocation if it is requested to be aligned with SMP_CACHE_BYTES. > > > >> > > > > >> > Signed-off-by: Atish Patra <atish.patra@wdc.com> > > > >> > --- > > > >> > arch/riscv/include/asm/cache.h | 4 ++++ > > > >> > 1 file changed, 4 insertions(+) > > > >> > > > > >> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h > > > >> > index 9b58b104559e..c9c669ea2fe6 100644 > > > >> > --- a/arch/riscv/include/asm/cache.h > > > >> > +++ b/arch/riscv/include/asm/cache.h > > > >> > @@ -7,7 +7,11 @@ > > > >> > #ifndef _ASM_RISCV_CACHE_H > > > >> > #define _ASM_RISCV_CACHE_H > > > >> > > > > >> > +#ifdef CONFIG_64BIT > > > >> > #define L1_CACHE_SHIFT 6 > > > >> > +#else > > > >> > +#define L1_CACHE_SHIFT 5 > > > >> > +#endif > > > >> > > > > >> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT) > > > >> > > > >> Should we not instead just > > > >> > > > >> #define SMP_CACHE_BYTES L1_CACHE_BYTES > > > >> > > > >> like a handful of architectures do? > > > >> > > > > > > > > The generic code already defines it that way in include/linux/cache.h > > > > > > > >> The cache size is sort of fake here, as we don't have any non-coherent > > > >> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte > > > >> cache lines in RISC-V implementations as software may assume that for > > > >> performance reasons. Not really a strong reason, but I'd prefer to just make > > > >> these match. > > > >> > > > > > > > > If it is documented somewhere in the kernel, we should update that. I > > > > think SMP_CACHE_BYTES being 64 > > > > actually degrades the performance as there will be a fragmented memory > > > > blocks with 32 bit bytes gap wherever > > > > SMP_CACHE_BYTES is used as an alignment requirement. > > > > > > I don't buy that: if you're trying to align to the cache size then the gaps are > > > the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so > > > there's really no reason for these to be different between the base ISAs. > > > > > > > Got your point. I noticed this when fixing the resource tree issue > > where the SMP_CACHE_BYTES > > alignment was not intentional but causing the issue. The real issue > > was solved via another patch in this series though. > > > > Just to clarify, if the allocation function intends to allocate > > consecutive memory, it should use 32 instead of SMP_CACHE_BYTES. > > This will lead to a #ifdef macro in the code. > > > > > > In addition to that, Geert Uytterhoeven mentioned some panic on vex32 > > > > without this patch. > > > > I didn't see anything in Qemu though. > > > > > > Something like that is probably only going to show up on real hardware, QEMU > > > doesn't really do anything with the cache line size. That said, as there's > > > nothing in our kernel now related to non-coherent memory there really should > > > only be performance issue (at least until we have non-coherent systems). > > > > > > I'd bet that the change is just masking some other bug, either in the software > > > or the hardware. I'd prefer to root cause this rather than just working around > > > it, as it'll probably come back later and in a more difficult way to find. > > > > > > > Agreed. @Geert Uytterhoeven Can you do a further analysis of the panic > > you were saying ? > > We may need to change an alignment requirement to 32 for RV32 manually > > at some place in code. > > My findings were in > https://lore.kernel.org/linux-riscv/CAMuHMdWf6K-5y02+WJ6Khu1cD6P0n5x1wYQikrECkuNtAA1pgg@mail.gmail.com/ > > Note that when the memblock.reserved list kept increasing, it kept on > adding the same entry to the list. But that was fixed by "[PATCH 1/4] > RISC-V: Do not allocate memblock while iterating reserved memblocks". > > After that, only the (reproducible) "Unable to handle kernel paging > request at virtual address 61636473" was left, always at the same place. > No idea where the actual corruption happened. > Yes. I was asking about this panic. I don't have the litex fpga to reproduce this as well. Can you take a look at the epc & ra to figure out where exactly is the fault ? That will help to understand the real cause for this panic. > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Atish, On Sat, Jan 16, 2021 at 2:39 AM Atish Patra <atishp@atishpatra.org> wrote: > On Thu, Jan 14, 2021 at 11:59 PM Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > > On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <atishp@atishpatra.org> wrote: > > > On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > > > On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote: > > > > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > > > >> > > > > >> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote: > > > > >> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of > > > > >> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock > > > > >> > allocation if it is requested to be aligned with SMP_CACHE_BYTES. > > > > >> > > > > > >> > Signed-off-by: Atish Patra <atish.patra@wdc.com> > > > > >> > --- > > > > >> > arch/riscv/include/asm/cache.h | 4 ++++ > > > > >> > 1 file changed, 4 insertions(+) > > > > >> > > > > > >> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h > > > > >> > index 9b58b104559e..c9c669ea2fe6 100644 > > > > >> > --- a/arch/riscv/include/asm/cache.h > > > > >> > +++ b/arch/riscv/include/asm/cache.h > > > > >> > @@ -7,7 +7,11 @@ > > > > >> > #ifndef _ASM_RISCV_CACHE_H > > > > >> > #define _ASM_RISCV_CACHE_H > > > > >> > > > > > >> > +#ifdef CONFIG_64BIT > > > > >> > #define L1_CACHE_SHIFT 6 > > > > >> > +#else > > > > >> > +#define L1_CACHE_SHIFT 5 > > > > >> > +#endif > > > > >> > > > > > >> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT) > > > > >> > > > > >> Should we not instead just > > > > >> > > > > >> #define SMP_CACHE_BYTES L1_CACHE_BYTES > > > > >> > > > > >> like a handful of architectures do? > > > > >> > > > > > > > > > > The generic code already defines it that way in include/linux/cache.h > > > > > > > > > >> The cache size is sort of fake here, as we don't have any non-coherent > > > > >> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte > > > > >> cache lines in RISC-V implementations as software may assume that for > > > > >> performance reasons. Not really a strong reason, but I'd prefer to just make > > > > >> these match. > > > > >> > > > > > > > > > > If it is documented somewhere in the kernel, we should update that. I > > > > > think SMP_CACHE_BYTES being 64 > > > > > actually degrades the performance as there will be a fragmented memory > > > > > blocks with 32 bit bytes gap wherever > > > > > SMP_CACHE_BYTES is used as an alignment requirement. > > > > > > > > I don't buy that: if you're trying to align to the cache size then the gaps are > > > > the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so > > > > there's really no reason for these to be different between the base ISAs. > > > > > > > > > > Got your point. I noticed this when fixing the resource tree issue > > > where the SMP_CACHE_BYTES > > > alignment was not intentional but causing the issue. The real issue > > > was solved via another patch in this series though. > > > > > > Just to clarify, if the allocation function intends to allocate > > > consecutive memory, it should use 32 instead of SMP_CACHE_BYTES. > > > This will lead to a #ifdef macro in the code. > > > > > > > > In addition to that, Geert Uytterhoeven mentioned some panic on vex32 > > > > > without this patch. > > > > > I didn't see anything in Qemu though. > > > > > > > > Something like that is probably only going to show up on real hardware, QEMU > > > > doesn't really do anything with the cache line size. That said, as there's > > > > nothing in our kernel now related to non-coherent memory there really should > > > > only be performance issue (at least until we have non-coherent systems). > > > > > > > > I'd bet that the change is just masking some other bug, either in the software > > > > or the hardware. I'd prefer to root cause this rather than just working around > > > > it, as it'll probably come back later and in a more difficult way to find. > > > > > > > > > > Agreed. @Geert Uytterhoeven Can you do a further analysis of the panic > > > you were saying ? > > > We may need to change an alignment requirement to 32 for RV32 manually > > > at some place in code. > > > > My findings were in > > https://lore.kernel.org/linux-riscv/CAMuHMdWf6K-5y02+WJ6Khu1cD6P0n5x1wYQikrECkuNtAA1pgg@mail.gmail.com/ > > > > Note that when the memblock.reserved list kept increasing, it kept on > > adding the same entry to the list. But that was fixed by "[PATCH 1/4] > > RISC-V: Do not allocate memblock while iterating reserved memblocks". > > > > After that, only the (reproducible) "Unable to handle kernel paging > > request at virtual address 61636473" was left, always at the same place. > > No idea where the actual corruption happened. > > > > Yes. I was asking about this panic. I don't have the litex fpga to > reproduce this as well. > Can you take a look at the epc & ra to figure out where exactly is the fault ? > > That will help to understand the real cause for this panic. [...] Freeing initrd memory: 8192K workingset: timestamp_bits=30 max_order=15 bucket_order=0 Block layer SCSI generic (bsg) driver version 0.4 loaded (major 253) io scheduler mq-deadline registered io scheduler kyber registered Unable to handle kernel paging request at virtual address 61636473 Oops [#1] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.11.0-rc3-orangecrab-00068-g267ecb2e2e9d-dirty #37 epc: c000f8e4 ra : c00110e8 sp : c082bc70 gp : c0665948 tp : c0830000 t0 : c08ba500 t1 : 00000002 t2 : 00000000 s0 : c082bc80 s1 : 00000000 a0 : c05e2dec a1 : c08ba4e0 a2 : c0665d38 a3 : 61636473 a4 : f0004003 a5 : f0004000 a6 : c7efffb8 a7 : c08ba4e0 s2 : 01001f00 s3 : c0666000 s4 : c05e2e0c s5 : c0666000 s6 : 80000000 s7 : 00000006 s8 : c05a4000 s9 : c08ba4e0 s10: c05e2dec s11: 00000000 t3 : c08ba500 t4 : 00000001 t5 : 00076400 t6 : c08bbb5e status: 00000120 badaddr: 61636473 cause: 0000000d ---[ end trace 50524759df172195 ]--- Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]--- Looking up epc and ra in System.map, closest symbols are: c000f8b0 t __request_resource c0010ff4 T __request_region The above is with a kernel built from my own config, but using litex_vexriscv_defconfig with https://github.com/geertu/linux branch litex-v5.11 and commit 718aaf7d1c351035 ("RISC-V: Fix L1_CACHE_BYTES for RV32") reverted gives the exact same results. Gr{oetje,eeting}s, Geert
Hi Palmer, On Fri, Jan 15, 2021 at 11:44 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > On Thu, 14 Jan 2021 23:59:04 PST (-0800), geert@linux-m68k.org wrote: > > On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <atishp@atishpatra.org> wrote: > >> On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > >> > On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote: > >> > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > >> > >> > >> > >> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote: > >> > >> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of > >> > >> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock > >> > >> > allocation if it is requested to be aligned with SMP_CACHE_BYTES. > >> > >> > > >> > >> > Signed-off-by: Atish Patra <atish.patra@wdc.com> > >> > >> > --- > >> > >> > arch/riscv/include/asm/cache.h | 4 ++++ > >> > >> > 1 file changed, 4 insertions(+) > >> > >> > > >> > >> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h > >> > >> > index 9b58b104559e..c9c669ea2fe6 100644 > >> > >> > --- a/arch/riscv/include/asm/cache.h > >> > >> > +++ b/arch/riscv/include/asm/cache.h > >> > >> > @@ -7,7 +7,11 @@ > >> > >> > #ifndef _ASM_RISCV_CACHE_H > >> > >> > #define _ASM_RISCV_CACHE_H > >> > >> > > >> > >> > +#ifdef CONFIG_64BIT > >> > >> > #define L1_CACHE_SHIFT 6 > >> > >> > +#else > >> > >> > +#define L1_CACHE_SHIFT 5 > >> > >> > +#endif > >> > >> > > >> > >> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT) > >> > >> > >> > >> Should we not instead just > >> > >> > >> > >> #define SMP_CACHE_BYTES L1_CACHE_BYTES > >> > >> > >> > >> like a handful of architectures do? > >> > >> > >> > > > >> > > The generic code already defines it that way in include/linux/cache.h > >> > > > >> > >> The cache size is sort of fake here, as we don't have any non-coherent > >> > >> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte > >> > >> cache lines in RISC-V implementations as software may assume that for > >> > >> performance reasons. Not really a strong reason, but I'd prefer to just make > >> > >> these match. > >> > >> > >> > > > >> > > If it is documented somewhere in the kernel, we should update that. I > >> > > think SMP_CACHE_BYTES being 64 > >> > > actually degrades the performance as there will be a fragmented memory > >> > > blocks with 32 bit bytes gap wherever > >> > > SMP_CACHE_BYTES is used as an alignment requirement. > >> > > >> > I don't buy that: if you're trying to align to the cache size then the gaps are > >> > the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so > >> > there's really no reason for these to be different between the base ISAs. > >> > > >> > >> Got your point. I noticed this when fixing the resource tree issue > >> where the SMP_CACHE_BYTES > >> alignment was not intentional but causing the issue. The real issue > >> was solved via another patch in this series though. > >> > >> Just to clarify, if the allocation function intends to allocate > >> consecutive memory, it should use 32 instead of SMP_CACHE_BYTES. > >> This will lead to a #ifdef macro in the code. > >> > >> > > In addition to that, Geert Uytterhoeven mentioned some panic on vex32 > >> > > without this patch. > >> > > I didn't see anything in Qemu though. > >> > > >> > Something like that is probably only going to show up on real hardware, QEMU > >> > doesn't really do anything with the cache line size. That said, as there's > >> > nothing in our kernel now related to non-coherent memory there really should > >> > only be performance issue (at least until we have non-coherent systems). > >> > > >> > I'd bet that the change is just masking some other bug, either in the software > >> > or the hardware. I'd prefer to root cause this rather than just working around > >> > it, as it'll probably come back later and in a more difficult way to find. > >> > > >> > >> Agreed. @Geert Uytterhoeven Can you do a further analysis of the panic > >> you were saying ? > >> We may need to change an alignment requirement to 32 for RV32 manually > >> at some place in code. > > > > My findings were in > > https://lore.kernel.org/linux-riscv/CAMuHMdWf6K-5y02+WJ6Khu1cD6P0n5x1wYQikrECkuNtAA1pgg@mail.gmail.com/ > > > > Note that when the memblock.reserved list kept increasing, it kept on > > adding the same entry to the list. But that was fixed by "[PATCH 1/4] > > RISC-V: Do not allocate memblock while iterating reserved memblocks". > > > > After that, only the (reproducible) "Unable to handle kernel paging > > request at virtual address 61636473" was left, always at the same place. > > No idea where the actual corruption happened. > > Thanks. Presumably I need an FPGA to run this? That will make it tricky to > find anything here on my end. In theory, it should work with the LiteX simulation, too. I.e. follow the instructions at https://github.com/litex-hub/linux-on-litex-vexriscv You can find prebuilt binaries at https://github.com/litex-hub/linux-on-litex-vexriscv/issues/164 Take images/opensbi.bin from opensbi_2020_12_15.zip, and images/rootfs.cpio from linux_2021_01_11.zip. Take images/Image from your own kernel build. Unfortunately it seems the simulator is currently broken, and kernels (both prebuilt and my own config) hang after "sched_clock: 64 bits at 1000kHz, resolution 1000ns, wraps every 2199023255500ns" Gr{oetje,eeting}s, Geert
On Sun, Jan 17, 2021 at 8:03 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Fri, Jan 15, 2021 at 11:44 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Thu, 14 Jan 2021 23:59:04 PST (-0800), geert@linux-m68k.org wrote: > > > On Thu, Jan 14, 2021 at 10:11 PM Atish Patra <atishp@atishpatra.org> wrote: > > >> On Thu, Jan 14, 2021 at 11:46 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > >> > On Thu, 14 Jan 2021 10:33:01 PST (-0800), atishp@atishpatra.org wrote: > > >> > > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > >> > >> > > >> > >> On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote: > > >> > >> > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of > > >> > >> > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock > > >> > >> > allocation if it is requested to be aligned with SMP_CACHE_BYTES. > > >> > >> > > > >> > >> > Signed-off-by: Atish Patra <atish.patra@wdc.com> > > >> > >> > --- > > >> > >> > arch/riscv/include/asm/cache.h | 4 ++++ > > >> > >> > 1 file changed, 4 insertions(+) > > >> > >> > > > >> > >> > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h > > >> > >> > index 9b58b104559e..c9c669ea2fe6 100644 > > >> > >> > --- a/arch/riscv/include/asm/cache.h > > >> > >> > +++ b/arch/riscv/include/asm/cache.h > > >> > >> > @@ -7,7 +7,11 @@ > > >> > >> > #ifndef _ASM_RISCV_CACHE_H > > >> > >> > #define _ASM_RISCV_CACHE_H > > >> > >> > > > >> > >> > +#ifdef CONFIG_64BIT > > >> > >> > #define L1_CACHE_SHIFT 6 > > >> > >> > +#else > > >> > >> > +#define L1_CACHE_SHIFT 5 > > >> > >> > +#endif > > >> > >> > > > >> > >> > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT) > > >> > >> > > >> > >> Should we not instead just > > >> > >> > > >> > >> #define SMP_CACHE_BYTES L1_CACHE_BYTES > > >> > >> > > >> > >> like a handful of architectures do? > > >> > >> > > >> > > > > >> > > The generic code already defines it that way in include/linux/cache.h > > >> > > > > >> > >> The cache size is sort of fake here, as we don't have any non-coherent > > >> > >> mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte > > >> > >> cache lines in RISC-V implementations as software may assume that for > > >> > >> performance reasons. Not really a strong reason, but I'd prefer to just make > > >> > >> these match. > > >> > >> > > >> > > > > >> > > If it is documented somewhere in the kernel, we should update that. I > > >> > > think SMP_CACHE_BYTES being 64 > > >> > > actually degrades the performance as there will be a fragmented memory > > >> > > blocks with 32 bit bytes gap wherever > > >> > > SMP_CACHE_BYTES is used as an alignment requirement. > > >> > > > >> > I don't buy that: if you're trying to align to the cache size then the gaps are > > >> > the whole point. IIUC the 64-byte cache lines come from DDR, not XLEN, so > > >> > there's really no reason for these to be different between the base ISAs. > > >> > > > >> > > >> Got your point. I noticed this when fixing the resource tree issue > > >> where the SMP_CACHE_BYTES > > >> alignment was not intentional but causing the issue. The real issue > > >> was solved via another patch in this series though. > > >> > > >> Just to clarify, if the allocation function intends to allocate > > >> consecutive memory, it should use 32 instead of SMP_CACHE_BYTES. > > >> This will lead to a #ifdef macro in the code. > > >> > > >> > > In addition to that, Geert Uytterhoeven mentioned some panic on vex32 > > >> > > without this patch. > > >> > > I didn't see anything in Qemu though. > > >> > > > >> > Something like that is probably only going to show up on real hardware, QEMU > > >> > doesn't really do anything with the cache line size. That said, as there's > > >> > nothing in our kernel now related to non-coherent memory there really should > > >> > only be performance issue (at least until we have non-coherent systems). > > >> > > > >> > I'd bet that the change is just masking some other bug, either in the software > > >> > or the hardware. I'd prefer to root cause this rather than just working around > > >> > it, as it'll probably come back later and in a more difficult way to find. > > >> > > > >> > > >> Agreed. @Geert Uytterhoeven Can you do a further analysis of the panic > > >> you were saying ? > > >> We may need to change an alignment requirement to 32 for RV32 manually > > >> at some place in code. > > > > > > My findings were in > > > https://lore.kernel.org/linux-riscv/CAMuHMdWf6K-5y02+WJ6Khu1cD6P0n5x1wYQikrECkuNtAA1pgg@mail.gmail.com/ > > > > > > Note that when the memblock.reserved list kept increasing, it kept on > > > adding the same entry to the list. But that was fixed by "[PATCH 1/4] > > > RISC-V: Do not allocate memblock while iterating reserved memblocks". > > > > > > After that, only the (reproducible) "Unable to handle kernel paging > > > request at virtual address 61636473" was left, always at the same place. > > > No idea where the actual corruption happened. > > > > Thanks. Presumably I need an FPGA to run this? That will make it tricky to > > find anything here on my end. > > In theory, it should work with the LiteX simulation, too. > I.e. follow the instructions at > https://github.com/litex-hub/linux-on-litex-vexriscv > You can find prebuilt binaries at > https://github.com/litex-hub/linux-on-litex-vexriscv/issues/164 > Take images/opensbi.bin from opensbi_2020_12_15.zip, and > images/rootfs.cpio from linux_2021_01_11.zip. > Take images/Image from your own kernel build. > > Unfortunately it seems the simulator is currently broken, and kernels > (both prebuilt and my own config) hang after > "sched_clock: 64 bits at 1000kHz, resolution 1000ns, wraps every > 2199023255500ns" In the mean time, the simulator got fixed. Unfortunately the crash does not happen on the simulator. Gr{oetje,eeting}s, Geert
On Thu, Jan 14, 2021 at 7:34 PM Atish Patra <atishp@atishpatra.org> wrote: > On Wed, Jan 13, 2021 at 9:10 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Thu, 07 Jan 2021 01:26:51 PST (-0800), Atish Patra wrote: > > > SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of > > > 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock > > > allocation if it is requested to be aligned with SMP_CACHE_BYTES. > > > > > > Signed-off-by: Atish Patra <atish.patra@wdc.com> > > > --- > > > arch/riscv/include/asm/cache.h | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h > > > index 9b58b104559e..c9c669ea2fe6 100644 > > > --- a/arch/riscv/include/asm/cache.h > > > +++ b/arch/riscv/include/asm/cache.h > > > @@ -7,7 +7,11 @@ > > > #ifndef _ASM_RISCV_CACHE_H > > > #define _ASM_RISCV_CACHE_H > > > > > > +#ifdef CONFIG_64BIT > > > #define L1_CACHE_SHIFT 6 > > > +#else > > > +#define L1_CACHE_SHIFT 5 > > > +#endif > > > > > > #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT) > > > > Should we not instead just > > > > #define SMP_CACHE_BYTES L1_CACHE_BYTES > > > > like a handful of architectures do? > > > > The generic code already defines it that way in include/linux/cache.h > > > The cache size is sort of fake here, as we don't have any non-coherent > > mechanisms, but IIRC we wrote somewhere that it's recommended to have 64-byte > > cache lines in RISC-V implementations as software may assume that for > > performance reasons. Not really a strong reason, but I'd prefer to just make > > these match. > > > > If it is documented somewhere in the kernel, we should update that. I > think SMP_CACHE_BYTES being 64 > actually degrades the performance as there will be a fragmented memory > blocks with 32 bit bytes gap wherever > SMP_CACHE_BYTES is used as an alignment requirement. > > In addition to that, Geert Uytterhoeven mentioned some panic on vex32 > without this patch. > I didn't see anything in Qemu though. FTR, I no longer need this patch on vexriscv. It seems it just masked an issue. Gr{oetje,eeting}s, Geert
diff --git a/arch/riscv/include/asm/cache.h b/arch/riscv/include/asm/cache.h index 9b58b104559e..c9c669ea2fe6 100644 --- a/arch/riscv/include/asm/cache.h +++ b/arch/riscv/include/asm/cache.h @@ -7,7 +7,11 @@ #ifndef _ASM_RISCV_CACHE_H #define _ASM_RISCV_CACHE_H +#ifdef CONFIG_64BIT #define L1_CACHE_SHIFT 6 +#else +#define L1_CACHE_SHIFT 5 +#endif #define L1_CACHE_BYTES (1 << L1_CACHE_SHIFT)
SMP_CACHE_BYTES/L1_CACHE_BYTES should be defined as 32 instead of 64 for RV32. Otherwise, there will be hole of 32 bytes with each memblock allocation if it is requested to be aligned with SMP_CACHE_BYTES. Signed-off-by: Atish Patra <atish.patra@wdc.com> --- arch/riscv/include/asm/cache.h | 4 ++++ 1 file changed, 4 insertions(+)