Re: [PATCH 06/55] KVM: Per-architecture hypercall definitions

[Avi Kivity <avi@qumranet.com>]

[copying Anthony, the original author]

Pavel Machek wrote:
> Hi!
>
>   
>> Currently kvm provides hypercalls only for x86* architectures. To
>> provide hypercall infrastructure for other kvm architectures I split
>> kvm_para.h into a generic header file and architecture specific
>> definitions.
>>     

>> diff --git a/include/asm-x86/kvm_para.h b/include/asm-x86/kvm_para.h
>> new file mode 100644
>> index 0000000..c6f3fd8
>> --- /dev/null
>> +++ b/include/asm-x86/kvm_para.h
>> @@ -0,0 +1,105 @@
>> +#ifndef __X86_KVM_PARA_H
>> +#define __X86_KVM_PARA_H
>> +
>> +/* This CPUID returns the signature 'KVMKVMKVM' in ebx, ecx, and edx.  It
>> + * should be used to determine that a VM is running under KVM.
>> + */
>> +#define KVM_CPUID_SIGNATURE	0x40000000
>>     
>
> so it returns 'KVMKVMKVM' in %rax, too? 
>
>   

No, as documented. 'KVMKVMKVM' is spread among all three registers (9 
bytes won't fit into one...)

>> +/* For KVM hypercalls, a three-byte sequence of either the vmrun or the vmmrun
>> + * instruction.  The hypervisor may replace it with something else but only the
>> + * instructions are guaranteed to be supported.
>> + *
>> + * Up to four arguments may be passed in rbx, rcx, rdx, and rsi respectively.
>> + * The hypercall number should be placed in rax and the return
>>     
>
> rax? First, this file is shared with i386, AFAICT.
>   

rax is used here in the sense of 'rax on x86-64, eax on i386'.  I guess 
this should be documented.

>   
>> + * placed in rax.  No other registers will be clobbered unless explicited
>> + * noted by the particular hypercall.
>> + */
>> +
>> +static inline long kvm_hypercall0(unsigned int nr)
>> +{
>> +	long ret;
>> +	asm volatile(KVM_HYPERCALL
>> +		     : "=a"(ret)
>> +		     : "a"(nr));
>>     
>
> Second, if it is to be placed in rax, nr should be unsigned long?
>
>
>   

Hm.  Since hypercall numbers are shared with i386, we can't have >4G of 
them.  So the hypercall number should be redefined to be in eax, 
regardless of arch (while the arguments still are unsigned longs).

>> +static inline int kvm_para_available(void)
>> +{
>> +	unsigned int eax, ebx, ecx, edx;
>> +	char signature[13];
>> +
>> +	cpuid(KVM_CPUID_SIGNATURE, &eax, &ebx, &ecx, &edx);
>> +	memcpy(signature + 0, &ebx, 4);
>> +	memcpy(signature + 4, &ecx, 4);
>> +	memcpy(signature + 8, &edx, 4);
>>     
>
>   
>> +	signature[12] = 0;
>> +
>>     
>                                ebx|ecx|ed
>   
>> +	if (strcmp(signature, "KVMKVMKVM") == 0)
>> +		return 1;
>>     
>
> Should the comment say
>
>   
>> +/* This CPUID returns the signature 'KVMKVMKVM\0\0\0' in ebx, ecx, and edx.  It
>> + * should be used to determine that a VM is running under KVM.
>> + */
>>     
>
> Plus, I'd use memcmp, and actually test for those zeros, too.
>
> ...which probably can be done later, as this is pure move...
> 									Pavel
>   

Or maybe direct 32-bit compares, and document the 32-bit values of the 
registers directly, to avoid any chance of confusion.

Thanks for the comments, I'll update the patches.


-- 
error compiling committee.c: too many arguments to function