| Message ID | 7d5904ae-e66c-5594-cfff-747578d91f4b@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
On 06/19/17 18:57, Mark Cave-Ayland wrote: > On 19/06/17 15:28, Eduardo Habkost wrote: > >> On Mon, Jun 19, 2017 at 01:59:07PM +0100, Mark Cave-Ayland wrote: >>> In preparation for calling fw_cfg_init1() during realize rather than during >>> init, move the assert() checking for existing fw_cfg devices and the linking >>> of the device to the machine with object_property_add_child() to a new >>> fw_cfg instance_init() function. >>> >>> This guarantees that we will still assert() correctly if more than one fw_cfg >>> device is instantiated by accident. >>> >>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >>> Reviewed-by: Laszlo Ersek <lersek@redhat.com> >>> Tested-by: Laszlo Ersek <lersek@redhat.com> >>> --- >>> hw/nvram/fw_cfg.c | 14 ++++++++++---- >>> 1 file changed, 10 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c >>> index 99bdbc2..af45012 100644 >>> --- a/hw/nvram/fw_cfg.c >>> +++ b/hw/nvram/fw_cfg.c >>> @@ -915,10 +915,6 @@ static void fw_cfg_init1(DeviceState *dev) >>> MachineState *machine = MACHINE(qdev_get_machine()); >>> uint32_t version = FW_CFG_VERSION; >>> >>> - assert(!object_resolve_path(FW_CFG_PATH, NULL)); >>> - >>> - object_property_add_child(OBJECT(machine), FW_CFG_NAME, OBJECT(s), NULL); >>> - >>> qdev_init_nofail(dev); >>> >>> fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4); >>> @@ -1020,6 +1016,15 @@ FWCfgState *fw_cfg_find(void) >>> return FW_CFG(object_resolve_path(FW_CFG_PATH, NULL)); >>> } >>> >>> +static void fw_cfg_init(Object *obj) >>> +{ >>> + MachineState *machine = MACHINE(qdev_get_machine()); >>> + >>> + assert(!object_resolve_path(FW_CFG_PATH, NULL)); >>> + >>> + object_property_add_child(OBJECT(machine), FW_CFG_NAME, obj, NULL); >> >> I don't think this belongs to instance_init. We must always be >> able to instantiate objects without crashing QEMU or affecting >> QEMU global state. This patch makes device-list-properties >> crash: >> >> $ qemu-system-x86_64 -display none -qmp unix:/tmp/qmp,server,nowait & >> [1] 2848 >> $ echo 'device-list-properties typename=fw_cfg_mem' | ./scripts/qmp/qmp-shell /tmp/qmp >> Welcome to the QMP low-level shell! >> Connected to QEMU 2.9.50 >> >> qemu-system-x86_64: qemu/hw/nvram/fw_cfg.c:974: fw_cfg_init: Assertion `!object_resolve_path(FW_CFG_PATH, NULL)' failed. >> (QEMU) Disconnected >> [1]+ Aborted (core dumped) qemu-system-x86_64 -display none -qmp unix:/tmp/qmp,server,nowait >> $ >> >> >> I suggest moving this check to realize, like the rest of >> fw_cfg_init1(), but change it to do proper error reporting >> instead of asserting. > > In that case what do you think is the best way to prevent realization of > a second instance? With some playing I've managed to come up with the > following (partial) diff that seems to work in some simple local tests: > > > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c > index 9b0aaa2..e678ec0 100644 > --- a/hw/nvram/fw_cfg.c > +++ b/hw/nvram/fw_cfg.c > @@ -862,12 +862,20 @@ static void fw_cfg_machine_ready(struct Notifier > *n, void *data) > > -static void fw_cfg_common_realize(DeviceState *dev) > +static void fw_cfg_common_realize(DeviceState *dev, Error **errp) > { > FWCfgState *s = FW_CFG(dev); > MachineState *machine = MACHINE(qdev_get_machine()); > uint32_t version = FW_CFG_VERSION; > - > + Object *obj; > + > + obj = object_resolve_path(FW_CFG_PATH, NULL); > + if (obj != OBJECT(dev)) { > + error_setg(errp, "Only one fw_cfg device can be instantiated per " > + "machine"); > + return; > + } > + > fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4); > fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16); > fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, > (uint16_t)!machine->enable_graphics); > > > What seems to happen is that calling object_property_add_child() only > succeeds for the first instance and so a simple comparison is enough to > determine that the device already exists at FW_CFG_PATH. Or is this a > fairly terrible (ab)use of the QOM APIs? This has jogged my memory about how we ensure "at most one" for the vmgenid device. Please see: vmgenid_realize() [hw/acpi/vmgenid.c] find_vmgenid_dev() [include/hw/acpi/vmgenid.h] ... I was quite silly not to think of this on my own now, despite having authored commit f92063028a0e ("hw/acpi/vmgenid: prevent more than one vmgenid device", 2017-03-20) :/ Thanks, Laszlo
On 19/06/17 18:09, Laszlo Ersek wrote: >> What seems to happen is that calling object_property_add_child() only >> succeeds for the first instance and so a simple comparison is enough to >> determine that the device already exists at FW_CFG_PATH. Or is this a >> fairly terrible (ab)use of the QOM APIs? > > This has jogged my memory about how we ensure "at most one" for the > vmgenid device. Please see: > > vmgenid_realize() [hw/acpi/vmgenid.c] > find_vmgenid_dev() [include/hw/acpi/vmgenid.h] > > ... I was quite silly not to think of this on my own now, despite having > authored commit f92063028a0e ("hw/acpi/vmgenid: prevent more than one > vmgenid device", 2017-03-20) :/ Right that definitely helps - the following code seems to work correctly when trying to instantiate a mixture of fw_cfg_io and/or fw_cfg_mem types: if (!object_resolve_path_type("", TYPE_FW_CFG, NULL)) { error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG); return; } I've also copied the wording from the above commit to make everything consistent. Does that seem okay? If so, I'll fold it into a v7 patchset. ATB, Mark.
On 06/19/17 20:49, Mark Cave-Ayland wrote: > On 19/06/17 18:09, Laszlo Ersek wrote: > >>> What seems to happen is that calling object_property_add_child() only >>> succeeds for the first instance and so a simple comparison is enough to >>> determine that the device already exists at FW_CFG_PATH. Or is this a >>> fairly terrible (ab)use of the QOM APIs? >> >> This has jogged my memory about how we ensure "at most one" for the >> vmgenid device. Please see: >> >> vmgenid_realize() [hw/acpi/vmgenid.c] >> find_vmgenid_dev() [include/hw/acpi/vmgenid.h] >> >> ... I was quite silly not to think of this on my own now, despite having >> authored commit f92063028a0e ("hw/acpi/vmgenid: prevent more than one >> vmgenid device", 2017-03-20) :/ > > Right that definitely helps - the following code seems to work correctly > when trying to instantiate a mixture of fw_cfg_io and/or fw_cfg_mem types: > > if (!object_resolve_path_type("", TYPE_FW_CFG, NULL)) { > error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG); > return; > } > > I've also copied the wording from the above commit to make everything > consistent. Does that seem okay? If so, I'll fold it into a v7 patchset. It looks good to me, but please await Eduardo's reply as well. In particular, it should be confirmed that object_resolve_path_type() matches instances of *subclasses* as well (as I expect it would). Your test results confirm that; let's make sure it is intentional behavior. Eduardo (or someone else on the CC list), can you please comment on that? Thanks! Laszlo
On 19/06/17 23:43, Laszlo Ersek wrote: > It looks good to me, but please await Eduardo's reply as well. > > In particular, it should be confirmed that object_resolve_path_type() > matches instances of *subclasses* as well (as I expect it would). Your > test results confirm that; let's make sure it is intentional behavior. > Eduardo (or someone else on the CC list), can you please comment on that? Thanks Laszlo. Eduardo, can you confirm this is the correct behaviour? I now have a v7 patchset ready to go (currently hosted at https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo, I've currently left off your Tested-by tag since I'm not sure it's still valid for less-than-trivial changes - if you're happy for me to re-add it before I send the v7 patchset to the list, please let me know. ATB, Mark.
On 06/21/17 08:58, Mark Cave-Ayland wrote: > On 19/06/17 23:43, Laszlo Ersek wrote: > >> It looks good to me, but please await Eduardo's reply as well. >> >> In particular, it should be confirmed that object_resolve_path_type() >> matches instances of *subclasses* as well (as I expect it would). Your >> test results confirm that; let's make sure it is intentional behavior. >> Eduardo (or someone else on the CC list), can you please comment on that? > > Thanks Laszlo. Eduardo, can you confirm this is the correct behaviour? > > I now have a v7 patchset ready to go (currently hosted at > https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo, > I've currently left off your Tested-by tag since I'm not sure it's still > valid for less-than-trivial changes - if you're happy for me to re-add > it before I send the v7 patchset to the list, please let me know. I intend to test v7 when you post it. Thanks, Laszlo
On Wed, Jun 21, 2017 at 09:48:16AM +0200, Laszlo Ersek wrote: > On 06/21/17 08:58, Mark Cave-Ayland wrote: > > On 19/06/17 23:43, Laszlo Ersek wrote: > > > >> It looks good to me, but please await Eduardo's reply as well. > >> > >> In particular, it should be confirmed that object_resolve_path_type() > >> matches instances of *subclasses* as well (as I expect it would). Your > >> test results confirm that; let's make sure it is intentional behavior. > >> Eduardo (or someone else on the CC list), can you please comment on that? > > > > Thanks Laszlo. Eduardo, can you confirm this is the correct behaviour? Sorry for taking so long to reply. Yes, it should be the correct behavior. It's how it's documented: "This is similar to object_resolve_path. However, when looking for a partial path only matches that implement the given type are considered. This restricts the search and avoids spuriously flagging matches as ambiguous." (Key part here is "implement the given type"). Approach used by commit f92063028a "hw/acpi/vmgenid: prevent more than one vmgenid device" looks good to me. > > > > I now have a v7 patchset ready to go (currently hosted at > > https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo, > > I've currently left off your Tested-by tag since I'm not sure it's still > > valid for less-than-trivial changes - if you're happy for me to re-add > > it before I send the v7 patchset to the list, please let me know. > > I intend to test v7 when you post it. I still see the instance_init assert() in that branch (commit 17d75643f880). Is that correct?
On 21/06/17 12:36, Eduardo Habkost wrote: > On Wed, Jun 21, 2017 at 09:48:16AM +0200, Laszlo Ersek wrote: >> On 06/21/17 08:58, Mark Cave-Ayland wrote: >>> On 19/06/17 23:43, Laszlo Ersek wrote: >>> >>>> It looks good to me, but please await Eduardo's reply as well. >>>> >>>> In particular, it should be confirmed that object_resolve_path_type() >>>> matches instances of *subclasses* as well (as I expect it would). Your >>>> test results confirm that; let's make sure it is intentional behavior. >>>> Eduardo (or someone else on the CC list), can you please comment on that? >>> >>> Thanks Laszlo. Eduardo, can you confirm this is the correct behaviour? > > Sorry for taking so long to reply. Yes, it should be the correct > behavior. It's how it's documented: > > "This is similar to object_resolve_path. However, when looking > for a partial path only matches that implement the given type are > considered. This restricts the search and avoids spuriously > flagging matches as ambiguous." > > (Key part here is "implement the given type"). > > Approach used by commit f92063028a "hw/acpi/vmgenid: prevent more > than one vmgenid device" looks good to me. That is great news, thanks for confirming this. >>> >>> I now have a v7 patchset ready to go (currently hosted at >>> https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo, >>> I've currently left off your Tested-by tag since I'm not sure it's still >>> valid for less-than-trivial changes - if you're happy for me to re-add >>> it before I send the v7 patchset to the list, please let me know. >> >> I intend to test v7 when you post it. > > I still see the instance_init assert() in that branch (commit > 17d75643f880). Is that correct? Yes that was the intention. In 17d75643f880 both the assert() and object_property_add_child() are moved into the instance_init() function, and then in the follow-up commit eddedb5 the assert() is removed from instance_init() and the object_resolve_path_type() check added into fw_cfg_init1() as part of its conversion into the fw_cfg_common_realize() function. One last question which might avoid a future v8 revision: does the error handling in eddedb5 for the fw_cfg_*_realize() functions look correct? The existing check for fw_cfg_file_slots_allocate() uses a local_err Error variable, whereas I've seen a mixture of callers using both this approach and using the errp Error variable directly. Is there any reason to prefer one over the other? Currently I'm also using local_err in order to keep the fw_cfg_*_realize() functions consistent. ATB, Mark.
On Wed, Jun 21, 2017 at 01:17:06PM +0100, Mark Cave-Ayland wrote: > On 21/06/17 12:36, Eduardo Habkost wrote: > > > On Wed, Jun 21, 2017 at 09:48:16AM +0200, Laszlo Ersek wrote: > >> On 06/21/17 08:58, Mark Cave-Ayland wrote: > >>> On 19/06/17 23:43, Laszlo Ersek wrote: > >>> > >>>> It looks good to me, but please await Eduardo's reply as well. > >>>> > >>>> In particular, it should be confirmed that object_resolve_path_type() > >>>> matches instances of *subclasses* as well (as I expect it would). Your > >>>> test results confirm that; let's make sure it is intentional behavior. > >>>> Eduardo (or someone else on the CC list), can you please comment on that? > >>> > >>> Thanks Laszlo. Eduardo, can you confirm this is the correct behaviour? > > > > Sorry for taking so long to reply. Yes, it should be the correct > > behavior. It's how it's documented: > > > > "This is similar to object_resolve_path. However, when looking > > for a partial path only matches that implement the given type are > > considered. This restricts the search and avoids spuriously > > flagging matches as ambiguous." > > > > (Key part here is "implement the given type"). > > > > Approach used by commit f92063028a "hw/acpi/vmgenid: prevent more > > than one vmgenid device" looks good to me. > > That is great news, thanks for confirming this. > > >>> > >>> I now have a v7 patchset ready to go (currently hosted at > >>> https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo, > >>> I've currently left off your Tested-by tag since I'm not sure it's still > >>> valid for less-than-trivial changes - if you're happy for me to re-add > >>> it before I send the v7 patchset to the list, please let me know. > >> > >> I intend to test v7 when you post it. > > > > I still see the instance_init assert() in that branch (commit > > 17d75643f880). Is that correct? > > Yes that was the intention. In 17d75643f880 both the assert() and > object_property_add_child() are moved into the instance_init() function, > and then in the follow-up commit eddedb5 the assert() is removed from > instance_init() and the object_resolve_path_type() check added into > fw_cfg_init1() as part of its conversion into the > fw_cfg_common_realize() function. We can't move assert() + linking to instance_init even if it's just temporary, as it makes device-list-properties crash. Just linking in instance_init is also a problem, because instance_init should never affect global QEMU state. You could move linking to realize as well, but: just like you already moved sysbus_add_io() calls outside realize, I believe linking belongs outside realize too. I need to re-read the discussion threads to understand the motivation behind that. > > One last question which might avoid a future v8 revision: does the error > handling in eddedb5 for the fw_cfg_*_realize() functions look correct? The error handling looks correct to me. > > The existing check for fw_cfg_file_slots_allocate() uses a local_err > Error variable, whereas I've seen a mixture of callers using both this > approach and using the errp Error variable directly. Is there any reason > to prefer one over the other? Currently I'm also using local_err in > order to keep the fw_cfg_*_realize() functions consistent. You need a local_err variable if you need to handle/check for errors before propagating them. Quoting qapi/error.h: Create a new error and pass it to the caller: error_setg(errp, "situation normal, all fouled up"); Call a function and receive an error from it: Error *err = NULL; foo(arg, &err); if (err) { handle the error... } [...] Receive an error and pass it on to the caller: Error *err = NULL; foo(arg, &err); if (err) { handle the error... error_propagate(errp, err); } where Error **errp is a parameter, by convention the last one. Do *not* "optimize" this to foo(arg, errp); if (*errp) { // WRONG! handle the error... } because errp may be NULL! But when all you do with the error is pass it on, please use foo(arg, errp); for readability. In fw_cfg_*_realize() you can call fw_cfg_common_realize(dev, errp) directly, but it won't be a problem if you use local_err just to keep it consistent with the other error checks in the function.
On 21/06/17 14:23, Eduardo Habkost wrote: >>>>> I now have a v7 patchset ready to go (currently hosted at >>>>> https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo, >>>>> I've currently left off your Tested-by tag since I'm not sure it's still >>>>> valid for less-than-trivial changes - if you're happy for me to re-add >>>>> it before I send the v7 patchset to the list, please let me know. >>>> >>>> I intend to test v7 when you post it. >>> >>> I still see the instance_init assert() in that branch (commit >>> 17d75643f880). Is that correct? >> >> Yes that was the intention. In 17d75643f880 both the assert() and >> object_property_add_child() are moved into the instance_init() function, >> and then in the follow-up commit eddedb5 the assert() is removed from >> instance_init() and the object_resolve_path_type() check added into >> fw_cfg_init1() as part of its conversion into the >> fw_cfg_common_realize() function. > > We can't move assert() + linking to instance_init even if it's > just temporary, as it makes device-list-properties crash. > > Just linking in instance_init is also a problem, because > instance_init should never affect global QEMU state. > > You could move linking to realize as well, but: just like you > already moved sysbus_add_io() calls outside realize, I believe > linking belongs outside realize too. I need to re-read the > discussion threads to understand the motivation behind that. Ultimately the question we're trying to answer is "has someone instantiated another fw_cfg device for this machine?" and the way it works currently is that fw_cfg_init_io() and fw_cfg_init_mem() attach the fw_cfg device to the /machine object and then check after realize whether a /machine/fw_cfg device already exists, aborting if it does. So in the current implementation we're not actually concerned with the placement of fw_cfg within the model itself, and indeed with a fixed location in the QOM tree it's already completely wrong. If you take a look at the QOM tree for the sparc/sparc64/ppc machines you'll see that they really are very far from reality. For me the use of object_resolve_path_type() during realize is a good solution since regardless of the location of the fw_cfg we can always detect whether we have more than one device instance. However what seems unappealing about this is that while all existing users which use fw_cfg_init_io() and fw_cfg_init_mem() are fine, in the case where I am wiring up the device myself then for my sun4u example the code looks like this: dev = qdev_create(NULL, TYPE_FW_CFG_IO); ... qdev_init_nofail(dev); memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT, &FW_CFG_IO(dev)->comb_iomem); Here you can see that the device is active because it is mapped into the correct IO address space, but notice I have forgotten to link it to the QOM /machine object myself. Hence I can instantiate and map as many instances as I like and never trigger the duplicate instance check which makes the check fairly ineffective. This makes me think that I'm still misunderstanding something, so I'd be grateful for any further suggestions. >> One last question which might avoid a future v8 revision: does the error >> handling in eddedb5 for the fw_cfg_*_realize() functions look correct? > > The error handling looks correct to me. Thanks for the review, in that case I will leave it in its current form. ATB, Mark.
On Fri, Jun 23, 2017 at 09:12:01AM +0100, Mark Cave-Ayland wrote: > On 21/06/17 14:23, Eduardo Habkost wrote: > > >>>>> I now have a v7 patchset ready to go (currently hosted at > >>>>> https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo, > >>>>> I've currently left off your Tested-by tag since I'm not sure it's still > >>>>> valid for less-than-trivial changes - if you're happy for me to re-add > >>>>> it before I send the v7 patchset to the list, please let me know. > >>>> > >>>> I intend to test v7 when you post it. > >>> > >>> I still see the instance_init assert() in that branch (commit > >>> 17d75643f880). Is that correct? > >> > >> Yes that was the intention. In 17d75643f880 both the assert() and > >> object_property_add_child() are moved into the instance_init() function, > >> and then in the follow-up commit eddedb5 the assert() is removed from > >> instance_init() and the object_resolve_path_type() check added into > >> fw_cfg_init1() as part of its conversion into the > >> fw_cfg_common_realize() function. > > > > We can't move assert() + linking to instance_init even if it's > > just temporary, as it makes device-list-properties crash. > > > > Just linking in instance_init is also a problem, because > > instance_init should never affect global QEMU state. > > > > You could move linking to realize as well, but: just like you > > already moved sysbus_add_io() calls outside realize, I believe > > linking belongs outside realize too. I need to re-read the > > discussion threads to understand the motivation behind that. > > Ultimately the question we're trying to answer is "has someone > instantiated another fw_cfg device for this machine?" and the way it > works currently is that fw_cfg_init_io() and fw_cfg_init_mem() attach > the fw_cfg device to the /machine object and then check after realize > whether a /machine/fw_cfg device already exists, aborting if it does. > > So in the current implementation we're not actually concerned with the > placement of fw_cfg within the model itself, and indeed with a fixed > location in the QOM tree it's already completely wrong. If you take a > look at the QOM tree for the sparc/sparc64/ppc machines you'll see that > they really are very far from reality. > > For me the use of object_resolve_path_type() during realize is a good > solution since regardless of the location of the fw_cfg we can always > detect whether we have more than one device instance. > > However what seems unappealing about this is that while all existing > users which use fw_cfg_init_io() and fw_cfg_init_mem() are fine, in the > case where I am wiring up the device myself then for my sun4u example > the code looks like this: > > dev = qdev_create(NULL, TYPE_FW_CFG_IO); > ... > qdev_init_nofail(dev); > memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT, > &FW_CFG_IO(dev)->comb_iomem); > > Here you can see that the device is active because it is mapped into the > correct IO address space, but notice I have forgotten to link it to the > QOM /machine object myself. Hence I can instantiate and map as many > instances as I like and never trigger the duplicate instance check which > makes the check fairly ineffective. This is a good point, but I have a question about that: will something break if a machine decides to create two fw_cfg objects and map them to different addresses? If it won't break, I see no reason to try to enforce a single instance in the device code. If it will break, then a check in realize is still a hack, but might be a good enough solution.
On 06/23/17 13:50, Eduardo Habkost wrote: > On Fri, Jun 23, 2017 at 09:12:01AM +0100, Mark Cave-Ayland wrote: >> On 21/06/17 14:23, Eduardo Habkost wrote: >> >>>>>>> I now have a v7 patchset ready to go (currently hosted at >>>>>>> https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo, >>>>>>> I've currently left off your Tested-by tag since I'm not sure it's still >>>>>>> valid for less-than-trivial changes - if you're happy for me to re-add >>>>>>> it before I send the v7 patchset to the list, please let me know. >>>>>> >>>>>> I intend to test v7 when you post it. >>>>> >>>>> I still see the instance_init assert() in that branch (commit >>>>> 17d75643f880). Is that correct? >>>> >>>> Yes that was the intention. In 17d75643f880 both the assert() and >>>> object_property_add_child() are moved into the instance_init() function, >>>> and then in the follow-up commit eddedb5 the assert() is removed from >>>> instance_init() and the object_resolve_path_type() check added into >>>> fw_cfg_init1() as part of its conversion into the >>>> fw_cfg_common_realize() function. >>> >>> We can't move assert() + linking to instance_init even if it's >>> just temporary, as it makes device-list-properties crash. >>> >>> Just linking in instance_init is also a problem, because >>> instance_init should never affect global QEMU state. >>> >>> You could move linking to realize as well, but: just like you >>> already moved sysbus_add_io() calls outside realize, I believe >>> linking belongs outside realize too. I need to re-read the >>> discussion threads to understand the motivation behind that. >> >> Ultimately the question we're trying to answer is "has someone >> instantiated another fw_cfg device for this machine?" and the way it >> works currently is that fw_cfg_init_io() and fw_cfg_init_mem() attach >> the fw_cfg device to the /machine object and then check after realize >> whether a /machine/fw_cfg device already exists, aborting if it does. >> >> So in the current implementation we're not actually concerned with the >> placement of fw_cfg within the model itself, and indeed with a fixed >> location in the QOM tree it's already completely wrong. If you take a >> look at the QOM tree for the sparc/sparc64/ppc machines you'll see that >> they really are very far from reality. >> >> For me the use of object_resolve_path_type() during realize is a good >> solution since regardless of the location of the fw_cfg we can always >> detect whether we have more than one device instance. >> >> However what seems unappealing about this is that while all existing >> users which use fw_cfg_init_io() and fw_cfg_init_mem() are fine, in the >> case where I am wiring up the device myself then for my sun4u example >> the code looks like this: >> >> dev = qdev_create(NULL, TYPE_FW_CFG_IO); >> ... >> qdev_init_nofail(dev); >> memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT, >> &FW_CFG_IO(dev)->comb_iomem); >> >> Here you can see that the device is active because it is mapped into the >> correct IO address space, but notice I have forgotten to link it to the >> QOM /machine object myself. Hence I can instantiate and map as many >> instances as I like and never trigger the duplicate instance check which >> makes the check fairly ineffective. > > This is a good point, but I have a question about that: will something > break if a machine decides to create two fw_cfg objects and map them to > different addresses? If it won't break, I see no reason to try to > enforce a single instance in the device code. If it will break, then a > check in realize is still a hack, but might be a good enough solution. > (1) For the guest, it makes no sense to encounter two fw_cfg devices. Also, a lot of the existent code in QEMU assumes at most one fw_cfg device (for example, there is some related ACPI generation). (2) Relatedly, the fw_cfg_find() helper function is used quite widely, and it shouldn't break -- either due to more-than-one device instances, or due to the one fw_cfg device being linked under a path that is different from FW_CFG_PATH. Thanks Laszlo
On Fri, Jun 23, 2017 at 05:52:03PM +0200, Laszlo Ersek wrote: > On 06/23/17 13:50, Eduardo Habkost wrote: > > On Fri, Jun 23, 2017 at 09:12:01AM +0100, Mark Cave-Ayland wrote: > >> On 21/06/17 14:23, Eduardo Habkost wrote: > >> > >>>>>>> I now have a v7 patchset ready to go (currently hosted at > >>>>>>> https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo, > >>>>>>> I've currently left off your Tested-by tag since I'm not sure it's still > >>>>>>> valid for less-than-trivial changes - if you're happy for me to re-add > >>>>>>> it before I send the v7 patchset to the list, please let me know. > >>>>>> > >>>>>> I intend to test v7 when you post it. > >>>>> > >>>>> I still see the instance_init assert() in that branch (commit > >>>>> 17d75643f880). Is that correct? > >>>> > >>>> Yes that was the intention. In 17d75643f880 both the assert() and > >>>> object_property_add_child() are moved into the instance_init() function, > >>>> and then in the follow-up commit eddedb5 the assert() is removed from > >>>> instance_init() and the object_resolve_path_type() check added into > >>>> fw_cfg_init1() as part of its conversion into the > >>>> fw_cfg_common_realize() function. > >>> > >>> We can't move assert() + linking to instance_init even if it's > >>> just temporary, as it makes device-list-properties crash. > >>> > >>> Just linking in instance_init is also a problem, because > >>> instance_init should never affect global QEMU state. > >>> > >>> You could move linking to realize as well, but: just like you > >>> already moved sysbus_add_io() calls outside realize, I believe > >>> linking belongs outside realize too. I need to re-read the > >>> discussion threads to understand the motivation behind that. > >> > >> Ultimately the question we're trying to answer is "has someone > >> instantiated another fw_cfg device for this machine?" and the way it > >> works currently is that fw_cfg_init_io() and fw_cfg_init_mem() attach > >> the fw_cfg device to the /machine object and then check after realize > >> whether a /machine/fw_cfg device already exists, aborting if it does. > >> > >> So in the current implementation we're not actually concerned with the > >> placement of fw_cfg within the model itself, and indeed with a fixed > >> location in the QOM tree it's already completely wrong. If you take a > >> look at the QOM tree for the sparc/sparc64/ppc machines you'll see that > >> they really are very far from reality. > >> > >> For me the use of object_resolve_path_type() during realize is a good > >> solution since regardless of the location of the fw_cfg we can always > >> detect whether we have more than one device instance. > >> > >> However what seems unappealing about this is that while all existing > >> users which use fw_cfg_init_io() and fw_cfg_init_mem() are fine, in the > >> case where I am wiring up the device myself then for my sun4u example > >> the code looks like this: > >> > >> dev = qdev_create(NULL, TYPE_FW_CFG_IO); > >> ... > >> qdev_init_nofail(dev); > >> memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT, > >> &FW_CFG_IO(dev)->comb_iomem); > >> > >> Here you can see that the device is active because it is mapped into the > >> correct IO address space, but notice I have forgotten to link it to the > >> QOM /machine object myself. Hence I can instantiate and map as many > >> instances as I like and never trigger the duplicate instance check which > >> makes the check fairly ineffective. > > > > This is a good point, but I have a question about that: will something > > break if a machine decides to create two fw_cfg objects and map them to > > different addresses? If it won't break, I see no reason to try to > > enforce a single instance in the device code. If it will break, then a > > check in realize is still a hack, but might be a good enough solution. > > > > (1) For the guest, it makes no sense to encounter two fw_cfg devices. > Also, a lot of the existent code in QEMU assumes at most one fw_cfg > device (for example, there is some related ACPI generation). This is an argument for making board code ensure there's only one device, and possibly for providing a helper that board code can use. But it doesn't require validation on realize. > > (2) Relatedly, the fw_cfg_find() helper function is used quite widely, > and it shouldn't break -- either due to more-than-one device instances, > or due to the one fw_cfg device being linked under a path that is > different from FW_CFG_PATH. This is also an argument for providing a helper that ensures fw_cfg_find() will work, but doesn't require validation on realize. All that said, I don't have a strong argument against doing it on realize, except my gut feeling that this is not how qdev was designed[1]. If doing it on realize is the simplest way to do it, I won't be the one complaining. [1] I believe the original intent was to make every single device user-creatable and define boards in a declarative way in config files. We are very far from that goal.
On 06/23/17 18:10, Eduardo Habkost wrote: > On Fri, Jun 23, 2017 at 05:52:03PM +0200, Laszlo Ersek wrote: >> On 06/23/17 13:50, Eduardo Habkost wrote: >>> On Fri, Jun 23, 2017 at 09:12:01AM +0100, Mark Cave-Ayland wrote: >>>> On 21/06/17 14:23, Eduardo Habkost wrote: >>>> >>>>>>>>> I now have a v7 patchset ready to go (currently hosted at >>>>>>>>> https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo, >>>>>>>>> I've currently left off your Tested-by tag since I'm not sure it's still >>>>>>>>> valid for less-than-trivial changes - if you're happy for me to re-add >>>>>>>>> it before I send the v7 patchset to the list, please let me know. >>>>>>>> >>>>>>>> I intend to test v7 when you post it. >>>>>>> >>>>>>> I still see the instance_init assert() in that branch (commit >>>>>>> 17d75643f880). Is that correct? >>>>>> >>>>>> Yes that was the intention. In 17d75643f880 both the assert() and >>>>>> object_property_add_child() are moved into the instance_init() function, >>>>>> and then in the follow-up commit eddedb5 the assert() is removed from >>>>>> instance_init() and the object_resolve_path_type() check added into >>>>>> fw_cfg_init1() as part of its conversion into the >>>>>> fw_cfg_common_realize() function. >>>>> >>>>> We can't move assert() + linking to instance_init even if it's >>>>> just temporary, as it makes device-list-properties crash. >>>>> >>>>> Just linking in instance_init is also a problem, because >>>>> instance_init should never affect global QEMU state. >>>>> >>>>> You could move linking to realize as well, but: just like you >>>>> already moved sysbus_add_io() calls outside realize, I believe >>>>> linking belongs outside realize too. I need to re-read the >>>>> discussion threads to understand the motivation behind that. >>>> >>>> Ultimately the question we're trying to answer is "has someone >>>> instantiated another fw_cfg device for this machine?" and the way it >>>> works currently is that fw_cfg_init_io() and fw_cfg_init_mem() attach >>>> the fw_cfg device to the /machine object and then check after realize >>>> whether a /machine/fw_cfg device already exists, aborting if it does. >>>> >>>> So in the current implementation we're not actually concerned with the >>>> placement of fw_cfg within the model itself, and indeed with a fixed >>>> location in the QOM tree it's already completely wrong. If you take a >>>> look at the QOM tree for the sparc/sparc64/ppc machines you'll see that >>>> they really are very far from reality. >>>> >>>> For me the use of object_resolve_path_type() during realize is a good >>>> solution since regardless of the location of the fw_cfg we can always >>>> detect whether we have more than one device instance. >>>> >>>> However what seems unappealing about this is that while all existing >>>> users which use fw_cfg_init_io() and fw_cfg_init_mem() are fine, in the >>>> case where I am wiring up the device myself then for my sun4u example >>>> the code looks like this: >>>> >>>> dev = qdev_create(NULL, TYPE_FW_CFG_IO); >>>> ... >>>> qdev_init_nofail(dev); >>>> memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT, >>>> &FW_CFG_IO(dev)->comb_iomem); >>>> >>>> Here you can see that the device is active because it is mapped into the >>>> correct IO address space, but notice I have forgotten to link it to the >>>> QOM /machine object myself. Hence I can instantiate and map as many >>>> instances as I like and never trigger the duplicate instance check which >>>> makes the check fairly ineffective. >>> >>> This is a good point, but I have a question about that: will something >>> break if a machine decides to create two fw_cfg objects and map them to >>> different addresses? If it won't break, I see no reason to try to >>> enforce a single instance in the device code. If it will break, then a >>> check in realize is still a hack, but might be a good enough solution. >>> >> >> (1) For the guest, it makes no sense to encounter two fw_cfg devices. >> Also, a lot of the existent code in QEMU assumes at most one fw_cfg >> device (for example, there is some related ACPI generation). > > This is an argument for making board code ensure there's only one > device, and possibly for providing a helper that board code can use. > But it doesn't require validation on realize. > >> >> (2) Relatedly, the fw_cfg_find() helper function is used quite widely, >> and it shouldn't break -- either due to more-than-one device instances, >> or due to the one fw_cfg device being linked under a path that is >> different from FW_CFG_PATH. > > This is also an argument for providing a helper that ensures > fw_cfg_find() will work, but doesn't require validation on realize. I agree. If you read back the discussion threads under the earlier versions of this patch set, you'll find that I argued for keeping the unicity stuff -- and possibly some other stuff -- that currently resides in the fw_cfg_init1() *helper* function outside of device code (realize or otherwise). I didn't disagree with the reorganization of the code, but I suggested to preserve this stuff in helper functions, which board code could call. This would clearly place the same burden on Mark's new sun4u board code -- i.e., that new board code would *also* have to call these helper function(s). Mark disliked this board requirement (he only wanted to instantiate the device and be done with it, in board code); and we went from there. Really, please go back to the earlier discussion around fw_cfg_init1() and you'll see my original point (which matches what you just voiced). > All that said, I don't have a strong argument against doing it on > realize, except my gut feeling that this is not how qdev was > designed[1]. If doing it on realize is the simplest way to do it, I > won't be the one complaining. > > [1] I believe the original intent was to make every single device > user-creatable and define boards in a declarative way in config > files. We are very far from that goal. I'm fine either way, I just wanted to accommodate Mark's preference, because he seemed to strongly dislike having to call any helper functions from board code, beyond initing and realizing the device. This is my recollection of the earlier discussion anyway. Thanks Laszlo
On Fri, Jun 23, 2017 at 06:48:40PM +0200, Laszlo Ersek wrote: > On 06/23/17 18:10, Eduardo Habkost wrote: > > On Fri, Jun 23, 2017 at 05:52:03PM +0200, Laszlo Ersek wrote: > >> On 06/23/17 13:50, Eduardo Habkost wrote: > >>> On Fri, Jun 23, 2017 at 09:12:01AM +0100, Mark Cave-Ayland wrote: > >>>> On 21/06/17 14:23, Eduardo Habkost wrote: > >>>> > >>>>>>>>> I now have a v7 patchset ready to go (currently hosted at > >>>>>>>>> https://github.com/mcayland/qemu/tree/fwcfg7 for the curious). Laszlo, > >>>>>>>>> I've currently left off your Tested-by tag since I'm not sure it's still > >>>>>>>>> valid for less-than-trivial changes - if you're happy for me to re-add > >>>>>>>>> it before I send the v7 patchset to the list, please let me know. > >>>>>>>> > >>>>>>>> I intend to test v7 when you post it. > >>>>>>> > >>>>>>> I still see the instance_init assert() in that branch (commit > >>>>>>> 17d75643f880). Is that correct? > >>>>>> > >>>>>> Yes that was the intention. In 17d75643f880 both the assert() and > >>>>>> object_property_add_child() are moved into the instance_init() function, > >>>>>> and then in the follow-up commit eddedb5 the assert() is removed from > >>>>>> instance_init() and the object_resolve_path_type() check added into > >>>>>> fw_cfg_init1() as part of its conversion into the > >>>>>> fw_cfg_common_realize() function. > >>>>> > >>>>> We can't move assert() + linking to instance_init even if it's > >>>>> just temporary, as it makes device-list-properties crash. > >>>>> > >>>>> Just linking in instance_init is also a problem, because > >>>>> instance_init should never affect global QEMU state. > >>>>> > >>>>> You could move linking to realize as well, but: just like you > >>>>> already moved sysbus_add_io() calls outside realize, I believe > >>>>> linking belongs outside realize too. I need to re-read the > >>>>> discussion threads to understand the motivation behind that. > >>>> > >>>> Ultimately the question we're trying to answer is "has someone > >>>> instantiated another fw_cfg device for this machine?" and the way it > >>>> works currently is that fw_cfg_init_io() and fw_cfg_init_mem() attach > >>>> the fw_cfg device to the /machine object and then check after realize > >>>> whether a /machine/fw_cfg device already exists, aborting if it does. > >>>> > >>>> So in the current implementation we're not actually concerned with the > >>>> placement of fw_cfg within the model itself, and indeed with a fixed > >>>> location in the QOM tree it's already completely wrong. If you take a > >>>> look at the QOM tree for the sparc/sparc64/ppc machines you'll see that > >>>> they really are very far from reality. > >>>> > >>>> For me the use of object_resolve_path_type() during realize is a good > >>>> solution since regardless of the location of the fw_cfg we can always > >>>> detect whether we have more than one device instance. > >>>> > >>>> However what seems unappealing about this is that while all existing > >>>> users which use fw_cfg_init_io() and fw_cfg_init_mem() are fine, in the > >>>> case where I am wiring up the device myself then for my sun4u example > >>>> the code looks like this: > >>>> > >>>> dev = qdev_create(NULL, TYPE_FW_CFG_IO); > >>>> ... > >>>> qdev_init_nofail(dev); > >>>> memory_region_add_subregion(pci_address_space_io(ebus), BIOS_CFG_IOPORT, > >>>> &FW_CFG_IO(dev)->comb_iomem); > >>>> > >>>> Here you can see that the device is active because it is mapped into the > >>>> correct IO address space, but notice I have forgotten to link it to the > >>>> QOM /machine object myself. Hence I can instantiate and map as many > >>>> instances as I like and never trigger the duplicate instance check which > >>>> makes the check fairly ineffective. > >>> > >>> This is a good point, but I have a question about that: will something > >>> break if a machine decides to create two fw_cfg objects and map them to > >>> different addresses? If it won't break, I see no reason to try to > >>> enforce a single instance in the device code. If it will break, then a > >>> check in realize is still a hack, but might be a good enough solution. > >>> > >> > >> (1) For the guest, it makes no sense to encounter two fw_cfg devices. > >> Also, a lot of the existent code in QEMU assumes at most one fw_cfg > >> device (for example, there is some related ACPI generation). > > > > This is an argument for making board code ensure there's only one > > device, and possibly for providing a helper that board code can use. > > But it doesn't require validation on realize. > > > >> > >> (2) Relatedly, the fw_cfg_find() helper function is used quite widely, > >> and it shouldn't break -- either due to more-than-one device instances, > >> or due to the one fw_cfg device being linked under a path that is > >> different from FW_CFG_PATH. > > > > This is also an argument for providing a helper that ensures > > fw_cfg_find() will work, but doesn't require validation on realize. > > I agree. > > If you read back the discussion threads under the earlier versions of > this patch set, you'll find that I argued for keeping the unicity stuff > -- and possibly some other stuff -- that currently resides in the > fw_cfg_init1() *helper* function outside of device code (realize or > otherwise). > > I didn't disagree with the reorganization of the code, but I suggested > to preserve this stuff in helper functions, which board code could call. > This would clearly place the same burden on Mark's new sun4u board code > -- i.e., that new board code would *also* have to call these helper > function(s). Mark disliked this board requirement (he only wanted to > instantiate the device and be done with it, in board code); and we went > from there. > > Really, please go back to the earlier discussion around fw_cfg_init1() > and you'll see my original point (which matches what you just voiced). Yep. I was just not sure validation on realize was necessary or convenient. It looks like we agree it is just convenient. > > > All that said, I don't have a strong argument against doing it on > > realize, except my gut feeling that this is not how qdev was > > designed[1]. If doing it on realize is the simplest way to do it, I > > won't be the one complaining. > > > > [1] I believe the original intent was to make every single device > > user-creatable and define boards in a declarative way in config > > files. We are very far from that goal. > > I'm fine either way, I just wanted to accommodate Mark's preference, > because he seemed to strongly dislike having to call any helper > functions from board code, beyond initing and realizing the device. > > This is my recollection of the earlier discussion anyway. I think we are in agreement, then. If there are no objections from others against doing object_property_add_child() on realize, I'm also OK with that.
On 23/06/17 19:50, Eduardo Habkost wrote: >> Really, please go back to the earlier discussion around fw_cfg_init1() >> and you'll see my original point (which matches what you just voiced). > > Yep. I was just not sure validation on realize was necessary or > convenient. It looks like we agree it is just convenient. > > >> >>> All that said, I don't have a strong argument against doing it on >>> realize, except my gut feeling that this is not how qdev was >>> designed[1]. If doing it on realize is the simplest way to do it, I >>> won't be the one complaining. >>> >>> [1] I believe the original intent was to make every single device >>> user-creatable and define boards in a declarative way in config >>> files. We are very far from that goal. >> >> I'm fine either way, I just wanted to accommodate Mark's preference, >> because he seemed to strongly dislike having to call any helper >> functions from board code, beyond initing and realizing the device. >> >> This is my recollection of the earlier discussion anyway. > > I think we are in agreement, then. If there are no objections from > others against doing object_property_add_child() on realize, I'm also OK > with that. Just to clarify here that my objection wasn't so much about calling helper functions from board code, it was that as the current patch exposes the MemoryRegions in TYPE_FW_CFG_IO and TYPE_FW_CFG_MEM then as per my example where the machine link is omitted then the check becomes useless. I can see that device_set_realized() will always set the device parent to /machine/unattached before calling the realize function if the device doesn't have a parent. So is it even possible to add the device via object_property_add_child() to the machine during realize? Or could it be done by making /machine/fw_cfg an alias to its real location in the QOM tree at realize time without breaking the object_resolve_path_type() check? The other interesting option to explore is that since fw_cfg already has a machine_ready notifier, the check could be moved there similar to as done in hw/core/machine.c's error_on_sysbus_device() if the check shouldn't be present in realize. That still doesn't answer the question as to how to enforce that the device is correctly linked to the machine though. ATB, Mark.
On Sun, Jun 25, 2017 at 07:58:04PM +0100, Mark Cave-Ayland wrote: > On 23/06/17 19:50, Eduardo Habkost wrote: > > >> Really, please go back to the earlier discussion around fw_cfg_init1() > >> and you'll see my original point (which matches what you just voiced). > > > > Yep. I was just not sure validation on realize was necessary or > > convenient. It looks like we agree it is just convenient. > > > > > >> > >>> All that said, I don't have a strong argument against doing it on > >>> realize, except my gut feeling that this is not how qdev was > >>> designed[1]. If doing it on realize is the simplest way to do it, I > >>> won't be the one complaining. > >>> > >>> [1] I believe the original intent was to make every single device > >>> user-creatable and define boards in a declarative way in config > >>> files. We are very far from that goal. > >> > >> I'm fine either way, I just wanted to accommodate Mark's preference, > >> because he seemed to strongly dislike having to call any helper > >> functions from board code, beyond initing and realizing the device. > >> > >> This is my recollection of the earlier discussion anyway. > > > > I think we are in agreement, then. If there are no objections from > > others against doing object_property_add_child() on realize, I'm also OK > > with that. > > Just to clarify here that my objection wasn't so much about calling > helper functions from board code, it was that as the current patch > exposes the MemoryRegions in TYPE_FW_CFG_IO and TYPE_FW_CFG_MEM then as > per my example where the machine link is omitted then the check becomes > useless. I don't understand this part. When and why would the check become useless? > > I can see that device_set_realized() will always set the device parent > to /machine/unattached before calling the realize function if the device > doesn't have a parent. So is it even possible to add the device via > object_property_add_child() to the machine during realize? Or could it > be done by making /machine/fw_cfg an alias to its real location in the > QOM tree at realize time without breaking the object_resolve_path_type() > check? Well, if we can't do object_property_add_child() on ->instance_init() and doing it on ->realize() would require a more complex solution involving QOM links, I believe the simplest solution is to provide a helper function. > > The other interesting option to explore is that since fw_cfg already has > a machine_ready notifier, the check could be moved there similar to as > done in hw/core/machine.c's error_on_sysbus_device() if the check > shouldn't be present in realize. That still doesn't answer the question > as to how to enforce that the device is correctly linked to the machine > though. I think both manually mapping+linking from board code or calling a helper function from board code would be acceptable.
On 27/06/17 01:49, Eduardo Habkost wrote: > On Sun, Jun 25, 2017 at 07:58:04PM +0100, Mark Cave-Ayland wrote: >> On 23/06/17 19:50, Eduardo Habkost wrote: >> >>>> Really, please go back to the earlier discussion around fw_cfg_init1() >>>> and you'll see my original point (which matches what you just voiced). >>> >>> Yep. I was just not sure validation on realize was necessary or >>> convenient. It looks like we agree it is just convenient. >>> >>> >>>> >>>>> All that said, I don't have a strong argument against doing it on >>>>> realize, except my gut feeling that this is not how qdev was >>>>> designed[1]. If doing it on realize is the simplest way to do it, I >>>>> won't be the one complaining. >>>>> >>>>> [1] I believe the original intent was to make every single device >>>>> user-creatable and define boards in a declarative way in config >>>>> files. We are very far from that goal. >>>> >>>> I'm fine either way, I just wanted to accommodate Mark's preference, >>>> because he seemed to strongly dislike having to call any helper >>>> functions from board code, beyond initing and realizing the device. >>>> >>>> This is my recollection of the earlier discussion anyway. >>> >>> I think we are in agreement, then. If there are no objections from >>> others against doing object_property_add_child() on realize, I'm also OK >>> with that. >> >> Just to clarify here that my objection wasn't so much about calling >> helper functions from board code, it was that as the current patch >> exposes the MemoryRegions in TYPE_FW_CFG_IO and TYPE_FW_CFG_MEM then as >> per my example where the machine link is omitted then the check becomes >> useless. > > I don't understand this part. When and why would the check become > useless? Well because when using the standard QEMU pattern of qdev_create()...qdev_init_nofail() it is possible to realize the device and wire up its MemoryRegions manually as I have done with the sun4u (i.e. it will respond to accesses) multiple times without calling the helper function and triggering the check. The ingenious part here is that only the developers who aren't aware that they have to manually call the helper function will be the ones who get caught out trying to instantiate the device multiple times ;) I'm also aware that with QOM we've got 2 conflicting ideas with fw_cfg: on the one hand we're saying that QOM hierarchy should at some level represent the topology of the machine, i.e. for sun4u the fw_cfg device should appear under the ebus device. whereas at the moment we assume a fixed path of FW_CFG_PATH. Based upon this I been thinking along the following lines: 1) Alter fw_cfg_find() to use object_resolve_path_type("", TYPE_FW_CFG, NULL) This solves the issue of allowing the QOM tree to best represent the device topology as it will allow fw_cfg_find() to locate the fw_cfg device regardless of its location, and hence it can be placed as appropriate for each machine. 2) Add a check at the top of fw_cfg_common_realize() along the lines of: if (object_unattached(OBJECT(dev)) { error_setg(errp, "%s device must be explicitly added as a child " object before realize", TYPE_FW_CFG); return; } This makes it obvious to the developer that they MUST wire up the fw_cfg device to the machine before realize, and then if more than one device is instantiated we can still trigger the existing error from my v7 branch: if (!object_resolve_path_type("", TYPE_FW_CFG, NULL)) { error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG); return; } I prefer this method because it is impossible for a developer to accidentally realize the fw_cfg device without attaching it to the machine first (i.e. fw_cfg_find() will always succeed if this is the case if altered as per 1) above) and it can only be realized once. And following the POLA the device can be wired up using object_property_add_child() as per the numerous existing examples throughout the QEMU codebase. Now the minor issue with 2) is that I can't find an easy way to determine if the device is unattached at realize time. I've tried something like this: if (!object_resolve_path_type("/machine/unattached", TYPE_FW_CFG, NULL)) { ... ... } However that doesn't work because as the rules differ between partial and absolute path resolution, we end up resolving the container itself as opposed to iterating down through the QOM tree. Is there an existing solution for this or would I need to come up with something that iterates over the container children to search for a TYPE_FW_CFG device myself? ATB, Mark.
On Wed, 28 Jun 2017 08:09:35 +0100 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > On 27/06/17 01:49, Eduardo Habkost wrote: > > > On Sun, Jun 25, 2017 at 07:58:04PM +0100, Mark Cave-Ayland wrote: > >> On 23/06/17 19:50, Eduardo Habkost wrote: > >> > >>>> Really, please go back to the earlier discussion around fw_cfg_init1() > >>>> and you'll see my original point (which matches what you just voiced). > >>> > >>> Yep. I was just not sure validation on realize was necessary or > >>> convenient. It looks like we agree it is just convenient. > >>> > >>> > >>>> > >>>>> All that said, I don't have a strong argument against doing it on > >>>>> realize, except my gut feeling that this is not how qdev was > >>>>> designed[1]. If doing it on realize is the simplest way to do it, I > >>>>> won't be the one complaining. > >>>>> > >>>>> [1] I believe the original intent was to make every single device > >>>>> user-creatable and define boards in a declarative way in config > >>>>> files. We are very far from that goal. > >>>> > >>>> I'm fine either way, I just wanted to accommodate Mark's preference, > >>>> because he seemed to strongly dislike having to call any helper > >>>> functions from board code, beyond initing and realizing the device. > >>>> > >>>> This is my recollection of the earlier discussion anyway. > >>> > >>> I think we are in agreement, then. If there are no objections from > >>> others against doing object_property_add_child() on realize, I'm also OK > >>> with that. > >> > >> Just to clarify here that my objection wasn't so much about calling > >> helper functions from board code, it was that as the current patch > >> exposes the MemoryRegions in TYPE_FW_CFG_IO and TYPE_FW_CFG_MEM then as > >> per my example where the machine link is omitted then the check becomes > >> useless. > > > > I don't understand this part. When and why would the check become > > useless? > > Well because when using the standard QEMU pattern of > qdev_create()...qdev_init_nofail() it is possible to realize the device > and wire up its MemoryRegions manually as I have done with the sun4u > (i.e. it will respond to accesses) multiple times without calling the > helper function and triggering the check. The ingenious part here is > that only the developers who aren't aware that they have to manually > call the helper function will be the ones who get caught out trying to > instantiate the device multiple times ;) > > I'm also aware that with QOM we've got 2 conflicting ideas with fw_cfg: > on the one hand we're saying that QOM hierarchy should at some level > represent the topology of the machine, i.e. for sun4u the fw_cfg device > should appear under the ebus device. whereas at the moment we assume a > fixed path of FW_CFG_PATH. > > Based upon this I been thinking along the following lines: > > 1) Alter fw_cfg_find() to use object_resolve_path_type("", TYPE_FW_CFG, > NULL) I'd make use of the 3rd argument &ambiguous and assert on it > This solves the issue of allowing the QOM tree to best represent the > device topology as it will allow fw_cfg_find() to locate the fw_cfg > device regardless of its location, and hence it can be placed as > appropriate for each machine. looks reasonable, that's what we were doing in a bunch of other cases > 2) Add a check at the top of fw_cfg_common_realize() along the lines of: > > if (object_unattached(OBJECT(dev)) { > error_setg(errp, "%s device must be explicitly added as a child " > object before realize", TYPE_FW_CFG); > return; > } that would be never trigger as ancestor's DEVICE.realize() always sets parent if it wasn't set manually before child's realize is called. in other words it's guarantied that device has parent by the time realize() is run by device_set_realized() > This makes it obvious to the developer that they MUST wire up the fw_cfg > device to the machine before realize, and then if more than one device > is instantiated we can still trigger the existing error from my v7 branch: > > if (!object_resolve_path_type("", TYPE_FW_CFG, NULL)) { > error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG); > return; > } reuse fw_cfg_find() here? > I prefer this method because it is impossible for a developer to > accidentally realize the fw_cfg device without attaching it to the > machine first (i.e. fw_cfg_find() will always succeed if this is the > case if altered as per 1) above) and it can only be realized once. And > following the POLA the device can be wired up using > object_property_add_child() as per the numerous existing examples > throughout the QEMU codebase. > > Now the minor issue with 2) is that I can't find an easy way to > determine if the device is unattached at realize time. I've tried > something like this: > > if (!object_resolve_path_type("/machine/unattached", > TYPE_FW_CFG, NULL)) { > ... > ... > } > > However that doesn't work because as the rules differ between partial > and absolute path resolution, we end up resolving the container itself > as opposed to iterating down through the QOM tree. Is there an existing > solution for this or would I need to come up with something that > iterates over the container children to search for a TYPE_FW_CFG device > myself? > > > ATB, > > Mark. > >
On 06/28/17 16:12, Igor Mammedov wrote: > On Wed, 28 Jun 2017 08:09:35 +0100 > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: >> 1) Alter fw_cfg_find() to use object_resolve_path_type("", TYPE_FW_CFG, >> NULL) > I'd make use of the 3rd argument &ambiguous and assert on it I vaguely recall playing with "ambiguous" in find_vmgenid_dev(), but it didn't work as I expected (or, it wasn't necessary to use). Not sure about the details, I only remember that, when calling object_resolve_path_type() like above, from within a realize function, the object under realization is already considered existent, so if we're realizing the second (or later) instance of the class, object_resolve_path_type() will return NULL (regardless of "ambiguous"). If we're realizing the very first instance, then object_resolve_path_type() will return non-NULL. I don't mind "ambiguous" if it can be made work fine, just thought that I'd add this tidbit. Thanks, Laszlo
On Wed, 28 Jun 2017 16:21:40 +0200 Laszlo Ersek <lersek@redhat.com> wrote: > On 06/28/17 16:12, Igor Mammedov wrote: > > On Wed, 28 Jun 2017 08:09:35 +0100 > > Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > >> 1) Alter fw_cfg_find() to use object_resolve_path_type("", TYPE_FW_CFG, > >> NULL) > > I'd make use of the 3rd argument &ambiguous and assert on it > > I vaguely recall playing with "ambiguous" in find_vmgenid_dev(), but it > didn't work as I expected (or, it wasn't necessary to use). Not sure > about the details, I only remember that, when calling > object_resolve_path_type() like above, from within a realize function, > the object under realization is already considered existent, so if we're > realizing the second (or later) instance of the class, > object_resolve_path_type() will return NULL (regardless of "ambiguous"). > If we're realizing the very first instance, then > object_resolve_path_type() will return non-NULL. > > I don't mind "ambiguous" if it can be made work fine, just thought that > I'd add this tidbit. looking at object_resolve_partial_path() if (ambiguous && *ambiguous) { return NULL; } might make object_resolve_partial_path() return non NULL if ambiguous is NULL object_resolve_partial_path(,,, NULL) p0 - p00 - fw0 | | | fw1 | fw2 p00 returns NULL && only fw2 is found without knowing at all about p00 failure > > Thanks, > Laszlo
On 28/06/17 15:12, Igor Mammedov wrote: >>> I don't understand this part. When and why would the check become >>> useless? >> >> Well because when using the standard QEMU pattern of >> qdev_create()...qdev_init_nofail() it is possible to realize the device >> and wire up its MemoryRegions manually as I have done with the sun4u >> (i.e. it will respond to accesses) multiple times without calling the >> helper function and triggering the check. The ingenious part here is >> that only the developers who aren't aware that they have to manually >> call the helper function will be the ones who get caught out trying to >> instantiate the device multiple times ;) >> >> I'm also aware that with QOM we've got 2 conflicting ideas with fw_cfg: >> on the one hand we're saying that QOM hierarchy should at some level >> represent the topology of the machine, i.e. for sun4u the fw_cfg device >> should appear under the ebus device. whereas at the moment we assume a >> fixed path of FW_CFG_PATH. >> >> Based upon this I been thinking along the following lines: >> >> 1) Alter fw_cfg_find() to use object_resolve_path_type("", TYPE_FW_CFG, >> NULL) > I'd make use of the 3rd argument &ambiguous and assert on it I don't think this is relevant here, as one of the aims of the patchset is to ensure that only one fw_cfg device is realized, since as Laszlo points out this is an underlying assumption in the code. >> This solves the issue of allowing the QOM tree to best represent the >> device topology as it will allow fw_cfg_find() to locate the fw_cfg >> device regardless of its location, and hence it can be placed as >> appropriate for each machine. > looks reasonable, that's what we were doing in a bunch of other cases Okay. >> 2) Add a check at the top of fw_cfg_common_realize() along the lines of: >> >> if (object_unattached(OBJECT(dev)) { >> error_setg(errp, "%s device must be explicitly added as a child " >> object before realize", TYPE_FW_CFG); >> return; >> } > that would be never trigger as ancestor's DEVICE.realize() always sets > parent if it wasn't set manually before child's realize is called. > > in other words it's guarantied that device has parent by the time > realize() is run by device_set_realized() Yes I understand that, although I may not have made it clear enough that this is pseudo code (there is no object_unattached() function in QEMU). If you look below you can see that the criteria I am using to distinguish whether a device is a child or not is to see whether it exists underneath /machine/unattached, since as you correctly point out the parent is already set by the time the device is realized. >> This makes it obvious to the developer that they MUST wire up the fw_cfg >> device to the machine before realize, and then if more than one device >> is instantiated we can still trigger the existing error from my v7 branch: >> >> if (!object_resolve_path_type("", TYPE_FW_CFG, NULL)) { >> error_setg(errp, "at most one %s device is permitted", TYPE_FW_CFG); >> return; >> } > reuse fw_cfg_find() here? Yes that is entirely valid - I've made this change locally. >> I prefer this method because it is impossible for a developer to >> accidentally realize the fw_cfg device without attaching it to the >> machine first (i.e. fw_cfg_find() will always succeed if this is the >> case if altered as per 1) above) and it can only be realized once. And >> following the POLA the device can be wired up using >> object_property_add_child() as per the numerous existing examples >> throughout the QEMU codebase. >> >> Now the minor issue with 2) is that I can't find an easy way to >> determine if the device is unattached at realize time. I've tried >> something like this: >> >> if (!object_resolve_path_type("/machine/unattached", >> TYPE_FW_CFG, NULL)) { >> ... >> ... >> } >> >> However that doesn't work because as the rules differ between partial >> and absolute path resolution, we end up resolving the container itself >> as opposed to iterating down through the QOM tree. Is there an existing >> solution for this or would I need to come up with something that >> iterates over the container children to search for a TYPE_FW_CFG device >> myself? Actually the visitor function isn't too complicated at all and I have something working now. Let me tidy up the patchset and if it looks good, I'll re-post it as a v7. ATB, Mark.
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 9b0aaa2..e678ec0 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -862,12 +862,20 @@ static void fw_cfg_machine_ready(struct Notifier *n, void *data) -static void fw_cfg_common_realize(DeviceState *dev) +static void fw_cfg_common_realize(DeviceState *dev, Error **errp) { FWCfgState *s = FW_CFG(dev); MachineState *machine = MACHINE(qdev_get_machine()); uint32_t version = FW_CFG_VERSION; - + Object *obj; + + obj = object_resolve_path(FW_CFG_PATH, NULL); + if (obj != OBJECT(dev)) { + error_setg(errp, "Only one fw_cfg device can be instantiated per " + "machine"); + return; + } + fw_cfg_add_bytes(s, FW_CFG_SIGNATURE, (char *)"QEMU", 4); fw_cfg_add_bytes(s, FW_CFG_UUID, &qemu_uuid, 16); fw_cfg_add_i16(s, FW_CFG_NOGRAPHIC, (uint16_t)!machine->enable_graphics);