Re: [PATCHv2 3/4] Add HYPER-V apic access MSRs.

[Avi Kivity <avi@redhat.com>]

On 01/17/2010 02:36 PM, Gleb Natapov wrote:
> +
>>> +int kvm_hv_vapic_msr_read(struct kvm_vcpu *vcpu, u32 reg, u64 *data)
>>> +{
>>> +	struct kvm_lapic *apic = vcpu->arch.apic;
>>> +	u32 low, high = 0;
>>> +
>>> +	if (!irqchip_in_kernel(vcpu->kvm))
>>> +		return 1;
>>> +
>>> +	if (apic_reg_read(apic, reg, 4,&low))
>>> +		return 1;
>>> +	if (reg == APIC_ICR)
>>> +		apic_reg_read(apic, APIC_ICR2, 4,&high);
>>> +
>>> +	*data = (((u64)high)<<   32) | low;
>>> +
>>> +	return 0;
>>> +}
>>>        
>> I prefer putting this in x86.c (maybe later split into hyperv.c).
>>
>>      
> This implements part of apic behaviour. It uses internal lapic functions
> like apic_reg_read()/apic_reg_write(). Why move it from lapic.c?
>    

The new functions implement hyper-v behaviour.  Why scatter them all around?

Maybe apic_reg_{read,write} need to be exported.

>>>   static int set_msr_hyperv(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>>>   {
>>> -	pr_unimpl(vcpu, "HYPER-V unimplemented wrmsr: 0x%x data 0x%llx\n",
>>> -		  msr, data);
>>> +	switch (msr) {
>>> +	case HV_X64_MSR_APIC_ASSIST_PAGE: {
>>> +		unsigned long vaddr;
>>> +		void *addr;
>>> +		struct page *page;
>>> +		vcpu->arch.hv_vapic = data;
>>> +		if (!kvm_hv_vapic_enabled(vcpu))
>>> +			break;
>>> +		vaddr = gfn_to_hva(vcpu->kvm, data>>
>>> +				  HV_X64_MSR_APIC_ASSIST_PAGE_ADDRESS_SHIFT);
>>> +		if (kvm_is_error_hva(vaddr))
>>> +			return 1;
>>> +		page = virt_to_page(vaddr);
>>>        
>> virt_to_page() takes a kernel address, not a user address.  This is
>> get_user_pages().  But I think the whole thing is done better with
>> put_user().
>>
>>      
> So there is no function to get struct page from user virtual address?
>    

get_user_pages_fast().


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