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

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

[Elliot Berman]

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 */
 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 */
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 4f72c438c0..22b27366ee 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -419,6 +419,9 @@ static void pflash_update(PFlashCFI02 *pfl, int offset, int size)
 {
     int offset_end;
     int ret;
+
+    memory_region_flush_rom_device(&pfl->orig_mem, offset, size);
+
     if (pfl->blk) {
         offset_end = offset + size;
         /* widen to sector boundaries */
diff --git a/system/physmem.c b/system/physmem.c
index db3dda563d..c532e29571 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3162,7 +3162,7 @@ void memory_region_flush_rom_device(MemoryRegion *mr, hwaddr addr, hwaddr size)
      * necessary.  Other memory regions should use the
      * address_space_read/write() APIs.
      */
-    assert(memory_region_is_romd(mr));
+    assert(mr->rom_device);
 
     invalidate_and_set_dirty(mr, addr, size);
 }

---
base-commit: 7e8d3c7fc460fe7cd28a3e423d7466fa292bc269
change-id: 20261015-pflash-update-invalidate-tbs-7d7e9fa3a3cf

Best regards,
--  
Elliot Berman <elliotjb+anduril@elliotjb.com>

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

[Philippe Mathieu-Daudé]

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.


> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 4f72c438c0..22b27366ee 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -419,6 +419,9 @@ static void pflash_update(PFlashCFI02 *pfl, int offset, int size)

Same comment update missing:

  /*
   * Flush modified flash content: mark the range dirty, invalidate the
   * TBs translated from it, and write it back to the block backend.
   */

>   {
>       int offset_end;
>       int ret;
> +
> +    memory_region_flush_rom_device(&pfl->orig_mem, offset, size);
> +
>       if (pfl->blk) {
>           offset_end = offset + size;
>           /* widen to sector boundaries */
> diff --git a/system/physmem.c b/system/physmem.c
> index db3dda563d..c532e29571 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -3162,7 +3162,7 @@ void memory_region_flush_rom_device(MemoryRegion *mr, hwaddr addr, hwaddr size)
>        * necessary.  Other memory regions should use the
>        * address_space_read/write() APIs.

      * The ROMD mode is not checked: the RAM backing a ROM device can be
      * modified while the region is in MMIO mode.

>        */
> -    assert(memory_region_is_romd(mr));
> +    assert(mr->rom_device);
>   
>       invalidate_and_set_dirty(mr, addr, size);
>   }
We need to update the API contract:

-- >8 --
  /**
   * memory_region_flush_rom_device: Mark a range of pages dirty and 
invalidate
   *                                 TBs (for self-modifying code).
   *
   * The MemoryRegionOps->write() callback of a ROM device must use this 
function
   * to mark byte ranges that have been modified internally, such as by 
directly
   * accessing the memory returned by memory_region_get_ram_ptr().
   *
+ * This function may be called whether or not the region is in ROMD mode:
+ * a device that leaves ROMD mode (see memory_region_rom_device_set_romd())
+ * while it is being programmed must still flush the modified range, so 
that
+ * translations created before ROMD mode was left are not reused once it is
+ * entered again.
+ *
   * This function marks the range dirty and invalidates TBs so that TCG can
   * detect self-modifying code.
   *
- * @mr: the region being flushed.
+ * @mr: the region being flushed; must be a ROM device.
   * @addr: the start, relative to the start of the region, of the range 
being
   *        flushed.
   * @size: the size, in bytes, of the range being flushed.
   */
---

Please split this patch in 2, first the memory_region_flush_rom_device
change then the pflash ones (2 distinct subsystems, better to bisect
or partially cherry-pick). Change LGTM otherwise!

Regards,

Phil.

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

[Fabiano Rosas]

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.

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

[Philippe Mathieu-Daudé]

On 26/10/26 22:02, Fabiano Rosas wrote:
> 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.

Thank you Fabiano for checking and confirming :)