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.
