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");@@ -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@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