On Tue, Aug 20, 2019 at 12:46:50AM +0300, Nikita Leshenko wrote:
> The checks are written in the same order and structure as they appear in "SDM
> 26.3.1.5 - Checks on Guest Non-Register State", to ease verification.
> 
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Signed-off-by: Nikita Leshenko <nikita.leshchenko@oracle.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 24 ++++++++++++++++++++++++
>  arch/x86/kvm/vmx/vmcs.h   | 13 +++++++++++++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 24734946ec75..e2ee217f8ffe 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2666,10 +2666,34 @@ static int nested_vmx_check_vmcs_link_ptr(struct kvm_vcpu *vcpu,
>   */
>  static int nested_check_guest_non_reg_state(struct vmcs12 *vmcs12)
>  {
> +	/* Activity state must contain supported value */
>  	if (vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE &&
>  	    vmcs12->guest_activity_state != GUEST_ACTIVITY_HLT)
>  		return -EINVAL;
>  
> +	/* Must not be HLT if SS DPL is not 0 */
> +	if (VMX_AR_DPL(vmcs12->guest_ss_ar_bytes) != 0 &&
> +	    vmcs12->guest_activity_state == GUEST_ACTIVITY_HLT)
> +		return -EINVAL;
> +
> +	/* Must be active if blocking by MOV-SS or STI */
> +	if ((vmcs12->guest_interruptibility_info &
> +	    (GUEST_INTR_STATE_MOV_SS | GUEST_INTR_STATE_STI)) &&
> +	    vmcs12->guest_activity_state != GUEST_ACTIVITY_ACTIVE)

IMO, following the SDM verbatim doesn't help readability in this case.
E.g. filtering out ACTIVE state right away cuts down on the amount of
code and helps the reader focus on the unique aspects of each check.

	if (vmcs12->guest_activity_state == GUEST_ACTIVITY_ACTIVE)
		return 0;

> +		return -EINVAL;
> +
> +	/* In HLT, only some interruptions are allowed */
> +	if (vmcs12->vm_entry_intr_info_field & INTR_INFO_VALID_MASK &&
> +	    vmcs12->guest_activity_state == GUEST_ACTIVITY_HLT) {
> +		u32 intr_info = vmcs12->vm_entry_intr_info_field;
> +		if (!is_ext_interrupt(intr_info) &&
> +		    !is_nmi(intr_info) &&
> +		    !is_debug(intr_info) &&
> +		    !is_machine_check(intr_info) &&
> +		    !is_mtf(intr_info))
> +		    return -EINVAL;

Bad indentation.  scripts/checkpatch.pl will catch this.  It'll also
complain about the changelog running past 75 chars and about "Missing a
blank line after declarations" for u32 intr_info, both of which are
easy nits to fix.

> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/arch/x86/kvm/vmx/vmcs.h b/arch/x86/kvm/vmx/vmcs.h
> index cb6079f8a227..c5577c40b19d 100644
> --- a/arch/x86/kvm/vmx/vmcs.h
> +++ b/arch/x86/kvm/vmx/vmcs.h
> @@ -102,6 +102,13 @@ static inline bool is_machine_check(u32 intr_info)
>  		(INTR_TYPE_HARD_EXCEPTION | MC_VECTOR | INTR_INFO_VALID_MASK);
>  }
>  
> +static inline bool is_mtf(u32 intr_info)

We may want to call this is_pending_mtf().  MTF has a dedicated VM-Exit
type and doesn't populate VM_EXIT_INTRO_INFO, while all of the other
helpers are valid for both VM-Enter and VM-Exit.

> +{
> +	return (intr_info & (INTR_INFO_INTR_TYPE_MASK | INTR_INFO_VECTOR_MASK |
> +			     INTR_INFO_VALID_MASK)) ==
> +		(INTR_TYPE_OTHER_EVENT | 0 | INTR_INFO_VALID_MASK);

This reminded me a patch I've been meaning to write.  Any objection to
carrying the attached patch as cleanup and reworking this code accordingly?

> +}
> +
>  /* Undocumented: icebp/int1 */
>  static inline bool is_icebp(u32 intr_info)
>  {
> @@ -115,6 +122,12 @@ static inline bool is_nmi(u32 intr_info)
>  		== (INTR_TYPE_NMI_INTR | INTR_INFO_VALID_MASK);
>  }
>  
> +static inline bool is_ext_interrupt(u32 intr_info)
> +{
> +	return (intr_info & (INTR_INFO_INTR_TYPE_MASK | INTR_INFO_VALID_MASK))
> +		== (INTR_TYPE_EXT_INTR | INTR_INFO_VALID_MASK);
> +}
> +
>  enum vmcs_field_width {
>  	VMCS_FIELD_WIDTH_U16 = 0,
>  	VMCS_FIELD_WIDTH_U64 = 1,
> -- 
> 2.20.1
>