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.
