Re: [PATCH v2 09/13] hw/ppc/e500: Implement pflash handling

[Bernhard Beschow]

Am 3. Oktober 2022 21:21:15 UTC schrieb "Philippe Mathieu-Daudé" 
<f4bug@amsat.org>:
>On 3/10/22 22:31, Bernhard Beschow wrote:
>> Allows e500 boards to have their root file system reside on flash using
>> only builtin devices located in the eLBC memory region.
>> 
>> Note that the flash memory area is only created when a -pflash argument is
>> given, and that the size is determined by the given file. The idea is to
>> put users into control.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   docs/system/ppc/ppce500.rst | 12 ++++++
>>   hw/ppc/Kconfig              |  1 +
>>   hw/ppc/e500.c               | 76 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 89 insertions(+)
>
>> @@ -856,6 +892,7 @@ void ppce500_init(MachineState *machine)
>>       unsigned int pci_irq_nrs[PCI_NUM_PINS] = {1, 2, 3, 4};
>>       IrqLines *irqs;
>>       DeviceState *dev, *mpicdev;
>> +    DriveInfo *dinfo;
>>       CPUPPCState *firstenv = NULL;
>>       MemoryRegion *ccsr_addr_space;
>>       SysBusDevice *s;
>> @@ -1024,6 +1061,45 @@ void ppce500_init(MachineState *machine)
>>                                   pmc->platform_bus_base,
>>                                   &pms->pbus_dev->mmio);
>>   +    dinfo = drive_get(IF_PFLASH, 0, 0);
>> +    if (dinfo) {
>> +        BlockBackend *blk = blk_by_legacy_dinfo(dinfo);
>> +        BlockDriverState *bs = blk_bs(blk);
>> +        uint64_t size = bdrv_getlength(bs);
>> +        uint64_t mmio_size = pms->pbus_dev->mmio.size;
>> +        uint32_t sector_len = 64 * KiB;
>> +
>> +        if (ctpop64(size) != 1) {
>> +            error_report("Size of pflash file must be a power of two.");
>
>This is a PFLASH restriction (which you already fixed in the previous
>patch), not a board one.

I agree that this check seems redundant to the one in cfi01. I added this one 
for clearer error messages since cfi01 only complains about the "device size" 
not being a power of two while this message at least gives a hint towards the 
source of the problem (the file given in the pflash option).

Usually the size of the pflash area is hardcoded in the board while I choose to 
derive it from the size of the backing file in order to avoid hardcoding it. My 
idea is to put users into control by offering more flexibility.

>
>> +            exit(1);
>> +        }
>> +
>> +        if (size > mmio_size) {
>> +            error_report("Size of pflash file must not be bigger than %" 
>> PRIu64
>> +                         " bytes.", mmio_size);
>
>There is no hardware limitation here, you can wire flash bigger than the
>memory aperture. What is above the aperture will simply be ignored.
>
>Should we display a warning here instead of a fatal error?

While this is technically possible, is that what users would expect? Couldn't 
we just require users to truncate their files if they really want the 
"aperture" behavior?

>
>> +            exit(1);
>> +        }
>> +
>> +        assert(QEMU_IS_ALIGNED(size, sector_len));
>
>Similarly, this doesn't seem a problem the board code should worry
>about: better to defer it to PFLASH realize().

The reason for the assert() here is that size isn't stored directly in the 
cfi01 device. Instead, it must be calculated by the properties "num-blocks" 
times "sector-length". For this to work, size must be divisible by sector_len 
without remainder, which is checked by the assertion.

We could theoretically add a "size" property which would violate the single 
point of truth principle, though. Do you see a different solution?

Best regards,
Bernhard

>
>> +        dev = qdev_new(TYPE_PFLASH_CFI01);
>> +        qdev_prop_set_drive(dev, "drive", blk);
>> +        qdev_prop_set_uint32(dev, "num-blocks", size / sector_len);
>> +        qdev_prop_set_uint64(dev, "sector-length", sector_len);
>> +        qdev_prop_set_uint8(dev, "width", 2);
>> +        qdev_prop_set_bit(dev, "big-endian", true);
>> +        qdev_prop_set_uint16(dev, "id0", 0x89);
>> +        qdev_prop_set_uint16(dev, "id1", 0x18);
>> +        qdev_prop_set_uint16(dev, "id2", 0x0000);
>> +        qdev_prop_set_uint16(dev, "id3", 0x0);
>> +        qdev_prop_set_string(dev, "name", "e500.flash");
>> +        s = SYS_BUS_DEVICE(dev);
>> +        sysbus_realize_and_unref(s, &error_fatal);
>> +
>> +        memory_region_add_subregion(&pms->pbus_dev->mmio, 0,
>> +                                    sysbus_mmio_get_region(s, 0));
>> +    }
>> +
>>       /*
>>        * Smart firmware defaults ahead!
>>        *
>