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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/