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