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

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> 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.

Your stated purpose for multiple -pflash:

    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.

Such a split between writable part and read-only part makes sense to me.
How is it done in physical hardware?  Single device with configurable
write-protect, or two separate devices?

>> 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 a sysbus device, and these still don't work with -device.  My
pending "[PATCH v3 00/10] Clean up and fix no_user" makes them
unavailable with -device.

>> 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.

Actually, s/aren't available/don't work/ without my pending series just
mentioned.

> 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".

Yes, except I wouldn't expect such fancy defaults.

> Thanks for looking into this, you doubtlessly know way more about the
> device model than I do.

No problem, I just wanted to figure out whether we're creating even more
legacy -drive headaches here, so why not share my reasoning.