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

[Qemu-devel] [PATCH] 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] cpu_physical_memory_write_rom() needs to do TB invalidates

[Alexander Graf]

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

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

[David Gibson]

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

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

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

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

[Alexander Graf]

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

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

[David Gibson]

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.

-- 
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] cpu_physical_memory_write_rom() needs to do TB invalidates

[Alexander Graf]

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

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

[Alexander Graf]

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

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2414 bytes --]

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2813 bytes --]

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.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3466 bytes --]

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.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Alexander Graf]

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

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

[Avi Kivity]

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.


-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] [PATCH] 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() 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(+)

I've sent this before, and there was some discussion about the
details, but as far as I can tell the conclusion was that this patch
was as good as any, at least for fixing the existing bug in 1.2 /
stable.  However, it still hasn't been merged.

Please apply for stable as well as mainline.

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