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

Tejun Heo
2009-02-10T01:28:41Z

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