Re: [Xen-merge] First 6 patches

[Chris Wright <chrisw@xxxxxxxx>]

* Martin J. Bligh (mbligh@xxxxxxxxxx) wrote:
> >> http://mbligh.org/xen/001-xen-cpu_common.c
> > 
> > I still think this one needs further refinement.  Most of that stuff can
> > be factored out.
> 
> Agreed, but it's a question of how much we have to do before merge.
> I'll come back to it at the end of the cycle if need be.
>  
> >> http://mbligh.org/xen/002-xen-i386_ksyms.c
> > 
> > This one turns out to be moot, just make it unconditionally conditional ;-)
> > (it's already moved upstream to a location that we'll override anyway).
> 
> Don't know enough about why that was done to comment. I can see
> people w/o ACPI using APM or something, but it's not my area. If you're
> sure, we can just do that, but need a justification for it somehow.

I think it's leftover from the fact that the symbol is undefined if
it's not dom0.  May have broken compilation w/out in domU kernels (just
a guess).

> >> http://mbligh.org/xen/003-ioport.c
> > 
> > When I had looked at this one a while back, I preferred to do it with
> > some generic helpers to keep from splitting it.  That was only for one
> > reason...it's quite security sensitive area (albeit rarely changing)
> > and had wanted to keep it in a single file as much as possible.
> 
> That's going to make a huge mess -  there seemed to be a lot of changes
> in that file - only set_bitmap was left unscathed ...
> 
> >> http://mbligh.org/xen/004-ldt.c
> > 
> > This one is nice.
> > 
> >> http://mbligh.org/xen/005-pci-dma.c
> > 
> > Couple things here.  Typo (inlinee).  Header should be ASM_MACH...
> > And you can't add dma_map_$foo here like that.  They're already in
> > header files, and in fact I already did this one in the header cleanup.
> > The normal bits are now in mach-default/mach_dma_mapping.h as static inlines
> > as they already were.  And the xen bits are non-inlined (declared only)
> > in mach-xen/mach_dma_mapping.h.  And I don't think xen_contig_memory
> > should be in global namespace.  Finally, sidenote (a latent bug, not your
> > fault), dma_map_single calls kmalloc(GFP_KERNEL) but can be called from
> > contexts where it can't sleep.
> 
> OK, I'll revisit this.
>  
> >> http://mbligh.org/xen/006-signal.c
> > 
> > +#ifdef CONFIG_XEN
> > +   if ((regs->xcs & 2) != 2)
> > +#else
> >     if ((regs->xcs & 3) != 3)
> > +#endif
> > 
> > That change is all over the place.  That should be a macro value that's
> > defined by the arch.  The only reason I didn't do that in the other
> > patch I submitted is I couldn't think of a good name.  KERNEL_CS_PL?
> > Something...at any rate, change should only be in header.  Also, I
> > missed why the breakout of sigframe.h?  They are identical?  It's an
> > unncessary change.
> 
> Sure, that was a late-night quick-hack ;-) What exactly is the underlying
> change here for (2 vs 3)?

It's for distinguishing kernelspace from userspace.  And as Vincent
reminds us, it's already been handled upstream,  with user_mode() macro.

> sigframe.h is being moved from arch/i386 to 
> include/asm-i386 just because having header files in the wrong place is 
> morally offensive. Was a generic cleanup that maybe should have been 
> split out.

I figured as much.  I don't think it's worth it.  In fact, folks will
argue that local only headers should stay in local directories.  Anyway,
let's drop that bit.

thanks,
-chris

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