Re: [PULL 07/17] i386: move hyperv_vendor_id initialization to x86_cpu_realizefn()

[Claudio Fontana]

On 12/18/20 2:05 AM, Eduardo Habkost wrote:
> On Fri, Dec 18, 2020 at 01:07:33AM +0100, Claudio Fontana wrote:
>> On 12/18/20 12:47 AM, Eduardo Habkost wrote:
>>> On Fri, Dec 18, 2020 at 12:34:46AM +0100, Claudio Fontana wrote:
>>>> On 12/17/20 11:53 PM, Eduardo Habkost wrote:
>>>>> On Thu, Dec 17, 2020 at 11:33:57PM +0100, Claudio Fontana wrote:
>>>>>> Hello all,
>>>>>>
>>>>>> On 12/17/20 7:46 PM, Eduardo Habkost wrote:
>>>>>>> From: Vitaly Kuznetsov <vkuznets@redhat.com>
>>>>>>>
>>>>>>> As a preparation to expanding Hyper-V CPU features early, move
>>>>>>> hyperv_vendor_id initialization to x86_cpu_realizefn(). Introduce
>>>>>>> x86_cpu_hyperv_realize() to not not pollute x86_cpu_realizefn()
>>>>>>> itself.
>>>>>>
>>>>>> this seems to fit very well the ongoing work on separating accelerator 
>>>>>> specific realize functions;
>>>>>>
>>>>>> related to the previous discussions about the class hierarchies,
>>>>>> do you think that we should have a separate class in target/i386/kvm/ 
>>>>>> for a hyperv variant of the kvm-cpu.c?
>>>>>>
>>>>>> Should it be a separate class or a subclass of "kvm-accel-x86_64-cpu" ?
>>>>>
>>>>> I don't see how a separate QOM class for Hyper-V would be helpful
>>>>> here.  What would be the problem you are trying to solve in this
>>>>> case?
>>>>
>>>> there is now a call to accelerator specific code x86_hyperv_cpu_realize 
>>>> just before cpu_exec_realize,
>>>> tying the generic target/i386/cpu.c code to kvm/hyperv-specific accel 
>>>> initialization.
>>>>
>>>> if this is just a feature of the kvm accel, maybe I should just move all 
>>>> to kvm-cpu.c and that's it.
>>>
>>> That would make sense.  If we decide this is a KVM-specific
>>> feature, this code can be moved to kvm_cpu_realizefn(), provided
>>> by the kvm-accel-x86_64-cpu class added by your series.
>>>
>>> However, I'm not sure we can say this is a KVM-specific feature.
>>> The feature is currently only supported by the KVM accelerator,
>>> but I'd say it is a generic feature.
>>>
>>
>> Maybe in the future it will be a generic feature, and we can export it the 
>> right way?
>> It will be super-easy if the feature is well contained.
>>
>> Until it really is generic though, should it really appear in the middle of 
>> x86_cpu_realizefn?
>> currently the generic code in target/i386/cpu.c contains an unconditional 
>> call to:
>>
>> x86_cpu_hyperv_realize(cpu);
>>
>> before the call to cpu_exec_realizefn().
>>
>> the function is defined in this very same file before as:
>>
>> static void x86_cpu_hyperv_realize(X86CPU *cpu)
>> {
>>  ...
>> }
>>
>> with a bunch of initializations of hyperv specific data, for example 
>> cpu->hyperv_interface_id, cpu->hyperv_vendor_id, cpu->hyperv_version_id.
>>
>> That data is ever only used in kvm/kvm.c, which is built conditionally under 
>> CONFIG_KVM.
>>
>> So the existing situation is fairly inconsistent in my view, at least for 
>> how things are now, and in principle the extra code of the initializations 
>> for hyperv should never be executed if !CONFIG_KVM.
> 
> What's the problem you are trying to solve?  Speed?  Binary size?
> Something else?
> 
> Moving the code in x86_cpu_hyperv_realize() to kvm-cpu.c sounds
> like a drop in the bucket.  It wouldn't get rid of
> hyperv-specific code in cpu.c, which includes:
> - the "hv-*" properties in x86_cpu_properties[];
> - FEAT_HYPERV_* CPUID leaf definitions;
> - hyperv code in x86_cpu_get_crash_info().
> 
> I wouldn't object if somebody really wants to move it, but I
> don't see it as a problem.
> 

Nm I'll take a look later on.

Thanks!

Claudio