[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
- [Qemu-devel] [PATCH v3 0/6] dataplane: switch to N:M devices-per-thread model, Stefan Hajnoczi, 2014/02/20
- [Qemu-devel] [PATCH v3 2/6] aio: add aio_context_acquire() and aio_context_release(), Stefan Hajnoczi, 2014/02/20
- [Qemu-devel] [PATCH v3 1/6] rfifolock: add recursive FIFO lock, Stefan Hajnoczi, 2014/02/20
- [Qemu-devel] [PATCH v3 3/6] iothread: add I/O thread object, Stefan Hajnoczi, 2014/02/20
- [Qemu-devel] [PATCH v3 4/6] qdev: add get_pointer_and_free() for temporary strings, Stefan Hajnoczi, 2014/02/20
- [Qemu-devel] [PATCH v3 5/6] iothread: add "iothread" qdev property type, Stefan Hajnoczi, 2014/02/20
- Re: [Qemu-devel] [PATCH v3 5/6] iothread: add "iothread" qdev property type, Paolo Bonzini, 2014/02/20
- Re: [Qemu-devel] [PATCH v3 5/6] iothread: add "iothread" qdev property type, Stefan Hajnoczi, 2014/02/21
- Re: [Qemu-devel] [PATCH v3 5/6] iothread: add "iothread" qdev property type, Paolo Bonzini, 2014/02/21
- Re: [Qemu-devel] [PATCH v3 5/6] iothread: add "iothread" qdev property type,
Stefan Hajnoczi <=
- Re: [Qemu-devel] [PATCH v3 5/6] iothread: add "iothread" qdev property type, Paolo Bonzini, 2014/02/21
- Re: [Qemu-devel] [PATCH v3 5/6] iothread: add "iothread" qdev property type, Stefan Hajnoczi, 2014/02/21
- Re: [Qemu-devel] [PATCH v3 5/6] iothread: add "iothread" qdev property type, Paolo Bonzini, 2014/02/21
- Re: [Qemu-devel] [PATCH v3 5/6] iothread: add "iothread" qdev property type, Stefan Hajnoczi, 2014/02/21
[Qemu-devel] [PATCH v3 6/6] dataplane: replace internal thread with IOThread, Stefan Hajnoczi, 2014/02/20