Re: [Qemu-devel] [PATCH] cpu_physical_memory_write_rom() needs to do TB invalidates

[David Gibson <dwg@au1.ibm.com>]

On Wed, Aug 22, 2012 at 07:55:31AM +0200, Alexander Graf wrote:
> 
> On 22.08.2012, at 06:59, David Gibson wrote:
> 
> > cpu_physical_memory_write_rom(), despite the name, can also be used to
> > write images into RAM - and will often be used that way if the machine
> > uses load_image_targphys() into RAM addresses.
> > 
> > However, cpu_physical_memory_write_rom(), unlike cpu_physical_memory_rw()
> > does invalidate any cached TBs which might be affected by the region
> > written.
> > 
> > This was breaking reset (under full emu) on the pseries machine - we loaded
> > our firmware image into RAM, and while executing it rewrite the code at
> > the entry point (correctly causing a TB invalidate/refresh).  When we
> > reset the firmware image was reloaded, but the TB from the rewrite was
> > still active and caused us to get an illegal instruction trap.
> > 
> > This patch fixes the bug by duplicating the tb invalidate code from
> > cpu_physical_memory_rw() in cpu_physical_memory_write_rom().
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > exec.c |    7 +++++++
> > 1 file changed, 7 insertions(+)
> > 
> > diff --git a/exec.c b/exec.c
> > index 5834766..eff40d7 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -3523,6 +3523,13 @@ void cpu_physical_memory_write_rom(target_phys_addr_t addr,
> >             /* ROM/RAM case */
> >             ptr = qemu_get_ram_ptr(addr1);
> >             memcpy(ptr, buf, l);
> > +            if (!cpu_physical_memory_is_dirty(addr1)) {
> > +                /* invalidate code */
> > +                tb_invalidate_phys_page_range(addr1, addr1 + l, 0);
> > +                /* set dirty bit */
> > +                cpu_physical_memory_set_dirty_flags(
> > +                    addr1, (0xff & ~CODE_DIRTY_FLAG));
> > +            }
> 
> Can't we just call cpu_physical_memory_rw in the RAM case? The
> function only tries to not do MMIO accesses on ROM pages, right?

Maybe.  It's not clear at all to me what cases
cpu_physical_memory_write_rom() is supposed to be for, as opposed to
just using cpu_physical_memory_rw().

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson