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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3429 bytes --]

On 2012-03-16 14:46, Lee Essen wrote:
> 
> 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.

Maybe QEMU isn't consistent as well. :)

> 
>>> +#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?

First of all: understand if and why this is needed. Talk to the Joyent
people, check if it works without, comment on the why. But please do not
just dump code that may date back to early solaris-kvm days and were
possibly just hacks. This is upstream here and should ideally carry only
the cleaned up versions (we are trying to achieve this during the
qemu-kvm -> qemu upstreaming as well).

> 
>>> +#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?

Well, if there are working, considered stable versions of solaris-kvm
out there that expose this ABI, we probably want to support this anyway.
If the released stuff is experimental only anyway and can be changed
before it becomes stable, then lets go for that destination.

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

Pushing fixed-up headers to qemu should still be only an temporary
solution. Fixing the headers upstream so that future solaris-kvm
versions provide them properly remains a worthwhile goal nevertheless.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]