Re: [Qemu-devel] [PATCH 12/16] IPMI: Add a PC ISA type structure

[Corey Minyard]

On 07/16/2012 02:16 AM, Paolo Bonzini wrote:
> Il 15/07/2012 22:25, address@hidden ha scritto:
> > +    /* Clear the property from this device so we can put it elsewhere */
> > +    chr = isa->chr;
> > +    qdev_prop_set_chr(&dev->qdev, "chardev", NULL);
> > +
> > +    if (chr) {
> > +       bdev = qdev_create(NULL, "ipmi-bmc-extern");
> > +       qdev_prop_set_chr(bdev, "chardev", chr);
> > +    } else {
> > +       bdev = qdev_create(NULL, "ipmi-bmc-sim");
> > +    }
> > +    snprintf(typename, sizeof(typename), "ipmi-interface-%s", isa->interface);
> > +    idev = qdev_create(&isa_bus->qbus, typename);
> > +    qdev_prop_set_ptr(bdev, "ipmiif", idev);
> > +    qdev_prop_set_uint64(idev, "iobase", isa->iobase);
> > +    qdev_prop_set_ptr(idev, "bmc", bdev);
> > +    qdev_prop_set_uint8(idev, "slave-addr", isa->slave_addr);
> > +    rc = qdev_init(idev);
> I think this is adding a bit of extra complication.  You do not need to
> add explicit devices for this.

Yeah, but the qdevs did a lot of nice things for me :).  But I understand.

> If you want to make ipmi-bmc-* and ipmi-interface-* objects, you can
> create them with object_new, set the properties manually with the
> returned pointer and add the object to the parent with
> object_property_add_child.

The main reason I went with qdevs originally was because all the 
properties are defined for qdevs.  I'd have to do all my own properties 
otherwise.

However, I think all the properties are represented by what is already 
there in isa-ipmi, so just having pointers to the interface and bmc 
objects is ok, I think.

> Otherwise, it should also be fine if you just add them as function pointers.
>
> Either way also lets you keep the ownership of the chardev in the ISA
> IPMI device, and avoids that "info qtree" looks strange and does not
> show the chardev you assigned.

Yeah, I see that now.  Makes sense.

BTW, where do you want me to document this?  I didn't see an obvious 
place in qemu-options.hx.

I'm working on tests, too.  I didn't find much there already, but I'll 
see what I can add.

Thanks,

-corey