Re: [patch 2/3] KVM: x86: introduce ioctl to allow frequency hypercalls

Marcelo Tosatti
2017-02-22T21:18:45Z

On Fri, Feb 03, 2017 at 06:03:37PM +0100, Radim Krcmar wrote:
> 2017-02-02 15:47-0200, Marcelo Tosatti:
> > For most VMs, modifying the host frequency is an undesired
> > operation. Introduce ioctl to enable the guest to
> > modify host CPU frequency.
> >
> >
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> > ---
> > arch/x86/include/asm/kvm_host.h | 2 ++
> > arch/x86/include/uapi/asm/kvm.h | 5 +++++
> > arch/x86/kvm/x86.c | 20 ++++++++++++++++++++
> > include/uapi/linux/kvm.h | 3 +++
> > virt/kvm/kvm_main.c | 2 ++
> > 5 files changed, 32 insertions(+)
> >
> > Index: kvm-pvfreq/arch/x86/kvm/x86.c
> > ===================================================================
> > --- kvm-pvfreq.orig/arch/x86/kvm/x86.c 2017-01-31 10:32:33.023378783 -0200
> > +++ kvm-pvfreq/arch/x86/kvm/x86.c 2017-01-31 10:34:25.443618639 -0200
> > @@ -3665,6 +3665,26 @@
> > r = kvm_vcpu_ioctl_enable_cap(vcpu, &cap);
> > break;
> > }
> > + case KVM_SET_VCPU_ALLOW_FREQ_HC: {
>
> Just enable the frequency hypercalls with KVM_ENABLE_CAP ioctl and get
> rid of this ioctl.
> (I don't think that we want to allow disabling this capability.)

Not sure. What if you change the role of vcpus and now want
to change vcpu-1 from a realtime vcpu (one where only DPDK runs)
to a multi-user vcpu without a reboot?

> > + struct kvm_vcpu_allow_freq freq;
> > +
> > + r = -EFAULT;
> > + if (copy_from_user(&freq, argp, sizeof(freq)))
> > + goto out;
> > + vcpu->arch.allow_freq_hypercall = freq.enable;
>
> Enabling the capability should also set a bit in KVM_CPUID_FEATURES,
> a well-behaved guest won't use it otherwise.

Fixed.

> > + r = 0;
> > + break;
> > + }
> > + case KVM_GET_VCPU_ALLOW_FREQ_HC: {
>
> And this ioctl has no use, so it should be omitted.
>
> (Userspace doesn't learn anything it doesn't already know.
> The feature is unsafe, so the default must be 0.)

Fixed.

> > + struct kvm_vcpu_allow_freq freq;
> > +
> > + memset(&freq, 0, sizeof(struct kvm_vcpu_allow_freq));
> > + r = -EFAULT;
> > + if (copy_to_user(&freq, argp, sizeof(freq)))
> > + break;
> > + r = 0;
> > + break;
> > + }
> > default:
> > r = -EINVAL;
> > }
> > Index: kvm-pvfreq/include/uapi/linux/kvm.h
> > ===================================================================
> > --- kvm-pvfreq.orig/include/uapi/linux/kvm.h 2017-01-31 10:32:33.023378783 -0200
> > +++ kvm-pvfreq/include/uapi/linux/kvm.h 2017-01-31 10:32:38.000389402 -0200
> > @@ -871,6 +871,7 @@
> > #define KVM_CAP_S390_USER_INSTR0 130
> > #define KVM_CAP_MSI_DEVID 131
> > #define KVM_CAP_PPC_HTM 132
> > +#define KVM_CAP_ALLOW_FREQ_HC 133
> >
> > #ifdef KVM_CAP_IRQ_ROUTING
> >
> > @@ -1281,6 +1282,8 @@
> > #define KVM_S390_GET_IRQ_STATE _IOW(KVMIO, 0xb6, struct kvm_s390_irq_state)
> > /* Available with KVM_CAP_X86_SMM */
> > #define KVM_SMI _IO(KVMIO, 0xb7)
> > +#define KVM_SET_VCPU_ALLOW_FREQ_HC _IO(KVMIO, 0xb8)
> > +#define KVM_GET_VCPU_ALLOW_FREQ_HC _IO(KVMIO, 0xb9)
> >
> > #define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0)
> > #define KVM_DEV_ASSIGN_PCI_2_3 (1 << 1)
> > Index: kvm-pvfreq/arch/x86/include/uapi/asm/kvm.h
> > ===================================================================
> > --- kvm-pvfreq.orig/arch/x86/include/uapi/asm/kvm.h 2017-01-31 10:32:33.023378783 -0200
> > +++ kvm-pvfreq/arch/x86/include/uapi/asm/kvm.h 2017-01-31 10:32:38.000389402 -0200
> > @@ -357,4 +357,9 @@
> > #define KVM_X86_QUIRK_LINT0_REENABLED (1 << 0)
> > #define KVM_X86_QUIRK_CD_NW_CLEARED (1 << 1)
> >
> > +struct kvm_vcpu_allow_freq {
> > + __u16 enable;
> > + __u16 pad[7];
> > +};
> > +
> > #endif /* _ASM_X86_KVM_H */
> > Index: kvm-pvfreq/virt/kvm/kvm_main.c
> > ===================================================================
> > --- kvm-pvfreq.orig/virt/kvm/kvm_main.c 2017-01-31 10:32:33.023378783 -0200
> > +++ kvm-pvfreq/virt/kvm/kvm_main.c 2017-01-31 10:32:38.001389404 -0200
> > @@ -2938,6 +2938,8 @@
> > #endif
> > case KVM_CAP_MAX_VCPU_ID:
> > return KVM_MAX_VCPU_ID;
> > + case KVM_CAP_ALLOW_FREQ_HC:
>
> This can share an existing return 1 and would be elsewhere with
> KVM_ENABLE_CAP.

Fixed.