Re: [Qemu-devel] [PATCH v2 14/23] hyperv: qom-ify SynIC

[Roman Kagan]

On Thu, Jun 29, 2017 at 05:05:46PM +0200, Igor Mammedov wrote:
> On Wed, 21 Jun 2017 19:24:15 +0300
> Roman Kagan <address@hidden> 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.