Virtualization - Jun 2007 - [PATCH 0/5] KVM paravirt_ops implementation

Hi,

This patch series is an update of my previous paravirt_ops patches.  
They are loosely based on Ingo's original paravirt_ops implementation 
for KVM.  Some of the changes since the last series include:

1) Switch to using CPUID 0x40000000 instead of using MSR writes to 
discover shared memory area
2) Attempt to deal with SMP guests
3) Support for generic CR read caching
4) Support for batching of MMU operations

Some known issues:

1) Not really sure what is needed for CONFIG_PREEMPT support.  I'm not 
sure which paravirt_ops calls are actually re-entrant.
2) The paravirt_ops implementation is registered with core_initcall().  
However, the paravirt_ops banner is also printed with core_initcall() so 
that fact that this works now is just the luck of build order.  Need a 
better way to initialize the KVM paravirt_ops backend.
3) These patches probably break guest save/restore.  Need a generic way 
to get additional save/restore callbacks in the kernel (perhaps the 
in-kernel APIC series addresses this already?).

The latest versions of these patches are available at:

http://hg.codemonkey.ws/kvm-paravirt

Regards,

Anthony Liguori

Regards,

Anthony Liguori
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kvm-paravirt-core.diff
Type: text/x-patch
Size: 14058 bytes
Desc: not available
Url :
http://lists.linux-foundation.org/pipermail/virtualization/attachments/20070617/a65e02e4/kvm-paravirt-core.bin

Regards,

Anthony Liguori
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kvm-cr-caching.diff
Type: text/x-patch
Size: 4391 bytes
Desc: not available
Url :
http://lists.linux-foundation.org/pipermail/virtualization/attachments/20070617/11c1a07b/kvm-cr-caching.bin

Regards,

Anthony Liguori
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kvm-mmu-write.diff
Type: text/x-patch
Size: 5151 bytes
Desc: not available
Url :
http://lists.linux-foundation.org/pipermail/virtualization/attachments/20070617/226f09eb/kvm-mmu-write-0001.bin

Regards,

Anthony Liguori
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kvm-hypercall-queue.diff
Type: text/x-patch
Size: 9670 bytes
Desc: not available
Url :
http://lists.linux-foundation.org/pipermail/virtualization/attachments/20070617/3c200e1a/kvm-hypercall-queue.bin

Regards,

Anthony Liguori
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kvm-paravirt-time.diff
Type: text/x-patch
Size: 4464 bytes
Desc: not available
Url :
http://lists.linux-foundation.org/pipermail/virtualization/attachments/20070617/8ec9938c/kvm-paravirt-time.bin

Anthony Liguori wrote:
> 1) Not really sure what is needed for CONFIG_PREEMPT support.  I'm not
> sure which paravirt_ops calls are actually re-entrant.
I'm not sure that has specifically come up.  The main issue is whether a
particular call can be preempted and whether that matters.  I guess the
calls which affect a particular CPU's state will generally be called in
a non-preemptable context, but I guess we can't assume that; the best
approach is to assume that each call be atomic with respect to preemption.

Things like batching must be completed with preemption disabled over the
whole batch.  I check that with BUG_ON in the Xen code.
> 2) The paravirt_ops implementation is registered with
> core_initcall().  However, the paravirt_ops banner is also printed
> with core_initcall() so that fact that this works now is just the luck
> of build order.  Need a better way to initialize the KVM paravirt_ops
> backend.
Hm.  We could make the banner printing later; obviously its purely
cosmetic.  I put it as a core_initcall on the assumption that pv-ops
would be set up very early as it is with Xen and lguest, and so the
banner should print relatively early.  But if your model is that you
boot fully virtualized for a while, and then become paravirtualized
later, then it would make sense to defer banner printing until then.

    J

Anthony Liguori wrote:
> With hardware virtualization, CR reads often times require a VMEXIT which
is
> rather expensive.  Instead of reading CR and taking the VMEXIT, maintain a
> copy of each CR and return that on CR reads.
>   
Looks good.  Any measurable performance impact?

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

Anthony Liguori wrote:
> +/*
> + * Control register reads can be trapped.  Since trapping is relatively
> + * expensive, we can avoid paying the cost by caching logically.
> + */
> +static unsigned long kvm_read_cr(int reg)
> +{
> +	struct kvm_paravirt_state *state
> +		= per_cpu(paravirt_state, smp_processor_id());
> +
> +	if (unlikely(!state->cr_valid[reg])) {
> +		if (reg == 0)
> +			state->cached_cr[reg] = native_read_cr0();
> +		else if (reg == 3)
> +			state->cached_cr[reg] = native_read_cr3();
> +		else if (reg == 4)
> +			state->cached_cr[reg] = native_read_cr4();
> +		else
> +			BUG();
> +		state->cr_valid[reg] = 1;
> +	}
> +	return state->cached_cr[reg];
> +}
> +
It would be good to declare this (and kvm_write_cr) always_inline.  
These functions are never called with a non-constant reg parameters, and 
the unsightly if tree (more readable as a switch, IMO) will fold nicely 
when inlined.


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

Anthony Liguori wrote:
> +static int kvm_hypercall_get_ktime(struct kvm_vcpu *vcpu, gva_t va)
> +{
> +	struct timespec now;
> +	int ret;
> +
> +	ktime_get_ts(&now);
> +
> +	ret = kvm_write_guest(vcpu, va, sizeof(now), &now);
> +	if (unlikely(ret))
> +		return -EFAULT;
> +
> +	return 0;
> +}
Please use physical addresses (much faster, less possibility of 
confusion).  struct kvm_timespec instead of timespec. KVM_EFAULT instead 
of EFAULT.



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

Avi Kivity wrote:
> Anthony Liguori wrote:
>> +/*
>> + * Control register reads can be trapped.  Since trapping is
relatively
>> + * expensive, we can avoid paying the cost by caching logically.
>> + */
>> +static unsigned long kvm_read_cr(int reg)
>> +{
>> +    struct kvm_paravirt_state *state
>> +        = per_cpu(paravirt_state, smp_processor_id());
>> +
>> +    if (unlikely(!state->cr_valid[reg])) {
>> +        if (reg == 0)
>> +            state->cached_cr[reg] = native_read_cr0();
>> +        else if (reg == 3)
>> +            state->cached_cr[reg] = native_read_cr3();
>> +        else if (reg == 4)
>> +            state->cached_cr[reg] = native_read_cr4();
>> +        else
>> +            BUG();
>> +        state->cr_valid[reg] = 1;
>> +    }
>> +    return state->cached_cr[reg];
>> +}
>> +
>
> It would be good to declare this (and kvm_write_cr) always_inline.  
> These functions are never called with a non-constant reg parameters, 
> and the unsightly if tree (more readable as a switch, IMO) will fold 
> nicely when inlined.
Ok.

Regards,

Anthony Liguori