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.
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/virtualization
