Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
Paolo Bonzini writes: > On 19/03/20 08:01, Markus Armbruster wrote: >> Paolo Bonzini writes: >> >>> On 18/03/20 16:06, Markus Armbruster wrote: > - object initialization should cause no side effects outside the subtree > of the object object_initialize_child() and its users like sysbus_init_child_obj() violate this rule: they add a child property to the subtree's parent. Correct? >>> >>> At least object_initialize_child() adds the property to the object >>> itself, so it's fine. >> >> It seems to >> >> 1. Initialize @childobj >> 2. Set a bunch of properties >> 3. Add the child property to @parentobj >> 4. Call .complete() if it's a TYPE_USER_CREATABLE >> 5. Adjust reference count >> >> Step 3. modifies outside the sub-tree rooted at @childobj. What am I >> missing? >> >> Hmm, maybe this: using object_initialize_child() when initializing >> @parentobj is fine; while object_initialize_child() leaves the sub-tree >> rooted at @childobj, it still stays within the sub-tree rooted at >> @parentobj. >> >> If this is how the function must be used, its contract should spell it >> out! > > Yes, this is what I meant. > > - realization can also cause side effects outside the subtree of the > object, but if unrealization is possible, they must be undone by > unrealization. if an object is realized and unrealization is not > possible, finalization will never run (and in that case, side effects of > realization need not be undone at all). One possible answer the question what should be done in realize() is whatever must not be done earlier. Is that the best we can do? >>> >>> That's the lower bound of descriptivity. The upper bound is anything >>> that is visible from the guest. The truth could be in the middle. >> >> Can we set aside a bit of time to write docs/devel/qom.rst together? I >> know object.h is lovingly commented, but unless you already know how QOM >> works, you're drowning in detail there. You'd have to provide most of >> the contents, I'm afraid, because you know so much more about it. Could >> save you time in the long run, though: fewer questions to answer, fewer >> mistakes to correct. > > Yes, this is sorely needed. I will do it; if you have more random > questions you'd like an answer for, that can help. I can then stitch > them together in a coherent text. A year ago, we discussed QOM interfaces: Subject: Issues around TYPE_INTERFACE Message-ID: <87h8c82woh@dusky.pond.sub.org> We touched naming conventions, the fact that interfaces can't have state, how an interface type should be defined (even though it has no state), and what it means to convert to such a type. I'd love to see this covered in qom.rst.
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 19/03/20 08:01, Markus Armbruster wrote: > Paolo Bonzini writes: > >> On 18/03/20 16:06, Markus Armbruster wrote: - object initialization should cause no side effects outside the subtree of the object >>> >>> object_initialize_child() and its users like sysbus_init_child_obj() >>> violate this rule: they add a child property to the subtree's parent. >>> Correct? >> >> At least object_initialize_child() adds the property to the object >> itself, so it's fine. > > It seems to > > 1. Initialize @childobj > 2. Set a bunch of properties > 3. Add the child property to @parentobj > 4. Call .complete() if it's a TYPE_USER_CREATABLE > 5. Adjust reference count > > Step 3. modifies outside the sub-tree rooted at @childobj. What am I > missing? > > Hmm, maybe this: using object_initialize_child() when initializing > @parentobj is fine; while object_initialize_child() leaves the sub-tree > rooted at @childobj, it still stays within the sub-tree rooted at > @parentobj. > > If this is how the function must be used, its contract should spell it > out! Yes, this is what I meant. - realization can also cause side effects outside the subtree of the object, but if unrealization is possible, they must be undone by unrealization. if an object is realized and unrealization is not possible, finalization will never run (and in that case, side effects of realization need not be undone at all). >>> >>> One possible answer the question what should be done in realize() is >>> whatever must not be done earlier. Is that the best we can do? >> >> That's the lower bound of descriptivity. The upper bound is anything >> that is visible from the guest. The truth could be in the middle. > > Can we set aside a bit of time to write docs/devel/qom.rst together? I > know object.h is lovingly commented, but unless you already know how QOM > works, you're drowning in detail there. You'd have to provide most of > the contents, I'm afraid, because you know so much more about it. Could > save you time in the long run, though: fewer questions to answer, fewer > mistakes to correct. Yes, this is sorely needed. I will do it; if you have more random questions you'd like an answer for, that can help. I can then stitch them together in a coherent text. Paolo
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
Paolo Bonzini writes: > On 18/03/20 16:06, Markus Armbruster wrote: >>> - object initialization should cause no side effects outside the subtree >>> of the object >> >> object_initialize_child() and its users like sysbus_init_child_obj() >> violate this rule: they add a child property to the subtree's parent. >> Correct? > > At least object_initialize_child() adds the property to the object > itself, so it's fine. It seems to 1. Initialize @childobj 2. Set a bunch of properties 3. Add the child property to @parentobj 4. Call .complete() if it's a TYPE_USER_CREATABLE 5. Adjust reference count Step 3. modifies outside the sub-tree rooted at @childobj. What am I missing? Hmm, maybe this: using object_initialize_child() when initializing @parentobj is fine; while object_initialize_child() leaves the sub-tree rooted at @childobj, it still stays within the sub-tree rooted at @parentobj. If this is how the function must be used, its contract should spell it out! A review of existing uses for misuses may be in order. > sysbus_init_child_obj() adds a link to the object to the sysbus object > (via qdev_set_parent_bus/bus_add_child). This does violate the rule. > However, currently we have: > > - create link on qdev_set_parent_bus, before realizing > > - remove link on device_unparent, after unrealizing By "create link", do you mean bus_add_child() in qdev_set_parent_bus()? By "remove link", do you mean bus_remove_child() in device_unparent()? > which makes the device setup even more complicated than necessary. In > other words I don't think the bug is in the function, instead it > probably makes sense to do something else: > > - create link in device_set_realized, before calling ->pre_plug (undo on > failure) > > - remove link in device_set_realized, after calling ->unrealize (if it > succeeds) > > and leave sysbus_init_child_obj() as is. I'm barely following you. Me reviewing an actual patch could help. >>> - setting properties can cause side effects outside the subtree of the >>> object (e.g. marking an object as "in use"), but they must be undone by >>> finalization. >> >> Textbook example is the 1:1 connection between device frontend and >> backend: block frontend's property "drive", char frontend's property >> "chardev", NIC frontend property "netdev", ... >> >> Can we come up with some guardrails for what property setting may do? >> Affecting guest-visible state is off limits. What else is? > > Yes, guest-visible state is off limits until realization. > >>> - realization can also cause side effects outside the subtree of the >>> object, but if unrealization is possible, they must be undone by >>> unrealization. if an object is realized and unrealization is not >>> possible, finalization will never run (and in that case, side effects of >>> realization need not be undone at all). >> >> One possible answer the question what should be done in realize() is >> whatever must not be done earlier. Is that the best we can do? > > That's the lower bound of descriptivity. The upper bound is anything > that is visible from the guest. The truth could be in the middle. Can we set aside a bit of time to write docs/devel/qom.rst together? I know object.h is lovingly commented, but unless you already know how QOM works, you're drowning in detail there. You'd have to provide most of the contents, I'm afraid, because you know so much more about it. Could save you time in the long run, though: fewer questions to answer, fewer mistakes to correct.
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 18/03/20 16:06, Markus Armbruster wrote: >> - object initialization should cause no side effects outside the subtree >> of the object > > object_initialize_child() and its users like sysbus_init_child_obj() > violate this rule: they add a child property to the subtree's parent. > Correct? At least object_initialize_child() adds the property to the object itself, so it's fine. sysbus_init_child_obj() adds a link to the object to the sysbus object (via qdev_set_parent_bus/bus_add_child). This does violate the rule. However, currently we have: - create link on qdev_set_parent_bus, before realizing - remove link on device_unparent, after unrealizing which makes the device setup even more complicated than necessary. In other words I don't think the bug is in the function, instead it probably makes sense to do something else: - create link in device_set_realized, before calling ->pre_plug (undo on failure) - remove link in device_set_realized, after calling ->unrealize (if it succeeds) and leave sysbus_init_child_obj() as is. >> - setting properties can cause side effects outside the subtree of the >> object (e.g. marking an object as "in use"), but they must be undone by >> finalization. > > Textbook example is the 1:1 connection between device frontend and > backend: block frontend's property "drive", char frontend's property > "chardev", NIC frontend property "netdev", ... > > Can we come up with some guardrails for what property setting may do? > Affecting guest-visible state is off limits. What else is? Yes, guest-visible state is off limits until realization. >> - realization can also cause side effects outside the subtree of the >> object, but if unrealization is possible, they must be undone by >> unrealization. if an object is realized and unrealization is not >> possible, finalization will never run (and in that case, side effects of >> realization need not be undone at all). > > One possible answer the question what should be done in realize() is > whatever must not be done earlier. Is that the best we can do? That's the lower bound of descriptivity. The upper bound is anything that is visible from the guest. The truth could be in the middle. Paolo
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
Paolo Bonzini writes: > On 18/03/20 14:02, Markus Armbruster wrote: >> Object instantiation must be completely reverted by finalization. >> >> device-introspect-test guards against a particularly egregious violation >> of this rule, namely output of "info qtree" after initialization + >> finalization differing from output before. Surprisingly common issue. >> It's what made Peter invite me to this thread. >> >> Device realization must be completely reverted by unrealization. > > So far so good. > >> We don't always implement unrealize. Fine when the device cannot be >> unrealized. But the code doesn't seem to guard against omitting >> unrealize when the device actually can be unrealized. >> >> Properties for use with device_add must exist after object >> instantiation. >> >> But is that true? Setting a property can run arbitrary code. What if >> setting property P to value V runs code that adds property Q? Then >> device_add P=V,Q=W works provided P gets set before Q, which is >> anybody's guess. >> >> So "must exist" is actually "should exist", and we' already moved from >> the self-evident to the unclear. > > Right, and there is already one case where the properties don't exist, > namely the "fake array" properties. I tried to strangle them in their crib, but failed. >> Even less clear: what side effects may be visible between object >> initialization and realization / finalization? >> >> A safe but constricting answer would be "only host resource >> reservation". >> >> What's your answer? > > Here are some random thoughts about it: > > - object initialization should cause no side effects outside the subtree > of the object object_initialize_child() and its users like sysbus_init_child_obj() violate this rule: they add a child property to the subtree's parent. Correct? > - setting properties can cause side effects outside the subtree of the > object (e.g. marking an object as "in use"), but they must be undone by > finalization. Textbook example is the 1:1 connection between device frontend and backend: block frontend's property "drive", char frontend's property "chardev", NIC frontend property "netdev", ... Can we come up with some guardrails for what property setting may do? Affecting guest-visible state is off limits. What else is? > - realization can also cause side effects outside the subtree of the > object, but if unrealization is possible, they must be undone by > unrealization. if an object is realized and unrealization is not > possible, finalization will never run (and in that case, side effects of > realization need not be undone at all). One possible answer the question what should be done in realize() is whatever must not be done earlier. Is that the best we can do?
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On Wed, 18 Mar 2020 at 13:22, Paolo Bonzini wrote: > Here are some random thoughts about it: > > - object initialization should cause no side effects outside the subtree > of the object > > - setting properties can cause side effects outside the subtree of the > object (e.g. marking an object as "in use"), but they must be undone by > finalization. > > - realization can also cause side effects outside the subtree of the > object, but if unrealization is possible, they must be undone by > unrealization. if an object is realized and unrealization is not > possible, finalization will never run (and in that case, side effects of > realization need not be undone at all). - if realization fails then any side effects caused by the partial realize must be undone before returning the failure. (I bet we don't always get this right, especially in cases where a subclass has to call a parent class realize method...) thanks -- PMM
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 18/03/20 14:02, Markus Armbruster wrote: > Object instantiation must be completely reverted by finalization. > > device-introspect-test guards against a particularly egregious violation > of this rule, namely output of "info qtree" after initialization + > finalization differing from output before. Surprisingly common issue. > It's what made Peter invite me to this thread. > > Device realization must be completely reverted by unrealization. So far so good. > We don't always implement unrealize. Fine when the device cannot be > unrealized. But the code doesn't seem to guard against omitting > unrealize when the device actually can be unrealized. > > Properties for use with device_add must exist after object > instantiation. > > But is that true? Setting a property can run arbitrary code. What if > setting property P to value V runs code that adds property Q? Then > device_add P=V,Q=W works provided P gets set before Q, which is > anybody's guess. > > So "must exist" is actually "should exist", and we' already moved from > the self-evident to the unclear. Right, and there is already one case where the properties don't exist, namely the "fake array" properties. > Even less clear: what side effects may be visible between object > initialization and realization / finalization? > > A safe but constricting answer would be "only host resource > reservation". > > What's your answer? Here are some random thoughts about it: - object initialization should cause no side effects outside the subtree of the object - setting properties can cause side effects outside the subtree of the object (e.g. marking an object as "in use"), but they must be undone by finalization. - realization can also cause side effects outside the subtree of the object, but if unrealization is possible, they must be undone by unrealization. if an object is realized and unrealization is not possible, finalization will never run (and in that case, side effects of realization need not be undone at all). Paolo
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
Paolo Bonzini writes: > On 16/03/20 07:03, Markus Armbruster wrote: >> Paolo Bonzini writes: >> >>> On 15/03/20 15:56, Markus Armbruster wrote: > > The question is why they are not, i.e. where does the above reasoning > break. I don't know. But let's for the sake of the argument assume this actually worked. Asking for help in the monitor then *still* has side effects visible in the time span between .instance_init() and finalization. Why is that harmless? >>> >>> I don't really have an answer, but if that is a problem we could change >>> "info qtree" to skip non-realized devices. >> >> Can we convince ourselves that "info qtree" is the *only* way to observe >> these side effects? > > There is of course qom-get/qom-set, but those _should_ show this side > effect. > > If we decide that "info qtree" should only show devices visible to the > guest (as opposed to all objects that have been created), then "show > only realized devices" is not even a hack but the correct implementation > of the concept. > > Paolo > >> If yes, a hack to ignore unrealized devices "fixes" the problem. >> >> If no, it sweeps it under the rug. I fear we're getting lost in the weeds. The question what qom-get / qom-set / info qtree should show is of no concern to me when writing a device model. I need guidance on how to spread the work between instance initialization and realize, and on the requirements for unrealize and finalize. Let's try to separate the self-evident parts from the unclear parts, starting with the self-evident. Please correct mistakes. Object instantiation must be completely reverted by finalization. device-introspect-test guards against a particularly egregious violation of this rule, namely output of "info qtree" after initialization + finalization differing from output before. Surprisingly common issue. It's what made Peter invite me to this thread. Device realization must be completely reverted by unrealization. We don't always implement unrealize. Fine when the device cannot be unrealized. But the code doesn't seem to guard against omitting unrealize when the device actually can be unrealized. Properties for use with device_add must exist after object instantiation. But is that true? Setting a property can run arbitrary code. What if setting property P to value V runs code that adds property Q? Then device_add P=V,Q=W works provided P gets set before Q, which is anybody's guess. So "must exist" is actually "should exist", and we' already moved from the self-evident to the unclear. Even less clear: what side effects may be visible between object initialization and realization / finalization? A safe but constricting answer would be "only host resource reservation". What's your answer? I've hardly begun talking about the distribution of work between initialization and realize, and I'm floundering already. May I have some clear, hard rules, please?
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 16/03/20 07:03, Markus Armbruster wrote: > Paolo Bonzini writes: > >> On 15/03/20 15:56, Markus Armbruster wrote: The question is why they are not, i.e. where does the above reasoning break. >>> I don't know. But let's for the sake of the argument assume this >>> actually worked. Asking for help in the monitor then *still* has side >>> effects visible in the time span between .instance_init() and >>> finalization. >>> >>> Why is that harmless? >> >> I don't really have an answer, but if that is a problem we could change >> "info qtree" to skip non-realized devices. > > Can we convince ourselves that "info qtree" is the *only* way to observe > these side effects? There is of course qom-get/qom-set, but those _should_ show this side effect. If we decide that "info qtree" should only show devices visible to the guest (as opposed to all objects that have been created), then "show only realized devices" is not even a hack but the correct implementation of the concept. Paolo > If yes, a hack to ignore unrealized devices "fixes" the problem. > > If no, it sweeps it under the rug.
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
Paolo Bonzini writes: > On 15/03/20 15:56, Markus Armbruster wrote: >>> >>> The question is why they are not, i.e. where does the above reasoning break. >> I don't know. But let's for the sake of the argument assume this >> actually worked. Asking for help in the monitor then *still* has side >> effects visible in the time span between .instance_init() and >> finalization. >> >> Why is that harmless? > > I don't really have an answer, but if that is a problem we could change > "info qtree" to skip non-realized devices. Can we convince ourselves that "info qtree" is the *only* way to observe these side effects? If yes, a hack to ignore unrealized devices "fixes" the problem. If no, it sweeps it under the rug.
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 15/03/20 15:56, Markus Armbruster wrote: >> >> The question is why they are not, i.e. where does the above reasoning break. > I don't know. But let's for the sake of the argument assume this > actually worked. Asking for help in the monitor then *still* has side > effects visible in the time span between .instance_init() and > finalization. > > Why is that harmless? I don't really have an answer, but if that is a problem we could change "info qtree" to skip non-realized devices. Paolo
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
Mark Cave-Ayland writes:
> On 10/03/2020 09:07, Markus Armbruster wrote:
>
>> Widespread QOM usage anti-pattern ahead; cc: QOM maintainers.
>>
>> Peter Maydell writes:
>>
>>> On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan wrote:
On 3/9/2020 5:21 PM, Peter Maydell wrote:
> Could you explain more? My thought is that we should be using
> sysbus_init_child_obj() and we should be doing it in the init method.
> Why does that break the tests ? It's the same thing various other
> devices do.
device-introspect-test do the follow check for each device type:
qtree_start = qtest_hmp(qts, "info qtree");
...
qtest_qmp(qts, "{'execute': 'device-list-properties','arguments':
{'typename': %s}}", type);
...
qtree_end = qtest_hmp(qts, "info qtree");
g_assert_cmpstr(qtree_start, ==, qtree_end);
If we do qdev_set_parent_bus in init, it will check fail when type =
'mac_via'.
mac_via_init() is called by q800_init(). But it will not be called in
qtest(-machine none) in the step qtree_start.
And after we call 'device-list-properties', mac_via_init() was called and
set dev parent bus. We can find these
devices in the qtree_end. So it break the test on the assert.
>>>
>>> Markus, do you know what's happening here? Why is
>>> trying to use sysbus_init_child_obj() breaking the
>>> device-introspect-test for this particular device,
>>> but fine for the other places where we use it?
>>> (Maybe we're accidentally leaking a reference to
>>> something so the sub-device stays on the sysbus
>>> when it should have removed itself when the
>>> device was deinited ?)
>>
>> Two questions: (1) why does it break here, and (2) why doesn't it break
>> elsewhere.
>>
>> First question: why does it break here?
>>
>> It breaks here because asking for help with "device_add mac_via,help"
>> has a side effect visible in "info qtree".
>>
Here is the output:
# Testing device 'mac_via'
>> [Uninteresting stuff snipped...]
>>
>> "info qtree" before asking for help:
>>
qtree_start: bus: main-system-bus
type System
>>
>> "info qtree" after asking for help:
>>
qtree_end: bus: main-system-bus
type System
dev: mos6522-q800-via2, id ""
gpio-in "via2-irq" 8
gpio-out "sysbus-irq" 1
frequency = 0 (0x0)
mmio /0010
dev: mos6522-q800-via1, id ""
gpio-in "via1-irq" 8
gpio-out "sysbus-irq" 1
frequency = 0 (0x0)
mmio /0010
>>
>> How come?
>>
>> "device_add mac_via,help" shows properties of device "mac_via". It does
>> so even though "mac_via" is not available with device_add. Useful
>> because "info qtree" shows properties for such devices.
>>
>> These properties are defined dynamically, either for the instance
>> (traditional) or the class. The former typically happens in QOM
>> TypeInfo method .instance_init(), the latter in .class_init().
>>
>> "Typically", because properties can be added elsewhere, too. In
>> particular, QOM properties not meant for device_add are often created in
>> DeviceClass method .realize(). More on that below.
>>
>> To make properties created in .instance_init() visible in help, we need
>> to create and destroy an instance. This must be free of observable side
>> effects.
>>
>> This has been such a fertile source of crashes that I added
>> device-introspect-test:
>>
>> commit 2d1abb850fd15fd6eb75a92290be5f93b2772ec5
>> Author: Markus Armbruster
>> Date: Thu Oct 1 10:59:56 2015 +0200
>>
>> device-introspect-test: New, covering device introspection
>>
>> The test doesn't check that the output makes any sense, only that QEMU
>> survives. Useful since we've had an astounding number of crash bugs
>> around there.
>>
>> In fact, we have a bunch of them right now: a few devices crash or
>> hang, and some leave dangling pointers behind. The test skips testing
>> the broken parts. The next commits will fix them up, and drop the
>> skipping.
>>
>> Signed-off-by: Markus Armbruster
>> Reviewed-by: Eric Blake
>> Message-Id: <144368-12182-8-git-send-email-arm...@redhat.com>
>>
>> Now let's examine device "mac_via". It defines properties both in its
>> .class_init() and its .instance_init().
>>
>> static void mac_via_class_init(ObjectClass *oc, void *data)
>> {
>> DeviceClass *dc = DEVICE_CLASS(oc);
>>
>> dc->realize = mac_via_realize;
>> dc->reset = mac_via_reset;
>> dc->vmsd = _mac_via;
>> --->device_class_set_props(dc, mac_via_properties);
>> }
>>
>> where
>>
>> static Property mac_via_properties[] = {
>> DEFINE_PROP_DRIVE("drive", MacVIAState, blk),
>> DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> And
>>
>> static void mac_via_init(Object *obj)
>> {
>> SysBusDevice
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
Paolo Bonzini writes: > On 14/03/20 14:19, Mark Cave-Ayland wrote: >>> Observe that mac_via_init() has obvious side effects. In particular, it >>> creates two devices that are then visible in "info qtree", and that's >>> caught by device-introspect-test. >>> >>> I believe these things need to be done in .realize(). > > That is not a problem; the devices should be removed when the device is > finalized. In theory the steps would be: > > - the child properties are removed > > - this causes unparent to be called on the child devices > > - this causes the child devices to be unrealized > > - this causes the child devices to remove themselves from their bus (and > from "info qtree") > > - this causes the refcount to drop to zero and the devices to be > finalized themselves. > > The question is why they are not, i.e. where does the above reasoning break. I don't know. But let's for the sake of the argument assume this actually worked. Asking for help in the monitor then *still* has side effects visible in the time span between .instance_init() and finalization. Why is that harmless? > So, sysbus_init_child_obj is fine.
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 14/03/20 14:19, Mark Cave-Ayland wrote: >> Observe that mac_via_init() has obvious side effects. In particular, it >> creates two devices that are then visible in "info qtree", and that's >> caught by device-introspect-test. >> >> I believe these things need to be done in .realize(). That is not a problem; the devices should be removed when the device is finalized. In theory the steps would be: - the child properties are removed - this causes unparent to be called on the child devices - this causes the child devices to be unrealized - this causes the child devices to remove themselves from their bus (and from "info qtree") - this causes the refcount to drop to zero and the devices to be finalized themselves. The question is why they are not, i.e. where does the above reasoning break. So, sysbus_init_child_obj is fine. Paolo
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 10/03/2020 09:07, Markus Armbruster wrote:
> Widespread QOM usage anti-pattern ahead; cc: QOM maintainers.
>
> Peter Maydell writes:
>
>> On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan wrote:
>>> On 3/9/2020 5:21 PM, Peter Maydell wrote:
Could you explain more? My thought is that we should be using
sysbus_init_child_obj() and we should be doing it in the init method.
Why does that break the tests ? It's the same thing various other
devices do.
>>>
>>> device-introspect-test do the follow check for each device type:
>>>
>>> qtree_start = qtest_hmp(qts, "info qtree");
>>> ...
>>> qtest_qmp(qts, "{'execute': 'device-list-properties','arguments':
>>> {'typename': %s}}", type);
>>> ...
>>> qtree_end = qtest_hmp(qts, "info qtree");
>>> g_assert_cmpstr(qtree_start, ==, qtree_end);
>>>
>>> If we do qdev_set_parent_bus in init, it will check fail when type =
>>> 'mac_via'.
>>> mac_via_init() is called by q800_init(). But it will not be called in
>>> qtest(-machine none) in the step qtree_start.
>>> And after we call 'device-list-properties', mac_via_init() was called and
>>> set dev parent bus. We can find these
>>> devices in the qtree_end. So it break the test on the assert.
>>
>> Markus, do you know what's happening here? Why is
>> trying to use sysbus_init_child_obj() breaking the
>> device-introspect-test for this particular device,
>> but fine for the other places where we use it?
>> (Maybe we're accidentally leaking a reference to
>> something so the sub-device stays on the sysbus
>> when it should have removed itself when the
>> device was deinited ?)
>
> Two questions: (1) why does it break here, and (2) why doesn't it break
> elsewhere.
>
> First question: why does it break here?
>
> It breaks here because asking for help with "device_add mac_via,help"
> has a side effect visible in "info qtree".
>
>>> Here is the output:
>>>
>>> # Testing device 'mac_via'
> [Uninteresting stuff snipped...]
>
> "info qtree" before asking for help:
>
>>> qtree_start: bus: main-system-bus
>>> type System
>
> "info qtree" after asking for help:
>
>>> qtree_end: bus: main-system-bus
>>> type System
>>> dev: mos6522-q800-via2, id ""
>>> gpio-in "via2-irq" 8
>>> gpio-out "sysbus-irq" 1
>>> frequency = 0 (0x0)
>>> mmio /0010
>>> dev: mos6522-q800-via1, id ""
>>> gpio-in "via1-irq" 8
>>> gpio-out "sysbus-irq" 1
>>> frequency = 0 (0x0)
>>> mmio /0010
>
> How come?
>
> "device_add mac_via,help" shows properties of device "mac_via". It does
> so even though "mac_via" is not available with device_add. Useful
> because "info qtree" shows properties for such devices.
>
> These properties are defined dynamically, either for the instance
> (traditional) or the class. The former typically happens in QOM
> TypeInfo method .instance_init(), the latter in .class_init().
>
> "Typically", because properties can be added elsewhere, too. In
> particular, QOM properties not meant for device_add are often created in
> DeviceClass method .realize(). More on that below.
>
> To make properties created in .instance_init() visible in help, we need
> to create and destroy an instance. This must be free of observable side
> effects.
>
> This has been such a fertile source of crashes that I added
> device-introspect-test:
>
> commit 2d1abb850fd15fd6eb75a92290be5f93b2772ec5
> Author: Markus Armbruster
> Date: Thu Oct 1 10:59:56 2015 +0200
>
> device-introspect-test: New, covering device introspection
>
> The test doesn't check that the output makes any sense, only that QEMU
> survives. Useful since we've had an astounding number of crash bugs
> around there.
>
> In fact, we have a bunch of them right now: a few devices crash or
> hang, and some leave dangling pointers behind. The test skips testing
> the broken parts. The next commits will fix them up, and drop the
> skipping.
>
> Signed-off-by: Markus Armbruster
> Reviewed-by: Eric Blake
> Message-Id: <144368-12182-8-git-send-email-arm...@redhat.com>
>
> Now let's examine device "mac_via". It defines properties both in its
> .class_init() and its .instance_init().
>
> static void mac_via_class_init(ObjectClass *oc, void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(oc);
>
> dc->realize = mac_via_realize;
> dc->reset = mac_via_reset;
> dc->vmsd = _mac_via;
> --->device_class_set_props(dc, mac_via_properties);
> }
>
> where
>
> static Property mac_via_properties[] = {
> DEFINE_PROP_DRIVE("drive", MacVIAState, blk),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> And
>
> static void mac_via_init(Object *obj)
> {
> SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> MacVIAState *m = MAC_VIA(obj);
> MOS6522State *ms;
>
> /* MMIO */
> memory_region_init(>mmio, obj,
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On Tue, 10 Mar 2020, Markus Armbruster wrote: Root cause of this issue: nobody knows how to use QOM correctly (first order approximation). In particular, people are perenially confused on how to split work between .instance_init() and .realize(). We really, really, really need to provide some guidance there! Right now, all we provide are hundreds of examples, many of them bad. Agreed, it's hard to find consise and useful docs on this. The best I've found was this: https://habkost.net/posts/2016/11/incomplete-list-of-qemu-apis.html but not sure how relevant is it still. Maybe should be more prominently linked or put on the wiki. Also renaming init to alloc might help to clear some confusion. (Alloc and realize are still a bit confusing but less so than also renaming realize to init.) Regards, BALATON Zoltan
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On Tue, 10 Mar 2020 at 09:08, Markus Armbruster wrote:
> We have >200 calls of sysbus_init_child_obj() in some 40 files. I'm
> arbitrarily picking hw/arm/allwinner-a10.c for a closer look.
>
> It calls it from device allwinner-a10's .instance_init() method
> aw_a10_init(). Side effect, clearly wrong.
Huh. This implies that sysbus_init_child_obj() is a fundamentally
broken API. It bundles up two things: (a) init the child object and
(b) say it is on the sysbus. But (a) should be done in 'init' and (b)
should be done in 'realize'. So that's another line of boilerplate
code needed, plus because "put the thing on the sysbus" has
to be its own call it's easier to forget. That's a shame, as
"this object has a child object" is already pretty boilerplate-heavy,
and now it looks like:
* in init, call object_initialize_chid()
* in realize, call qdev_set_parent_bus()
* in realize, realize child (which is 5 lines of code because
of the "if (err != NULL) { error_propagate(errp, err); return; }")
It's a shame we didn't realize sysbus_init_child_obj() was
fundamentally broken before we did a lot of conversion
of code to use it...
thanks
-- PMM
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
Widespread QOM usage anti-pattern ahead; cc: QOM maintainers.
Peter Maydell writes:
> On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan wrote:
>> On 3/9/2020 5:21 PM, Peter Maydell wrote:
>> > Could you explain more? My thought is that we should be using
>> > sysbus_init_child_obj() and we should be doing it in the init method.
>> > Why does that break the tests ? It's the same thing various other
>> > devices do.
>>
>> device-introspect-test do the follow check for each device type:
>>
>> qtree_start = qtest_hmp(qts, "info qtree");
>> ...
>> qtest_qmp(qts, "{'execute': 'device-list-properties','arguments':
>> {'typename': %s}}", type);
>> ...
>> qtree_end = qtest_hmp(qts, "info qtree");
>> g_assert_cmpstr(qtree_start, ==, qtree_end);
>>
>> If we do qdev_set_parent_bus in init, it will check fail when type =
>> 'mac_via'.
>> mac_via_init() is called by q800_init(). But it will not be called in
>> qtest(-machine none) in the step qtree_start.
>> And after we call 'device-list-properties', mac_via_init() was called and
>> set dev parent bus. We can find these
>> devices in the qtree_end. So it break the test on the assert.
>
> Markus, do you know what's happening here? Why is
> trying to use sysbus_init_child_obj() breaking the
> device-introspect-test for this particular device,
> but fine for the other places where we use it?
> (Maybe we're accidentally leaking a reference to
> something so the sub-device stays on the sysbus
> when it should have removed itself when the
> device was deinited ?)
Two questions: (1) why does it break here, and (2) why doesn't it break
elsewhere.
First question: why does it break here?
It breaks here because asking for help with "device_add mac_via,help"
has a side effect visible in "info qtree".
>> Here is the output:
>>
>> # Testing device 'mac_via'
[Uninteresting stuff snipped...]
"info qtree" before asking for help:
>> qtree_start: bus: main-system-bus
>> type System
"info qtree" after asking for help:
>> qtree_end: bus: main-system-bus
>> type System
>> dev: mos6522-q800-via2, id ""
>> gpio-in "via2-irq" 8
>> gpio-out "sysbus-irq" 1
>> frequency = 0 (0x0)
>> mmio /0010
>> dev: mos6522-q800-via1, id ""
>> gpio-in "via1-irq" 8
>> gpio-out "sysbus-irq" 1
>> frequency = 0 (0x0)
>> mmio /0010
How come?
"device_add mac_via,help" shows properties of device "mac_via". It does
so even though "mac_via" is not available with device_add. Useful
because "info qtree" shows properties for such devices.
These properties are defined dynamically, either for the instance
(traditional) or the class. The former typically happens in QOM
TypeInfo method .instance_init(), the latter in .class_init().
"Typically", because properties can be added elsewhere, too. In
particular, QOM properties not meant for device_add are often created in
DeviceClass method .realize(). More on that below.
To make properties created in .instance_init() visible in help, we need
to create and destroy an instance. This must be free of observable side
effects.
This has been such a fertile source of crashes that I added
device-introspect-test:
commit 2d1abb850fd15fd6eb75a92290be5f93b2772ec5
Author: Markus Armbruster
Date: Thu Oct 1 10:59:56 2015 +0200
device-introspect-test: New, covering device introspection
The test doesn't check that the output makes any sense, only that QEMU
survives. Useful since we've had an astounding number of crash bugs
around there.
In fact, we have a bunch of them right now: a few devices crash or
hang, and some leave dangling pointers behind. The test skips testing
the broken parts. The next commits will fix them up, and drop the
skipping.
Signed-off-by: Markus Armbruster
Reviewed-by: Eric Blake
Message-Id: <144368-12182-8-git-send-email-arm...@redhat.com>
Now let's examine device "mac_via". It defines properties both in its
.class_init() and its .instance_init().
static void mac_via_class_init(ObjectClass *oc, void *data)
{
DeviceClass *dc = DEVICE_CLASS(oc);
dc->realize = mac_via_realize;
dc->reset = mac_via_reset;
dc->vmsd = _mac_via;
--->device_class_set_props(dc, mac_via_properties);
}
where
static Property mac_via_properties[] = {
DEFINE_PROP_DRIVE("drive", MacVIAState, blk),
DEFINE_PROP_END_OF_LIST(),
};
And
static void mac_via_init(Object *obj)
{
SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
MacVIAState *m = MAC_VIA(obj);
MOS6522State *ms;
/* MMIO */
memory_region_init(>mmio, obj, "mac-via", 2 * VIA_SIZE);
sysbus_init_mmio(sbd, >mmio);
memory_region_init_io(>via1mem, obj, _q800_via1_ops,
>mos6522_via1, "via1", VIA_SIZE);
memory_region_add_subregion(>mmio, 0x0, >via1mem);
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 3/10/2020 12:16 AM, Mark Cave-Ayland wrote:
> On 09/03/2020 14:14, Markus Armbruster wrote:
>
>> Pan Nengyuan writes:
>>
>>> On 3/9/2020 8:34 PM, Markus Armbruster wrote:
Peter Maydell writes:
> On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan wrote:
>> On 3/9/2020 5:21 PM, Peter Maydell wrote:
>>> Could you explain more? My thought is that we should be using
>>> sysbus_init_child_obj() and we should be doing it in the init method.
>>> Why does that break the tests ? It's the same thing various other
>>> devices do.
>>
>> device-introspect-test do the follow check for each device type:
>>
>> qtree_start = qtest_hmp(qts, "info qtree");
>> ...
>> qtest_qmp(qts, "{'execute': 'device-list-properties','arguments':
>> {'typename': %s}}", type);
>> ...
>> qtree_end = qtest_hmp(qts, "info qtree");
>> g_assert_cmpstr(qtree_start, ==, qtree_end);
>>
>> If we do qdev_set_parent_bus in init, it will check fail when type =
>> 'mac_via'.
>> mac_via_init() is called by q800_init(). But it will not be called in
>> qtest(-machine none) in the step qtree_start.
>> And after we call 'device-list-properties', mac_via_init() was called
>> and set dev parent bus. We can find these
>> devices in the qtree_end. So it break the test on the assert.
>
> Markus, do you know what's happening here? Why is
> trying to use sysbus_init_child_obj() breaking the
> device-introspect-test for this particular device,
> but fine for the other places where we use it?
> (Maybe we're accidentally leaking a reference to
> something so the sub-device stays on the sysbus
> when it should have removed itself when the
> device was deinited ?)
Pan Nengyuan, please provide the exact patch that fails for you.
>>>
>>> As the follow patch:
>>>
>>> >From 9b4f35e294597410cc03b967c127242ce099692e Mon Sep 17 00:00:00 2001
>>> From: Pan Nengyuan
>>> Date: Wed, 4 Mar 2020 11:29:28 +0800
>>> Subject: [PATCH] mac_via: fix incorrect creation of mos6522 device in
>>> mac_via
>>>
>>> This patch fix a bug in mac_via where it failed to actually realize devices
>>> it was using.
>>> And move the init codes which inits the mos6522 objects and properties on
>>> them from realize()
>>> into init(). However, we keep qdev_set_parent_bus in realize(), otherwise
>>> it will cause
>>> device-introspect-test test fail. Then do the realize mos6522 device in the
>>> mac_vir_realize.
>>>
>>> Signed-off-by: Pan Nengyuan
>>> ---
>>> hw/misc/mac_via.c | 40 ++--
>>> 1 file changed, 26 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>>> index b7d0012794..4c5c432140 100644
>>> --- a/hw/misc/mac_via.c
>>> +++ b/hw/misc/mac_via.c
>>> @@ -868,24 +868,21 @@ static void mac_via_reset(DeviceState *dev)
>>> static void mac_via_realize(DeviceState *dev, Error **errp)
>>> {
>>> MacVIAState *m = MAC_VIA(dev);
>>> -MOS6522State *ms;
>>> struct tm tm;
>>> int ret;
>>> +Error *err = NULL;
>>>
>>> -/* Init VIAs 1 and 2 */
>>> -sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
>>> - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
>>> -
>>> -sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
>>> - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
>>> +object_property_set_bool(OBJECT(>mos6522_via1), true, "realized",
>>> );
>>> +if (err != NULL) {
>>> +error_propagate(errp, err);
>>> +return;
>>> +}
>>>
>>> -/* Pass through mos6522 output IRQs */
>>> -ms = MOS6522(>mos6522_via1);
>>> -object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
>>> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
>>> -ms = MOS6522(>mos6522_via2);
>>> -object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
>>> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
>>> +object_property_set_bool(OBJECT(>mos6522_via2), true, "realized",
>>> );
>>> +if (err != NULL) {
>>> +error_propagate(errp, err);
>>> +return;
>>> +}
>>>
>>> /* Pass through mos6522 input IRQs */
>>> qdev_pass_gpios(DEVICE(>mos6522_via1), dev, "via1-irq");
>>> @@ -932,6 +929,7 @@ static void mac_via_init(Object *obj)
>>> {
>>> SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>>> MacVIAState *m = MAC_VIA(obj);
>>> +MOS6522State *ms;
>>>
>>> /* MMIO */
>>> memory_region_init(>mmio, obj, "mac-via", 2 * VIA_SIZE);
>>> @@ -948,6 +946,20 @@ static void mac_via_init(Object *obj)
>>> /* ADB */
>>> qbus_create_inplace((BusState *)>adb_bus, sizeof(m->adb_bus),
>>> TYPE_ADB_BUS, DEVICE(obj), "adb.0");
>>> +
>>> +/* Init VIAs 1 and 2 */
>>> +sysbus_init_child_obj(OBJECT(m), "via1", >mos6522_via1,
>>> +
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 09/03/2020 14:14, Markus Armbruster wrote:
> Pan Nengyuan writes:
>
>> On 3/9/2020 8:34 PM, Markus Armbruster wrote:
>>> Peter Maydell writes:
>>>
On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan wrote:
> On 3/9/2020 5:21 PM, Peter Maydell wrote:
>> Could you explain more? My thought is that we should be using
>> sysbus_init_child_obj() and we should be doing it in the init method.
>> Why does that break the tests ? It's the same thing various other
>> devices do.
>
> device-introspect-test do the follow check for each device type:
>
> qtree_start = qtest_hmp(qts, "info qtree");
> ...
> qtest_qmp(qts, "{'execute': 'device-list-properties','arguments':
> {'typename': %s}}", type);
> ...
> qtree_end = qtest_hmp(qts, "info qtree");
> g_assert_cmpstr(qtree_start, ==, qtree_end);
>
> If we do qdev_set_parent_bus in init, it will check fail when type =
> 'mac_via'.
> mac_via_init() is called by q800_init(). But it will not be called in
> qtest(-machine none) in the step qtree_start.
> And after we call 'device-list-properties', mac_via_init() was called and
> set dev parent bus. We can find these
> devices in the qtree_end. So it break the test on the assert.
Markus, do you know what's happening here? Why is
trying to use sysbus_init_child_obj() breaking the
device-introspect-test for this particular device,
but fine for the other places where we use it?
(Maybe we're accidentally leaking a reference to
something so the sub-device stays on the sysbus
when it should have removed itself when the
device was deinited ?)
>>>
>>> Pan Nengyuan, please provide the exact patch that fails for you.
>>
>> As the follow patch:
>>
>> >From 9b4f35e294597410cc03b967c127242ce099692e Mon Sep 17 00:00:00 2001
>> From: Pan Nengyuan
>> Date: Wed, 4 Mar 2020 11:29:28 +0800
>> Subject: [PATCH] mac_via: fix incorrect creation of mos6522 device in mac_via
>>
>> This patch fix a bug in mac_via where it failed to actually realize devices
>> it was using.
>> And move the init codes which inits the mos6522 objects and properties on
>> them from realize()
>> into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it
>> will cause
>> device-introspect-test test fail. Then do the realize mos6522 device in the
>> mac_vir_realize.
>>
>> Signed-off-by: Pan Nengyuan
>> ---
>> hw/misc/mac_via.c | 40 ++--
>> 1 file changed, 26 insertions(+), 14 deletions(-)
>>
>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>> index b7d0012794..4c5c432140 100644
>> --- a/hw/misc/mac_via.c
>> +++ b/hw/misc/mac_via.c
>> @@ -868,24 +868,21 @@ static void mac_via_reset(DeviceState *dev)
>> static void mac_via_realize(DeviceState *dev, Error **errp)
>> {
>> MacVIAState *m = MAC_VIA(dev);
>> -MOS6522State *ms;
>> struct tm tm;
>> int ret;
>> +Error *err = NULL;
>>
>> -/* Init VIAs 1 and 2 */
>> -sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
>> - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
>> -
>> -sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
>> - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
>> +object_property_set_bool(OBJECT(>mos6522_via1), true, "realized",
>> );
>> +if (err != NULL) {
>> +error_propagate(errp, err);
>> +return;
>> +}
>>
>> -/* Pass through mos6522 output IRQs */
>> -ms = MOS6522(>mos6522_via1);
>> -object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
>> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
>> -ms = MOS6522(>mos6522_via2);
>> -object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
>> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
>> +object_property_set_bool(OBJECT(>mos6522_via2), true, "realized",
>> );
>> +if (err != NULL) {
>> +error_propagate(errp, err);
>> +return;
>> +}
>>
>> /* Pass through mos6522 input IRQs */
>> qdev_pass_gpios(DEVICE(>mos6522_via1), dev, "via1-irq");
>> @@ -932,6 +929,7 @@ static void mac_via_init(Object *obj)
>> {
>> SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> MacVIAState *m = MAC_VIA(obj);
>> +MOS6522State *ms;
>>
>> /* MMIO */
>> memory_region_init(>mmio, obj, "mac-via", 2 * VIA_SIZE);
>> @@ -948,6 +946,20 @@ static void mac_via_init(Object *obj)
>> /* ADB */
>> qbus_create_inplace((BusState *)>adb_bus, sizeof(m->adb_bus),
>> TYPE_ADB_BUS, DEVICE(obj), "adb.0");
>> +
>> +/* Init VIAs 1 and 2 */
>> +sysbus_init_child_obj(OBJECT(m), "via1", >mos6522_via1,
>> + sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
>> +sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
>> +
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
Pan Nengyuan writes:
> On 3/9/2020 8:34 PM, Markus Armbruster wrote:
>> Peter Maydell writes:
>>
>>> On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan wrote:
On 3/9/2020 5:21 PM, Peter Maydell wrote:
> Could you explain more? My thought is that we should be using
> sysbus_init_child_obj() and we should be doing it in the init method.
> Why does that break the tests ? It's the same thing various other
> devices do.
device-introspect-test do the follow check for each device type:
qtree_start = qtest_hmp(qts, "info qtree");
...
qtest_qmp(qts, "{'execute': 'device-list-properties','arguments':
{'typename': %s}}", type);
...
qtree_end = qtest_hmp(qts, "info qtree");
g_assert_cmpstr(qtree_start, ==, qtree_end);
If we do qdev_set_parent_bus in init, it will check fail when type =
'mac_via'.
mac_via_init() is called by q800_init(). But it will not be called in
qtest(-machine none) in the step qtree_start.
And after we call 'device-list-properties', mac_via_init() was called and
set dev parent bus. We can find these
devices in the qtree_end. So it break the test on the assert.
>>>
>>> Markus, do you know what's happening here? Why is
>>> trying to use sysbus_init_child_obj() breaking the
>>> device-introspect-test for this particular device,
>>> but fine for the other places where we use it?
>>> (Maybe we're accidentally leaking a reference to
>>> something so the sub-device stays on the sysbus
>>> when it should have removed itself when the
>>> device was deinited ?)
>>
>> Pan Nengyuan, please provide the exact patch that fails for you.
>
> As the follow patch:
>
>>From 9b4f35e294597410cc03b967c127242ce099692e Mon Sep 17 00:00:00 2001
> From: Pan Nengyuan
> Date: Wed, 4 Mar 2020 11:29:28 +0800
> Subject: [PATCH] mac_via: fix incorrect creation of mos6522 device in mac_via
>
> This patch fix a bug in mac_via where it failed to actually realize devices
> it was using.
> And move the init codes which inits the mos6522 objects and properties on
> them from realize()
> into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it
> will cause
> device-introspect-test test fail. Then do the realize mos6522 device in the
> mac_vir_realize.
>
> Signed-off-by: Pan Nengyuan
> ---
> hw/misc/mac_via.c | 40 ++--
> 1 file changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index b7d0012794..4c5c432140 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -868,24 +868,21 @@ static void mac_via_reset(DeviceState *dev)
> static void mac_via_realize(DeviceState *dev, Error **errp)
> {
> MacVIAState *m = MAC_VIA(dev);
> -MOS6522State *ms;
> struct tm tm;
> int ret;
> +Error *err = NULL;
>
> -/* Init VIAs 1 and 2 */
> -sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
> - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
> -
> -sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
> - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
> +object_property_set_bool(OBJECT(>mos6522_via1), true, "realized",
> );
> +if (err != NULL) {
> +error_propagate(errp, err);
> +return;
> +}
>
> -/* Pass through mos6522 output IRQs */
> -ms = MOS6522(>mos6522_via1);
> -object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
> -ms = MOS6522(>mos6522_via2);
> -object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
> +object_property_set_bool(OBJECT(>mos6522_via2), true, "realized",
> );
> +if (err != NULL) {
> +error_propagate(errp, err);
> +return;
> +}
>
> /* Pass through mos6522 input IRQs */
> qdev_pass_gpios(DEVICE(>mos6522_via1), dev, "via1-irq");
> @@ -932,6 +929,7 @@ static void mac_via_init(Object *obj)
> {
> SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> MacVIAState *m = MAC_VIA(obj);
> +MOS6522State *ms;
>
> /* MMIO */
> memory_region_init(>mmio, obj, "mac-via", 2 * VIA_SIZE);
> @@ -948,6 +946,20 @@ static void mac_via_init(Object *obj)
> /* ADB */
> qbus_create_inplace((BusState *)>adb_bus, sizeof(m->adb_bus),
> TYPE_ADB_BUS, DEVICE(obj), "adb.0");
> +
> +/* Init VIAs 1 and 2 */
> +sysbus_init_child_obj(OBJECT(m), "via1", >mos6522_via1,
> + sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
> +sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
> + sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
> +
> +/* Pass through mos6522 output IRQs */
> +ms = MOS6522(>mos6522_via1);
> +object_property_add_alias(OBJECT(m), "irq[0]",
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 3/9/2020 8:34 PM, Markus Armbruster wrote:
> Peter Maydell writes:
>
>> On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan wrote:
>>> On 3/9/2020 5:21 PM, Peter Maydell wrote:
Could you explain more? My thought is that we should be using
sysbus_init_child_obj() and we should be doing it in the init method.
Why does that break the tests ? It's the same thing various other
devices do.
>>>
>>> device-introspect-test do the follow check for each device type:
>>>
>>> qtree_start = qtest_hmp(qts, "info qtree");
>>> ...
>>> qtest_qmp(qts, "{'execute': 'device-list-properties','arguments':
>>> {'typename': %s}}", type);
>>> ...
>>> qtree_end = qtest_hmp(qts, "info qtree");
>>> g_assert_cmpstr(qtree_start, ==, qtree_end);
>>>
>>> If we do qdev_set_parent_bus in init, it will check fail when type =
>>> 'mac_via'.
>>> mac_via_init() is called by q800_init(). But it will not be called in
>>> qtest(-machine none) in the step qtree_start.
>>> And after we call 'device-list-properties', mac_via_init() was called and
>>> set dev parent bus. We can find these
>>> devices in the qtree_end. So it break the test on the assert.
>>
>> Markus, do you know what's happening here? Why is
>> trying to use sysbus_init_child_obj() breaking the
>> device-introspect-test for this particular device,
>> but fine for the other places where we use it?
>> (Maybe we're accidentally leaking a reference to
>> something so the sub-device stays on the sysbus
>> when it should have removed itself when the
>> device was deinited ?)
>
> Pan Nengyuan, please provide the exact patch that fails for you.
As the follow patch:
>From 9b4f35e294597410cc03b967c127242ce099692e Mon Sep 17 00:00:00 2001
From: Pan Nengyuan
Date: Wed, 4 Mar 2020 11:29:28 +0800
Subject: [PATCH] mac_via: fix incorrect creation of mos6522 device in mac_via
This patch fix a bug in mac_via where it failed to actually realize devices it
was using.
And move the init codes which inits the mos6522 objects and properties on them
from realize()
into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it
will cause
device-introspect-test test fail. Then do the realize mos6522 device in the
mac_vir_realize.
Signed-off-by: Pan Nengyuan
---
hw/misc/mac_via.c | 40 ++--
1 file changed, 26 insertions(+), 14 deletions(-)
diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index b7d0012794..4c5c432140 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -868,24 +868,21 @@ static void mac_via_reset(DeviceState *dev)
static void mac_via_realize(DeviceState *dev, Error **errp)
{
MacVIAState *m = MAC_VIA(dev);
-MOS6522State *ms;
struct tm tm;
int ret;
+Error *err = NULL;
-/* Init VIAs 1 and 2 */
-sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
- sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
-
-sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
- sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
+object_property_set_bool(OBJECT(>mos6522_via1), true, "realized", );
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
-/* Pass through mos6522 output IRQs */
-ms = MOS6522(>mos6522_via1);
-object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
- SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
-ms = MOS6522(>mos6522_via2);
-object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
- SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
+object_property_set_bool(OBJECT(>mos6522_via2), true, "realized", );
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
/* Pass through mos6522 input IRQs */
qdev_pass_gpios(DEVICE(>mos6522_via1), dev, "via1-irq");
@@ -932,6 +929,7 @@ static void mac_via_init(Object *obj)
{
SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
MacVIAState *m = MAC_VIA(obj);
+MOS6522State *ms;
/* MMIO */
memory_region_init(>mmio, obj, "mac-via", 2 * VIA_SIZE);
@@ -948,6 +946,20 @@ static void mac_via_init(Object *obj)
/* ADB */
qbus_create_inplace((BusState *)>adb_bus, sizeof(m->adb_bus),
TYPE_ADB_BUS, DEVICE(obj), "adb.0");
+
+/* Init VIAs 1 and 2 */
+sysbus_init_child_obj(OBJECT(m), "via1", >mos6522_via1,
+ sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
+sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
+ sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
+
+/* Pass through mos6522 output IRQs */
+ms = MOS6522(>mos6522_via1);
+object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms),
+ SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
+ms = MOS6522(>mos6522_via2);
+object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms),
+
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
Peter Maydell writes:
> On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan wrote:
>> On 3/9/2020 5:21 PM, Peter Maydell wrote:
>> > Could you explain more? My thought is that we should be using
>> > sysbus_init_child_obj() and we should be doing it in the init method.
>> > Why does that break the tests ? It's the same thing various other
>> > devices do.
>>
>> device-introspect-test do the follow check for each device type:
>>
>> qtree_start = qtest_hmp(qts, "info qtree");
>> ...
>> qtest_qmp(qts, "{'execute': 'device-list-properties','arguments':
>> {'typename': %s}}", type);
>> ...
>> qtree_end = qtest_hmp(qts, "info qtree");
>> g_assert_cmpstr(qtree_start, ==, qtree_end);
>>
>> If we do qdev_set_parent_bus in init, it will check fail when type =
>> 'mac_via'.
>> mac_via_init() is called by q800_init(). But it will not be called in
>> qtest(-machine none) in the step qtree_start.
>> And after we call 'device-list-properties', mac_via_init() was called and
>> set dev parent bus. We can find these
>> devices in the qtree_end. So it break the test on the assert.
>
> Markus, do you know what's happening here? Why is
> trying to use sysbus_init_child_obj() breaking the
> device-introspect-test for this particular device,
> but fine for the other places where we use it?
> (Maybe we're accidentally leaking a reference to
> something so the sub-device stays on the sysbus
> when it should have removed itself when the
> device was deinited ?)
Pan Nengyuan, please provide the exact patch that fails for you.
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan wrote:
> On 3/9/2020 5:21 PM, Peter Maydell wrote:
> > Could you explain more? My thought is that we should be using
> > sysbus_init_child_obj() and we should be doing it in the init method.
> > Why does that break the tests ? It's the same thing various other
> > devices do.
>
> device-introspect-test do the follow check for each device type:
>
> qtree_start = qtest_hmp(qts, "info qtree");
> ...
> qtest_qmp(qts, "{'execute': 'device-list-properties','arguments':
> {'typename': %s}}", type);
> ...
> qtree_end = qtest_hmp(qts, "info qtree");
> g_assert_cmpstr(qtree_start, ==, qtree_end);
>
> If we do qdev_set_parent_bus in init, it will check fail when type =
> 'mac_via'.
> mac_via_init() is called by q800_init(). But it will not be called in
> qtest(-machine none) in the step qtree_start.
> And after we call 'device-list-properties', mac_via_init() was called and set
> dev parent bus. We can find these
> devices in the qtree_end. So it break the test on the assert.
Markus, do you know what's happening here? Why is
trying to use sysbus_init_child_obj() breaking the
device-introspect-test for this particular device,
but fine for the other places where we use it?
(Maybe we're accidentally leaking a reference to
something so the sub-device stays on the sysbus
when it should have removed itself when the
device was deinited ?)
> Here is the output:
>
> # Testing device 'mac_via'
> adb.0=>
> drive=- Node name or ID of a block device to use as a
> backend
> irq[0]=>
> irq[1]=>
> mac-via[0]=>
> via1=>
> via1[0]=>
> via2=>
> via2[0]=>
> qtree_start: bus: main-system-bus
> type System
>
> qtree_end: bus: main-system-bus
> type System
> dev: mos6522-q800-via2, id ""
> gpio-in "via2-irq" 8
> gpio-out "sysbus-irq" 1
> frequency = 0 (0x0)
> mmio /0010
> dev: mos6522-q800-via1, id ""
> gpio-in "via1-irq" 8
> gpio-out "sysbus-irq" 1
> frequency = 0 (0x0)
> mmio /0010
thanks
-- PMM
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 3/9/2020 5:21 PM, Peter Maydell wrote:
> On Mon, 9 Mar 2020 at 00:56, Pan Nengyuan wrote:
>>
>>
>>
>> On 3/8/2020 9:29 PM, Peter Maydell wrote:
>>> On Thu, 5 Mar 2020 at 06:39, Pan Nengyuan wrote:
-/* Init VIAs 1 and 2 */
-sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
- sizeof(m->mos6522_via1),
TYPE_MOS6522_Q800_VIA1);
+qdev_set_parent_bus(DEVICE(>mos6522_via1), sysbus_get_default());
+qdev_set_parent_bus(DEVICE(>mos6522_via2), sysbus_get_default());
>>>
>>> Rather than manually setting the parent bus, you can use
>>> sysbus_init_child_obj() instead of object_initialize_child() --
>>> it is a convenience function that does both object_initialize_child()
>>> and qdev_set_parent_bus() for you.
>>
>> Actually I used sysbus_init_child_obj() first, but it will fail to run
>> device-introspect-test.
>> Because qdev_set_parent_bus() will change 'info qtree' after we call
>> 'device-list-properties'.
>> Thus, I do it in the realize.
>
> Could you explain more? My thought is that we should be using
> sysbus_init_child_obj() and we should be doing it in the init method.
> Why does that break the tests ? It's the same thing various other
> devices do.
device-introspect-test do the follow check for each device type:
qtree_start = qtest_hmp(qts, "info qtree");
...
qtest_qmp(qts, "{'execute': 'device-list-properties','arguments':
{'typename': %s}}", type);
...
qtree_end = qtest_hmp(qts, "info qtree");
g_assert_cmpstr(qtree_start, ==, qtree_end);
If we do qdev_set_parent_bus in init, it will check fail when type = 'mac_via'.
mac_via_init() is called by q800_init(). But it will not be called in
qtest(-machine none) in the step qtree_start.
And after we call 'device-list-properties', mac_via_init() was called and set
dev parent bus. We can find these
devices in the qtree_end. So it break the test on the assert.
Here is the output:
# Testing device 'mac_via'
adb.0=>
drive=- Node name or ID of a block device to use as a backend
irq[0]=>
irq[1]=>
mac-via[0]=>
via1=>
via1[0]=>
via2=>
via2[0]=>
qtree_start: bus: main-system-bus
type System
qtree_end: bus: main-system-bus
type System
dev: mos6522-q800-via2, id ""
gpio-in "via2-irq" 8
gpio-out "sysbus-irq" 1
frequency = 0 (0x0)
mmio /0010
dev: mos6522-q800-via1, id ""
gpio-in "via1-irq" 8
gpio-out "sysbus-irq" 1
frequency = 0 (0x0)
mmio /0010
>
> thanks
> -- PMM
> .
>
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On Mon, 9 Mar 2020 at 00:56, Pan Nengyuan wrote: > > > > On 3/8/2020 9:29 PM, Peter Maydell wrote: > > On Thu, 5 Mar 2020 at 06:39, Pan Nengyuan wrote: > >> -/* Init VIAs 1 and 2 */ > >> -sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1, > >> - sizeof(m->mos6522_via1), > >> TYPE_MOS6522_Q800_VIA1); > >> +qdev_set_parent_bus(DEVICE(>mos6522_via1), sysbus_get_default()); > >> +qdev_set_parent_bus(DEVICE(>mos6522_via2), sysbus_get_default()); > > > > Rather than manually setting the parent bus, you can use > > sysbus_init_child_obj() instead of object_initialize_child() -- > > it is a convenience function that does both object_initialize_child() > > and qdev_set_parent_bus() for you. > > Actually I used sysbus_init_child_obj() first, but it will fail to run > device-introspect-test. > Because qdev_set_parent_bus() will change 'info qtree' after we call > 'device-list-properties'. > Thus, I do it in the realize. Could you explain more? My thought is that we should be using sysbus_init_child_obj() and we should be doing it in the init method. Why does that break the tests ? It's the same thing various other devices do. thanks -- PMM
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 3/8/2020 9:29 PM, Peter Maydell wrote:
> On Thu, 5 Mar 2020 at 06:39, Pan Nengyuan wrote:
>>
>> This patch fix a bug in mac_via where it failed to actually realize devices
>> it was using.
>> And move the init codes which inits the mos6522 objects and properties on
>> them from realize()
>> into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it
>> will cause
>> device-introspect-test test fail. Then do the realize mos6522 device in the
>> mac_vir_realize.
>>
>> Signed-off-by: Pan Nengyuan
>> ---
>> Cc: Laurent Vivier
>> Cc: Mark Cave-Ayland
>> ---
>
>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>> index b7d0012794..4c5ad08805 100644
>> --- a/hw/misc/mac_via.c
>> +++ b/hw/misc/mac_via.c
>> @@ -868,24 +868,24 @@ static void mac_via_reset(DeviceState *dev)
>> static void mac_via_realize(DeviceState *dev, Error **errp)
>> {
>> MacVIAState *m = MAC_VIA(dev);
>> -MOS6522State *ms;
>> struct tm tm;
>> int ret;
>> +Error *err = NULL;
>>
>> -/* Init VIAs 1 and 2 */
>> -sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
>> - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
>> +qdev_set_parent_bus(DEVICE(>mos6522_via1), sysbus_get_default());
>> +qdev_set_parent_bus(DEVICE(>mos6522_via2), sysbus_get_default());
>
> Rather than manually setting the parent bus, you can use
> sysbus_init_child_obj() instead of object_initialize_child() --
> it is a convenience function that does both object_initialize_child()
> and qdev_set_parent_bus() for you.
Actually I used sysbus_init_child_obj() first, but it will fail to run
device-introspect-test.
Because qdev_set_parent_bus() will change 'info qtree' after we call
'device-list-properties'.
Thus, I do it in the realize.
>
>> -sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
>> - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
>> +object_property_set_bool(OBJECT(>mos6522_via1), true, "realized",
>> );
>> +if (err != NULL) {
>> +error_propagate(errp, err);
>> +return;
>> +}
>>
>> -/* Pass through mos6522 output IRQs */
>> -ms = MOS6522(>mos6522_via1);
>> -object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
>> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
>> -ms = MOS6522(>mos6522_via2);
>> -object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
>> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
>> +object_property_set_bool(OBJECT(>mos6522_via2), true, "realized",
>> );
>> +if (err != NULL) {
>> +error_propagate(errp, err);
>> +return;
>> +}
>>
>> /* Pass through mos6522 input IRQs */
>> qdev_pass_gpios(DEVICE(>mos6522_via1), dev, "via1-irq");
>> @@ -932,6 +932,7 @@ static void mac_via_init(Object *obj)
>> {
>> SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> MacVIAState *m = MAC_VIA(obj);
>> +MOS6522State *ms;
>>
>> /* MMIO */
>> memory_region_init(>mmio, obj, "mac-via", 2 * VIA_SIZE);
>> @@ -948,6 +949,22 @@ static void mac_via_init(Object *obj)
>> /* ADB */
>> qbus_create_inplace((BusState *)>adb_bus, sizeof(m->adb_bus),
>> TYPE_ADB_BUS, DEVICE(obj), "adb.0");
>> +
>> +/* Init VIAs 1 and 2 */
>> +object_initialize_child(OBJECT(m), "via1", >mos6522_via1,
>> +sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1,
>> +_abort, NULL);
>> +object_initialize_child(OBJECT(m), "via2", >mos6522_via2,
>> +sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2,
>> +_abort, NULL);
>> +
>> +/* Pass through mos6522 output IRQs */
>> +ms = MOS6522(>mos6522_via1);
>> +object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms),
>> + SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
>
> There's no point in using the MOS6522() cast to get a MOS6522*,
> because the only thing you do with it is immediately cast it
> to an Object* with the OBJECT() cast. Just use the OBJECT cast directly.
Ok, will do it.
Thanks.
>
>> +ms = MOS6522(>mos6522_via2);
>> +object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms),
>> + SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
>> }
>>
>> static void postload_update_cb(void *opaque, int running, RunState state)
>
> thanks
> -- PMM
> .
>
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On Thu, 5 Mar 2020 at 06:39, Pan Nengyuan wrote:
>
> This patch fix a bug in mac_via where it failed to actually realize devices
> it was using.
> And move the init codes which inits the mos6522 objects and properties on
> them from realize()
> into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it
> will cause
> device-introspect-test test fail. Then do the realize mos6522 device in the
> mac_vir_realize.
>
> Signed-off-by: Pan Nengyuan
> ---
> Cc: Laurent Vivier
> Cc: Mark Cave-Ayland
> ---
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index b7d0012794..4c5ad08805 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -868,24 +868,24 @@ static void mac_via_reset(DeviceState *dev)
> static void mac_via_realize(DeviceState *dev, Error **errp)
> {
> MacVIAState *m = MAC_VIA(dev);
> -MOS6522State *ms;
> struct tm tm;
> int ret;
> +Error *err = NULL;
>
> -/* Init VIAs 1 and 2 */
> -sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
> - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
> +qdev_set_parent_bus(DEVICE(>mos6522_via1), sysbus_get_default());
> +qdev_set_parent_bus(DEVICE(>mos6522_via2), sysbus_get_default());
Rather than manually setting the parent bus, you can use
sysbus_init_child_obj() instead of object_initialize_child() --
it is a convenience function that does both object_initialize_child()
and qdev_set_parent_bus() for you.
> -sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
> - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
> +object_property_set_bool(OBJECT(>mos6522_via1), true, "realized",
> );
> +if (err != NULL) {
> +error_propagate(errp, err);
> +return;
> +}
>
> -/* Pass through mos6522 output IRQs */
> -ms = MOS6522(>mos6522_via1);
> -object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
> -ms = MOS6522(>mos6522_via2);
> -object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
> +object_property_set_bool(OBJECT(>mos6522_via2), true, "realized",
> );
> +if (err != NULL) {
> +error_propagate(errp, err);
> +return;
> +}
>
> /* Pass through mos6522 input IRQs */
> qdev_pass_gpios(DEVICE(>mos6522_via1), dev, "via1-irq");
> @@ -932,6 +932,7 @@ static void mac_via_init(Object *obj)
> {
> SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> MacVIAState *m = MAC_VIA(obj);
> +MOS6522State *ms;
>
> /* MMIO */
> memory_region_init(>mmio, obj, "mac-via", 2 * VIA_SIZE);
> @@ -948,6 +949,22 @@ static void mac_via_init(Object *obj)
> /* ADB */
> qbus_create_inplace((BusState *)>adb_bus, sizeof(m->adb_bus),
> TYPE_ADB_BUS, DEVICE(obj), "adb.0");
> +
> +/* Init VIAs 1 and 2 */
> +object_initialize_child(OBJECT(m), "via1", >mos6522_via1,
> +sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1,
> +_abort, NULL);
> +object_initialize_child(OBJECT(m), "via2", >mos6522_via2,
> +sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2,
> +_abort, NULL);
> +
> +/* Pass through mos6522 output IRQs */
> +ms = MOS6522(>mos6522_via1);
> +object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms),
> + SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
There's no point in using the MOS6522() cast to get a MOS6522*,
because the only thing you do with it is immediately cast it
to an Object* with the OBJECT() cast. Just use the OBJECT cast directly.
> +ms = MOS6522(>mos6522_via2);
> +object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms),
> + SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
> }
>
> static void postload_update_cb(void *opaque, int running, RunState state)
thanks
-- PMM
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 3/5/2020 2:54 PM, Pan Nengyuan wrote:
> This patch fix a bug in mac_via where it failed to actually realize devices
> it was using.
> And move the init codes which inits the mos6522 objects and properties on
> them from realize()
> into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it
> will cause
> device-introspect-test test fail. Then do the realize mos6522 device in the
> mac_vir_realize.
>
> Signed-off-by: Pan Nengyuan
> ---
> Cc: Laurent Vivier
> Cc: Mark Cave-Ayland
> ---
> v4->v3:
> - split v3 into two patches, this patch fix incorrect creation of mos6522,
> move inits and props
> from realize into init. The v3 is:
> https://patchwork.kernel.org/patch/11407635/
> ---
> hw/misc/mac_via.c | 43 ++-
> 1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index b7d0012794..4c5ad08805 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -868,24 +868,24 @@ static void mac_via_reset(DeviceState *dev)
> static void mac_via_realize(DeviceState *dev, Error **errp)
> {
> MacVIAState *m = MAC_VIA(dev);
> -MOS6522State *ms;
> struct tm tm;
> int ret;
> +Error *err = NULL;
>
> -/* Init VIAs 1 and 2 */
> -sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
> - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
> +qdev_set_parent_bus(DEVICE(>mos6522_via1), sysbus_get_default());
> +qdev_set_parent_bus(DEVICE(>mos6522_via2), sysbus_get_default());
>
> -sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
> - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
> +object_property_set_bool(OBJECT(>mos6522_via1), true, "realized",
> );
> +if (err != NULL) {
> +error_propagate(errp, err);
> +return;
> +}
>
> -/* Pass through mos6522 output IRQs */
> -ms = MOS6522(>mos6522_via1);
> -object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
> -ms = MOS6522(>mos6522_via2);
> -object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
> +object_property_set_bool(OBJECT(>mos6522_via2), true, "realized",
> );
> +if (err != NULL) {
> +error_propagate(errp, err);
> +return;
> +}
>
> /* Pass through mos6522 input IRQs */
> qdev_pass_gpios(DEVICE(>mos6522_via1), dev, "via1-irq");
> @@ -932,6 +932,7 @@ static void mac_via_init(Object *obj)
> {
> SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> MacVIAState *m = MAC_VIA(obj);
> +MOS6522State *ms;
>
> /* MMIO */
> memory_region_init(>mmio, obj, "mac-via", 2 * VIA_SIZE);
> @@ -948,6 +949,22 @@ static void mac_via_init(Object *obj)
> /* ADB */
> qbus_create_inplace((BusState *)>adb_bus, sizeof(m->adb_bus),
> TYPE_ADB_BUS, DEVICE(obj), "adb.0");
> +
> +/* Init VIAs 1 and 2 */
> +object_initialize_child(OBJECT(m), "via1", >mos6522_via1,
Sorry, one more space at the end of the above line, and fail to run checkpatch.
Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
On 3/5/2020 2:54 PM, Pan Nengyuan wrote:
> This patch fix a bug in mac_via where it failed to actually realize devices
> it was using.
> And move the init codes which inits the mos6522 objects and properties on
> them from realize()
> into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it
> will cause
> device-introspect-test test fail. Then do the realize mos6522 device in the
> mac_vir_realize.
>
> Signed-off-by: Pan Nengyuan
> ---
> Cc: Laurent Vivier
> Cc: Mark Cave-Ayland
> ---
> v4->v3:
> - split v3 into two patches, this patch fix incorrect creation of mos6522,
> move inits and props
> from realize into init. The v3 is:
> https://patchwork.kernel.org/patch/11407635/
> ---
> hw/misc/mac_via.c | 43 ++-
> 1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index b7d0012794..4c5ad08805 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -868,24 +868,24 @@ static void mac_via_reset(DeviceState *dev)
> static void mac_via_realize(DeviceState *dev, Error **errp)
> {
> MacVIAState *m = MAC_VIA(dev);
> -MOS6522State *ms;
> struct tm tm;
> int ret;
> +Error *err = NULL;
>
> -/* Init VIAs 1 and 2 */
> -sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
> - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
> +qdev_set_parent_bus(DEVICE(>mos6522_via1), sysbus_get_default());
> +qdev_set_parent_bus(DEVICE(>mos6522_via2), sysbus_get_default());
>
> -sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
> - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
> +object_property_set_bool(OBJECT(>mos6522_via1), true, "realized",
> );
> +if (err != NULL) {
> +error_propagate(errp, err);
> +return;
> +}
>
> -/* Pass through mos6522 output IRQs */
> -ms = MOS6522(>mos6522_via1);
> -object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
> -ms = MOS6522(>mos6522_via2);
> -object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
> - SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
> +object_property_set_bool(OBJECT(>mos6522_via2), true, "realized",
> );
> +if (err != NULL) {
> +error_propagate(errp, err);
> +return;
> +}
>
> /* Pass through mos6522 input IRQs */
> qdev_pass_gpios(DEVICE(>mos6522_via1), dev, "via1-irq");
> @@ -932,6 +932,7 @@ static void mac_via_init(Object *obj)
> {
> SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> MacVIAState *m = MAC_VIA(obj);
> +MOS6522State *ms;
>
> /* MMIO */
> memory_region_init(>mmio, obj, "mac-via", 2 * VIA_SIZE);
> @@ -948,6 +949,22 @@ static void mac_via_init(Object *obj)
> /* ADB */
> qbus_create_inplace((BusState *)>adb_bus, sizeof(m->adb_bus),
> TYPE_ADB_BUS, DEVICE(obj), "adb.0");
> +
> +/* Init VIAs 1 and 2 */
> +object_initialize_child(OBJECT(m), "via1", >mos6522_via1,Sorry, one
> more space at the end of the above line, and fail to run checkpatch.
> +sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1,
> +_abort, NULL);
> +object_initialize_child(OBJECT(m), "via2", >mos6522_via2,
> +sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2,
> +_abort, NULL);
> +
> +/* Pass through mos6522 output IRQs */
> +ms = MOS6522(>mos6522_via1);
> +object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms),
> + SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
> +ms = MOS6522(>mos6522_via2);
> +object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms),
> + SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
> }
>
> static void postload_update_cb(void *opaque, int running, RunState state)
>
[PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
This patch fix a bug in mac_via where it failed to actually realize devices it
was using.
And move the init codes which inits the mos6522 objects and properties on them
from realize()
into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it
will cause
device-introspect-test test fail. Then do the realize mos6522 device in the
mac_vir_realize.
Signed-off-by: Pan Nengyuan
---
Cc: Laurent Vivier
Cc: Mark Cave-Ayland
---
v4->v3:
- split v3 into two patches, this patch fix incorrect creation of mos6522, move
inits and props
from realize into init. The v3 is:
https://patchwork.kernel.org/patch/11407635/
---
hw/misc/mac_via.c | 43 ++-
1 file changed, 30 insertions(+), 13 deletions(-)
diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index b7d0012794..4c5ad08805 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -868,24 +868,24 @@ static void mac_via_reset(DeviceState *dev)
static void mac_via_realize(DeviceState *dev, Error **errp)
{
MacVIAState *m = MAC_VIA(dev);
-MOS6522State *ms;
struct tm tm;
int ret;
+Error *err = NULL;
-/* Init VIAs 1 and 2 */
-sysbus_init_child_obj(OBJECT(dev), "via1", >mos6522_via1,
- sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1);
+qdev_set_parent_bus(DEVICE(>mos6522_via1), sysbus_get_default());
+qdev_set_parent_bus(DEVICE(>mos6522_via2), sysbus_get_default());
-sysbus_init_child_obj(OBJECT(dev), "via2", >mos6522_via2,
- sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2);
+object_property_set_bool(OBJECT(>mos6522_via1), true, "realized", );
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
-/* Pass through mos6522 output IRQs */
-ms = MOS6522(>mos6522_via1);
-object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms),
- SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
-ms = MOS6522(>mos6522_via2);
-object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms),
- SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
+object_property_set_bool(OBJECT(>mos6522_via2), true, "realized", );
+if (err != NULL) {
+error_propagate(errp, err);
+return;
+}
/* Pass through mos6522 input IRQs */
qdev_pass_gpios(DEVICE(>mos6522_via1), dev, "via1-irq");
@@ -932,6 +932,7 @@ static void mac_via_init(Object *obj)
{
SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
MacVIAState *m = MAC_VIA(obj);
+MOS6522State *ms;
/* MMIO */
memory_region_init(>mmio, obj, "mac-via", 2 * VIA_SIZE);
@@ -948,6 +949,22 @@ static void mac_via_init(Object *obj)
/* ADB */
qbus_create_inplace((BusState *)>adb_bus, sizeof(m->adb_bus),
TYPE_ADB_BUS, DEVICE(obj), "adb.0");
+
+/* Init VIAs 1 and 2 */
+object_initialize_child(OBJECT(m), "via1", >mos6522_via1,
+sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1,
+_abort, NULL);
+object_initialize_child(OBJECT(m), "via2", >mos6522_via2,
+sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2,
+_abort, NULL);
+
+/* Pass through mos6522 output IRQs */
+ms = MOS6522(>mos6522_via1);
+object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms),
+ SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
+ms = MOS6522(>mos6522_via2);
+object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms),
+ SYSBUS_DEVICE_GPIO_IRQ "[0]", _abort);
}
static void postload_update_cb(void *opaque, int running, RunState state)
--
2.18.2
