On Thu, Jan 10, 2008 at 06:31:00PM +0000, Andrew Doran wrote:
> 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:

thanks !

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

But is it worth it for a few lines of assembly ? Or maybe we should have
locore.S, locore_i386.S and locore_xen.S ? At last the startup
code could be moved. If so, should locore_xen.S be in arch/i386/i386
or arch/xen/i386 ?


> 
> > +#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.

I think so. I think I tried to use GUFS/GUGS_SEL, but this didn't work.
I didn't investicate more, as the current code doesn't use it either
for now.


> 
> > @@ -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.

Yes, and it's true for some functions in amd64 too. But I'd prefer to do
it 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? :-)

Yes. But I wouldn't mind removing the ifdef and always have [2].
Or use a #define to a different value for Xen and native.

-- 
Manuel Bouyer <bouyer@antioche.eu.org>
     NetBSD: 26 ans d'experience feront toujours la difference
--