Re: [PATCH 3/3] x86/kvm/hyper-v: inject #GP only when invalid SINTx vector is unmasked

[Roman Kagan]

On Wed, Feb 28, 2018 at 02:44:01PM +0100, Vitaly Kuznetsov wrote:
> Hyper-V 2016 on KVM with SynIC enabled doesn't boot with the following
> trace:
> 
>     kvm_entry:            vcpu 0
>     kvm_exit:             reason MSR_WRITE rip 0xfffff8000131c1e5 info 0 0
>     kvm_hv_synic_set_msr: vcpu_id 0 msr 0x40000090 data 0x10000 host 0
>     kvm_msr:              msr_write 40000090 = 0x10000 (#GP)
>     kvm_inj_exception:    #GP (0x0)

I don't remember having seen this...  Does this happen with the mainline
QEMU, which doesn't set the SintPollingModeAvailable (17) bit in cpuid
0x40000003:edx?

> 
> KVM acts according to the following statement from TLFS:
> 
> "
> 11.8.4 SINTx Registers
> ...
> Valid values for vector are 16-255 inclusive. Specifying an invalid
> vector number results in #GP.
> "
> 
> However, I checked and genuine Hyper-V doesn't #GP when we write 0x10000
> to SINTx. I checked with Microsoft and they confirmed that if either the
> Masked bit (bit 16) or the Polling bit (bit 18) is set to 1, then they
> ignore the value of Vector. Make KVM act accordingly.

I wonder if that cpuid setting affects this behavior?  Also curious what
exactly the guest is trying to achieve writing this bogus value?

> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> ---
>  arch/x86/include/uapi/asm/hyperv.h | 1 +
>  arch/x86/kvm/hyperv.c              | 7 ++++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/uapi/asm/hyperv.h b/arch/x86/include/uapi/asm/hyperv.h
> index 62c778a303a1..a492dc357bd7 100644
> --- a/arch/x86/include/uapi/asm/hyperv.h
> +++ b/arch/x86/include/uapi/asm/hyperv.h
> @@ -326,6 +326,7 @@ typedef struct _HV_REFERENCE_TSC_PAGE {
>  #define HV_SYNIC_SIEFP_ENABLE		(1ULL << 0)
>  #define HV_SYNIC_SINT_MASKED		(1ULL << 16)
>  #define HV_SYNIC_SINT_AUTO_EOI		(1ULL << 17)
> +#define HV_SYNIC_SINT_POLLING		(1ULL << 18)
>  #define HV_SYNIC_SINT_VECTOR_MASK	(0xFF)
>  
>  #define HV_SYNIC_STIMER_COUNT		(4)
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index 6d14f808145d..d3d866c32976 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -95,9 +95,14 @@ static int synic_set_sint(struct kvm_vcpu_hv_synic *synic, int sint,
>  			  u64 data, bool host)
>  {
>  	int vector, old_vector;
> +	bool masked, polling;
>  
>  	vector = data & HV_SYNIC_SINT_VECTOR_MASK;
> -	if (vector < HV_SYNIC_FIRST_VALID_VECTOR && !host)
> +	masked = data & HV_SYNIC_SINT_MASKED;
> +	polling = data & HV_SYNIC_SINT_POLLING;
> +
> +	if (vector < HV_SYNIC_FIRST_VALID_VECTOR &&
> +	    !host && !masked && !polling)
>  		return 1;
>  	/*
>  	 * Guest may configure multiple SINTs to use the same vector, so

I'm not sure this is enough to implement the polling mode: per spec,

> Setting the polling bit will have the effect of unmasking an interrupt
> source, except that an actual interrupt is not generated.

However, if the guest sets a valid vector and the masked bit cleared,
we'll consider it a usual SINT and add to masks and inject interrupts,
etc, regardless of the polling bit.

I must admit I'm confused by the above quote from the spec: is the
polling bit supposed to come together with the masked bit?  If so, then
we probably should validate it here (but your logs indicate otherwise).
In general I'm missing the utility of this mode: why should an interrupt
controller be involved in polling at all?

Roman.