Patchwork cpu_physical_memory_write_rom() needs to do TB invalidates

login
register
mail settings
Submitter David Gibson
Date Aug. 22, 2012, 4:59 a.m.
Message ID <1345611560-8290-1-git-send-email-david@gibson.dropbear.id.au>
Download mbox | patch
Permalink /patch/179225/
State New
Headers show

Comments

David Gibson - Aug. 22, 2012, 4:59 a.m.
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(+)
Alexander Graf - Aug. 22, 2012, 5:55 a.m.
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?


Alex
David Gibson - Aug. 22, 2012, 5:57 a.m.
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().
Alexander Graf - Aug. 22, 2012, 6:02 a.m.
On 22.08.2012, at 07:57, David Gibson wrote:

> 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().

I can only guess, but the code looks to me as if it wants to be a nop on ROM pages, while basically doing cpu_physical_memory_rw for RAM pages. Usually in QEMU, every non-RAM page gets treated as MMIO which might eventually lead to machine checks.


Alex
David Gibson - Aug. 22, 2012, 6:10 a.m.
On Wed, Aug 22, 2012 at 08:02:11AM +0200, Alexander Graf wrote:
> 
> On 22.08.2012, at 07:57, David Gibson wrote:
> 
> > 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().
> 
> I can only guess, but the code looks to me as if it wants to be a
> nop on ROM pages, while basically doing cpu_physical_memory_rw for
> RAM pages. Usually in QEMU, every non-RAM page gets treated as MMIO
> which might eventually lead to machine checks.

Maybe.  Anthony, can you make a ruling on this, or tell me who can.  I
don't really care how I fix it, but it's definitely broken right now.
Alexander Graf - Aug. 22, 2012, 6:12 a.m.
On 22.08.2012, at 08:10, David Gibson wrote:

> On Wed, Aug 22, 2012 at 08:02:11AM +0200, Alexander Graf wrote:
>> 
>> On 22.08.2012, at 07:57, David Gibson wrote:
>> 
>>> 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().
>> 
>> I can only guess, but the code looks to me as if it wants to be a
>> nop on ROM pages, while basically doing cpu_physical_memory_rw for
>> RAM pages. Usually in QEMU, every non-RAM page gets treated as MMIO
>> which might eventually lead to machine checks.
> 
> Maybe.  Anthony, can you make a ruling on this, or tell me who can.  I
> don't really care how I fix it, but it's definitely broken right now.

Yeah, and for a 1.2 quick fix your original patch should be perfectly fine too.


Alex
Alexander Graf - Aug. 22, 2012, 6:31 a.m.
On 22.08.2012, at 08:10, David Gibson wrote:

> On Wed, Aug 22, 2012 at 08:02:11AM +0200, Alexander Graf wrote:
>> 
>> On 22.08.2012, at 07:57, David Gibson wrote:
>> 
>>> 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().
>> 
>> I can only guess, but the code looks to me as if it wants to be a
>> nop on ROM pages, while basically doing cpu_physical_memory_rw for
>> RAM pages. Usually in QEMU, every non-RAM page gets treated as MMIO
>> which might eventually lead to machine checks.
> 
> Maybe.  Anthony, can you make a ruling on this, or tell me who can.  I
> don't really care how I fix it, but it's definitely broken right now.

Also, does tb_invalidate_phys_page_range() do an icache flush on KVM?


Alex
Jan Kiszka - Aug. 22, 2012, 6:47 a.m.
On 2012-08-22 07:57, David Gibson wrote:
> 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().

write_rom ignores write protection - that you usually find on ROMs. That
makes no difference under KVM so far as there we lack read-only
sections. But that will be fixed soon, patches are on the list.

Jan
Jan Kiszka - Aug. 22, 2012, 7:05 a.m.
On 2012-08-22 08:47, Jan Kiszka wrote:
> On 2012-08-22 07:57, David Gibson wrote:
>> 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().
> 
> write_rom ignores write protection - that you usually find on ROMs. That
> makes no difference under KVM so far as there we lack read-only
> sections. But that will be fixed soon, patches are on the list.

In fact, it does make a difference also for KVM mode as
cpu_physical_memory_rw works from userspace while the limitation only
affects guest code running under KVM control.

Jan

PS: I'm still facing a bogus Mail-Followup-To tag in your postings,
David, thus you easily fall from the To list on reply.
David Gibson - Aug. 22, 2012, 11:38 a.m.
On Wed, Aug 22, 2012 at 09:05:52AM +0200, Jan Kiszka wrote:
> On 2012-08-22 08:47, Jan Kiszka wrote:
> > On 2012-08-22 07:57, David Gibson wrote:
> >> 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().
> > 
> > write_rom ignores write protection - that you usually find on ROMs. That
> > makes no difference under KVM so far as there we lack read-only
> > sections. But that will be fixed soon, patches are on the list.
> 
> In fact, it does make a difference also for KVM mode as
> cpu_physical_memory_rw works from userspace while the limitation only
> affects guest code running under KVM control.

Ok, so IIUC, that means we do need the cpu_physical_memory_write_rom()
version for load_image_targphys(), and so my original patch is
basically the right fix.

> Jan
> 
> PS: I'm still facing a bogus Mail-Followup-To tag in your postings,
> David, thus you easily fall from the To list on reply.

Ah, yes, thanks for the reminder.  I think I finally found the right
option to stop mutt from doing that.
Alexander Graf - Aug. 22, 2012, 11:47 a.m.
On 22.08.2012, at 13:38, David Gibson wrote:

> On Wed, Aug 22, 2012 at 09:05:52AM +0200, Jan Kiszka wrote:
>> On 2012-08-22 08:47, Jan Kiszka wrote:
>>> On 2012-08-22 07:57, David Gibson wrote:
>>>> 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().
>>> 
>>> write_rom ignores write protection - that you usually find on ROMs. That
>>> makes no difference under KVM so far as there we lack read-only
>>> sections. But that will be fixed soon, patches are on the list.
>> 
>> In fact, it does make a difference also for KVM mode as
>> cpu_physical_memory_rw works from userspace while the limitation only
>> affects guest code running under KVM control.
> 
> Ok, so IIUC, that means we do need the cpu_physical_memory_write_rom()
> version for load_image_targphys(), and so my original patch is
> basically the right fix.

Sure it is, I don't think anyone argued about that :). But it's duplicating code in a slow path. So my proposal was instead of doing the write manually in the "this is read-write RAM" case, just fall back to the known-to-work cpu_physical_memory_rw for those pages. That would make the rom function smaller, more obvious and duplicate less code.


Alex
Avi Kivity - Aug. 22, 2012, 1:09 p.m.
On 08/22/2012 02:47 PM, Alexander Graf wrote:
>> 
>> Ok, so IIUC, that means we do need the cpu_physical_memory_write_rom()
>> version for load_image_targphys(), and so my original patch is
>> basically the right fix.
> 
> Sure it is, I don't think anyone argued about that :). But it's duplicating code in a slow path. So my proposal was instead of doing the write manually in the "this is read-write RAM" case, just fall back to the known-to-work cpu_physical_memory_rw for those pages. That would make the rom function smaller, more obvious and duplicate less code.

I think there were patches (from Xen) extracting that snippet into a helper.

Patch

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));
+            }
             qemu_put_ram_ptr(ptr);
         }
         len -= l;