qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PULL 07/17] i386: move hyperv_vendor_id initialization to x86_cpu_r


From: Eduardo Habkost
Subject: Re: [PULL 07/17] i386: move hyperv_vendor_id initialization to x86_cpu_realizefn()
Date: Thu, 17 Dec 2020 20:05:40 -0500

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.

-- 
Eduardo




reply via email to

[Prev in Thread] Current Thread [Next in Thread]