Re: [PATCH 10/11] x86: make lazy %gs optional on x86_32

[Tejun Heo <tj@kernel.org>]

Hello,

Jeremy Fitzhardinge wrote:
>> * Save and restore %gs along with other registers in entry_32.S unless
>>   LAZY_GS.  Note that this unfortunately adds "pushl $0" on SAVE_ALL
>>   even when LAZY_GS.  However, it adds no overhead to common exit path
>>   and simplifies entry path with error code.
>>   
> 
> I don't think it will make a measurable difference.  "subl $4, %esp"
> might be worth using too, or "lea -4(%esp), %esp" to avoid touching the
> flags.

You mean for PUSH_GS?  It's only used as a part of SAVE_ALL so I don't
think we need to worry about eflags.  The reason why I chose push $0
was because it was the shortest.

 push $0		: 6a 00
 sub $4, %esp		: 83 ec 04
 lea -4(%esp), %esp	: 8d 64 24 fc

>> +.macro POP_GS pop=0
>> +98:    popl %gs
>> +    CFI_ADJUST_CFA_OFFSET -4
>> +    /*CFI_RESTORE gs*/
>> +  .if \pop <> 0
>> +    add $\pop, %esp
>> +    CFI_ADJUST_CFA_OFFSET -\pop
>> +  .endif
>> +.endm
>> +.macro POP_GS_EX
>> +.pushsection .fixup, "ax"
>> +99:    movl $0, (%esp)
>> +    jmp 98b
>> +.section __ex_table, "a"
>> +    .align 4
>> +    .long 98b, 99b
>> +.popsection
>>   
> 
> Why not just fold the exception block into the POP_GS macro?  I don't
> think they need to be separated (ditto other exception handlers).

I originally did that but in ia32_sysenter_target(), it seems that the
exception stuff should be after CFI_ENDPROC, so I split them.  Don't
know much about debugging info so I tried to stick with what's already
there.  Is it okay to move exception block inside CFI_ENDPROC?

>> --- a/arch/x86/xen/enlighten.c
>> +++ b/arch/x86/xen/enlighten.c
>> @@ -323,13 +323,14 @@ static void load_TLS_descriptor(struct
>> thread_struct *t,
>>  static void xen_load_tls(struct thread_struct *t, unsigned int cpu)
>>  {
>>      /*
>> -     * XXX sleazy hack: If we're being called in a lazy-cpu zone,
>> -     * it means we're in a context switch, and %gs has just been
>> -     * saved.  This means we can zero it out to prevent faults on
>> -     * exit from the hypervisor if the next process has no %gs.
>> -     * Either way, it has been saved, and the new value will get
>> -     * loaded properly.  This will go away as soon as Xen has been
>> -     * modified to not save/restore %gs for normal hypercalls.
>> +     * XXX sleazy hack: If we're being called in a lazy-cpu zone
>> +     * and lazy gs handling is enabled, it means we're in a
>> +     * context switch, and %gs has just been saved.  This means we
>> +     * can zero it out to prevent faults on exit from the
>> +     * hypervisor if the next process has no %gs.  Either way, it
>> +     * has been saved, and the new value will get loaded properly.
>> +     * This will go away as soon as Xen has been modified to not
>> +     * save/restore %gs for normal hypercalls.
>>   
> 
> No, this change isn't quite right; the "and lazy gs handling is enabled"
> qualifier is wrong, because the condition the comment describes is
> independent of whether we're doing lazy gs handling.  This would be better:
> 
>    XXX sleazy hack: If we're being called in a lazy-cpu zone, it means
>    we're in a context switch, and %gs has definitely been saved (just
>    saved if we're doing lazy gs handling, and saved on entry if not).
>    This means we can zero it out to prevent faults on exit from the
>    hypervisor if the next process has no %gs. Either way, it has been
>    saved, and the new value will get loaded properly. This will go away
>    as soon as Xen has been modified to not save/restore %gs for normal
>    hypercalls.

Hmmm... I was (lazily) trying to add that %gs can only be cleared if
it's being managed lazily because otherwise it might be being used by
the kernel for other purposes.  :-)

Is my understanding correct?

Thanks.

-- 
tejun