On Fri, 25 Jan 2019 at 15:11, Markus Armbruster <armbru@xxxxxxxxxx> wrote: > (1) cfi.pflash01 isn't available with -device. > > (2) "Magic board code picks up the backend [created for -drive > if=pflash], creates a frontend (a cfi.pflash01 device), and maps it > into the guest's address space." When we replace if=pflash by > if=none, we get to replicate that magic on top of -device. > > Issue (1) isn't too hard: we add the device to the dynamic sysbus device > white-list, move a sysbus_mmio_map() from pflash_cfi01_realize() into > pflash_cfi01_realize(). The latter requires a new device property to > configure the base address. I got a working prototype. Since this > makes the device model's name and properties ABI, review would be > advisable. Flash devices exist on the board at specific addresses, so they should in general be created by the board model. Creating them by the user on the command line is a mess because then the user has to know the right base address. And then the board code needs to do something for "if the user didn't create this then we need to do it", because the flash device should exist in the model whether the user cared about its contents or not. Dynamic sysbus is something I'd rather we did less of, not more of. It's there because it solves an annoying problem (people want to do device passthrough of hardware that's not on a nice pluggable probeable bus), but it's really awkward. > To solve (2), we first have to understand the magic. Device > cfi.pflash01 has the following properties: > > num-blocks Size of the device in blocks > sector-length Size of a block > (admire the choice of names) > width Bank width > big-endian Endianess (d'oh) > id0, id1, id2, id3 Some kind of device ID, guest-visible, > default to zero, few boards change it Note that most of this is stuff that the hardware has. A lot of boards set these to garbage values which happened to be what the very old implementation of pflash hardcoded, because most guests don't care. This is strictly speaking wrong and they should use whatever the hardware really has, but most of these cases are for old not-very-maintained dev boards where probably nobody even has the relevant hardware even if they cared enough to find out what its ID values are. > name Memory region name > (why is this even configurable?) (a) for debug purposes, so a machine can create two flash devices and give them names which make them easier to tell apart in monitor "info mem" and similar command output (b) more importantly, the memory region name is used as the migration vmstate ram name, so if you change it you break migration compat. > phys-addr Physical base address > (this is the new device property > mentioned above) > secure For restricting access to firmware, > default off > device-width you don't want to know, > there is a default, but it's documented > as "bad, do not use", yet pretty much > all boards use it See above about "old not-very-maintained dev boards". A board which does use this is one that's doing it for back-compat because nobody's cared to fix and test. > max-device-width defaults to device-width > not actually set anywhere > old-multiple-chip-handling back-compat gunk for > machine types 2.8 and older > > The magic board code in hw/i386/pc_sysfw.c configures as follows: > > num-blocks computed from backend size > sector-length 4096 > width 1 > big-endian 0 > id0, id1, id2, id3 all 0 > name system.pflash<U>, where U is -drive's > unit number > phys-addr computed so > unit 0 ends right below 0x100000000, > unit n+1 ends at right below unit n > > "secure", "device-width", "max-device-width", > "old-multiple-chip-handling" are left at the default. > > One additional bit of magic is actually in libvirt: it configures > "secure" by flipping its default with > -global driver=cfi.pflash01,property=secure,value=on. > > Now let's consider how to replicate this magic on top of device. > > Perhaps machine-type specific defaults could take care of sector-length, > width, big-endian, id0, id1, id2, id3. Leaves num-blocks, name, and > phys-addr. You could at least in theory have a machine with several different flash devices of different make/ID. This is why they're device properties. I think they're fine the way they are, ie set by the code that creates the device. thanks -- PMM -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
