qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: "Damien Hedde" <damien.hedde@greensocs.com>,
	edgar.iglesias@xilinx.com,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"Eric Blake" <eblake@redhat.com>,
	"Mark Burton" <mark.burton@greensocs.com>,
	qemu-devel@nongnu.org,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	"Eric Auger" <eric.auger@redhat.com>,
	"Mirela Grujic" <mirela.grujic@greensocs.com>,
	"Alistair Francis" <alistair.francis@wdc.com>,
	"Gerd Hoffmann" <kraxel@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: Re: [RFC PATCH v3 0/5] QMP support for cold-plugging devices
Date: Wed, 24 Nov 2021 14:07:31 +0000	[thread overview]
Message-ID: <YZ5HI0ms/sd4gEIS@redhat.com> (raw)
In-Reply-To: <87lf1dwtm4.fsf@dusky.pond.sub.org>

On Wed, Nov 24, 2021 at 02:50:11PM +0100, Markus Armbruster wrote:
> Damien Hedde <damien.hedde@greensocs.com> writes:
> 
> > The biggest difference is the fw_cfg option I think: it is related
> > with the rom_set_order_override()/rom_reset_order_override() (line 17
> > and 25). There is also the usb devices parts in between. I lack the 
> > knowledge about fw_cfg/usb to tell if it is important or not.
> >
> > What I wanted to say is I don't know if the difference is
> > acceptable. If we want device_add to support all -device use cases, it
> > is not. In that case we need to stop either in the middle of this
> > function (line 15) or at the end (better with your sketch in mind).
> >
> > Note that rom_set_order_override()/rom_reset_order_override() only
> > set/reset a switch variable that changes how fw_cfg files are
> > sorted. It could be integrated into device_add code (and removed from
> > the above function) without changing the behavior.
> 
> For me, the part that puts me off is interleaving CLI and QMP.
> 
> We process the CLI in an order few people understand, and only while
> staring at the code.  That's bad.
> 
> Injecting QMP at certain points in that sequence can only make it worse.

Yep, I share your unease here.. especially wrt this quoted text
from later:

  > >> Users can do as much or as little with the CLI as they want.  You'd
  > >> probably want to set up a QMP monitor and no more.

I would say that is a case of overkill. It can only make our
lives harder as maintainers in the long term, if we have to
worry about such arbitrary mixing of QMP and CLI. This is
also why I'm pretty uneasy about the 'preconfig' stuff as
implemented today in general.

It is a half-way house that doesn't really give mgmt apps
what they want, which is a 100% QAPI-only config. If mgmt
apps start using preconfig, it won't make life any better
for them and will also lock QEMU maintainers into supporting
this half-way house.

We have a bit of a track record with QEMU of introducing
partial solutions and never quite finishing the job. There's
little strong incentive to ever finish it, if you can freely
mix both old and new style forever, and thus maintainers are
burdened forever with both.

IMHO, we should only try to support the non-mixed scenarios

  - 100% of hardware configured via CLI args
  - 100% of hardware configured via QAPI (whether live in
    QMP, or fed in via a QAPI based JSON/YAML config file)

so that we only have two clear cases we need to worry about
dealing with.

Focus our efforts 100% of the 100% QAPI scenario and don't
divert energy into short term hybrid solutions.


> >> Let me quote the sketch I posted to the "Stabilize preconfig" thread:
> >> 
> >> 1. Start event loop
> >> 
> >> 2. Feed it CLI left to right.  Each option runs a handler just like each
> >>     QMP command does.
> >> 
> >>     Options that read a configuration file inject the file into the feed.
> >> 
> >>     Options that create a monitor create it suspended.
> >> 
> >>     Options may advance the phase / run state, and they may require
> >>     certain phase(s).
> >> 
> >> 3. When we're done with CLI, resume any monitors we created.
> >> 
> >> 4. Monitors now feed commands to the event loop.  Commands may advance
> >>     the phase / run state, and they may require certain phase(s).
> >> 
> >> Users can do as much or as little with the CLI as they want.  You'd
> >> probably want to set up a QMP monitor and no more.
> >> 
> >> device_add becomes possible at a certain state of the phase / run state
> >> machine.  It changes from cold to hot plug at a certain later state.
> >> 
> >>> [1]: https://lore.kernel.org/qemu-devel/b31f442d28920447690a6b8cee865bdbacde1283.1635160056.git.mprivozn@redhat.com
> >>>
> >>> Thanks for your feedback.
> >> 
> 

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 :|



  reply	other threads:[~2021-11-24 14:09 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-17 14:46 [RFC PATCH v3 0/5] QMP support for cold-plugging devices Damien Hedde
2021-11-17 14:46 ` [RFC PATCH v3 1/5] rename MachineInitPhase enum constants for QAPI compatibility Damien Hedde
2021-11-19 13:17   ` Alistair Francis
2021-11-17 14:47 ` [RFC PATCH v3 2/5] qapi: Implement query-machine-phase QMP command Damien Hedde
2021-11-19 13:21   ` Alistair Francis
2021-11-17 14:47 ` [RFC PATCH v3 3/5] qapi: Implement x-machine-init " Damien Hedde
2021-11-17 14:47 ` [RFC PATCH v3 4/5] qapi: Allow device_add to execute in machine initialized phase Damien Hedde
2021-11-17 14:47 ` [RFC PATCH v3 5/5] docs/system: improve doc about preconfig Damien Hedde
2021-11-20  9:00 ` [RFC PATCH v3 0/5] QMP support for cold-plugging devices Markus Armbruster
2021-11-23 16:11   ` Damien Hedde
2021-11-24 13:50     ` Markus Armbruster
2021-11-24 14:07       ` Daniel P. Berrangé [this message]
2021-11-24 14:51         ` Markus Armbruster
2021-11-25 12:42           ` Damien Hedde
2021-11-25 12:55           ` Daniel P. Berrangé
2021-11-29 19:55   ` Dr. David Alan Gilbert

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YZ5HI0ms/sd4gEIS@redhat.com \
    --to=berrange@redhat.com \
    --cc=alistair.francis@wdc.com \
    --cc=armbru@redhat.com \
    --cc=damien.hedde@greensocs.com \
    --cc=dgilbert@redhat.com \
    --cc=eblake@redhat.com \
    --cc=edgar.iglesias@xilinx.com \
    --cc=ehabkost@redhat.com \
    --cc=eric.auger@redhat.com \
    --cc=kraxel@redhat.com \
    --cc=mark.burton@greensocs.com \
    --cc=mirela.grujic@greensocs.com \
    --cc=mst@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=philmd@redhat.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).