Re: [Xen-devel] [PATCH] x86/entry: Fix passing 6th argument for compat hypercalls

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.03.18 at 21:08, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 16/03/18 19:55, Jason Andryuk wrote:
>> Commit ec05090403ef4d760fbe701e31afd0f0edc414d5 ("x86/entry: Erase guest
>> GPR state on entry to Xen") zero-ed %rbp, compat arg 6, but it is not
>> restored before passing to hypercalls.  We need to pass the saved compat
>> arg 6 to the hypercall in r9, the 6th function argument.
>>
>> Signed-off-by: Jason Andryuk <jandryuk@xxxxxxxxx>
>> ---
> 
> Jan: for reference, this is precisely the reason why I recommended that
> you backport the minimal 51e5d6c7a29 which I provided, rather than try
> to fix up the existing on-stack logic.

And I continue to disagree that infrastructure changes should
generally be backported. There's always going to be _some_
point where this can't (reasonably) be done anymore (e.g.
without backporting further and further infrastructure changes),
so better not to do it at all.

> Looking at that commit, there is another bug in LOAD_C_CLOBBERED, where
> passing compat=0 xc=0 corrupts %rcx with whatever was on the stack for %r10

That's intended behavior, albeit perhaps not immediately obvious
without looking at the change the original commit does to
xen/arch/x86/x86_64/entry.S: It replaces a move from %r10 to
%rcx by a LOAD_C_CLOBBERED invocation. For these old trees
I decided that's an acceptable thing to do, even if perhaps I
wouldn't want to see something like that in master.

>> This is against Xen 4.6.  I believe it is also applicable to 4.7.

Yes, I'll put this into both.

>>  xen/arch/x86/x86_64/compat/entry.S | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/x86_64/compat/entry.S 
>> b/xen/arch/x86/x86_64/compat/entry.S
>> index bc1f509672..b99b142e45 100644
>> --- a/xen/arch/x86/x86_64/compat/entry.S
>> +++ b/xen/arch/x86/x86_64/compat/entry.S
>> @@ -56,7 +56,7 @@ UNLIKELY_END(msi_check)
>>          xchgl %ecx,%esi              /* Arg 2, Arg 4 */
>>          movl  %edx,%edx              /* Arg 3        */
>>          movl  %edi,%r8d              /* Arg 5        */
>> -        movl  %ebp,%r9d              /* Arg 6        */
>> +        movl  UREGS_rbp(%rsp),%r9d   /* Arg 6        */
>>          movl  UREGS_rbx(%rsp),%edi   /* Arg 1        */
> 
> This looks correct, (but there is a good reason why I purged this code
> from the codebase 3 releases ago).

Right, yet independent of your purging of it perhaps most
fundamentally the point that iirc there are no 6-argument
hypercalls in the first place.

Jan


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