Re: [PATCH v2 2/2] hw/pflash: implement update buffer for block writes

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Gerd,

On 8/1/24 13:53, Philippe Mathieu-Daudé wrote:
> From: Gerd Hoffmann <kraxel@redhat.com>
> 
> Add an update buffer where all block updates are staged.
> Flush or discard updates properly, so we should never see
> half-completed block writes in pflash storage.
> 
> Drop a bunch of FIXME comments ;)
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Message-ID: <20240105135855.268064-3-kraxel@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/block/pflash_cfi01.c | 106 ++++++++++++++++++++++++++++++----------
>   1 file changed, 80 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index ce63ba43b6..0120462648 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -80,16 +80,39 @@ struct PFlashCFI01 {
>       uint16_t ident3;
>       uint8_t cfi_table[0x52];
>       uint64_t counter;
> -    unsigned int writeblock_size;
> +    uint32_t writeblock_size;
>       MemoryRegion mem;
>       char *name;
>       void *storage;
>       VMChangeStateEntry *vmstate;
>       bool old_multiple_chip_handling;
> +
> +    /* block update buffer */
> +    unsigned char *blk_bytes;

I'd rather use a 'void *' type here, but then we need to
use a (uinptr_t) cast in pflash_data_write().

> +    uint32_t blk_offset;
>   };
>   
>   static int pflash_post_load(void *opaque, int version_id);
>   
> +static bool pflash_blk_write_state_needed(void *opaque)
> +{
> +    PFlashCFI01 *pfl = opaque;
> +
> +    return (pfl->blk_offset != -1);
> +}
> +
> +static const VMStateDescription vmstate_pflash_blk_write = {
> +    .name = "pflash_cfi01_blk_write",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = pflash_blk_write_state_needed,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_VBUFFER_UINT32(blk_bytes, PFlashCFI01, 0, NULL, writeblock_size),

I don't get the difference with VMSTATE_VBUFFER_ALLOC_UINT32() which
sets VMS_ALLOC. In this case pflash_cfi01_realize() does the alloc so
we don't need VMS_ALLOC?

> +        VMSTATE_UINT32(blk_offset, PFlashCFI01),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>   static const VMStateDescription vmstate_pflash = {
>       .name = "pflash_cfi01",
>       .version_id = 1,
> @@ -101,6 +124,10 @@ static const VMStateDescription vmstate_pflash = {
>           VMSTATE_UINT8(status, PFlashCFI01),
>           VMSTATE_UINT64(counter, PFlashCFI01),
>           VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (const VMStateDescription * const []) {
> +        &vmstate_pflash_blk_write,
> +        NULL
>       }
>   };
>   
> @@ -376,12 +403,51 @@ static void pflash_update(PFlashCFI01 *pfl, int offset,
>       }
>   }
>   
> +/* copy current flash content to block update buffer */
> +static void pflash_blk_write_start(PFlashCFI01 *pfl, hwaddr offset)
> +{
> +    hwaddr mask = ~(pfl->writeblock_size - 1);
> +
> +    pfl->blk_offset = offset & mask;
> +    memcpy(pfl->blk_bytes, pfl->storage + pfl->blk_offset,
> +           pfl->writeblock_size);
> +}
> +
> +/* commit block update buffer changes */
> +static void pflash_blk_write_flush(PFlashCFI01 *pfl)
> +{
> +    g_assert(pfl->blk_offset != -1);
> +    memcpy(pfl->storage + pfl->blk_offset, pfl->blk_bytes,
> +           pfl->writeblock_size);
> +    pflash_update(pfl, pfl->blk_offset, pfl->writeblock_size);
> +    pfl->blk_offset = -1;
> +}
> +
> +/* discard block update buffer changes */
> +static void pflash_blk_write_abort(PFlashCFI01 *pfl)
> +{
> +    pfl->blk_offset = -1;
> +}
> +
>   static inline void pflash_data_write(PFlashCFI01 *pfl, hwaddr offset,
>                                        uint32_t value, int width, int be)
>   {
> -    uint8_t *p = pfl->storage;
> +    uint8_t *p;
> +
> +    if (pfl->blk_offset != -1) {

I'd rather have a trace event in this if() ladder.

> +        /* block write: redirect writes to block update buffer */
> +        if ((offset < pfl->blk_offset) ||
> +            (offset + width > pfl->blk_offset + pfl->writeblock_size)) {
> +            pfl->status |= 0x10; /* Programming error */
> +            return;
> +        }
> +        p = pfl->blk_bytes + (offset - pfl->blk_offset);
> +    } else {
> +        /* write directly to storage */
> +        trace_pflash_data_write(pfl->name, offset, width, value, pfl->counter);
> +        p = pfl->storage + offset;
> +    }
>   
> -    trace_pflash_data_write(pfl->name, offset, width, value, pfl->counter);
>       if (be) {
>           stn_be_p(p, width, value);
>       } else {
> @@ -504,6 +570,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>               trace_pflash_write_block(pfl->name, value);
>               pfl->counter = value;
>               pfl->wcycle++;
> +            pflash_blk_write_start(pfl, offset);
>               break;
>           case 0x60:
>               if (cmd == 0xd0) {
> @@ -534,12 +601,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>           switch (pfl->cmd) {
>           case 0xe8: /* Block write */
>               /* FIXME check @offset, @width */
> -            if (!pfl->ro) {
> -                /*
> -                 * FIXME writing straight to memory is *wrong*.  We
> -                 * should write to a buffer, and flush it to memory
> -                 * only on confirm command (see below).
> -                 */
> +            if (!pfl->ro && (pfl->blk_offset != -1)) {
>                   pflash_data_write(pfl, offset, value, width, be);
>               } else {
>                   pfl->status |= 0x10; /* Programming error */
> @@ -548,18 +610,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>               pfl->status |= 0x80;
>   
>               if (!pfl->counter) {
> -                hwaddr mask = pfl->writeblock_size - 1;
> -                mask = ~mask;
> -
>                   trace_pflash_write(pfl->name, "block write finished");
>                   pfl->wcycle++;
> -                if (!pfl->ro) {
> -                    /* Flush the entire write buffer onto backing storage.  */
> -                    /* FIXME premature! */
> -                    pflash_update(pfl, offset & mask, pfl->writeblock_size);
> -                } else {
> -                    pfl->status |= 0x10; /* Programming error */
> -                }
>               }
>   
>               pfl->counter--;
> @@ -571,20 +623,17 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset,
>       case 3: /* Confirm mode */
>           switch (pfl->cmd) {
>           case 0xe8: /* Block write */
> -            if (cmd == 0xd0) {
> -                /* FIXME this is where we should write out the buffer */
> +            if ((cmd == 0xd0) && !(pfl->status & 0x10)) {
> +                pflash_blk_write_flush(pfl);
>                   pfl->wcycle = 0;
>                   pfl->status |= 0x80;
>               } else {
> -                qemu_log_mask(LOG_UNIMP,
> -                    "%s: Aborting write to buffer not implemented,"
> -                    " the data is already written to storage!\n"
> -                    "Flash device reset into READ mode.\n",
> -                    __func__);
> +                pflash_blk_write_abort(pfl);
>                   goto mode_read_array;
>               }
>               break;
>           default:
> +            pflash_blk_write_abort(pfl);
>               goto error_flash;
>           }
>           break;
> @@ -818,6 +867,9 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>       pfl->cmd = 0x00;
>       pfl->status = 0x80; /* WSM ready */
>       pflash_cfi01_fill_cfi_table(pfl);
> +
> +    pfl->blk_bytes = g_malloc(pfl->writeblock_size);
> +    pfl->blk_offset = -1;
>   }
>   
>   static void pflash_cfi01_system_reset(DeviceState *dev)
> @@ -837,6 +889,8 @@ static void pflash_cfi01_system_reset(DeviceState *dev)
>        * This model deliberately ignores this delay.
>        */
>       pfl->status = 0x80;
> +
> +    pfl->blk_offset = -1;
>   }
>   
>   static Property pflash_cfi01_properties[] = {

Patch LGTM. If you want I can apply the changes suggested
and post a v3/queue.

Regards,

Phil.