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

[Anup Patel]

> -----Original Message-----
> From: Anup Patel <Anup.Patel at wdc.com>
> Sent: 04 December 2020 21:22
> To: Atish Patra <Atish.Patra at wdc.com>; Alistair Francis
> <Alistair.Francis at wdc.com>
> Cc: Anup Patel <anup at brainfault.org>; opensbi at lists.infradead.org; Anup
> Patel <Anup.Patel at wdc.com>; Alistair Francis <Alistair.Francis at 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 at wdc.com>
> Reviewed-by: Alistair Francis <alistair.francis at wdc.com>
> Reviewed-by: Atish Patra <atish.patra at 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