| Message ID | CAAu8pHt6DjdtumFDT2+SoyEnDhyMJcnjV8ouAFhO4M17=J1_iA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 03/18/2012 11:51 AM, Blue Swirl wrote: > On Sun, Mar 18, 2012 at 09:44, Avi Kivity <avi@redhat.com> wrote: > > On 03/18/2012 04:01 AM, Mark Cave-Ayland wrote: > >> Hi Avi/Blue, > >> > >> I've just updated to git master and found that SPARC64 is broken > >> again; a git bisect shows the following commit causes this: > >> > >> > >> commit f3705d53296d78b14f5823472ae2add16a25a0a5 > >> Author: Avi Kivity <avi@redhat.com> > >> Date: Thu Mar 8 16:16:34 2012 +0200 > >> > >> memory: make phys_page_find() return an unadjusted section > >> > >> We'd like to store the section index in the iotlb, so we can't > >> adjust it before returning. Return an unadjusted section and > >> instead introduce section_addr(), which does the adjustment later. > >> > >> Signed-off-by: Avi Kivity <avi@redhat.com> > >> > >> > >> The symptom is that qemu-system-sparc64 segfaults immediately on > >> startup (note this is with an OpenBIOS image built from SVN r1048). > >> I've included a couple of backtraces below: > >> > > > > Please try the attached patch. > > I tried this approach instead, seems to work IMO, my patch is better. tlb_set_page() should not deal with offsets within a page. If you prefer your approach, I suggest masking the address up front in the beginning of tlb_set_page() instead. > (except Sparc32, Sparc64 > and PPC displays are still not refreshed correctly). Details about this please.
On Sun, Mar 18, 2012 at 10:31, Avi Kivity <avi@redhat.com> wrote: > On 03/18/2012 11:51 AM, Blue Swirl wrote: >> On Sun, Mar 18, 2012 at 09:44, Avi Kivity <avi@redhat.com> wrote: >> > On 03/18/2012 04:01 AM, Mark Cave-Ayland wrote: >> >> Hi Avi/Blue, >> >> >> >> I've just updated to git master and found that SPARC64 is broken >> >> again; a git bisect shows the following commit causes this: >> >> >> >> >> >> commit f3705d53296d78b14f5823472ae2add16a25a0a5 >> >> Author: Avi Kivity <avi@redhat.com> >> >> Date: Thu Mar 8 16:16:34 2012 +0200 >> >> >> >> memory: make phys_page_find() return an unadjusted section >> >> >> >> We'd like to store the section index in the iotlb, so we can't >> >> adjust it before returning. Return an unadjusted section and >> >> instead introduce section_addr(), which does the adjustment later. >> >> >> >> Signed-off-by: Avi Kivity <avi@redhat.com> >> >> >> >> >> >> The symptom is that qemu-system-sparc64 segfaults immediately on >> >> startup (note this is with an OpenBIOS image built from SVN r1048). >> >> I've included a couple of backtraces below: >> >> >> > >> > Please try the attached patch. >> >> I tried this approach instead, seems to work > > IMO, my patch is better. tlb_set_page() should not deal with offsets > within a page. It looks like all targets except Sparc32/64 mask the addresses before passing to tlb_set_page(), so I agree. > If you prefer your approach, I suggest masking the address up front in > the beginning of tlb_set_page() instead. > >> (except Sparc32, Sparc64 >> and PPC displays are still not refreshed correctly). > > Details about this please. Screen is not updated correctly, there are lines from previous screenful. Pressing ctrl-alt-1 refreshes the display. Perhaps dirty tracking is broken? VGA in x86 works. > > -- > error compiling committee.c: too many arguments to function >
On 18/03/12 10:51, Blue Swirl wrote: >>> (except Sparc32, Sparc64 >>> and PPC displays are still not refreshed correctly). >> >> Details about this please. > > Screen is not updated correctly, there are lines from previous > screenful. Pressing ctrl-alt-1 refreshes the display. Perhaps dirty > tracking is broken? VGA in x86 works. Is that using SDL? Do you see the same issue with VNC too? ATB, Mark.
On Sun, Mar 18, 2012 at 11:03, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > On 18/03/12 10:51, Blue Swirl wrote: > >>>> (except Sparc32, Sparc64 >>>> and PPC displays are still not refreshed correctly). >>> >>> >>> Details about this please. >> >> >> Screen is not updated correctly, there are lines from previous >> screenful. Pressing ctrl-alt-1 refreshes the display. Perhaps dirty >> tracking is broken? VGA in x86 works. > > > Is that using SDL? Do you see the same issue with VNC too? Yes to both. > > > ATB, > > Mark.
On 03/18/2012 12:51 PM, Blue Swirl wrote: > > > > IMO, my patch is better. tlb_set_page() should not deal with offsets > > within a page. > > It looks like all targets except Sparc32/64 mask the addresses before > passing to tlb_set_page(), so I agree. Ok. Commit my patch then? > > If you prefer your approach, I suggest masking the address up front in > > the beginning of tlb_set_page() instead. > > > >> (except Sparc32, Sparc64 > >> and PPC displays are still not refreshed correctly). > > > > Details about this please. > > Screen is not updated correctly, there are lines from previous > screenful. Pressing ctrl-alt-1 refreshes the display. Perhaps dirty > tracking is broken? VGA in x86 works. Ok, I see it. Will investigate.
On Sun, Mar 18, 2012 at 12:08, Avi Kivity <avi@redhat.com> wrote: > On 03/18/2012 12:51 PM, Blue Swirl wrote: >> > >> > IMO, my patch is better. tlb_set_page() should not deal with offsets >> > within a page. >> >> It looks like all targets except Sparc32/64 mask the addresses before >> passing to tlb_set_page(), so I agree. > > Ok. Commit my patch then? I sent a different patch which masks incoming address earlier and removes unnecessary masking in other places. >> > If you prefer your approach, I suggest masking the address up front in >> > the beginning of tlb_set_page() instead. >> > >> >> (except Sparc32, Sparc64 >> >> and PPC displays are still not refreshed correctly). >> > >> > Details about this please. >> >> Screen is not updated correctly, there are lines from previous >> screenful. Pressing ctrl-alt-1 refreshes the display. Perhaps dirty >> tracking is broken? VGA in x86 works. > > Ok, I see it. Will investigate. > > -- > error compiling committee.c: too many arguments to function >
On 03/18/2012 02:10 PM, Blue Swirl wrote: > On Sun, Mar 18, 2012 at 12:08, Avi Kivity <avi@redhat.com> wrote: > > On 03/18/2012 12:51 PM, Blue Swirl wrote: > >> > > >> > IMO, my patch is better. tlb_set_page() should not deal with offsets > >> > within a page. > >> > >> It looks like all targets except Sparc32/64 mask the addresses before > >> passing to tlb_set_page(), so I agree. > > > > Ok. Commit my patch then? > > I sent a different patch which masks incoming address earlier and > removes unnecessary masking in other places. My patch missed the "boot mode" thing. Does your patch fix it?
On Sun, Mar 18, 2012 at 12:13, Avi Kivity <avi@redhat.com> wrote: > On 03/18/2012 02:10 PM, Blue Swirl wrote: >> On Sun, Mar 18, 2012 at 12:08, Avi Kivity <avi@redhat.com> wrote: >> > On 03/18/2012 12:51 PM, Blue Swirl wrote: >> >> > >> >> > IMO, my patch is better. tlb_set_page() should not deal with offsets >> >> > within a page. >> >> >> >> It looks like all targets except Sparc32/64 mask the addresses before >> >> passing to tlb_set_page(), so I agree. >> > >> > Ok. Commit my patch then? >> >> I sent a different patch which masks incoming address earlier and >> removes unnecessary masking in other places. > > My patch missed the "boot mode" thing. Does your patch fix it? Yes. I also fixed both Sparc32 (which didn't have problems) and Sparc64. > > -- > error compiling committee.c: too many arguments to function >
diff --git a/exec.c b/exec.c index 8fd50a1..ad455be 100644 --- a/exec.c +++ b/exec.c @@ -1474,8 +1474,8 @@ static void breakpoint_invalidate(CPUArchState *env, target_ulong pc) || (section->mr->rom_device && section->mr->readable))) { return; } - ram_addr = (memory_region_get_ram_addr(section->mr) & TARGET_PAGE_MASK) - + section_addr(section, addr); + ram_addr = (memory_region_get_ram_addr(section->mr) + + section_addr(section, pc)) & TARGET_PAGE_MASK; tb_invalidate_phys_page_range(ram_addr, ram_addr + 1, 0); }