diff mbox series

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

Message ID 20220714182836.89602-6-wuhaotsh@google.com
State New
Headers show
Series Misc NPCM7XX patches | expand

Commit Message

Hao Wu July 14, 2022, 6:28 p.m. UTC
This type is used to represent block devs that are not suitable to
be represented by other existing types.

A sample use is to represent an at24c eeprom device defined in
hw/nvram/eeprom_at24c.c. The block device can be used to contain the
content of the said eeprom device.

Signed-off-by: Hao Wu <wuhaotsh@google.com>
---
 blockdev.c                | 4 +++-
 include/sysemu/blockdev.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

Comments

Markus Armbruster July 18, 2022, 9:49 a.m. UTC | #1
Hao Wu <wuhaotsh@google.com> writes:

> This type is used to represent block devs that are not suitable to
> be represented by other existing types.
>
> A sample use is to represent an at24c eeprom device defined in
> hw/nvram/eeprom_at24c.c. The block device can be used to contain the
> content of the said eeprom device.
>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>

Let me add a bit more history in the hope of helping other reviewers.

v3 of this series[1] used IF_NONE for the EEPROM device.

Peter Maydell questioned that[2], and we concluded it's wrong.  I wrote

    [A] board can use any traditional interface type.  If none of them
    matches, and common decency prevents use of a non-matching one,
    invent a new one.  We could do IF_OTHER.

Thomas Huth cleaned up the existing abuse of IF_NONE to use IF_PFLASH
instead, in commit 6b717a8d44:

    hw/misc/sifive_u_otp: Use IF_PFLASH for the OTP device instead of IF_NONE

    Configuring a drive with "if=none" is meant for creation of a backend
    only, it should not get automatically assigned to a device frontend.
    Use "if=pflash" for the One-Time-Programmable device instead (like
    it is e.g. also done for the efuse device in hw/arm/xlnx-zcu102.c).

    Since the old way of configuring the device has already been published
    with the previous QEMU versions, we cannot remove this immediately, but
    have to deprecate it and support it for at least two more releases.

    Signed-off-by: Thomas Huth <thuth@redhat.com>
    Acked-by: Philippe Mathieu-Daudé <philmd@redhat.com>
    Reviewed-by: Markus Armbruster <armbru@redhat.com>
    Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
    Message-id: 20211119102549.217755-1-thuth@redhat.com
    Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

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?

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?

> ---
>  blockdev.c                | 4 +++-
>  include/sysemu/blockdev.h | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 9230888e34..befd69ac5f 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -82,6 +82,7 @@ static const char *const if_name[IF_COUNT] = {
>      [IF_MTD] = "mtd",
>      [IF_SD] = "sd",
>      [IF_VIRTIO] = "virtio",
> +    [IF_OTHER] = "other",
>      [IF_XEN] = "xen",
>  };
>  
> @@ -726,7 +727,8 @@ QemuOptsList qemu_legacy_drive_opts = {
>          },{
>              .name = "if",
>              .type = QEMU_OPT_STRING,
> -            .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
> +            .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio,"
> +                    " other)",
>          },{
>              .name = "file",
>              .type = QEMU_OPT_STRING,
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 3211b16513..d9dd5af291 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -21,6 +21,7 @@ typedef enum {
>       */
>      IF_NONE = 0,
>      IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
> +    IF_OTHER,
>      IF_COUNT
>  } BlockInterfaceType;


[1] https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg00372.html

[2] Subject: does drive_get_next(IF_NONE) make sense?
    Message-ID: <CAFEAcA9zmPds0+jHm8VY465XEhK6bbVPd+nDob1ruRPaHOua_Q@mail.gmail.com>
    https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg00710.html
Thomas Huth July 18, 2022, 9:54 a.m. UTC | #2
On 18/07/2022 11.49, Markus Armbruster wrote:
[...]
> 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?
> 
> 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?

Maybe we should simply rename IF_PFLASH to IF_EPROM or IF_FLASH to make it 
clear that it is not only about parallel flashes anymore? Just my 0.02 €.

  Thomas
Kevin Wolf July 27, 2022, 7:03 p.m. UTC | #3
Am 18.07.2022 um 11:49 hat Markus Armbruster geschrieben:
> Hao Wu <wuhaotsh@google.com> writes:
> 
> > This type is used to represent block devs that are not suitable to
> > be represented by other existing types.
> >
> > A sample use is to represent an at24c eeprom device defined in
> > hw/nvram/eeprom_at24c.c. The block device can be used to contain the
> > content of the said eeprom device.
> >
> > Signed-off-by: Hao Wu <wuhaotsh@google.com>
> 
> Let me add a bit more history in the hope of helping other reviewers.
> 
> v3 of this series[1] used IF_NONE for the EEPROM device.
> 
> Peter Maydell questioned that[2], and we concluded it's wrong.  I wrote
> 
>     [A] board can use any traditional interface type.  If none of them
>     matches, and common decency prevents use of a non-matching one,
>     invent a new one.  We could do IF_OTHER.
> 
> Thomas Huth cleaned up the existing abuse of IF_NONE to use IF_PFLASH
> instead, in commit 6b717a8d44:
> 
>     hw/misc/sifive_u_otp: Use IF_PFLASH for the OTP device instead of IF_NONE
> 
>     Configuring a drive with "if=none" is meant for creation of a backend
>     only, it should not get automatically assigned to a device frontend.
>     Use "if=pflash" for the One-Time-Programmable device instead (like
>     it is e.g. also done for the efuse device in hw/arm/xlnx-zcu102.c).
> 
>     Since the old way of configuring the device has already been published
>     with the previous QEMU versions, we cannot remove this immediately, but
>     have to deprecate it and support it for at least two more releases.
> 
>     Signed-off-by: Thomas Huth <thuth@redhat.com>
>     Acked-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>     Reviewed-by: Markus Armbruster <armbru@redhat.com>
>     Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>     Message-id: 20211119102549.217755-1-thuth@redhat.com
>     Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> 
> 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".

Kevin
Peter Maydell July 28, 2022, 9:46 a.m. UTC | #4
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...

thanks
-- PMM
Kevin Wolf July 28, 2022, 1:29 p.m. UTC | #5
Am 28.07.2022 um 11:46 hat Peter Maydell geschrieben:
> 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...

How confident are we that no board will ever have two devices of this
kind?

As long as every board has at most one, if=other is a bad user interface
in terms of descriptiveness, but still more or less workable as long as
you know what it means for the specific board you use.

But if you have more than one device, it becomes hard to predict which
device gets which backend - it depends on the initialisation order in
the code then, and I'm pretty sure that this isn't something that should
have significance in external interfaces and therefore become a stable
API.

Kevin
Peter Maydell July 28, 2022, 1:43 p.m. UTC | #6
On Thu, 28 Jul 2022 at 14:30, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 28.07.2022 um 11:46 hat Peter Maydell geschrieben:
> > On Wed, 27 Jul 2022 at 20:03, Kevin Wolf <kwolf@redhat.com> wrote:
> > > 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...
>
> How confident are we that no board will ever have two devices of this
> kind?
>
> As long as every board has at most one, if=other is a bad user interface
> in terms of descriptiveness, but still more or less workable as long as
> you know what it means for the specific board you use.

Extremely non-confident, but that holds for all these things,
unless we add new IF_* for every possible new thing:
 IF_PFLASH
 IF_EEPROM
 IF_EFUSE
 IF_BBRAM
 ...
?

thanks
-- PMM
Cédric Le Goater July 28, 2022, 1:57 p.m. UTC | #7
On 7/28/22 15:29, Kevin Wolf wrote:
> Am 28.07.2022 um 11:46 hat Peter Maydell geschrieben:
>> 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...
> 
> How confident are we that no board will ever have two devices of this
> kind?

The BMC machines have a lot of eeproms.

> 
> As long as every board has at most one, if=other is a bad user interface
> in terms of descriptiveness, but still more or less workable as long as
> you know what it means for the specific board you use.
> 
> But if you have more than one device, it becomes hard to predict which
> device gets which backend - it depends on the initialisation order in
> the code then, and I'm pretty sure that this isn't something that should
> have significance in external interfaces and therefore become a stable
> API.

Can't we use the drive index ?


There has been various attempts to solve this problem for the Aspeed
machines also. See below. May be we should introduce and IF_EEPROM for
the purpose.

Thanks,

C.

[PATCH v2] aspeed: qcom: add block backed FRU devices
https://www.mail-archive.com/qemu-devel@nongnu.org/msg900485.html

[PATCH] aspeed: Enable backend file for eeprom
http://patchwork.ozlabs.org/project/qemu-devel/patch/20220728061228.152704-1-wangzhiqiang02@inspur.com/
Markus Armbruster July 28, 2022, 2:50 p.m. UTC | #8
Kevin Wolf <kwolf@redhat.com> writes:

> Am 28.07.2022 um 11:46 hat Peter Maydell geschrieben:
>> 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...
>
> How confident are we that no board will ever have two devices of this
> kind?
>
> As long as every board has at most one, if=other is a bad user interface
> in terms of descriptiveness, but still more or less workable as long as
> you know what it means for the specific board you use.
>
> But if you have more than one device, it becomes hard to predict which
> device gets which backend - it depends on the initialisation order in
> the code then,

Really?  Board code should use IF_OTHER devices just like it uses the
other interface types, namely connecting each frontend device to a
backend device with a well-known and fixed interface type and index (or
bus and unit instead, where appropriate).

>                and I'm pretty sure that this isn't something that should
> have significance in external interfaces and therefore become a stable
> API.

I agree that "implied by execution order" is a bad idea: commit
95fd260f0a "blockdev: Drop unused drive_get_next()".
Peter Maydell July 28, 2022, 2:58 p.m. UTC | #9
On Thu, 28 Jul 2022 at 15:50, Markus Armbruster <armbru@redhat.com> wrote:
> Kevin Wolf <kwolf@redhat.com> writes:
> >
> > But if you have more than one device, it becomes hard to predict which
> > device gets which backend - it depends on the initialisation order in
> > the code then,
>
> Really?  Board code should use IF_OTHER devices just like it uses the
> other interface types, namely connecting each frontend device to a
> backend device with a well-known and fixed interface type and index (or
> bus and unit instead, where appropriate).

I think part of the problem is that unlike the typical disk
interface, where there is some idea of bus-and-unit-number or
index number that it makes sense to expose to users, these
"miscellaneous storage" devices don't have any particular index
concept -- in the real hardware there are just a random set of
devices that are connected in various places. So you're requiring
users to look up the documentation for "index 0 is this eeprom,
index 1 is that other eeprom, index 2 is ...".

thanks
-- PMM
Kevin Wolf July 28, 2022, 5:08 p.m. UTC | #10
Am 28.07.2022 um 16:50 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 28.07.2022 um 11:46 hat Peter Maydell geschrieben:
> >> 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...
> >
> > How confident are we that no board will ever have two devices of this
> > kind?
> >
> > As long as every board has at most one, if=other is a bad user interface
> > in terms of descriptiveness, but still more or less workable as long as
> > you know what it means for the specific board you use.
> >
> > But if you have more than one device, it becomes hard to predict which
> > device gets which backend - it depends on the initialisation order in
> > the code then,
> 
> Really?  Board code should use IF_OTHER devices just like it uses the
> other interface types, namely connecting each frontend device to a
> backend device with a well-known and fixed interface type and index (or
> bus and unit instead, where appropriate).
> 
> >                and I'm pretty sure that this isn't something that should
> > have significance in external interfaces and therefore become a stable
> > API.
> 
> I agree that "implied by execution order" is a bad idea: commit
> 95fd260f0a "blockdev: Drop unused drive_get_next()".

Ah good, I was indeed thinking of something drive_get_next()-like.

In case the board works with explicit indices, the situation is not as
bad as I was afraid. It will certainly be workable (even if not obvious)
for any boards that have a fixed number of devices with block backends,
which should cover everything we're intending to cover here.

I still consider if=other a bad user interface because what it means is
completely opaque, but if that's okay for you in your board, who am I to
object.

(Of course, the real solution would be having a generic way to set qdev
properties for on-board devices. I'm not expecting that we're getting
this anytime soon, though.)

Kevin
Markus Armbruster Aug. 4, 2022, 2:27 p.m. UTC | #11
Peter Maydell <peter.maydell@linaro.org> writes:

> On Thu, 28 Jul 2022 at 15:50, Markus Armbruster <armbru@redhat.com> wrote:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> >
>> > But if you have more than one device, it becomes hard to predict which
>> > device gets which backend - it depends on the initialisation order in
>> > the code then,
>>
>> Really?  Board code should use IF_OTHER devices just like it uses the
>> other interface types, namely connecting each frontend device to a
>> backend device with a well-known and fixed interface type and index (or
>> bus and unit instead, where appropriate).
>
> I think part of the problem is that unlike the typical disk
> interface, where there is some idea of bus-and-unit-number or
> index number that it makes sense to expose to users, these
> "miscellaneous storage" devices don't have any particular index
> concept -- in the real hardware there are just a random set of
> devices that are connected in various places. So you're requiring
> users to look up the documentation for "index 0 is this eeprom,
> index 1 is that other eeprom, index 2 is ...".

"Unit number on a bus" makes perfect sense for SCSI and PATA.  For SATA,
the only valid unit number is 0, which may or may not make sense to
users.  Not a problem in practice, though.

Bus numbers are arbitrary, though.  Harmless enough when you have to
deal only with very few of them, e.g. a single SCSI HBA (one bus, number
0), a single PATA HBA (two buses, number 0 and 1), a single SATA HBA
(typically six buses, numbers 0..5).

For anything else, we use "index" rather than "bus" and "unit", and the
indexes are completely arbitrary.  Again, harmless enough when you have
to deal only with a few of each interface type.

*Names* rather than arbitrary index or bus numbers would arguably be a
better interface.

Nothing of this is new with IF_OTHER.
Daniel P. Berrangé Aug. 4, 2022, 2:34 p.m. UTC | #12
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

With regards,
Daniel
Markus Armbruster Aug. 4, 2022, 2:56 p.m. UTC | #13
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.
Daniel P. Berrangé Aug. 4, 2022, 3:17 p.m. UTC | #14
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.

With regards,
Daniel
Markus Armbruster Aug. 4, 2022, 3:30 p.m. UTC | #15
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
Daniel P. Berrangé Aug. 4, 2022, 3:44 p.m. UTC | #16
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.

With regards,
Daniel
Markus Armbruster Aug. 8, 2022, 6:26 a.m. UTC | #17
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.
Kevin Wolf Aug. 8, 2022, 8:34 a.m. UTC | #18
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
Markus Armbruster Aug. 8, 2022, 10:14 a.m. UTC | #19
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.
diff mbox series

Patch

diff --git a/blockdev.c b/blockdev.c
index 9230888e34..befd69ac5f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -82,6 +82,7 @@  static const char *const if_name[IF_COUNT] = {
     [IF_MTD] = "mtd",
     [IF_SD] = "sd",
     [IF_VIRTIO] = "virtio",
+    [IF_OTHER] = "other",
     [IF_XEN] = "xen",
 };
 
@@ -726,7 +727,8 @@  QemuOptsList qemu_legacy_drive_opts = {
         },{
             .name = "if",
             .type = QEMU_OPT_STRING,
-            .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio)",
+            .help = "interface (ide, scsi, sd, mtd, floppy, pflash, virtio,"
+                    " other)",
         },{
             .name = "file",
             .type = QEMU_OPT_STRING,
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 3211b16513..d9dd5af291 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -21,6 +21,7 @@  typedef enum {
      */
     IF_NONE = 0,
     IF_IDE, IF_SCSI, IF_FLOPPY, IF_PFLASH, IF_MTD, IF_SD, IF_VIRTIO, IF_XEN,
+    IF_OTHER,
     IF_COUNT
 } BlockInterfaceType;