On 11/25/13 16:22, Markus Armbruster wrote: > Laszlo Ersek <ler...@redhat.com> writes: > >> On 11/22/13 13:21, Markus Armbruster wrote: >>> Laszlo Ersek <ler...@redhat.com> writes: >>> >>>> This patch allows the user to usefully specify >>>> >>>> -drive file=img_1,if=pflash,format=raw,readonly \ >>>> -drive file=img_2,if=pflash,format=raw >>>> >>>> on the command line. The flash images will be mapped under 4G in their >>>> reverse unit order -- that is, with their base addresses progressing >>>> downwards, in increasing unit order. >>>> >>>> (The unit number increases with command line order if not explicitly >>>> specified.) >>>> >>>> This accommodates the following use case: suppose that OVMF is split in >>>> two parts, a writeable host file for non-volatile variable storage, and a >>>> read-only part for bootstrap and decompressible executable code. >>>> >>>> The binary code part would be read-only, centrally managed on the host >>>> system, and passed in as unit 0. The variable store would be writeable, >>>> VM-specific, and passed in as unit 1. >>>> >>>> 00000000ffe00000-00000000ffe1ffff (prio 0, R-): system.flash1 >>>> 00000000ffe20000-00000000ffffffff (prio 0, R-): system.flash0 >>>> >>>> (If the guest tries to write to the flash range that is backed by the >>>> read-only drive, bdrv_write() in pflash_update() will correctly deny the >>>> write with -EACCES, and pflash_update() won't care, which suits us well.) >>> >>> Before this patch: >>> >>> You can define as many if=pflash drives as you want. Any with non-zero >>> index are silently ignored. >>> >>> If you don't specify one with index=0, you get a ROM at the top of the >>> 32 bit address space, contents taken from -bios (default "bios.bin"). >>> Up to its last 128KiB are aliased at the top of the ISA address space. >>> >>> If you do specify one with index=0, you get a pflash device at the top >>> of the 32 bit address space, with contents from the drive, and -bios is >>> silently ignored. Up to its last 128KiB are copied to a ROM at the top >>> of the (20 bit) ISA address space. >>> >>> After this patch (please correct misunderstandings): >>> >>> Now the drives after the first unused index are silently ignored. >>> >>> If you don't specify one with index=0, no change. >>> >>> If you do, you now get N pflash devices, where N is the first unused >>> index. Each pflash's contents is taken from the respective drive. The >>> flashes are mapped at the top of the 32 bit address space in reverse >>> index order. -bios is silently ignored, as before. Up to the last >>> 128KiB of the index=0 flash are copied to a ROM at the top of the ISA >>> address space. >>> >>> Thus, no change for index=0. For index=1..N, we now get additional >>> flash devices. >>> >>> Correct? >> >> Yes. > > Thanks. > > 1. Multiple pflash devices > > Is there any way for a guest to see the N separate pflash devices, or do > they look exactly like a single pflash device?
The interpretation of multiple -pflash options is board specific. I grepped the source for IF_PFLASH first, and found some boards that use more than one flash device. Merging them in one contiguous target-phys address range would be i386 specific. This doesn't break compatibility (because multiple pflash devices used not to be supported for i386 targets at all), but I agree that this would posit their interpretation for the future, and thus it might need deeper thought. >From looking at "hw/block/pflash_cfi01.c", it seems that the guest can interrogate the flash device about its size (nb_blocs is stored in cfi_table starting at 0x2D, and cfi_table can be queried by command 0x98 in pflash_read()). So, if the guest cares, it can figure out that there are multiple devices backing the range. I think. > 2. More board code device configuration magic > > Our long term goal is to configure machines by configuration files > (i.e. data) rather than code. > > Your patch extends the PC board code dealing with if=pflash for multiple > drives. > > Reminder: -drive if=X (where X != none) creates a backend, and leaves it > in a place where board code can find it. Board code may elect to create > a frontend using that backend. Yes, I'm aware. I did think about this -- eg. just create a drive with if=none, and add a frontend with -device something. But, the frontend here is not a device (a "peripheral"), it's a memory region with special mmio ops. Pflash frontends can apparently be created with "-device cfi.pflash01,...': cfi.pflash01.drive=drive cfi.pflash01.num-blocks=uint32 cfi.pflash01.sector-length=uint64 cfi.pflash01.width=uint8 cfi.pflash01.big-endian=uint8 cfi.pflash01.id0=uint16 cfi.pflash01.id1=uint16 cfi.pflash01.id2=uint16 cfi.pflash01.id3=uint16 cfi.pflash01.name=string We can even point it to the backing drive as well. But these properties don't seem to allow for the i386 specific "processing", eg. where to map the device in target-phys address space. > It's desirable that any new board code creating a frontend for -drive > if=X,... is sufficiently simple so that users can get the same result > with some -drive if=none,... -device ... incantation. The second form > provides full control over device properties. See section "Block > Devices" in docs/qdev-device-use.txt for examples of such mappings. > > This is the case for if=ide, if=scsi, if=floppy and if=virtio (see > docs/qdev-device-use.txt). It's not yet the case for if=pflash, because > the qdev used with it (cfi.pflash01) is a sysbus device, and these > aren't available with -device, yet. Thanks, I didn't know that that was in the background. > When that gets fixed, I'd expect support for putting the flash device at > a specific address. An equivalent if=none incantation (free of board > code magic) should be possible. > > Thus, the board magic your patch adds should be of the harmless kind. > Thanks. I think I managed to follow your train of thought, I just wasn't sure where you'd end up. I think, as you say, once sysbus devices can be specified with -device, cfi.pflash01 could take an address, and if that's omitted, the default for i386 could be "map it just below the previous one". Thanks for looking into this, you doubtlessly know way more about the device model than I do. Laszlo