* Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing
2022-08-12 15:40 [PATCH v2] RISC-V: Clean up the Zicbom block size probing Palmer Dabbelt
@ 2022-08-12 17:26 ` Conor.Dooley
2022-08-12 17:43 ` Atish Patra
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Conor.Dooley @ 2022-08-12 17:26 UTC (permalink / raw)
To: palmer, linux-riscv, anup; +Cc: lkp
On 12/08/2022 16:40, Palmer Dabbelt wrote:
> This fixes two issues: I truncated the warning's hart ID when porting to
> the 64-bit hart ID code, and the original code's warning handling could
> fire on an uninitialized hart ID.
>
> The biggest change here is that riscv_cbom_block_size is no longer
> initialized, as IMO the default isn't sane: there's nothing in the ISA
> that mandates any specific cache block size, so falling back to one will
> just silently produce the wrong answer on some systems. This also
> changes the probing order so the cache block size is known before
> enabling Zicbom support.
>
> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
I just manually edited riscv/defconfig to add enable the
zicbom config option, but I get the below with clang-15:
/stuff/linux/arch/riscv/mm/dma-noncoherent.c:104:6: warning: format specifies type 'int' but the argument has type 'unsigned long' [-Wformat]
cbom_hartid, hartid);
^~~~~~~~~~~
/stuff/linux/include/linux/printk.h:517:37: note: expanded from macro 'pr_warn'
printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
/stuff/linux/include/linux/printk.h:464:60: note: expanded from macro 'printk'
#define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
/stuff/linux/include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap'
_p_func(_fmt, ##__VA_ARGS__); \
~~~~ ^~~~~~~~~~~
/stuff/linux/arch/riscv/mm/dma-noncoherent.c:104:6: warning: variable 'cbom_hartid' is uninitialized when used here [-Wuninitialized]
cbom_hartid, hartid);
^~~~~~~~~~~
/stuff/linux/include/linux/printk.h:517:37: note: expanded from macro 'pr_warn'
printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
^~~~~~~~~~~
/stuff/linux/include/linux/printk.h:464:60: note: expanded from macro 'printk'
#define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
^~~~~~~~~~~
/stuff/linux/include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap'
_p_func(_fmt, ##__VA_ARGS__); \
^~~~~~~~~~~
/stuff/linux/arch/riscv/mm/dma-noncoherent.c:87:36: note: initialize the variable 'cbom_hartid' to silence this warning
unsigned long hartid, cbom_hartid;
^
= 0
/stuff/linux/arch/riscv/mm/dma-noncoherent.c:116:10: error: too many arguments provided to function-like macro invocation
"Non-coherent DMA support enabled without a block size\n");
^
/stuff/linux/include/asm-generic/bug.h:121:9: note: macro 'WARN_ON' defined here
#define WARN_ON(condition) ({ \
^
/stuff/linux/arch/riscv/mm/dma-noncoherent.c:115:2: error: use of undeclared identifier 'WARN_ON'
WARN_ON(!riscv_cbom_block_size,
^
2 warnings and 2 errors generated.
make[5]: *** [/stuff/linux/scripts/Makefile.build:250: arch/riscv/mm/dma-noncoherent.o] Error 1
make[4]: *** [/stuff/linux/scripts/Makefile.build:525: arch/riscv/mm] Error 2
>
> ---
>
> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>:
>
> * Everything but the unsigned long cbom_hartid.
> ---
> arch/riscv/kernel/setup.c | 2 +-
> arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++----------
> 2 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 95ef6e2bf45c..2dfc463b86bb 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p)
> setup_smp();
> #endif
>
> - riscv_fill_hwcap();
> riscv_init_cbom_blocksize();
> + riscv_fill_hwcap();
> apply_boot_alternatives();
> }
>
> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> index cd2225304c82..3aa3572715d6 100644
> --- a/arch/riscv/mm/dma-noncoherent.c
> +++ b/arch/riscv/mm/dma-noncoherent.c
> @@ -12,7 +12,7 @@
> #include <linux/of_device.h>
> #include <asm/cacheflush.h>
>
> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
> +static unsigned int riscv_cbom_block_size;
> static bool noncoherent_supported;
>
> void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void)
> {
> struct device_node *node;
> int ret;
> - u32 val;
> + u32 val, probed_block_size;
>
> + probed_block_size = 0;
> for_each_of_cpu_node(node) {
> - unsigned long hartid;
> - int cbom_hartid;
> + unsigned long hartid, cbom_hartid;
>
> ret = riscv_of_processor_hartid(node, &hartid);
> if (ret)
> continue;
>
> - if (hartid < 0)
> - continue;
> -
> /* set block-size for cbom extension if available */
> ret = of_property_read_u32(node, "riscv,cbom-block-size", &val);
> if (ret)
> continue;
>
> - if (!riscv_cbom_block_size) {
> - riscv_cbom_block_size = val;
> + if (!probed_block_size) {
> + probed_block_size = val;
> cbom_hartid = hartid;
> } else {
> - if (riscv_cbom_block_size != val)
> + if (probed_block_size != val)
> pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
> cbom_hartid, hartid);
> }
> }
> +
> + if (probed_block_size)
> + riscv_cbom_block_size = probed_block_size;
> }
> #endif
>
> void riscv_noncoherent_supported(void)
> {
> + WARN_ON(!riscv_cbom_block_size,
> + "Non-coherent DMA support enabled without a block size\n");
> noncoherent_supported = true;
> }
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing
2022-08-12 15:40 [PATCH v2] RISC-V: Clean up the Zicbom block size probing Palmer Dabbelt
2022-08-12 17:26 ` Conor.Dooley
@ 2022-08-12 17:43 ` Atish Patra
2022-08-12 23:07 ` Palmer Dabbelt
2022-08-15 15:40 ` Nathan Chancellor
2022-09-01 15:57 ` Heiko Stübner
3 siblings, 1 reply; 11+ messages in thread
From: Atish Patra @ 2022-08-12 17:43 UTC (permalink / raw)
To: Palmer Dabbelt; +Cc: linux-riscv, anup, kernel test robot
On Fri, Aug 12, 2022 at 9:06 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> This fixes two issues: I truncated the warning's hart ID when porting to
> the 64-bit hart ID code, and the original code's warning handling could
> fire on an uninitialized hart ID.
>
> The biggest change here is that riscv_cbom_block_size is no longer
> initialized, as IMO the default isn't sane: there's nothing in the ISA
> that mandates any specific cache block size, so falling back to one will
> just silently produce the wrong answer on some systems. This also
> changes the probing order so the cache block size is known before
> enabling Zicbom support.
>
> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>
> ---
>
> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>:
>
> * Everything but the unsigned long cbom_hartid.
> ---
> arch/riscv/kernel/setup.c | 2 +-
> arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++----------
> 2 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 95ef6e2bf45c..2dfc463b86bb 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p)
> setup_smp();
> #endif
>
> - riscv_fill_hwcap();
> riscv_init_cbom_blocksize();
> + riscv_fill_hwcap();
> apply_boot_alternatives();
> }
>
> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> index cd2225304c82..3aa3572715d6 100644
> --- a/arch/riscv/mm/dma-noncoherent.c
> +++ b/arch/riscv/mm/dma-noncoherent.c
> @@ -12,7 +12,7 @@
> #include <linux/of_device.h>
> #include <asm/cacheflush.h>
>
> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
> +static unsigned int riscv_cbom_block_size;
What is the expected behavior if the block size is zero in CMO
operations ? As per my understanding, it will be equivalent to a nop.
Let me know if I am wrong.
If that is the case, this is misleading as well. Maybe we should just
disable CMO extension altogether if it can't find the DT property.
> static bool noncoherent_supported;
>
> void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void)
> {
> struct device_node *node;
> int ret;
> - u32 val;
> + u32 val, probed_block_size;
>
> + probed_block_size = 0;
> for_each_of_cpu_node(node) {
> - unsigned long hartid;
> - int cbom_hartid;
> + unsigned long hartid, cbom_hartid;
>
> ret = riscv_of_processor_hartid(node, &hartid);
> if (ret)
> continue;
>
> - if (hartid < 0)
> - continue;
> -
> /* set block-size for cbom extension if available */
> ret = of_property_read_u32(node, "riscv,cbom-block-size", &val);
> if (ret)
> continue;
>
> - if (!riscv_cbom_block_size) {
> - riscv_cbom_block_size = val;
> + if (!probed_block_size) {
> + probed_block_size = val;
> cbom_hartid = hartid;
> } else {
> - if (riscv_cbom_block_size != val)
> + if (probed_block_size != val)
> pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
> cbom_hartid, hartid);
> }
> }
> +
> + if (probed_block_size)
> + riscv_cbom_block_size = probed_block_size;
> }
> #endif
>
> void riscv_noncoherent_supported(void)
> {
> + WARN_ON(!riscv_cbom_block_size,
> + "Non-coherent DMA support enabled without a block size\n");
> noncoherent_supported = true;
> }
> --
> 2.34.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
--
Regards,
Atish
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing
2022-08-12 17:43 ` Atish Patra
@ 2022-08-12 23:07 ` Palmer Dabbelt
2022-08-22 22:02 ` Conor.Dooley
0 siblings, 1 reply; 11+ messages in thread
From: Palmer Dabbelt @ 2022-08-12 23:07 UTC (permalink / raw)
To: atishp; +Cc: linux-riscv, anup, lkp
On Fri, 12 Aug 2022 10:43:02 PDT (-0700), atishp@atishpatra.org wrote:
> On Fri, Aug 12, 2022 at 9:06 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>>
>> This fixes two issues: I truncated the warning's hart ID when porting to
>> the 64-bit hart ID code, and the original code's warning handling could
>> fire on an uninitialized hart ID.
>>
>> The biggest change here is that riscv_cbom_block_size is no longer
>> initialized, as IMO the default isn't sane: there's nothing in the ISA
>> that mandates any specific cache block size, so falling back to one will
>> just silently produce the wrong answer on some systems. This also
>> changes the probing order so the cache block size is known before
>> enabling Zicbom support.
>>
>> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
>> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>>
>> ---
>>
>> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>:
>>
>> * Everything but the unsigned long cbom_hartid.
>> ---
>> arch/riscv/kernel/setup.c | 2 +-
>> arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++----------
>> 2 files changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>> index 95ef6e2bf45c..2dfc463b86bb 100644
>> --- a/arch/riscv/kernel/setup.c
>> +++ b/arch/riscv/kernel/setup.c
>> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p)
>> setup_smp();
>> #endif
>>
>> - riscv_fill_hwcap();
>> riscv_init_cbom_blocksize();
>> + riscv_fill_hwcap();
>> apply_boot_alternatives();
>> }
>>
>> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
>> index cd2225304c82..3aa3572715d6 100644
>> --- a/arch/riscv/mm/dma-noncoherent.c
>> +++ b/arch/riscv/mm/dma-noncoherent.c
>> @@ -12,7 +12,7 @@
>> #include <linux/of_device.h>
>> #include <asm/cacheflush.h>
>>
>> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
>> +static unsigned int riscv_cbom_block_size;
>
> What is the expected behavior if the block size is zero in CMO
> operations ? As per my understanding, it will be equivalent to a nop.
> Let me know if I am wrong.
>
> If that is the case, this is misleading as well. Maybe we should just
> disable CMO extension altogether if it can't find the DT property.
That seems reasonable to me, even if having a 0 block size is allowed by
the spec it seems way more likely to have been a mistake. I'll send a
v3, after puttting together a toolchain that actually builds this
(assuming that's why I'm not getting the failures/warnings).
>
>> static bool noncoherent_supported;
>>
>> void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
>> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void)
>> {
>> struct device_node *node;
>> int ret;
>> - u32 val;
>> + u32 val, probed_block_size;
>>
>> + probed_block_size = 0;
>> for_each_of_cpu_node(node) {
>> - unsigned long hartid;
>> - int cbom_hartid;
>> + unsigned long hartid, cbom_hartid;
>>
>> ret = riscv_of_processor_hartid(node, &hartid);
>> if (ret)
>> continue;
>>
>> - if (hartid < 0)
>> - continue;
>> -
>> /* set block-size for cbom extension if available */
>> ret = of_property_read_u32(node, "riscv,cbom-block-size", &val);
>> if (ret)
>> continue;
>>
>> - if (!riscv_cbom_block_size) {
>> - riscv_cbom_block_size = val;
>> + if (!probed_block_size) {
>> + probed_block_size = val;
>> cbom_hartid = hartid;
>> } else {
>> - if (riscv_cbom_block_size != val)
>> + if (probed_block_size != val)
>> pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
>> cbom_hartid, hartid);
>> }
>> }
>> +
>> + if (probed_block_size)
>> + riscv_cbom_block_size = probed_block_size;
>> }
>> #endif
>>
>> void riscv_noncoherent_supported(void)
>> {
>> + WARN_ON(!riscv_cbom_block_size,
>> + "Non-coherent DMA support enabled without a block size\n");
>> noncoherent_supported = true;
>> }
>> --
>> 2.34.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing
2022-08-12 23:07 ` Palmer Dabbelt
@ 2022-08-22 22:02 ` Conor.Dooley
0 siblings, 0 replies; 11+ messages in thread
From: Conor.Dooley @ 2022-08-22 22:02 UTC (permalink / raw)
To: palmer, atishp; +Cc: linux-riscv, anup, lkp
On 13/08/2022 00:07, Palmer Dabbelt wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Fri, 12 Aug 2022 10:43:02 PDT (-0700), atishp@atishpatra.org wrote:
>> On Fri, Aug 12, 2022 at 9:06 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>>>
>>> This fixes two issues: I truncated the warning's hart ID when porting to
>>> the 64-bit hart ID code, and the original code's warning handling could
>>> fire on an uninitialized hart ID.
>>>
>>> The biggest change here is that riscv_cbom_block_size is no longer
>>> initialized, as IMO the default isn't sane: there's nothing in the ISA
>>> that mandates any specific cache block size, so falling back to one will
>>> just silently produce the wrong answer on some systems. This also
>>> changes the probing order so the cache block size is known before
>>> enabling Zicbom support.
>>>
>>> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
>>> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>>>
>>> ---
>>>
>>> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>:
>>>
>>> * Everything but the unsigned long cbom_hartid.
>>> ---
>>> arch/riscv/kernel/setup.c | 2 +-
>>> arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++----------
>>> 2 files changed, 13 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>>> index 95ef6e2bf45c..2dfc463b86bb 100644
>>> --- a/arch/riscv/kernel/setup.c
>>> +++ b/arch/riscv/kernel/setup.c
>>> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p)
>>> setup_smp();
>>> #endif
>>>
>>> - riscv_fill_hwcap();
>>> riscv_init_cbom_blocksize();
>>> + riscv_fill_hwcap();
>>> apply_boot_alternatives();
>>> }
>>>
>>> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
>>> index cd2225304c82..3aa3572715d6 100644
>>> --- a/arch/riscv/mm/dma-noncoherent.c
>>> +++ b/arch/riscv/mm/dma-noncoherent.c
>>> @@ -12,7 +12,7 @@
>>> #include <linux/of_device.h>
>>> #include <asm/cacheflush.h>
>>>
>>> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
>>> +static unsigned int riscv_cbom_block_size;
>>
>> What is the expected behavior if the block size is zero in CMO
>> operations ? As per my understanding, it will be equivalent to a nop.
>> Let me know if I am wrong.
>>
>> If that is the case, this is misleading as well. Maybe we should just
>> disable CMO extension altogether if it can't find the DT property.
>
> That seems reasonable to me, even if having a 0 block size is allowed by
> the spec it seems way more likely to have been a mistake. I'll send a
> v3, after puttting together a toolchain that actually builds this
> (assuming that's why I'm not getting the failures/warnings).
What's the plan here with v3?
I folded the following into this version locally to clear both the original
warning & the build error with this patch on clang-15:
diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
index 3aa3572715d6..8a49ea5ba01d 100644
--- a/arch/riscv/mm/dma-noncoherent.c
+++ b/arch/riscv/mm/dma-noncoherent.c
@@ -79,12 +79,13 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
void riscv_init_cbom_blocksize(void)
{
struct device_node *node;
- int ret;
+ unsigned long cbom_hartid;
u32 val, probed_block_size;
+ int ret;
probed_block_size = 0;
for_each_of_cpu_node(node) {
- unsigned long hartid, cbom_hartid;
+ unsigned long hartid;
ret = riscv_of_processor_hartid(node, &hartid);
if (ret)
@@ -100,7 +101,7 @@ void riscv_init_cbom_blocksize(void)
cbom_hartid = hartid;
} else {
if (probed_block_size != val)
- pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
+ pr_warn("cbom-block-size mismatched between harts %lu and %lu\n",
cbom_hartid, hartid);
}
}
@@ -112,7 +113,7 @@ void riscv_init_cbom_blocksize(void)
void riscv_noncoherent_supported(void)
{
- WARN_ON(!riscv_cbom_block_size,
- "Non-coherent DMA support enabled without a block size\n");
+ WARN(!riscv_cbom_block_size,
+ "Non-coherent DMA support enabled without a block size\n");
noncoherent_supported = true;
}
>
>>
>>> static bool noncoherent_supported;
>>>
>>> void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
>>> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void)
>>> {
>>> struct device_node *node;
>>> int ret;
>>> - u32 val;
>>> + u32 val, probed_block_size;
>>>
>>> + probed_block_size = 0;
>>> for_each_of_cpu_node(node) {
>>> - unsigned long hartid;
>>> - int cbom_hartid;
>>> + unsigned long hartid, cbom_hartid;
>>>
>>> ret = riscv_of_processor_hartid(node, &hartid);
>>> if (ret)
>>> continue;
>>>
>>> - if (hartid < 0)
>>> - continue;
>>> -
>>> /* set block-size for cbom extension if available */
>>> ret = of_property_read_u32(node, "riscv,cbom-block-size", &val);
>>> if (ret)
>>> continue;
>>>
>>> - if (!riscv_cbom_block_size) {
>>> - riscv_cbom_block_size = val;
>>> + if (!probed_block_size) {
>>> + probed_block_size = val;
>>> cbom_hartid = hartid;
>>> } else {
>>> - if (riscv_cbom_block_size != val)
>>> + if (probed_block_size != val)
>>> pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
>>> cbom_hartid, hartid);
>>> }
>>> }
>>> +
>>> + if (probed_block_size)
>>> + riscv_cbom_block_size = probed_block_size;
>>> }
>>> #endif
>>>
>>> void riscv_noncoherent_supported(void)
>>> {
>>> + WARN_ON(!riscv_cbom_block_size,
>>> + "Non-coherent DMA support enabled without a block size\n");
>>> noncoherent_supported = true;
>>> }
>>> --
>>> 2.34.1
>>>
>>>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing
2022-08-12 15:40 [PATCH v2] RISC-V: Clean up the Zicbom block size probing Palmer Dabbelt
2022-08-12 17:26 ` Conor.Dooley
2022-08-12 17:43 ` Atish Patra
@ 2022-08-15 15:40 ` Nathan Chancellor
2022-08-15 17:36 ` Jessica Clarke
2022-09-01 15:57 ` Heiko Stübner
3 siblings, 1 reply; 11+ messages in thread
From: Nathan Chancellor @ 2022-08-15 15:40 UTC (permalink / raw)
To: Palmer Dabbelt; +Cc: linux-riscv, anup, kernel test robot
Hi Palmer,
On Fri, Aug 12, 2022 at 08:40:10AM -0700, Palmer Dabbelt wrote:
> This fixes two issues: I truncated the warning's hart ID when porting to
> the 64-bit hart ID code, and the original code's warning handling could
> fire on an uninitialized hart ID.
>
> The biggest change here is that riscv_cbom_block_size is no longer
> initialized, as IMO the default isn't sane: there's nothing in the ISA
> that mandates any specific cache block size, so falling back to one will
> just silently produce the wrong answer on some systems. This also
> changes the probing order so the cache block size is known before
> enabling Zicbom support.
>
> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>
> ---
>
> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>:
>
> * Everything but the unsigned long cbom_hartid.
> ---
> arch/riscv/kernel/setup.c | 2 +-
> arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++----------
> 2 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 95ef6e2bf45c..2dfc463b86bb 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p)
> setup_smp();
> #endif
>
> - riscv_fill_hwcap();
> riscv_init_cbom_blocksize();
> + riscv_fill_hwcap();
> apply_boot_alternatives();
> }
>
> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> index cd2225304c82..3aa3572715d6 100644
> --- a/arch/riscv/mm/dma-noncoherent.c
> +++ b/arch/riscv/mm/dma-noncoherent.c
> @@ -12,7 +12,7 @@
> #include <linux/of_device.h>
> #include <asm/cacheflush.h>
>
> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
> +static unsigned int riscv_cbom_block_size;
> static bool noncoherent_supported;
>
> void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void)
> {
> struct device_node *node;
> int ret;
> - u32 val;
> + u32 val, probed_block_size;
>
> + probed_block_size = 0;
> for_each_of_cpu_node(node) {
> - unsigned long hartid;
> - int cbom_hartid;
> + unsigned long hartid, cbom_hartid;
>
> ret = riscv_of_processor_hartid(node, &hartid);
> if (ret)
> continue;
>
> - if (hartid < 0)
> - continue;
> -
> /* set block-size for cbom extension if available */
> ret = of_property_read_u32(node, "riscv,cbom-block-size", &val);
> if (ret)
> continue;
>
> - if (!riscv_cbom_block_size) {
> - riscv_cbom_block_size = val;
> + if (!probed_block_size) {
> + probed_block_size = val;
> cbom_hartid = hartid;
> } else {
> - if (riscv_cbom_block_size != val)
> + if (probed_block_size != val)
> pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
^ %lu?
> cbom_hartid, hartid);
> }
> }
> +
> + if (probed_block_size)
> + riscv_cbom_block_size = probed_block_size;
> }
> #endif
>
> void riscv_noncoherent_supported(void)
> {
> + WARN_ON(!riscv_cbom_block_size,
> + "Non-coherent DMA support enabled without a block size\n");
> noncoherent_supported = true;
> }
> --
> 2.34.1
For what it's worth, while this should address the uninitialized
cbom_hartid at runtime (from the quick glance I gave it), it doesn't
address the compile time warning. I am not sure how to make it clear to
clang that the if statement will be executed during the first loop
iteration because probed_block_size is initialized to zero...
Additionally, it appears that WARN() is the right macro, not WARN_ON()
and an '#include <asm/bug.h>' is needed.
arch/riscv/mm/dma-noncoherent.c:104:6: error: variable 'cbom_hartid' is uninitialized when used here [-Werror,-Wuninitialized]
cbom_hartid, hartid);
^~~~~~~~~~~
include/linux/printk.h:517:37: note: expanded from macro 'pr_warn'
printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
^~~~~~~~~~~
include/linux/printk.h:464:60: note: expanded from macro 'printk'
#define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
^~~~~~~~~~~
include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap'
_p_func(_fmt, ##__VA_ARGS__); \
^~~~~~~~~~~
arch/riscv/mm/dma-noncoherent.c:87:36: note: initialize the variable 'cbom_hartid' to silence this warning
unsigned long hartid, cbom_hartid;
^
= 0
arch/riscv/mm/dma-noncoherent.c:116:10: error: too many arguments provided to function-like macro invocation
"Non-coherent DMA support enabled without a block size\n");
^
include/asm-generic/bug.h:121:9: note: macro 'WARN_ON' defined here
#define WARN_ON(condition) ({ \
^
arch/riscv/mm/dma-noncoherent.c:115:2: error: use of undeclared identifier 'WARN_ON'
WARN_ON(!riscv_cbom_block_size,
^
3 errors generated.
Cheers,
Nathan
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing
2022-08-15 15:40 ` Nathan Chancellor
@ 2022-08-15 17:36 ` Jessica Clarke
2022-08-15 18:16 ` Nathan Chancellor
0 siblings, 1 reply; 11+ messages in thread
From: Jessica Clarke @ 2022-08-15 17:36 UTC (permalink / raw)
To: Nathan Chancellor; +Cc: Palmer Dabbelt, linux-riscv, anup, kernel test robot
> On 15 Aug 2022, at 16:40, Nathan Chancellor <nathan@kernel.org> wrote:
>
> Hi Palmer,
>
> On Fri, Aug 12, 2022 at 08:40:10AM -0700, Palmer Dabbelt wrote:
>> This fixes two issues: I truncated the warning's hart ID when porting to
>> the 64-bit hart ID code, and the original code's warning handling could
>> fire on an uninitialized hart ID.
>>
>> The biggest change here is that riscv_cbom_block_size is no longer
>> initialized, as IMO the default isn't sane: there's nothing in the ISA
>> that mandates any specific cache block size, so falling back to one will
>> just silently produce the wrong answer on some systems. This also
>> changes the probing order so the cache block size is known before
>> enabling Zicbom support.
>>
>> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
>> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>>
>> ---
>>
>> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>:
>>
>> * Everything but the unsigned long cbom_hartid.
>> ---
>> arch/riscv/kernel/setup.c | 2 +-
>> arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++----------
>> 2 files changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>> index 95ef6e2bf45c..2dfc463b86bb 100644
>> --- a/arch/riscv/kernel/setup.c
>> +++ b/arch/riscv/kernel/setup.c
>> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p)
>> setup_smp();
>> #endif
>>
>> - riscv_fill_hwcap();
>> riscv_init_cbom_blocksize();
>> + riscv_fill_hwcap();
>> apply_boot_alternatives();
>> }
>>
>> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
>> index cd2225304c82..3aa3572715d6 100644
>> --- a/arch/riscv/mm/dma-noncoherent.c
>> +++ b/arch/riscv/mm/dma-noncoherent.c
>> @@ -12,7 +12,7 @@
>> #include <linux/of_device.h>
>> #include <asm/cacheflush.h>
>>
>> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
>> +static unsigned int riscv_cbom_block_size;
>> static bool noncoherent_supported;
>>
>> void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
>> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void)
>> {
>> struct device_node *node;
>> int ret;
>> - u32 val;
>> + u32 val, probed_block_size;
>>
>> + probed_block_size = 0;
>> for_each_of_cpu_node(node) {
>> - unsigned long hartid;
>> - int cbom_hartid;
>> + unsigned long hartid, cbom_hartid;
>>
>> ret = riscv_of_processor_hartid(node, &hartid);
>> if (ret)
>> continue;
>>
>> - if (hartid < 0)
>> - continue;
>> -
>> /* set block-size for cbom extension if available */
>> ret = of_property_read_u32(node, "riscv,cbom-block-size", &val);
>> if (ret)
>> continue;
>>
>> - if (!riscv_cbom_block_size) {
>> - riscv_cbom_block_size = val;
>> + if (!probed_block_size) {
>> + probed_block_size = val;
>> cbom_hartid = hartid;
>> } else {
>> - if (riscv_cbom_block_size != val)
>> + if (probed_block_size != val)
>> pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
>
> ^ %lu?
>
>> cbom_hartid, hartid);
>> }
>> }
>> +
>> + if (probed_block_size)
>> + riscv_cbom_block_size = probed_block_size;
>> }
>> #endif
>>
>> void riscv_noncoherent_supported(void)
>> {
>> + WARN_ON(!riscv_cbom_block_size,
>> + "Non-coherent DMA support enabled without a block size\n");
>> noncoherent_supported = true;
>> }
>> --
>> 2.34.1
>
> For what it's worth, while this should address the uninitialized
> cbom_hartid at runtime (from the quick glance I gave it), it doesn't
> address the compile time warning. I am not sure how to make it clear to
> clang that the if statement will be executed during the first loop
> iteration because probed_block_size is initialized to zero...
The warnings are correct; the variables are declared inside the body,
as I pointed out on IRC when people were discussing the function.
Jess
> Additionally, it appears that WARN() is the right macro, not WARN_ON()
> and an '#include <asm/bug.h>' is needed.
>
> arch/riscv/mm/dma-noncoherent.c:104:6: error: variable 'cbom_hartid' is uninitialized when used here [-Werror,-Wuninitialized]
> cbom_hartid, hartid);
> ^~~~~~~~~~~
> include/linux/printk.h:517:37: note: expanded from macro 'pr_warn'
> printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
> ^~~~~~~~~~~
> include/linux/printk.h:464:60: note: expanded from macro 'printk'
> #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
> ^~~~~~~~~~~
> include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap'
> _p_func(_fmt, ##__VA_ARGS__); \
> ^~~~~~~~~~~
> arch/riscv/mm/dma-noncoherent.c:87:36: note: initialize the variable 'cbom_hartid' to silence this warning
> unsigned long hartid, cbom_hartid;
> ^
> = 0
> arch/riscv/mm/dma-noncoherent.c:116:10: error: too many arguments provided to function-like macro invocation
> "Non-coherent DMA support enabled without a block size\n");
> ^
> include/asm-generic/bug.h:121:9: note: macro 'WARN_ON' defined here
> #define WARN_ON(condition) ({ \
> ^
> arch/riscv/mm/dma-noncoherent.c:115:2: error: use of undeclared identifier 'WARN_ON'
> WARN_ON(!riscv_cbom_block_size,
> ^
> 3 errors generated.
>
> Cheers,
> Nathan
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing
2022-08-15 17:36 ` Jessica Clarke
@ 2022-08-15 18:16 ` Nathan Chancellor
0 siblings, 0 replies; 11+ messages in thread
From: Nathan Chancellor @ 2022-08-15 18:16 UTC (permalink / raw)
To: Jessica Clarke; +Cc: Palmer Dabbelt, linux-riscv, anup, kernel test robot
On Mon, Aug 15, 2022 at 06:36:14PM +0100, Jessica Clarke wrote:
> > On 15 Aug 2022, at 16:40, Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > Hi Palmer,
> >
> > On Fri, Aug 12, 2022 at 08:40:10AM -0700, Palmer Dabbelt wrote:
> >> This fixes two issues: I truncated the warning's hart ID when porting to
> >> the 64-bit hart ID code, and the original code's warning handling could
> >> fire on an uninitialized hart ID.
> >>
> >> The biggest change here is that riscv_cbom_block_size is no longer
> >> initialized, as IMO the default isn't sane: there's nothing in the ISA
> >> that mandates any specific cache block size, so falling back to one will
> >> just silently produce the wrong answer on some systems. This also
> >> changes the probing order so the cache block size is known before
> >> enabling Zicbom support.
> >>
> >> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
> >> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> >>
> >> ---
> >>
> >> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>:
> >>
> >> * Everything but the unsigned long cbom_hartid.
> >> ---
> >> arch/riscv/kernel/setup.c | 2 +-
> >> arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++----------
> >> 2 files changed, 13 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> >> index 95ef6e2bf45c..2dfc463b86bb 100644
> >> --- a/arch/riscv/kernel/setup.c
> >> +++ b/arch/riscv/kernel/setup.c
> >> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p)
> >> setup_smp();
> >> #endif
> >>
> >> - riscv_fill_hwcap();
> >> riscv_init_cbom_blocksize();
> >> + riscv_fill_hwcap();
> >> apply_boot_alternatives();
> >> }
> >>
> >> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> >> index cd2225304c82..3aa3572715d6 100644
> >> --- a/arch/riscv/mm/dma-noncoherent.c
> >> +++ b/arch/riscv/mm/dma-noncoherent.c
> >> @@ -12,7 +12,7 @@
> >> #include <linux/of_device.h>
> >> #include <asm/cacheflush.h>
> >>
> >> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
> >> +static unsigned int riscv_cbom_block_size;
> >> static bool noncoherent_supported;
> >>
> >> void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
> >> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void)
> >> {
> >> struct device_node *node;
> >> int ret;
> >> - u32 val;
> >> + u32 val, probed_block_size;
> >>
> >> + probed_block_size = 0;
> >> for_each_of_cpu_node(node) {
> >> - unsigned long hartid;
> >> - int cbom_hartid;
> >> + unsigned long hartid, cbom_hartid;
> >>
> >> ret = riscv_of_processor_hartid(node, &hartid);
> >> if (ret)
> >> continue;
> >>
> >> - if (hartid < 0)
> >> - continue;
> >> -
> >> /* set block-size for cbom extension if available */
> >> ret = of_property_read_u32(node, "riscv,cbom-block-size", &val);
> >> if (ret)
> >> continue;
> >>
> >> - if (!riscv_cbom_block_size) {
> >> - riscv_cbom_block_size = val;
> >> + if (!probed_block_size) {
> >> + probed_block_size = val;
> >> cbom_hartid = hartid;
> >> } else {
> >> - if (riscv_cbom_block_size != val)
> >> + if (probed_block_size != val)
> >> pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
> >
> > ^ %lu?
> >
> >> cbom_hartid, hartid);
> >> }
> >> }
> >> +
> >> + if (probed_block_size)
> >> + riscv_cbom_block_size = probed_block_size;
> >> }
> >> #endif
> >>
> >> void riscv_noncoherent_supported(void)
> >> {
> >> + WARN_ON(!riscv_cbom_block_size,
> >> + "Non-coherent DMA support enabled without a block size\n");
> >> noncoherent_supported = true;
> >> }
> >> --
> >> 2.34.1
> >
> > For what it's worth, while this should address the uninitialized
> > cbom_hartid at runtime (from the quick glance I gave it), it doesn't
> > address the compile time warning. I am not sure how to make it clear to
> > clang that the if statement will be executed during the first loop
> > iteration because probed_block_size is initialized to zero...
>
> The warnings are correct; the variables are declared inside the body,
> as I pointed out on IRC when people were discussing the function.
Ugh, I don't know how I missed that :/ guess that's what I get for
replying to emails before I am fully awake...
Cheers,
Nathan
> > Additionally, it appears that WARN() is the right macro, not WARN_ON()
> > and an '#include <asm/bug.h>' is needed.
> >
> > arch/riscv/mm/dma-noncoherent.c:104:6: error: variable 'cbom_hartid' is uninitialized when used here [-Werror,-Wuninitialized]
> > cbom_hartid, hartid);
> > ^~~~~~~~~~~
> > include/linux/printk.h:517:37: note: expanded from macro 'pr_warn'
> > printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
> > ^~~~~~~~~~~
> > include/linux/printk.h:464:60: note: expanded from macro 'printk'
> > #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
> > ^~~~~~~~~~~
> > include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap'
> > _p_func(_fmt, ##__VA_ARGS__); \
> > ^~~~~~~~~~~
> > arch/riscv/mm/dma-noncoherent.c:87:36: note: initialize the variable 'cbom_hartid' to silence this warning
> > unsigned long hartid, cbom_hartid;
> > ^
> > = 0
> > arch/riscv/mm/dma-noncoherent.c:116:10: error: too many arguments provided to function-like macro invocation
> > "Non-coherent DMA support enabled without a block size\n");
> > ^
> > include/asm-generic/bug.h:121:9: note: macro 'WARN_ON' defined here
> > #define WARN_ON(condition) ({ \
> > ^
> > arch/riscv/mm/dma-noncoherent.c:115:2: error: use of undeclared identifier 'WARN_ON'
> > WARN_ON(!riscv_cbom_block_size,
> > ^
> > 3 errors generated.
> >
> > Cheers,
> > Nathan
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing
2022-08-12 15:40 [PATCH v2] RISC-V: Clean up the Zicbom block size probing Palmer Dabbelt
` (2 preceding siblings ...)
2022-08-15 15:40 ` Nathan Chancellor
@ 2022-09-01 15:57 ` Heiko Stübner
2022-09-02 9:55 ` Conor.Dooley
3 siblings, 1 reply; 11+ messages in thread
From: Heiko Stübner @ 2022-09-01 15:57 UTC (permalink / raw)
To: linux-riscv, anup
Cc: Palmer Dabbelt, kernel test robot, Palmer Dabbelt, conor.dooley
Am Freitag, 12. August 2022, 17:40:10 CEST schrieb Palmer Dabbelt:
> This fixes two issues: I truncated the warning's hart ID when porting to
> the 64-bit hart ID code, and the original code's warning handling could
> fire on an uninitialized hart ID.
>
> The biggest change here is that riscv_cbom_block_size is no longer
> initialized, as IMO the default isn't sane: there's nothing in the ISA
> that mandates any specific cache block size, so falling back to one will
> just silently produce the wrong answer on some systems. This also
> changes the probing order so the cache block size is known before
> enabling Zicbom support.
>
> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
With Conor's fixes folded in, this compiles and breaks the T-Head CMOs
as they rely on that default value :-) .
Can we do the following:
(1) pick Anup's patch moving the block-size init over to cacheflush [0]
(2) apply this patch (with Conor's fixes and adapted to the changed
location) and add this one additional line:
diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
index d4b1526538ad..67fa078f303f 100644
--- a/arch/riscv/errata/thead/errata.c
+++ b/arch/riscv/errata/thead/errata.c
@@ -42,6 +42,7 @@ static bool errata_probe_cmo(unsigned int stage,
if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
return false;
+ riscv_cbom_block_size = L1_CACHE_BYTES;
riscv_noncoherent_supported();
return true;
}
With that done everything works (again) and looks great, so would be
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Heiko
[0] https://lore.kernel.org/r/20220830044642.566769-3-apatel@ventanamicro.com
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing
2022-09-01 15:57 ` Heiko Stübner
@ 2022-09-02 9:55 ` Conor.Dooley
2022-09-06 6:08 ` Andrew Jones
0 siblings, 1 reply; 11+ messages in thread
From: Conor.Dooley @ 2022-09-02 9:55 UTC (permalink / raw)
To: heiko, linux-riscv, anup; +Cc: palmer, lkp
On 01/09/2022 16:57, Heiko Stübner wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> Am Freitag, 12. August 2022, 17:40:10 CEST schrieb Palmer Dabbelt:
>> This fixes two issues: I truncated the warning's hart ID when porting to
>> the 64-bit hart ID code, and the original code's warning handling could
>> fire on an uninitialized hart ID.
>>
>> The biggest change here is that riscv_cbom_block_size is no longer
>> initialized, as IMO the default isn't sane: there's nothing in the ISA
>> that mandates any specific cache block size, so falling back to one will
>> just silently produce the wrong answer on some systems. This also
>> changes the probing order so the cache block size is known before
>> enabling Zicbom support.
>>
>> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
>> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>
> With Conor's fixes folded in, this compiles and breaks the T-Head CMOs
> as they rely on that default value :-) .
>
> Can we do the following:
>
> (1) pick Anup's patch moving the block-size init over to cacheflush [0]
> (2) apply this patch (with Conor's fixes and adapted to the changed
> location) and add this one additional line:
>
> diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
> index d4b1526538ad..67fa078f303f 100644
> --- a/arch/riscv/errata/thead/errata.c
> +++ b/arch/riscv/errata/thead/errata.c
> @@ -42,6 +42,7 @@ static bool errata_probe_cmo(unsigned int stage,
> if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
> return false;
>
> + riscv_cbom_block_size = L1_CACHE_BYTES;
> riscv_noncoherent_supported();
> return true;
> }
>
>
> With that done everything works (again) and looks great, so would be
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
With all of the above, it'd also be:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>
>
> Heiko
>
> [0] https://lore.kernel.org/r/20220830044642.566769-3-apatel@ventanamicro.com
>
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing
2022-09-02 9:55 ` Conor.Dooley
@ 2022-09-06 6:08 ` Andrew Jones
0 siblings, 0 replies; 11+ messages in thread
From: Andrew Jones @ 2022-09-06 6:08 UTC (permalink / raw)
To: Conor.Dooley; +Cc: heiko, linux-riscv, anup, palmer, lkp
On Fri, Sep 02, 2022 at 09:55:27AM +0000, Conor.Dooley@microchip.com wrote:
> On 01/09/2022 16:57, Heiko Stübner wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > Am Freitag, 12. August 2022, 17:40:10 CEST schrieb Palmer Dabbelt:
> >> This fixes two issues: I truncated the warning's hart ID when porting to
> >> the 64-bit hart ID code, and the original code's warning handling could
> >> fire on an uninitialized hart ID.
> >>
> >> The biggest change here is that riscv_cbom_block_size is no longer
> >> initialized, as IMO the default isn't sane: there's nothing in the ISA
> >> that mandates any specific cache block size, so falling back to one will
> >> just silently produce the wrong answer on some systems. This also
> >> changes the probing order so the cache block size is known before
> >> enabling Zicbom support.
> >>
> >> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
> >> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> >
> > With Conor's fixes folded in, this compiles and breaks the T-Head CMOs
> > as they rely on that default value :-) .
> >
> > Can we do the following:
> >
> > (1) pick Anup's patch moving the block-size init over to cacheflush [0]
> > (2) apply this patch (with Conor's fixes and adapted to the changed
> > location) and add this one additional line:
> >
> > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
> > index d4b1526538ad..67fa078f303f 100644
> > --- a/arch/riscv/errata/thead/errata.c
> > +++ b/arch/riscv/errata/thead/errata.c
> > @@ -42,6 +42,7 @@ static bool errata_probe_cmo(unsigned int stage,
> > if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
> > return false;
> >
> > + riscv_cbom_block_size = L1_CACHE_BYTES;
> > riscv_noncoherent_supported();
> > return true;
> > }
> >
> >
> > With that done everything works (again) and looks great, so would be
> > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > Tested-by: Heiko Stuebner <heiko@sntech.de>
>
> With all of the above, it'd also be:
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
I needed these fixes for a patch series enabling KVM guests to use Zicbom
so I've gone ahead and made Conor's and Heiko's changes [1]. I rebased
Anup's moving patch on top of that [2]. I'll go ahead and post this patch
separately in case that makes things easier.
[1] https://github.com/jones-drew/linux/commit/af361283ec3129846307f787a3ebb19bd4a9c421
[2] https://github.com/jones-drew/linux/commit/61c404299c63c8706c129d2a67071f5aae94594f
(I forgot Conor's r-b on this patch in the branch, but I'll pick it up now
while posting.)
Thanks,
drew
>
> >
> >
> > Heiko
> >
> > [0] https://lore.kernel.org/r/20220830044642.566769-3-apatel@ventanamicro.com
> >
> >
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 11+ messages in thread