Patchwork [2/2] cpu_physical_memory_write_rom() needs to do TB invalidates

login
register
mail settings
Submitter David Gibson
Date Sept. 12, 2012, 5:57 a.m.
Message ID <20120912055706.GX12554@truffula.fritz.box>
Download mbox | patch
Permalink /patch/183250/
State New
Headers show

Comments

David Gibson - Sept. 12, 2012, 5:57 a.m.
On Mon, Sep 10, 2012 at 03:27:45PM +0200, Andreas Färber wrote:
> Am 10.09.2012 04:30, schrieb David Gibson:
> > 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
> 
> "doesn't"?
> 
> Otherwise doesn't look wrong.

Oops, comment updated.

From 6b913afaf83f52ee787271827c84b492e8ac5895 Mon Sep 17 00:00:00 2001
From: David Gibson <david@gibson.dropbear.id.au>
Date: Wed, 22 Aug 2012 14:58:04 +1000
Subject: [PATCH] cpu_physical_memory_write_rom() needs to do TB invalidates

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() doesn't 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(+)
Pavel Hrdina - Oct. 1, 2012, 1:43 p.m.
On 09/12/2012 07:57 AM, David Gibson wrote:
> On Mon, Sep 10, 2012 at 03:27:45PM +0200, Andreas Färber wrote:
>> Am 10.09.2012 04:30, schrieb David Gibson:
>>> 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
>> "doesn't"?
>>
>> Otherwise doesn't look wrong.
> Oops, comment updated.
>
>  From 6b913afaf83f52ee787271827c84b492e8ac5895 Mon Sep 17 00:00:00 2001
> From: David Gibson <david@gibson.dropbear.id.au>
> Date: Wed, 22 Aug 2012 14:58:04 +1000
> Subject: [PATCH] cpu_physical_memory_write_rom() needs to do TB invalidates
>
> 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() doesn't 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));
> +            }
>               qemu_put_ram_ptr(ptr);
>           }
>           len -= l;
Hi,

this patch breaks Windows XP guest at all. Windows XP boot ends in loob 
by restarting itself after time-out expires in windows advanced boot 
options.

I started the guest using this command-line:

./x86_64-softmmu/qemu-system-x86_64 -m 2G -drive 
file=/data/data-shared/images/winxp-test.img -vnc 0.0.0.0:0

Pavel
Anthony Liguori - Oct. 1, 2012, 2:28 p.m.
Pavel Hrdina <phrdina@redhat.com> writes:

> On 09/12/2012 07:57 AM, David Gibson wrote:
>> On Mon, Sep 10, 2012 at 03:27:45PM +0200, Andreas Färber wrote:
>>> Am 10.09.2012 04:30, schrieb David Gibson:
>>>> 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
>>> "doesn't"?
>>>
>>> Otherwise doesn't look wrong.
>> Oops, comment updated.
>>
>>  From 6b913afaf83f52ee787271827c84b492e8ac5895 Mon Sep 17 00:00:00 2001
>> From: David Gibson <david@gibson.dropbear.id.au>
>> Date: Wed, 22 Aug 2012 14:58:04 +1000
>> Subject: [PATCH] cpu_physical_memory_write_rom() needs to do TB invalidates
>>
>> 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() doesn't 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));
>> +            }
>>               qemu_put_ram_ptr(ptr);
>>           }
>>           len -= l;
> Hi,
>
> this patch breaks Windows XP guest at all. Windows XP boot ends in loob 
> by restarting itself after time-out expires in windows advanced boot 
> options.
>
> I started the guest using this command-line:
>
> ./x86_64-softmmu/qemu-system-x86_64 -m 2G -drive 
> file=/data/data-shared/images/winxp-test.img -vnc 0.0.0.0:0

Does changing the tb_invalidate_phys_page_range() call to:

tb_invalidate_phys_page_range(addr1, addr1 + MAX(l, TARGET_PAGE_SIZE), 0);

The dirty flag is being reset for the full page but we're potentially
only invalidating a subset of TBs that occur on the page.

Regards,

Anthony Liguori


>
> Pavel
Pavel Hrdina - Oct. 1, 2012, 3:34 p.m.
On 10/01/2012 04:28 PM, Anthony Liguori wrote:
> Pavel Hrdina <phrdina@redhat.com> writes:
>
>> On 09/12/2012 07:57 AM, David Gibson wrote:
>>> On Mon, Sep 10, 2012 at 03:27:45PM +0200, Andreas Färber wrote:
>>>> Am 10.09.2012 04:30, schrieb David Gibson:
>>>>> 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
>>>> "doesn't"?
>>>>
>>>> Otherwise doesn't look wrong.
>>> Oops, comment updated.
>>>
>>>   From 6b913afaf83f52ee787271827c84b492e8ac5895 Mon Sep 17 00:00:00 2001
>>> From: David Gibson <david@gibson.dropbear.id.au>
>>> Date: Wed, 22 Aug 2012 14:58:04 +1000
>>> Subject: [PATCH] cpu_physical_memory_write_rom() needs to do TB invalidates
>>>
>>> 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() doesn't 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));
>>> +            }
>>>                qemu_put_ram_ptr(ptr);
>>>            }
>>>            len -= l;
>> Hi,
>>
>> this patch breaks Windows XP guest at all. Windows XP boot ends in loob
>> by restarting itself after time-out expires in windows advanced boot
>> options.
>>
>> I started the guest using this command-line:
>>
>> ./x86_64-softmmu/qemu-system-x86_64 -m 2G -drive
>> file=/data/data-shared/images/winxp-test.img -vnc 0.0.0.0:0
> Does changing the tb_invalidate_phys_page_range() call to:
>
> tb_invalidate_phys_page_range(addr1, addr1 + MAX(l, TARGET_PAGE_SIZE), 0);
>
> The dirty flag is being reset for the full page but we're potentially
> only invalidating a subset of TBs that occur on the page.
>
> Regards,
>
> Anthony Liguori
>

No, it doesn't fix this bug.

Pavel

>> Pavel
Anthony Liguori - Oct. 1, 2012, 4:21 p.m.
Pavel Hrdina <phrdina@redhat.com> writes:

> On 10/01/2012 04:28 PM, Anthony Liguori wrote:
>> Pavel Hrdina <phrdina@redhat.com> writes:
>>
>>> On 09/12/2012 07:57 AM, David Gibson wrote:
>>>> On Mon, Sep 10, 2012 at 03:27:45PM +0200, Andreas Färber wrote:
>>>>> Am 10.09.2012 04:30, schrieb David Gibson:
>>>>>> 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
>>>>> "doesn't"?
>>>>>
>>>>> Otherwise doesn't look wrong.
>>>> Oops, comment updated.
>>>>
>>>>   From 6b913afaf83f52ee787271827c84b492e8ac5895 Mon Sep 17 00:00:00 2001
>>>> From: David Gibson <david@gibson.dropbear.id.au>
>>>> Date: Wed, 22 Aug 2012 14:58:04 +1000
>>>> Subject: [PATCH] cpu_physical_memory_write_rom() needs to do TB invalidates
>>>>
>>>> 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() doesn't 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));
>>>> +            }
>>>>                qemu_put_ram_ptr(ptr);
>>>>            }
>>>>            len -= l;
>>> Hi,
>>>
>>> this patch breaks Windows XP guest at all. Windows XP boot ends in loob
>>> by restarting itself after time-out expires in windows advanced boot
>>> options.
>>>
>>> I started the guest using this command-line:
>>>
>>> ./x86_64-softmmu/qemu-system-x86_64 -m 2G -drive
>>> file=/data/data-shared/images/winxp-test.img -vnc 0.0.0.0:0
>> Does changing the tb_invalidate_phys_page_range() call to:
>>
>> tb_invalidate_phys_page_range(addr1, addr1 + MAX(l, TARGET_PAGE_SIZE), 0);
>>
>> The dirty flag is being reset for the full page but we're potentially
>> only invalidating a subset of TBs that occur on the page.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>
> No, it doesn't fix this bug.

Then I'm confused...  invalidating TBs should never have a functional
impact IIUC.  Are you confident in your bisection?  Reverting this patch
fixes the problem?

Regards,

Anthony Liguori

>
> Pavel
>
>>> Pavel
Pavel Hrdina - Oct. 1, 2012, 4:25 p.m.
On 10/01/2012 06:21 PM, Anthony Liguori wrote:
> Pavel Hrdina <phrdina@redhat.com> writes:
>
>> On 10/01/2012 04:28 PM, Anthony Liguori wrote:
>>> Pavel Hrdina <phrdina@redhat.com> writes:
>>>
>>>> On 09/12/2012 07:57 AM, David Gibson wrote:
>>>>> On Mon, Sep 10, 2012 at 03:27:45PM +0200, Andreas Färber wrote:
>>>>>> Am 10.09.2012 04:30, schrieb David Gibson:
>>>>>>> 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
>>>>>> "doesn't"?
>>>>>>
>>>>>> Otherwise doesn't look wrong.
>>>>> Oops, comment updated.
>>>>>
>>>>>    From 6b913afaf83f52ee787271827c84b492e8ac5895 Mon Sep 17 00:00:00 2001
>>>>> From: David Gibson <david@gibson.dropbear.id.au>
>>>>> Date: Wed, 22 Aug 2012 14:58:04 +1000
>>>>> Subject: [PATCH] cpu_physical_memory_write_rom() needs to do TB invalidates
>>>>>
>>>>> 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() doesn't 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));
>>>>> +            }
>>>>>                 qemu_put_ram_ptr(ptr);
>>>>>             }
>>>>>             len -= l;
>>>> Hi,
>>>>
>>>> this patch breaks Windows XP guest at all. Windows XP boot ends in loob
>>>> by restarting itself after time-out expires in windows advanced boot
>>>> options.
>>>>
>>>> I started the guest using this command-line:
>>>>
>>>> ./x86_64-softmmu/qemu-system-x86_64 -m 2G -drive
>>>> file=/data/data-shared/images/winxp-test.img -vnc 0.0.0.0:0
>>> Does changing the tb_invalidate_phys_page_range() call to:
>>>
>>> tb_invalidate_phys_page_range(addr1, addr1 + MAX(l, TARGET_PAGE_SIZE), 0);
>>>
>>> The dirty flag is being reset for the full page but we're potentially
>>> only invalidating a subset of TBs that occur on the page.
>>>
>>> Regards,
>>>
>>> Anthony Liguori
>>>
>> No, it doesn't fix this bug.
> Then I'm confused...  invalidating TBs should never have a functional
> impact IIUC.  Are you confident in your bisection?  Reverting this patch
> fixes the problem?
>
> Regards,
>
> Anthony Liguori

Yes I'm 100% sure that this commit cause this bug. I've tried to revert 
this patch and it works OK.

Pavel
>> Pavel
>>
>>>> Pavel
>

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;