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

Avi Kivity
2007-01-09T11:24:42Z

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