Re: [PATCH] KVM: Sanitize cpuid

["Roedel, Joerg" <Joerg.Roedel@amd.com>]

On Thu, May 12, 2011 at 09:23:45AM -0400, Avi Kivity wrote:
> Instead of blacklisting known-unsupported cpuid leaves, whitelist known-
> supported leaves.  This is more conservative and prevents us from reporting
> features we don't support.  Also whitelist a few more leaves while at it.

Good improvement, some small annotations inline.

> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
> 
> Joerg, if you can review the AMD bits... I was somewhat dazed and confused
> when I got there (but tried to continue).
> 
>  arch/x86/kvm/x86.c |   37 +++++++++++++++++++++++++++++++++++--
>  1 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 77c9d867..78149d3 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2283,6 +2283,13 @@ static void do_cpuid_1_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  	entry->flags = 0;
>  }
>  
> +static bool supported_xcr0_bit(unsigned bit)
> +{
> +	u64 mask = ((u64)1 << bit);
> +
> +	return mask & (XSTATE_FP | XSTATE_SSE | XSTATE_YMM) & host_xcr0;
> +}
> +
>  #define F(x) bit(X86_FEATURE_##x)
>  
>  static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
> @@ -2393,6 +2400,8 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  		}
>  		break;
>  	}
> +	case 9:
> +		break;
>  	case 0xb: {
>  		int i, level_type;
>  
> @@ -2414,7 +2423,7 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  
>  		entry->flags |= KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
>  		for (i = 1; *nent < maxnent && i < 64; ++i) {
> -			if (entry[i].eax == 0)
> +			if (entry[i].eax == 0 || !supported_xcr0_bit(i))
>  				continue;
>  			do_cpuid_1_ent(&entry[i], function, i);
>  			entry[i].flags |=
> @@ -2451,6 +2460,24 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  		entry->ecx &= kvm_supported_word6_x86_features;
>  		cpuid_mask(&entry->ecx, 6);
>  		break;
> +	case 0x80000008: {
> +		u8 g_phys_as = entry->eax >> 16;
> +		u8 virt_as = max(entry->eax >> 8, 48U);

Shouldn't that be 'max((entry->eax >> 8) & 0xff, 48U)'? Seems safer when
the entry->eax contains a non-zero g_phys value.

> +		u8 phys_as = entry->eax;
> +
> +		if (!g_phys_as)
> +			g_phys_as = phys_as;
> +		entry->eax = g_phys_as | (virt_as << 8);

It is optional, but since we support nesting we can also encode
g_phys_as in bits 23:16.

> +		entry->ebx = entry->edx = 0;
> +		break;
> +	}
> +	case 0x80000019:
> +		entry->ecx = entry->edx = 0;
> +		break;
> +	case 0x8000001a:
> +		break;
> +	case 0x8000001d:
> +		break;
>  	/*Add support for Centaur's CPUID instruction*/
>  	case 0xC0000000:
>  		/*Just support up to 0xC0000004 now*/
> @@ -2460,10 +2487,16 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  		entry->edx &= kvm_supported_word5_x86_features;
>  		cpuid_mask(&entry->edx, 5);
>  		break;
> +	case 3: /* Processor serial number */
> +	case 5: /* MONITOR/MWAIT */
> +	case 6: /* Thermal management */
> +	case 0xA: /* Architectural Performance Monitoring */
> +	case 0x80000007: /* Advanced power management */
>  	case 0xC0000002:
>  	case 0xC0000003:
>  	case 0xC0000004:
> -		/*Now nothing to do, reserved for the future*/
> +	default:
> +		entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
>  		break;
>  	}
>  
> -- 
> 1.7.4.3
>