Re: [PATCH 1/3] KVM paravirt_ops infrastructure

Rusty Russell
2007-05-31T01:02:27Z

On Wed, 2007-05-30 at 09:52 -0500, Anthony Liguori wrote:
> This patch adds the basic infrastructure for paravirtualizing a KVM
> guest.

Hi Anthony!

Nice patch, comments below.

> Discovery of running under KVM is done by sharing a page of memory
> between
> the guest and host (initially through an MSR write).

I missed the shared page in this patch? If you are going to do that,
perhaps putting the hypercall magic in that page is a good idea?

> +extern unsigned char hypercall_addr[4];

Perhaps in a header?

> +asm (
> + ".globl hypercall_addr\n"
> + ".align 4\n"
> + "hypercall_addr:\n"
> + "movl $-38, %eax\n"
> + "ret\n"
> +);

I don't think we want the hypercall returning Linux error numbers, and
magic numbers are bad too. ud2 here I think.

> + para_state->guest_version = KVM_PARA_API_VERSION;
> + para_state->host_version = -1;
> + para_state->size = sizeof(*para_state);
> + para_state->ret = 0;
> + para_state->hypercall_gpa = __pa(hypercall_addr);

Two versions, size *and* ret? This seems like overkill...

> + if (wrmsr_safe(MSR_KVM_API_MAGIC, __pa(para_state), 0)) {
> + printk(KERN_INFO "KVM guest: WRMSR probe failed.\n");
> + return -ENOENT;
> + }

How about printk(KERN_INFO "I am not a KVM guest\n");?

> +static int __init kvm_guest_init(void)
> +{
> + int rc;
> +
> + rc = kvm_guest_register_para(smp_processor_id());
> + if (rc) {
> + printk(KERN_INFO "paravirt KVM unavailable\n");

Double-printk when KVM isn't detected seems overkill. Perhaps you could
just fold this all into one function...

(Personal gripe: I consider a variable named "rc" to be an admission of
semantic defeat... "err" would be better here...)

Thanks!
Rusty.