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@xxxxxxxxxx> 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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
