Re: does drive_get_next(IF_NONE) make sense?
On Thu, 18 Nov 2021 at 13:04, Alistair Francis wrote: > > On Tue, Nov 16, 2021 at 2:10 AM Thomas Huth 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
Re: does drive_get_next(IF_NONE) make sense?
On Tue, Nov 16, 2021 at 2:10 AM Thomas Huth wrote: > > On 15/11/2021 08.12, Alistair Francis wrote: > > On Mon, Nov 15, 2021 at 3:32 PM Markus Armbruster wrote: > >> > >> Peter Maydell writes: > >> > >>> On Fri, 12 Nov 2021 at 13:34, Markus Armbruster wrote: > > Thomas Huth writes: > > > On 03/11/2021 09.41, Markus Armbruster wrote: > >> Peter Maydell 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?
On 15/11/2021 08.12, Alistair Francis wrote: On Mon, Nov 15, 2021 at 3:32 PM Markus Armbruster wrote: Peter Maydell writes: On Fri, 12 Nov 2021 at 13:34, Markus Armbruster wrote: Thomas Huth writes: On 03/11/2021 09.41, Markus Armbruster wrote: Peter Maydell 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?
Peter Maydell writes: > On Mon, 15 Nov 2021 at 13:24, Kevin Wolf 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?
Am 15.11.2021 um 14:31 hat Peter Maydell geschrieben: > On Mon, 15 Nov 2021 at 13:24, Kevin Wolf 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?
On Mon, 15 Nov 2021 at 13:24, Kevin Wolf 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?
Am 15.11.2021 um 06:31 hat Markus Armbruster geschrieben: > Peter Maydell writes: > > > On Fri, 12 Nov 2021 at 13:34, Markus Armbruster wrote: > >> > >> Thomas Huth writes: > >> > >> > On 03/11/2021 09.41, Markus Armbruster wrote: > >> >> Peter Maydell 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?
On Mon, Nov 15, 2021 at 3:32 PM Markus Armbruster wrote: > > Peter Maydell writes: > > > On Fri, 12 Nov 2021 at 13:34, Markus Armbruster wrote: > >> > >> Thomas Huth writes: > >> > >> > On 03/11/2021 09.41, Markus Armbruster wrote: > >> >> Peter Maydell 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?
Peter Maydell writes: > On Fri, 12 Nov 2021 at 13:34, Markus Armbruster wrote: >> >> Thomas Huth writes: >> >> > On 03/11/2021 09.41, Markus Armbruster wrote: >> >> Peter Maydell 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?
On Fri, 12 Nov 2021 at 13:34, Markus Armbruster wrote: > > Thomas Huth writes: > > > On 03/11/2021 09.41, Markus Armbruster wrote: > >> Peter Maydell 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?
Thomas Huth writes: > On 03/11/2021 09.41, Markus Armbruster wrote: >> Peter Maydell 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?
On 03/11/2021 09.41, Markus Armbruster wrote: Peter Maydell 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?
Peter Maydell 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-wuhao...@google.com/20211101232346.1070813-6-wuhao...@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?
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-wuhao...@google.com/20211101232346.1070813-6-wuhao...@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?
does drive_get_next(IF_NONE) make sense?
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-wuhao...@google.com/20211101232346.1070813-6-wuhao...@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
