Re: [RFC PATCH v2 3/3] hw/pflash_cfi01: Allow devices to have a smaller backing device

[David Edmondson <dme@dme.org>]

On Monday, 2021-02-22 at 15:06:35 +01, Philippe Mathieu-Daudé wrote:

> Hi David,
>
> On 2/22/21 10:07 AM, David Edmondson wrote:
>> Allow the backing device to be smaller than the extent of the flash
>> device by mapping it as a subregion of the flash device region.
>> 
>> Return zeroes for all reads of the flash device beyond the extent of
>> the backing device.
>> 
>> For writes beyond the extent of the underlying device, fail on
>> read-only devices and discard them for writable devices.
>
> This looks much simpler now.

Thanks for looking at it.

>> Signed-off-by: David Edmondson <david.edmondson@oracle.com>
>> ---
>>  hw/block/pflash_cfi01.c | 108 ++++++++++++++++++++++++++++++----------
>>  hw/block/trace-events   |   3 ++
>>  2 files changed, 86 insertions(+), 25 deletions(-)
>
>>      if (pfl->blk) {
>>          uint64_t perm;
>> +
>>          pfl->ro = !blk_supports_write_perm(pfl->blk);
>>          perm = BLK_PERM_CONSISTENT_READ | (pfl->ro ? 0 : BLK_PERM_WRITE);
>>          ret = blk_set_perm(pfl->blk, perm, BLK_PERM_ALL, errp);
>>          if (ret < 0) {
>>              return;
>>          }
>> +
>> +        inner_len = blk_getlength(pfl->blk);
>> +
>> +        if (inner_len > outer_len) {
>> +            error_setg(errp,
>> +                       "block backend provides %" HWADDR_PRIu " bytes, "
>> +                       "device limited to %" PRIu64 " bytes",
>> +                       inner_len, outer_len);
>> +            return;
>> +        }
>
> Do you mind extracting this change in a previous patch?

Before this change that test was performed by
blk_check_size_and_read_all(), which insisted that the block device was
exactly the same size as the region (because it was told to read enough
bytes to fill the region).

Now that we pass the size of the backing device to that function, rather
than the size of the region, it's necessary to add the extra check
before the call.

Hence, I don't think that it's useful to add this as an earlier patch.

It would perhaps make sense to switch away from
blk_check_size_and_read_all(), preferring to just call blk_pread() now,
which would be cleaner?

dme.
-- 
Do I have to tell the story, of a thousand rainy days since we first met?