Re: [PATCH] Mini-OS update of events initialisation

[Grzegorz Milos <gm281@cam.ac.uk>]

[-- Attachment #1: Type: text/plain, Size: 2369 bytes --]

> We have been seeing the same problem with a 32bit Mini-OS guest.
> 
> I suspect the problem is in the initialisation order.  Looking at
> start_kernel() in kernel.c:
> 
>     arch_init(si);
> 
>     trap_init();
> 
>     /* ENABLE EVENT DELIVERY. This is disabled at start of day. */
>     __sti();
>     
>     <code omitted...>
> 
>     /* Set up events. */
>     init_events();
> 
> The function arch_init() registers hypervisor_callback, and __sti()
> enables events to be delivered.  Entry point hypervisor_callback is in
> the assembly code in x86_32.S which calls do_hypervisor_callback() in
> hypervisor.c, which in turn calls do_event() in events.c.

That's all correct so far.

> Suppose a callback occurs between the calls of __sti() and
> init_events().  The function do_event() calls the handler indirectly via
> the array ev_actions.  But ev_actions is initialised in init_events(),
> so if do_event() is called too early, the function pointer "handler" in
> ev_actions will still be 0 (default for static storage).  We start again
> at virtual address 0, which is the entry point of Mini-OS, but %esi will
> not now point to the start_info page.  I think this explains why Mini-OS
> sometimes gets "restarted", and when it does the start_info page seems
> to be garbage.

This seems to explain the "restart". However, my understanding was that 
  an event port has to be explicitly unmasked in order for any event to 
be delivered (i.e. that all event ports are masked by default). 
Unmasking is done by bind_evtchn() or bind_virq() in event.s. All calls 
to these functions happen after init_events() (in init_time(), 
init_console() and init_xenbus()). I guess the best way to verify if 
your scenario is correct is to put 'printk' in do_events(). Could you 
try this Melvin?

> I am not convinced that Grzegorz' patch closes the window of opportunity
> for a misplaced callback, but it does reduce it.  Shouldn't __sti() be
> after init_events(), not before it?

That certainly cannot hurt. I'm attaching a patch. Keir could you apply?
Patch description:
Events enabled after init_events() call. This guarantees that internal 
datastructures are set up correctly. Problem spotted/explained by Melvin 
Anderson and Micha Moffie.

Signed-off-by: Grzegorz Milos <gm281@cam.ac.uk>


> 
> Thanks are due to Micha Moffie, who came up with key insights.



[-- Attachment #2: mini-os_events.patch --]
[-- Type: text/plain, Size: 718 bytes --]

diff -r f7cc9ce481d9 -r 669ea1ab20d4 extras/mini-os/kernel.c
--- a/extras/mini-os/kernel.c	Wed Nov 15 22:35:24 2006 +0000
+++ b/extras/mini-os/kernel.c	Fri Nov 17 15:22:54 2006 +0000
@@ -100,10 +100,7 @@ void start_kernel(start_info_t *si)
     arch_init(si);
 
     trap_init();
-
-    /* ENABLE EVENT DELIVERY. This is disabled at start of day. */
-    __sti();
-    
+   
     /* print out some useful information  */
     printk("Xen Minimal OS!\n");
     printk("start_info:   %p\n",    si);
@@ -118,6 +115,9 @@ void start_kernel(start_info_t *si)
 
     /* Set up events. */
     init_events();
+ 
+    /* ENABLE EVENT DELIVERY. This is disabled at start of day. */
+    __sti();
     
     arch_print_info();
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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