Re: [Qemu-devel] [PATCH for-1.4 04/12] kvm: Create kvm_arch_vcpu_id() function

[Andreas Färber]

Am 18.01.2013 13:53, schrieb Eduardo Habkost:
> 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?

Could you suggest a text for me to add please?

>>> 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'.

It's inconsistent anyway, 'cs' is relatively new and I see no reason to
use it in the prototype.
But OK, once 03/12 gets an ack I'll start applying.

> 
> (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.)

Yes, I chose to use "cpu" for the more frequently used type. I don't
really like "cs" but it seemed better than "base_cpu" or so. When
changing something over from "env" something with three letters looks
nicer though, easier to review. Can't have everything. ;)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg