qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v3 5/6] iothread: add "iothread" qdev property t


From: Stefan Hajnoczi
Subject: Re: [Qemu-devel] [PATCH v3 5/6] iothread: add "iothread" qdev property type
Date: Fri, 21 Feb 2014 13:45:50 +0100

On Fri, Feb 21, 2014 at 12:33 PM, Paolo Bonzini <address@hidden> wrote:
> Il 21/02/2014 12:03, Stefan Hajnoczi ha scritto:
>
>> On Thu, Feb 20, 2014 at 01:58:06PM +0100, Paolo Bonzini wrote:
>>>>
>>>> @@ -151,6 +152,8 @@ extern PropertyInfo qdev_prop_arraylen;
>>>>     DEFINE_PROP(_n, _s, _f, qdev_prop_vlan, NICPeers)
>>>> #define DEFINE_PROP_DRIVE(_n, _s, _f) \
>>>>     DEFINE_PROP(_n, _s, _f, qdev_prop_drive, BlockDriverState *)
>>>> +#define DEFINE_PROP_IOTHREAD(_n, _s, _f)             \
>>>> +    DEFINE_PROP(_n, _s, _f, qdev_prop_iothread, IOThread *)
>>>> #define DEFINE_PROP_MACADDR(_n, _s, _f)         \
>>>>     DEFINE_PROP(_n, _s, _f, qdev_prop_macaddr, MACAddr)
>>>> #define DEFINE_PROP_LOSTTICKPOLICY(_n, _s, _f, _d) \
>>>>
>>>
>>> Should become a link sooner rather than later, but I'm not holding
>>> the series for this.
>>
>>
>> I don't mind doing the work but I don't quite understand it:
>
>
> I won't claim I understand it 100% either, in fact it is why I don't think
> it should block the series.  But we have actual link users and thus actual
> bugs, which we should fix.
>
>
>> Links are a special QOM property type: a unidirectional relationship
>> where the property holds the path and a reference to another object.
>>
>> I don't understand how the link's reference is released since
>> object_property_add_link() internally doesn't pass a release() function
>> pointer to object_property_add().  I also don't see callers explicitly
>> calling object_unref() on their link pointer.  Any ideas?
>
>
> Bug, I guess.
>
>
>> I'm concerned that existing object_property_add_link() users are
>> assigning the link pointer without incrementing the reference count like
>> object_set_link_property() would.  That sounds like a recipe for
>> disaster if someone uses qom-set or equivalent.
>
>
> This is okay; object_property_add_link reference count takes ownership of
> the original value of the pointer.

Here's an example:

static void pxa2xx_pcmcia_initfn(Object *obj)
{
    ...
    object_property_add_link(obj, "card", TYPE_PCMCIA_CARD,
                             (Object **)&s->card, NULL);
}

/* Insert a new card into a slot */
int pxa2xx_pcmcia_attach(void *opaque, PCMCIACardState *card)
{
    PXA2xxPCMCIAState *s = (PXA2xxPCMCIAState *) opaque;
    PCMCIACardClass *pcc;

    if (s->slot.attached) {
        return -EEXIST;
    }

    if (s->cd_irq) {
        qemu_irq_raise(s->cd_irq);
    }

    s->card = card;
    pcc = PCMCIA_CARD_GET_CLASS(s->card);

    s->slot.attached = true;
    s->card->slot = &s->slot;
    pcc->attach(s->card);

    return 0;
}

On one hand "card" is a link.  On the other hand we manually assign to
s->card without using object_property_set_link() and without
object_ref(card).

This is broken.  We're abusing the link property because the
pxa2xx_pcmcia_attach() function is semantically different from
object_property_set_link().  What's worse is that you can still call
object_property_set_link() and it will not perform the extra steps
that pxa2xx_pcmcia_attach() is taking to raise an IRQ and prevent
attaching to an already attached slot.

Either I don't understand QOM links or pxa2xx is broken code.

> The real disaster is that links cannot be "locked" at realize time.  For
> this to happen, links need to have a setter like object_property_add_str
> (not sure if they need a getter).

Yes, this would allow the weird pxa2xx example to behavior itself
better because _attach() would become the set() callback function.

>> The rng device examples don't seem to help because there is no way to
>> specify the rng backend via a qdev property (we always create a default
>> backend).  I need to be able to specify the object via a qdev property
>> to the virtio-blk-pci device.
>
>
> You can do that, see virtio-rng-pci.  It creates a link and forwards that to
> virtio-rng.

No, virtio-rng-pci has no rng qdev property.  The user cannot set it
on the command-line:

#define DEFINE_VIRTIO_RNG_PROPERTIES(_state, _conf_field)                    \
        DEFINE_PROP_UINT64("max-bytes", _state, _conf_field.max_bytes,       \
                           INT64_MAX),                                       \
        DEFINE_PROP_UINT32("period", _state, _conf_field.period_ms, 1 << 16)

And here's the proof:

$ x86_64-softmmu/qemu-system-x86_64 -device virtio-rng-pci,\?
virtio-rng-pci.indirect_desc=on/off
virtio-rng-pci.event_idx=on/off
virtio-rng-pci.max-bytes=uint64
virtio-rng-pci.period=uint32
virtio-rng-pci.addr=pci-devfn
virtio-rng-pci.romfile=str
virtio-rng-pci.rombar=uint32
virtio-rng-pci.multifunction=on/off
virtio-rng-pci.command_serr_enable=on/off

>> Do I need to define a link<> qdev property:
>> DEFINE_PROP_LINK("iothread", _state, _field.iothread, TYPE_IOTHREAD,
>> IOThread *)
>
>
> Perhaps, but to do that we need to first fix object_property_add_link.

Okay, I'll start working on that and use "x-iothread" in the meantime
for this series.

Stefan



reply via email to

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