does drive_get_next(IF_NONE) make sense?

does drive_get_next(IF_NONE) make sense?

[Peter Maydell]

Does it make sense for a device/board to do drive_get_next(IF_NONE) ?

At the moment we have exactly one user of this, which is
hw/misc/sifive_u_otp.c. This is a model of a one-time-programmable
fuse, and the drive is providing the backing store for the fuse
contents. Borrowing an IF_NONE for this seems a bit odd, but
it's not clear any of the other IF_ types is better.

We also just (this release cycle) added models of the Xilinx
efuse OTP fuses. Those have been implemented to use IF_PFLASH.
(This is a somewhat unfortunate inconsistency I guess.)

We also have a patchseries currently in the code review stage
which uses IF_NONE:
https://patchew.org/QEMU/20211101232346.1070813-1-wuhaotsh@google.com/20211101232346.1070813-6-wuhaotsh@google.com/
Here we are trying to provide a drive as backing store for some
EEPROMs that hang off the i2c buses on some npcm7xx boards.

Are these uses of IF_NONE OK, or should we be doing something
else (using IF_PFLASH, defining a new IF_*, ???)

thanks
-- PMM

Re: does drive_get_next(IF_NONE) make sense?

[Philippe Mathieu-Daudé]

On 11/2/21 16:14, Peter Maydell wrote:
> Does it make sense for a device/board to do drive_get_next(IF_NONE) ?
> 
> At the moment we have exactly one user of this, which is
> hw/misc/sifive_u_otp.c. This is a model of a one-time-programmable
> fuse, and the drive is providing the backing store for the fuse
> contents. Borrowing an IF_NONE for this seems a bit odd, but
> it's not clear any of the other IF_ types is better.
> 
> We also just (this release cycle) added models of the Xilinx
> efuse OTP fuses. Those have been implemented to use IF_PFLASH.
> (This is a somewhat unfortunate inconsistency I guess.)
> 
> We also have a patchseries currently in the code review stage
> which uses IF_NONE:
> https://patchew.org/QEMU/20211101232346.1070813-1-wuhaotsh@google.com/20211101232346.1070813-6-wuhaotsh@google.com/
> Here we are trying to provide a drive as backing store for some
> EEPROMs that hang off the i2c buses on some npcm7xx boards.
> 
> Are these uses of IF_NONE OK, or should we be doing something
> else (using IF_PFLASH, defining a new IF_*, ???)

IIUC '-drive if=xxx' is deprecated, replaced by '-blockdev'.

Personally I expect a BlockInterfaceType to be a bus interface
and see IF_NONE as a buggy case.

See also:
https://lore.kernel.org/qemu-devel/YDY7KKI1Xme29UlQ@stefanha-x1.localdomain/

I am not sure about the amount of work required to fully leverage
-blockdev and remove -drive. Can it be sugar-expanded from the CLI?

What else is missing to finish the blockdev conversion?

Re: does drive_get_next(IF_NONE) make sense?

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> Does it make sense for a device/board to do drive_get_next(IF_NONE) ?

Short answer: hell, no!  ;)

Long answer below.

> At the moment we have exactly one user of this, which is
> hw/misc/sifive_u_otp.c. This is a model of a one-time-programmable
> fuse, and the drive is providing the backing store for the fuse
> contents. Borrowing an IF_NONE for this seems a bit odd, but
> it's not clear any of the other IF_ types is better.
>
> We also just (this release cycle) added models of the Xilinx
> efuse OTP fuses. Those have been implemented to use IF_PFLASH.
> (This is a somewhat unfortunate inconsistency I guess.)
>
> We also have a patchseries currently in the code review stage
> which uses IF_NONE:
> https://patchew.org/QEMU/20211101232346.1070813-1-wuhaotsh@google.com/20211101232346.1070813-6-wuhaotsh@google.com/
> Here we are trying to provide a drive as backing store for some
> EEPROMs that hang off the i2c buses on some npcm7xx boards.
>
> Are these uses of IF_NONE OK, or should we be doing something
> else (using IF_PFLASH, defining a new IF_*, ???)

Two questions, really: one, may boards IF_NONE for onboard devices, and
two, should new board code use drive_get_next().


drive_get_next() is basically a bad idea.  It returns the "next" block
backend of a certain interface type.  "Next" means bus=0,unit=N, where
subsequent calls count N up from zero, per interface type.

This lets you define unit numbers implicitly by execution order.  If the
order changes, or new calls appear "in the middle", unit numbers change.
ABI break.  Hard to spot in review.

Old hat: explicit is better than implicit.  Better use drive_get().
Even if that means you have to pass unit numbers around.


IF_NONE is *only* for use with -device.

Traditional -drive (before IF_NONE) does two things:

(1) Create a block backend device

(2) Request the board to create a block frontend device

    Which kind of device the board creates is up to the board.  Nothing
    but common decency stops a board from creating a floppy when asked
    for an IF_PFLASH.

When -device was invented, we needed a way to create just a block
backend.

A working (at the time), but obviously bad way was to use -drive to
create one the board ignores.

Improvement: invent an interface type all the boards ignore.  This is
IF_NONE.

Hindsight: we should have created a separate option instead of
overloading -drive.  Such an option now exists: -blockdev.

The difference between IF_NONE and the other interface types is more
than just convention: we actually detect when the board ignores a
request to create a block device, like this:

    $ qemu-system-x86_64 -S -drive if=mtd
    qemu-system-x86_64: -drive if=mtd: machine type does not support if=mtd,bus=0,unit=0

We don't do this for if=none, because those may still be used with
device_add:

    $ qemu-system-x86_64 -nodefaults -display none -S -drive if=none,id=mt -device virtio-scsi -monitor stdio
    QEMU 6.1.50 monitor - type 'help' for more information
    (qemu) device_add scsi-cd,drive=mt,id=cd0
    (qemu) info block
    mt: [not inserted]
        Attached to:      cd0
        Removable device: not locked, tray closed

Therefore, having the board use IF_NONE just like a traditional
interface type is *wrong*.

As I explained above, the 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.

Re: does drive_get_next(IF_NONE) make sense?

[Thomas Huth]

On 03/11/2021 09.41, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> Does it make sense for a device/board to do drive_get_next(IF_NONE) ?
> 
> Short answer: hell, no!  ;)

Would it make sense to add an "assert(type != IF_NONE)" to drive_get() to 
avoid such mistakes in the future?

  Thomas

Re: does drive_get_next(IF_NONE) make sense?

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 03/11/2021 09.41, Markus Armbruster wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>> 
>>> Does it make sense for a device/board to do drive_get_next(IF_NONE) ?
>> Short answer: hell, no!  ;)
>
> Would it make sense to add an "assert(type != IF_NONE)" to drive_get()
> to avoid such mistakes in the future?

Worth a try.

Re: does drive_get_next(IF_NONE) make sense?

[Peter Maydell]

On Fri, 12 Nov 2021 at 13:34, Markus Armbruster <armbru@redhat.com> wrote:
>
> Thomas Huth <thuth@redhat.com> writes:
>
> > On 03/11/2021 09.41, Markus Armbruster wrote:
> >> Peter Maydell <peter.maydell@linaro.org> writes:
> >>
> >>> Does it make sense for a device/board to do drive_get_next(IF_NONE) ?
> >> Short answer: hell, no!  ;)
> >
> > Would it make sense to add an "assert(type != IF_NONE)" to drive_get()
> > to avoid such mistakes in the future?
>
> Worth a try.

You need to fix the sifive_u_otp device first :-)

-- PMM

Re: does drive_get_next(IF_NONE) make sense?

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 12 Nov 2021 at 13:34, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Thomas Huth <thuth@redhat.com> writes:
>>
>> > On 03/11/2021 09.41, Markus Armbruster wrote:
>> >> Peter Maydell <peter.maydell@linaro.org> writes:
>> >>
>> >>> Does it make sense for a device/board to do drive_get_next(IF_NONE) ?
>> >> Short answer: hell, no!  ;)
>> >
>> > Would it make sense to add an "assert(type != IF_NONE)" to drive_get()
>> > to avoid such mistakes in the future?
>>
>> Worth a try.
>
> You need to fix the sifive_u_otp device first :-)

And for that, we may want Hao Wu's "[PATCH v4 5/7] blockdev: Add a new
IF type IF_OTHER" first.

Re: does drive_get_next(IF_NONE) make sense?

[Alistair Francis]

On Mon, Nov 15, 2021 at 3:32 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Fri, 12 Nov 2021 at 13:34, Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Thomas Huth <thuth@redhat.com> writes:
> >>
> >> > On 03/11/2021 09.41, Markus Armbruster wrote:
> >> >> Peter Maydell <peter.maydell@linaro.org> writes:
> >> >>
> >> >>> Does it make sense for a device/board to do drive_get_next(IF_NONE) ?
> >> >> Short answer: hell, no!  ;)
> >> >
> >> > Would it make sense to add an "assert(type != IF_NONE)" to drive_get()
> >> > to avoid such mistakes in the future?
> >>
> >> Worth a try.
> >
> > You need to fix the sifive_u_otp device first :-)
>
> And for that, we may want Hao Wu's "[PATCH v4 5/7] blockdev: Add a new
> IF type IF_OTHER" first.

I can fixup sifive_u_otp, just let me know what the prefered solution is

Alistair

Re: does drive_get_next(IF_NONE) make sense?

[Kevin Wolf]

Am 15.11.2021 um 06:31 hat Markus Armbruster geschrieben:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > On Fri, 12 Nov 2021 at 13:34, Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Thomas Huth <thuth@redhat.com> writes:
> >>
> >> > On 03/11/2021 09.41, Markus Armbruster wrote:
> >> >> Peter Maydell <peter.maydell@linaro.org> writes:
> >> >>
> >> >>> Does it make sense for a device/board to do drive_get_next(IF_NONE) ?
> >> >> Short answer: hell, no!  ;)
> >> >
> >> > Would it make sense to add an "assert(type != IF_NONE)" to drive_get()
> >> > to avoid such mistakes in the future?
> >>
> >> Worth a try.
> >
> > You need to fix the sifive_u_otp device first :-)
> 
> And for that, we may want Hao Wu's "[PATCH v4 5/7] blockdev: Add a new
> IF type IF_OTHER" first.

Same question as for Hao Wu's series: Wouldn't the proper solution be to
add a drive property to the machine type?

If you can't use -blockdev, it's not done right.

Kevin

Re: does drive_get_next(IF_NONE) make sense?

[Peter Maydell]

On Mon, 15 Nov 2021 at 13:24, Kevin Wolf <kwolf@redhat.com> wrote:
> Same question as for Hao Wu's series: Wouldn't the proper solution be to
> add a drive property to the machine type?
>
> If you can't use -blockdev, it's not done right.

Is there an example of "doing it right" for built-in-to-the-machine
devices?

(My experience with the new-style options is that almost
always they're designed for x86 where the device they're attached
to is also created on the command line, and then handling of boards
where the device is builtin is either an afterthought or forgotten.
See also -netdev, where it took forever for built-in-ethernet to
be usable.)

-- PMM

Re: does drive_get_next(IF_NONE) make sense?

[Kevin Wolf]

Am 15.11.2021 um 14:31 hat Peter Maydell geschrieben:
> On Mon, 15 Nov 2021 at 13:24, Kevin Wolf <kwolf@redhat.com> wrote:
> > Same question as for Hao Wu's series: Wouldn't the proper solution be to
> > add a drive property to the machine type?
> >
> > If you can't use -blockdev, it's not done right.
> 
> Is there an example of "doing it right" for built-in-to-the-machine
> devices?
> 
> (My experience with the new-style options is that almost
> always they're designed for x86 where the device they're attached
> to is also created on the command line, and then handling of boards
> where the device is builtin is either an afterthought or forgotten.
> See also -netdev, where it took forever for built-in-ethernet to
> be usable.)

As long as we don't have a separate way to configure built-in devices
without creating them, for boards where the device is built in, the
properties for the device have to become machine properties.

I seem to remember that Markus implemented this for some boards.

Just doing without properties for these devices, either by just hard
coding things instead of providing options to the user, or by bypassing
the property system and accessing global state instead feels wrong.

Maybe once we have .instance_config (see my QOM/QAPI integration RFC),
forwarding these properties from -machine could actually become trivial,
just one call to set the properties for each built-in device. For now,
it's probably not as nice because each property needs to be forwarded
individually instead of just providing access to all properties of the
device.

Kevin

Re: does drive_get_next(IF_NONE) make sense?

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 15 Nov 2021 at 13:24, Kevin Wolf <kwolf@redhat.com> wrote:
>> Same question as for Hao Wu's series: Wouldn't the proper solution be to
>> add a drive property to the machine type?
>>
>> If you can't use -blockdev, it's not done right.
>
> Is there an example of "doing it right" for built-in-to-the-machine
> devices?
>
> (My experience with the new-style options is that almost
> always they're designed for x86 where the device they're attached
> to is also created on the command line, and then handling of boards
> where the device is builtin is either an afterthought or forgotten.
> See also -netdev, where it took forever for built-in-ethernet to
> be usable.)

I'm afraid the situation for onboard block devices is far worse than it
ever was for NICs.  See my reply "On configuring onboard block devices
with -blockdev" to Kevin's other message on the topic.

To be fair, improving onboard device configuration is *hard*.  Our
general device configuration interface doesn't cover them, and we've so
far failed finding a general solution.  Without one, we're drowning in
the sheer number of boards and onboard devices.  Which is ever growing.

Re: does drive_get_next(IF_NONE) make sense?

[Thomas Huth]

On 15/11/2021 08.12, Alistair Francis wrote:
> On Mon, Nov 15, 2021 at 3:32 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>
>>> On Fri, 12 Nov 2021 at 13:34, Markus Armbruster <armbru@redhat.com> wrote:
>>>>
>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>
>>>>> On 03/11/2021 09.41, Markus Armbruster wrote:
>>>>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>>>>
>>>>>>> Does it make sense for a device/board to do drive_get_next(IF_NONE) ?
>>>>>> Short answer: hell, no!  ;)
>>>>>
>>>>> Would it make sense to add an "assert(type != IF_NONE)" to drive_get()
>>>>> to avoid such mistakes in the future?
>>>>
>>>> Worth a try.
>>>
>>> You need to fix the sifive_u_otp device first :-)
>>
>> And for that, we may want Hao Wu's "[PATCH v4 5/7] blockdev: Add a new
>> IF type IF_OTHER" first.
> 
> I can fixup sifive_u_otp, just let me know what the prefered solution is

What kind of device is that OTP exactly? If it is some kind of non-serial 
flash device, maybe you could simply use IF_PFLASH instead?

  Thomas

Re: does drive_get_next(IF_NONE) make sense?

[Alistair Francis]

On Tue, Nov 16, 2021 at 2:10 AM Thomas Huth <thuth@redhat.com> wrote:
>
> On 15/11/2021 08.12, Alistair Francis wrote:
> > On Mon, Nov 15, 2021 at 3:32 PM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Peter Maydell <peter.maydell@linaro.org> writes:
> >>
> >>> On Fri, 12 Nov 2021 at 13:34, Markus Armbruster <armbru@redhat.com> wrote:
> >>>>
> >>>> Thomas Huth <thuth@redhat.com> writes:
> >>>>
> >>>>> On 03/11/2021 09.41, Markus Armbruster wrote:
> >>>>>> Peter Maydell <peter.maydell@linaro.org> writes:
> >>>>>>
> >>>>>>> Does it make sense for a device/board to do drive_get_next(IF_NONE) ?
> >>>>>> Short answer: hell, no!  ;)
> >>>>>
> >>>>> Would it make sense to add an "assert(type != IF_NONE)" to drive_get()
> >>>>> to avoid such mistakes in the future?
> >>>>
> >>>> Worth a try.
> >>>
> >>> You need to fix the sifive_u_otp device first :-)
> >>
> >> And for that, we may want Hao Wu's "[PATCH v4 5/7] blockdev: Add a new
> >> IF type IF_OTHER" first.
> >
> > I can fixup sifive_u_otp, just let me know what the prefered solution is
>
> What kind of device is that OTP exactly? If it is some kind of non-serial
> flash device, maybe you could simply use IF_PFLASH instead?

It just says "one time programmable memory". I'm guessing it's
sometype of eFuse.

Alistair

>
>   Thomas
>

Re: does drive_get_next(IF_NONE) make sense?

[Peter Maydell]

On Thu, 18 Nov 2021 at 13:04, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, Nov 16, 2021 at 2:10 AM Thomas Huth <thuth@redhat.com> wrote:
> > What kind of device is that OTP exactly? If it is some kind of non-serial
> > flash device, maybe you could simply use IF_PFLASH instead?
>
> It just says "one time programmable memory". I'm guessing it's
> sometype of eFuse.

We used IF_PFLASH for the Xilinx efuse devices we recently added.
So we should probably use that for consistency, unless we want
to instead say that efuses should be something other than IF_PFLASH.

Either way it's a compatibility break for command lines, so we
should probably try to have only one rather than two :-)

-- PMM