Re: [PATCH] hw/block/pflash: invalidate TBs on flash update

[Fabiano Rosas <farosas@suse.de>]

Philippe Mathieu-Daudé <philmd@oss.qualcomm.com> writes:

> Hi Elliot,
>
> On 16/10/26 21:15, Elliot Berman wrote:
>> When pflash_cfi01/02 are out of Read Array mode and being reprogrammed
>> or erased (such as a firmware update) by a guest under TCG, TBs from
>> when the guest was executing instructions from the pflash are never
>> invalidated. After re-enabling Read Array, the guest continues to
>> execute instructions from the stale TBs.
>> 
>> Call memory_region_flush_rom_device() from pflash_update() so the TBs
>> over the updated range are invalidated.
>> 
>> memory_region_flush_rom_device() asserted that the memory region was in
>> romd_mode (i.e. Read Array mode), but the pflash is not in romd_mode
>> when processing MemoryRegionOps->write() commands. Since the RAM can
>> change even when !romd_mode, relax the assertion so it only checks
>> rom_device.
>> 
>> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/4490
>> Suggested-by: Alex Bennée <alex.bennee@linaro.org>
>> Signed-off-by: Elliot Berman <elliotjb+anduril@elliotjb.com>
>> ---
>>   hw/block/pflash_cfi01.c | 5 ++++-
>>   hw/block/pflash_cfi02.c | 3 +++
>>   system/physmem.c        | 2 +-
>>   3 files changed, 8 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
>> index 486fc6350d..66ecdb0969 100644
>> --- a/hw/block/pflash_cfi01.c
>> +++ b/hw/block/pflash_cfi01.c
>> @@ -388,12 +388,15 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset,
>>       return ret;
>>   }
>>   
>> -/* update flash content on disk */
>> +/* update flash content on disk and invalidate the guest view of RAM */
>
>   /*
>    * Flush modified flash content: mark the range dirty, invalidate the
>    * TBs translated from it, and write it back to the block backend.
>    */
>
>>   static void pflash_update(PFlashCFI01 *pfl, int offset,
>>                             int size)
>>   {
>>       int offset_end;
>>       int ret;
>> +
>> +    memory_region_flush_rom_device(&pfl->mem, offset, size);
>> +
>>       if (pfl->blk) {
>>           offset_end = offset + size;
>>           /* widen to sector boundaries */
>
> I'd like feedback from migration maintainers: could we be writing
> staled content during live migration? Or maybe we are already safe.
> invalidate_and_set_dirty() gets DIRTY_MEMORY_MIGRATION set via the
> memory_region_get_dirty_log_mask call. Hmm so this change also fixes
> migration then. If confirmed, worth mentioning in the patch description:
>
>
>    Direct writes to the flash RAM were not marked dirty for migration,
>    so pages reprogrammed during pre-copy were not re-sent, and
>    postload_update_cb() wrote the stale content to the block backend.
>    The memory_region_flush_rom_device() call now also fixes this.
>

Yes, this does seem to be the case. I couldn't find any path where the
writes from pflash_write() are invalidated.