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

[Markus Armbruster]

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