On Wed, Jan 09, 2008 at 08:34:12PM +0100, Manuel Bouyer wrote:

> I'm almost done merging xen support back in arch/i386, in the same way it
> was done for amd64 Xen supoort (there's much, much less duplicate code this
> way). There's only spl.S and vector.S remaining, which I hope to handle this
> evening, as well as reworking a bit the xen/i386 config files to match amd64
> (especially, introduce a arch/xen/include/i386 directory and move some files
> here).
> 
> 
> I'd like to merge the branch to HEAD ASAP, possibly tomorow evening.
> Once done, I'll keep the branch active to work on x86PAE suport for
> Xen. If you have a problem with the content of the branch or the merge to
> HEAD, please tell !

A few comments:

>+/*
> + * void lgdt_finish(void);
> + * Finish load a new GDT pointer (do any necessary cleanup).
> + * XXX It's somewhat questionable whether reloading all the segment registers
> + * is necessary, since the actual descriptor data is not changed except by
> + * process creation and exit, both of which clean up via task switches.  OTOH,
> + * this only happens at run time when the GDT is resized.
> + */
> +/* LINTSTUB: Func: void lgdt_finish(void) */
> +NENTRY(lgdt_finish)
> +       movl    $GSEL(GDATA_SEL, SEL_KPL),%eax
> +       movw    %ax,%ds
> +       movw    %ax,%es
> +       movw    %ax,%gs
> +       movw    %ax,%ss
> +       movl    $GSEL(GCPU_SEL, SEL_KPL),%eax
> +       movw    %ax,%fs
> +       /* Reload code selector by doing intersegment return. */
> +       popl    %eax
> +       pushl   $GSEL(GCODE_SEL, SEL_KPL)
> +       pushl   %eax
> +       lret

Following the convention this one belongs in xenfunc.S (although I realise
there is a xenfunc.c which IIRC I added).

> +#ifndef XEN
>         tf->tf_gs = GSEL(GUGS_SEL, SEL_UPL);
>         tf->tf_fs = GSEL(GUFS_SEL, SEL_UPL);
> +#else
> +       tf->tf_gs = GSEL(GUDATA_SEL, SEL_UPL);
> +       tf->tf_fs = GSEL(GUDATA_SEL, SEL_UPL);
> +#endif
 
Xen should use GUFS/GUGS_SEL too. They aren't updated in cpu_switchto()
yet - it looks like a couple of hypervisor calls would be needed.

> @@ -2117,10 +2365,13 @@
>  void
>  cpu_reset()
>  {
> +#ifdef XEN
> +       HYPERVISOR_reboot();
> +       for (;;);
> +#else /* XEN */
>         struct region_descriptor region;
>  
>         x86_disable_intr();
> -
>  #ifdef XBOX
>         if (arch_i386_is_xbox) {
>                 xbox_reboot();
> @@ -2184,6 +2435,7 @@
>  #endif
>  
>         for (;;);
> +#endif /* XEN */
>  }
 
Where entire functions are different it would be nice to put them in different
files. But, I don't mind, it can be done later.

> +#ifdef XEN
> +                       pmap_kenter_ma(eva, pa, VM_PROT_READ);
> +#else
>                         pmap_kenter_pa(eva, pa, VM_PROT_READ);
> +#endif
> 
> +#ifndef XEN
> +int gdt_size[1];       /* total number of GDT entries */
> +int gdt_count[1];      /* number of GDT entries in use */
> +int gdt_next[1];       /* next available slot for sweeping */
> +int gdt_free[1];       /* next free slot; terminated with GNULL_SEL */
> +#else
> +int gdt_size[2];       /* total number of GDT entries */
> +int gdt_count[2];      /* number of GDT entries in use */
> +int gdt_next[2];       /* next available slot for sweeping */
> +int gdt_free[2];       /* next free slot; terminated with GNULL_SEL */
> +#endif
 
Why not [2] on both? Saves a few bytes? :-)

Thanks,
Andrew