diff mbox series

[v2,07/11] lib: sbi: Add error prints in sbi_domain_finalize()

Message ID 20201204155224.733188-8-anup.patel@wdc.com
State Accepted
Headers show
Series OpenSBI domain configuration using device tree | expand

Commit Message

Anup Patel Dec. 4, 2020, 3:52 p.m. UTC 
We add error prints in sbi_domain_finalize() and sanitize_domain()
to help debug domain configuration issues.

Signed-off-by: Anup Patel <anup.patel@wdc.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Atish Patra <atish.patra@wdc.com>
---
 lib/sbi/sbi_domain.c | 63 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 52 insertions(+), 11 deletions(-)

Comments

Anup Patel Dec. 5, 2020, 9:50 a.m. UTC | #1 
> -----Original Message-----
> From: Anup Patel <Anup.Patel@wdc.com>
> Sent: 04 December 2020 21:22
> To: Atish Patra <Atish.Patra@wdc.com>; Alistair Francis
> <Alistair.Francis@wdc.com>
> Cc: Anup Patel <anup@brainfault.org>; opensbi@lists.infradead.org; Anup
> Patel <Anup.Patel@wdc.com>; Alistair Francis <Alistair.Francis@wdc.com>
> Subject: [PATCH v2 07/11] lib: sbi: Add error prints in sbi_domain_finalize()
> 
> We add error prints in sbi_domain_finalize() and sanitize_domain() to help
> debug domain configuration issues.
> 
> Signed-off-by: Anup Patel <anup.patel@wdc.com>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Atish Patra <atish.patra@wdc.com>
> ---
>  lib/sbi/sbi_domain.c | 63 ++++++++++++++++++++++++++++++++++++-----
> ---
>  1 file changed, 52 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c index
> 1b50604..639e016 100644
> --- a/lib/sbi/sbi_domain.c
> +++ b/lib/sbi/sbi_domain.c
> @@ -174,19 +174,33 @@ static int sanitize_domain(const struct sbi_platform
> *plat,
>  	struct sbi_domain_memregion treg, *reg, *reg1;
> 
>  	/* Check possible HARTs */
> -	if (!dom->possible_harts)
> +	if (!dom->possible_harts) {
> +		sbi_printf("%s: %s possible HART mask is NULL\n",
> +			   __func__, dom->name);
>  		return SBI_EINVAL;
> +	}
>  	sbi_hartmask_for_each_hart(i, dom->possible_harts) {
> -		if (sbi_platform_hart_invalid(plat, i))
> +		if (sbi_platform_hart_invalid(plat, i)) {
> +			sbi_printf("%s: %s possible HART mask has invalid "
> +				   "hart %d\n", __func__, dom->name, i);
>  			return SBI_EINVAL;
> +		}
>  	};
> 
>  	/* Check memory regions */
> -	if (!dom->regions)
> +	if (!dom->regions) {
> +		sbi_printf("%s: %s regions is NULL\n",
> +			   __func__, dom->name);
>  		return SBI_EINVAL;
> +	}
>  	sbi_domain_for_each_memregion(dom, reg) {
> -		if (!is_region_valid(reg))
> +		if (!is_region_valid(reg)) {
> +			sbi_printf("%s: %s has invalid region base=0x%lx "
> +				   "order=%lu flags=0x%lx\n", __func__,
> +				   dom->name, reg->base, reg->order,
> +				   reg->flags);
>  			return SBI_EINVAL;
> +		}
>  	}
> 
>  	/* Count memory regions and check presence of firmware region */
> @@ -199,8 +213,11 @@ static int sanitize_domain(const struct sbi_platform
> *plat,
>  			have_fw_reg = TRUE;
>  		count++;
>  	}
> -	if (!have_fw_reg)
> +	if (!have_fw_reg) {
> +		sbi_printf("%s: %s does not have firmware region\n",
> +			   __func__, dom->name);
>  		return SBI_EINVAL;
> +	}
> 
>  	/* Sort the memory regions */
>  	for (i = 0; i < (count - 1); i++) {
> @@ -208,8 +225,15 @@ static int sanitize_domain(const struct sbi_platform
> *plat,
>  		for (j = i + 1; j < count; j++) {
>  			reg1 = &dom->regions[j];
> 
> -			if (is_region_conflict(reg1, reg))
> +			if (is_region_conflict(reg1, reg)) {
> +				sbi_printf("%s: %s conflict between regions "
> +					"(base=0x%lx order=%lu flags=0x%lx)
> and "
> +					"(base=0x%lx order=%lu
> flags=0x%lx)\n",
> +					__func__, dom->name,
> +					reg->base, reg->order, reg->flags,
> +					reg1->base, reg1->order, reg1-
> >flags);
>  				return SBI_EINVAL;
> +			}
> 
>  			if (!is_region_before(reg1, reg))
>  				continue;
> @@ -233,13 +257,19 @@ static int sanitize_domain(const struct sbi_platform
> *plat,
>  	 * protect M-mode context and enforce checks on memory accesses.
>  	 */
>  	if (dom->next_mode != PRV_S &&
> -	    dom->next_mode != PRV_U)
> +	    dom->next_mode != PRV_U) {
> +		sbi_printf("%s: %s invalid next booting stage mode 0x%lx\n",
> +			   __func__, dom->name, dom->next_mode);
>  		return SBI_EINVAL;
> +	}
> 
>  	/* Check next address and next mode*/
>  	if (!sbi_domain_check_addr(dom, dom->next_addr, dom-
> >next_mode,
> -				   SBI_DOMAIN_EXECUTE))
> +				   SBI_DOMAIN_EXECUTE)) {
> +		sbi_printf("%s: %s next booting stage addres 0x%lx can't "
> +			   "execute\n", __func__, dom->name, dom-
> >next_addr);
>  		return SBI_EINVAL;
> +	}
> 
>  	return 0;
>  }
> @@ -371,13 +401,20 @@ int sbi_domain_finalize(struct sbi_scratch *scratch,
> u32 cold_hartid)
>  			 * Ensure that we have room for Domain Index to
>  			 * HART ID mapping
>  			 */
> -			if (SBI_DOMAIN_MAX_INDEX <= domain_count)
> +			if (SBI_DOMAIN_MAX_INDEX <= domain_count) {
> +				sbi_printf("%s: No room for %s\n",
> +					   __func__, dom->name);
>  				return SBI_ENOSPC;
> +			}
> 
>  			/* Sanitize discovered domain */
>  			rc = sanitize_domain(plat, dom);
> -			if (rc)
> +			if (rc) {
> +				sbi_printf("%s: sanity checks failed for"
> +					   " %s (error %d)\n", __func__,
> +					   dom->name, rc);
>  				return rc;
> +			}
> 
>  			/* Assign index to domain */
>  			dom->index = domain_count++;
> @@ -437,8 +474,12 @@ int sbi_domain_finalize(struct sbi_scratch *scratch,
> u32 cold_hartid)
>  						dom->next_addr,
>  						dom->next_mode,
>  						dom->next_arg1);
> -			if (rc)
> +			if (rc) {
> +				sbi_printf("%s: failed to start boot HART %d"
> +					   " for %s (error %d)\n", __func__,
> +					   dhart, dom->name, rc);
>  				return rc;
> +			}
>  		}
>  	}
> 
> --
> 2.25.1

Applied this patch to the riscv/opensbi repo.

Regards,
Anup
diff mbox series

Patch

diff --git a/lib/sbi/sbi_domain.c b/lib/sbi/sbi_domain.c
index 1b50604..639e016 100644
--- a/lib/sbi/sbi_domain.c
+++ b/lib/sbi/sbi_domain.c
@@ -174,19 +174,33 @@  static int sanitize_domain(const struct sbi_platform *plat,
 	struct sbi_domain_memregion treg, *reg, *reg1;
 
 	/* Check possible HARTs */
-	if (!dom->possible_harts)
+	if (!dom->possible_harts) {
+		sbi_printf("%s: %s possible HART mask is NULL\n",
+			   __func__, dom->name);
 		return SBI_EINVAL;
+	}
 	sbi_hartmask_for_each_hart(i, dom->possible_harts) {
-		if (sbi_platform_hart_invalid(plat, i))
+		if (sbi_platform_hart_invalid(plat, i)) {
+			sbi_printf("%s: %s possible HART mask has invalid "
+				   "hart %d\n", __func__, dom->name, i);
 			return SBI_EINVAL;
+		}
 	};
 
 	/* Check memory regions */
-	if (!dom->regions)
+	if (!dom->regions) {
+		sbi_printf("%s: %s regions is NULL\n",
+			   __func__, dom->name);
 		return SBI_EINVAL;
+	}
 	sbi_domain_for_each_memregion(dom, reg) {
-		if (!is_region_valid(reg))
+		if (!is_region_valid(reg)) {
+			sbi_printf("%s: %s has invalid region base=0x%lx "
+				   "order=%lu flags=0x%lx\n", __func__,
+				   dom->name, reg->base, reg->order,
+				   reg->flags);
 			return SBI_EINVAL;
+		}
 	}
 
 	/* Count memory regions and check presence of firmware region */
@@ -199,8 +213,11 @@  static int sanitize_domain(const struct sbi_platform *plat,
 			have_fw_reg = TRUE;
 		count++;
 	}
-	if (!have_fw_reg)
+	if (!have_fw_reg) {
+		sbi_printf("%s: %s does not have firmware region\n",
+			   __func__, dom->name);
 		return SBI_EINVAL;
+	}
 
 	/* Sort the memory regions */
 	for (i = 0; i < (count - 1); i++) {
@@ -208,8 +225,15 @@  static int sanitize_domain(const struct sbi_platform *plat,
 		for (j = i + 1; j < count; j++) {
 			reg1 = &dom->regions[j];
 
-			if (is_region_conflict(reg1, reg))
+			if (is_region_conflict(reg1, reg)) {
+				sbi_printf("%s: %s conflict between regions "
+					"(base=0x%lx order=%lu flags=0x%lx) and "
+					"(base=0x%lx order=%lu flags=0x%lx)\n",
+					__func__, dom->name,
+					reg->base, reg->order, reg->flags,
+					reg1->base, reg1->order, reg1->flags);
 				return SBI_EINVAL;
+			}
 
 			if (!is_region_before(reg1, reg))
 				continue;
@@ -233,13 +257,19 @@  static int sanitize_domain(const struct sbi_platform *plat,
 	 * protect M-mode context and enforce checks on memory accesses.
 	 */
 	if (dom->next_mode != PRV_S &&
-	    dom->next_mode != PRV_U)
+	    dom->next_mode != PRV_U) {
+		sbi_printf("%s: %s invalid next booting stage mode 0x%lx\n",
+			   __func__, dom->name, dom->next_mode);
 		return SBI_EINVAL;
+	}
 
 	/* Check next address and next mode*/
 	if (!sbi_domain_check_addr(dom, dom->next_addr, dom->next_mode,
-				   SBI_DOMAIN_EXECUTE))
+				   SBI_DOMAIN_EXECUTE)) {
+		sbi_printf("%s: %s next booting stage addres 0x%lx can't "
+			   "execute\n", __func__, dom->name, dom->next_addr);
 		return SBI_EINVAL;
+	}
 
 	return 0;
 }
@@ -371,13 +401,20 @@  int sbi_domain_finalize(struct sbi_scratch *scratch, u32 cold_hartid)
 			 * Ensure that we have room for Domain Index to
 			 * HART ID mapping
 			 */
-			if (SBI_DOMAIN_MAX_INDEX <= domain_count)
+			if (SBI_DOMAIN_MAX_INDEX <= domain_count) {
+				sbi_printf("%s: No room for %s\n",
+					   __func__, dom->name);
 				return SBI_ENOSPC;
+			}
 
 			/* Sanitize discovered domain */
 			rc = sanitize_domain(plat, dom);
-			if (rc)
+			if (rc) {
+				sbi_printf("%s: sanity checks failed for"
+					   " %s (error %d)\n", __func__,
+					   dom->name, rc);
 				return rc;
+			}
 
 			/* Assign index to domain */
 			dom->index = domain_count++;
@@ -437,8 +474,12 @@  int sbi_domain_finalize(struct sbi_scratch *scratch, u32 cold_hartid)
 						dom->next_addr,
 						dom->next_mode,
 						dom->next_arg1);
-			if (rc)
+			if (rc) {
+				sbi_printf("%s: failed to start boot HART %d"
+					   " for %s (error %d)\n", __func__,
+					   dhart, dom->name, rc);
 				return rc;
+			}
 		}
 	}