Re: [PATCH 1/3] KVM paravirt_ops infrastructure

[Rusty Russell <rusty@rustcorp.com.au>]

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.