[PATCH v3 -next 0/3] RISC-V: ACPI improvements

[PATCH v3 -next 0/3] RISC-V: ACPI improvements

[Sunil V L]

This series is a set of patches which were originally part of RFC v1 series
[1] to add ACPI support in RISC-V interrupt controllers. Since these
patches are independent of the interrupt controllers, creating this new
series which helps to merge instead of waiting for big series.

This set of patches primarily adds support below ECR [2] which is approved
by the ASWG and adds below features.

- Get CBO block sizes from RHCT on ACPI based systems.

Additionally, the series contains a patch to improve acpi_os_ioremap().

[1] - https://lore.kernel.org/lkml/20230803175202.3173957-1-sunilvl@ventanamicro.com/
[2] - https://drive.google.com/file/d/1sKbOa8m1UZw1JkquZYe3F1zQBN1xXsaf/view?usp=sharing

Changes since v2:
	1) Modified acpi_get_cbo_block_size() not to take cpu parameter
	   but follow same pattern as DT (Feedback from Samuel and Drew)
	2) Dropped timer patch from the series since it is already
	   applied.
	3) Selected ARCH_KEEP_MEMBLOCK only if ACPI (Feedback from
	   Alex).
	4) Added RB tags received so far except RHCT patch which has
	   changed quite significantly from previous version.

Changes since RFC v1:
	1) Separated the patches from interrupt controller support series.
	2) Addressed feedback from Andy and Drew.
	3) Rebased to Palmer's for-next tree.
	4) Added RB tags received on RFC v1.


Sunil V L (3):
  RISC-V: ACPI: Enhance acpi_os_ioremap with MMIO remapping
  RISC-V: ACPI: RHCT: Add function to get CBO block sizes
  RISC-V: cacheflush: Initialize CBO variables on ACPI systems

 arch/riscv/Kconfig            |  1 +
 arch/riscv/include/asm/acpi.h |  6 +++
 arch/riscv/kernel/acpi.c      | 87 +++++++++++++++++++++++++++++++-
 arch/riscv/mm/cacheflush.c    | 25 +++++++---
 drivers/acpi/riscv/rhct.c     | 93 +++++++++++++++++++++++++++++++++++
 5 files changed, 204 insertions(+), 8 deletions(-)

-- 
2.39.2

[PATCH v3 -next 1/3] RISC-V: ACPI: Enhance acpi_os_ioremap with MMIO remapping

[Sunil V L]

Enhance the acpi_os_ioremap() to support opregions in MMIO space. Also,
have strict checks using EFI memory map to allow remapping the RAM similar
to arm64.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/Kconfig       |  1 +
 arch/riscv/kernel/acpi.c | 87 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 86 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index d607ab0f7c6d..805c8ab7f23b 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -39,6 +39,7 @@ config RISCV
 	select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
 	select ARCH_HAS_UBSAN_SANITIZE_ALL
 	select ARCH_HAS_VDSO_DATA
+	select ARCH_KEEP_MEMBLOCK if ACPI
 	select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX
 	select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT
 	select ARCH_STACKWALK
diff --git a/arch/riscv/kernel/acpi.c b/arch/riscv/kernel/acpi.c
index 56cb2c986c48..e619edc8b0cc 100644
--- a/arch/riscv/kernel/acpi.c
+++ b/arch/riscv/kernel/acpi.c
@@ -14,9 +14,10 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/efi.h>
 #include <linux/io.h>
+#include <linux/memblock.h>
 #include <linux/pci.h>
-#include <linux/efi.h>
 
 int acpi_noirq = 1;		/* skip ACPI IRQ initialization */
 int acpi_disabled = 1;
@@ -217,7 +218,89 @@ void __init __acpi_unmap_table(void __iomem *map, unsigned long size)
 
 void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
 {
-	return (void __iomem *)memremap(phys, size, MEMREMAP_WB);
+	efi_memory_desc_t *md, *region = NULL;
+	pgprot_t prot;
+
+	if (WARN_ON_ONCE(!efi_enabled(EFI_MEMMAP)))
+		return NULL;
+
+	for_each_efi_memory_desc(md) {
+		u64 end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT);
+
+		if (phys < md->phys_addr || phys >= end)
+			continue;
+
+		if (phys + size > end) {
+			pr_warn(FW_BUG "requested region covers multiple EFI memory regions\n");
+			return NULL;
+		}
+		region = md;
+		break;
+	}
+
+	/*
+	 * It is fine for AML to remap regions that are not represented in the
+	 * EFI memory map at all, as it only describes normal memory, and MMIO
+	 * regions that require a virtual mapping to make them accessible to
+	 * the EFI runtime services.
+	 */
+	prot = PAGE_KERNEL_IO;
+	if (region) {
+		switch (region->type) {
+		case EFI_LOADER_CODE:
+		case EFI_LOADER_DATA:
+		case EFI_BOOT_SERVICES_CODE:
+		case EFI_BOOT_SERVICES_DATA:
+		case EFI_CONVENTIONAL_MEMORY:
+		case EFI_PERSISTENT_MEMORY:
+			if (memblock_is_map_memory(phys) ||
+			    !memblock_is_region_memory(phys, size)) {
+				pr_warn(FW_BUG "requested region covers kernel memory\n");
+				return NULL;
+			}
+
+			/*
+			 * Mapping kernel memory is permitted if the region in
+			 * question is covered by a single memblock with the
+			 * NOMAP attribute set: this enables the use of ACPI
+			 * table overrides passed via initramfs.
+			 * This particular use case only requires read access.
+			 */
+			fallthrough;
+
+		case EFI_RUNTIME_SERVICES_CODE:
+			/*
+			 * This would be unusual, but not problematic per se,
+			 * as long as we take care not to create a writable
+			 * mapping for executable code.
+			 */
+			prot = PAGE_KERNEL_RO;
+			break;
+
+		case EFI_ACPI_RECLAIM_MEMORY:
+			/*
+			 * ACPI reclaim memory is used to pass firmware tables
+			 * and other data that is intended for consumption by
+			 * the OS only, which may decide it wants to reclaim
+			 * that memory and use it for something else. We never
+			 * do that, but we usually add it to the linear map
+			 * anyway, in which case we should use the existing
+			 * mapping.
+			 */
+			if (memblock_is_map_memory(phys))
+				return (void __iomem *)__va(phys);
+			fallthrough;
+
+		default:
+			if (region->attribute & EFI_MEMORY_WB)
+				prot = PAGE_KERNEL;
+			else if ((region->attribute & EFI_MEMORY_WC) ||
+				 (region->attribute & EFI_MEMORY_WT))
+				prot = pgprot_writecombine(PAGE_KERNEL);
+		}
+	}
+
+	return ioremap_prot(phys, size, pgprot_val(prot));
 }
 
 #ifdef CONFIG_PCI
-- 
2.39.2

[PATCH v3 -next 2/3] RISC-V: ACPI: RHCT: Add function to get CBO block sizes

[Sunil V L]

Cache Block Operation (CBO) related block size in ACPI is provided by RHCT.
Add support to read the CMO node in RHCT to get this information.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/include/asm/acpi.h |  6 +++
 drivers/acpi/riscv/rhct.c     | 93 +++++++++++++++++++++++++++++++++++
 2 files changed, 99 insertions(+)

diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
index d5604d2073bc..7dad0cf9d701 100644
--- a/arch/riscv/include/asm/acpi.h
+++ b/arch/riscv/include/asm/acpi.h
@@ -66,6 +66,8 @@ int acpi_get_riscv_isa(struct acpi_table_header *table,
 		       unsigned int cpu, const char **isa);
 
 static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
+void acpi_get_cbo_block_size(struct acpi_table_header *table, u32 *cbom_size,
+			     u32 *cboz_size, u32 *cbop_size);
 #else
 static inline void acpi_init_rintc_map(void) { }
 static inline struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
@@ -79,6 +81,10 @@ static inline int acpi_get_riscv_isa(struct acpi_table_header *table,
 	return -EINVAL;
 }
 
+static inline void acpi_get_cbo_block_size(struct acpi_table_header *table,
+					   u32 *cbom_size, u32 *cboz_size,
+					   u32 *cbop_size) { }
+
 #endif /* CONFIG_ACPI */
 
 #endif /*_ASM_ACPI_H*/
diff --git a/drivers/acpi/riscv/rhct.c b/drivers/acpi/riscv/rhct.c
index b280b3e9c7d9..105f1aaa3fac 100644
--- a/drivers/acpi/riscv/rhct.c
+++ b/drivers/acpi/riscv/rhct.c
@@ -8,6 +8,7 @@
 #define pr_fmt(fmt)     "ACPI: RHCT: " fmt
 
 #include <linux/acpi.h>
+#include <linux/bits.h>
 
 static struct acpi_table_header *acpi_get_rhct(void)
 {
@@ -81,3 +82,95 @@ int acpi_get_riscv_isa(struct acpi_table_header *table, unsigned int cpu, const
 
 	return -1;
 }
+
+static void acpi_parse_hart_info_cmo_node(struct acpi_table_rhct *rhct,
+					  struct acpi_rhct_hart_info *hart_info,
+					  u32 *cbom_size, u32 *cboz_size, u32 *cbop_size)
+{
+	u32 size_hartinfo = sizeof(struct acpi_rhct_hart_info);
+	u32 size_hdr = sizeof(struct acpi_rhct_node_header);
+	struct acpi_rhct_node_header *ref_node;
+	struct acpi_rhct_cmo_node *cmo_node;
+	u32 *hart_info_node_offset;
+
+	hart_info_node_offset = ACPI_ADD_PTR(u32, hart_info, size_hartinfo);
+	for (int i = 0; i < hart_info->num_offsets; i++) {
+		ref_node = ACPI_ADD_PTR(struct acpi_rhct_node_header,
+					rhct, hart_info_node_offset[i]);
+		if (ref_node->type == ACPI_RHCT_NODE_TYPE_CMO) {
+			cmo_node = ACPI_ADD_PTR(struct acpi_rhct_cmo_node,
+						ref_node, size_hdr);
+			if (cbom_size && cmo_node->cbom_size <= 30) {
+				if (!*cbom_size) {
+					*cbom_size = BIT(cmo_node->cbom_size);
+				} else if (*cbom_size !=
+						BIT(cmo_node->cbom_size)) {
+					pr_warn("CBOM size is not the same across harts\n");
+				}
+			}
+
+			if (cboz_size && cmo_node->cboz_size <= 30) {
+				if (!*cboz_size) {
+					*cboz_size = BIT(cmo_node->cboz_size);
+				} else if (*cboz_size !=
+						BIT(cmo_node->cboz_size)) {
+					pr_warn("CBOZ size is not the same across harts\n");
+				}
+			}
+
+			if (cbop_size && cmo_node->cbop_size <= 30) {
+				if (!*cbop_size) {
+					*cbop_size = BIT(cmo_node->cbop_size);
+				} else if (*cbop_size !=
+						BIT(cmo_node->cbop_size)) {
+					pr_warn("CBOP size is not the same across harts\n");
+				}
+			}
+		}
+	}
+}
+
+/*
+ * During early boot, the caller should call acpi_get_table() and pass its pointer to
+ * these functions(and free up later). At run time, since this table can be used
+ * multiple times, pass NULL so that the table remains in memory
+ */
+void acpi_get_cbo_block_size(struct acpi_table_header *table, u32 *cbom_size,
+			     u32 *cboz_size, u32 *cbop_size)
+{
+	u32 size_hdr = sizeof(struct acpi_rhct_node_header);
+	struct acpi_rhct_node_header *node, *end;
+	struct acpi_rhct_hart_info *hart_info;
+	struct acpi_table_rhct *rhct;
+
+	if (acpi_disabled)
+		return;
+
+	if (table) {
+		rhct = (struct acpi_table_rhct *)table;
+	} else {
+		rhct = (struct acpi_table_rhct *)acpi_get_rhct();
+		if (!rhct)
+			return;
+	}
+
+	if (cbom_size)
+		*cbom_size = 0;
+
+	if (cboz_size)
+		*cboz_size = 0;
+
+	if (cbop_size)
+		*cbop_size = 0;
+
+	end = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->header.length);
+	for (node = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->node_offset);
+	     node < end;
+	     node = ACPI_ADD_PTR(struct acpi_rhct_node_header, node, node->length)) {
+		if (node->type == ACPI_RHCT_NODE_TYPE_HART_INFO) {
+			hart_info = ACPI_ADD_PTR(struct acpi_rhct_hart_info, node, size_hdr);
+			acpi_parse_hart_info_cmo_node(rhct, hart_info, cbom_size,
+						      cboz_size, cbop_size);
+		}
+	}
+}
-- 
2.39.2

[PATCH v3 -next 3/3] RISC-V: cacheflush: Initialize CBO variables on ACPI systems

[Sunil V L]

Initialize the CBO variables on ACPI based systems using information in
RHCT.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 arch/riscv/mm/cacheflush.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
index f1387272a551..55a34f2020a8 100644
--- a/arch/riscv/mm/cacheflush.c
+++ b/arch/riscv/mm/cacheflush.c
@@ -3,7 +3,9 @@
  * Copyright (C) 2017 SiFive
  */
 
+#include <linux/acpi.h>
 #include <linux/of.h>
+#include <asm/acpi.h>
 #include <asm/cacheflush.h>
 
 #ifdef CONFIG_SMP
@@ -124,13 +126,24 @@ void __init riscv_init_cbo_blocksizes(void)
 	unsigned long cbom_hartid, cboz_hartid;
 	u32 cbom_block_size = 0, cboz_block_size = 0;
 	struct device_node *node;
+	struct acpi_table_header *rhct;
+	acpi_status status;
+
+	if (acpi_disabled) {
+		for_each_of_cpu_node(node) {
+			/* set block-size for cbom and/or cboz extension if available */
+			cbo_get_block_size(node, "riscv,cbom-block-size",
+					   &cbom_block_size, &cbom_hartid);
+			cbo_get_block_size(node, "riscv,cboz-block-size",
+					   &cboz_block_size, &cboz_hartid);
+		}
+	} else {
+		status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
+		if (ACPI_FAILURE(status))
+			return;
 
-	for_each_of_cpu_node(node) {
-		/* set block-size for cbom and/or cboz extension if available */
-		cbo_get_block_size(node, "riscv,cbom-block-size",
-				   &cbom_block_size, &cbom_hartid);
-		cbo_get_block_size(node, "riscv,cboz-block-size",
-				   &cboz_block_size, &cboz_hartid);
+		acpi_get_cbo_block_size(rhct, &cbom_block_size, &cboz_block_size, NULL);
+		acpi_put_table((struct acpi_table_header *)rhct);
 	}
 
 	if (cbom_block_size)
-- 
2.39.2

Re: [PATCH v3 -next 2/3] RISC-V: ACPI: RHCT: Add function to get CBO block sizes

[Andrew Jones]

On Mon, Oct 16, 2023 at 10:19:57PM +0530, Sunil V L wrote:
> Cache Block Operation (CBO) related block size in ACPI is provided by RHCT.
> Add support to read the CMO node in RHCT to get this information.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  arch/riscv/include/asm/acpi.h |  6 +++
>  drivers/acpi/riscv/rhct.c     | 93 +++++++++++++++++++++++++++++++++++
>  2 files changed, 99 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> index d5604d2073bc..7dad0cf9d701 100644
> --- a/arch/riscv/include/asm/acpi.h
> +++ b/arch/riscv/include/asm/acpi.h
> @@ -66,6 +66,8 @@ int acpi_get_riscv_isa(struct acpi_table_header *table,
>  		       unsigned int cpu, const char **isa);
>  
>  static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
> +void acpi_get_cbo_block_size(struct acpi_table_header *table, u32 *cbom_size,
> +			     u32 *cboz_size, u32 *cbop_size);
>  #else
>  static inline void acpi_init_rintc_map(void) { }
>  static inline struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> @@ -79,6 +81,10 @@ static inline int acpi_get_riscv_isa(struct acpi_table_header *table,
>  	return -EINVAL;
>  }
>  
> +static inline void acpi_get_cbo_block_size(struct acpi_table_header *table,
> +					   u32 *cbom_size, u32 *cboz_size,
> +					   u32 *cbop_size) { }
> +
>  #endif /* CONFIG_ACPI */
>  
>  #endif /*_ASM_ACPI_H*/
> diff --git a/drivers/acpi/riscv/rhct.c b/drivers/acpi/riscv/rhct.c
> index b280b3e9c7d9..105f1aaa3fac 100644
> --- a/drivers/acpi/riscv/rhct.c
> +++ b/drivers/acpi/riscv/rhct.c
> @@ -8,6 +8,7 @@
>  #define pr_fmt(fmt)     "ACPI: RHCT: " fmt
>  
>  #include <linux/acpi.h>
> +#include <linux/bits.h>
>  
>  static struct acpi_table_header *acpi_get_rhct(void)
>  {
> @@ -81,3 +82,95 @@ int acpi_get_riscv_isa(struct acpi_table_header *table, unsigned int cpu, const
>  
>  	return -1;
>  }
> +
> +static void acpi_parse_hart_info_cmo_node(struct acpi_table_rhct *rhct,
> +					  struct acpi_rhct_hart_info *hart_info,
> +					  u32 *cbom_size, u32 *cboz_size, u32 *cbop_size)
> +{
> +	u32 size_hartinfo = sizeof(struct acpi_rhct_hart_info);
> +	u32 size_hdr = sizeof(struct acpi_rhct_node_header);
> +	struct acpi_rhct_node_header *ref_node;
> +	struct acpi_rhct_cmo_node *cmo_node;
> +	u32 *hart_info_node_offset;
> +
> +	hart_info_node_offset = ACPI_ADD_PTR(u32, hart_info, size_hartinfo);
> +	for (int i = 0; i < hart_info->num_offsets; i++) {
> +		ref_node = ACPI_ADD_PTR(struct acpi_rhct_node_header,
> +					rhct, hart_info_node_offset[i]);
> +		if (ref_node->type == ACPI_RHCT_NODE_TYPE_CMO) {
> +			cmo_node = ACPI_ADD_PTR(struct acpi_rhct_cmo_node,
> +						ref_node, size_hdr);
> +			if (cbom_size && cmo_node->cbom_size <= 30) {
> +				if (!*cbom_size) {
> +					*cbom_size = BIT(cmo_node->cbom_size);
> +				} else if (*cbom_size !=
> +						BIT(cmo_node->cbom_size)) {

No need to break the if line, we can go to 100 chars. And then, since both
the if and else if arms only have single statements, all the {} can be
dropped too. Same comment for cboz and cbop.

> +					pr_warn("CBOM size is not the same across harts\n");
> +				}
> +			}
> +
> +			if (cboz_size && cmo_node->cboz_size <= 30) {
> +				if (!*cboz_size) {
> +					*cboz_size = BIT(cmo_node->cboz_size);
> +				} else if (*cboz_size !=
> +						BIT(cmo_node->cboz_size)) {
> +					pr_warn("CBOZ size is not the same across harts\n");
> +				}
> +			}
> +
> +			if (cbop_size && cmo_node->cbop_size <= 30) {
> +				if (!*cbop_size) {
> +					*cbop_size = BIT(cmo_node->cbop_size);
> +				} else if (*cbop_size !=
> +						BIT(cmo_node->cbop_size)) {
> +					pr_warn("CBOP size is not the same across harts\n");
> +				}
> +			}
> +		}
> +	}
> +}
> +
> +/*
> + * During early boot, the caller should call acpi_get_table() and pass its pointer to
> + * these functions(and free up later). At run time, since this table can be used
                     ^ add a space here

> + * multiple times, pass NULL so that the table remains in memory
> + */
> +void acpi_get_cbo_block_size(struct acpi_table_header *table, u32 *cbom_size,
> +			     u32 *cboz_size, u32 *cbop_size)
> +{
> +	u32 size_hdr = sizeof(struct acpi_rhct_node_header);
> +	struct acpi_rhct_node_header *node, *end;
> +	struct acpi_rhct_hart_info *hart_info;
> +	struct acpi_table_rhct *rhct;
> +
> +	if (acpi_disabled)
> +		return;
> +
> +	if (table) {
> +		rhct = (struct acpi_table_rhct *)table;
> +	} else {
> +		rhct = (struct acpi_table_rhct *)acpi_get_rhct();

Not an issue of this patch, but it seems like acpi_get_rhct() should
return a struct acpi_table_rhct pointer instead of a struct
acpi_table_header pointer since it's specifically returning an RHCT.

> +		if (!rhct)
> +			return;
> +	}
> +
> +	if (cbom_size)
> +		*cbom_size = 0;
> +
> +	if (cboz_size)
> +		*cboz_size = 0;
> +
> +	if (cbop_size)
> +		*cbop_size = 0;
> +
> +	end = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->header.length);
> +	for (node = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->node_offset);
> +	     node < end;
> +	     node = ACPI_ADD_PTR(struct acpi_rhct_node_header, node, node->length)) {
> +		if (node->type == ACPI_RHCT_NODE_TYPE_HART_INFO) {
> +			hart_info = ACPI_ADD_PTR(struct acpi_rhct_hart_info, node, size_hdr);
> +			acpi_parse_hart_info_cmo_node(rhct, hart_info, cbom_size,
> +						      cboz_size, cbop_size);
> +		}
> +	}
> +}
> -- 
> 2.39.2
>

Other than the nits

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew

Re: [PATCH v3 -next 3/3] RISC-V: cacheflush: Initialize CBO variables on ACPI systems

[Andrew Jones]

On Mon, Oct 16, 2023 at 10:19:58PM +0530, Sunil V L wrote:
> Initialize the CBO variables on ACPI based systems using information in
> RHCT.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  arch/riscv/mm/cacheflush.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> index f1387272a551..55a34f2020a8 100644
> --- a/arch/riscv/mm/cacheflush.c
> +++ b/arch/riscv/mm/cacheflush.c
> @@ -3,7 +3,9 @@
>   * Copyright (C) 2017 SiFive
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/of.h>
> +#include <asm/acpi.h>
>  #include <asm/cacheflush.h>
>  
>  #ifdef CONFIG_SMP
> @@ -124,13 +126,24 @@ void __init riscv_init_cbo_blocksizes(void)
>  	unsigned long cbom_hartid, cboz_hartid;
>  	u32 cbom_block_size = 0, cboz_block_size = 0;
>  	struct device_node *node;
> +	struct acpi_table_header *rhct;
> +	acpi_status status;
> +
> +	if (acpi_disabled) {
> +		for_each_of_cpu_node(node) {
> +			/* set block-size for cbom and/or cboz extension if available */
> +			cbo_get_block_size(node, "riscv,cbom-block-size",
> +					   &cbom_block_size, &cbom_hartid);
> +			cbo_get_block_size(node, "riscv,cboz-block-size",
> +					   &cboz_block_size, &cboz_hartid);
> +		}
> +	} else {
> +		status = acpi_get_table(ACPI_SIG_RHCT, 0, &rhct);
> +		if (ACPI_FAILURE(status))
> +			return;
>  
> -	for_each_of_cpu_node(node) {
> -		/* set block-size for cbom and/or cboz extension if available */
> -		cbo_get_block_size(node, "riscv,cbom-block-size",
> -				   &cbom_block_size, &cbom_hartid);
> -		cbo_get_block_size(node, "riscv,cboz-block-size",
> -				   &cboz_block_size, &cboz_hartid);
> +		acpi_get_cbo_block_size(rhct, &cbom_block_size, &cboz_block_size, NULL);
> +		acpi_put_table((struct acpi_table_header *)rhct);
>  	}
>  
>  	if (cbom_block_size)
> -- 
> 2.39.2
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH v3 -next 2/3] RISC-V: ACPI: RHCT: Add function to get CBO block sizes

[Sunil V L]

On Tue, Oct 17, 2023 at 10:37:41AM +0200, Andrew Jones wrote:
> On Mon, Oct 16, 2023 at 10:19:57PM +0530, Sunil V L wrote:
> > Cache Block Operation (CBO) related block size in ACPI is provided by RHCT.
> > Add support to read the CMO node in RHCT to get this information.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  arch/riscv/include/asm/acpi.h |  6 +++
> >  drivers/acpi/riscv/rhct.c     | 93 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 99 insertions(+)
> > 
> > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> > index d5604d2073bc..7dad0cf9d701 100644
> > --- a/arch/riscv/include/asm/acpi.h
> > +++ b/arch/riscv/include/asm/acpi.h
> > @@ -66,6 +66,8 @@ int acpi_get_riscv_isa(struct acpi_table_header *table,
> >  		       unsigned int cpu, const char **isa);
> >  
> >  static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
> > +void acpi_get_cbo_block_size(struct acpi_table_header *table, u32 *cbom_size,
> > +			     u32 *cboz_size, u32 *cbop_size);
> >  #else
> >  static inline void acpi_init_rintc_map(void) { }
> >  static inline struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> > @@ -79,6 +81,10 @@ static inline int acpi_get_riscv_isa(struct acpi_table_header *table,
> >  	return -EINVAL;
> >  }
> >  
> > +static inline void acpi_get_cbo_block_size(struct acpi_table_header *table,
> > +					   u32 *cbom_size, u32 *cboz_size,
> > +					   u32 *cbop_size) { }
> > +
> >  #endif /* CONFIG_ACPI */
> >  
> >  #endif /*_ASM_ACPI_H*/
> > diff --git a/drivers/acpi/riscv/rhct.c b/drivers/acpi/riscv/rhct.c
> > index b280b3e9c7d9..105f1aaa3fac 100644
> > --- a/drivers/acpi/riscv/rhct.c
> > +++ b/drivers/acpi/riscv/rhct.c
> > @@ -8,6 +8,7 @@
> >  #define pr_fmt(fmt)     "ACPI: RHCT: " fmt
> >  
> >  #include <linux/acpi.h>
> > +#include <linux/bits.h>
> >  
> >  static struct acpi_table_header *acpi_get_rhct(void)
> >  {
> > @@ -81,3 +82,95 @@ int acpi_get_riscv_isa(struct acpi_table_header *table, unsigned int cpu, const
> >  
> >  	return -1;
> >  }
> > +
> > +static void acpi_parse_hart_info_cmo_node(struct acpi_table_rhct *rhct,
> > +					  struct acpi_rhct_hart_info *hart_info,
> > +					  u32 *cbom_size, u32 *cboz_size, u32 *cbop_size)
> > +{
> > +	u32 size_hartinfo = sizeof(struct acpi_rhct_hart_info);
> > +	u32 size_hdr = sizeof(struct acpi_rhct_node_header);
> > +	struct acpi_rhct_node_header *ref_node;
> > +	struct acpi_rhct_cmo_node *cmo_node;
> > +	u32 *hart_info_node_offset;
> > +
> > +	hart_info_node_offset = ACPI_ADD_PTR(u32, hart_info, size_hartinfo);
> > +	for (int i = 0; i < hart_info->num_offsets; i++) {
> > +		ref_node = ACPI_ADD_PTR(struct acpi_rhct_node_header,
> > +					rhct, hart_info_node_offset[i]);
> > +		if (ref_node->type == ACPI_RHCT_NODE_TYPE_CMO) {
> > +			cmo_node = ACPI_ADD_PTR(struct acpi_rhct_cmo_node,
> > +						ref_node, size_hdr);
> > +			if (cbom_size && cmo_node->cbom_size <= 30) {
> > +				if (!*cbom_size) {
> > +					*cbom_size = BIT(cmo_node->cbom_size);
> > +				} else if (*cbom_size !=
> > +						BIT(cmo_node->cbom_size)) {
> 
> No need to break the if line, we can go to 100 chars. And then, since both
> the if and else if arms only have single statements, all the {} can be
> dropped too. Same comment for cboz and cbop.
> 
Yeah, it is a side effect of working on repos with different coding
standards. It is interesting that checkpatch didn't recommend to remove
the braces. Let me fix it in next revision. Thanks!.

> > +					pr_warn("CBOM size is not the same across harts\n");
> > +				}
> > +			}
> > +
> > +			if (cboz_size && cmo_node->cboz_size <= 30) {
> > +				if (!*cboz_size) {
> > +					*cboz_size = BIT(cmo_node->cboz_size);
> > +				} else if (*cboz_size !=
> > +						BIT(cmo_node->cboz_size)) {
> > +					pr_warn("CBOZ size is not the same across harts\n");
> > +				}
> > +			}
> > +
> > +			if (cbop_size && cmo_node->cbop_size <= 30) {
> > +				if (!*cbop_size) {
> > +					*cbop_size = BIT(cmo_node->cbop_size);
> > +				} else if (*cbop_size !=
> > +						BIT(cmo_node->cbop_size)) {
> > +					pr_warn("CBOP size is not the same across harts\n");
> > +				}
> > +			}
> > +		}
> > +	}
> > +}
> > +
> > +/*
> > + * During early boot, the caller should call acpi_get_table() and pass its pointer to
> > + * these functions(and free up later). At run time, since this table can be used
>                      ^ add a space here
> 
> > + * multiple times, pass NULL so that the table remains in memory
> > + */
> > +void acpi_get_cbo_block_size(struct acpi_table_header *table, u32 *cbom_size,
> > +			     u32 *cboz_size, u32 *cbop_size)
> > +{
> > +	u32 size_hdr = sizeof(struct acpi_rhct_node_header);
> > +	struct acpi_rhct_node_header *node, *end;
> > +	struct acpi_rhct_hart_info *hart_info;
> > +	struct acpi_table_rhct *rhct;
> > +
> > +	if (acpi_disabled)
> > +		return;
> > +
> > +	if (table) {
> > +		rhct = (struct acpi_table_rhct *)table;
> > +	} else {
> > +		rhct = (struct acpi_table_rhct *)acpi_get_rhct();
> 
> Not an issue of this patch, but it seems like acpi_get_rhct() should
> return a struct acpi_table_rhct pointer instead of a struct
> acpi_table_header pointer since it's specifically returning an RHCT.
> 
Makes sense. Let me add a patch to improve this.

> > +		if (!rhct)
> > +			return;
> > +	}
> > +
> > +	if (cbom_size)
> > +		*cbom_size = 0;
> > +
> > +	if (cboz_size)
> > +		*cboz_size = 0;
> > +
> > +	if (cbop_size)
> > +		*cbop_size = 0;
> > +
> > +	end = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->header.length);
> > +	for (node = ACPI_ADD_PTR(struct acpi_rhct_node_header, rhct, rhct->node_offset);
> > +	     node < end;
> > +	     node = ACPI_ADD_PTR(struct acpi_rhct_node_header, node, node->length)) {
> > +		if (node->type == ACPI_RHCT_NODE_TYPE_HART_INFO) {
> > +			hart_info = ACPI_ADD_PTR(struct acpi_rhct_hart_info, node, size_hdr);
> > +			acpi_parse_hart_info_cmo_node(rhct, hart_info, cbom_size,
> > +						      cboz_size, cbop_size);
> > +		}
> > +	}
> > +}
> > -- 
> > 2.39.2
> >
> 
> Other than the nits
> 
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> 
Thanks a lot for the review!
Sunil

Re: [PATCH v3 -next 2/3] RISC-V: ACPI: RHCT: Add function to get CBO block sizes

[Andrew Jones]

On Tue, Oct 17, 2023 at 09:09:23PM +0530, Sunil V L wrote:
> On Tue, Oct 17, 2023 at 10:37:41AM +0200, Andrew Jones wrote:
> > On Mon, Oct 16, 2023 at 10:19:57PM +0530, Sunil V L wrote:
> > > Cache Block Operation (CBO) related block size in ACPI is provided by RHCT.
> > > Add support to read the CMO node in RHCT to get this information.
> > > 
> > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > > ---
> > >  arch/riscv/include/asm/acpi.h |  6 +++
> > >  drivers/acpi/riscv/rhct.c     | 93 +++++++++++++++++++++++++++++++++++
> > >  2 files changed, 99 insertions(+)
> > > 
> > > diff --git a/arch/riscv/include/asm/acpi.h b/arch/riscv/include/asm/acpi.h
> > > index d5604d2073bc..7dad0cf9d701 100644
> > > --- a/arch/riscv/include/asm/acpi.h
> > > +++ b/arch/riscv/include/asm/acpi.h
> > > @@ -66,6 +66,8 @@ int acpi_get_riscv_isa(struct acpi_table_header *table,
> > >  		       unsigned int cpu, const char **isa);
> > >  
> > >  static inline int acpi_numa_get_nid(unsigned int cpu) { return NUMA_NO_NODE; }
> > > +void acpi_get_cbo_block_size(struct acpi_table_header *table, u32 *cbom_size,
> > > +			     u32 *cboz_size, u32 *cbop_size);
> > >  #else
> > >  static inline void acpi_init_rintc_map(void) { }
> > >  static inline struct acpi_madt_rintc *acpi_cpu_get_madt_rintc(int cpu)
> > > @@ -79,6 +81,10 @@ static inline int acpi_get_riscv_isa(struct acpi_table_header *table,
> > >  	return -EINVAL;
> > >  }
> > >  
> > > +static inline void acpi_get_cbo_block_size(struct acpi_table_header *table,
> > > +					   u32 *cbom_size, u32 *cboz_size,
> > > +					   u32 *cbop_size) { }
> > > +
> > >  #endif /* CONFIG_ACPI */
> > >  
> > >  #endif /*_ASM_ACPI_H*/
> > > diff --git a/drivers/acpi/riscv/rhct.c b/drivers/acpi/riscv/rhct.c
> > > index b280b3e9c7d9..105f1aaa3fac 100644
> > > --- a/drivers/acpi/riscv/rhct.c
> > > +++ b/drivers/acpi/riscv/rhct.c
> > > @@ -8,6 +8,7 @@
> > >  #define pr_fmt(fmt)     "ACPI: RHCT: " fmt
> > >  
> > >  #include <linux/acpi.h>
> > > +#include <linux/bits.h>
> > >  
> > >  static struct acpi_table_header *acpi_get_rhct(void)
> > >  {
> > > @@ -81,3 +82,95 @@ int acpi_get_riscv_isa(struct acpi_table_header *table, unsigned int cpu, const
> > >  
> > >  	return -1;
> > >  }
> > > +
> > > +static void acpi_parse_hart_info_cmo_node(struct acpi_table_rhct *rhct,
> > > +					  struct acpi_rhct_hart_info *hart_info,
> > > +					  u32 *cbom_size, u32 *cboz_size, u32 *cbop_size)
> > > +{
> > > +	u32 size_hartinfo = sizeof(struct acpi_rhct_hart_info);
> > > +	u32 size_hdr = sizeof(struct acpi_rhct_node_header);
> > > +	struct acpi_rhct_node_header *ref_node;
> > > +	struct acpi_rhct_cmo_node *cmo_node;
> > > +	u32 *hart_info_node_offset;
> > > +
> > > +	hart_info_node_offset = ACPI_ADD_PTR(u32, hart_info, size_hartinfo);
> > > +	for (int i = 0; i < hart_info->num_offsets; i++) {
> > > +		ref_node = ACPI_ADD_PTR(struct acpi_rhct_node_header,
> > > +					rhct, hart_info_node_offset[i]);
> > > +		if (ref_node->type == ACPI_RHCT_NODE_TYPE_CMO) {
> > > +			cmo_node = ACPI_ADD_PTR(struct acpi_rhct_cmo_node,
> > > +						ref_node, size_hdr);
> > > +			if (cbom_size && cmo_node->cbom_size <= 30) {
> > > +				if (!*cbom_size) {
> > > +					*cbom_size = BIT(cmo_node->cbom_size);
> > > +				} else if (*cbom_size !=
> > > +						BIT(cmo_node->cbom_size)) {
> > 
> > No need to break the if line, we can go to 100 chars. And then, since both
> > the if and else if arms only have single statements, all the {} can be
> > dropped too. Same comment for cboz and cbop.
> > 
> Yeah, it is a side effect of working on repos with different coding
> standards. It is interesting that checkpatch didn't recommend to remove
> the braces. Let me fix it in next revision. Thanks!.

I think checkpatch is OK with single lines that get broken into multiple
lines having braces around them. In fact, it may even be preferred. My
suggestion is to not break the line though, so checkpatch would likely
complain after turning these lines into true single lines.

Thanks,
drew

Re: [PATCH v3 -next 3/3] RISC-V: cacheflush: Initialize CBO variables on ACPI systems

[Samuel Holland]

On 2023-10-16 11:49 AM, Sunil V L wrote:
> Initialize the CBO variables on ACPI based systems using information in
> RHCT.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  arch/riscv/mm/cacheflush.c | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)

Reviewed-by: Samuel Holland <samuel.holland@sifive.com>