| 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
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(-)