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

[Marcelo Tosatti <mtosatti@redhat.com>]

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.