[U-Boot] [PATCH v3 6/7] ARM: extend non-secure switch to also go into HYP mode

[Masahiro Yamada]

Hello Andre,



> -#ifdef CONFIG_ARMV7_NONSEC
> +#if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
>  
>  enum nonsec_virt_errors {
>  	NONSEC_VIRT_SUCCESS,
>  	NONSEC_ERR_NO_SEC_EXT,
>  	NONSEC_ERR_NO_GIC_ADDRESS,
>  	NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB,
> +	VIRT_ALREADY_HYP_MODE,
> +	VIRT_ERR_NO_VIRT_EXT,
> +	VIRT_ERR_NOT_HYP_MODE
>  };

This looks weird to me.
If you want to use enum, why don't you separate it like follows? 

enum nonsec_errors {
	NONSEC_SUCCESS,
	NONSEC_ERR_NO_SEC_EXT,
	NONSEC_ERR_NO_GIC_ADDRESS,
	NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB,
};

enum virt_errors {
	VIRT_SUCCESS,
	VIRT_ALREADY_HYP_MODE,
	VIRT_ERR_NO_VIRT_EXT,
	VIRT_ERR_NOT_HYP_MODE
};


>  enum nonsec_virt_errors armv7_switch_nonsec(void);
> +enum nonsec_virt_errors armv7_switch_hyp(void);

enum nonsec_errors armv7_switch_nonsec(void);
+enum virt_errors armv7_switch_hyp(void);


But in the first place,
I like better to fix do_nonsec_virt_switch().



>  static void do_nonsec_virt_switch(void)
>  {
> -#ifdef CONFIG_ARMV7_NONSEC
> +#if defined(CONFIG_ARMV7_NONSEC) || defined(CONFIG_ARMV7_VIRT)
>  	int ret;
>  
>  	ret = armv7_switch_nonsec();
> +#ifdef CONFIG_ARMV7_VIRT
> +	if (ret == NONSEC_VIRT_SUCCESS)
> +		ret = armv7_switch_hyp();
> +#endif
>  	switch (ret) {
>  	case NONSEC_VIRT_SUCCESS:
> -		debug("entered non-secure state\n");
> +		debug("entered non-secure state or HYP mode\n");
>  		break;
>  	case NONSEC_ERR_NO_SEC_EXT:
>  		printf("nonsec: Security extensions not implemented.\n");
> @@ -209,6 +213,15 @@ static void do_nonsec_virt_switch(void)
>  	case NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB:
>  		printf("nonsec: PERIPHBASE is above 4 GB, no access.\n");
>  		break;
> +	case VIRT_ERR_NO_VIRT_EXT:
> +		printf("HYP mode: Virtualization extensions not implemented.\n");
> +		break;
> +	case VIRT_ALREADY_HYP_MODE:
> +		debug("CPU already in HYP mode\n");
> +		break;
> +	case VIRT_ERR_NOT_HYP_MODE:
> +		printf("HYP mode: switch not successful.\n");
> +		break;
>  	}
>  #endif


As for this part, I agreed on Christoffer's opinion:


On Tue, 30 Jul 2013 07:23:58 -0700
Christoffer Dall <christoffer.dall at linaro.org> wrote:
> I won't hold back my ack for the patch series based on this, but I do
> think it's over-engineered.  I think at least just returning -1 for
> error and 0 for success (or even make it a bool) and just printing a
> generic error message is cleaner - the level of details as to why the
> switch to hyp/nonsec didn't work could then be debug statements that a
> board developer could enable with a "#define DEBUG 1" in the
> corresponding file.




Best Regards
Masahiro Yamada