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

[Kevin Wolf]

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