Re: [patch 17/24] Xen-paravirt_ops: avoid having a bad selector in %gs during context switch

Re: [patch 17/24] Xen-paravirt_ops: avoid having a bad selector in %gs during context switch

[Andi Kleen]

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

Re: [patch 17/24] Xen-paravirt_ops: avoid having a bad selector in %gs during context switch

[Jeremy Fitzhardinge]

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

Re: [patch 17/24] Xen-paravirt_ops: avoid having a bad selector in %gs during context switch

[Keir Fraser]

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

Re: [Xen-devel] Re: [patch 17/24] Xen-paravirt_ops: avoid having a bad selector in %gs during context switch

[Zachary Amsden]

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

Re: [Xen-devel] Re: [patch 17/24] Xen-paravirt_ops: avoid having a bad selector in %gs during context switch

[Jeremy Fitzhardinge]

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

Re: [patch 05/24] Xen-paravirt_ops: Add pagetable accessors to pack and unpack pagetable entries

[Andi Kleen]

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

Re: [patch 05/24] Xen-paravirt_ops: Add pagetable accessors to pack and unpack pagetable entries

[Jeremy Fitzhardinge]

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

Re: [patch 05/24] Xen-paravirt_ops: Add pagetable accessors to pack and unpack pagetable entries

[Rusty Russell]

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.

Re: [Xen-devel] Re: [patch 05/24] Xen-paravirt_ops: Add pagetable accessors to pack and unpack pagetable entries

[Jeremy Fitzhardinge]

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

Re: [patch 18/24] Xen-paravirt_ops: Some generic early printk & boot console fixups

[Andrew Morton]

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?

Re: [patch 18/24] Xen-paravirt_ops: Some generic early printk & boot console fixups

[Jeremy Fitzhardinge]

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

Re: [patch 18/24] Xen-paravirt_ops: Some generic early printk & boot console fixups

[Gerd Hoffmann]

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>