Re: [patch] KVM: add MSR based hypercall API

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Ingo Molnar wrote:
> * Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote:
>
>   
>>> @@ -237,6 +238,8 @@ struct kvm_vcpu {
>>> 	unsigned long cr0;
>>> 	unsigned long cr2;
>>> 	unsigned long cr3;
>>> +	struct kvm_vcpu_para_state *para_state;
>>>  
>>>       
>> Do we want this as part of kvm_vcpu or kvm?  I can see arguments for 
>> both views.
>>     
>
> definitely needs to be a property of the vcpu. For example the cr3 cache 
> is attached to the physical CPU. SMP scalability necessiates this too - 
> we want to use the para_state to pass information between the guest and 
> the host without any hypercall.
>
>   

Ok, agreed.

It makes guest-side registration a bit more icky (need execute the code 
on all vcpus).

>>> +struct kvm_cr3_cache {
>>> +	struct kvm_cr3_cache_entry entry[KVM_CR3_CACHE_SIZE];
>>> +	u32 max_idx;
>>> +};
>>>  
>>>       
>> This will require an api version bump whenever KVM_CR3_CACHE_SIZE 
>> changes.
>>
>> Better to advertise the gpa of the cache, so it can be unlimited.
>>     
>
> the gpa of the cache, and its guest-side size, right?
>
>   

Yes (can use max_idx, no?).

BTW, max_idx is ambiguous: is it the last valid entry or one past the 
end?  entry_count is more explicit IMO.

>>> +
>>> +	struct kvm_cr3_cache cr3_cache;
>>> +
>>> +} __attribute__ ((aligned(PAGE_SIZE)));
>>>  
>>>       
>> Perhaps packed too, to avoid 32/64 ambiguity.  Or even better, pad it 
>> explicitly to avoid unaligned fields.
>>     
>
> it should already be padded - i layed it out that way. (if it's not then 
> let me know where it's not padded)
>
>   

Right, I was confused by the cr3 cache, but it's the last field.

> +
> +	if (kvm_arch_ops->patch_hypercall) {
>   

It's safe to assume that the arch op exists.

> +EXPORT_SYMBOL_GPL(gpa_to_hpa);
>   

Is this needed now?  If so, it needs a kvm_ prefix.

> @@ -1448,6 +1448,17 @@ static int handle_io(struct kvm_vcpu *vc
>  	return 0;
>  }
>  
> +static void
> +vmx_patch_hypercall(struct kvm_vcpu *vcpu, unsigned char *hypercall)
> +{
> +	/*
> +	 * Patch in the VMCALL instruction:
> +	 */
> +	hypercall[0] = 0x0f;
> +	hypercall[1] = 0x01;
> +	hypercall[2] = 0xc1;
> +}
> +
>  static int handle_cr(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  {
>  	u64 exit_qualification;
> @@ -2042,6 +2053,7 @@ static struct kvm_arch_ops vmx_arch_ops 
>  	.run = vmx_vcpu_run,
>  	.skip_emulated_instruction = skip_emulated_instruction,
>  	.vcpu_setup = vmx_vcpu_setup,
> +	.patch_hypercall = vmx_patch_hypercall,
>  };
>   
Where is the vmcall exit handler?

Please add the svm code too.  I can test it if you lack amd hardware.

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV