| Message ID | 20241014094705.71775-1-cuiyunhui@bytedance.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISC-V: ACPI: fix early_ioremap to early_memremap | expand |
Hi Yunhui, On Mon, Oct 14, 2024 at 05:47:05PM +0800, Yunhui Cui wrote: > When SVPBMT is enabled, __acpi_map_table() will directly access the > data in DDR through the IO attribute, rather than through hardware > cache consistency, resulting in incorrect data in the obtained ACPI > table. > > The log: ACPI: [ACPI:0x18] Invalid zero length. > > We do not assume whether the bootloader flushes or not. We should > access in a cacheable way instead of maintaining cache consistency > by software. > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > --- > arch/riscv/kernel/acpi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c > index 6e0d333f57e5..3177c9af8764 100644 > --- a/arch/riscv/kernel/acpi.c > +++ b/arch/riscv/kernel/acpi.c > @@ -210,7 +210,7 @@ void __init __iomem *__acpi_map_table(unsigned long phys, unsigned long size) > if (!size) > return NULL; > > - return early_ioremap(phys, size); > + return early_memremap(phys, size); > } > I think __acpi_unmap_table() also needs similar change. You might need to typecast to suppress the sparse error [1] then. [1] - https://lore.kernel.org/oe-kbuild-all/202305201427.I7QhPjNW-lkp@intel.com/#r Thanks, Sunil
Hi Yunhui, On 14/10/2024 11:47, Yunhui Cui wrote: > When SVPBMT is enabled, __acpi_map_table() will directly access the > data in DDR through the IO attribute, rather than through hardware > cache consistency, resulting in incorrect data in the obtained ACPI > table. > > The log: ACPI: [ACPI:0x18] Invalid zero length. > > We do not assume whether the bootloader flushes or not. We should > access in a cacheable way instead of maintaining cache consistency > by software. > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > --- > arch/riscv/kernel/acpi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c > index 6e0d333f57e5..3177c9af8764 100644 > --- a/arch/riscv/kernel/acpi.c > +++ b/arch/riscv/kernel/acpi.c > @@ -210,7 +210,7 @@ void __init __iomem *__acpi_map_table(unsigned long phys, unsigned long size) > if (!size) > return NULL; > > - return early_ioremap(phys, size); > + return early_memremap(phys, size); > } > > void __init __acpi_unmap_table(void __iomem *map, unsigned long size) It makes sense to me since with this, we don't have to care about how the firmware mapped the table. And it mimics all other architectures (arm64, loongarch and x86). Here is the corresponding fixes tag: Fixes: 3b426d4b5b14 ("RISC-V: ACPI : Fix for usage of pointers in different address space") With the corresponding fix in __acpi_unmap_table() as pointed by Sunil, you can add: Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> And regarding the sparse error, I don't see any other architecture casting to __iomem, so maybe that's not necessary anymore? Thanks, Alex
Hi Sunil, On Mon, Oct 14, 2024 at 7:35 PM Sunil V L <sunilvl@ventanamicro.com> wrote: > > Hi Yunhui, > On Mon, Oct 14, 2024 at 05:47:05PM +0800, Yunhui Cui wrote: > > When SVPBMT is enabled, __acpi_map_table() will directly access the > > data in DDR through the IO attribute, rather than through hardware > > cache consistency, resulting in incorrect data in the obtained ACPI > > table. > > > > The log: ACPI: [ACPI:0x18] Invalid zero length. > > > > We do not assume whether the bootloader flushes or not. We should > > access in a cacheable way instead of maintaining cache consistency > > by software. > > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > > --- > > arch/riscv/kernel/acpi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c > > index 6e0d333f57e5..3177c9af8764 100644 > > --- a/arch/riscv/kernel/acpi.c > > +++ b/arch/riscv/kernel/acpi.c > > @@ -210,7 +210,7 @@ void __init __iomem *__acpi_map_table(unsigned long phys, unsigned long size) > > if (!size) > > return NULL; > > > > - return early_ioremap(phys, size); > > + return early_memremap(phys, size); > > } > > > I think __acpi_unmap_table() also needs similar change. You might need > to typecast to suppress the sparse error [1] then. OK. I will make the changes in v2. Regarding the sparse error, I will use another patch specifically to solve it. Is that okay? > > [1] - https://lore.kernel.org/oe-kbuild-all/202305201427.I7QhPjNW-lkp@intel.com/#r > > Thanks, > Sunil Thanks, Yunhui
Hi Alex, On Mon, Oct 14, 2024 at 8:12 PM Alexandre Ghiti <alex@ghiti.fr> wrote: > > Hi Yunhui, > > On 14/10/2024 11:47, Yunhui Cui wrote: > > When SVPBMT is enabled, __acpi_map_table() will directly access the > > data in DDR through the IO attribute, rather than through hardware > > cache consistency, resulting in incorrect data in the obtained ACPI > > table. > > > > The log: ACPI: [ACPI:0x18] Invalid zero length. > > > > We do not assume whether the bootloader flushes or not. We should > > access in a cacheable way instead of maintaining cache consistency > > by software. > > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > > --- > > arch/riscv/kernel/acpi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c > > index 6e0d333f57e5..3177c9af8764 100644 > > --- a/arch/riscv/kernel/acpi.c > > +++ b/arch/riscv/kernel/acpi.c > > @@ -210,7 +210,7 @@ void __init __iomem *__acpi_map_table(unsigned long phys, unsigned long size) > > if (!size) > > return NULL; > > > > - return early_ioremap(phys, size); > > + return early_memremap(phys, size); > > } > > > > void __init __acpi_unmap_table(void __iomem *map, unsigned long size) > > > It makes sense to me since with this, we don't have to care about how > the firmware mapped the table. And it mimics all other architectures > (arm64, loongarch and x86). > > Here is the corresponding fixes tag: > > Fixes: 3b426d4b5b14 ("RISC-V: ACPI : Fix for usage of pointers in > different address space") > > With the corresponding fix in __acpi_unmap_table() as pointed by Sunil, > you can add: > > Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> > > And regarding the sparse error, I don't see any other architecture > casting to __iomem, so maybe that's not necessary anymore? OK. I will make the changes in v2. Regarding the sparse error, I will use another patch specifically to solve it. Is that okay? > > Thanks, > > Alex > > Thanks, Yunhui
On 14/10/2024 14:30, yunhui cui wrote: > Hi Alex, > > On Mon, Oct 14, 2024 at 8:12 PM Alexandre Ghiti <alex@ghiti.fr> wrote: >> Hi Yunhui, >> >> On 14/10/2024 11:47, Yunhui Cui wrote: >>> When SVPBMT is enabled, __acpi_map_table() will directly access the >>> data in DDR through the IO attribute, rather than through hardware >>> cache consistency, resulting in incorrect data in the obtained ACPI >>> table. >>> >>> The log: ACPI: [ACPI:0x18] Invalid zero length. >>> >>> We do not assume whether the bootloader flushes or not. We should >>> access in a cacheable way instead of maintaining cache consistency >>> by software. >>> >>> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> >>> --- >>> arch/riscv/kernel/acpi.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c >>> index 6e0d333f57e5..3177c9af8764 100644 >>> --- a/arch/riscv/kernel/acpi.c >>> +++ b/arch/riscv/kernel/acpi.c >>> @@ -210,7 +210,7 @@ void __init __iomem *__acpi_map_table(unsigned long phys, unsigned long size) >>> if (!size) >>> return NULL; >>> >>> - return early_ioremap(phys, size); >>> + return early_memremap(phys, size); >>> } >>> >>> void __init __acpi_unmap_table(void __iomem *map, unsigned long size) >> >> It makes sense to me since with this, we don't have to care about how >> the firmware mapped the table. And it mimics all other architectures >> (arm64, loongarch and x86). >> >> Here is the corresponding fixes tag: >> >> Fixes: 3b426d4b5b14 ("RISC-V: ACPI : Fix for usage of pointers in >> different address space") >> >> With the corresponding fix in __acpi_unmap_table() as pointed by Sunil, >> you can add: >> >> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> >> >> And regarding the sparse error, I don't see any other architecture >> casting to __iomem, so maybe that's not necessary anymore? > OK. I will make the changes in v2. Regarding the sparse error, I will > use another patch specifically to solve it. Is that okay? If the second patch only consists in casting, I would not use another patch since the patch 2 would fix something introduced in patch 1. But if patch 2 is more complicated, it may make sense to do as you suggest, the goal is to merge patch 1 asap. Thanks! Alex > >> Thanks, >> >> Alex >> >> > Thanks, > Yunhui
Hi Alex, On Mon, Oct 14, 2024 at 9:01 PM Alexandre Ghiti <alex@ghiti.fr> wrote: > > On 14/10/2024 14:30, yunhui cui wrote: > > Hi Alex, > > > > On Mon, Oct 14, 2024 at 8:12 PM Alexandre Ghiti <alex@ghiti.fr> wrote: > >> Hi Yunhui, > >> > >> On 14/10/2024 11:47, Yunhui Cui wrote: > >>> When SVPBMT is enabled, __acpi_map_table() will directly access the > >>> data in DDR through the IO attribute, rather than through hardware > >>> cache consistency, resulting in incorrect data in the obtained ACPI > >>> table. > >>> > >>> The log: ACPI: [ACPI:0x18] Invalid zero length. > >>> > >>> We do not assume whether the bootloader flushes or not. We should > >>> access in a cacheable way instead of maintaining cache consistency > >>> by software. > >>> > >>> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > >>> --- > >>> arch/riscv/kernel/acpi.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c > >>> index 6e0d333f57e5..3177c9af8764 100644 > >>> --- a/arch/riscv/kernel/acpi.c > >>> +++ b/arch/riscv/kernel/acpi.c > >>> @@ -210,7 +210,7 @@ void __init __iomem *__acpi_map_table(unsigned long phys, unsigned long size) > >>> if (!size) > >>> return NULL; > >>> > >>> - return early_ioremap(phys, size); > >>> + return early_memremap(phys, size); > >>> } > >>> > >>> void __init __acpi_unmap_table(void __iomem *map, unsigned long size) > >> > >> It makes sense to me since with this, we don't have to care about how > >> the firmware mapped the table. And it mimics all other architectures > >> (arm64, loongarch and x86). > >> > >> Here is the corresponding fixes tag: > >> > >> Fixes: 3b426d4b5b14 ("RISC-V: ACPI : Fix for usage of pointers in > >> different address space") > >> > >> With the corresponding fix in __acpi_unmap_table() as pointed by Sunil, > >> you can add: > >> > >> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> > >> > >> And regarding the sparse error, I don't see any other architecture > >> casting to __iomem, so maybe that's not necessary anymore? > > OK. I will make the changes in v2. Regarding the sparse error, I will > > use another patch specifically to solve it. Is that okay? > > > If the second patch only consists in casting, I would not use another > patch since the patch 2 would fix something introduced in patch 1. > > But if patch 2 is more complicated, it may make sense to do as you > suggest, the goal is to merge patch 1 asap. Regarding patch 2, I executed "make & make install" for sparse of the riscv arch. I "git cloned" from https://github.com/ConchuOD/sparse.git. Then when compiling the kernel, the following error was reported: cd /data00/cyh/linux-next/ make defconfig cp .config build-riscv64/ make O=build-riscv64/ C=1 W=1 arch/riscv/kernel/ drivers/acpi/ drivers/mailbox/ -j make[1]: Entering directory '/data00/cyh/linux-next/build-riscv64' GEN Makefile CC scripts/mod/empty.o CC scripts/mod/devicetable-offsets.s CHECK ../scripts/mod/empty.c invalid argument to '-march': '_zicsr_zifencei' make[3]: *** [../scripts/Makefile.build:229: scripts/mod/empty.o] Error 1 make[3]: *** Deleting file 'scripts/mod/empty.o' make[3]: *** Waiting for unfinished jobs.... make[2]: *** [/data00/cyh/linux-next/Makefile:1215: prepare0] Error 2 make[1]: *** [/data00/cyh/linux-next/Makefile:224: __sub-make] Error 2 Do you know the solution? > > Thanks! > > Alex > > > > > >> Thanks, > >> > >> Alex > >> > >> > > Thanks, > > Yunhui Thanks, Yunhui
Hi Yunhui, Sorry for the late reply, I was on vacation last week. On Mon, Oct 21, 2024 at 8:45 AM yunhui cui <cuiyunhui@bytedance.com> wrote: > > Hi Alex, > > On Mon, Oct 14, 2024 at 9:01 PM Alexandre Ghiti <alex@ghiti.fr> wrote: > > > > On 14/10/2024 14:30, yunhui cui wrote: > > > Hi Alex, > > > > > > On Mon, Oct 14, 2024 at 8:12 PM Alexandre Ghiti <alex@ghiti.fr> wrote: > > >> Hi Yunhui, > > >> > > >> On 14/10/2024 11:47, Yunhui Cui wrote: > > >>> When SVPBMT is enabled, __acpi_map_table() will directly access the > > >>> data in DDR through the IO attribute, rather than through hardware > > >>> cache consistency, resulting in incorrect data in the obtained ACPI > > >>> table. > > >>> > > >>> The log: ACPI: [ACPI:0x18] Invalid zero length. > > >>> > > >>> We do not assume whether the bootloader flushes or not. We should > > >>> access in a cacheable way instead of maintaining cache consistency > > >>> by software. > > >>> > > >>> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > > >>> --- > > >>> arch/riscv/kernel/acpi.c | 2 +- > > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > > >>> > > >>> diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c > > >>> index 6e0d333f57e5..3177c9af8764 100644 > > >>> --- a/arch/riscv/kernel/acpi.c > > >>> +++ b/arch/riscv/kernel/acpi.c > > >>> @@ -210,7 +210,7 @@ void __init __iomem *__acpi_map_table(unsigned long phys, unsigned long size) > > >>> if (!size) > > >>> return NULL; > > >>> > > >>> - return early_ioremap(phys, size); > > >>> + return early_memremap(phys, size); > > >>> } > > >>> > > >>> void __init __acpi_unmap_table(void __iomem *map, unsigned long size) > > >> > > >> It makes sense to me since with this, we don't have to care about how > > >> the firmware mapped the table. And it mimics all other architectures > > >> (arm64, loongarch and x86). > > >> > > >> Here is the corresponding fixes tag: > > >> > > >> Fixes: 3b426d4b5b14 ("RISC-V: ACPI : Fix for usage of pointers in > > >> different address space") > > >> > > >> With the corresponding fix in __acpi_unmap_table() as pointed by Sunil, > > >> you can add: > > >> > > >> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> > > >> > > >> And regarding the sparse error, I don't see any other architecture > > >> casting to __iomem, so maybe that's not necessary anymore? > > > OK. I will make the changes in v2. Regarding the sparse error, I will > > > use another patch specifically to solve it. Is that okay? > > > > > > If the second patch only consists in casting, I would not use another > > patch since the patch 2 would fix something introduced in patch 1. > > > > But if patch 2 is more complicated, it may make sense to do as you > > suggest, the goal is to merge patch 1 asap. > > Regarding patch 2, I executed "make & make install" for sparse of the > riscv arch. I "git cloned" from > https://github.com/ConchuOD/sparse.git. > Then when compiling the kernel, the following error was reported: > > cd /data00/cyh/linux-next/ > make defconfig > cp .config build-riscv64/ > > make O=build-riscv64/ C=1 W=1 arch/riscv/kernel/ drivers/acpi/ > drivers/mailbox/ -j > make[1]: Entering directory '/data00/cyh/linux-next/build-riscv64' > GEN Makefile > CC scripts/mod/empty.o > CC scripts/mod/devicetable-offsets.s > CHECK ../scripts/mod/empty.c > invalid argument to '-march': '_zicsr_zifencei' > > make[3]: *** [../scripts/Makefile.build:229: scripts/mod/empty.o] Error 1 > make[3]: *** Deleting file 'scripts/mod/empty.o' > make[3]: *** Waiting for unfinished jobs.... > make[2]: *** [/data00/cyh/linux-next/Makefile:1215: prepare0] Error 2 > make[1]: *** [/data00/cyh/linux-next/Makefile:224: __sub-make] Error 2 > > Do you know the solution? In case you did not find the solution yet, I would say that your toolchain is too old. Let me know if I can help, Thanks, Alex > > > > > Thanks! > > > > Alex > > > > > > > > > >> Thanks, > > >> > > >> Alex > > >> > > >> > > > Thanks, > > > Yunhui > > Thanks, > Yunhui
Hi Alex, On Mon, Oct 28, 2024 at 9:37 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote: > > Hi Yunhui, > > Sorry for the late reply, I was on vacation last week. > > On Mon, Oct 21, 2024 at 8:45 AM yunhui cui <cuiyunhui@bytedance.com> wrote: > > > > Hi Alex, > > > > On Mon, Oct 14, 2024 at 9:01 PM Alexandre Ghiti <alex@ghiti.fr> wrote: > > > > > > On 14/10/2024 14:30, yunhui cui wrote: > > > > Hi Alex, > > > > > > > > On Mon, Oct 14, 2024 at 8:12 PM Alexandre Ghiti <alex@ghiti.fr> wrote: > > > >> Hi Yunhui, > > > >> > > > >> On 14/10/2024 11:47, Yunhui Cui wrote: > > > >>> When SVPBMT is enabled, __acpi_map_table() will directly access the > > > >>> data in DDR through the IO attribute, rather than through hardware > > > >>> cache consistency, resulting in incorrect data in the obtained ACPI > > > >>> table. > > > >>> > > > >>> The log: ACPI: [ACPI:0x18] Invalid zero length. > > > >>> > > > >>> We do not assume whether the bootloader flushes or not. We should > > > >>> access in a cacheable way instead of maintaining cache consistency > > > >>> by software. > > > >>> > > > >>> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > > > >>> --- > > > >>> arch/riscv/kernel/acpi.c | 2 +- > > > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > > > >>> > > > >>> diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c > > > >>> index 6e0d333f57e5..3177c9af8764 100644 > > > >>> --- a/arch/riscv/kernel/acpi.c > > > >>> +++ b/arch/riscv/kernel/acpi.c > > > >>> @@ -210,7 +210,7 @@ void __init __iomem *__acpi_map_table(unsigned long phys, unsigned long size) > > > >>> if (!size) > > > >>> return NULL; > > > >>> > > > >>> - return early_ioremap(phys, size); > > > >>> + return early_memremap(phys, size); > > > >>> } > > > >>> > > > >>> void __init __acpi_unmap_table(void __iomem *map, unsigned long size) > > > >> > > > >> It makes sense to me since with this, we don't have to care about how > > > >> the firmware mapped the table. And it mimics all other architectures > > > >> (arm64, loongarch and x86). > > > >> > > > >> Here is the corresponding fixes tag: > > > >> > > > >> Fixes: 3b426d4b5b14 ("RISC-V: ACPI : Fix for usage of pointers in > > > >> different address space") > > > >> > > > >> With the corresponding fix in __acpi_unmap_table() as pointed by Sunil, > > > >> you can add: > > > >> > > > >> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> > > > >> > > > >> And regarding the sparse error, I don't see any other architecture > > > >> casting to __iomem, so maybe that's not necessary anymore? > > > > OK. I will make the changes in v2. Regarding the sparse error, I will > > > > use another patch specifically to solve it. Is that okay? > > > > > > > > > If the second patch only consists in casting, I would not use another > > > patch since the patch 2 would fix something introduced in patch 1. > > > > > > But if patch 2 is more complicated, it may make sense to do as you > > > suggest, the goal is to merge patch 1 asap. > > > > Regarding patch 2, I executed "make & make install" for sparse of the > > riscv arch. I "git cloned" from > > https://github.com/ConchuOD/sparse.git. > > Then when compiling the kernel, the following error was reported: > > > > cd /data00/cyh/linux-next/ > > make defconfig > > cp .config build-riscv64/ > > > > make O=build-riscv64/ C=1 W=1 arch/riscv/kernel/ drivers/acpi/ > > drivers/mailbox/ -j > > make[1]: Entering directory '/data00/cyh/linux-next/build-riscv64' > > GEN Makefile > > CC scripts/mod/empty.o > > CC scripts/mod/devicetable-offsets.s > > CHECK ../scripts/mod/empty.c > > invalid argument to '-march': '_zicsr_zifencei' > > > > make[3]: *** [../scripts/Makefile.build:229: scripts/mod/empty.o] Error 1 > > make[3]: *** Deleting file 'scripts/mod/empty.o' > > make[3]: *** Waiting for unfinished jobs.... > > make[2]: *** [/data00/cyh/linux-next/Makefile:1215: prepare0] Error 2 > > make[1]: *** [/data00/cyh/linux-next/Makefile:224: __sub-make] Error 2 > > > > Do you know the solution? > > In case you did not find the solution yet, I would say that your > toolchain is too old. > > Let me know if I can help, It may not be a problem of an old compiler. The version of the compiler I am using is 13.2. It is a problem with sparse. When running "sparse --arch=riscv -march=rv64imac_zicsr_zifencei scripts/mod/empty.c", there is an invalid argument for '-march': '_zicsr_zifencei'. > > Thanks, > > Alex > > > > > > > > > Thanks! > > > > > > Alex > > > > > > > > > > > > > >> Thanks, > > > >> > > > >> Alex > > > >> > > > >> > > > > Thanks, > > > > Yunhui > > > > Thanks, > > Yunhui Thanks, Yunhui
On Tue, Oct 29, 2024 at 11:04:49AM +0800, yunhui cui wrote: > Hi Alex, > > > On Mon, Oct 28, 2024 at 9:37 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote: > > > > Hi Yunhui, > > > > Sorry for the late reply, I was on vacation last week. > > > > On Mon, Oct 21, 2024 at 8:45 AM yunhui cui <cuiyunhui@bytedance.com> wrote: > > > > > > Hi Alex, > > > > > > On Mon, Oct 14, 2024 at 9:01 PM Alexandre Ghiti <alex@ghiti.fr> wrote: > > > > > > > > On 14/10/2024 14:30, yunhui cui wrote: > > > > > Hi Alex, > > > > > > > > > > On Mon, Oct 14, 2024 at 8:12 PM Alexandre Ghiti <alex@ghiti.fr> wrote: > > > > >> Hi Yunhui, > > > > >> > > > > >> On 14/10/2024 11:47, Yunhui Cui wrote: > > > > >>> When SVPBMT is enabled, __acpi_map_table() will directly access the > > > > >>> data in DDR through the IO attribute, rather than through hardware > > > > >>> cache consistency, resulting in incorrect data in the obtained ACPI > > > > >>> table. > > > > >>> > > > > >>> The log: ACPI: [ACPI:0x18] Invalid zero length. > > > > >>> > > > > >>> We do not assume whether the bootloader flushes or not. We should > > > > >>> access in a cacheable way instead of maintaining cache consistency > > > > >>> by software. > > > > >>> > > > > >>> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> > > > > >>> --- > > > > >>> arch/riscv/kernel/acpi.c | 2 +- > > > > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > > > > >>> > > > > >>> diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c > > > > >>> index 6e0d333f57e5..3177c9af8764 100644 > > > > >>> --- a/arch/riscv/kernel/acpi.c > > > > >>> +++ b/arch/riscv/kernel/acpi.c > > > > >>> @@ -210,7 +210,7 @@ void __init __iomem *__acpi_map_table(unsigned long phys, unsigned long size) > > > > >>> if (!size) > > > > >>> return NULL; > > > > >>> > > > > >>> - return early_ioremap(phys, size); > > > > >>> + return early_memremap(phys, size); > > > > >>> } > > > > >>> > > > > >>> void __init __acpi_unmap_table(void __iomem *map, unsigned long size) > > > > >> > > > > >> It makes sense to me since with this, we don't have to care about how > > > > >> the firmware mapped the table. And it mimics all other architectures > > > > >> (arm64, loongarch and x86). > > > > >> > > > > >> Here is the corresponding fixes tag: > > > > >> > > > > >> Fixes: 3b426d4b5b14 ("RISC-V: ACPI : Fix for usage of pointers in > > > > >> different address space") > > > > >> > > > > >> With the corresponding fix in __acpi_unmap_table() as pointed by Sunil, > > > > >> you can add: > > > > >> > > > > >> Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> > > > > >> > > > > >> And regarding the sparse error, I don't see any other architecture > > > > >> casting to __iomem, so maybe that's not necessary anymore? > > > > > OK. I will make the changes in v2. Regarding the sparse error, I will > > > > > use another patch specifically to solve it. Is that okay? > > > > > > > > > > > > If the second patch only consists in casting, I would not use another > > > > patch since the patch 2 would fix something introduced in patch 1. > > > > > > > > But if patch 2 is more complicated, it may make sense to do as you > > > > suggest, the goal is to merge patch 1 asap. > > > > > > Regarding patch 2, I executed "make & make install" for sparse of the > > > riscv arch. I "git cloned" from > > > https://github.com/ConchuOD/sparse.git. > > > Then when compiling the kernel, the following error was reported: > > > > > > cd /data00/cyh/linux-next/ > > > make defconfig > > > cp .config build-riscv64/ > > > > > > make O=build-riscv64/ C=1 W=1 arch/riscv/kernel/ drivers/acpi/ > > > drivers/mailbox/ -j > > > make[1]: Entering directory '/data00/cyh/linux-next/build-riscv64' > > > GEN Makefile > > > CC scripts/mod/empty.o > > > CC scripts/mod/devicetable-offsets.s > > > CHECK ../scripts/mod/empty.c > > > invalid argument to '-march': '_zicsr_zifencei' > > > > > > make[3]: *** [../scripts/Makefile.build:229: scripts/mod/empty.o] Error 1 > > > make[3]: *** Deleting file 'scripts/mod/empty.o' > > > make[3]: *** Waiting for unfinished jobs.... > > > make[2]: *** [/data00/cyh/linux-next/Makefile:1215: prepare0] Error 2 > > > make[1]: *** [/data00/cyh/linux-next/Makefile:224: __sub-make] Error 2 > > > > > > Do you know the solution? > > > > In case you did not find the solution yet, I would say that your > > toolchain is too old. > > > > Let me know if I can help, > It may not be a problem of an old compiler. The version of the > compiler I am using is 13.2. > It is a problem with sparse. When running "sparse --arch=riscv > -march=rv64imac_zicsr_zifencei scripts/mod/empty.c", there is an > invalid argument for '-march': '_zicsr_zifencei'. > Hi Yunhui, I think you need latest sparse. I remember building sparse from latest sources to avoid this error. https://www.kernel.org/doc/html/v4.11/dev-tools/sparse.html Thanks, Sunil
diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c index 6e0d333f57e5..3177c9af8764 100644 --- a/arch/riscv/kernel/acpi.c +++ b/arch/riscv/kernel/acpi.c @@ -210,7 +210,7 @@ void __init __iomem *__acpi_map_table(unsigned long phys, unsigned long size) if (!size) return NULL; - return early_ioremap(phys, size); + return early_memremap(phys, size); } void __init __acpi_unmap_table(void __iomem *map, unsigned long size)
When SVPBMT is enabled, __acpi_map_table() will directly access the data in DDR through the IO attribute, rather than through hardware cache consistency, resulting in incorrect data in the obtained ACPI table. The log: ACPI: [ACPI:0x18] Invalid zero length. We do not assume whether the bootloader flushes or not. We should access in a cacheable way instead of maintaining cache consistency by software. Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com> --- arch/riscv/kernel/acpi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)