Re: [Xen-devel] [PATCH] x86/xpti: avoid copying L4 page table contents when possible

[Juergen Gross <jgross@xxxxxxxx>]

On 20/02/18 10:10, Jan Beulich wrote:
>>>> On 20.02.18 at 10:03, <jgross@xxxxxxxx> wrote:
>> On 20/02/18 09:56, Jan Beulich wrote:
>>>>>> On 19.02.18 at 18:19, <jgross@xxxxxxxx> wrote:
>>>> On 19/02/18 17:59, Jan Beulich wrote:
>>>>>>>> On 15.02.18 at 13:52, <jgross@xxxxxxxx> wrote:
>>>>>> --- a/xen/arch/x86/x86_64/entry.S
>>>>>> +++ b/xen/arch/x86/x86_64/entry.S
>>>>>> @@ -46,10 +46,13 @@ restore_all_guest:
>>>>>>  .Lrag_cr3_start:
>>>>>>          mov   VCPU_cr3(%rbx), %r9
>>>>>>          GET_STACK_END(dx)
>>>>>> -        mov   STACK_CPUINFO_FIELD(pv_cr3)(%rdx), %rdi
>>>>>> +        mov   STACK_CPUINFO_FIELD(pv_cr3)(%rdx), %rax
>>>>>> +        cmpb  $0, STACK_CPUINFO_FIELD(copy_l4)(%rdx)
>>>>>> +        je    .Lrag_copyend
>>>>>> +        movb  $0, STACK_CPUINFO_FIELD(copy_l4)(%rdx)
>>>>>
>>>>> Use %bl instead of $0 in both cases (with a comment)?
>>>>
>>>> Do you really think we should add such a micro optimization relying
>>>> on the vcpu pointer being aligned to page boundary? And are you sure
>>>> adding the register dependency isn't hurting more than we will gain
>>>> from saving an instruction byte?
>>>
>>> Well, both of what you say are the precise reasons why I've
>>> asked a question rather than asking for the change to be done
>>> unconditionally. I'm not really worried about the register
>>> dependency - %rbx is loaded sufficiently much earlier. And
>>> the struct-vcpu-is-aligned aspect is why I did suggest adding
>>> a comment; I assume you realize we can't easily break that
>>> alignment, hence I see no significant risk here. But I'm not
>>> going to insist, and I'm open for further counter arguments.
>>
>> What about the following idea: instead of special casing this single
>> instance of replacing $0 with a register we could e.g. replace all
>> %rbx uses by %r13 and set %rbx to 0 globally. This would enable us
>> to replace _all_ $0 uses by %rbx/%ebx/%bx/%bl.
> 
> I dislike this, both because use of %r13 will require a REX prefix
> even on insns that currently don't need one, and because I
> dislike wasting a register just to hold zero. It's no like we have
> that many callee-saved registers left, now that we had to start
> using some of them beyond %rbx and %rbp.

Okay, fair enough.

Nevertheless I'd like to defer replacing $0 by %bl to another patch,
especially as there are are multiple other places where this could be
done (e.g. in test_events).


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel