[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() fu
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function |
Date: |
Fri, 18 Jan 2013 10:53:40 -0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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
[Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function, Eduardo Habkost, 2013/01/17
- Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function, Andreas Färber, 2013/01/18
- Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function,
Eduardo Habkost <=
- Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function, Andreas Färber, 2013/01/18
- Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function, Eduardo Habkost, 2013/01/18
- Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function, Eric Blake, 2013/01/18
- Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function, Eduardo Habkost, 2013/01/18
- Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function, Eric Blake, 2013/01/18
- Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function, Andreas Färber, 2013/01/21
- Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function, Eric Blake, 2013/01/21
- Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function, Eduardo Habkost, 2013/01/22
Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function, Marcelo Tosatti, 2013/01/21
[Qemu-devel] [PATCH for-1.4 07/12] target-i386/cpu: Introduce apic_id_for_cpu() function, Eduardo Habkost, 2013/01/17