Re: [PATCH 1/3] Implement bare minimum of HYPER-V MSRs.

[Avi Kivity <avi@redhat.com>]

On 01/13/2010 03:59 PM, Gleb Natapov wrote:
> Minimum HYPER-V implementation should have GUEST_OS_ID, HYPERCALL and
> VP_INDEX MSRs.
>
>
> diff --git a/arch/x86/include/asm/kvm_hyperv.h b/arch/x86/include/asm/kvm_hyperv.h
> new file mode 100644
> index 0000000..91211f3
> --- /dev/null
> +++ b/arch/x86/include/asm/kvm_hyperv.h
>    

Please name this asm/hyperv.h, so it can be used for 
Linux-as-Hyper-V-guest, not just Linux-as-host-impersonating-Hyper-V.+

Also put this in a separate patch.


> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 6811e5e..6972b2b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -628,7 +628,8 @@ static u32 msrs_to_save[] = {
>   #ifdef CONFIG_X86_64
>   	MSR_CSTAR, MSR_KERNEL_GS_BASE, MSR_SYSCALL_MASK, MSR_LSTAR,
>   #endif
> -	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA
> +	MSR_IA32_TSC, MSR_IA32_PERF_STATUS, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
> +	HV_X64_MSR_GUEST_OS_ID, HV_X64_MSR_HYPERCALL
>    

End with trailing comma so future patches are nicer.

>
> +static bool kvm_hv_hypercall_enabled(struct kvm *kvm)
> +{
> +	return !!(kvm->arch.hv_hypercall&  HV_X64_MSR_HYPERCALL_ENABLE);
> +}
>    

!! is unnecessary for bool:

_Bool and(unsigned x) { return x & 16; }

0000000000000010 <and>:
   10:   c1 ef 04                shr    $0x4,%edi
   13:   89 f8                   mov    %edi,%eax
   15:   83 e0 01                and    $0x1,%eax
   18:   c3                      retq



> +
> +static int set_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> +{
> +	struct kvm *kvm = vcpu->kvm;
> +
> +	switch (msr) {
> +	case HV_X64_MSR_GUEST_OS_ID:
> +		kvm->arch.hv_guest_os_id = data;
> +		/* setting guest os id to zero disables hypercall page */
> +		if (!kvm->arch.hv_guest_os_id)
> +			kvm->arch.hv_hypercall&= ~HV_X64_MSR_HYPERCALL_ENABLE;
> +		break;
> +	case HV_X64_MSR_HYPERCALL: {
> +		u64 gfn;
> +		unsigned long addr;
> +		/* if guest os id is not set hypercall should remain disabled */
> +		if (!kvm->arch.hv_guest_os_id&&  data)
> +			break;
> +		kvm->arch.hv_hypercall = data;
> +		if(!kvm_hv_hypercall_enabled(kvm))
> +			break;
> +		gfn = kvm->arch.hv_hypercall>>
> +			HV_X64_MSR_HYPERCALL_PAGE_ADDRESS_SHIFT;
> +		addr = gfn_to_hva(kvm, gfn);
> +		if (kvm_is_error_hva(addr))
> +			return 1;
> +		kvm_x86_ops->patch_hypercall(vcpu, (unsigned char*)addr);
> +		((unsigned char*)addr)[3] = 0xc3; /* ret */
> +		break;
> +	}
> +	default:
> +		pr_unimpl(vcpu, "HYPER-V unimplemented wrmsr: 0x%x "
> +			  "data 0x%llx\n", msr, data);
> +		return 1;
> +	}
> +	return 0;
> +}
>    

We need locking in case a malicious guest issues partition-wide msrs 
from multiple vcpus simultaneously.

>   int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>   {
>   	switch (msr) {
> @@ -1117,6 +1181,16 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>   		pr_unimpl(vcpu, "unimplemented perfctr wrmsr: "
>   			"0x%x data 0x%llx\n", msr, data);
>   		break;
> +	case HV_X64_MSR_GUEST_OS_ID ... HV_X64_MSR_SINT15:
> +		if (kvm_hv_msr_partition_wide(msr)) {
> +			int r;
> +			mutex_lock(&vcpu->kvm->lock);
> +			r = set_msr_hyperv_pw(vcpu, msr, data);
> +			mutex_unlock(&vcpu->kvm->lock);
>    

We do have locking.  Any reason not to put it in set_msr_hyperv_pw?  
Seems cleaner.

> +static int get_msr_hyperv_pw(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
> +{
> +	u64 data = 0;
> +	struct kvm *kvm = vcpu->kvm;
> +
> +	switch (msr) {
> +	case HV_X64_MSR_GUEST_OS_ID:
> +		data = kvm->arch.hv_guest_os_id;
> +		break;
> +	case HV_X64_MSR_HYPERCALL:
> +		data = kvm->arch.hv_hypercall;
> +		break;
>    

This could be non-atomic on i386.  I don't think it matters.

>   int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>   {
>   	unsigned long nr, a0, a1, a2, a3, ret;
>   	int r = 1;
>
> +	if(kvm_hv_hypercall_enabled(vcpu->kvm))
> +		return kvm_hv_hypercall(vcpu);
> +
>    

Space after if.

-- 
error compiling committee.c: too many arguments to function