Re: [XenPPC] [PATCH 2/2] linux: build start_info_t from devtree only

[Ryan Harper <ryanh@xxxxxxxxxx>]

* Jimi Xenidis <jimix@xxxxxxxxxxxxxx> [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  
> >start_info_t
> >for dom0.
> >
> >-- 
> >Ryan Harper
> >Software Engineer; Linux Technology Center
> >IBM Corp., Austin, Tx
> >(512) 838-9253   T/L: 678-9253
> >ryanh@xxxxxxxxxx
> >
> >
> >diffstat output:
> > setup.c |   33 +++++++++++++++------------------
> > 1 files changed, 15 insertions(+), 18 deletions(-)
> >
> >Signed-off-by: Ryan Harper <ryanh@xxxxxxxxxx>
> >---
> >diff -r a6adf094e08e arch/powerpc/platforms/xen/setup.c
> >--- a/arch/powerpc/platforms/xen/setup.c     Tue Feb 06 13:55:48 2007  
> >-0600
> >+++ b/arch/powerpc/platforms/xen/setup.c     Wed Feb 07 11:33:10 2007  
> >-0600
> >@@ -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, 
the

   xen_start_info = &xsi 

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

> >+
> >+    /* 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.  

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

_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel