2020-02-27 03:21:21

by Miaohe Lin

[permalink] [raw]
Subject: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl

From: Miaohe Lin <[email protected]>

When kvm_vcpu_ioctl_get_cpuid2() fails, we set cpuid->nent to the value of
vcpu->arch.cpuid_nent. But this is in vain as cpuid->nent is not copied to
userspace by copy_to_user() from call site. Also cpuid->nent is not updated
to indicate how many entries were retrieved on success case. So this ioctl
is straight up broken. And in fact, it's not used anywhere. So it should be
deprecated.

Suggested-by: Paolo Bonzini <[email protected]>
Signed-off-by: Miaohe Lin <[email protected]>
---
arch/x86/kvm/cpuid.c | 20 --------------------
arch/x86/kvm/cpuid.h | 3 ---
arch/x86/kvm/x86.c | 16 ++--------------
include/uapi/linux/kvm.h | 1 +
tools/include/uapi/linux/kvm.h | 1 +
5 files changed, 4 insertions(+), 37 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index b1c469446b07..5e041a1282b8 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -261,26 +261,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
return r;
}

-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->arch.cpuid_nent)
- goto out;
- r = -EFAULT;
- if (copy_to_user(entries, &vcpu->arch.cpuid_entries,
- vcpu->arch.cpuid_nent * sizeof(struct kvm_cpuid_entry2)))
- goto out;
- return 0;
-
-out:
- cpuid->nent = vcpu->arch.cpuid_nent;
- return r;
-}
-
static __always_inline void cpuid_mask(u32 *word, int wordnum)
{
reverse_cpuid_check(wordnum);
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 7366c618aa04..76555de38e1b 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -19,9 +19,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
struct kvm_cpuid2 *cpuid,
struct kvm_cpuid_entry2 __user *entries);
-int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
- struct kvm_cpuid2 *cpuid,
- struct kvm_cpuid_entry2 __user *entries);
bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
u32 *ecx, u32 *edx, bool check_limit);

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ddd1d296bd20..a6d99abedb2c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4295,21 +4295,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
cpuid_arg->entries);
break;
}
+ /* KVM_GET_CPUID2 is deprecated, should not be used. */
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;
+ r = -EINVAL;
break;
}
case KVM_GET_MSRS: {
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 4b95f9a31a2f..61524780603d 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1380,6 +1380,7 @@ struct kvm_s390_ucas_mapping {
#define KVM_GET_LAPIC _IOR(KVMIO, 0x8e, struct kvm_lapic_state)
#define KVM_SET_LAPIC _IOW(KVMIO, 0x8f, struct kvm_lapic_state)
#define KVM_SET_CPUID2 _IOW(KVMIO, 0x90, struct kvm_cpuid2)
+/* KVM_GET_CPUID2 is deprecated, should not be used. */
#define KVM_GET_CPUID2 _IOWR(KVMIO, 0x91, struct kvm_cpuid2)
/* Available with KVM_CAP_VAPIC */
#define KVM_TPR_ACCESS_REPORTING _IOWR(KVMIO, 0x92, struct kvm_tpr_access_ctl)
diff --git a/tools/include/uapi/linux/kvm.h b/tools/include/uapi/linux/kvm.h
index f0a16b4adbbd..2ef719af4c57 100644
--- a/tools/include/uapi/linux/kvm.h
+++ b/tools/include/uapi/linux/kvm.h
@@ -1379,6 +1379,7 @@ struct kvm_s390_ucas_mapping {
#define KVM_GET_LAPIC _IOR(KVMIO, 0x8e, struct kvm_lapic_state)
#define KVM_SET_LAPIC _IOW(KVMIO, 0x8f, struct kvm_lapic_state)
#define KVM_SET_CPUID2 _IOW(KVMIO, 0x90, struct kvm_cpuid2)
+/* KVM_GET_CPUID2 is deprecated, should not be used. */
#define KVM_GET_CPUID2 _IOWR(KVMIO, 0x91, struct kvm_cpuid2)
/* Available with KVM_CAP_VAPIC */
#define KVM_TPR_ACCESS_REPORTING _IOWR(KVMIO, 0x92, struct kvm_tpr_access_ctl)
--
2.19.1


2020-02-27 11:37:37

by Vitaly Kuznetsov

[permalink] [raw]
Subject: Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl

linmiaohe <[email protected]> writes:

> From: Miaohe Lin <[email protected]>
>
> When kvm_vcpu_ioctl_get_cpuid2() fails, we set cpuid->nent to the value of
> vcpu->arch.cpuid_nent. But this is in vain as cpuid->nent is not copied to
> userspace by copy_to_user() from call site. Also cpuid->nent is not updated
> to indicate how many entries were retrieved on success case. So this ioctl
> is straight up broken. And in fact, it's not used anywhere. So it should be
> deprecated.
>
> Suggested-by: Paolo Bonzini <[email protected]>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> arch/x86/kvm/cpuid.c | 20 --------------------
> arch/x86/kvm/cpuid.h | 3 ---
> arch/x86/kvm/x86.c | 16 ++--------------
> include/uapi/linux/kvm.h | 1 +
> tools/include/uapi/linux/kvm.h | 1 +
> 5 files changed, 4 insertions(+), 37 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index b1c469446b07..5e041a1282b8 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -261,26 +261,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
> return r;
> }
>
> -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->arch.cpuid_nent)
> - goto out;
> - r = -EFAULT;
> - if (copy_to_user(entries, &vcpu->arch.cpuid_entries,
> - vcpu->arch.cpuid_nent * sizeof(struct kvm_cpuid_entry2)))
> - goto out;
> - return 0;
> -
> -out:
> - cpuid->nent = vcpu->arch.cpuid_nent;
> - return r;
> -}
> -
> static __always_inline void cpuid_mask(u32 *word, int wordnum)
> {
> reverse_cpuid_check(wordnum);
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index 7366c618aa04..76555de38e1b 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -19,9 +19,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
> int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
> struct kvm_cpuid2 *cpuid,
> struct kvm_cpuid_entry2 __user *entries);
> -int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
> - struct kvm_cpuid2 *cpuid,
> - struct kvm_cpuid_entry2 __user *entries);
> bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
> u32 *ecx, u32 *edx, bool check_limit);
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ddd1d296bd20..a6d99abedb2c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4295,21 +4295,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> cpuid_arg->entries);
> break;
> }
> + /* KVM_GET_CPUID2 is deprecated, should not be used. */
> 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;
> + r = -EINVAL;
> break;
> }

Braces are not really needed not but all other cases in the switch have
it so let's leave them here too.

> case KVM_GET_MSRS: {
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 4b95f9a31a2f..61524780603d 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1380,6 +1380,7 @@ struct kvm_s390_ucas_mapping {
> #define KVM_GET_LAPIC _IOR(KVMIO, 0x8e, struct kvm_lapic_state)
> #define KVM_SET_LAPIC _IOW(KVMIO, 0x8f, struct kvm_lapic_state)
> #define KVM_SET_CPUID2 _IOW(KVMIO, 0x90, struct kvm_cpuid2)
> +/* KVM_GET_CPUID2 is deprecated, should not be used. */
> #define KVM_GET_CPUID2 _IOWR(KVMIO, 0x91, struct kvm_cpuid2)
> /* Available with KVM_CAP_VAPIC */
> #define KVM_TPR_ACCESS_REPORTING _IOWR(KVMIO, 0x92, struct kvm_tpr_access_ctl)
> diff --git a/tools/include/uapi/linux/kvm.h b/tools/include/uapi/linux/kvm.h
> index f0a16b4adbbd..2ef719af4c57 100644
> --- a/tools/include/uapi/linux/kvm.h
> +++ b/tools/include/uapi/linux/kvm.h
> @@ -1379,6 +1379,7 @@ struct kvm_s390_ucas_mapping {
> #define KVM_GET_LAPIC _IOR(KVMIO, 0x8e, struct kvm_lapic_state)
> #define KVM_SET_LAPIC _IOW(KVMIO, 0x8f, struct kvm_lapic_state)
> #define KVM_SET_CPUID2 _IOW(KVMIO, 0x90, struct kvm_cpuid2)
> +/* KVM_GET_CPUID2 is deprecated, should not be used. */

"should not be used" pre-patch, post-patch we can say "Can only be used
as a reliable source of -EINVAL" :-)

> #define KVM_GET_CPUID2 _IOWR(KVMIO, 0x91, struct kvm_cpuid2)
> /* Available with KVM_CAP_VAPIC */
> #define KVM_TPR_ACCESS_REPORTING _IOWR(KVMIO, 0x92, struct kvm_tpr_access_ctl)

Surprisingly (or not), KVM_GET_CPUID2 is not even described in
Documentation/virt/kvm/api.txt.

Reviewed-by: Vitaly Kuznetsov <[email protected]>

--
Vitaly

2020-02-28 01:37:46

by Miaohe Lin

[permalink] [raw]
Subject: Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl

Vitaly Kuznetsov <[email protected]> writes:
>linmiaohe <[email protected]> writes:
>
>> From: Miaohe Lin <[email protected]>
>> - if (copy_to_user(cpuid_arg, &cpuid, sizeof(cpuid)))
>> - goto out;
>> - r = 0;
>> + r = -EINVAL;
>> break;
>> }
>
>Braces are not really needed not but all other cases in the switch have it so let's leave them here too.
>

That's what I think too. :)

>>
>> case KVM_GET_MSRS: {
>> +/* KVM_GET_CPUID2 is deprecated, should not be used. */
>
>"should not be used" pre-patch, post-patch we can say "Can only be used as a reliable source of -EINVAL" :-)

That's right.

>
> #define KVM_GET_CPUID2 _IOWR(KVMIO, 0x91, struct kvm_cpuid2)
> /* Available with KVM_CAP_VAPIC */
> #define KVM_TPR_ACCESS_REPORTING _IOWR(KVMIO, 0x92, struct
> kvm_tpr_access_ctl)
>
>Surprisingly (or not), KVM_GET_CPUID2 is not even described in Documentation/virt/kvm/api.txt.
>

Maybe KVM_GET_CPUID2 is defined for integrity only.

>
>Reviewed-by: Vitaly Kuznetsov <[email protected]>
>

Many thanks for your review!

2020-03-02 16:32:12

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl

On 27/02/20 12:36, Vitaly Kuznetsov wrote:
>> - if (copy_to_user(cpuid_arg, &cpuid, sizeof(cpuid)))
>> - goto out;
>> - r = 0;
>> + r = -EINVAL;
>> break;
>> }
> Braces are not really needed not but all other cases in the switch have
> it so let's leave them here too.
>

We can remove the case altogether.

Paolo

2020-03-02 16:33:10

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl

On 27/02/20 04:21, linmiaohe wrote:
> From: Miaohe Lin <[email protected]>
>
> When kvm_vcpu_ioctl_get_cpuid2() fails, we set cpuid->nent to the value of
> vcpu->arch.cpuid_nent. But this is in vain as cpuid->nent is not copied to
> userspace by copy_to_user() from call site. Also cpuid->nent is not updated
> to indicate how many entries were retrieved on success case. So this ioctl
> is straight up broken. And in fact, it's not used anywhere. So it should be
> deprecated.
>
> Suggested-by: Paolo Bonzini <[email protected]>
> Signed-off-by: Miaohe Lin <[email protected]>
> ---
> arch/x86/kvm/cpuid.c | 20 --------------------
> arch/x86/kvm/cpuid.h | 3 ---
> arch/x86/kvm/x86.c | 16 ++--------------
> include/uapi/linux/kvm.h | 1 +
> tools/include/uapi/linux/kvm.h | 1 +
> 5 files changed, 4 insertions(+), 37 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index b1c469446b07..5e041a1282b8 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -261,26 +261,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
> return r;
> }
>
> -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->arch.cpuid_nent)
> - goto out;
> - r = -EFAULT;
> - if (copy_to_user(entries, &vcpu->arch.cpuid_entries,
> - vcpu->arch.cpuid_nent * sizeof(struct kvm_cpuid_entry2)))
> - goto out;
> - return 0;
> -
> -out:
> - cpuid->nent = vcpu->arch.cpuid_nent;
> - return r;
> -}
> -
> static __always_inline void cpuid_mask(u32 *word, int wordnum)
> {
> reverse_cpuid_check(wordnum);
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index 7366c618aa04..76555de38e1b 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -19,9 +19,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
> int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
> struct kvm_cpuid2 *cpuid,
> struct kvm_cpuid_entry2 __user *entries);
> -int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
> - struct kvm_cpuid2 *cpuid,
> - struct kvm_cpuid_entry2 __user *entries);
> bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
> u32 *ecx, u32 *edx, bool check_limit);
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ddd1d296bd20..a6d99abedb2c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4295,21 +4295,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> cpuid_arg->entries);
> break;
> }
> + /* KVM_GET_CPUID2 is deprecated, should not be used. */
> 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;
> + r = -EINVAL;
> break;
> }
> case KVM_GET_MSRS: {
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 4b95f9a31a2f..61524780603d 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1380,6 +1380,7 @@ struct kvm_s390_ucas_mapping {
> #define KVM_GET_LAPIC _IOR(KVMIO, 0x8e, struct kvm_lapic_state)
> #define KVM_SET_LAPIC _IOW(KVMIO, 0x8f, struct kvm_lapic_state)
> #define KVM_SET_CPUID2 _IOW(KVMIO, 0x90, struct kvm_cpuid2)
> +/* KVM_GET_CPUID2 is deprecated, should not be used. */
> #define KVM_GET_CPUID2 _IOWR(KVMIO, 0x91, struct kvm_cpuid2)
> /* Available with KVM_CAP_VAPIC */
> #define KVM_TPR_ACCESS_REPORTING _IOWR(KVMIO, 0x92, struct kvm_tpr_access_ctl)
> diff --git a/tools/include/uapi/linux/kvm.h b/tools/include/uapi/linux/kvm.h
> index f0a16b4adbbd..2ef719af4c57 100644
> --- a/tools/include/uapi/linux/kvm.h
> +++ b/tools/include/uapi/linux/kvm.h
> @@ -1379,6 +1379,7 @@ struct kvm_s390_ucas_mapping {
> #define KVM_GET_LAPIC _IOR(KVMIO, 0x8e, struct kvm_lapic_state)
> #define KVM_SET_LAPIC _IOW(KVMIO, 0x8f, struct kvm_lapic_state)
> #define KVM_SET_CPUID2 _IOW(KVMIO, 0x90, struct kvm_cpuid2)
> +/* KVM_GET_CPUID2 is deprecated, should not be used. */
> #define KVM_GET_CPUID2 _IOWR(KVMIO, 0x91, struct kvm_cpuid2)
> /* Available with KVM_CAP_VAPIC */
> #define KVM_TPR_ACCESS_REPORTING _IOWR(KVMIO, 0x92, struct kvm_tpr_access_ctl)
>

Queued, thanks.

Paolo

2020-03-02 17:03:40

by Jim Mattson

[permalink] [raw]
Subject: Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl

On Wed, Feb 26, 2020 at 7:20 PM linmiaohe <[email protected]> wrote:
>
> From: Miaohe Lin <[email protected]>
>
> When kvm_vcpu_ioctl_get_cpuid2() fails, we set cpuid->nent to the value of
> vcpu->arch.cpuid_nent. But this is in vain as cpuid->nent is not copied to
> userspace by copy_to_user() from call site. Also cpuid->nent is not updated
> to indicate how many entries were retrieved on success case. So this ioctl
> is straight up broken. And in fact, it's not used anywhere. So it should be
> deprecated.

I don't know how you can make the assertion that this ioctl is not
used anywhere. For instance, I see a use of it in Google's code base.

2020-03-02 17:10:11

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl

On 02/03/20 18:01, Jim Mattson wrote:
>> And in fact, it's not used anywhere. So it should be
>> deprecated.
> I don't know how you can make the assertion that this ioctl is not
> used anywhere. For instance, I see a use of it in Google's code base.

Right, it does not seem to be used anywhere according to e.g. Debian
code search but of course it can have users.

What are you using it for? It's true that cpuid->nent is never written
back to userspace, so the ioctl is basically unusable unless you already
know how many entries are written. Or unless you fill the CPUID entries
with garbage before calling it, I guess; is that what you are doing?

Paolo

2020-03-02 17:45:24

by Jim Mattson

[permalink] [raw]
Subject: Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl

On Mon, Mar 2, 2020 at 9:09 AM Paolo Bonzini <[email protected]> wrote:
>
> On 02/03/20 18:01, Jim Mattson wrote:
> >> And in fact, it's not used anywhere. So it should be
> >> deprecated.
> > I don't know how you can make the assertion that this ioctl is not
> > used anywhere. For instance, I see a use of it in Google's code base.
>
> Right, it does not seem to be used anywhere according to e.g. Debian
> code search but of course it can have users.
>
> What are you using it for? It's true that cpuid->nent is never written
> back to userspace, so the ioctl is basically unusable unless you already
> know how many entries are written. Or unless you fill the CPUID entries
> with garbage before calling it, I guess; is that what you are doing?

One could use GET_CPUID2 after SET_CPUID2, to see what changes kvm
made to the requested guest CPUID information without telling you.

2020-03-02 18:04:45

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl

On 02/03/20 18:44, Jim Mattson wrote:
> On Mon, Mar 2, 2020 at 9:09 AM Paolo Bonzini <[email protected]> wrote:
>>
>> On 02/03/20 18:01, Jim Mattson wrote:
>>>> And in fact, it's not used anywhere. So it should be
>>>> deprecated.
>>> I don't know how you can make the assertion that this ioctl is not
>>> used anywhere. For instance, I see a use of it in Google's code base.
>>
>> Right, it does not seem to be used anywhere according to e.g. Debian
>> code search but of course it can have users.
>>
>> What are you using it for? It's true that cpuid->nent is never written
>> back to userspace, so the ioctl is basically unusable unless you already
>> know how many entries are written. Or unless you fill the CPUID entries
>> with garbage before calling it, I guess; is that what you are doing?
>
> One could use GET_CPUID2 after SET_CPUID2, to see what changes kvm
> made to the requested guest CPUID information without telling you.

Yeah, I think GET_CPUID2 with the same number of leaves that you have
passed to SET_CPUID2 should work.

Paolo

2020-03-02 18:30:49

by Jim Mattson

[permalink] [raw]
Subject: Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl

On Mon, Mar 2, 2020 at 10:02 AM Paolo Bonzini <[email protected]> wrote:
>
> On 02/03/20 18:44, Jim Mattson wrote:
> > On Mon, Mar 2, 2020 at 9:09 AM Paolo Bonzini <[email protected]> wrote:
> >>
> >> On 02/03/20 18:01, Jim Mattson wrote:
> >>>> And in fact, it's not used anywhere. So it should be
> >>>> deprecated.
> >>> I don't know how you can make the assertion that this ioctl is not
> >>> used anywhere. For instance, I see a use of it in Google's code base.
> >>
> >> Right, it does not seem to be used anywhere according to e.g. Debian
> >> code search but of course it can have users.
> >>
> >> What are you using it for? It's true that cpuid->nent is never written
> >> back to userspace, so the ioctl is basically unusable unless you already
> >> know how many entries are written. Or unless you fill the CPUID entries
> >> with garbage before calling it, I guess; is that what you are doing?
> >
> > One could use GET_CPUID2 after SET_CPUID2, to see what changes kvm
> > made to the requested guest CPUID information without telling you.
>
> Yeah, I think GET_CPUID2 with the same number of leaves that you have
> passed to SET_CPUID2 should work.

Having said that, it doesn't look like the method that invokes this
ioctl (in Google's code base) gets called from anywhere.