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

[Lee Essen]

On 16 Mar 2012, at 11:56, Paolo Bonzini wrote:

> Il 16/03/2012 10:23, Lee Essen ha scritto:
>> +    while (mlock(base, (nbytes = step * ps)) == -1) {
>> +        if (errno != EAGAIN) {
>> +            return -1;
>> +        }
>> +
>> +        if (waiting == 0) {
>> +            waiting = gethrtime();
> 
> Please use qemu_get_clock_ns(rt_clock) here.

ok. done in my upcoming revision.

>> +#ifndef __sun__
>> const floatx80 floatx80_default_nan =
>> make_floatx80(floatx80_default_nan_high,
>> 
>> floatx80_default_nan_low);
>> +#endif
> 
> Why is this needed.

This is actually an issue with -std=gnu99, I believe Andreas has a patch for 
this, so I will remove it from
my ones. 

>> +#ifdef __sun__
>> +#include <sys/kvm.h>
>> +#else
>> #include <linux/kvm.h>
>> #include <linux/kvm_para.h>
>> +#endif
> 
> Perhaps you can put this in kvm.h instead, and just include that file:
> 
> #ifdef __sun__
> #include <sys/kvm.h>
> #else
> #include <linux/kvm.h>
> #endif
> #include <linux/kvm_para.h>
> 
> kvm_para.h should be independent of the host OS, hoping there's no
> conflict between Solaris and Linux headers.
> 

I've created a set of headers which are a mix of the original Illumos ones and 
the linux ones
and put them in solaris-headers. It all seems to work nicely and results in 
much cleaner code.

>> +#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 */
> 
> What is this needed for?

[discussed (and hopefully improved) in other email thread]

>> 
>> +    env->kvm_fd = ret;
>> +    env->kvm_state = kvm_state;
>> +
>> +    ret = ioctl(env->kvm_fd, KVM_CREATE_VCPU, env->cpu_index);
>> +#else
>>     ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, env->cpu_index);
>> +#endif
>>     if (ret < 0) {
>>         DPRINTF("kvm_create_vcpu failed\n");
>>         goto err;
>>     }
>> 
>> +#ifndef CONFIG_SOLARIS
>>     env->kvm_fd = ret;
>>     env->kvm_state = s;
>> +#endif
> 
> env->kvm_state assignment need not be split, right?

Correct … but actually this raises another question … why create a separate 
pointer to kvm_state…

    KVMState *s = kvm_state;

… why not just use kvm_state throughout the function?

This seems to be a common approach in many of the functions in kvm-all.c … is 
there a reason?

>> 
>> +#ifdef CONFIG_EVENTFD
>>     int ret;
>>     struct kvm_ioeventfd iofd;
>> 
>> @@ -1665,10 +1737,14 @@ int kvm_set_ioeventfd_mmio_long(int fd, uint32_t
>> addr, uint32_t val, bool assign
>>     }
>> 
>>     return 0;
>> +#else
>> +    return -ENOSYS;
>> +#endif
> 
> The guard probably needs to be added also around ioeventfd definitions
> in virtio-pci.c.

With the full set of header files this doesn't seem to be needed anymore anyway.

> 
> Did you have any problem with IOV_MAX?  IIRC, it's quite low (16
> perhaps) in Solaris.
> 

Hmmm, yes IOV_MAX is 16, I've not seen any issues with this yet .. although I 
haven't
really looked … there do seem to be places not considering IOV_MAX, I'll have a
deeper look when I get a chance.

Cheers,

Lee.