| Message ID | 20220205124526.500158-3-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/i386: QOM-attach CPUs/SGX-EPC objects to their parents | expand |
On Sat, 5 Feb 2022 13:45:24 +0100 Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Previously CPUs were exposed in the QOM tree at a path > > /machine/unattached/device[nn] > > where the 'nn' of the first CPU is usually zero, but can > vary depending on what devices were already created. > > With this change the CPUs are now at > > /machine/cpu[nn] > > where the 'nn' of the first CPU is always zero. Could you add to commit message the reason behind the change? > Note: This (intentionally) breaks compatibility with current > libvirt code that looks for "/machine/unattached/device[0]" > in the assumption it is the first CPU. Why libvirt does this in the first place? > Cc: libvir-list@redhat.com > Suggested-by: Daniel P. Berrangé <berrange@redhat.com> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/i386/x86.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > index b84840a1bb9..50bf249c700 100644 > --- a/hw/i386/x86.c > +++ b/hw/i386/x86.c > @@ -108,6 +108,7 @@ void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp) > { > Object *cpu = object_new(MACHINE(x86ms)->cpu_type); > > + object_property_add_child(OBJECT(x86ms), "cpu[*]", OBJECT(cpu)); that will take in account only initial cpus, -device/device_add cpus will still go to wherever device_add attaches them (see qdev_set_id) > if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) { > goto out; > }
On Mon, 7 Feb 2022 09:14:37 +0100 Igor Mammedov <imammedo@redhat.com> wrote: > On Sat, 5 Feb 2022 13:45:24 +0100 > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > Previously CPUs were exposed in the QOM tree at a path > > > > /machine/unattached/device[nn] > > > > where the 'nn' of the first CPU is usually zero, but can > > vary depending on what devices were already created. > > > > With this change the CPUs are now at > > > > /machine/cpu[nn] > > > > where the 'nn' of the first CPU is always zero. > > Could you add to commit message the reason behind the change? regardless, it looks like unwarranted movement to me prompted by livirt accessing/expecting a QOM patch which is not stable ABI. I'd rather get it fixed on libvirt side. If libvirt needs for some reason access a CPU instance, it should use @query-hotpluggable-cpus to get a list of CPUs (which includes QOM path of already present CPUs) instead of hard-codding some 'well-known' path as there is no any guarantee that it will stay stable whatsoever. > > Note: This (intentionally) breaks compatibility with current > > libvirt code that looks for "/machine/unattached/device[0]" > > in the assumption it is the first CPU. > Why libvirt does this in the first place? > > > > Cc: libvir-list@redhat.com > > Suggested-by: Daniel P. Berrangé <berrange@redhat.com> > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > --- > > hw/i386/x86.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > > index b84840a1bb9..50bf249c700 100644 > > --- a/hw/i386/x86.c > > +++ b/hw/i386/x86.c > > @@ -108,6 +108,7 @@ void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp) > > { > > Object *cpu = object_new(MACHINE(x86ms)->cpu_type); > > > > + object_property_add_child(OBJECT(x86ms), "cpu[*]", OBJECT(cpu)); > > that will take in account only initial cpus, -device/device_add cpus > will still go to wherever device_add attaches them (see qdev_set_id) > > > if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) { > > goto out; > > } >
On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote: > On Mon, 7 Feb 2022 09:14:37 +0100 > Igor Mammedov <imammedo@redhat.com> wrote: > > > On Sat, 5 Feb 2022 13:45:24 +0100 > > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > > > Previously CPUs were exposed in the QOM tree at a path > > > > > > /machine/unattached/device[nn] > > > > > > where the 'nn' of the first CPU is usually zero, but can > > > vary depending on what devices were already created. > > > > > > With this change the CPUs are now at > > > > > > /machine/cpu[nn] > > > > > > where the 'nn' of the first CPU is always zero. > > > > Could you add to commit message the reason behind the change? > > regardless, it looks like unwarranted movement to me > prompted by livirt accessing/expecting a QOM patch which is > not stable ABI. I'd rather get it fixed on libvirt side. > > If libvirt needs for some reason access a CPU instance, > it should use @query-hotpluggable-cpus to get a list of CPUs > (which includes QOM path of already present CPUs) instead of > hard-codding some 'well-known' path as there is no any guarantee > that it will stay stable whatsoever. I don't disagree with you about the use of hardcoded path, but the way of using @query-hotpluggable-cpus is not really aligning well for how it's being used. To shed a bit more light, libvirt uses the following hardcoded path #define QOM_CPU_PATH "/machine/unattached/device[0]" in code which is used to query CPU flags. That code doesn't care at all which cpus are present but wants to get any of them. So yes, calling query-hotpluggable-cpus is possible but a bit pointless. In general the code probing cpu flags via qom-get is very cumbersome as it ends up doing ~400 QMP calls at startup of a VM in cases when we deem it necessary to probe the cpu fully. It would be much better (And would sidestep the issue altoghether) if we had a more sane interface to probe all cpu flags in one go, and ideally the argument specifying the cpu being optional. Libvirt can do the adjustment, but for now IMO the path to the first cpu (/machine/unattached/device[0]) became de-facto ABI by the virtue that it was used by libvirt and if I remember correctly it was suggested by the folks dealing with the CPU when the code was added originally. Even if we change it in libvirt right away, changing qemu will break forward compatibility. While we don't guarantee it, it still creates user grief.
On Mon, Feb 07, 2022 at 10:36:42 +0100, Peter Krempa wrote: > On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote: > > On Mon, 7 Feb 2022 09:14:37 +0100 > > Igor Mammedov <imammedo@redhat.com> wrote: [...] > Even if we change it in libvirt right away, changing qemu will break > forward compatibility. While we don't guarantee it, it still creates > user grief. I've filed an upstream issue: https://gitlab.com/libvirt/libvirt/-/issues/272
On Mon, Feb 07, 2022 at 10:36:42AM +0100, Peter Krempa wrote: > On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote: > > On Mon, 7 Feb 2022 09:14:37 +0100 > > Igor Mammedov <imammedo@redhat.com> wrote: > > > > > On Sat, 5 Feb 2022 13:45:24 +0100 > > > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > > > > > Previously CPUs were exposed in the QOM tree at a path > > > > > > > > /machine/unattached/device[nn] > > > > > > > > where the 'nn' of the first CPU is usually zero, but can > > > > vary depending on what devices were already created. > > > > > > > > With this change the CPUs are now at > > > > > > > > /machine/cpu[nn] > > > > > > > > where the 'nn' of the first CPU is always zero. > > > > > > Could you add to commit message the reason behind the change? > > > > regardless, it looks like unwarranted movement to me > > prompted by livirt accessing/expecting a QOM patch which is > > not stable ABI. I'd rather get it fixed on libvirt side. > > > > If libvirt needs for some reason access a CPU instance, > > it should use @query-hotpluggable-cpus to get a list of CPUs > > (which includes QOM path of already present CPUs) instead of > > hard-codding some 'well-known' path as there is no any guarantee > > that it will stay stable whatsoever. > > I don't disagree with you about the use of hardcoded path, but the way > of using @query-hotpluggable-cpus is not really aligning well for how > it's being used. > > To shed a bit more light, libvirt uses the following hardcoded path > > #define QOM_CPU_PATH "/machine/unattached/device[0]" > > in code which is used to query CPU flags. That code doesn't care at all > which cpus are present but wants to get any of them. So yes, calling > query-hotpluggable-cpus is possible but a bit pointless. > > In general the code probing cpu flags via qom-get is very cumbersome as > it ends up doing ~400 QMP calls at startup of a VM in cases when we deem > it necessary to probe the cpu fully. Yes, that's one QMP call per CPUID feature bit that QEMU knows about. It is a massive performance bottleneck that we need a much better solution for. We really should have raised this with QEMU right away when we found we had this need for 100's of QMP commands. Regards, Daniel
On Mon, Feb 7, 2022 at 3:12 PM Peter Krempa <pkrempa@redhat.com> wrote: > > On Mon, Feb 07, 2022 at 10:36:42 +0100, Peter Krempa wrote: > > On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote: > > > On Mon, 7 Feb 2022 09:14:37 +0100 > > > Igor Mammedov <imammedo@redhat.com> wrote: > > [...] > > > Even if we change it in libvirt right away, changing qemu will break > > forward compatibility. While we don't guarantee it, it still creates > > user grief. > > I've filed an upstream issue: > > https://gitlab.com/libvirt/libvirt/-/issues/272 I can look into this bug. Feel free to assign it to me.
On Mon, 7 Feb 2022 10:36:42 +0100 Peter Krempa <pkrempa@redhat.com> wrote: > On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote: > > On Mon, 7 Feb 2022 09:14:37 +0100 > > Igor Mammedov <imammedo@redhat.com> wrote: > > > > > On Sat, 5 Feb 2022 13:45:24 +0100 > > > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > > > > > Previously CPUs were exposed in the QOM tree at a path > > > > > > > > /machine/unattached/device[nn] > > > > > > > > where the 'nn' of the first CPU is usually zero, but can > > > > vary depending on what devices were already created. > > > > > > > > With this change the CPUs are now at > > > > > > > > /machine/cpu[nn] > > > > > > > > where the 'nn' of the first CPU is always zero. > > > > > > Could you add to commit message the reason behind the change? > > > > regardless, it looks like unwarranted movement to me > > prompted by livirt accessing/expecting a QOM patch which is > > not stable ABI. I'd rather get it fixed on libvirt side. > > > > If libvirt needs for some reason access a CPU instance, > > it should use @query-hotpluggable-cpus to get a list of CPUs > > (which includes QOM path of already present CPUs) instead of > > hard-codding some 'well-known' path as there is no any guarantee > > that it will stay stable whatsoever. > > I don't disagree with you about the use of hardcoded path, but the way > of using @query-hotpluggable-cpus is not really aligning well for how > it's being used. > > To shed a bit more light, libvirt uses the following hardcoded path > > #define QOM_CPU_PATH "/machine/unattached/device[0]" > > in code which is used to query CPU flags. That code doesn't care at all > which cpus are present but wants to get any of them. So yes, calling > query-hotpluggable-cpus is possible but a bit pointless. Even though query-hotpluggable-cpus is cumbersome it still lets you avoid hardcodding QOM path and let you get away with keeping "_400 QMP calls" probing while something better comes along. > In general the code probing cpu flags via qom-get is very cumbersome as > it ends up doing ~400 QMP calls at startup of a VM in cases when we deem > it necessary to probe the cpu fully. > > It would be much better (And would sidestep the issue altoghether) if we > had a more sane interface to probe all cpu flags in one go, and ideally > the argument specifying the cpu being optional. > > Libvirt can do the adjustment, but for now IMO the path to the first cpu > (/machine/unattached/device[0]) became de-facto ABI by the virtue that > it was used by libvirt and if I remember correctly it was suggested by > the folks dealing with the CPU when the code was added originally. I would've argued against that back then as well, there weren't any guarantee and I wouldn't like precedent of QOM abuse becoming de-facto ABI. Note: this patch breaks this so called ABI as well and introduces yet another harcodded path without any stability guarantee whatsoever. > > Even if we change it in libvirt right away, changing qemu will break > forward compatibility. While we don't guarantee it, it still creates > user grief. >
On Mon, Feb 07, 2022 at 16:50:28 +0530, Ani Sinha wrote: > On Mon, Feb 7, 2022 at 3:12 PM Peter Krempa <pkrempa@redhat.com> wrote: > > > > On Mon, Feb 07, 2022 at 10:36:42 +0100, Peter Krempa wrote: > > > On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote: > > > > On Mon, 7 Feb 2022 09:14:37 +0100 > > > > Igor Mammedov <imammedo@redhat.com> wrote: > > > > [...] > > > > > Even if we change it in libvirt right away, changing qemu will break > > > forward compatibility. While we don't guarantee it, it still creates > > > user grief. > > > > I've filed an upstream issue: > > > > https://gitlab.com/libvirt/libvirt/-/issues/272 > > I can look into this bug. Feel free to assign it to me. No need to. I've noticed that we already call query-hotpluggable-cpus so with a simple modification the VM startup code path can be easily fixed so I've done so.
On Mon, Feb 7, 2022 at 4:58 PM Peter Krempa <pkrempa@redhat.com> wrote: > > On Mon, Feb 07, 2022 at 16:50:28 +0530, Ani Sinha wrote: > > On Mon, Feb 7, 2022 at 3:12 PM Peter Krempa <pkrempa@redhat.com> wrote: > > > > > > On Mon, Feb 07, 2022 at 10:36:42 +0100, Peter Krempa wrote: > > > > On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote: > > > > > On Mon, 7 Feb 2022 09:14:37 +0100 > > > > > Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > [...] > > > > > > > Even if we change it in libvirt right away, changing qemu will break > > > > forward compatibility. While we don't guarantee it, it still creates > > > > user grief. > > > > > > I've filed an upstream issue: > > > > > > https://gitlab.com/libvirt/libvirt/-/issues/272 > > > > I can look into this bug. Feel free to assign it to me. > > No need to. I've noticed that we already call query-hotpluggable-cpus > so with a simple modification the VM startup code path can be easily > fixed so I've done so. Ok I will look for your patch in the mailing list and review them. >
On Mon, Feb 07, 2022 at 12:22:22PM +0100, Igor Mammedov wrote: > On Mon, 7 Feb 2022 10:36:42 +0100 > Peter Krempa <pkrempa@redhat.com> wrote: > > > On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote: > > > On Mon, 7 Feb 2022 09:14:37 +0100 > > > Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > On Sat, 5 Feb 2022 13:45:24 +0100 > > > > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > > > > > > > Previously CPUs were exposed in the QOM tree at a path > > > > > > > > > > /machine/unattached/device[nn] > > > > > > > > > > where the 'nn' of the first CPU is usually zero, but can > > > > > vary depending on what devices were already created. > > > > > > > > > > With this change the CPUs are now at > > > > > > > > > > /machine/cpu[nn] > > > > > > > > > > where the 'nn' of the first CPU is always zero. > > > > > > > > Could you add to commit message the reason behind the change? > > > > > > regardless, it looks like unwarranted movement to me > > > prompted by livirt accessing/expecting a QOM patch which is > > > not stable ABI. I'd rather get it fixed on libvirt side. > > > > > > If libvirt needs for some reason access a CPU instance, > > > it should use @query-hotpluggable-cpus to get a list of CPUs > > > (which includes QOM path of already present CPUs) instead of > > > hard-codding some 'well-known' path as there is no any guarantee > > > that it will stay stable whatsoever. > > > > I don't disagree with you about the use of hardcoded path, but the way > > of using @query-hotpluggable-cpus is not really aligning well for how > > it's being used. > > > > To shed a bit more light, libvirt uses the following hardcoded path > > > > #define QOM_CPU_PATH "/machine/unattached/device[0]" > > > > in code which is used to query CPU flags. That code doesn't care at all > > which cpus are present but wants to get any of them. So yes, calling > > query-hotpluggable-cpus is possible but a bit pointless. > > Even though query-hotpluggable-cpus is cumbersome > it still lets you avoid hardcodding QOM path and let you > get away with keeping "_400 QMP calls" probing while > something better comes along. > > > > In general the code probing cpu flags via qom-get is very cumbersome as > > it ends up doing ~400 QMP calls at startup of a VM in cases when we deem > > it necessary to probe the cpu fully. > > > > It would be much better (And would sidestep the issue altoghether) if we > > had a more sane interface to probe all cpu flags in one go, and ideally > > the argument specifying the cpu being optional. > > > > Libvirt can do the adjustment, but for now IMO the path to the first cpu > > (/machine/unattached/device[0]) became de-facto ABI by the virtue that > > it was used by libvirt and if I remember correctly it was suggested by > > the folks dealing with the CPU when the code was added originally. > I would've argued against that back then as well, > there weren't any guarantee and I wouldn't like precedent of > QOM abuse becoming de-facto ABI. > Note: this patch breaks this so called ABI as well and introduces > yet another harcodded path without any stability guarantee whatsoever. AFAIK, we've never defined anything about QOM paths wrt ABI one way or the other ? In the absence of guidelines then it comes down to what are reasonable expectations of the mgmt app. These expectations will be influenced by what it is actually possible to acheive given our API as exposed. I think it is unreasonable to expect /machine/unattached to be stable because by its very nature it is just a dumping ground for anything where the dev hasn't put in any thought to the path placement. IOW, it was/is definitely a bad idea for libvirt to rely on /machine/unattached in any way. That we're liable to be broken is not nice, but its really our own fault - we should have asked for a better solution from day one here. I think it is somewhat reasonable to expect other QOM paths to be stable as there's been some degree of thought put into their placement. If we don't want apps to be considering other paths to be stable, then we need to explain exactly what they can and can't rely on, and most importantly actually provide a way for them to do what they need For example, libvirt needs a QOM path to query memory balloon stats. All libvirt knows is that it set 'id=balloon0' when creating it on the CLI. To find the balloon device in QOM it then looks for all paths under '/machine/peripheral', and tries to find one called 'child<$ID>' where $ID is the id=xxx value from the CLI. We do the same dance when we need to find out where thue default VGA device we created lives. This all feels kinda silly as we're going through a dance to dynamically find the device, but in practice it is no better than just hardcoding it. The problem we face in these examp\les is that as an input we are giving QMEU a device 'id' but at runtime we're needing to then use a QOM path instead of the 'id'. So we need a way to translate an 'id' to a QOM path. What is the right way to do this in a supported manner without making any assumptions about QOM paths ? For the CPUs cases, we don't have any 'id' on the CLI since CPUs aren't configured that way, so we just hardcoded the full path. You've pointed out query-hotpluggable-cpus which is a possible solution. In another case we're assuming that '/machine' has a property called 'rtc-time'. IMHO it is reasonable to assume that '/machine' as a QOM path is stable. It isn't as simple as just saying "all QOM paths are unstable". I struggle to come up with a good rule to explain what apps can / can't rely on wrt QOM paths, other than stay far away from anything with '/unattached'. Regards, Daniel
On Mon, 7 Feb 2022 11:48:27 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote: > On Mon, Feb 07, 2022 at 12:22:22PM +0100, Igor Mammedov wrote: > > On Mon, 7 Feb 2022 10:36:42 +0100 > > Peter Krempa <pkrempa@redhat.com> wrote: > > > > > On Mon, Feb 07, 2022 at 10:18:43 +0100, Igor Mammedov wrote: > > > > On Mon, 7 Feb 2022 09:14:37 +0100 > > > > Igor Mammedov <imammedo@redhat.com> wrote: > > > > > > > > > On Sat, 5 Feb 2022 13:45:24 +0100 > > > > > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > > > > > > > > > > Previously CPUs were exposed in the QOM tree at a path > > > > > > > > > > > > /machine/unattached/device[nn] > > > > > > > > > > > > where the 'nn' of the first CPU is usually zero, but can > > > > > > vary depending on what devices were already created. > > > > > > > > > > > > With this change the CPUs are now at > > > > > > > > > > > > /machine/cpu[nn] > > > > > > > > > > > > where the 'nn' of the first CPU is always zero. > > > > > > > > > > Could you add to commit message the reason behind the change? > > > > > > > > regardless, it looks like unwarranted movement to me > > > > prompted by livirt accessing/expecting a QOM patch which is > > > > not stable ABI. I'd rather get it fixed on libvirt side. > > > > > > > > If libvirt needs for some reason access a CPU instance, > > > > it should use @query-hotpluggable-cpus to get a list of CPUs > > > > (which includes QOM path of already present CPUs) instead of > > > > hard-codding some 'well-known' path as there is no any guarantee > > > > that it will stay stable whatsoever. > > > > > > I don't disagree with you about the use of hardcoded path, but the way > > > of using @query-hotpluggable-cpus is not really aligning well for how > > > it's being used. > > > > > > To shed a bit more light, libvirt uses the following hardcoded path > > > > > > #define QOM_CPU_PATH "/machine/unattached/device[0]" > > > > > > in code which is used to query CPU flags. That code doesn't care at all > > > which cpus are present but wants to get any of them. So yes, calling > > > query-hotpluggable-cpus is possible but a bit pointless. > > > > Even though query-hotpluggable-cpus is cumbersome > > it still lets you avoid hardcodding QOM path and let you > > get away with keeping "_400 QMP calls" probing while > > something better comes along. > > > > > > > In general the code probing cpu flags via qom-get is very cumbersome as > > > it ends up doing ~400 QMP calls at startup of a VM in cases when we deem > > > it necessary to probe the cpu fully. > > > > > > It would be much better (And would sidestep the issue altoghether) if we > > > had a more sane interface to probe all cpu flags in one go, and ideally > > > the argument specifying the cpu being optional. > > > > > > Libvirt can do the adjustment, but for now IMO the path to the first cpu > > > (/machine/unattached/device[0]) became de-facto ABI by the virtue that > > > it was used by libvirt and if I remember correctly it was suggested by > > > the folks dealing with the CPU when the code was added originally. > > I would've argued against that back then as well, > > there weren't any guarantee and I wouldn't like precedent of > > QOM abuse becoming de-facto ABI. > > Note: this patch breaks this so called ABI as well and introduces > > yet another harcodded path without any stability guarantee whatsoever. > > AFAIK, we've never defined anything about QOM paths wrt ABI one way > or the other ? In the absence of guidelines then it comes down to not written in docs anyways (all I have is vague recollection that we really didn't want to make of QOM path/tree an ABI). For more on this topic see the comment at the end. > what are reasonable expectations of the mgmt app. These expectations > will be influenced by what it is actually possible to acheive given > our API as exposed. > > I think it is unreasonable to expect /machine/unattached to be > stable because by its very nature it is just a dumping ground > for anything where the dev hasn't put in any thought to the path > placement. IOW, it was/is definitely a bad idea for libvirt to > rely on /machine/unattached in any way. That we're liable to be > broken is not nice, but its really our own fault - we should > have asked for a better solution from day one here. > > > I think it is somewhat reasonable to expect other QOM paths to > be stable as there's been some degree of thought put into their > placement. If we don't want apps to be considering other > paths to be stable, then we need to explain exactly what they > can and can't rely on, and most importantly actually provide > a way for them to do what they need > > > For example, libvirt needs a QOM path to query memory balloon > stats. All libvirt knows is that it set 'id=balloon0' when > creating it on the CLI. To find the balloon device in QOM it > then looks for all paths under '/machine/peripheral', and > tries to find one called 'child<$ID>' where $ID is the id=xxx > value from the CLI. > > We do the same dance when we need to find out where thue > default VGA device we created lives. > > This all feels kinda silly as we're going through a dance to > dynamically find the device, but in practice it is no better > than just hardcoding it. > > The problem we face in these examp\les is that as an input we > are giving QMEU a device 'id' but at runtime we're needing to > then use a QOM path instead of the 'id'. So we need a way to > translate an 'id' to a QOM path. What is the right way to do > this in a supported manner without making any assumptions > about QOM paths ? Maybe we should have a QMP command that could find object instance by ID? > For the CPUs cases, we don't have any 'id' on the CLI since > CPUs aren't configured that way, so we just hardcoded the > full path. You've pointed out query-hotpluggable-cpus which > is a possible solution. > > In another case we're assuming that '/machine' has a property > called 'rtc-time'. IMHO it is reasonable to assume that > '/machine' as a QOM path is stable. /machine probably will stay stable, but I wouldn't bet any money on that either. But, don't we have a QMP interface to query machine properties? (if there were a need with a good for QEMU justification to change /machine, it most likely would've been merged as long as it doesn't break QEMU itself, same goes for other QOM paths) > It isn't as simple as just saying "all QOM paths are unstable". > > I struggle to come up with a good rule to explain what apps can > / can't rely on wrt QOM paths, other than stay far away from > anything with '/unattached'. If I recall right, the main reason against declaring QOM path as stable ABI, was maintenance headache QEMU side would get with that as we wanted to have an ability to change tree freely without breaking external users. My fear is that making QOM path a stable ABI, will eventually dig us trap similar to CLI interface, only more deeply embedded into the code. What this series does is trying to introduce an alternative management API on top of exiting QMP one (willfully changing child parent relations), that's fine if that's a way to go forward, but it should be thought over and documented before we go this route. I always assumed that if a stable interface was necessary for external users, the QMP was the way to go. (it's easy to (ab)use direct QMP access, as usually it gives one the access to the feature since the day it was merged, but then it backfires, when QEMU changes unexpectedly, since no one has been any paying attention to path stability nor adds any compat knobs when it happens) Anyways, we should discuss QOM path ABI topic again and makeup our mind on this (and preferably documenting it somewhere) to avoid wildly different assumptions about it. > Regards, > Daniel
On Mon, 7 Feb 2022 at 11:56, Daniel P. Berrangé <berrange@redhat.com> wrote: > AFAIK, we've never defined anything about QOM paths wrt ABI one way > or the other ? In the absence of guidelines then it comes down to > what are reasonable expectations of the mgmt app. These expectations > will be influenced by what it is actually possible to acheive given > our API as exposed. > > I think it is unreasonable to expect /machine/unattached to be > stable because by its very nature it is just a dumping ground > for anything where the dev hasn't put in any thought to the path > placement. IOW, it was/is definitely a bad idea for libvirt to > rely on /machine/unattached in any way. That we're liable to be > broken is not nice, but its really our own fault - we should > have asked for a better solution from day one here. > > > I think it is somewhat reasonable to expect other QOM paths to > be stable as there's been some degree of thought put into their > placement. If we don't want apps to be considering other > paths to be stable, then we need to explain exactly what they > can and can't rely on, and most importantly actually provide > a way for them to do what they need I wouldn't personally expect other QOM paths to be stable except in the sense of "probably don't change very often". There are internal-to-QEMU code refactorings and rearrangements that will change QOM paths, and there is no clear "warning, don't change this stuff" to point people away from making that kind of code change, nor are there tests in the test suite that will catch accidental changes. -- PMM
diff --git a/hw/i386/x86.c b/hw/i386/x86.c index b84840a1bb9..50bf249c700 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -108,6 +108,7 @@ void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp) { Object *cpu = object_new(MACHINE(x86ms)->cpu_type); + object_property_add_child(OBJECT(x86ms), "cpu[*]", OBJECT(cpu)); if (!object_property_set_uint(cpu, "apic-id", apic_id, errp)) { goto out; }