| Message ID | 20230526165958.908-5-jszhang@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: Reduce ARCH_KMALLOC_MINALIGN to 8 | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/cover_letter | success | Series has a cover letter |
| conchuod/tree_selection | success | Guessed tree name to be for-next at HEAD ac9a78681b92 |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 6 and now 6 |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 2308 this patch: 2308 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 13388 this patch: 13388 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 3 this patch: 3 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 76 lines checked |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
Hey Jisheng, On Sat, May 27, 2023 at 12:59:56AM +0800, Jisheng Zhang wrote: > We will soon take different actions by checking the HW is noncoherent > or not, I.E ZICBOM/ERRATA_THEAD_CMO or not. > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > --- > arch/riscv/errata/thead/errata.c | 19 +++++++++++-------- > arch/riscv/include/asm/cacheflush.h | 4 ++-- > arch/riscv/kernel/setup.c | 6 +++++- > arch/riscv/mm/dma-noncoherent.c | 10 ++++++---- > 4 files changed, 24 insertions(+), 15 deletions(-) > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > index be84b14f0118..c192b80a5166 100644 > --- a/arch/riscv/errata/thead/errata.c > +++ b/arch/riscv/errata/thead/errata.c > @@ -36,21 +36,24 @@ static bool errata_probe_pbmt(unsigned int stage, > static bool errata_probe_cmo(unsigned int stage, > unsigned long arch_id, unsigned long impid) > { > - if (!IS_ENABLED(CONFIG_ERRATA_THEAD_CMO)) > - return false; > - > - if (arch_id != 0 || impid != 0) > - return false; > + bool cmo; > > if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > return false; > > + if (IS_ENABLED(CONFIG_ERRATA_THEAD_CMO) && > + (arch_id == 0 && impid == 0)) > + cmo = true; > + else > + cmo = false; > + > if (stage == RISCV_ALTERNATIVES_BOOT) { > - riscv_cbom_block_size = L1_CACHE_BYTES; > - riscv_noncoherent_supported(); > + if (cmo) > + riscv_cbom_block_size = L1_CACHE_BYTES; > + riscv_noncoherent_supported(cmo); > } > > - return true; > + return cmo; I don't really understand the changes that you are making to this function, so that is tries really hard to call riscv_noncoherent_supported(). Why do we need to always call the function in the erratum's probe function, if the erratum is not detected, given that riscv_noncoherent_supported() is called immediately after apply_boot_alternatives() in setup_arch()? > } > > static bool errata_probe_pmu(unsigned int stage, > diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h > index 8091b8bf4883..9d056c9b625a 100644 > --- a/arch/riscv/include/asm/cacheflush.h > +++ b/arch/riscv/include/asm/cacheflush.h > @@ -54,9 +54,9 @@ extern unsigned int riscv_cboz_block_size; > void riscv_init_cbo_blocksizes(void); > > #ifdef CONFIG_RISCV_DMA_NONCOHERENT > -void riscv_noncoherent_supported(void); > +void riscv_noncoherent_supported(bool cmo); I think it would "read better" if you renamed this variable to "have_cmo". > #else > -static inline void riscv_noncoherent_supported(void) {} > +static inline void riscv_noncoherent_supported(bool cmo) {} > #endif > > /* > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index 36b026057503..565f3e20169b 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -264,6 +264,7 @@ static void __init parse_dtb(void) > > void __init setup_arch(char **cmdline_p) > { > + bool cmo; > parse_dtb(); > setup_initial_init_mm(_stext, _etext, _edata, _end); > > @@ -298,7 +299,10 @@ void __init setup_arch(char **cmdline_p) > apply_boot_alternatives(); > if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) && > riscv_isa_extension_available(NULL, ZICBOM)) > - riscv_noncoherent_supported(); > + cmo = true; > + else > + cmo = false; > + riscv_noncoherent_supported(cmo); As a nit, could you put a newline before the call to riscv_noncoherent_supported()? > } > > static int __init topology_init(void) > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c > index d51a75864e53..0e172e2b4751 100644 > --- a/arch/riscv/mm/dma-noncoherent.c > +++ b/arch/riscv/mm/dma-noncoherent.c > @@ -72,9 +72,11 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, > dev->dma_coherent = coherent; > } > > -void riscv_noncoherent_supported(void) > +void riscv_noncoherent_supported(bool cmo) > { > - WARN(!riscv_cbom_block_size, > - "Non-coherent DMA support enabled without a block size\n"); > - noncoherent_supported = true; > + if (cmo) { > + WARN(!riscv_cbom_block_size, > + "Non-coherent DMA support enabled without a block size\n"); > + noncoherent_supported = true; > + } The other places that we do a WARN() because of screwed up devicetrees for CMO things, we do a WARN_TAINT(CPU_OUT_OF_SPEC). Should we do the same here too? Cheers, Conor.
On Mon, May 29, 2023 at 12:13:10PM +0100, Conor Dooley wrote: > Hey Jisheng, Hi Conor, > > On Sat, May 27, 2023 at 12:59:56AM +0800, Jisheng Zhang wrote: > > We will soon take different actions by checking the HW is noncoherent > > or not, I.E ZICBOM/ERRATA_THEAD_CMO or not. > > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > > --- > > arch/riscv/errata/thead/errata.c | 19 +++++++++++-------- > > arch/riscv/include/asm/cacheflush.h | 4 ++-- > > arch/riscv/kernel/setup.c | 6 +++++- > > arch/riscv/mm/dma-noncoherent.c | 10 ++++++---- > > 4 files changed, 24 insertions(+), 15 deletions(-) > > > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > > index be84b14f0118..c192b80a5166 100644 > > --- a/arch/riscv/errata/thead/errata.c > > +++ b/arch/riscv/errata/thead/errata.c > > @@ -36,21 +36,24 @@ static bool errata_probe_pbmt(unsigned int stage, > > static bool errata_probe_cmo(unsigned int stage, > > unsigned long arch_id, unsigned long impid) > > { > > - if (!IS_ENABLED(CONFIG_ERRATA_THEAD_CMO)) > > - return false; > > - > > - if (arch_id != 0 || impid != 0) > > - return false; > > + bool cmo; > > > > if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > > return false; > > > > + if (IS_ENABLED(CONFIG_ERRATA_THEAD_CMO) && > > + (arch_id == 0 && impid == 0)) > > + cmo = true; > > + else > > + cmo = false; > > + > > if (stage == RISCV_ALTERNATIVES_BOOT) { > > - riscv_cbom_block_size = L1_CACHE_BYTES; > > - riscv_noncoherent_supported(); > > + if (cmo) > > + riscv_cbom_block_size = L1_CACHE_BYTES; > > + riscv_noncoherent_supported(cmo); > > } > > > > - return true; > > + return cmo; > > I don't really understand the changes that you are making to this > function, so that is tries really hard to call > riscv_noncoherent_supported(). Why do we need to always call the function > in the erratum's probe function, if the erratum is not detected, given In one unified kernel Image, to support both coherent and noncoherent platforms(currently, either T-HEAD CMO or ZICBOM), we need to let the kmalloc meet both cases, specifically, ARCH_DMA_MINALIGN aligned. Once we know the underlying HW is coherent, I.E neither T-HEAD CMO nor ZICBOM, we need to notice kmalloc we are safe to reduce the alignment to 1. The notice action is done in patch 5: + } else { + dma_cache_alignment = 1; > that riscv_noncoherent_supported() is called immediately after > apply_boot_alternatives() in setup_arch()? > > > } > > > > static bool errata_probe_pmu(unsigned int stage, > > diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h > > index 8091b8bf4883..9d056c9b625a 100644 > > --- a/arch/riscv/include/asm/cacheflush.h > > +++ b/arch/riscv/include/asm/cacheflush.h > > @@ -54,9 +54,9 @@ extern unsigned int riscv_cboz_block_size; > > void riscv_init_cbo_blocksizes(void); > > > > #ifdef CONFIG_RISCV_DMA_NONCOHERENT > > -void riscv_noncoherent_supported(void); > > +void riscv_noncoherent_supported(bool cmo); > > I think it would "read better" if you renamed this variable to > "have_cmo". > > > #else > > -static inline void riscv_noncoherent_supported(void) {} > > +static inline void riscv_noncoherent_supported(bool cmo) {} > > #endif > > > > /* > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > > index 36b026057503..565f3e20169b 100644 > > --- a/arch/riscv/kernel/setup.c > > +++ b/arch/riscv/kernel/setup.c > > @@ -264,6 +264,7 @@ static void __init parse_dtb(void) > > > > void __init setup_arch(char **cmdline_p) > > { > > + bool cmo; > > parse_dtb(); > > setup_initial_init_mm(_stext, _etext, _edata, _end); > > > > @@ -298,7 +299,10 @@ void __init setup_arch(char **cmdline_p) > > apply_boot_alternatives(); > > if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) && > > riscv_isa_extension_available(NULL, ZICBOM)) > > - riscv_noncoherent_supported(); > > + cmo = true; > > + else > > + cmo = false; > > + riscv_noncoherent_supported(cmo); > > As a nit, could you put a newline before the call to > riscv_noncoherent_supported()? > > > } > > > > static int __init topology_init(void) > > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c > > index d51a75864e53..0e172e2b4751 100644 > > --- a/arch/riscv/mm/dma-noncoherent.c > > +++ b/arch/riscv/mm/dma-noncoherent.c > > @@ -72,9 +72,11 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, > > dev->dma_coherent = coherent; > > } > > > > -void riscv_noncoherent_supported(void) > > +void riscv_noncoherent_supported(bool cmo) > > { > > - WARN(!riscv_cbom_block_size, > > - "Non-coherent DMA support enabled without a block size\n"); > > - noncoherent_supported = true; > > + if (cmo) { > > + WARN(!riscv_cbom_block_size, > > + "Non-coherent DMA support enabled without a block size\n"); > > + noncoherent_supported = true; > > + } > > The other places that we do a WARN() because of screwed up devicetrees > for CMO things, we do a WARN_TAINT(CPU_OUT_OF_SPEC). Should we do the > same here too? > > Cheers, > Conor.
On Wed, May 31, 2023 at 11:24:19PM +0800, Jisheng Zhang wrote: > On Mon, May 29, 2023 at 12:13:10PM +0100, Conor Dooley wrote: > > Hey Jisheng, > > Hi Conor, > > > > > On Sat, May 27, 2023 at 12:59:56AM +0800, Jisheng Zhang wrote: > > > We will soon take different actions by checking the HW is noncoherent > > > or not, I.E ZICBOM/ERRATA_THEAD_CMO or not. > > > > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > > > --- > > > arch/riscv/errata/thead/errata.c | 19 +++++++++++-------- > > > arch/riscv/include/asm/cacheflush.h | 4 ++-- > > > arch/riscv/kernel/setup.c | 6 +++++- > > > arch/riscv/mm/dma-noncoherent.c | 10 ++++++---- > > > 4 files changed, 24 insertions(+), 15 deletions(-) > > > > > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > > > index be84b14f0118..c192b80a5166 100644 > > > --- a/arch/riscv/errata/thead/errata.c > > > +++ b/arch/riscv/errata/thead/errata.c > > > @@ -36,21 +36,24 @@ static bool errata_probe_pbmt(unsigned int stage, > > > static bool errata_probe_cmo(unsigned int stage, > > > unsigned long arch_id, unsigned long impid) > > > { > > > - if (!IS_ENABLED(CONFIG_ERRATA_THEAD_CMO)) > > > - return false; > > > - > > > - if (arch_id != 0 || impid != 0) > > > - return false; > > > + bool cmo; > > > > > > if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > > > return false; > > > > > > + if (IS_ENABLED(CONFIG_ERRATA_THEAD_CMO) && > > > + (arch_id == 0 && impid == 0)) > > > + cmo = true; > > > + else > > > + cmo = false; > > > + > > > if (stage == RISCV_ALTERNATIVES_BOOT) { > > > - riscv_cbom_block_size = L1_CACHE_BYTES; > > > - riscv_noncoherent_supported(); > > > + if (cmo) > > > + riscv_cbom_block_size = L1_CACHE_BYTES; > > > + riscv_noncoherent_supported(cmo); > > > } > > > > > > - return true; > > > + return cmo; > > > > I don't really understand the changes that you are making to this > > function, so that is tries really hard to call > > riscv_noncoherent_supported(). Why do we need to always call the function > > in the erratum's probe function, if the erratum is not detected, given > > In one unified kernel Image, to support both coherent and noncoherent > platforms(currently, either T-HEAD CMO or ZICBOM), we need to let the > kmalloc meet both cases, specifically, ARCH_DMA_MINALIGN aligned. seems adding three words can make it better: kmalloc meet both cases at the beginning, specifically ... > Once we know the underlying HW is coherent, I.E neither T-HEAD CMO nor > ZICBOM, we need to notice kmalloc we are safe to reduce the alignment > to 1. The notice action is done in patch 5: > > + } else { > + dma_cache_alignment = 1; > > > > that riscv_noncoherent_supported() is called immediately after > > apply_boot_alternatives() in setup_arch()? > > > > > } > > > > > > static bool errata_probe_pmu(unsigned int stage, > > > diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h > > > index 8091b8bf4883..9d056c9b625a 100644 > > > --- a/arch/riscv/include/asm/cacheflush.h > > > +++ b/arch/riscv/include/asm/cacheflush.h > > > @@ -54,9 +54,9 @@ extern unsigned int riscv_cboz_block_size; > > > void riscv_init_cbo_blocksizes(void); > > > > > > #ifdef CONFIG_RISCV_DMA_NONCOHERENT > > > -void riscv_noncoherent_supported(void); > > > +void riscv_noncoherent_supported(bool cmo); > > > > I think it would "read better" if you renamed this variable to > > "have_cmo". > > > > > #else > > > -static inline void riscv_noncoherent_supported(void) {} > > > +static inline void riscv_noncoherent_supported(bool cmo) {} > > > #endif > > > > > > /* > > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > > > index 36b026057503..565f3e20169b 100644 > > > --- a/arch/riscv/kernel/setup.c > > > +++ b/arch/riscv/kernel/setup.c > > > @@ -264,6 +264,7 @@ static void __init parse_dtb(void) > > > > > > void __init setup_arch(char **cmdline_p) > > > { > > > + bool cmo; > > > parse_dtb(); > > > setup_initial_init_mm(_stext, _etext, _edata, _end); > > > > > > @@ -298,7 +299,10 @@ void __init setup_arch(char **cmdline_p) > > > apply_boot_alternatives(); > > > if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) && > > > riscv_isa_extension_available(NULL, ZICBOM)) > > > - riscv_noncoherent_supported(); > > > + cmo = true; > > > + else > > > + cmo = false; > > > + riscv_noncoherent_supported(cmo); > > > > As a nit, could you put a newline before the call to > > riscv_noncoherent_supported()? > > > > > } > > > > > > static int __init topology_init(void) > > > diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c > > > index d51a75864e53..0e172e2b4751 100644 > > > --- a/arch/riscv/mm/dma-noncoherent.c > > > +++ b/arch/riscv/mm/dma-noncoherent.c > > > @@ -72,9 +72,11 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, > > > dev->dma_coherent = coherent; > > > } > > > > > > -void riscv_noncoherent_supported(void) > > > +void riscv_noncoherent_supported(bool cmo) > > > { > > > - WARN(!riscv_cbom_block_size, > > > - "Non-coherent DMA support enabled without a block size\n"); > > > - noncoherent_supported = true; > > > + if (cmo) { > > > + WARN(!riscv_cbom_block_size, > > > + "Non-coherent DMA support enabled without a block size\n"); > > > + noncoherent_supported = true; > > > + } > > > > The other places that we do a WARN() because of screwed up devicetrees > > for CMO things, we do a WARN_TAINT(CPU_OUT_OF_SPEC). Should we do the > > same here too? > > > > Cheers, > > Conor. > >
On Wed, May 31, 2023 at 11:28:22PM +0800, Jisheng Zhang wrote: > On Wed, May 31, 2023 at 11:24:19PM +0800, Jisheng Zhang wrote: > > On Mon, May 29, 2023 at 12:13:10PM +0100, Conor Dooley wrote: > > > On Sat, May 27, 2023 at 12:59:56AM +0800, Jisheng Zhang wrote: > > > > We will soon take different actions by checking the HW is noncoherent > > > > or not, I.E ZICBOM/ERRATA_THEAD_CMO or not. > > > > > > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > > > > --- > > > > arch/riscv/errata/thead/errata.c | 19 +++++++++++-------- > > > > arch/riscv/include/asm/cacheflush.h | 4 ++-- > > > > arch/riscv/kernel/setup.c | 6 +++++- > > > > arch/riscv/mm/dma-noncoherent.c | 10 ++++++---- > > > > 4 files changed, 24 insertions(+), 15 deletions(-) > > > > > > > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > > > > index be84b14f0118..c192b80a5166 100644 > > > > --- a/arch/riscv/errata/thead/errata.c > > > > +++ b/arch/riscv/errata/thead/errata.c > > > > @@ -36,21 +36,24 @@ static bool errata_probe_pbmt(unsigned int stage, > > > > static bool errata_probe_cmo(unsigned int stage, > > > > unsigned long arch_id, unsigned long impid) > > > > { > > > > - if (!IS_ENABLED(CONFIG_ERRATA_THEAD_CMO)) > > > > - return false; > > > > - > > > > - if (arch_id != 0 || impid != 0) > > > > - return false; > > > > + bool cmo; > > > > > > > > if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > > > > return false; > > > > > > > > + if (IS_ENABLED(CONFIG_ERRATA_THEAD_CMO) && > > > > + (arch_id == 0 && impid == 0)) > > > > + cmo = true; > > > > + else > > > > + cmo = false; > > > > + > > > > if (stage == RISCV_ALTERNATIVES_BOOT) { > > > > - riscv_cbom_block_size = L1_CACHE_BYTES; > > > > - riscv_noncoherent_supported(); > > > > + if (cmo) > > > > + riscv_cbom_block_size = L1_CACHE_BYTES; > > > > + riscv_noncoherent_supported(cmo); > > > > } > > > > > > > > - return true; > > > > + return cmo; > > > > > > I don't really understand the changes that you are making to this > > > function, so that is tries really hard to call > > > riscv_noncoherent_supported(). Why do we need to always call the function > > > in the erratum's probe function, if the erratum is not detected, given > > > > In one unified kernel Image, to support both coherent and noncoherent > > platforms(currently, either T-HEAD CMO or ZICBOM), we need to let the > > kmalloc meet both cases, specifically, ARCH_DMA_MINALIGN aligned. > > seems adding three words can make it better: > > kmalloc meet both cases at the beginning, specifically ... > > > Once we know the underlying HW is coherent, I.E neither T-HEAD CMO nor > > ZICBOM, we need to notice kmalloc we are safe to reduce the alignment > > to 1. The notice action is done in patch 5: > > > > + } else { > > + dma_cache_alignment = 1; > > > > > > > that riscv_noncoherent_supported() is called immediately after > > > apply_boot_alternatives() in setup_arch()? This bit here is the key part of my confusion. You try really hard in the errata stuff to call riscv_noncoherent_supported(), which I do understand is because of the other branch that you add to the function later in the series. What I do not understand is why we are not able to rely on the call to it in setup_arch() to trigger it when we do not have T-HEAD CMOs or Zicbom. You've explained why you want to make sure it always gets called during boot, but my question is about why it looks like it is being called more than once. Actually, now that I think of it, what happens on a T-HEAD system where there is no T-HEAD CMOs, but there is Zicbom. In theory, this could exist. Bear with me here a moment in case I am completely wrong, snippet is from setup_arch() apply_boot_alternatives(); On my example system, this will trigger, eventually sending us into errata_probe_cmo(), where we will call riscv_noncoherent_supported() with false, setting dma_cache_alignment to 1. if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) && riscv_isa_extension_available(NULL, ZICBOM)) cmo = true; On this system, this will be true. else cmo = false; riscv_noncoherent_supported(cmo); now riscv_noncoherent_supported() is called with true, and we have dma_cache_alignment = 1 still. Is that not problematic? Or the inverse, where the T-HEAD system has its custom CMOs and there is no Zicbom, it gets called twice with different args too. There's clearly something fundamental that I am missing here, this seems like it should be immediately obvious why this either cannot happen or is not a problem, but I can't see it. Sorry, Conor.
On Wed, May 31, 2023 at 05:28:56PM +0100, Conor Dooley wrote: > On Wed, May 31, 2023 at 11:28:22PM +0800, Jisheng Zhang wrote: > > On Wed, May 31, 2023 at 11:24:19PM +0800, Jisheng Zhang wrote: > > > On Mon, May 29, 2023 at 12:13:10PM +0100, Conor Dooley wrote: > > > > On Sat, May 27, 2023 at 12:59:56AM +0800, Jisheng Zhang wrote: > > > > > We will soon take different actions by checking the HW is noncoherent > > > > > or not, I.E ZICBOM/ERRATA_THEAD_CMO or not. > > > > > > > > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > > > > > --- > > > > > arch/riscv/errata/thead/errata.c | 19 +++++++++++-------- > > > > > arch/riscv/include/asm/cacheflush.h | 4 ++-- > > > > > arch/riscv/kernel/setup.c | 6 +++++- > > > > > arch/riscv/mm/dma-noncoherent.c | 10 ++++++---- > > > > > 4 files changed, 24 insertions(+), 15 deletions(-) > > > > > > > > > > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c > > > > > index be84b14f0118..c192b80a5166 100644 > > > > > --- a/arch/riscv/errata/thead/errata.c > > > > > +++ b/arch/riscv/errata/thead/errata.c > > > > > @@ -36,21 +36,24 @@ static bool errata_probe_pbmt(unsigned int stage, > > > > > static bool errata_probe_cmo(unsigned int stage, > > > > > unsigned long arch_id, unsigned long impid) > > > > > { > > > > > - if (!IS_ENABLED(CONFIG_ERRATA_THEAD_CMO)) > > > > > - return false; > > > > > - > > > > > - if (arch_id != 0 || impid != 0) > > > > > - return false; > > > > > + bool cmo; > > > > > > > > > > if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) > > > > > return false; > > > > > > > > > > + if (IS_ENABLED(CONFIG_ERRATA_THEAD_CMO) && > > > > > + (arch_id == 0 && impid == 0)) > > > > > + cmo = true; > > > > > + else > > > > > + cmo = false; > > > > > + > > > > > if (stage == RISCV_ALTERNATIVES_BOOT) { > > > > > - riscv_cbom_block_size = L1_CACHE_BYTES; > > > > > - riscv_noncoherent_supported(); > > > > > + if (cmo) > > > > > + riscv_cbom_block_size = L1_CACHE_BYTES; > > > > > + riscv_noncoherent_supported(cmo); > > > > > } > > > > > > > > > > - return true; > > > > > + return cmo; > > > > > > > > I don't really understand the changes that you are making to this > > > > function, so that is tries really hard to call > > > > riscv_noncoherent_supported(). Why do we need to always call the function > > > > in the erratum's probe function, if the erratum is not detected, given > > > > > > In one unified kernel Image, to support both coherent and noncoherent > > > platforms(currently, either T-HEAD CMO or ZICBOM), we need to let the > > > kmalloc meet both cases, specifically, ARCH_DMA_MINALIGN aligned. > > > > seems adding three words can make it better: > > > > kmalloc meet both cases at the beginning, specifically ... > > > > > Once we know the underlying HW is coherent, I.E neither T-HEAD CMO nor > > > ZICBOM, we need to notice kmalloc we are safe to reduce the alignment > > > to 1. The notice action is done in patch 5: > > > > > > + } else { > > > + dma_cache_alignment = 1; > > > > > > > > > > that riscv_noncoherent_supported() is called immediately after > > > > apply_boot_alternatives() in setup_arch()? > > This bit here is the key part of my confusion. You try really hard in > the errata stuff to call riscv_noncoherent_supported(), which I do > understand is because of the other branch that you add to the function > later in the series. > > What I do not understand is why we are not able to rely on the call to > it in setup_arch() to trigger it when we do not have T-HEAD CMOs or > Zicbom. > You've explained why you want to make sure it always gets called during > boot, but my question is about why it looks like it is being called more > than once. > > Actually, now that I think of it, what happens on a T-HEAD system where > there is no T-HEAD CMOs, but there is Zicbom. In theory, this could > exist. > Bear with me here a moment in case I am completely wrong, snippet is > from setup_arch() > apply_boot_alternatives(); > On my example system, this will trigger, eventually sending us into > errata_probe_cmo(), where we will call riscv_noncoherent_supported() > with false, setting dma_cache_alignment to 1. > > if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) && > riscv_isa_extension_available(NULL, ZICBOM)) > cmo = true; > > On this system, this will be true. > > else > cmo = false; > riscv_noncoherent_supported(cmo); > > now riscv_noncoherent_supported() is called with true, and we have > dma_cache_alignment = 1 still. Is that not problematic? Or the inverse, > where the T-HEAD system has its custom CMOs and there is no Zicbom, it > gets called twice with different args too. > Thank you Conor. You pointed out a bug in my series. It looks like we need to defer the dma_cache_alignment modification a bit until T-HEAD CMO probing is done, but we also need to think carefully about T-HEAD related CONFIG option is disabled. I will take care this case in v2 once Catalin's series is merged.
diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c index be84b14f0118..c192b80a5166 100644 --- a/arch/riscv/errata/thead/errata.c +++ b/arch/riscv/errata/thead/errata.c @@ -36,21 +36,24 @@ static bool errata_probe_pbmt(unsigned int stage, static bool errata_probe_cmo(unsigned int stage, unsigned long arch_id, unsigned long impid) { - if (!IS_ENABLED(CONFIG_ERRATA_THEAD_CMO)) - return false; - - if (arch_id != 0 || impid != 0) - return false; + bool cmo; if (stage == RISCV_ALTERNATIVES_EARLY_BOOT) return false; + if (IS_ENABLED(CONFIG_ERRATA_THEAD_CMO) && + (arch_id == 0 && impid == 0)) + cmo = true; + else + cmo = false; + if (stage == RISCV_ALTERNATIVES_BOOT) { - riscv_cbom_block_size = L1_CACHE_BYTES; - riscv_noncoherent_supported(); + if (cmo) + riscv_cbom_block_size = L1_CACHE_BYTES; + riscv_noncoherent_supported(cmo); } - return true; + return cmo; } static bool errata_probe_pmu(unsigned int stage, diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h index 8091b8bf4883..9d056c9b625a 100644 --- a/arch/riscv/include/asm/cacheflush.h +++ b/arch/riscv/include/asm/cacheflush.h @@ -54,9 +54,9 @@ extern unsigned int riscv_cboz_block_size; void riscv_init_cbo_blocksizes(void); #ifdef CONFIG_RISCV_DMA_NONCOHERENT -void riscv_noncoherent_supported(void); +void riscv_noncoherent_supported(bool cmo); #else -static inline void riscv_noncoherent_supported(void) {} +static inline void riscv_noncoherent_supported(bool cmo) {} #endif /* diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c index 36b026057503..565f3e20169b 100644 --- a/arch/riscv/kernel/setup.c +++ b/arch/riscv/kernel/setup.c @@ -264,6 +264,7 @@ static void __init parse_dtb(void) void __init setup_arch(char **cmdline_p) { + bool cmo; parse_dtb(); setup_initial_init_mm(_stext, _etext, _edata, _end); @@ -298,7 +299,10 @@ void __init setup_arch(char **cmdline_p) apply_boot_alternatives(); if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) && riscv_isa_extension_available(NULL, ZICBOM)) - riscv_noncoherent_supported(); + cmo = true; + else + cmo = false; + riscv_noncoherent_supported(cmo); } static int __init topology_init(void) diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c index d51a75864e53..0e172e2b4751 100644 --- a/arch/riscv/mm/dma-noncoherent.c +++ b/arch/riscv/mm/dma-noncoherent.c @@ -72,9 +72,11 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, dev->dma_coherent = coherent; } -void riscv_noncoherent_supported(void) +void riscv_noncoherent_supported(bool cmo) { - WARN(!riscv_cbom_block_size, - "Non-coherent DMA support enabled without a block size\n"); - noncoherent_supported = true; + if (cmo) { + WARN(!riscv_cbom_block_size, + "Non-coherent DMA support enabled without a block size\n"); + noncoherent_supported = true; + } }
We will soon take different actions by checking the HW is noncoherent or not, I.E ZICBOM/ERRATA_THEAD_CMO or not. Signed-off-by: Jisheng Zhang <jszhang@kernel.org> --- arch/riscv/errata/thead/errata.c | 19 +++++++++++-------- arch/riscv/include/asm/cacheflush.h | 4 ++-- arch/riscv/kernel/setup.c | 6 +++++- arch/riscv/mm/dma-noncoherent.c | 10 ++++++---- 4 files changed, 24 insertions(+), 15 deletions(-)