Re: [PATCH v5 5/8] blockdev: Add a new IF type IF_OTHER

[Kevin Wolf]

Am 08.08.2022 um 08:26 hat Markus Armbruster geschrieben:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Thu, Aug 04, 2022 at 05:30:40PM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> 
> >> > On Thu, Aug 04, 2022 at 04:56:15PM +0200, Markus Armbruster wrote:
> >> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> >> 
> >> >> > On Thu, Jul 28, 2022 at 10:46:35AM +0100, Peter Maydell wrote:
> >> >> >> On Wed, 27 Jul 2022 at 20:03, Kevin Wolf <kwolf@redhat.com> wrote:
> >> >> >> >
> >> >> >> > Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
> >> >> >> > > An OTP device isn't really a parallel flash, and neither are 
> >> >> >> > > eFuses.
> >> >> >> > > More fast-and-lose use of IF_PFLASH may exist in the tree, and 
> >> >> >> > > maybe of
> >> >> >> > > other interface types, too.
> >> >> >> > >
> >> >> >> > > This patch introduces IF_OTHER.  The patch after next uses it 
> >> >> >> > > for an
> >> >> >> > > EEPROM device.
> >> >> >> > >
> >> >> >> > > Do we want IF_OTHER?
> >> >> >> >
> >> >> >> > What would the semantics even be? Any block device that doesn't 
> >> >> >> > pick up
> >> >> >> > a different category may pick up IF_OTHER backends?
> >> >> >> >
> >> >> >> > It certainly feels like a strange interface to ask for "other" 
> >> >> >> > disk and
> >> >> >> > then getting as surprise what this other thing might be. It's
> >> >> >> > essentially the same as having an explicit '-device other', and I
> >> >> >> > suppose most people would find that strange.
> >> >> >> >
> >> >> >> > > If no, I guess we get to abuse IF_PFLASH some more.
> >> >> >> > >
> >> >> >> > > If yes, I guess we should use IF_PFLASH only for actual parallel 
> >> >> >> > > flash
> >> >> >> > > memory going forward.  Cleaning up existing abuse of IF_PFLASH 
> >> >> >> > > may not
> >> >> >> > > be worth the trouble, though.
> >> >> >> > >
> >> >> >> > > Thoughts?
> >> >> >> >
> >> >> >> > If the existing types aren't good enough (I don't have an opinion 
> >> >> >> > on
> >> >> >> > whether IF_PFLASH is a good match), let's add a new one. But a 
> >> >> >> > specific
> >> >> >> > new one, not just "other".
> >> >> >> 
> >> >> >> I think the common thread is "this isn't what anybody actually thinks
> >> >> >> of as being a 'disk', but we would like to back it with a block 
> >> >> >> device
> >> >> >> anyway". That can cover a fair range of possibilities...
> >> >> >
> >> >> > Given that, do we even want/have to use -drive for this ?    We can 
> >> >> > use
> >> >> > -blockdev for the backend and reference that from any -device we want
> >> >> > to create, and leave -drive out of the picture entirely
> >> >> 
> >> >> -drive is our only means to configure onboard devices.
> >> >> 
> >> >> We've talked about better means a few times, but no conclusions.  I can
> >> >> dig up pointers, if you're interested.
> >> >
> >> > For onboard pflash with x86, we've just got properties against the
> >> > machine that we can point to a blockdev.
> >> 
> >> True, but the vast majority of onboard block devices doesn't come with
> >> such properties.  Please see
> >> 
> >> Subject: On configuring onboard block devices with -blockdev (was: [PATCH 
> >> v4 6/7] hw/nvram: Update at24c EEPROM init function in NPCM7xx boards)
> >> Date: Mon, 15 Nov 2021 16:28:33 +0100
> >> Message-ID: <875ystigke.fsf_-_@dusky.pond.sub.org>
> >> https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg03173.html
> >
> > My take away from your mail there is that in the absence of better ideas
> > we should at least use machine properties for anything new we do so we
> > don't make the problem worse than it already is. It feels more useful
> > than inventing new IF_xxx possibilities for something we think is the
> > wrong approach already.
> 
> The difficulty of providing machine properties for existing devices and
> the lack of adoption even for new devices make me doubt they are a
> viable path forward.  In the message I referenced above, I wrote:
> 
>     If "replace them all by machine properties" is the way forward, we
>     need to get going.  At the current rate of one file a year (measured
>     charitably), we'll be done around 2090, provided we don't add more
>     (we've added quite a few since I did the first replacement).
> 
> I figure this has slipped to the 22nd century by now.
> 
> Yes, more uses of -drive are steps backward.  But they are trivially
> easy, and also drops in the bucket.  Machine properties are more
> difficult, and whether they actually take us forward seems doubtful.

Machine properties are also not what we really want, but just a
workaround. How about implementing the real thing, providing qdev
properties for built-in devices?

Looking at a few random users of drive_get(), they look like this:

    DriveInfo *dinfo = drive_get(...);
    dev = qdev_new("driver-type");
    qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo));
    qdev_realize_and_unref(...);

What if we assigned a name to every built-in device and had a
qdev_new_builtin(type, id) that applies qdev properties given on the
command line to the device? Could be either with plain old '-device'
(we're already doing a similar thing with default devices where adding
-device for the existing device removes the default device) or with a
new command line option.

The qdev_prop_set_drive() would then become conditional and would only
be done if the property wasn't already set in qdev_new_builtin(). Or
maybe a new function that checks for conflicting -drive and -device.
Though that part is just implementation details.

Kevin