[Qemu-devel] [0/2] Bugfixes for 1.2 stable series

[Qemu-devel] [0/2] Bugfixes for 1.2 stable series

[David Gibson]

Here are a couple of pending bugfixes which should go into the 1.2
stable series.  2/2 is the fix for missing TB invalidates from
cpu_physical_memory_write_rom() (which is invoked by
load_image_targphys()) which I have sent before.  1/2 is a new fix
where FD_ISSET can be called with a negative fd, which has undefined
behaviour.

[Qemu-devel] [PATCH 1/2] qemu-char: BUGFIX, don't call FD_ISSET with negative fd

[David Gibson]

tcp_chr_connect(), unlike for example udp_chr_update_read_handler() does
not check if the fd it is using is valid (>= 0) before passing it to
qemu_set_fd_handler2().  If using e.g. a TCP serial port, which is not
initially connected, this can result in -1 being passed to FD_ISSET, which
has undefined behaviour.  On x86 it seems to harmlessly return 0, but on
PowerPC, it causes a fortify buffer overflow error to be thrown.

This patch fixes this by putting an extra test in tcp_chr_connect(), and
also adds an assert qemu_set_fd_handler2() to catch other such errors on
all platforms, rather than just some.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 iohandler.c |    2 ++
 qemu-char.c |    6 ++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/iohandler.c b/iohandler.c
index dea4355..a2d871b 100644
--- a/iohandler.c
+++ b/iohandler.c
@@ -56,6 +56,8 @@ int qemu_set_fd_handler2(int fd,
 {
     IOHandlerRecord *ioh;
 
+    assert(fd >= 0);
+
     if (!fd_read && !fd_write) {
         QLIST_FOREACH(ioh, &io_handlers, next) {
             if (ioh->fd == fd) {
diff --git a/qemu-char.c b/qemu-char.c
index 398baf1..73e48ff 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2332,8 +2332,10 @@ static void tcp_chr_connect(void *opaque)
     TCPCharDriver *s = chr->opaque;
 
     s->connected = 1;
-    qemu_set_fd_handler2(s->fd, tcp_chr_read_poll,
-                         tcp_chr_read, NULL, chr);
+    if (s->fd >= 0) {
+        qemu_set_fd_handler2(s->fd, tcp_chr_read_poll,
+                             tcp_chr_read, NULL, chr);
+    }
     qemu_chr_generic_open(chr);
 }
 
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 1/2] qemu-char: BUGFIX, don't call FD_ISSET with negative fd

[Anthony Liguori]

David Gibson <david@gibson.dropbear.id.au> writes:

> tcp_chr_connect(), unlike for example udp_chr_update_read_handler() does
> not check if the fd it is using is valid (>= 0) before passing it to
> qemu_set_fd_handler2().  If using e.g. a TCP serial port, which is not
> initially connected, this can result in -1 being passed to FD_ISSET, which
> has undefined behaviour.  On x86 it seems to harmlessly return 0, but on
> PowerPC, it causes a fortify buffer overflow error to be thrown.
>
> This patch fixes this by putting an extra test in tcp_chr_connect(), and
> also adds an assert qemu_set_fd_handler2() to catch other such errors on
> all platforms, rather than just some.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Applied. Thanks.

Regards,

Anthony Liguori


> ---
>  iohandler.c |    2 ++
>  qemu-char.c |    6 ++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/iohandler.c b/iohandler.c
> index dea4355..a2d871b 100644
> --- a/iohandler.c
> +++ b/iohandler.c
> @@ -56,6 +56,8 @@ int qemu_set_fd_handler2(int fd,
>  {
>      IOHandlerRecord *ioh;
>  
> +    assert(fd >= 0);
> +
>      if (!fd_read && !fd_write) {
>          QLIST_FOREACH(ioh, &io_handlers, next) {
>              if (ioh->fd == fd) {
> diff --git a/qemu-char.c b/qemu-char.c
> index 398baf1..73e48ff 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2332,8 +2332,10 @@ static void tcp_chr_connect(void *opaque)
>      TCPCharDriver *s = chr->opaque;
>  
>      s->connected = 1;
> -    qemu_set_fd_handler2(s->fd, tcp_chr_read_poll,
> -                         tcp_chr_read, NULL, chr);
> +    if (s->fd >= 0) {
> +        qemu_set_fd_handler2(s->fd, tcp_chr_read_poll,
> +                             tcp_chr_read, NULL, chr);
> +    }
>      qemu_chr_generic_open(chr);
>  }
>  
> -- 
> 1.7.10.4

Re: [Qemu-devel] [PATCH 1/2] qemu-char: BUGFIX, don't call FD_ISSET with negative fd

[David Gibson]

On Mon, Sep 17, 2012 at 01:24:51PM -0500, Anthony Liguori wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
> > tcp_chr_connect(), unlike for example udp_chr_update_read_handler() does
> > not check if the fd it is using is valid (>= 0) before passing it to
> > qemu_set_fd_handler2().  If using e.g. a TCP serial port, which is not
> > initially connected, this can result in -1 being passed to FD_ISSET, which
> > has undefined behaviour.  On x86 it seems to harmlessly return 0, but on
> > PowerPC, it causes a fortify buffer overflow error to be thrown.
> >
> > This patch fixes this by putting an extra test in tcp_chr_connect(), and
> > also adds an assert qemu_set_fd_handler2() to catch other such errors on
> > all platforms, rather than just some.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Applied. Thanks.

Excellent.

Fwiw, I think this one should go into the stable branch, too.

-- 
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

Re: [Qemu-devel] [PATCH 1/2] qemu-char: BUGFIX, don't call FD_ISSET with negative fd

[Andreas Färber]

Am 18.09.2012 02:08, schrieb David Gibson:
> On Mon, Sep 17, 2012 at 01:24:51PM -0500, Anthony Liguori wrote:
>> David Gibson <david@gibson.dropbear.id.au> writes:
>>
>>> tcp_chr_connect(), unlike for example udp_chr_update_read_handler() does
>>> not check if the fd it is using is valid (>= 0) before passing it to
>>> qemu_set_fd_handler2().  If using e.g. a TCP serial port, which is not
>>> initially connected, this can result in -1 being passed to FD_ISSET, which
>>> has undefined behaviour.  On x86 it seems to harmlessly return 0, but on
>>> PowerPC, it causes a fortify buffer overflow error to be thrown.
>>>
>>> This patch fixes this by putting an extra test in tcp_chr_connect(), and
>>> also adds an assert qemu_set_fd_handler2() to catch other such errors on
>>> all platforms, rather than just some.
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>
>> Applied. Thanks.
> 
> Excellent.
> 
> Fwiw, I think this one should go into the stable branch, too.

...which you indicate by cc'ing qemu-stable since that is not handled by
Anthony himself.

Queued for stable-0.15.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/2] qemu-char: BUGFIX, don't call FD_ISSET with negative fd

[David Gibson]

On Tue, Sep 18, 2012 at 01:29:04PM +0200, Andreas Färber wrote:
> Am 18.09.2012 02:08, schrieb David Gibson:
> > On Mon, Sep 17, 2012 at 01:24:51PM -0500, Anthony Liguori wrote:
> >> David Gibson <david@gibson.dropbear.id.au> writes:
> >>
> >>> tcp_chr_connect(), unlike for example udp_chr_update_read_handler() does
> >>> not check if the fd it is using is valid (>= 0) before passing it to
> >>> qemu_set_fd_handler2().  If using e.g. a TCP serial port, which is not
> >>> initially connected, this can result in -1 being passed to FD_ISSET, which
> >>> has undefined behaviour.  On x86 it seems to harmlessly return 0, but on
> >>> PowerPC, it causes a fortify buffer overflow error to be thrown.
> >>>
> >>> This patch fixes this by putting an extra test in tcp_chr_connect(), and
> >>> also adds an assert qemu_set_fd_handler2() to catch other such errors on
> >>> all platforms, rather than just some.
> >>>
> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >>
> >> Applied. Thanks.
> > 
> > Excellent.
> > 
> > Fwiw, I think this one should go into the stable branch, too.
> 
> ...which you indicate by cc'ing qemu-stable since that is not handled by
> Anthony himself.

Ah, sorry, I was not aware.  Noted for the future.

> Queued for stable-0.15.

Thanks.

-- 
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

[Qemu-devel] [PATCH 2/2] cpu_physical_memory_write_rom() needs to do TB invalidates

[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
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;
-- 
1.7.10.4

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

[Andreas Färber]

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.

Andreas

> 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;
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[David Gibson]

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;
-- 
1.7.10.4

-- 
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

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

[Pavel Hrdina]

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

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

[Anthony Liguori]

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

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

[Pavel Hrdina]

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

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

[Anthony Liguori]

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

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

[Pavel Hrdina]

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
>