Re: [Xen-merge] First 6 patches

["Martin J. Bligh" <mbligh@xxxxxxxxxx>]

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

>> 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)? 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.
 
M.

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