LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Re: [patch 17/24] Xen-paravirt_ops: avoid having a bad selector in %gs during context switch
       [not found] ` <20070221205323.770169136@goop.org>
@ 2007-02-21 22:10   ` Andi Kleen
  2007-02-21 22:14     ` Jeremy Fitzhardinge
                       ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Andi Kleen @ 2007-02-21 22:10 UTC (permalink / raw)
  To: virtualization
  Cc: Jeremy Fitzhardinge, Chris Wright, xen-devel, Andrew Morton,
	linux-kernel


>  	/*
> +	 * Temporary hack: zero gs now that we've saved it so that Xen
> +	 * doesn't try to reload the old value after changing the GDT
> +	 * during the context switch.  This can go away once Xen has
> +	 * been taught to only reload %gs when it absolutely must.
> +	 */
> +	loadsegment(gs, 0);

Sorry, but i don't really want that unconditionally in the context switch.
Adding a paravirt ops for it would be also ugly. Can Xen be fixed?

-Andi

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch 17/24] Xen-paravirt_ops: avoid having a bad selector in %gs during context switch
  2007-02-21 22:10   ` [patch 17/24] Xen-paravirt_ops: avoid having a bad selector in %gs during context switch Andi Kleen
@ 2007-02-21 22:14     ` Jeremy Fitzhardinge
  2007-02-21 22:16     ` Keir Fraser
  2007-02-21 23:25     ` [Xen-devel] " Zachary Amsden
  2 siblings, 0 replies; 12+ messages in thread
From: Jeremy Fitzhardinge @ 2007-02-21 22:14 UTC (permalink / raw)
  To: Andi Kleen
  Cc: virtualization, Chris Wright, xen-devel, Andrew Morton, linux-kernel

Andi Kleen wrote:
> Sorry, but i don't really want that unconditionally in the context switch.
> Adding a paravirt ops for it would be also ugly. Can Xen be fixed?
>   

Yes.  I'm happy to drop this one.

    J

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch 05/24] Xen-paravirt_ops: Add pagetable accessors to pack and unpack pagetable entries
       [not found] ` <20070221205322.869165491@goop.org>
@ 2007-02-21 22:15   ` Andi Kleen
  2007-02-21 22:20     ` Jeremy Fitzhardinge
  2007-02-21 23:20     ` Rusty Russell
  0 siblings, 2 replies; 12+ messages in thread
From: Andi Kleen @ 2007-02-21 22:15 UTC (permalink / raw)
  To: virtualization
  Cc: Jeremy Fitzhardinge, Chris Wright, xen-devel, Andrew Morton,
	linux-kernel

On Wednesday 21 February 2007 21:52, Jeremy Fitzhardinge wrote:
> Add a set of accessors to pack, unpack and modify page table entries
> (at all levels).  This allows a paravirt implementation to control the
> contents of pgd/pmd/pte entries.  For example, Xen uses this to
> convert the (pseudo-)physical address into a machine address when
> populating a pagetable entry, and converting back to pphys address
> when an entry is read.

Do you have some lmbench numbers before/after this change? 
iirc at least fork and exit do a lot of pte accesses in various forms.
If it's measurable it might be needed to patch those for the native case.

-Andi

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch 17/24] Xen-paravirt_ops: avoid having a bad selector in %gs during context switch
  2007-02-21 22:10   ` [patch 17/24] Xen-paravirt_ops: avoid having a bad selector in %gs during context switch Andi Kleen
  2007-02-21 22:14     ` Jeremy Fitzhardinge
@ 2007-02-21 22:16     ` Keir Fraser
  2007-02-21 23:25     ` [Xen-devel] " Zachary Amsden
  2 siblings, 0 replies; 12+ messages in thread
From: Keir Fraser @ 2007-02-21 22:16 UTC (permalink / raw)
  To: Andi Kleen, virtualization
  Cc: Chris Wright, xen-devel, Andrew Morton, linux-kernel

On 21/2/07 22:10, "Andi Kleen" <ak@suse.de> wrote:

>> /*
>> +  * Temporary hack: zero gs now that we've saved it so that Xen
>> +  * doesn't try to reload the old value after changing the GDT
>> +  * during the context switch.  This can go away once Xen has
>> +  * been taught to only reload %gs when it absolutely must.
>> +  */
>> + loadsegment(gs, 0);
> 
> Sorry, but i don't really want that unconditionally in the context switch.
> Adding a paravirt ops for it would be also ugly. Can Xen be fixed?

This is not an issue for correctness so this can safely be removed and still
work with current Xen.

 -- Keir



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch 05/24] Xen-paravirt_ops: Add pagetable accessors to pack and unpack pagetable entries
  2007-02-21 22:15   ` [patch 05/24] Xen-paravirt_ops: Add pagetable accessors to pack and unpack pagetable entries Andi Kleen
@ 2007-02-21 22:20     ` Jeremy Fitzhardinge
  2007-02-21 23:20     ` Rusty Russell
  1 sibling, 0 replies; 12+ messages in thread
From: Jeremy Fitzhardinge @ 2007-02-21 22:20 UTC (permalink / raw)
  To: Andi Kleen
  Cc: virtualization, Chris Wright, xen-devel, Andrew Morton, linux-kernel

Andi Kleen wrote:
> Do you have some lmbench numbers before/after this change? 
> iirc at least fork and exit do a lot of pte accesses in various forms.
> If it's measurable it might be needed to patch those for the native case.
>   

I don't.  I think Rusty ran some numbers and found the pte accessors
became the most commonly used paravirt ops.

I'm working on revamping the pv_ops patching infrastructure at the
moment, so we should be able to deal with these fairly efficiently.

    J

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch 05/24] Xen-paravirt_ops: Add pagetable accessors to pack and unpack pagetable entries
  2007-02-21 22:15   ` [patch 05/24] Xen-paravirt_ops: Add pagetable accessors to pack and unpack pagetable entries Andi Kleen
  2007-02-21 22:20     ` Jeremy Fitzhardinge
@ 2007-02-21 23:20     ` Rusty Russell
  2007-02-21 23:27       ` [Xen-devel] " Jeremy Fitzhardinge
  1 sibling, 1 reply; 12+ messages in thread
From: Rusty Russell @ 2007-02-21 23:20 UTC (permalink / raw)
  To: Andi Kleen
  Cc: virtualization, Chris Wright, xen-devel, Andrew Morton, linux-kernel

On Wed, 2007-02-21 at 23:15 +0100, Andi Kleen wrote:
> On Wednesday 21 February 2007 21:52, Jeremy Fitzhardinge wrote:
> > Add a set of accessors to pack, unpack and modify page table entries
> > (at all levels).  This allows a paravirt implementation to control the
> > contents of pgd/pmd/pte entries.  For example, Xen uses this to
> > convert the (pseudo-)physical address into a machine address when
> > populating a pagetable entry, and converting back to pphys address
> > when an entry is read.
> 
> Do you have some lmbench numbers before/after this change? 
> iirc at least fork and exit do a lot of pte accesses in various forms.
> If it's measurable it might be needed to patch those for the native case.

Yes, __mkpte must be patched to avoid performance embarrassment.

Jeremy, did you want me to do this, or are you happy to?
Rusty.
PS. I really must revise my "paravirt-ops counter" patch which tallies
how much each op is getting called.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Xen-devel] Re: [patch 17/24] Xen-paravirt_ops: avoid having a bad selector in %gs during context switch
  2007-02-21 22:10   ` [patch 17/24] Xen-paravirt_ops: avoid having a bad selector in %gs during context switch Andi Kleen
  2007-02-21 22:14     ` Jeremy Fitzhardinge
  2007-02-21 22:16     ` Keir Fraser
@ 2007-02-21 23:25     ` Zachary Amsden
  2007-02-21 23:29       ` Jeremy Fitzhardinge
  2 siblings, 1 reply; 12+ messages in thread
From: Zachary Amsden @ 2007-02-21 23:25 UTC (permalink / raw)
  To: Andi Kleen
  Cc: virtualization, Chris Wright, Jeremy Fitzhardinge, xen-devel,
	Andrew Morton, linux-kernel

Andi Kleen wrote:
>>  	/*
>> +	 * Temporary hack: zero gs now that we've saved it so that Xen
>> +	 * doesn't try to reload the old value after changing the GDT
>> +	 * during the context switch.  This can go away once Xen has
>> +	 * been taught to only reload %gs when it absolutely must.
>> +	 */
>> +	loadsegment(gs, 0);
>>     
>
> Sorry, but i don't really want that unconditionally in the context switch.
> Adding a paravirt ops for it would be also ugly. Can Xen be fixed?
>   

I agree with that, but especially because this is not even the right 
place to save and clear gs; when userspace uses an LDT based %gs, you 
need to do this all the way back in mmu_context.h before you switch the 
LDT out.  We have the same bug, but I was loathe to try to fix it yet, 
since it could mean some strange code movement.  The awkward bit is that 
switch_mm doesn't have the pointer to the previous task, only it's mm, 
so there is no place to save gs.  Perhaps it is time to move the LDT 
switch into switch_to instead of switch_mm?  It's not like there are 
different per-process LDTs in the fixmap or something, which I could see 
as a valid reason to associate this with the mm switch.  But all process 
LDTs are global, so there isn't an issue.

And I noticed this martian hanging out in mmu_context.h:

#define deactivate_mm(tsk, mm)                  \
        asm("movl %0,%%gs": :"r" (0));

Zach

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Xen-devel] Re: [patch 05/24] Xen-paravirt_ops: Add pagetable accessors to pack and unpack pagetable entries
  2007-02-21 23:20     ` Rusty Russell
@ 2007-02-21 23:27       ` Jeremy Fitzhardinge
  0 siblings, 0 replies; 12+ messages in thread
From: Jeremy Fitzhardinge @ 2007-02-21 23:27 UTC (permalink / raw)
  To: Rusty Russell
  Cc: Andi Kleen, Chris Wright, virtualization, Andrew Morton,
	xen-devel, linux-kernel

Rusty Russell wrote:
> On Wed, 2007-02-21 at 23:15 +0100, Andi Kleen wrote:
>   
>> On Wednesday 21 February 2007 21:52, Jeremy Fitzhardinge wrote:
>>     
>>> Add a set of accessors to pack, unpack and modify page table entries
>>> (at all levels).  This allows a paravirt implementation to control the
>>> contents of pgd/pmd/pte entries.  For example, Xen uses this to
>>> convert the (pseudo-)physical address into a machine address when
>>> populating a pagetable entry, and converting back to pphys address
>>> when an entry is read.
>>>       
>> Do you have some lmbench numbers before/after this change? 
>> iirc at least fork and exit do a lot of pte accesses in various forms.
>> If it's measurable it might be needed to patch those for the native case.
>>     
>
> Yes, __mkpte must be patched to avoid performance embarrassment.
>
> Jeremy, did you want me to do this, or are you happy to?
>   
I'm working on it now.  I'll mail out a proposal in a little bit.

    J

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Xen-devel] Re: [patch 17/24] Xen-paravirt_ops: avoid having a bad selector in %gs during context switch
  2007-02-21 23:25     ` [Xen-devel] " Zachary Amsden
@ 2007-02-21 23:29       ` Jeremy Fitzhardinge
  0 siblings, 0 replies; 12+ messages in thread
From: Jeremy Fitzhardinge @ 2007-02-21 23:29 UTC (permalink / raw)
  To: Zachary Amsden
  Cc: Andi Kleen, virtualization, Chris Wright, xen-devel,
	Andrew Morton, linux-kernel

Zachary Amsden wrote:
> I agree with that, but especially because this is not even the right
> place to save and clear gs; when userspace uses an LDT based %gs, you
> need to do this all the way back in mmu_context.h before you switch
> the LDT out. 

Yeah.  This patch was really just to shut my debug Xen build up.  
There's no correctness issue one way or the other, so it doesn't really
matter that it doesn't catch LDT-using processes.

> And I noticed this martian hanging out in mmu_context.h:
>
> #define deactivate_mm(tsk, mm)                  \
>        asm("movl %0,%%gs": :"r" (0));

Yeah, I've always wondered what this is for.  Its a remnant of a
remnant, I think.

    J

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch 18/24] Xen-paravirt_ops: Some generic early printk & boot console fixups
       [not found] ` <20070221205323.842917348@goop.org>
@ 2007-02-22  3:10   ` Andrew Morton
  2007-02-22  5:48     ` Jeremy Fitzhardinge
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Morton @ 2007-02-22  3:10 UTC (permalink / raw)
  To: Jeremy Fitzhardinge
  Cc: Andi Kleen, linux-kernel, virtualization, xen-devel,
	Chris Wright, Zachary Amsden, Rusty Russell

On Wed, 21 Feb 2007 12:53:12 -0800 Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> Some generic early printk & boot console fixups
> (already sent to lkml).

hm, this patch series seems to have gone out of its way to cause problems.

This particular (pathetically changelogged) patch is already in my tree,
only in a later, much more comprehensive form.  Similarly the HVC changes
were already in Rusty's stuff and were supposed to be merged by Paulus, but
haven't been yet.

Can we just drop this one?  Does the Xen work actually depend on it?


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch 18/24] Xen-paravirt_ops: Some generic early printk & boot console fixups
  2007-02-22  3:10   ` [patch 18/24] Xen-paravirt_ops: Some generic early printk & boot console fixups Andrew Morton
@ 2007-02-22  5:48     ` Jeremy Fitzhardinge
  2007-02-22  8:33       ` Gerd Hoffmann
  0 siblings, 1 reply; 12+ messages in thread
From: Jeremy Fitzhardinge @ 2007-02-22  5:48 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Andi Kleen, linux-kernel, virtualization, xen-devel,
	Chris Wright, Zachary Amsden, Rusty Russell

Andrew Morton wrote:
> hm, this patch series seems to have gone out of its way to cause problems.
>
> This particular (pathetically changelogged) patch is already in my tree,
> only in a later, much more comprehensive form.  Similarly the HVC changes
> were already in Rusty's stuff and were supposed to be merged by Paulus, but
> haven't been yet.
>
> Can we just drop this one?  Does the Xen work actually depend on it?

Well, some form of the hvc patches is needed to get a console.  I'll
rationalize them with respect to Gerd and Rusty's latest patches, but I
did want to make the series stand on its own rather than have external
dependencies on unmerged patches.

    J

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [patch 18/24] Xen-paravirt_ops: Some generic early printk & boot console fixups
  2007-02-22  5:48     ` Jeremy Fitzhardinge
@ 2007-02-22  8:33       ` Gerd Hoffmann
  0 siblings, 0 replies; 12+ messages in thread
From: Gerd Hoffmann @ 2007-02-22  8:33 UTC (permalink / raw)
  To: Jeremy Fitzhardinge
  Cc: Andrew Morton, Andi Kleen, linux-kernel, virtualization,
	xen-devel, Chris Wright, Zachary Amsden, Rusty Russell

Jeremy Fitzhardinge wrote:
> Andrew Morton wrote:
>> hm, this patch series seems to have gone out of its way to cause problems.
>>
>> This particular (pathetically changelogged) patch is already in my tree,
>> only in a later, much more comprehensive form.  Similarly the HVC changes
>> were already in Rusty's stuff and were supposed to be merged by Paulus, but
>> haven't been yet.
>>
>> Can we just drop this one?  Does the Xen work actually depend on it?

Yes.  The other which made it into -mm via lkml is a more recent version
of the same patch (+documentation, +!x86 arch updates).

On the hvm_console.c changes: there is just a small change needed to
make it build on !ppc, that should be identical identical in my (xen)
and rusty (lguest) patch series.  Patching it only once is fine for
obvious reasons ;)

cheers,
  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2007-02-22  8:33 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20070221205254.169835700@goop.org>
     [not found] ` <20070221205323.770169136@goop.org>
2007-02-21 22:10   ` [patch 17/24] Xen-paravirt_ops: avoid having a bad selector in %gs during context switch Andi Kleen
2007-02-21 22:14     ` Jeremy Fitzhardinge
2007-02-21 22:16     ` Keir Fraser
2007-02-21 23:25     ` [Xen-devel] " Zachary Amsden
2007-02-21 23:29       ` Jeremy Fitzhardinge
     [not found] ` <20070221205322.869165491@goop.org>
2007-02-21 22:15   ` [patch 05/24] Xen-paravirt_ops: Add pagetable accessors to pack and unpack pagetable entries Andi Kleen
2007-02-21 22:20     ` Jeremy Fitzhardinge
2007-02-21 23:20     ` Rusty Russell
2007-02-21 23:27       ` [Xen-devel] " Jeremy Fitzhardinge
     [not found] ` <20070221205323.842917348@goop.org>
2007-02-22  3:10   ` [patch 18/24] Xen-paravirt_ops: Some generic early printk & boot console fixups Andrew Morton
2007-02-22  5:48     ` Jeremy Fitzhardinge
2007-02-22  8:33       ` Gerd Hoffmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).