| Message ID | 1233277976.28605.9.camel@mukti.sc.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On (Thu) Jan 29 2009 [17:12:56], Nitin A Kamble wrote: > Avi, > I reworked the earlier patch for exposing the host cpuid bits to > guests. Attached is the patch for your kvm.git tree. With this new code > in the kernel both the old and new qemu binaries are working. Please add a Signed-Off-By and a git-style description for patches sent for inclusion. Also, it'll be better to reply to the patch if you include in the message instead of attaching them (git send-email). > -#define KVM_MAX_CPUID_ENTRIES 40 > +#define KVM_MAX_CPUID_ENTRIES 100 Are we already hitting this limit? I don't think so. > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1060,19 +1060,25 @@ long kvm_arch_dev_ioctl(struct file *filp, > case KVM_GET_SUPPORTED_CPUID: { > struct kvm_cpuid2 __user *cpuid_arg = argp; > struct kvm_cpuid2 cpuid; > + int retry = 0; > > r = -EFAULT; > if (copy_from_user(&cpuid, cpuid_arg, sizeof cpuid)) > goto out; > r = kvm_dev_ioctl_get_supported_cpuid(&cpuid, > cpuid_arg->entries); > - if (r) > + if (r == -EAGAIN) > + retry = 1; > + else if (r) > goto out; > > r = -EFAULT; > if (copy_to_user(cpuid_arg, &cpuid, sizeof cpuid)) > goto out; > - r = 0; > + if (retry) > + r = -EAGAIN; > + else > + r = 0; > break; > } > default: I can't really get the point of doing this: do you want to return -EAGAIN when there are multiple values to be read off a given cpuid function? If that's the case, I'd suggest adding the necessary intelligence to the userspace to poll for as much data as can be fetched rather than (ab)using EAGAIN. > @@ -1325,10 +1331,14 @@ static int > kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid, > { > struct kvm_cpuid_entry2 *cpuid_entries; > int limit, nent = 0, r = -E2BIG; > + int sizer = 0; What does 'sizer' mean? Also, I've added support for handling function 0xd in userspace; can you add the handler for that in get_supported_cpuid()? Amit -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2009-01-30 at 03:25 -0800, Amit Shah wrote: > Please add a Signed-Off-By and a git-style description for patches sent > for inclusion. > Will do. > Also, it'll be better to reply to the patch if you include in the > message instead of attaching them (git send-email). > > > -#define KVM_MAX_CPUID_ENTRIES 40 > > +#define KVM_MAX_CPUID_ENTRIES 100 > > Are we already hitting this limit? I don't think so. It is not hitting the limit yet. Bumped it up for future use. > > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -1060,19 +1060,25 @@ long kvm_arch_dev_ioctl(struct file *filp, > > case KVM_GET_SUPPORTED_CPUID: { > > struct kvm_cpuid2 __user *cpuid_arg = argp; > > struct kvm_cpuid2 cpuid; > > + int retry = 0; > > > > r = -EFAULT; > > if (copy_from_user(&cpuid, cpuid_arg, sizeof cpuid)) > > goto out; > > r = kvm_dev_ioctl_get_supported_cpuid(&cpuid, > > cpuid_arg->entries); > > - if (r) > > + if (r == -EAGAIN) > > + retry = 1; > > + else if (r) > > goto out; > > > > r = -EFAULT; > > if (copy_to_user(cpuid_arg, &cpuid, sizeof cpuid)) > > goto out; > > - r = 0; > > + if (retry) > > + r = -EAGAIN; > > + else > > + r = 0; > > break; > > } > > default: > > I can't really get the point of doing this: do you want to return > -EAGAIN when there are multiple values to be read off a given cpuid > function? If that's the case, I'd suggest adding the necessary > intelligence to the userspace to poll for as much data as can be fetched > rather than (ab)using EAGAIN. This is based on the discussions with Avi last time. Sizer is to get the size of the list of items. Avi wanted to keep the backward compatibility of the ioctl API. > > > @@ -1325,10 +1331,14 @@ static int > > kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid, > > { > > struct kvm_cpuid_entry2 *cpuid_entries; > > int limit, nent = 0, r = -E2BIG; > > + int sizer = 0; > > What does 'sizer' mean? > > Also, I've added support for handling function 0xd in userspace; can you > add the handler for that in get_supported_cpuid()? > Currently KVM is limiting the basic cpuid limit to 0xb in the kernel code. That will need to be changed for leaf 0xd to go through. Doing that would be the next step. > Amit > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 55fd4c5..b450a69 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -87,7 +87,7 @@ #define KVM_NUM_MMU_PAGES (1 << KVM_MMU_HASH_SHIFT) #define KVM_MIN_FREE_MMU_PAGES 5 #define KVM_REFILL_PAGES 25 -#define KVM_MAX_CPUID_ENTRIES 40 +#define KVM_MAX_CPUID_ENTRIES 100 #define KVM_NR_FIXED_MTRR_REGION 88 #define KVM_NR_VAR_MTRR 8 diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index e96edda..4d5124b 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1060,19 +1060,25 @@ long kvm_arch_dev_ioctl(struct file *filp, case KVM_GET_SUPPORTED_CPUID: { struct kvm_cpuid2 __user *cpuid_arg = argp; struct kvm_cpuid2 cpuid; + int retry = 0; r = -EFAULT; if (copy_from_user(&cpuid, cpuid_arg, sizeof cpuid)) goto out; r = kvm_dev_ioctl_get_supported_cpuid(&cpuid, cpuid_arg->entries); - if (r) + if (r == -EAGAIN) + retry = 1; + else if (r) goto out; r = -EFAULT; if (copy_to_user(cpuid_arg, &cpuid, sizeof cpuid)) goto out; - r = 0; + if (retry) + r = -EAGAIN; + else + r = 0; break; } default: @@ -1325,10 +1331,14 @@ static int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid, { struct kvm_cpuid_entry2 *cpuid_entries; int limit, nent = 0, r = -E2BIG; + int sizer = 0; u32 func; - if (cpuid->nent < 1) - goto out; + if (cpuid->nent == 0) { + sizer = 1; + cpuid->nent = KVM_MAX_CPUID_ENTRIES; + } + r = -ENOMEM; cpuid_entries = vmalloc(sizeof(struct kvm_cpuid_entry2) * cpuid->nent); if (!cpuid_entries) @@ -1339,6 +1349,7 @@ static int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid, for (func = 1; func <= limit && nent < cpuid->nent; ++func) do_cpuid_ent(&cpuid_entries[nent], func, 0, &nent, cpuid->nent); + r = -E2BIG; if (nent >= cpuid->nent) goto out_free; @@ -1348,16 +1359,21 @@ static int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid, 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; + + if (sizer) + r = -EAGAIN; + else { + r = -EFAULT; + if (copy_to_user(entries, cpuid_entries, + nent * sizeof(struct kvm_cpuid_entry2))) + goto out_free; + r = 0; + } out_free: vfree(cpuid_entries); out: + cpuid->nent = nent; return r; }