pflash devices should mark the memory region dirty and invalidate TBs
after directly writing to the RAM backing the ROM device.

Note that pflash_cfi01_get_memory() is used by several machine types to
populate ROM contents directly.  Callers are untouched by this patch
because they only modify memory before the guest is started.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/pflash_cfi01.c | 5 +++++
 hw/block/pflash_cfi02.c | 8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index bffb4c40e7..5301c11c18 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -446,6 +446,7 @@ static inline void pflash_data_write(pflash_t *pfl, hwaddr offset,
         break;
     }
 
+    memory_region_flush_rom_device(&pfl->mem, offset, width);
 }
 
 static void pflash_write(pflash_t *pfl, hwaddr offset,
@@ -482,6 +483,8 @@ static void pflash_write(pflash_t *pfl, hwaddr offset,
             if (!pfl->ro) {
                 memset(p + offset, 0xff, pfl->sector_len);
                 pflash_update(pfl, offset, pfl->sector_len);
+                memory_region_flush_rom_device(&pfl->mem, offset,
+                                               pfl->sector_len);
             } else {
                 pfl->status |= 0x20; /* Block erase error */
             }
@@ -763,6 +766,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
             error_setg(errp, "failed to read the initial flash content");
             return;
         }
+
+        memory_region_flush_rom_device(&pfl->mem, 0, total_len);
     }
 
     /* Default to devices being used at their maximum device width. This was
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 0f8b7b8c7b..d04572eca4 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -378,6 +378,8 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
                     pflash_update(pfl, offset, 4);
                     break;
                 }
+
+                memory_region_flush_rom_device(&pfl->orig_mem, offset, width);
             }
             pfl->status = 0x00 | ~(value & 0x80);
             /* Let's pretend write is immediate */
@@ -426,6 +428,8 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
             if (!pfl->ro) {
                 memset(pfl->storage, 0xFF, pfl->chip_len);
                 pflash_update(pfl, 0, pfl->chip_len);
+                memory_region_flush_rom_device(&pfl->orig_mem, 0,
+                                               pfl->chip_len);
             }
             pfl->status = 0x00;
             /* Let's wait 5 seconds before chip erase is done */
@@ -441,6 +445,8 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
             if (!pfl->ro) {
                 memset(p + offset, 0xFF, pfl->sector_len);
                 pflash_update(pfl, offset, pfl->sector_len);
+                memory_region_flush_rom_device(&pfl->orig_mem, offset,
+                                               pfl->sector_len);
             }
             pfl->status = 0x00;
             /* Let's wait 1/2 second before sector erase is done */
@@ -590,6 +596,8 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
             error_setg(errp, "failed to read the initial flash content");
             return;
         }
+
+        memory_region_flush_rom_device(&pfl->orig_mem, 0, chip_len);
     }
 
     pflash_setup_mappings(pfl);
-- 
2.20.1

Hi Stefan,

On 1/23/19 10:22 PM, Stefan Hajnoczi wrote:
> pflash devices should mark the memory region dirty and invalidate TBs
> after directly writing to the RAM backing the ROM device.
> 
> Note that pflash_cfi01_get_memory() is used by several machine types to
> populate ROM contents directly.  Callers are untouched by this patch
> because they only modify memory before the guest is started.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 5 +++++
>  hw/block/pflash_cfi02.c | 8 ++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index bffb4c40e7..5301c11c18 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -446,6 +446,7 @@ static inline void pflash_data_write(pflash_t *pfl, hwaddr offset,
>          break;
>      }
>  
> +    memory_region_flush_rom_device(&pfl->mem, offset, width);
>  }
>  
>  static void pflash_write(pflash_t *pfl, hwaddr offset,
> @@ -482,6 +483,8 @@ static void pflash_write(pflash_t *pfl, hwaddr offset,
>              if (!pfl->ro) {
>                  memset(p + offset, 0xff, pfl->sector_len);
>                  pflash_update(pfl, offset, pfl->sector_len);
> +                memory_region_flush_rom_device(&pfl->mem, offset,
> +                                               pfl->sector_len);
>              } else {
>                  pfl->status |= 0x20; /* Block erase error */
>              }
> @@ -763,6 +766,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>              error_setg(errp, "failed to read the initial flash content");
>              return;
>          }
> +
> +        memory_region_flush_rom_device(&pfl->mem, 0, total_len);

The device realize() is also executed before the guest is started, is
this call really necessary?

>      }
>  
>      /* Default to devices being used at their maximum device width. This was
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 0f8b7b8c7b..d04572eca4 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -378,6 +378,8 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>                      pflash_update(pfl, offset, 4);
>                      break;
>                  }
> +
> +                memory_region_flush_rom_device(&pfl->orig_mem, offset, width);
>              }
>              pfl->status = 0x00 | ~(value & 0x80);
>              /* Let's pretend write is immediate */
> @@ -426,6 +428,8 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>              if (!pfl->ro) {
>                  memset(pfl->storage, 0xFF, pfl->chip_len);
>                  pflash_update(pfl, 0, pfl->chip_len);
> +                memory_region_flush_rom_device(&pfl->orig_mem, 0,
> +                                               pfl->chip_len);
>              }
>              pfl->status = 0x00;
>              /* Let's wait 5 seconds before chip erase is done */
> @@ -441,6 +445,8 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>              if (!pfl->ro) {
>                  memset(p + offset, 0xFF, pfl->sector_len);
>                  pflash_update(pfl, offset, pfl->sector_len);
> +                memory_region_flush_rom_device(&pfl->orig_mem, offset,
> +                                               pfl->sector_len);
>              }
>              pfl->status = 0x00;
>              /* Let's wait 1/2 second before sector erase is done */
> @@ -590,6 +596,8 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>              error_setg(errp, "failed to read the initial flash content");
>              return;
>          }
> +
> +        memory_region_flush_rom_device(&pfl->orig_mem, 0, chip_len);

Ditto.

>      }
>  
>      pflash_setup_mappings(pfl);
> 

On Thu, Jan 24, 2019 at 12:11:55PM +0100, Philippe Mathieu-Daudé wrote:
> On 1/23/19 10:22 PM, Stefan Hajnoczi wrote:
> > pflash devices should mark the memory region dirty and invalidate TBs
> > after directly writing to the RAM backing the ROM device.
> > 
> > Note that pflash_cfi01_get_memory() is used by several machine types to
> > populate ROM contents directly.  Callers are untouched by this patch
> > because they only modify memory before the guest is started.
...
> > @@ -763,6 +766,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
> >              error_setg(errp, "failed to read the initial flash content");
> >              return;
> >          }
> > +
> > +        memory_region_flush_rom_device(&pfl->mem, 0, total_len);
> 
> The device realize() is also executed before the guest is started, is
> this call really necessary?

My rationale was that machine init only happens before the guest is
started while ->realize() is called by hotplug too.

That said, can pflash devices be hotplugged?

If you guys prefer not flushing from pflash ->realize() then I'll drop
it.

Stefan

On Fri, 25 Jan 2019 at 10:28, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> On Thu, Jan 24, 2019 at 12:11:55PM +0100, Philippe Mathieu-Daudé wrote:
> > The device realize() is also executed before the guest is started, is
> > this call really necessary?
>
> My rationale was that machine init only happens before the guest is
> started while ->realize() is called by hotplug too.

Yes, but before realize the flash memory can't actually be
visible to the guest, so there's no need to invalidate anything.

> That said, can pflash devices be hotplugged?

I guess you could have one inside a hotplugged device in theory;
they can't be directly hotpluggged. (I bet that in practice
there are leaks though if you did do that, and we don't have
any devices that do so today.)

> If you guys prefer not flushing from pflash ->realize() then I'll drop
> it.

I think it makes more sense not to do a flush in the realize method.

thanks
-- PMM

On Fri, Jan 25, 2019 at 10:36:17AM +0000, Peter Maydell wrote:
> On Fri, 25 Jan 2019 at 10:28, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >
> > On Thu, Jan 24, 2019 at 12:11:55PM +0100, Philippe Mathieu-Daudé wrote:
> > > The device realize() is also executed before the guest is started, is
> > > this call really necessary?
> >
> > My rationale was that machine init only happens before the guest is
> > started while ->realize() is called by hotplug too.
> 
> Yes, but before realize the flash memory can't actually be
> visible to the guest, so there's no need to invalidate anything.
> 
> > That said, can pflash devices be hotplugged?
> 
> I guess you could have one inside a hotplugged device in theory;
> they can't be directly hotpluggged. (I bet that in practice
> there are leaks though if you did do that, and we don't have
> any devices that do so today.)
> 
> > If you guys prefer not flushing from pflash ->realize() then I'll drop
> > it.
> 
> I think it makes more sense not to do a flush in the realize method.

Okay, I'll resend this patch.

Stefan

On 1/25/19 4:37 PM, Stefan Hajnoczi wrote:
> On Fri, Jan 25, 2019 at 10:36:17AM +0000, Peter Maydell wrote:
>> On Fri, 25 Jan 2019 at 10:28, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>>
>>> On Thu, Jan 24, 2019 at 12:11:55PM +0100, Philippe Mathieu-Daudé wrote:
>>>> The device realize() is also executed before the guest is started, is
>>>> this call really necessary?
>>>
>>> My rationale was that machine init only happens before the guest is
>>> started while ->realize() is called by hotplug too.
>>
>> Yes, but before realize the flash memory can't actually be
>> visible to the guest, so there's no need to invalidate anything.
>>
>>> That said, can pflash devices be hotplugged?
>>
>> I guess you could have one inside a hotplugged device in theory;
>> they can't be directly hotpluggged. (I bet that in practice
>> there are leaks though if you did do that, and we don't have
>> any devices that do so today.)

Can we assume pflash aren't hotpluggable until someone find an use case
and is willing to work on it? I can't find any practical example.

>>> If you guys prefer not flushing from pflash ->realize() then I'll drop
>>> it.
>>
>> I think it makes more sense not to do a flush in the realize method.
> 
> Okay, I'll resend this patch.

Thanks!

Phil.