Re: [Qemu-devel] [PATCH] Initial support for Ilumos build and Illumos-kvm

[Lee Essen]

On 16 Mar 2012, at 13:14, Jan Kiszka wrote:

> On 2012-03-16 10:23, Lee Essen wrote:
>> +#ifdef __sun__
>> +#include <sys/kvm.h>
>> +#else
>> #include <linux/kvm.h>
>> #include <linux/kvm_para.h>
>> +#endif
> 
> As Paolo already said, this should somehow be centralised.

Yep, fair point. I'll address this one.

> Also, CONFIG_SOLARIS vs. __sun__: please use a consistent pattern.

Hmmm … I was trying to be consistent with the existing style :-) … see 
__linux__ and CONFIG_LINUX as well. I'll see what I can do to make this a bit 
tidier.

>> +#ifdef CONFIG_SOLARIS
>> +    for (p = (caddr_t)mem.userspace_addr;
>> +      p < (caddr_t)mem.userspace_addr + mem.memory_size;
>> +      p += PAGE_SIZE)
>> +        c = *p;
>> +#endif /* CONFIG_SOLARIS */
>> +
> 
> I bet gcc will like this write-only pattern and bark at you.
> 

It does indeed … this came from the original Joyent code, I must admit I did 
wonder whether gcc would optimise it away. I did consider adding something to 
stop gcc complaining, but I don't fully understand why this is necessary given 
the mlock() bit, so I thought it best to leave it alone.

Any suggestions?

>> +#else
>>     ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, env->cpu_index);
>> +#endif
> 
> There is no chance to fix the Solaris KVM to do fd cloning in the kernel
> and implement the same KVM_CREATE_VCPU ABI?
> 

I will raise this with the joyent guys, but they are pretty switched on and I 
suspect there is a reason.

My concern with the "fix the kernel" comments is that it would exclude the use 
of the newer qemu on existing installations, however I do understand the desire 
to not fill the code with workarounds that live forever.

How about a "broken_solaris_kvm_abi" option to configure with a suitable set of 
defines wrapping the code?

>> 
>> #ifdef CONFIG_KVM
>> +#ifdef __sun__
>> +#include <sys/kvm.h>
>> +/*
>> + * it's a bit horrible to include these here, but the kvm_para.h include 
>> file
>> + * isn't public with the illumos kvm implementation
> 
> Just provide a package of properly fixed kernel headers and let us carry
> them in solaris-headers or so, analogously to linux-headers.
> 

Interestingly this is what I did originally but then thought it best to use the 
"supplied" headers, but actually thinking more about it, this does make much 
more sense.

Regards,

Lee.