Re: [Qemu-devel] [PATCH] qemu-kvm: Add CPUID support for VIA CPU

[Jan Kiszka]

On 2011-05-06 03:06, address@hidden wrote:
> Hi, Jan
>     Thank you very much for your advice. That's helpful for me.
> 
>> Hi,
>>
>> the subject's tag (qemu-kvm) is misleading. This is actually targeting 
>> the uq/master patch queue, i.e. the upstream kvm staging area.
>>
> 
> If I want to submit a patch for the qemu-kvm-git,  should I use  
> "[QEMU-DEVEL][Patch]..." as the subject? Or there are other rules for 
> qemu-kvm upstream? If yes, would you like to tell me?. Thanks!

If you really have to target qemu-kvm only, then you tagging is fine.
But this patch does not qualify for such exclusiveness. Rather, your
feature should go into upstream's KVM first and will then be merged back
into qemu-kvm on next update.

> 
>> On 2011-05-05 05:03, address@hidden wrote:
>>> When KVM is running on VIA CPU with host cpu's model, the
>> feautures of
>>> VIA CPU will be passed into kvm guest by calling the CPUID
>> instruction
>>> for Centaur.
>>>
>>> Signed-off-by: BrillyWu<address@hidden>
>>> Signed-off-by: KaryJin<address@hidden>
>>> ---
>>>  target-i386/cpu.h   |    7 +++++++
>>>  target-i386/cpuid.c |   48
>>> +++++++++++++++++++++++++++++++++++++++++++++++-
>>
>> You patch is unfortunately line-wrapped.
> 
> Yes, I will be careful the next time.
> 
>>> @@ -721,6 +725,9 @@ typedef struct CPUX86State {
>>>      uint32_t cpuid_ext3_features;
>>>      uint32_t cpuid_apic_id;
>>>      int cpuid_vendor_override;
>>> +    /*Store the results of Centaur's CPUID instructions*/
>>
>> Please format comments like this /* comment text */, ie. with blanks 
>> after/before the /* / */.
> OK, I will check it.
>>
>>> +1050,15 @@ void cpu_x86_cpuid(CPUX86State *env, uin
>>>                     uint32_t *ecx, uint32_t *edx)  {
>>>      /* test if maximum index reached */
>>> -    if (index & 0x80000000) {
>>> +    if ((index & 0xC0000000) == 0xC0000000) {
>>> +   /* Handle the Centaur's CPUID instruction.*
>>> +   * If cpuid_xlevel2 is "0", then put into the*
>>> +   * default case. */
>>> +   if (env->cpuid_xlevel2 == 0)
>>> +       index = 0xF0000000;
>>> +   else if (index > env->cpuid_xlevel2)
>>> +       index = env->cpuid_xlevel2;
>>
>> Please validate your patch before posting with scripts/checkpatch.pl.
> 
> OK, I will do it. 
> I found that space is used to code indent other than tab, should I follow it 
> or use tab instead in my patch?
> If I use space, there are some warnings when using scripts/checkpatch.pl to 
> validate the patch. Can I ignore them?

Generally, the advices checkpatch provides are valid and shall be
applied. If you feel like you came across a corner case where the script
reports nonsense, please post your findings to qemu-devel.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux