[PATCH for-6.0] accel: Wire accel to /machine

[PATCH for-6.0] accel: Wire accel to /machine

[Roman Bolshakov]

There's no generic way to query current accel and its properties via QOM
because there's no link between an accel and current machine.

The change adds the link, i.e. if HVF is enabled the following will be
available in QOM:

  (qemu) qom-get /machine/accel type
  "hvf-accel"

Suggested-by: Markus Armbruster <armbru@redhat.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---

Hi,

this is a follow up patch that deprecates earlier series [1].

An outstanding issue is whether management applications can rely on the
value of /machine/accel/type and output of qom-list-types command [2][3]
to get current and present accels?

i.e. would it be ok if libvirt assumes that everything up to the first
dash in the accel type is the name of the accel (as specified via -M
accel=ACCEL flag) when it performs QEMU probing?

Also, Eduardo and Claudio earlier had ideas to provide cpu-specific
accel subclasses [4][5].

1. https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg03944.html
2. https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg04212.html
3. https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg07062.html
4. https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg06513.html
5. https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg06741.html

Thanks,
Roman

 accel/accel.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/accel/accel.c b/accel/accel.c
index cb555e3b06..45c5bf87b1 100644
--- a/accel/accel.c
+++ b/accel/accel.c
@@ -56,10 +56,11 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
     if (ret < 0) {
         ms->accelerator = NULL;
         *(acc->allowed) = false;
-        object_unref(OBJECT(accel));
     } else {
         object_set_accelerator_compat_props(acc->compat_props);
+        object_property_add_child(OBJECT(ms), "accel", OBJECT(accel));
     }
+    object_unref(OBJECT(accel));
     return ret;
 }
 
-- 
2.29.2

Re: [PATCH for-6.0] accel: Wire accel to /machine

[Eduardo Habkost]

On Mon, Dec 07, 2020 at 11:46:22AM +0300, Roman Bolshakov wrote:
> There's no generic way to query current accel and its properties via QOM
> because there's no link between an accel and current machine.
> 
> The change adds the link, i.e. if HVF is enabled the following will be
> available in QOM:
> 
>   (qemu) qom-get /machine/accel type
>   "hvf-accel"
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
> 
> Hi,
> 
> this is a follow up patch that deprecates earlier series [1].
> 

Is there a reference to the reasoning for dropping the earlier
approach?  Your previous approach seems preferable.

> An outstanding issue is whether management applications can rely on the
> value of /machine/accel/type and output of qom-list-types command [2][3]
> to get current and present accels?
> 
> i.e. would it be ok if libvirt assumes that everything up to the first
> dash in the accel type is the name of the accel (as specified via -M
> accel=ACCEL flag) when it performs QEMU probing?

There are two big assumption libvirt would need to make:
1) That /machine/accel is a stable path that will never change;
2) That the accel name => QOM type naming rules will never change.

Item #1 is unlikely to change, but having the freedom to change
#2 would be useful for future refactoring (like the idea
mentioned at [4]).

The main issue, however, is having those assumptions not being
documented anywhere.  A documented QMP interface is better than a
undocumented `qom-get`-based interface.

                            * * *

This patch is still a good idea, though.  Even if it is just for
debugging or for having clearer object ownership rules, having
the accel object as part of the QOM tree is useful.  So this has
my:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

but we need a commit message that doesn't make people think the
`qom-get` command above will always work.

> 
> Also, Eduardo and Claudio earlier had ideas to provide cpu-specific
> accel subclasses [4][5].
> 
> 1. https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg03944.html
> 2. https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg04212.html
> 3. https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg07062.html
> 4. https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg06513.html
> 5. https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg06741.html
> 
> Thanks,
> Roman
> 
>  accel/accel.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/accel/accel.c b/accel/accel.c
> index cb555e3b06..45c5bf87b1 100644
> --- a/accel/accel.c
> +++ b/accel/accel.c
> @@ -56,10 +56,11 @@ int accel_init_machine(AccelState *accel, MachineState *ms)
>      if (ret < 0) {
>          ms->accelerator = NULL;
>          *(acc->allowed) = false;
> -        object_unref(OBJECT(accel));
>      } else {
>          object_set_accelerator_compat_props(acc->compat_props);
> +        object_property_add_child(OBJECT(ms), "accel", OBJECT(accel));
>      }
> +    object_unref(OBJECT(accel));
>      return ret;
>  }
>  
> -- 
> 2.29.2
> 

-- 
Eduardo

Re: [PATCH for-6.0] accel: Wire accel to /machine

[Peter Krempa]

On Mon, Dec 07, 2020 at 12:38:49 -0500, Eduardo Habkost wrote:
> On Mon, Dec 07, 2020 at 11:46:22AM +0300, Roman Bolshakov wrote:
> > There's no generic way to query current accel and its properties via QOM
> > because there's no link between an accel and current machine.
> > 
> > The change adds the link, i.e. if HVF is enabled the following will be
> > available in QOM:
> > 
> >   (qemu) qom-get /machine/accel type
> >   "hvf-accel"
> > 
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > ---
> > 
> > Hi,
> > 
> > this is a follow up patch that deprecates earlier series [1].
> > 
> 
> Is there a reference to the reasoning for dropping the earlier
> approach?  Your previous approach seems preferable.

The gist of the discussion before was that deprecating old commands in
the same release cycle as introducing the replacement might be
problematic if libvirt wants to adapt ASAP and that the new command
should be elevated to a intermediate tier of stability, where ACK from
libvirt is needed to change it during the same release cycle
incompatibly.

That was meant generally for any command, and was started because we had
a similar issue recently.

My intention definitely was not to change the patch itself, but more a
process change so that we can keep cooperating on new stuff rapidly, but
without actually breaking what we do.

Re: [PATCH for-6.0] accel: Wire accel to /machine

[Roman Bolshakov]

On Mon, Dec 07, 2020 at 06:50:07PM +0100, Peter Krempa wrote:
> On Mon, Dec 07, 2020 at 12:38:49 -0500, Eduardo Habkost wrote:
> > On Mon, Dec 07, 2020 at 11:46:22AM +0300, Roman Bolshakov wrote:
> > > There's no generic way to query current accel and its properties via QOM
> > > because there's no link between an accel and current machine.
> > > 
> > > The change adds the link, i.e. if HVF is enabled the following will be
> > > available in QOM:
> > > 
> > >   (qemu) qom-get /machine/accel type
> > >   "hvf-accel"
> > > 
> > > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > > ---
> > > 
> > > Hi,
> > > 
> > > this is a follow up patch that deprecates earlier series [1].
> > > 
> > 
> > Is there a reference to the reasoning for dropping the earlier
> > approach?  Your previous approach seems preferable.
> 
> The gist of the discussion before was that deprecating old commands in
> the same release cycle as introducing the replacement might be
> problematic if libvirt wants to adapt ASAP and that the new command
> should be elevated to a intermediate tier of stability, where ACK from
> libvirt is needed to change it during the same release cycle
> incompatibly.
> 
> That was meant generally for any command, and was started because we had
> a similar issue recently.
> 
> My intention definitely was not to change the patch itself, but more a
> process change so that we can keep cooperating on new stuff rapidly, but
> without actually breaking what we do.
> 

Thanks Peter,

I'll drop deprecation patch in v2 of query-accel QMP command.

Ragards,
Roman

Re: [PATCH for-6.0] accel: Wire accel to /machine

[Peter Krempa]

On Tue, Dec 08, 2020 at 11:13:07 +0300, Roman Bolshakov wrote:
> On Mon, Dec 07, 2020 at 06:50:07PM +0100, Peter Krempa wrote:
> > On Mon, Dec 07, 2020 at 12:38:49 -0500, Eduardo Habkost wrote:
> > > On Mon, Dec 07, 2020 at 11:46:22AM +0300, Roman Bolshakov wrote:
> > > > There's no generic way to query current accel and its properties via QOM
> > > > because there's no link between an accel and current machine.
> > > > 
> > > > The change adds the link, i.e. if HVF is enabled the following will be
> > > > available in QOM:
> > > > 
> > > >   (qemu) qom-get /machine/accel type
> > > >   "hvf-accel"
> > > > 
> > > > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > > > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > > > ---
> > > > 
> > > > Hi,
> > > > 
> > > > this is a follow up patch that deprecates earlier series [1].
> > > > 
> > > 
> > > Is there a reference to the reasoning for dropping the earlier
> > > approach?  Your previous approach seems preferable.
> > 
> > The gist of the discussion before was that deprecating old commands in
> > the same release cycle as introducing the replacement might be
> > problematic if libvirt wants to adapt ASAP and that the new command
> > should be elevated to a intermediate tier of stability, where ACK from
> > libvirt is needed to change it during the same release cycle
> > incompatibly.
> > 
> > That was meant generally for any command, and was started because we had
> > a similar issue recently.
> > 
> > My intention definitely was not to change the patch itself, but more a
> > process change so that we can keep cooperating on new stuff rapidly, but
> > without actually breaking what we do.
> > 
> 
> Thanks Peter,
> 
> I'll drop deprecation patch in v2 of query-accel QMP command.

Actually In the discussion my stance is that you can deprecate the
old command itself, but starting from that point any semantic changes to
query-accel must be consulted with libvirt as if query-accel was already
released.

We do want to develop the use of the new command as soon as possible,
because that's beneficial to both sides and can actually show a design
problem in the replacement command, so having us replace it before qemu
releases it is good.

On the other hand once libvirt takes the replacement, we must be
involved in any changes to the command due to de-synced release
shchedules so that there isn't a libvirt which e.g. didnt work.

I specifically don't want to derail any of this collaboration, I just
want to enhance it by all parties knowingly agreeing to the "gentleman's
agreement" since complicating the process would actually be detremental.

The thing is that the command may be changed if we know about it and
e.g. didn't yet commit the replacement. We just need to communicate.

Re: [PATCH for-6.0] accel: Wire accel to /machine

[Roman Bolshakov]

On Mon, Dec 07, 2020 at 12:38:49PM -0500, Eduardo Habkost wrote:
> On Mon, Dec 07, 2020 at 11:46:22AM +0300, Roman Bolshakov wrote:
> > There's no generic way to query current accel and its properties via QOM
> > because there's no link between an accel and current machine.
> > 
> > The change adds the link, i.e. if HVF is enabled the following will be
> > available in QOM:
> > 
> >   (qemu) qom-get /machine/accel type
> >   "hvf-accel"
> > 
> > Suggested-by: Markus Armbruster <armbru@redhat.com>
> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> > ---
> > 
> > Hi,
> > 
> > this is a follow up patch that deprecates earlier series [1].
> > 
> 
> Is there a reference to the reasoning for dropping the earlier
> approach?  Your previous approach seems preferable.
> 

Okay I wasn't sure about that :) It's clear now from your and Daniel's
response that a documented QMP method is preferable for public API.

> but we need a commit message that doesn't make people think the
> `qom-get` command above will always work.
> 

How about the one below:

accel: Wire accel to /machine

An accel is a property of a machine but it's not reflected in QOM. An
ownership between machine and accel is also not expressed.

The change adds the link, i.e. if HVF is enabled the following will be
available in QOM:

  (qemu) qom-get /machine/accel type
  "hvf-accel"

Note that management applications shouldn't rely on the type as it may
change in future at QEMU's discretion.

---
Regards,
Roman

Re: [PATCH for-6.0] accel: Wire accel to /machine

[Daniel P. Berrangé]

On Mon, Dec 07, 2020 at 11:46:22AM +0300, Roman Bolshakov wrote:
> There's no generic way to query current accel and its properties via QOM
> because there's no link between an accel and current machine.
> 
> The change adds the link, i.e. if HVF is enabled the following will be
> available in QOM:
> 
>   (qemu) qom-get /machine/accel type
>   "hvf-accel"
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
> 
> Hi,
> 
> this is a follow up patch that deprecates earlier series [1].
> 
> An outstanding issue is whether management applications can rely on the
> value of /machine/accel/type and output of qom-list-types command [2][3]
> to get current and present accels?
> 
> i.e. would it be ok if libvirt assumes that everything up to the first
> dash in the accel type is the name of the accel (as specified via -M
> accel=ACCEL flag) when it performs QEMU probing?

Hmm, I think it is not nice - we shouldn't have to parse the
accel type names - IMHO typenames should be considered arbitrary
opaque strings, even if they'll not be expected to change.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH for-6.0] accel: Wire accel to /machine

[Roman Bolshakov]

On Mon, Dec 07, 2020 at 05:44:19PM +0000, Daniel P. Berrangé wrote:
> On Mon, Dec 07, 2020 at 11:46:22AM +0300, Roman Bolshakov wrote:
> > An outstanding issue is whether management applications can rely on the
> > value of /machine/accel/type and output of qom-list-types command [2][3]
> > to get current and present accels?
> > 
> > i.e. would it be ok if libvirt assumes that everything up to the first
> > dash in the accel type is the name of the accel (as specified via -M
> > accel=ACCEL flag) when it performs QEMU probing?
> 
> Hmm, I think it is not nice - we shouldn't have to parse the
> accel type names - IMHO typenames should be considered arbitrary
> opaque strings, even if they'll not be expected to change.
> 

Thanks Daniel. I'll then send v2 of ad-hoc QMP/HMP query. By the way, do
we need the HMP query if we get accel in QOM?

Regards,
Roman