Re: [PATCH 07/13] KVM: x86: API changes for SMM support

[Bandan Das <bsd@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> This patch includes changes to the external API for SMM support.
> All the changes are predicated by the availability of a new
> capability, KVM_CAP_X86_SMM, which is added at the end of the
> patch series.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  Documentation/virtual/kvm/api.txt | 34 ++++++++++++++++++++++++++++++----
>  arch/x86/include/asm/kvm_host.h   |  1 +
>  arch/x86/include/uapi/asm/kvm.h   |  7 +++++++
>  arch/x86/kvm/kvm_cache_regs.h     |  5 +++++
>  arch/x86/kvm/x86.c                | 30 +++++++++++++++++++++++++-----
>  include/uapi/linux/kvm.h          |  5 ++++-
>  6 files changed, 72 insertions(+), 10 deletions(-)
...
>
>  
> +#define KVM_RUN_X86_SMM		 (1 << 0)
> +
>  /* for KVM_GET_REGS and KVM_SET_REGS */
>  struct kvm_regs {
>  	/* out (KVM_GET_REGS) / in (KVM_SET_REGS) */
> @@ -281,6 +283,7 @@ struct kvm_reinject_control {
>  #define KVM_VCPUEVENT_VALID_NMI_PENDING	0x00000001
>  #define KVM_VCPUEVENT_VALID_SIPI_VECTOR	0x00000002
>  #define KVM_VCPUEVENT_VALID_SHADOW	0x00000004
> +#define KVM_VCPUEVENT_VALID_SMM		0x00000008

Note the formatting above is different in Linus' tree.
I can't seem to find where it changed.
/* When set in flags, include corresponding fields on KVM_SET_VCPU_EVENTS */
#define KVM_VCPUEVENT_VALID_NMI_PENDING	0x00000001
#define KVM_VCPUEVENT_VALID_SIPI_VECTOR	0x00000002
#define KVM_VCPUEVENT_VALID_SHADOW	0x00000004

...
> +static int kvm_vcpu_ioctl_smi(struct kvm_vcpu *vcpu)
> +{
> +	return 0;
> +}
> +
>  static int vcpu_ioctl_tpr_access_reporting(struct kvm_vcpu *vcpu,
>  					   struct kvm_tpr_access_ctl *tac)
>  {
> @@ -3116,29 +3121,31 @@ static void kvm_vcpu_ioctl_x86_get_vcpu_events(struct kvm_vcpu *vcpu,
>  					       struct kvm_vcpu_events *events)
>  {
>  	process_nmi(vcpu);
> +
> +	memset(events, 0, sizeof(*events));
I think that it's better that the caller memsets this before passing it over
to the *_get_vcpu_events function.
...
>  
>  	/* in (pre_kvm_run), out (post_kvm_run) */
>  	__u64 cr8;
> @@ -814,6 +814,7 @@ struct kvm_ppc_smmu_info {
>  #define KVM_CAP_S390_INJECT_IRQ 113
>  #define KVM_CAP_S390_IRQ_STATE 114
>  #define KVM_CAP_PPC_HWRNG 115
> +#define KVM_CAP_X86_SMM 120
Why didn't we reserve the next available number here ?

>  #ifdef KVM_CAP_IRQ_ROUTING
>  
> @@ -1199,6 +1200,8 @@ struct kvm_s390_ucas_mapping {
>  /* Available with KVM_CAP_S390_IRQ_STATE */
>  #define KVM_S390_SET_IRQ_STATE	  _IOW(KVMIO, 0xb5, struct kvm_s390_irq_state)
>  #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_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
>  #define KVM_DEV_ASSIGN_PCI_2_3		(1 << 1)