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

[Markus Armbruster]

Adding Paolo and Eduardo since we're wandering into QOM-land.

Kevin Wolf <kwolf@redhat.com> writes:

> 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

Before we continue: I like the idea of giving onboard devices
user-friendly, stable names.  "serial0" beats
"/machine/unattached/device[13]" hands down.  Even more so where the
"device[13]" part depends on other options (e.g. with "-vga none" it's
"device[12]", both for i440FX).

The question is what these names could be.  They can't be qdev IDs,
because we blew our chance to reserve names.

Could we solve this in QOM?

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

The idea sounds vaguely familiar.  Whether it's because we discussed
similar ones on the list, or because I mulled over similar ones in my
head I can't say for sure.

Overloading -device so it can also configure existing devices feels
iffy.

We already have means to set properties for (onboard) devices to pick
up: -global.  But it's per device *type*, and therefore falls apart as
soon as you have more than one of the type.  We need something that
affects a specific (onboard) device, and for that we need a way to
address one.  The QOM paths we have don't cut it, as I illustrated
above.