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

[Claudio Fontana <cfontana@suse.de>]

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.

Thanks,

Ciao,

Claudio