Re: [Qemu-devel] [qemu PATCH] hw/i386/pc_sysfw: support more than one flash drive

[Laszlo Ersek]

On 11/25/13 16:22, Markus Armbruster wrote:
> Laszlo Ersek <address@hidden> writes:
> 
>> On 11/22/13 13:21, Markus Armbruster wrote:
>>> Laszlo Ersek <address@hidden> 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