Mailing List Archive

* Martin J. Bligh (mbligh@mbligh.org) wrote:
> OK, this is where I got to today, between meetings ;-). I'm fairly sure
> none of it works properly yet, not even test compiled, but I'll polish
> it up tommorow. This is the basic idea I'm on though, so if people
> disagree, please yell ASAP (I'm going to bed for now though ;-))
>
> Chris, don't bother trying to stack on top of this until I've at least
> got it compile tested and booted on non-xen. These are against 2.6.12
> and should form the bottom of the stack ...

> Pointing out my stupid mistakes is welcome, but this is more to garner
> feedback on the approach.
>
> 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.

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

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

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

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

> I looked at io_apic.c and ... well, I'll need to go back when I'm
> feeling braver ;-) Will knock through a bunch more tommorow, I hope.

Here's to your bravery ;-)

thanks,
-chris

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

On Thu, Aug 04, 2005 at 11:58:14PM -0700, Chris Wright wrote:
> +#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.

a user_mode(regs) macro is already is 2.6.13-rc4.
you can backport it (I submitted it for 2.6.12, so not really a backport),
that would not be too much problem when applying a 2.6.13 patch.

--
Vincent Hanquez

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

> a user_mode(regs) macro is already is 2.6.13-rc4.
> you can backport it (I submitted it for 2.6.12, so not really a backport),
> that would not be too much problem when applying a 2.6.13 patch.

In general I think this merge should be done on 2.6.13-rc*, not 2.6.12.

-Andi

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

On Fri, Aug 05, 2005 at 01:43:03PM +0200, Vincent Hanquez wrote:
> a user_mode(regs) macro is already is 2.6.13-rc4.
> you can backport it (I submitted it for 2.6.12, so not really a backport),
> that would not be too much problem when applying a 2.6.13 patch.

as well, there's some useful macros to load and set debugs that are in now.
(there's some vmware patches too that add some idt and ldt macros, that
are probably going to get in too)

it's probably best to applied thoses to 2.6.12, to have a painfree
upgrade to 2.6.13.

Cheers,
--
Vincent Hanquez

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

On Fri, Aug 05, 2005 at 01:46:56PM +0200, Andi Kleen wrote:
> In general I think this merge should be done on 2.6.13-rc*, not 2.6.12.

Hi Andi,

The problem here is that we don't have a working 2.6.13 tree yet.
I've done some work on it (-rc3 actually), but it's still not ready
for public comsumption ;)

--
Vincent Hanquez

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

On Fri, Aug 05, 2005 at 01:59:56PM +0200, Vincent Hanquez wrote:
> On Fri, Aug 05, 2005 at 01:46:56PM +0200, Andi Kleen wrote:
> > In general I think this merge should be done on 2.6.13-rc*, not 2.6.12.
>
> Hi Andi,
>
> The problem here is that we don't have a working 2.6.13 tree yet.
> I've done some work on it (-rc3 actually), but it's still not ready
> for public comsumption ;)

Well, but that would be the first step of the merge. I don't think
it makes sense to generate any other patches without that.

-Andi

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

>> 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@lists.xensource.com
http://lists.xensource.com/xen-merge

* Martin J. Bligh (mbligh@mbligh.org) 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@lists.xensource.com
http://lists.xensource.com/xen-merge

* Chris Wright (chrisw@osdl.org) wrote:
> It's for distinguishing kernelspace from userspace. And as Vincent
> reminds us, it's already been handled upstream, with user_mode() macro.

Err, I'm a bonehead, but I've already done per subarch user_mode()
as well. But it includes VM_MASK check as well as cs check. I'll redo
it to be the same as upstream.

I'm breaking the patch down, and will resend in pieces in a bit. Should
be much easier to look at a trickle rather than the firehose ;-)

thanks,
-chris

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

On Fri, Aug 05, 2005 at 01:52:52PM +0200, Vincent Hanquez wrote:
> On Fri, Aug 05, 2005 at 01:43:03PM +0200, Vincent Hanquez wrote:
> > a user_mode(regs) macro is already is 2.6.13-rc4.
> > you can backport it (I submitted it for 2.6.12, so not really a backport),
> > that would not be too much problem when applying a 2.6.13 patch.
>
> as well, there's some useful macros to load and set debugs that are in now.
> (there's some vmware patches too that add some idt and ldt macros, that
> are probably going to get in too)

Feel free to apply the user_mode and debug reg changes to our sparse tree.
We generally don't want to pick up rc changes except if they are changes
which we have in a similar form already in our sparse tree anyway.

> it's probably best to applied thoses to 2.6.12, to have a painfree
> upgrade to 2.6.13.

yes.

christian


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