diff mbox series

[v3,3/3] RISC-V: Ensure Zicbom has a valid block size

Message ID 20221129143447.49714-4-ajones@ventanamicro.com (mailing list archive)
State Accepted
Commit 68dc0718407d8e9c84c530df5298078a57a8000a
Delegated to: Palmer Dabbelt
Headers show
Series RISC-V: Ensure Zicbom has a valid block size | expand

Checks

Context Check Description
conchuod/patch_count success Link
conchuod/cover_letter success Series has a cover letter
conchuod/tree_selection success Guessed tree name to be for-next
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/module_param success Was 0 now: 0
conchuod/build_rv32_defconfig success Build OK
conchuod/build_warn_rv64 success Errors and warnings before: 0 this patch: 0
conchuod/dtb_warn_rv64 success Errors and warnings before: 0 this patch: 0
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch success total: 0 errors, 0 warnings, 0 checks, 25 lines checked
conchuod/source_inline success Was 0 now: 0
conchuod/build_rv64_nommu_k210_defconfig fail Build failed
conchuod/verify_fixes success No Fixes tag
conchuod/build_rv64_nommu_virt_defconfig fail Build failed

Commit Message

Andrew Jones Nov. 29, 2022, 2:34 p.m. UTC
When a DT puts zicbom in the isa string, but does not provide a block
size, ALT_CMO_OP() will attempt to do cache operations on address
zero since the start address will be ANDed with zero. We can't simply
BUG() in riscv_init_cbom_blocksize() when we fail to find a block
size because the failure will happen before logging works, leaving
users to scratch their heads as to why the boot hung. Instead, ensure
Zicbom is disabled and output an error which will hopefully alert
people that the DT needs to be fixed. While at it, add a check that
the block size is a power-of-2 too.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/riscv/kernel/cpufeature.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Conor Dooley Nov. 29, 2022, 7:45 p.m. UTC | #1
Hey Drew,

On Tue, Nov 29, 2022 at 03:34:47PM +0100, Andrew Jones wrote:
> When a DT puts zicbom in the isa string, but does not provide a block
> size, ALT_CMO_OP() will attempt to do cache operations on address
> zero since the start address will be ANDed with zero. We can't simply
> BUG() in riscv_init_cbom_blocksize() when we fail to find a block
> size because the failure will happen before logging works, leaving
> users to scratch their heads as to why the boot hung. Instead, ensure
> Zicbom is disabled and output an error which will hopefully alert
> people that the DT needs to be fixed. While at it, add a check that
> the block size is a power-of-2 too.
> 
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

This seems to be failing on nommu :/ I've got host machines issues so I
could not reproduce it for you lcoally and paste an actual log, but if
you build rv64_nommu_virt_defconfig I think you should be able to
reproduce.

Thanks,
Conor.

> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  arch/riscv/kernel/cpufeature.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 220be7222129..93e45560af30 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -9,6 +9,7 @@
>  #include <linux/bitmap.h>
>  #include <linux/ctype.h>
>  #include <linux/libfdt.h>
> +#include <linux/log2.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <asm/alternative.h>
> @@ -70,6 +71,18 @@ EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
>  
>  static bool riscv_isa_extension_check(int id)
>  {
> +	switch (id) {
> +	case RISCV_ISA_EXT_ZICBOM:
> +		if (!riscv_cbom_block_size) {
> +			pr_err("Zicbom detected in ISA string, but no cbom-block-size found\n");
> +			return false;
> +		} else if (!is_power_of_2(riscv_cbom_block_size)) {
> +			pr_err("cbom-block-size present, but is not a power-of-2\n");
> +			return false;
> +		}
> +		return true;
> +	}
> +
>  	return true;
>  }
>  
> -- 
> 2.38.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Conor Dooley Nov. 30, 2022, 9:46 a.m. UTC | #2
On 29/11/2022 19:45, Conor Dooley wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hey Drew,
> 
> On Tue, Nov 29, 2022 at 03:34:47PM +0100, Andrew Jones wrote:
>> When a DT puts zicbom in the isa string, but does not provide a block
>> size, ALT_CMO_OP() will attempt to do cache operations on address
>> zero since the start address will be ANDed with zero. We can't simply
>> BUG() in riscv_init_cbom_blocksize() when we fail to find a block
>> size because the failure will happen before logging works, leaving
>> users to scratch their heads as to why the boot hung. Instead, ensure
>> Zicbom is disabled and output an error which will hopefully alert
>> people that the DT needs to be fixed. While at it, add a check that
>> the block size is a power-of-2 too.
>>
>> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> 
> This seems to be failing on nommu :/ I've got host machines issues so I
> could not reproduce it for you lcoally and paste an actual log, but if
> you build rv64_nommu_virt_defconfig I think you should be able to
> reproduce.

The actual error is:
riscv64-unknown-linux-gnu-ld: arch/riscv/kernel/cpufeature.o: in function `.L0 ':
cpufeature.c:(.text.unlikely+0x8): undefined reference to `riscv_cbom_block_size'


> 
> Thanks,
> Conor.
> 
>> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
>> ---
>>   arch/riscv/kernel/cpufeature.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index 220be7222129..93e45560af30 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -9,6 +9,7 @@
>>   #include <linux/bitmap.h>
>>   #include <linux/ctype.h>
>>   #include <linux/libfdt.h>
>> +#include <linux/log2.h>
>>   #include <linux/module.h>
>>   #include <linux/of.h>
>>   #include <asm/alternative.h>
>> @@ -70,6 +71,18 @@ EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
>>
>>   static bool riscv_isa_extension_check(int id)
>>   {
>> +     switch (id) {
>> +     case RISCV_ISA_EXT_ZICBOM:
>> +             if (!riscv_cbom_block_size) {
>> +                     pr_err("Zicbom detected in ISA string, but no cbom-block-size found\n");
>> +                     return false;
>> +             } else if (!is_power_of_2(riscv_cbom_block_size)) {
>> +                     pr_err("cbom-block-size present, but is not a power-of-2\n");
>> +                     return false;
>> +             }
>> +             return true;
>> +     }
>> +
>>        return true;
>>   }
>>
>> --
>> 2.38.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
Andrew Jones Nov. 30, 2022, 11:33 a.m. UTC | #3
On Wed, Nov 30, 2022 at 09:46:20AM +0000, Conor.Dooley@microchip.com wrote:
> On 29/11/2022 19:45, Conor Dooley wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Hey Drew,
> > 
> > On Tue, Nov 29, 2022 at 03:34:47PM +0100, Andrew Jones wrote:
> >> When a DT puts zicbom in the isa string, but does not provide a block
> >> size, ALT_CMO_OP() will attempt to do cache operations on address
> >> zero since the start address will be ANDed with zero. We can't simply
> >> BUG() in riscv_init_cbom_blocksize() when we fail to find a block
> >> size because the failure will happen before logging works, leaving
> >> users to scratch their heads as to why the boot hung. Instead, ensure
> >> Zicbom is disabled and output an error which will hopefully alert
> >> people that the DT needs to be fixed. While at it, add a check that
> >> the block size is a power-of-2 too.
> >>
> >> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > 
> > This seems to be failing on nommu :/ I've got host machines issues so I
> > could not reproduce it for you lcoally and paste an actual log, but if
> > you build rv64_nommu_virt_defconfig I think you should be able to
> > reproduce.
> 
> The actual error is:
> riscv64-unknown-linux-gnu-ld: arch/riscv/kernel/cpufeature.o: in function `.L0 ':
> cpufeature.c:(.text.unlikely+0x8): undefined reference to `riscv_cbom_block_size'

Thanks Conor,

I'll try to get this fixed and send a v4 ASAP.

drew

> 
> 
> > 
> > Thanks,
> > Conor.
> > 
> >> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> >> ---
> >>   arch/riscv/kernel/cpufeature.c | 13 +++++++++++++
> >>   1 file changed, 13 insertions(+)
> >>
> >> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> >> index 220be7222129..93e45560af30 100644
> >> --- a/arch/riscv/kernel/cpufeature.c
> >> +++ b/arch/riscv/kernel/cpufeature.c
> >> @@ -9,6 +9,7 @@
> >>   #include <linux/bitmap.h>
> >>   #include <linux/ctype.h>
> >>   #include <linux/libfdt.h>
> >> +#include <linux/log2.h>
> >>   #include <linux/module.h>
> >>   #include <linux/of.h>
> >>   #include <asm/alternative.h>
> >> @@ -70,6 +71,18 @@ EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
> >>
> >>   static bool riscv_isa_extension_check(int id)
> >>   {
> >> +     switch (id) {
> >> +     case RISCV_ISA_EXT_ZICBOM:
> >> +             if (!riscv_cbom_block_size) {
> >> +                     pr_err("Zicbom detected in ISA string, but no cbom-block-size found\n");
> >> +                     return false;
> >> +             } else if (!is_power_of_2(riscv_cbom_block_size)) {
> >> +                     pr_err("cbom-block-size present, but is not a power-of-2\n");
> >> +                     return false;
> >> +             }
> >> +             return true;
> >> +     }
> >> +
> >>        return true;
> >>   }
> >>
> >> --
> >> 2.38.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
>
Andrew Jones Nov. 30, 2022, 12:25 p.m. UTC | #4
On Wed, Nov 30, 2022 at 12:33:14PM +0100, Andrew Jones wrote:
> On Wed, Nov 30, 2022 at 09:46:20AM +0000, Conor.Dooley@microchip.com wrote:
> > On 29/11/2022 19:45, Conor Dooley wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > > 
> > > Hey Drew,
> > > 
> > > On Tue, Nov 29, 2022 at 03:34:47PM +0100, Andrew Jones wrote:
> > >> When a DT puts zicbom in the isa string, but does not provide a block
> > >> size, ALT_CMO_OP() will attempt to do cache operations on address
> > >> zero since the start address will be ANDed with zero. We can't simply
> > >> BUG() in riscv_init_cbom_blocksize() when we fail to find a block
> > >> size because the failure will happen before logging works, leaving
> > >> users to scratch their heads as to why the boot hung. Instead, ensure
> > >> Zicbom is disabled and output an error which will hopefully alert
> > >> people that the DT needs to be fixed. While at it, add a check that
> > >> the block size is a power-of-2 too.
> > >>
> > >> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > 
> > > This seems to be failing on nommu :/ I've got host machines issues so I
> > > could not reproduce it for you lcoally and paste an actual log, but if
> > > you build rv64_nommu_virt_defconfig I think you should be able to

You mean a 64-bit build with 'nommu_virt_defconfig', right? There isn't a
'rv64_nommu_virt_defconfig' that I know of.

> > > reproduce.
> > 
> > The actual error is:
> > riscv64-unknown-linux-gnu-ld: arch/riscv/kernel/cpufeature.o: in function `.L0 ':
> > cpufeature.c:(.text.unlikely+0x8): undefined reference to `riscv_cbom_block_size'
> 

I can't reproduce this. The following commands work fine for me

 $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-gnu- O=build nommu_virt_defconfig
 $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-gnu- O=build -j$(nproc)

And llvm also works

 $ make ARCH=riscv LLVM=1 O=build-llvm nommu_virt_defconfig
 $ make ARCH=riscv LLVM=1 O=build-llvm -j$(nproc)

Additionally, I can't see how riscv_cbom_block_size wouldn't be defined.
It's exported from arch/riscv/mm/cacheflush.c, which is always built,
and no ifdefery wraps it.

Thanks,
drew

> 
> drew
> 
> > 
> > 
> > > 
> > > Thanks,
> > > Conor.
> > > 
> > >> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > >> ---
> > >>   arch/riscv/kernel/cpufeature.c | 13 +++++++++++++
> > >>   1 file changed, 13 insertions(+)
> > >>
> > >> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > >> index 220be7222129..93e45560af30 100644
> > >> --- a/arch/riscv/kernel/cpufeature.c
> > >> +++ b/arch/riscv/kernel/cpufeature.c
> > >> @@ -9,6 +9,7 @@
> > >>   #include <linux/bitmap.h>
> > >>   #include <linux/ctype.h>
> > >>   #include <linux/libfdt.h>
> > >> +#include <linux/log2.h>
> > >>   #include <linux/module.h>
> > >>   #include <linux/of.h>
> > >>   #include <asm/alternative.h>
> > >> @@ -70,6 +71,18 @@ EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
> > >>
> > >>   static bool riscv_isa_extension_check(int id)
> > >>   {
> > >> +     switch (id) {
> > >> +     case RISCV_ISA_EXT_ZICBOM:
> > >> +             if (!riscv_cbom_block_size) {
> > >> +                     pr_err("Zicbom detected in ISA string, but no cbom-block-size found\n");
> > >> +                     return false;
> > >> +             } else if (!is_power_of_2(riscv_cbom_block_size)) {
> > >> +                     pr_err("cbom-block-size present, but is not a power-of-2\n");
> > >> +                     return false;
> > >> +             }
> > >> +             return true;
> > >> +     }
> > >> +
> > >>        return true;
> > >>   }
> > >>
> > >> --
> > >> 2.38.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
> >
Conor Dooley Nov. 30, 2022, 12:47 p.m. UTC | #5
On 30/11/2022 12:25, Andrew Jones wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, Nov 30, 2022 at 12:33:14PM +0100, Andrew Jones wrote:
>> On Wed, Nov 30, 2022 at 09:46:20AM +0000, Conor.Dooley@microchip.com wrote:
>>> On 29/11/2022 19:45, Conor Dooley wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> Hey Drew,
>>>>
>>>> On Tue, Nov 29, 2022 at 03:34:47PM +0100, Andrew Jones wrote:
>>>>> When a DT puts zicbom in the isa string, but does not provide a block
>>>>> size, ALT_CMO_OP() will attempt to do cache operations on address
>>>>> zero since the start address will be ANDed with zero. We can't simply
>>>>> BUG() in riscv_init_cbom_blocksize() when we fail to find a block
>>>>> size because the failure will happen before logging works, leaving
>>>>> users to scratch their heads as to why the boot hung. Instead, ensure
>>>>> Zicbom is disabled and output an error which will hopefully alert
>>>>> people that the DT needs to be fixed. While at it, add a check that
>>>>> the block size is a power-of-2 too.
>>>>>
>>>>> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
>>>>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>>>>
>>>> This seems to be failing on nommu :/ I've got host machines issues so I
>>>> could not reproduce it for you lcoally and paste an actual log, but if
>>>> you build rv64_nommu_virt_defconfig I think you should be able to
> 
> You mean a 64-bit build with 'nommu_virt_defconfig', right? There isn't a
> 'rv64_nommu_virt_defconfig' that I know of.
> 
>>>> reproduce.
>>>
>>> The actual error is:
>>> riscv64-unknown-linux-gnu-ld: arch/riscv/kernel/cpufeature.o: in function `.L0 ':
>>> cpufeature.c:(.text.unlikely+0x8): undefined reference to `riscv_cbom_block_size'
>>
> 
> I can't reproduce this. The following commands work fine for me
> 
>   $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-gnu- O=build nommu_virt_defconfig
>   $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-gnu- O=build -j$(nproc)
> 
> And llvm also works
> 
>   $ make ARCH=riscv LLVM=1 O=build-llvm nommu_virt_defconfig
>   $ make ARCH=riscv LLVM=1 O=build-llvm -j$(nproc)
> 
> Additionally, I can't see how riscv_cbom_block_size wouldn't be defined.
> It's exported from arch/riscv/mm/cacheflush.c, which is always built,
> and no ifdefery wraps it.

The base commit matters here, it picked riscv/for-next as the base for
this series. I guess this depends on some stuff that's in fixes only?

>>>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
>>>>> ---
>>>>>    arch/riscv/kernel/cpufeature.c | 13 +++++++++++++
>>>>>    1 file changed, 13 insertions(+)
>>>>>
>>>>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>>>>> index 220be7222129..93e45560af30 100644
>>>>> --- a/arch/riscv/kernel/cpufeature.c
>>>>> +++ b/arch/riscv/kernel/cpufeature.c
>>>>> @@ -9,6 +9,7 @@
>>>>>    #include <linux/bitmap.h>
>>>>>    #include <linux/ctype.h>
>>>>>    #include <linux/libfdt.h>
>>>>> +#include <linux/log2.h>
>>>>>    #include <linux/module.h>
>>>>>    #include <linux/of.h>
>>>>>    #include <asm/alternative.h>
>>>>> @@ -70,6 +71,18 @@ EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
>>>>>
>>>>>    static bool riscv_isa_extension_check(int id)
>>>>>    {
>>>>> +     switch (id) {
>>>>> +     case RISCV_ISA_EXT_ZICBOM:
>>>>> +             if (!riscv_cbom_block_size) {
>>>>> +                     pr_err("Zicbom detected in ISA string, but no cbom-block-size found\n");
>>>>> +                     return false;
>>>>> +             } else if (!is_power_of_2(riscv_cbom_block_size)) {
>>>>> +                     pr_err("cbom-block-size present, but is not a power-of-2\n");
>>>>> +                     return false;
>>>>> +             }
>>>>> +             return true;
>>>>> +     }
>>>>> +
>>>>>         return true;
>>>>>    }
>>>>>
>>>>> --
>>>>> 2.38.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
>>>
Andrew Jones Nov. 30, 2022, 1:55 p.m. UTC | #6
On Wed, Nov 30, 2022 at 12:47:03PM +0000, Conor.Dooley@microchip.com wrote:
> On 30/11/2022 12:25, Andrew Jones wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Wed, Nov 30, 2022 at 12:33:14PM +0100, Andrew Jones wrote:
> >> On Wed, Nov 30, 2022 at 09:46:20AM +0000, Conor.Dooley@microchip.com wrote:
> >>> On 29/11/2022 19:45, Conor Dooley wrote:
> >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>>
> >>>> Hey Drew,
> >>>>
> >>>> On Tue, Nov 29, 2022 at 03:34:47PM +0100, Andrew Jones wrote:
> >>>>> When a DT puts zicbom in the isa string, but does not provide a block
> >>>>> size, ALT_CMO_OP() will attempt to do cache operations on address
> >>>>> zero since the start address will be ANDed with zero. We can't simply
> >>>>> BUG() in riscv_init_cbom_blocksize() when we fail to find a block
> >>>>> size because the failure will happen before logging works, leaving
> >>>>> users to scratch their heads as to why the boot hung. Instead, ensure
> >>>>> Zicbom is disabled and output an error which will hopefully alert
> >>>>> people that the DT needs to be fixed. While at it, add a check that
> >>>>> the block size is a power-of-2 too.
> >>>>>
> >>>>> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> >>>>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> >>>>
> >>>> This seems to be failing on nommu :/ I've got host machines issues so I
> >>>> could not reproduce it for you lcoally and paste an actual log, but if
> >>>> you build rv64_nommu_virt_defconfig I think you should be able to
> > 
> > You mean a 64-bit build with 'nommu_virt_defconfig', right? There isn't a
> > 'rv64_nommu_virt_defconfig' that I know of.
> > 
> >>>> reproduce.
> >>>
> >>> The actual error is:
> >>> riscv64-unknown-linux-gnu-ld: arch/riscv/kernel/cpufeature.o: in function `.L0 ':
> >>> cpufeature.c:(.text.unlikely+0x8): undefined reference to `riscv_cbom_block_size'
> >>
> > 
> > I can't reproduce this. The following commands work fine for me
> > 
> >   $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-gnu- O=build nommu_virt_defconfig
> >   $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-gnu- O=build -j$(nproc)
> > 
> > And llvm also works
> > 
> >   $ make ARCH=riscv LLVM=1 O=build-llvm nommu_virt_defconfig
> >   $ make ARCH=riscv LLVM=1 O=build-llvm -j$(nproc)
> > 
> > Additionally, I can't see how riscv_cbom_block_size wouldn't be defined.
> > It's exported from arch/riscv/mm/cacheflush.c, which is always built,
> > and no ifdefery wraps it.
> 
> The base commit matters here, it picked riscv/for-next as the base for
> this series. I guess this depends on some stuff that's in fixes only?

It looks like riscv/for-next is based on v6.1-rc1, but commit 5c20a3a9df19
("RISC-V: Fix compilation without RISCV_ISA_ZICBOM") was merged for
v6.1-rc2.

Thanks,
drew
Conor Dooley Nov. 30, 2022, 2:55 p.m. UTC | #7
On 30/11/2022 13:55, Andrew Jones wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, Nov 30, 2022 at 12:47:03PM +0000, Conor.Dooley@microchip.com wrote:
>> On 30/11/2022 12:25, Andrew Jones wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Wed, Nov 30, 2022 at 12:33:14PM +0100, Andrew Jones wrote:
>>>> On Wed, Nov 30, 2022 at 09:46:20AM +0000, Conor.Dooley@microchip.com wrote:
>>>>> On 29/11/2022 19:45, Conor Dooley wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>
>>>>>> Hey Drew,
>>>>>>
>>>>>> On Tue, Nov 29, 2022 at 03:34:47PM +0100, Andrew Jones wrote:
>>>>>>> When a DT puts zicbom in the isa string, but does not provide a block
>>>>>>> size, ALT_CMO_OP() will attempt to do cache operations on address
>>>>>>> zero since the start address will be ANDed with zero. We can't simply
>>>>>>> BUG() in riscv_init_cbom_blocksize() when we fail to find a block
>>>>>>> size because the failure will happen before logging works, leaving
>>>>>>> users to scratch their heads as to why the boot hung. Instead, ensure
>>>>>>> Zicbom is disabled and output an error which will hopefully alert
>>>>>>> people that the DT needs to be fixed. While at it, add a check that
>>>>>>> the block size is a power-of-2 too.
>>>>>>>
>>>>>>> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
>>>>>>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>>>>>>
>>>>>> This seems to be failing on nommu :/ I've got host machines issues so I
>>>>>> could not reproduce it for you lcoally and paste an actual log, but if
>>>>>> you build rv64_nommu_virt_defconfig I think you should be able to
>>>
>>> You mean a 64-bit build with 'nommu_virt_defconfig', right? There isn't a
>>> 'rv64_nommu_virt_defconfig' that I know of.
>>>
>>>>>> reproduce.
>>>>>
>>>>> The actual error is:
>>>>> riscv64-unknown-linux-gnu-ld: arch/riscv/kernel/cpufeature.o: in function `.L0 ':
>>>>> cpufeature.c:(.text.unlikely+0x8): undefined reference to `riscv_cbom_block_size'
>>>>
>>>
>>> I can't reproduce this. The following commands work fine for me
>>>
>>>   $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-gnu- O=build nommu_virt_defconfig
>>>   $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-gnu- O=build -j$(nproc)
>>>
>>> And llvm also works
>>>
>>>   $ make ARCH=riscv LLVM=1 O=build-llvm nommu_virt_defconfig
>>>   $ make ARCH=riscv LLVM=1 O=build-llvm -j$(nproc)
>>>
>>> Additionally, I can't see how riscv_cbom_block_size wouldn't be defined.
>>> It's exported from arch/riscv/mm/cacheflush.c, which is always built,
>>> and no ifdefery wraps it.
>>
>> The base commit matters here, it picked riscv/for-next as the base for
>> this series. I guess this depends on some stuff that's in fixes only?
> 
> It looks like riscv/for-next is based on v6.1-rc1, but commit 5c20a3a9df19
> ("RISC-V: Fix compilation without RISCV_ISA_ZICBOM") was merged for
> v6.1-rc2.

Tut, silly me. Should have been immediately obvious...
I completely forgot about that, sorry for the trouble.
diff mbox series

Patch

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 220be7222129..93e45560af30 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -9,6 +9,7 @@ 
 #include <linux/bitmap.h>
 #include <linux/ctype.h>
 #include <linux/libfdt.h>
+#include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <asm/alternative.h>
@@ -70,6 +71,18 @@  EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
 
 static bool riscv_isa_extension_check(int id)
 {
+	switch (id) {
+	case RISCV_ISA_EXT_ZICBOM:
+		if (!riscv_cbom_block_size) {
+			pr_err("Zicbom detected in ISA string, but no cbom-block-size found\n");
+			return false;
+		} else if (!is_power_of_2(riscv_cbom_block_size)) {
+			pr_err("cbom-block-size present, but is not a power-of-2\n");
+			return false;
+		}
+		return true;
+	}
+
 	return true;
 }