Re: [kvm-devel] [PATCH 1/3] KVM paravirt_ops infrastructure

[Anthony Liguori <anthony@codemonkey.ws>]

Rusty Russell wrote:
> 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?
>   

para_state is the shared page.  The address is passed to the KVM via the 
MSR (so it's a shared page owned by the guest).

>> +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.
>   

Yeah, you're not the first one to suggest this.  The thing is, KVM 
already has host-side support for a hypercall API.  I didn't want to 
change that unless I had to.  However, based on the prior feedback re: 
using CPUID, I will be changing it so I'll update this too.

>> +       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...
>   

Yeah, I agree :-)  I actually am not a huge fan of using version 
numbers.  I think I'm going to try the next patch using a single version 
number and a feature bitmap.  Some of the optimizations (like MMU 
batching) don't make sense in a NPT/EPT environment but the guest 
shouldn't have to be aware of that.

>> +       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...
>   

Already have.

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

I'm not sure I agree that's one's better than the other.  Although I 
guess if (err) { reads a little better...

Regards,

Anthony Liguori

> Thanks!
> Rusty.
>
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by DB2 Express
> Download DB2 Express C - the FREE version of DB2 express and take
> control of your XML. No limits. Just data. Click to get it now.
> http://sourceforge.net/powerbar/db2/
> _______________________________________________
> kvm-devel mailing list
> kvm-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>
>