Re: [PATCH v2 2/4] x86/vmware: Add a header file for hypercall definitions

["Thomas Hellström (VMware)" <thomas_os@shipmail.org>]

On 8/27/19 5:44 PM, Borislav Petkov wrote:
> On Fri, Aug 23, 2019 at 10:13:14AM +0200, Thomas Hellström (VMware) wrote:
>> +/*
>> + * The high bandwidth out call. The low word of edx is presumed to have the
>> + * HB and OUT bits set.
>> + */
>> +#define VMWARE_HYPERCALL_HB_OUT						\
>> +	ALTERNATIVE_2("movw $" VMWARE_HYPERVISOR_PORT_HB ", %%dx; rep outsb", \
> Hmm, that looks fishy:
>
> This call in vmw_port_hb_out(), for example, gets converted to the asm
> below (I've left in the asm touching only rDX).
>
> # drivers/gpu/drm/vmwgfx/vmwgfx_msg.c:160:              VMW_PORT_HB_OUT(
> #NO_APP
>          movzwl  0(%rbp), %edx   # channel_20(D)->channel_id, channel_20(D)->channel_id
>
> 	...
>
>          sall    $16, %edx       #, tmp172
>          orl     $3, %edx        #, tmp173
>
> this is adding channel_id and flags:
>
>                          VMWARE_HYPERVISOR_HB | (channel->channel_id << 16) |
>                          VMWARE_HYPERVISOR_OUT,
>
> the $3 being (VMWARE_HYPERVISOR_HB | VMWARE_HYPERVISOR_OUT).
>
>          movslq  %edx, %rdx      # tmp173, tmp174
>
> Here it is sign-extending it.
>
> #APP
> # 160 "drivers/gpu/drm/vmwgfx/vmwgfx_msg.c" 1
>          push %rbp;mov %r8, %rbp;# ALT: oldinstr2        # bp
> 661:
>          movw $0x5659, %dx; rep outsb
>
> And now here you're overwriting the low word of %edx. And now it
> contains:
>
> 0x[channel_id]5659
>
> and the low word doesn't contain the 3, i.e., (VMWARE_HYPERVISOR_HB |
> VMWARE_HYPERVISOR_OUT) anymore. And that's before you do the hypercall
> so I'm guessing that cannot be right.
>
> Or?
>
It should be correct. The flags VMWARE_HYPERVISOR_HB and 
VMWARE_HYPERVISOR_OUT are only valid for the vmcall / vmmcall versions.

For the legacy version, the direction is toggled by the instruction (in 
vs out) and LB vs HB is toggled by the port number (0x5658 vs 0x5659)

So in essence the low word definition of %edx is different in the two 
versions. I've chosen to use the new vmcall/vmmcall definition in the 
driver code.

/Thomas