| Message ID | 20120912055706.GX12554@truffula.fritz.box |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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 >
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;