On Feb 8, 2007, at 8:45 AM, Ryan Harper wrote:

* Jimi Xenidis <[EMAIL PROTECTED]> [2007-02-08 06:48]:
some comments
On Feb 7, 2007, at 6:34 PM, Ryan Harper wrote:

This patch cleans up xen_init_early() to construct a start_info_t only
from the devtree as Patch1 fixes xen to no longer create a
for dom0.

Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
(512) 838-9253   T/L: 678-9253

diffstat output:
setup.c |   33 +++++++++++++++------------------
1 files changed, 15 insertions(+), 18 deletions(-)

Signed-off-by: Ryan Harper <[EMAIL PROTECTED]>
diff -r a6adf094e08e arch/powerpc/platforms/xen/setup.c
--- a/arch/powerpc/platforms/xen/setup.c        Tue Feb 06 13:55:48 2007
+++ b/arch/powerpc/platforms/xen/setup.c        Wed Feb 07 11:33:10 2007
@@ -88,29 +88,26 @@ static void __init xen_init_early(void)
static void __init xen_init_early(void)
        struct device_node *xen;
-       u64 *si;

        DBG(" -> %s\n", __func__);

        xen = of_find_node_by_path("/xen");

-       si = (u64 *)get_property(xen, "start-info", NULL);
-       /* build our own start_info_t if start-info property is not
present */
-       if (si != NULL) {
-               xen_start_info = (start_info_t *)__va(*si);
-       } else {
-               struct device_node *console;
-               struct device_node *store;
-               console = of_find_node_by_path("/xen/console");
-               store = of_find_node_by_path("/xen/store");
-               xen_start_info = &xsi;
-               /* fill out start_info_t from devtree */
-               xen_start_info->shared_info = *((u64 *)get_property(xen,
-                  "shared-info", NULL));
+       xen_start_info = &xsi;

Please make xsi static in its declaration earlier in the file.

Sure.  And while I wanted to access xsi vai xen_start_info,

   xen_start_info = &xsi

seemed a bit awkward.  Not sure if I should switch to xsi.  Thoughts?

There is too much code referring to xen_start_info as a pointer.
perhaps making that static __xen_start_info, so its obvious that we are just suing it for memory allocation, at some point when all references are gone we can flatten this out.

+       /* fill out start_info_t from devtree */
+       if ((char *)get_property(xen, "privileged", NULL))
+               xen_start_info->flags |= SIF_PRIVILEGED;
+       if ((char *)get_property(xen, "initdomain", NULL))
+               xen_start_info->flags |= SIF_INITDOMAIN;
+       xen_start_info->shared_info = *((u64 *)get_property(xen,
+          "shared-info", NULL));
+       /* only look for store and console for guest domains */

Hmm, I think you need to look for them always.
You _at_least_ need the console evtchn, which may not be zero and we
create the node/prop anyway.

Hrm, you may be right. I know that dom0 boots fine with this, but that
maybe because it defaults those values to 0.  I'll kill the if().

Feel free to "panic()" more:
NOTE: this is early so a "udbg_printf(); HYPERVISOR_shutdown
(SHUTDOWN_poweroff);" will do cuz panic() is no available yet.

Yeah, good idea though none of the messages get out if our shared_info
page isn't setup correctly, which I learned during my testing of this
patch, was the value most likely to get hosed.

Oh yeah, you probably want to:
        HYPERVISOR_shared_info = ...;
        xen_start_info->flags |= SIF_INITDOMAIN; # or not

As soon as you can, before you check everything else.

If you want more info earlier you can always build with:


Xen-ppc-devel mailing list

Reply via email to