On Thu, Jun 29, 2017 at 05:05:46PM +0200, Igor Mammedov wrote:
> On Wed, 21 Jun 2017 19:24:15 +0300
> Roman Kagan <[email protected]> wrote:
> > +static void synic_realize(DeviceState *dev, Error **errp)
> > +{
> > + Object *obj = OBJECT(dev);
> > + SynICState *synic = SYNIC(dev);
> > +
> > + synic->cpu = X86_CPU(obj->parent);
> usually devices shouldn't access parent this way
>
> [...]
> > +void hyperv_synic_add(X86CPU *cpu)
> this helper is called by/from parent so something like below should do
>
> > +{
> > + Object *obj;
> > +
> > + obj = object_new(TYPE_SYNIC);
> > + object_property_add_child(OBJECT(cpu), "synic", obj, &error_abort);
> > + object_unref(obj);
>
> SynICState *synic = SYNIC(obj)
> synic->cpu = cpu;
Indeed, this is better.
> or even make synic->cpu a link (object_property_add_link) and set it
> here, benefit will be when synic is introspected via QOM it will show
> that it refers back/uses the cpu + proper reference counting of CPU object.
Good point, will look into it, thanks.
> > + if (cpu->hyperv_synic) {
> > + if (kvm_vcpu_enable_cap(CPU(cpu), KVM_CAP_HYPERV_SYNIC, 0)) {
> > + fprintf(stderr, "failed to enable Hyper-V SynIC\n");
> > + return -ENOSYS;
> > + }
> > +
> > + hyperv_synic_add(cpu);
> is synic KVM specific or may it work with TCG accel?
No, it's exclusively KVM. Actually most of it sits in the kernel.
> in either case, looks like hyperv_synic_add() should be called from
> x86_cpu_realizefn(), the same like we do with APIC creating it
> depending feature being enabled.
I'm not sure I understand the reason, in view of it being exclusively
KVM.
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -1084,6 +1092,8 @@ void kvm_arch_reset_vcpu(X86CPU *cpu)
> > for (i = 0; i < ARRAY_SIZE(env->msr_hv_synic_sint); i++) {
> > env->msr_hv_synic_sint[i] = HV_SINT_MASKED;
> > }
> > +
> > + hyperv_synic_reset(cpu);
> ditto, could go to x86_cpu_machine_reset_cb()
> also calling it unconditionally will crash QEMU if
> get_synic() returns NULL (i.e. when feature is not enabled).
The context is not visibile in the patch, but this is actually called
inside
if (cpu->hyperv_synic) {
...
}
so no, it won't return NULL.
Thanks,
Roman.
