Re: [RFC] Expose host cpuid to guest

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Dan Kenigsberg wrote:
> These patches expose host CPU features (that are known to work under
> KVM) to guests. It makes a couple of benchmarks run faster, and
> generally gives kvm's user better info on its host.
>
> The kernel-space patch adds KVM_GET_SUPPORTED_CPUID ioctl to obtain the
> table of cpuid functions supported by the host. The user-space patch
> allows fine-tuning this table from the command-line.
>
> I had to define struct kvm_cpuid2, KVM_SET_CPUID2 etc., because cpuid
> functions are a little more complex than just function-value pairs.
>   
> commit e9775d0a16097cfb71779cb2fb985fb3e5040dc8
> Author: Dan Kenigsberg <danken-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
> Date:   Sun Nov 18 13:55:26 2007 +0200
>
>     Support -cpu host option. Negotiate cpuid table with userspace.
>   

The kernel doesn't have a -cpu option.  The description needs to be more 
descriptive (motivation, special cases in cpuid).

>     
>  
> +static int is_efer_nx(void) {
> +	u64 efer;
>   

blank line

> +	rdmsrl(MSR_EFER, efer);
> +	return efer & EFER_NX;
> +}
> +
>   

>  
> +static int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
> +				    struct kvm_cpuid2 *cpuid,
> +				    struct kvm_cpuid_entry2 __user *entries)
> +{
> +	int r;
> +
> +	r = -E2BIG;
> +	if (cpuid->nent < vcpu->cpuid_nent)
> +		goto out;
> +	r = -EFAULT;
> +	if (copy_to_user(entries, &vcpu->cpuid_entries,
> +			   vcpu->cpuid_nent * sizeof(struct kvm_cpuid_entry2)))
> +		goto out;
> +	return 0;
> +
> +out:
> +        cpuid->nent = vcpu->cpuid_nent;
>   

whitespace damage here

> +	return r;
> +
> +static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> +			 u32 index, int *nent, int maxnent)
> +{
> +	const __u32 kvm_supported_word0_x86_features = bit(X86_FEATURE_FPU) |
> +		bit(X86_FEATURE_VME) | bit(X86_FEATURE_DE) |
> +		bit(X86_FEATURE_PSE) | bit(X86_FEATURE_TSC) |
> +		bit(X86_FEATURE_MSR) | bit(X86_FEATURE_PAE) |
> +		bit(X86_FEATURE_CX8) | bit(X86_FEATURE_APIC) |
> +		bit(X86_FEATURE_SEP) | bit(X86_FEATURE_PGE) |
> +		bit(X86_FEATURE_CMOV) | bit(X86_FEATURE_PSE36) |
> +		bit(X86_FEATURE_CLFLSH) | bit(X86_FEATURE_MMX) |
> +		bit(X86_FEATURE_FXSR) | bit(X86_FEATURE_XMM) |
> +		bit(X86_FEATURE_XMM2) | bit(X86_FEATURE_SELFSNOOP);
>   

u32, not __u32.

> +	const __u32 kvm_supported_word1_x86_features = bit(X86_FEATURE_FPU) |
> +		bit(X86_FEATURE_VME) | bit(X86_FEATURE_DE) |
> +		bit(X86_FEATURE_PSE) | bit(X86_FEATURE_TSC) |
> +		bit(X86_FEATURE_MSR) | bit(X86_FEATURE_PAE) |
> +		bit(X86_FEATURE_CX8) | bit(X86_FEATURE_APIC) |
> +		bit(X86_FEATURE_PGE) |
> +		bit(X86_FEATURE_CMOV) | bit(X86_FEATURE_PSE36) |
> +		bit(X86_FEATURE_MMX) | bit(X86_FEATURE_FXSR) |
> +		bit(X86_FEATURE_SYSCALL) |
> +		(bit(X86_FEATURE_NX) && is_efer_nx()) |
> +#ifdef CONFIG_X86_64
> +		bit(X86_FEATURE_LM) |
> +#endif
> +		/* TODO: make sure the following features are
> +		 * safe for SVM guests */
> +		bit(X86_FEATURE_MMXEXT) |
> +		bit(X86_FEATURE_RDTSCP) | bit(X86_FEATURE_3DNOWEXT) |
> +		bit(X86_FEATURE_3DNOW);
>   

rdtscp isn't,   I believe.

> +	const __u32 kvm_supported_word3_x86_features =
> +		bit(X86_FEATURE_XMM3) | bit(X86_FEATURE_CX16);
> +	const __u32 kvm_supported_word6_x86_features =
> +		bit(X86_FEATURE_LAHF_LM) | bit(X86_FEATURE_CMP_LEGACY);
> +
> +	/* all func 2 cpuid_count() should be called on the same cpu */
> +	if (function==2)
> +		get_cpu();
>   

Avoid the special case, just to it unconditionally.

> +	do_cpuid_1_ent(entry, function, index);
> +	++*nent;
> +
> +	switch (function) {
> +	case 0:
> +		entry->eax = min(entry->eax, (u32)0xb);
> +		break;
> +	case 1:
> +		entry->edx &= kvm_supported_word0_x86_features;
> +		entry->ecx &= kvm_supported_word3_x86_features;
> +		break;
> +	/* function 2 entries are STATEFUL. That is, repeated cpuid commands
> +	 * may return different values. This forces us to get_cpu() before
> +	 * issuing the first command, and also to emulate this annoying behavior
> +	 * in kvm_emulate_cpuid() using KVM_CPUID_FLAG_STATE_READ_NEXT */
> +	case 2: {
> +			int t, times = entry->eax & 0xff;
>   

Indent this normally relative to other entries.

> +
> +			entry->flags |= KVM_CPUID_FLAG_STATEFUL_FUNC;
> +			for (t = 1; t < times && *nent < maxnent; ++t) {
> +				do_cpuid_1_ent(&entry[t], function, 0);
> +				entry[t].flags |= KVM_CPUID_FLAG_STATEFUL_FUNC;
> +				++*nent;
> +			}
> +			break;
> +		}
> +	/* function 4 and 0xb have additional index. */
> +	case 4: {
> +			int index, cache_type;
> +
> +			entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> +			/* read more entries until cache_type is zero */
> +			for (index = 1; *nent < maxnent; ++index) {
> +				cache_type = entry[index - 1].eax & 0x1f;
> +				if (!cache_type)
> +					break;
> +				do_cpuid_1_ent(&entry[index], function, index);
> +				entry[index].flags |=
> +				       KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> +				++*nent;
> +			}
> +			break;
> +		}
> +	case 0xb: {
> +			int index, level_type;
> +
> +			entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> +			/* read more entries until level_type is zero */
> +			for (index = 1; *nent < maxnent; ++index) {
> +				level_type = entry[index - 1].ecx & 0xff;
> +				if (!level_type)
> +					break;
> +				do_cpuid_1_ent(&entry[index], function, index);
> +				entry[index].flags |=
> +				       KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
> +				++*nent;
> +			}
> +			break;
> +		}
> +	case 0x80000000:
> +		entry->eax = min(entry->eax, 0x8000001a);
> +                break;
> +	case 0x80000001:
> +		entry->edx &= kvm_supported_word1_x86_features;
> +		entry->ecx &= kvm_supported_word6_x86_features;
> +		break;
> +	}
> +	if (function==2)
> +		put_cpu();
> +}
> +
> +static int kvm_vm_ioctl_get_supported_cpuid(struct kvm *kvm,
> +				    struct kvm_cpuid2 *cpuid,
> +				    struct kvm_cpuid_entry2 __user *entries)
> +{
> +	struct kvm_cpuid_entry2 *cpuid_entries;
> +	int limit, nent = 0, r = -E2BIG;
> +        u32 func

whitespace damage

> ;
> +
> +	if (cpuid->nent < 1)
> +		goto out;
> +	r = -ENOMEM;
> +	cpuid_entries = vmalloc(sizeof(struct kvm_cpuid_entry2) * cpuid->nent);
> +	if (!cpuid_entries)
> +		goto out;
> +
> +	do_cpuid_ent(&cpuid_entries[0], 0, 0, &nent, cpuid->nent);
> +	limit = cpuid_entries[0].eax;
> +	for (func = 1; func <= limit && nent < cpuid->nent; ++func)
> +		do_cpuid_ent(&cpuid_entries[nent], func, 0,
> +				&nent, cpuid->nent);
> +
> +	if (nent >= cpuid->nent)
> +		goto out_free;
>   

Should exit with an E2BIG here.  ENOMEM means kernel's out of memory, 
which isn't the case here.

> +	do_cpuid_ent(&cpuid_entries[nent], 0x80000000, 0, &nent, cpuid->nent);
> +	limit = cpuid_entries[nent - 1].eax;
> +	for (func = 0x80000001; func <= limit && nent < cpuid->nent; ++func)
> +		do_cpuid_ent(&cpuid_entries[nent], func, 0,
> +			       &nent, cpuid->nent);
> +	r = -EFAULT;
> +	if (copy_to_user(entries, cpuid_entries,
> +			nent * sizeof(struct kvm_cpuid_entry2)))
> +		goto out_free;
> +	cpuid->nent = nent;
> +	r = 0;
> +
> +out_free:
> +	vfree(cpuid_entries);
> +out:
> +	return r;
> +}
> +
>  static int kvm_vcpu_ioctl_get_lapic(struct kvm_vcpu *vcpu,
>  				    struct kvm_lapic_state *s)
>  {
> @@ -796,6 +1027,36 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>  			goto out;
>  		break;
>  	}
> +	case KVM_SET_CPUID2: {
> +		struct kvm_cpuid2 __user *cpuid_arg = argp;
> +		struct kvm_cpuid2 cpuid;
> +
> +		r = -EFAULT;
> +		if (copy_from_user(&cpuid, cpuid_arg, sizeof cpuid))
> +			goto out;
> +		r = kvm_vcpu_ioctl_set_cpuid2(vcpu, &cpuid,
> +				cpuid_arg->entries);
> +		if (r)
> +			goto out;
> +		break;
> +	}
> +	case KVM_GET_CPUID2: {
> +		struct kvm_cpuid2 __user *cpuid_arg = argp;
> +		struct kvm_cpuid2 cpuid;
> +
> +		r = -EFAULT;
> +		if (copy_from_user(&cpuid, cpuid_arg, sizeof cpuid))
> +			goto out;
> +		r = kvm_vcpu_ioctl_get_cpuid2(vcpu, &cpuid,
> +				cpuid_arg->entries);
> +		if (r)
> +			goto out;
> +		r = -EFAULT;
> +		if (copy_to_user(cpuid_arg, &cpuid, sizeof cpuid))
> +			goto out;
> +		r = 0;
> +		break;
> +	}
>  	case KVM_GET_MSRS:
>  		r = msr_io(vcpu, argp, kvm_get_msr, 1);
>  		break;
> @@ -1091,6 +1352,24 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  		r = 0;
>  		break;
>  	}
> +	case KVM_GET_SUPPORTED_CPUID: {
> +		struct kvm_cpuid2 __user *cpuid_arg = argp;
> +		struct kvm_cpuid2 cpuid;
> +
> +		r = -EFAULT;
> +		if (copy_from_user(&cpuid, cpuid_arg, sizeof cpuid))
> +			goto out;
> +		r = kvm_vm_ioctl_get_supported_cpuid(kvm, &cpuid,
> +			cpuid_arg->entries);
> +		if (r)
> +			goto out;
> +
> +		r = -EFAULT;
> +		if (copy_to_user(cpuid_arg, &cpuid, sizeof cpuid))
> +			goto out;
> +		r = 0;
> +		break;
> +	}
>  	default:
>  		;
>  	}
> @@ -1887,14 +2166,33 @@ void realmode_set_cr(struct kvm_vcpu *vcpu, int cr, unsigned long val,
>  	}
>  }
>  
> +static int move_to_next_stateful_cpuid_entry(struct kvm_vcpu *vcpu, int i)
> +{
> +	struct kvm_cpuid_entry2 *e = &vcpu->cpuid_entries[i];
> +	int j, nent = vcpu->cpuid_nent;
> +
> +	e->flags &= ~KVM_CPUID_FLAG_STATE_READ_NEXT;
> +	/* when no next entry is found, the current entry[i] is reselected */
> +	for (j = i + 1; j == i; j = (j + 1) % nent) {
> +		struct kvm_cpuid_entry2 *ej =
> +			&vcpu->cpuid_entries[j];
>   

80 column limit, not 40.

> +		if (ej->function == e->function) {
> +			ej->flags |= KVM_CPUID_FLAG_STATE_READ_NEXT;
> +			return j;
> +		}
> +	}
> +	return 0; /* silence gcc, even though control never reaches here */
> +}
> +
>  void kvm_emulate_cpuid(struct kvm_vcpu *vcpu)
>  {
>  	int i;
> -	u32 function;
> -	struct kvm_cpuid_entry *e, *best;
> +	u32 function, index;
> +	struct kvm_cpuid_entry2 *e, *best;
>  
>  	kvm_x86_ops->cache_regs(vcpu);
>  	function = vcpu->regs[VCPU_REGS_RAX];
> +	index = vcpu->regs[VCPU_REGS_RCX];
>  	vcpu->regs[VCPU_REGS_RAX] = 0;
>  	vcpu->regs[VCPU_REGS_RBX] = 0;
>  	vcpu->regs[VCPU_REGS_RCX] = 0;
> @@ -1902,7 +2200,15 @@ void kvm_emulate_cpuid(struct kvm_vcpu *vcpu)
>  	best = NULL;
>  	for (i = 0; i < vcpu->cpuid_nent; ++i) {
>  		e = &vcpu->cpuid_entries[i];
> -		if (e->function == function) {
> +		/* find an entry with matching function, matching index (if
> +		 * needed), and that should be read next (if it's stateful) */
> +		if (e->function == function &&
> +		    (!(e->flags & KVM_CPUID_FLAG_SIGNIFCANT_INDEX)
> +		      || e->index == index) &&
> +		    (!(e->flags & KVM_CPUID_FLAG_STATEFUL_FUNC)
> +		      || (e->flags & KVM_CPUID_FLAG_STATE_READ_NEXT) )) {
> +			if (e->flags & KVM_CPUID_FLAG_STATEFUL_FUNC)
> +				move_to_next_stateful_cpuid_entry(vcpu, i);
>  			best = e;
>   

Please move this to a function with multiple if ()s and return statements.

> ------------------------------------------------------------------------
>
> commit e5a590065ab37fb5eb0481e176d455fba98310dd
> Author: Dan Kenigsberg <danken-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
> Date:   Sun Nov 18 13:36:41 2007 +0200
>
>     Add -cpu host option. Negotiate cpuid table with kernel.
>     
>     Signed-off-by: Dan Kenigsberg <danken-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
>
> diff --git a/kernel/external-module-compat.h b/kernel/external-module-compat.h
> index e3f81fe..ef5d70b 100644
> --- a/kernel/external-module-compat.h
> +++ b/kernel/external-module-compat.h
> @@ -530,14 +530,6 @@ out:
>  #define dest_ExtINT       7
>  #endif
>  
> -/* empty_zero_page isn't exported in all kernels */
> -#include <asm/pgtable.h>
> -
> -#define empty_zero_page kvm_empty_zero_page
> -
> -static char empty_zero_page[PAGE_SIZE];
> -
> -static inline void blahblah(void)
> -{
> -	(void)empty_zero_page[0];
> -}
>   

Remove this removal.

>  
> +int kvm_get_supported_cpuid(kvm_context_t kvm, int *nent,
> +		struct kvm_cpuid_entry2 *entries)
> +{
> +	struct kvm_cpuid2 *cpuid;
> +	int r = -1;
>   

blank line.

> +#ifdef KVM_CAP_EXT_CPUID
> +	r = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_EXT_CPUID);
> +	if (!r)
> +		return -1;
>   

return -errno on errors

> +
> +	cpuid = malloc(sizeof(*cpuid) + *nent * sizeof(*entries));
> +	if (!cpuid)
> +		return -ENOMEM;
> +	cpuid->nent = *nent;
> +
> +	r = ioctl(kvm->vm_fd, KVM_GET_SUPPORTED_CPUID, cpuid);
> +
>   

error check

> +	memcpy(entries, cpuid->entries, *nent * sizeof(*entries));
> +	*nent = cpuid->nent;
> +	free(cpuid);
> +#endif
> +	return r;
> +}
> +
> +int kvm_get_cpuid(kvm_context_t kvm, int vcpu, int *nent,
> +		    struct kvm_cpuid_entry2 *entries)
> +{
> +	struct kvm_cpuid2 *cpuid;
> +	int r = -1;
>   

blank line

> +#ifdef KVM_CAP_EXT_CPUID
> +	r = ioctl(kvm->fd, KVM_CHECK_EXTENSION, KVM_CAP_EXT_CPUID);
> +	if (!r)
> +		return -1;
>   

-errno

> +
> +	cpuid = malloc(sizeof(*cpuid) + *nent * sizeof(*entries));
> +	if (!cpuid)
> +		return -ENOMEM;
> +	cpuid->nent = *nent;
> +
> +	r = ioctl(kvm->vcpu_fd[vcpu], KVM_GET_CPUID2, cpuid);
> +
>   

error check

> +	memcpy(entries, cpuid->entries, *nent * sizeof(*entries));
> +	*nent = cpuid->nent;
> +	free(cpuid);
> +#endif
> +	return r;
> +}
> +
>   


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


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/