On Tue, Jul 30, 2013 at 01:32:14PM +0200, Andre Przywara wrote:
> On 07/30/2013 12:02 AM, Christoffer Dall wrote:
> >On Wed, Jul 10, 2013 at 01:54:16AM +0200, Andre Przywara wrote:
> >
> >[...]
> >
> >>diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> >>index 1b6e0ac..7b0619e 100644
> >>--- a/arch/arm/lib/bootm.c
> >>+++ b/arch/arm/lib/bootm.c
> >>@@ -34,6 +34,10 @@
> >> #include <asm/bootm.h>
> >> #include <linux/compiler.h>
> >>
> >>+#ifdef CONFIG_ARMV7_NONSEC
> >>+#include <asm/armv7.h>
> >>+#endif
> >>+
> >> DECLARE_GLOBAL_DATA_PTR;
> >>
> >> static struct tag *params;
> >>@@ -186,6 +190,29 @@ static void setup_end_tag(bd_t *bd)
> >>
> >> __weak void setup_board_tags(struct tag **in_params) {}
> >>
> >>+static void do_nonsec_virt_switch(void)
> >>+{
> >>+#ifdef CONFIG_ARMV7_NONSEC
> >>+ int ret;
> >>+
> >>+ ret = armv7_switch_nonsec();
> >>+ switch (ret) {
> >>+ case NONSEC_VIRT_SUCCESS:
> >>+ debug("entered non-secure state\n");
> >>+ break;
> >>+ case NONSEC_ERR_NO_SEC_EXT:
> >>+ printf("nonsec: Security extensions not implemented.\n");
> >>+ break;
> >>+ case NONSEC_ERR_NO_GIC_ADDRESS:
> >>+ printf("nonsec: could not determine GIC address.\n");
> >>+ break;
> >>+ case NONSEC_ERR_GIC_ADDRESS_ABOVE_4GB:
> >>+ printf("nonsec: PERIPHBASE is above 4 GB, no access.\n");
> >>+ break;
> >>+ }
> >>+#endif
> >>+}
> >
> >I still don't get why you just don't make armv7_switch_nonsec a void and
> >print the error when they occur... ???
>
> My apologies for not elaborating on these comments I didn't incorporate:
>
> So, I don't like the idea of marrying a low-level routine with high
> level output. I don't want to constraint the usage of the routine by
> requiring an output channel. Also some parts may not be fatal for
> all users - someone could just try to switch and then behave
> differently if that failed - without bothering the user.
> May seem a bit over-engineered, but I like it better this way ;-)
>
> If that is a show-stopper for you, I can change it, of course.
>
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.
But ok, we've had the conversation, if you still feel this is better and
necessary, then I'll let it be.
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
