Re: [kernel patch] exposing host cpuids to guest

From: Amit Shah <amit.shah@redhat.com>
To: Nitin A Kamble <nitin.a.kamble@intel.com>
Cc: avi@redhat.com, kvm@vger.kernel.org
Subject: Re: [kernel patch] exposing host cpuids to guest
Date: Fri, 30 Jan 2009 16:55:21 +0530	[thread overview]
Message-ID: <20090130112521.GD25998@amit-x200.pnq.redhat.com> (raw)
In-Reply-To: <1233277976.28605.9.camel@mukti.sc.intel.com>

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