On Fri, Jan 18, 2013 at 12:11:29PM +0100, Andreas Färber wrote:
[...]
> > +/* Returns VCPU ID to be used on KVM_CREATE_VCPU ioctl() */
> > +unsigned long kvm_arch_vcpu_id(CPUState *cpu);
> > +
> > void kvm_arch_reset_vcpu(CPUState *cpu);
> >
> > int kvm_arch_on_sigbus_vcpu(CPUState *cpu, int code, void *addr);
> > diff --git a/kvm-all.c b/kvm-all.c
> > index 6278d61..995220d 100644
> > --- a/kvm-all.c
> > +++ b/kvm-all.c
> > @@ -222,7 +222,7 @@ int kvm_init_vcpu(CPUState *cpu)
> >
> > DPRINTF("kvm_init_vcpu\n");
> >
> > - ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, cpu->cpu_index);
> > + ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, kvm_arch_vcpu_id(cpu));
> > if (ret < 0) {
> > DPRINTF("kvm_create_vcpu failed\n");
> > goto err;
>
> This is changing the vararg from int to unsigned long. I have no
> insights yet on how this is handled and whether that is okay; I would at
> least expect this change to be mentioned in the commit message.
It was an unexpected change (I didn't notice that cpu_index was int),
but strictly speaking the previous code was incorrect (as ioctl() gets
an unsigned long argument, not int). I doubt there are cases where it
would really break, but it is a good thing to fix it.
I agree this should be mentioned in the commit message, though. Will you
add it before committing, or should I resubmit?
>
> > diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> > index 3acff40..5f3f789 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -411,6 +411,11 @@ static void cpu_update_state(void *opaque, int running, RunState state)
> > }
> > }
> >
> > +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> > +{
> > + return cpu->cpu_index;
> > +}
> > +
> > int kvm_arch_init_vcpu(CPUState *cs)
> > {
> > struct {
>
> Minor nit: If you change this to CPUState *cs you spare the renaming in
> 05/12. Alternatively use x86_cpu there (not much code affected so you
> can just ignore this, no need to respin just for that).
>
> Otherwise looks okay to me.
I actually wanted to rename the variable only when necessary, otherwise
this patch would be confusing if all architectures used 'cpu' and i386
used 'cs'.
(And I like using "cpu" for the more specific CPU type in the function
[e.g. CPUState or X86CPUState depending on the case] and abbreviations
[like 'cs'] for the more generic types. I believe I have seen this style
used in other parts of the code.)
>
> Andreas
>
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index 19e9f25..1e544ae 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -384,6 +384,11 @@ static inline void kvm_fixup_page_sizes(PowerPCCPU *cpu)
> >
> > #endif /* !defined (TARGET_PPC64) */
> >
> > +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> > +{
> > + return cpu->cpu_index;
> > +}
> > +
> > int kvm_arch_init_vcpu(CPUState *cs)
> > {
> > PowerPCCPU *cpu = POWERPC_CPU(cs);
> > diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> > index 6ec5e6d..bd9864c 100644
> > --- a/target-s390x/kvm.c
> > +++ b/target-s390x/kvm.c
> > @@ -72,6 +72,11 @@ int kvm_arch_init(KVMState *s)
> > return 0;
> > }
> >
> > +unsigned long kvm_arch_vcpu_id(CPUState *cpu)
> > +{
> > + return cpu->cpu_index;
> > +}
> > +
> > int kvm_arch_init_vcpu(CPUState *cpu)
> > {
> > int ret = 0;
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
--
Eduardo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
