| Message ID | 1345611560-8290-1-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
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
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().
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
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.
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
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
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
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.
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.
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
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.
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;
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(+)