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 <vkuzn...@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.
--
Eduardo
