Re: [Qemu-devel] [PATCH v4 04/10] block/pflash_cfi02: Implement intereleaved flash devices

[Philippe Mathieu-Daudé]

On 6/22/19 2:25 PM, Philippe Mathieu-Daudé wrote:
> Hi Stephen,
> 
> This series haven't fall through the cracks, however it is taking me
> longer than expected to review it.
> 
> On 4/26/19 6:26 PM, Stephen Checkoway wrote:
>> It's common for multiple narrow flash chips to be hooked up in parallel
>> to support wider buses. For example, four 8-bit wide flash chips (x8)
>> may be combined in parallel to produce a 32-bit wide device. Similarly,
>> two 16-bit wide chips (x16) may be combined.
>>
>> This commit introduces `device-width` and `max-device-width` properties,
>> similar to pflash_cfi01, with the following meanings:
>> - `width`: The width of the logical, qemu device (same as before);
>> - `device-width`: The width of an individual flash chip, defaulting to
>>   `width`; and
>> - `max-device-width`: The maximum width of an individual flash chip,
>>   defaulting to `device-width`.
>>
>> Nothing needs to change to support reading such interleaved devices but
>> commands (e.g., erase and programming) must be sent to all devices at
>> the same time or else the various chips will be in different states.
> 
> After some thoughts on this, I'd rather we model how hardware manage
> interleaved devices: do it at the bus level, and instanciate N devices
> in an interleaved config.
> I believe that would drastically reduce this device complexity, and we
> would match the real internal state machine.
> Also this could be reused by other parallel devices used in a such config.
> 
>> For example, a 4-byte wide logical device can be composed of four x8/x16
>> devices in x8 mode. That is, each device supports both x8 or x16 and
>> they're being used in the byte, rather than word, mode. This
>> configuration would have `width=4`, `device-width=1`, and
>> `max-device-width=2`.
> 
> 
> I'm thinking of this draft:
> 
> FlashDevice # x8
>   MemoryRegionOps
>     .valid.max_access_size = 1
> 
> FlashDevice # x16
>   MemoryRegionOps
>     .valid.min_access_size = 2
>     .valid.max_access_size = 2
> 
> FlashDevice # x8/x16
>   MemoryRegionOps
>     .valid.min_access_size = 1
>     .valid.max_access_size = 2
> 
> We might use .impl.min_access_size = 2 and consider all NOR flash using
> 16-bit words internally.
>     .impl.max_access_size = 2 is implicit.
> 
> So for you example we'd instanciate one:
> 
> InterleaverDevice
>   Property
>     .bus_width = 4 # 4-byte wide logical device, `width=4`
>     .device_width = 1 # `device-width=1`
>   MemoryRegionOps
>     .valid.max_access_size = .bus_width # 4, set at realize()
>     .impl.max_access_size = .device_width # 1, set at realize()
> 
> Then instanciate 4 pflash devices, and link them to the interleaver
> using object_property_set_link().
> 
> typedef struct {
>     SysBusDevice parent_obj;
>     MemoryRegion iomem;
>     char *name;
>     /*
>      * On a 64-bit wide bus we can have at most
>      * 8 devices in 8-bit access mode.
>      */
>     MemoryRegion device[8];
>     unsigned device_count;
>     unsigned device_index_mask;
>     /* Properties */
>     unsigned bus_width;
>     unsigned device_width;
> } InterleaverDeviceState;
> 
> static Property interleaver_properties[] = {
>     DEFINE_PROP_LINK("device[0]", InterleaverDeviceState,
>                      device[0],
>                      TYPE_MEMORY_REGION, MemoryRegion *),
>     ...
>     DEFINE_PROP_LINK("device[7]", InterleaverDeviceState,
>                      device[7],
>                      TYPE_MEMORY_REGION, MemoryRegion *),
>     DEFINE_PROP_END_OF_LIST(),
> };
> 
> Then previous to call InterleaverDevice.realize():
> 
> In the board realize():
> 
> 
>     for (i = 0; i < interleaved_devices; i++) {
>         pflash[i] = create_pflash(...);
>         ...
>     }
> 
>     ild = ... create InterleaverDevice ...
>     for (i = 0; i < interleaved_devices; i++) {
>         char *propname = g_strdup_printf("device[%u]", i);
> 
> 
>         object_property_set_link(OBJECT(&ild->device[i]),
>                                  OBJECT(pflash[i]),
>                                  propname, &err);
>         ...
>     }
> 
> Finally,
> 
> static void interleaved_realize(DeviceState *dev, Error **errp)
> {
>     InterleaverDeviceState *s = INTERLEAVER_DEVICE(opaque);
> 
>     s->device_count = s->bus_width / s->device_width;
>     s->device_index_mask = ~(s->device_count - 1);
>     ...
> }
> 
> static void interleaved_write(void *opaque, hwaddr offset,
>                               uint64_t value, unsigned size)
> {
>     InterleaverDeviceState *s = INTERLEAVER_DEVICE(opaque);
>     MemoryRegion *mr;
> 
>     /*
>      * Since we set .impl.max_access_size = device_width,
>      * access_with_adjusted_size() always call this with
>      * size = device_width.
>      *
>      * Adjust the address (offset).
>      */
>     offset >>= size;
>     /* Access the N interleaved device */
>     mr = s->device[offset & s->device_index_mask];
>     memory_region_dispatch_write(mr, offset, &value, size,
>                                  MEMTXATTRS_UNSPECIFIED);
> }
> 
> I'll try a PoC.

So I have a PoC, but then realize I can not use the same flash dump...

I need to deinterleave is. This is easily fixed with few lines of
Python, then if I want to store/share the dump (aka 'backend storage') I
have to re-interleave it.

I wonder if it would be possible/easy to add a 'interleave' option to
blockdev to be able to have 2 pflash devices sharing the same backend...
Is it worthwhile? Kevin/Markus/Max any thought on this?

Thanks,

Phil.

>> In addition to commands being sent to all devices, guest firmware
>> expects the status and CFI queries to be replicated for each device.
>> (The one exception to the response replication is that each device gets
>> to report its own status bit DQ7 while programming because its value
>> depends on the value being programmed which will usually differ for each
>> device.)
>>
>> Testing is limited to 16-bit wide devices due to the current inability
>> to override the properties set by `pflash_cfi02_register`, but multiple
>> configurations are tested.
>>
>> Stop using global_qtest. Instead, package the qtest variable inside the
>> FlashConfig structure.
>>
>> Signed-off-by: Stephen Checkoway <address@hidden>
>> Acked-by: Thomas Huth <address@hidden>
>> ---
>>  hw/block/pflash_cfi02.c   | 270 +++++++++++++++------
>>  tests/pflash-cfi02-test.c | 476 ++++++++++++++++++++++++++++++--------
>>  2 files changed, 576 insertions(+), 170 deletions(-)