Re: [PATCH v1 0/9] hw/mos6522: VIA timer emulation fixes and improvements
On Mon, 22 Nov 2021, Peter Maydell wrote: > On Sat, 20 Nov 2021 at 23:40, Finn Thain wrote: > > Anyway, thanks for taking the time to write. A competent reviewer has to > > do much more than that, but I'm not paying for competence so I suppose I'm > > asking too much. > > Please dial back the aggressive tone here, Finn: this kind of > thing is way out of line. We're all trying to help improve QEMU here, > and sniping at Mark is not constructive. > Peter, you seem to have misunderstood what I wrote. What I said was not sniping. "Incompetent" was my conclusion after I judiciously rejected "malicious". Here's what I mean by incompetent. CONTRIBUTOR: Here's a patch. MAINTAINER: I personally don't like that pattern. End of story. CONTRIBUTOR: I don't think I'll contribute further to this project. [Everyone loses.] Now, here's what I would consider "competent": CONTRIBUTOR: Here's a patch. MAINTAINER: That pattern (I've quoted it to help further the discussion) is widely deprecated. You should use a different pattern instead. [Or read this reference. Or refer to this code.] CONTRIBUTOR: OK, I see that this really is a problem, and I see that there really is a better way. However, the antipattern is already part of existing code, and my changes don't worsen the problem, and don't require that the problem persist. MAINTAINER: You're right. My bad (I'm new to this). Since I never bothered to fix the existing antipattern, and no-one else thought it was worth fixing either, clearly it's not that important, and I should not have sought to veto your work, which is substantially unrelated, and beneficial either way. CONTRIBUTOR: No problem. [Everyone wins.] Finally, here's the background for you to ponder, in case you would like to intervene to produce a better outcome. (I think you are potentially well positioned for that.) https://lore.kernel.org/qemu-devel/cover.1629799776.git.fth...@linux-m68k.org/ https://lore.kernel.org/qemu-devel/cover.1632437396.git.fth...@linux-m68k.org/
Re: [PATCH v1 0/9] hw/mos6522: VIA timer emulation fixes and improvements
On Sat, 20 Nov 2021, Mark Cave-Ayland wrote: > On 19/11/2021 08:39, Finn Thain wrote: > > > On Thu, 18 Nov 2021, Mark Cave-Ayland wrote: > > > >> > >> Hi Finn, > >> > >> I've not forgotten about this series - we're now in 6.2 freeze, but it's > >> on my TODO list to revisit in the next development cycle this along with > >> the ESP stress-ng changes which I've also been looking at. As mentioned > >> in my previous reviews the patch will need some further analysis: > >> particularly the logic in mos6522_read() that can generate a spurious > >> interrupt on a register read needs to be removed, > > > > If mos6522 fails to raise its IRQ then the counter value observed by the > > guest will not make sense. This relationship was explained in the > > description of patch 8. If you have a problem with that patch, please > > reply there so that your misapprehension can be placed in context. > > It is the existing code in mos6522_read() which is doing the wrong thing here, > which I mentioned in > https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg02883.html. > How disingenous. I responded to that message 2 months ago and you completely ignored my response. Basically, you found a bug in your own modified version of mainline code, and you claimed that this is somehow relevant to this patch series. (Apparently you failed to find that bug in my code.) Once again, if you have an objection to existing code in mainline QEMU, please take it up with the author of that code (commit cd8843ff25) which is Laurent. This patch series is a separate issue. It doesn't add anything objectionable (commit cd8843ff25 was not objected to), it fixes bugs, improves emulation fidelity, improves performance and reduces guest malfunctions. > > That is true, but as per the link above there is an existing bug in the > mos6522 device, and the patchset builds on this in its current form, > including adding a state field which shouldn't be required. > The state enum is required for the oneshot feature (patch 9). It is also needed to produce the correct relationship between irq and counter (patch 8), and between interrupt flag set and clear operations. Finally, it is also useful for debugging purposes. > From looking at mac_read_clk() presumably the problem here is that the > timer IRQ fires on 0 rather than on 0x when it overflows? Guests depend on the correct relationship between timer irq flag and timer counter value. If QEMU can't get that right, the Linux clocksource can't work without race conditions. This problem is almost certain to affect other guests too. You are being foolish to insist that this is somehow a Linux quirk. > If so, this should be a very small and simple patch. Once these 2 fixes > are in place, it will be much easier to test the remaining changes. > > >> I realized that I could easily cross-compile a 5.14 kernel to test > >> this theory with the test root image and .config you supplied at > >> https://gitlab.com/qemu-project/qemu/-/issues/611 using the QEMU > >> docker-m68k-cross image to avoid having to build a complete toolchain > >> by hand. The kernel builds successfully using this method, but during > >> boot it hangs sending the first SCSI CDB to the ESP device, failing > >> to send the last byte using PDMA. > >> > >> Are there known issues cross-compiling an m68k kernel on an x86 host? > > > > Not that I'm aware of. > > > >> Or are your current kernels being built from a separate branch > >> outside of mainline Linux git? > >> > > I use mainline Linux when testing QEMU. I provided a mainline build, > > attached to the same bug report, so you don't have to build it. > > The problem here is that I have no way to reproduce your results and > test any patches other than to try and build a kernel with your extra > warning and run it. Nonsense. Any programmer can easily observe the gettimeofday problem. Just run it in a loop. (How else would you establish monotonicity?) Similarly, anyone who can understand mos6522.c and can read patches could easily add assertions to establish any of the deficiencies claimed in these patches. The problem isn't that you "have no way to reproduce results". The problem is that you are unwilling or unable to understand the datasheet and the patches. > Even then I don't know how long to wait for the clock to jump, how much > it jumps by, or if there is anything else that needs to be done to > trigger the warning. > > Any help with being able to build a working cross-m68k kernel to test > this would be gratefu
Re: [PATCH v1 0/9] hw/mos6522: VIA timer emulation fixes and improvements
On Thu, 18 Nov 2021, Mark Cave-Ayland wrote: > > Hi Finn, > > I've not forgotten about this series - we're now in 6.2 freeze, but it's > on my TODO list to revisit in the next development cycle this along with > the ESP stress-ng changes which I've also been looking at. As mentioned > in my previous reviews the patch will need some further analysis: > particularly the logic in mos6522_read() that can generate a spurious > interrupt on a register read needs to be removed, If mos6522 fails to raise its IRQ then the counter value observed by the guest will not make sense. This relationship was explained in the description of patch 8. If you have a problem with that patch, please reply there so that your misapprehension can be placed in context. > and also testing is required to ensure that these changes don't affect > the CUDA clock warping which allows OS X to boot under qemu-system-ppc. > I'm not expecting any issues. What is required in order to confirm this? Would it be sufficient to boot a Mac OS X 10.4 installer DVD? > I'm confident that qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) is monotonic, As I mentioned, it is monotonic here. > since if it were not then there would be huge numbers of complaints from > QEMU users. It appears that Linux can potentially alter the ticks in > mac_read_clk() at > https://github.com/torvalds/linux/blob/master/arch/m68k/mac/via.c#L624 > which suggests the issue is related to timer wraparound. I'd like to > confirm exactly which part of your series fixes the specific problem of > the clock jumping backwards before merging these changes. > You really only need a good datasheet to review these patches. You don't need a deep understanding of any particular guest, and you shouldn't be targetting any particular guest. One of the purposes of this patch series is to allow the guest to change to better exploit actual, physical hardware. Since QEMU regression testing is part of the kernel development process, regressions need to be avoided. That means QEMU's shortcomings hinder Linux development. Therefore, QEMU should not target the via timer driver in Linux v2.6 or the one in v5.15 or the one in NetBSD etc. QEMU should target correctness -- especially when that can be had for cheap. Wouldn't you agree? QEMU deviates in numerous ways from the behaviour of real mos6522 timer. This patch series does not address all of these deviations (see patch 8). Instead, this patch series addresses only the most aggregious ones, and they do impact existing guests. > I realized that I could easily cross-compile a 5.14 kernel to test this > theory with the test root image and .config you supplied at > https://gitlab.com/qemu-project/qemu/-/issues/611 using the QEMU > docker-m68k-cross image to avoid having to build a complete toolchain by > hand. The kernel builds successfully using this method, but during boot > it hangs sending the first SCSI CDB to the ESP device, failing to send > the last byte using PDMA. > > Are there known issues cross-compiling an m68k kernel on an x86 host? Not that I'm aware of. > Or are your current kernels being built from a separate branch outside > of mainline Linux git? > I use mainline Linux when testing QEMU. I provided a mainline build, attached to the same bug report, so you don't have to build it.
Re: [PATCH v1 0/9] hw/mos6522: VIA timer emulation fixes and improvements
#x27; clock's 50% safety margin (2147483647)
> [ 1907.51] timekeeping: Your kernel is still fine, but is feeling a bit
> nervous
> [ 1907.90] INFO: timekeeping: Cycle offset (4294963248) is larger than
> the 'via1' clock's 50% safety margin (2147483647)
> [ 1907.90] timekeeping: Your kernel is still fine, but is feeling a bit
> nervous
>
>
> This problem can be partly blamed on a 6522 design limitation, which is
> that the timer counter has no overflow register. Hence, if a timer
> counter wraps around and the kernel is late to handle the subsequent
> interrupt, the kernel can't account for any missed ticks.
>
> On a real Quadra, the kernel mitigates this limitation by minimizing
> interrupt latency. But on QEMU, interrupt latency is unbounded. This
> can't be mitigated by the guest kernel and leads to clock drift.
>
> This latency can be observed by patching QEMU like so:
>
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -379,6 +379,12 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t
> val, unsigned size)
> s->pcr = val;
> break;
> case VIA_REG_IFR:
> +if (val & T1_INT) {
> +static int64_t last_t1_int_cleared;
> +int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +if (now - last_t1_int_cleared > 2000) printf("\t%s: t1 int
> clear is late\n", __func__);
> +last_t1_int_cleared = now;
> +}
> /* reset bits */
> s->ifr &= ~val;
> mos6522_update_irq(s);
>
>
> This logic asserts that, given that Linux/m68k sets CONFIG_HZ to 100,
> the emulator will theoretically see each timer 1 interrupt cleared
> within 20 ms of the previous one. But that deadline is often missed on
> my QEMU host [4].
>
> On real Mac hardware you could observe the same scenario if a high
> priority interrupt were to sufficiently delay the timer interrupt
> handler. (This is the reason why the VIA1 interrupt priority gets
> increased from level 1 to level 6 when running on Quadras.)
>
> Anyway, for now, the clocksource monotonicity problem in Linux/mac68k
> guests is still unresolved. Nonetheless, I think this patch series does
> improve the situation.
>
> [1] I've also been working on some improvements to Linux/m68k based on
> Arnd Bergman's clockevent RFC patch,
> https://lore.kernel.org/linux-m68k/20201008154651.1901126-14-a...@arndb.de/
> The idea is to add a oneshot clockevent device by making use of the
> second VIA1 timer. This approach should help mitigate the clock drift
> problem as well as assist with CONFIG_GENERIC_CLOCKEVENTS adoption,
> which would enable CONFIG_NO_HZ_IDLE etc.
>
> [2] https://github.com/mcayland/qemu/commits/q800.upstream
>
> [3] https://github.com/fthain/qemu/commits/via-timer
>
> [4] This theoretical 20 ms deadline is not missed prior to every
> backwards jump in the clocksource counter. AFAICT, that's because the
> true deadline is somewhat shorter than 20 ms.
>
> ---
> Changed since RFC:
> - Added Reviewed-by tags.
> - Re-ordered some patches to make fixes available earlier in the series.
> - Dropped patch 5/10 "Don't clear T1 interrupt flag on latch write".
> - Rebased on v6.1.0 release.
>
>
> Finn Thain (9):
> hw/mos6522: Remove get_load_time() methods and functions
> hw/mos6522: Remove get_counter_value() methods and functions
> hw/mos6522: Remove redundant mos6522_timer1_update() calls
> hw/mos6522: Rename timer callback functions
> hw/mos6522: Fix initial timer counter reload
> hw/mos6522: Call mos6522_update_irq() when appropriate
> hw/mos6522: Avoid using discrepant QEMU clock values
> hw/mos6522: Synchronize timer interrupt and timer counter
> hw/mos6522: Implement oneshot mode
>
> hw/misc/mos6522.c | 245 ++
> hw/misc/trace-events | 2 +-
> include/hw/misc/mos6522.h | 9 ++
> 3 files changed, 123 insertions(+), 133 deletions(-)
>
>
[PATCH v1 8/9] hw/mos6522: Synchronize timer interrupt and timer counter
We rely on a QEMUTimer callback to set the interrupt flag, and this races
with counter register accesses, such that the guest might see the counter
reloaded but might not see the interrupt flagged.
According to the datasheet, a real 6522 device counts down to , then
raises the relevant IRQ. After the count, the counter reloads from
the latch (for timer 1) or continues to decrement thru FFFE (for timer 2).
Therefore, the guest operating system may read zero from T1CH and infer
that the counter has not yet wrapped (given another full count hasn't
yet elapsed.)
Similarly, the guest may find the timer interrupt flag to be set and
infer that the counter is non-zero (given another full count hasn't yet
elapsed).
Synchronize the timer counter and interrupt flag such that the guest will
observe the intended sequence of states. Document the known deviations
from a real 6522 device.
Eliminate the duplication of logic in get_counter() and
get_next_irq_time() by calling the former before the latter.
Call get_counter() prior to clearing interrupt flags so that the flags
remain synchronized with the timer counter.
Call get_counter() prior to changing the latch so that the old latch value
may be used to reload the counter. Every reload must use the value of the
latch as it was at the moment of the reload (not some later value).
Remove excess calls to qemu_clock_get_ns() to help avoid the possiblity
of inconsistency between timer interrupt flags and counters.
Signed-off-by: Finn Thain
---
Changed since RFC:
- Improved code commentary and commit log text.
- Removed qemu_clock_get_ns() call from set_counter() for simplicity.
- Moved the changes to the QEMUTimer callbacks from the previous patch
into this one because they relate more to syncronization than to
register accesses.
---
hw/misc/mos6522.c | 171 +-
hw/misc/trace-events | 2 +-
include/hw/misc/mos6522.h | 7 ++
3 files changed, 103 insertions(+), 77 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 385ea81b62..8957c5e65c 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -52,33 +52,72 @@ static void mos6522_update_irq(MOS6522State *s)
}
}
+static void mos6522_timer_raise_irq(MOS6522State *s, MOS6522Timer *ti)
+{
+if (ti->state == irq) {
+return;
+}
+ti->state = irq;
+if (ti->index == 0) {
+s->ifr |= T1_INT;
+} else {
+s->ifr |= T2_INT;
+}
+mos6522_update_irq(s);
+}
+
static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti, int64_t now)
{
int64_t d;
unsigned int counter;
-
+bool reload;
+
+/*
+ * Timer 1 counts down from the latch value to -1 (period of latch + 2),
+ * then raises its interrupt and reloads.
+ * Timer 2 counts down from the latch value to -1, then raises its
+ * interrupt and continues to -2 and so on without any further interrupts.
+ *
+ * TODO
+ * This implementation deviates from hardware behaviour because it omits
+ * the phase two clock. On a real 6522, the counter is decremented on a
+ * falling edge and the interrupt is asserted on a rising edge. Register
+ * accesses are synchronous with this clock. That means successive
+ * accesses to T1CL or T2CL can't yield the same value because
+ * they can't happen in the same clock cycle.
+ */
d = muldiv64(now - ti->load_time, ti->frequency, NANOSECONDS_PER_SECOND);
-if (ti->index == 0) {
-/* the timer goes down from latch to -1 (period of latch + 2) */
-if (d <= (ti->counter_value + 1)) {
-counter = ti->counter_value - d;
-} else {
-int64_t d_post_reload = d - (ti->counter_value + 2);
-/* XXX this calculation assumes that ti->latch has not changed */
-counter = ti->latch - (d_post_reload % (ti->latch + 2));
-}
-} else {
-counter = ti->counter_value - d;
+reload = (d >= ti->counter_value + 2);
+
+if (ti->index == 0 && reload) {
+int64_t more_reloads;
+
+d -= ti->counter_value + 2;
+more_reloads = d / (ti->latch + 2);
+d -= more_reloads * (ti->latch + 2);
+ti->load_time += muldiv64(ti->counter_value + 2 +
+ more_reloads * (ti->latch + 2),
+ NANOSECONDS_PER_SECOND, ti->frequency);
+ti->counter_value = ti->latch;
+}
+
+counter = ti->counter_value - d;
+
+if (reload) {
+mos6522_timer_raise_irq(s, ti);
}
+
return counter & 0x;
}
-static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val)
+static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val,
+int64_t now)
{
trace_mos6522_set_counter(1 + ti->index, val);
-ti-
[PATCH v1 9/9] hw/mos6522: Implement oneshot mode
Timer 1 has two modes: continuous interrupt and oneshot.
Signed-off-by: Finn Thain
---
Changed since RFC:
- Moved to end of series. This patch is quite a bit shorter here.
---
hw/misc/mos6522.c | 6 --
include/hw/misc/mos6522.h | 2 ++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 8957c5e65c..bbed0b84c0 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -148,7 +148,8 @@ static void mos6522_timer1_update(MOS6522State *s,
MOS6522Timer *ti,
}
get_counter(s, ti, now);
ti->next_irq_time = get_next_irq_time(s, ti, now);
-if ((s->ier & T1_INT) == 0 || (s->acr & T1MODE) != T1MODE_CONT) {
+if ((s->ier & T1_INT) == 0 ||
+((s->acr & T1MODE) == T1MODE_ONESHOT && ti->state >= irq)) {
timer_del(ti->timer);
} else {
timer_mod(ti->timer, ti->next_irq_time);
@@ -163,7 +164,7 @@ static void mos6522_timer2_update(MOS6522State *s,
MOS6522Timer *ti,
}
get_counter(s, ti, now);
ti->next_irq_time = get_next_irq_time(s, ti, now);
-if ((s->ier & T2_INT) == 0) {
+if ((s->ier & T2_INT) == 0 || (s->acr & T2MODE) || ti->state >= irq) {
timer_del(ti->timer);
} else {
timer_mod(ti->timer, ti->next_irq_time);
@@ -345,6 +346,7 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val,
unsigned size)
case VIA_REG_ACR:
s->acr = val;
mos6522_timer1_update(s, &s->timers[0], now);
+mos6522_timer2_update(s, &s->timers[1], now);
break;
case VIA_REG_PCR:
s->pcr = val;
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index a2df5fa942..4dbba6b273 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -50,8 +50,10 @@
#define T1_INT 0x40/* Timer 1 interrupt */
/* Bits in ACR */
+#define T2MODE 0x20/* Timer 2 mode */
#define T1MODE 0xc0/* Timer 1 mode */
#define T1MODE_CONT0x40/* continuous interrupts */
+#define T1MODE_ONESHOT 0x00/* timed interrupt */
/* VIA registers */
#define VIA_REG_B 0x00
--
2.26.3
[PATCH v1 6/9] hw/mos6522: Call mos6522_update_irq() when appropriate
It necessary to call mos6522_update_irq() when the interrupt flags
change and unnecessary when they haven't.
Signed-off-by: Finn Thain
Reviewed-by: Philippe Mathieu-Daudé
---
hw/misc/mos6522.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 6bd60f2118..bfe1719b18 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -203,10 +203,12 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned
size)
if (now >= s->timers[0].next_irq_time) {
mos6522_timer1_update(s, &s->timers[0], now);
s->ifr |= T1_INT;
+mos6522_update_irq(s);
}
if (now >= s->timers[1].next_irq_time) {
mos6522_timer2_update(s, &s->timers[1], now);
s->ifr |= T2_INT;
+mos6522_update_irq(s);
}
switch (addr) {
case VIA_REG_B:
@@ -231,7 +233,6 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned
size)
break;
case VIA_REG_T1CH:
val = get_counter(s, &s->timers[0]) >> 8;
-mos6522_update_irq(s);
break;
case VIA_REG_T1LL:
val = s->timers[0].latch & 0xff;
--
2.26.3
[PATCH v1 7/9] hw/mos6522: Avoid using discrepant QEMU clock values
mos6522_read() and mos6522_write() may call various functions to determine
timer irq state, timer counter value and QEMUTimer deadline. All called
functions must use the same value for the present time.
Signed-off-by: Finn Thain
Reviewed-by: Philippe Mathieu-Daudé
---
Changed since RFC
- Moved the changes to QEMUTimer callbacks to the next patch.
- Fix whitespace.
---
hw/misc/mos6522.c | 47 +++
1 file changed, 23 insertions(+), 24 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index bfe1719b18..385ea81b62 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -39,9 +39,9 @@
/* XXX: implement all timer modes */
static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
- int64_t current_time);
+ int64_t now);
static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
- int64_t current_time);
+ int64_t now);
static void mos6522_update_irq(MOS6522State *s)
{
@@ -52,13 +52,12 @@ static void mos6522_update_irq(MOS6522State *s)
}
}
-static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
+static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti, int64_t now)
{
int64_t d;
unsigned int counter;
-d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
- ti->frequency, NANOSECONDS_PER_SECOND);
+d = muldiv64(now - ti->load_time, ti->frequency, NANOSECONDS_PER_SECOND);
if (ti->index == 0) {
/* the timer goes down from latch to -1 (period of latch + 2) */
@@ -88,7 +87,7 @@ static void set_counter(MOS6522State *s, MOS6522Timer *ti,
unsigned int val)
}
static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
- int64_t current_time)
+ int64_t now)
{
int64_t d, next_time;
unsigned int counter;
@@ -98,8 +97,7 @@ static int64_t get_next_irq_time(MOS6522State *s,
MOS6522Timer *ti,
}
/* current counter value */
-d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
- ti->frequency, NANOSECONDS_PER_SECOND);
+d = muldiv64(now - ti->load_time, ti->frequency, NANOSECONDS_PER_SECOND);
/* the timer goes down from latch to -1 (period of latch + 2) */
if (d <= (ti->counter_value + 1)) {
@@ -122,20 +120,19 @@ static int64_t get_next_irq_time(MOS6522State *s,
MOS6522Timer *ti,
trace_mos6522_get_next_irq_time(ti->latch, d, next_time - d);
next_time = muldiv64(next_time, NANOSECONDS_PER_SECOND, ti->frequency) +
ti->load_time;
-
-if (next_time <= current_time) {
-next_time = current_time + 1;
-}
return next_time;
}
static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
- int64_t current_time)
+ int64_t now)
{
if (!ti->timer) {
return;
}
-ti->next_irq_time = get_next_irq_time(s, ti, current_time);
+ti->next_irq_time = get_next_irq_time(s, ti, now);
+if (ti->next_irq_time <= now) {
+ti->next_irq_time = now + 1;
+}
if ((s->ier & T1_INT) == 0 || (s->acr & T1MODE) != T1MODE_CONT) {
timer_del(ti->timer);
} else {
@@ -144,12 +141,15 @@ static void mos6522_timer1_update(MOS6522State *s,
MOS6522Timer *ti,
}
static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
- int64_t current_time)
+ int64_t now)
{
if (!ti->timer) {
return;
}
-ti->next_irq_time = get_next_irq_time(s, ti, current_time);
+ti->next_irq_time = get_next_irq_time(s, ti, now);
+if (ti->next_irq_time <= now) {
+ti->next_irq_time = now + 1;
+}
if ((s->ier & T2_INT) == 0) {
timer_del(ti->timer);
} else {
@@ -227,12 +227,12 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned
size)
val = s->dira;
break;
case VIA_REG_T1CL:
-val = get_counter(s, &s->timers[0]) & 0xff;
+val = get_counter(s, &s->timers[0], now) & 0xff;
s->ifr &= ~T1_INT;
mos6522_update_irq(s);
break;
case VIA_REG_T1CH:
-val = get_counter(s, &s->timers[0]) >> 8;
+val = get_counter(s, &s->timers[0], now) >> 8;
break;
case VIA_REG_T1LL:
val = s->timers[0].latch & 0xff;
@@ -241,12 +241,12 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned
size)
val = (s->timers[0].latch >> 8) & 0xff;
break;
case VIA_REG_T2CL:
-val = get_counter(s, &s->timers[1]) & 0xff;
+v
[PATCH v1 0/9] hw/mos6522: VIA timer emulation fixes and improvements
some improvements to Linux/m68k based on Arnd Bergman's clockevent RFC patch, https://lore.kernel.org/linux-m68k/20201008154651.1901126-14-a...@arndb.de/ The idea is to add a oneshot clockevent device by making use of the second VIA1 timer. This approach should help mitigate the clock drift problem as well as assist with CONFIG_GENERIC_CLOCKEVENTS adoption, which would enable CONFIG_NO_HZ_IDLE etc. [2] https://github.com/mcayland/qemu/commits/q800.upstream [3] https://github.com/fthain/qemu/commits/via-timer [4] This theoretical 20 ms deadline is not missed prior to every backwards jump in the clocksource counter. AFAICT, that's because the true deadline is somewhat shorter than 20 ms. --- Changed since RFC: - Added Reviewed-by tags. - Re-ordered some patches to make fixes available earlier in the series. - Dropped patch 5/10 "Don't clear T1 interrupt flag on latch write". - Rebased on v6.1.0 release. Finn Thain (9): hw/mos6522: Remove get_load_time() methods and functions hw/mos6522: Remove get_counter_value() methods and functions hw/mos6522: Remove redundant mos6522_timer1_update() calls hw/mos6522: Rename timer callback functions hw/mos6522: Fix initial timer counter reload hw/mos6522: Call mos6522_update_irq() when appropriate hw/mos6522: Avoid using discrepant QEMU clock values hw/mos6522: Synchronize timer interrupt and timer counter hw/mos6522: Implement oneshot mode hw/misc/mos6522.c | 245 ++ hw/misc/trace-events | 2 +- include/hw/misc/mos6522.h | 9 ++ 3 files changed, 123 insertions(+), 133 deletions(-) -- 2.26.3
[PATCH v1 4/9] hw/mos6522: Rename timer callback functions
This improves readability.
Signed-off-by: Finn Thain
Reviewed-by: Philippe Mathieu-Daudé
---
hw/misc/mos6522.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 1d4a56077e..c0d6bee4cc 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -154,7 +154,7 @@ static void mos6522_timer2_update(MOS6522State *s,
MOS6522Timer *ti,
}
}
-static void mos6522_timer1(void *opaque)
+static void mos6522_timer1_expired(void *opaque)
{
MOS6522State *s = opaque;
MOS6522Timer *ti = &s->timers[0];
@@ -164,7 +164,7 @@ static void mos6522_timer1(void *opaque)
mos6522_update_irq(s);
}
-static void mos6522_timer2(void *opaque)
+static void mos6522_timer2_expired(void *opaque)
{
MOS6522State *s = opaque;
MOS6522Timer *ti = &s->timers[1];
@@ -445,8 +445,10 @@ static void mos6522_init(Object *obj)
s->timers[i].index = i;
}
-s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
-s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
+s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+ mos6522_timer1_expired, s);
+s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+ mos6522_timer2_expired, s);
}
static void mos6522_finalize(Object *obj)
--
2.26.3
[PATCH v1 2/9] hw/mos6522: Remove get_counter_value() methods and functions
This code appears to be unnecessary.
Signed-off-by: Finn Thain
Reviewed-by: Philippe Mathieu-Daudé
---
hw/misc/mos6522.c | 22 ++
1 file changed, 2 insertions(+), 20 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index a478c1ca43..ff246b5437 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -52,23 +52,13 @@ static void mos6522_update_irq(MOS6522State *s)
}
}
-static uint64_t get_counter_value(MOS6522State *s, MOS6522Timer *ti)
-{
-MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
-
-if (ti->index == 0) {
-return mdc->get_timer1_counter_value(s, ti);
-} else {
-return mdc->get_timer2_counter_value(s, ti);
-}
-}
-
static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
{
int64_t d;
unsigned int counter;
-d = get_counter_value(s, ti);
+d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
+ ti->frequency, NANOSECONDS_PER_SECOND);
if (ti->index == 0) {
/* the timer goes down from latch to -1 (period of latch + 2) */
@@ -191,12 +181,6 @@ static void mos6522_set_sr_int(MOS6522State *s)
mos6522_update_irq(s);
}
-static uint64_t mos6522_get_counter_value(MOS6522State *s, MOS6522Timer *ti)
-{
-return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
-ti->frequency, NANOSECONDS_PER_SECOND);
-}
-
static void mos6522_portA_write(MOS6522State *s)
{
qemu_log_mask(LOG_UNIMP, "portA_write unimplemented\n");
@@ -498,8 +482,6 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
mdc->portB_write = mos6522_portB_write;
mdc->portA_write = mos6522_portA_write;
mdc->update_irq = mos6522_update_irq;
-mdc->get_timer1_counter_value = mos6522_get_counter_value;
-mdc->get_timer2_counter_value = mos6522_get_counter_value;
}
static const TypeInfo mos6522_type_info = {
--
2.26.3
[PATCH v1 5/9] hw/mos6522: Fix initial timer counter reload
The first reload of timer 1 is early by half of a clock cycle as it gets
measured from a falling edge. By contrast, the succeeding reloads are
measured from rising edge to rising edge.
Neglecting that complication, the behaviour of the counter should be the
same from one reload to the next. The sequence is always:
N, N-1, N-2, ... 2, 1, 0, -1, N, N-1, N-2, ...
But at the first reload, the present driver does this instead:
N, N-1, N-2, ... 2, 1, 0, -1, N-1, N-2, ...
Fix this deviation for both timer 1 and timer 2, and allow for the fact
that on a real 6522 the timer 2 counter is not reloaded when it wraps.
Signed-off-by: Finn Thain
---
hw/misc/mos6522.c | 19 +++
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index c0d6bee4cc..6bd60f2118 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -63,15 +63,16 @@ static unsigned int get_counter(MOS6522State *s,
MOS6522Timer *ti)
if (ti->index == 0) {
/* the timer goes down from latch to -1 (period of latch + 2) */
if (d <= (ti->counter_value + 1)) {
-counter = (ti->counter_value - d) & 0x;
+counter = ti->counter_value - d;
} else {
-counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
-counter = (ti->latch - counter) & 0x;
+int64_t d_post_reload = d - (ti->counter_value + 2);
+/* XXX this calculation assumes that ti->latch has not changed */
+counter = ti->latch - (d_post_reload % (ti->latch + 2));
}
} else {
-counter = (ti->counter_value - d) & 0x;
+counter = ti->counter_value - d;
}
-return counter;
+return counter & 0x;
}
static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val)
@@ -102,11 +103,13 @@ static int64_t get_next_irq_time(MOS6522State *s,
MOS6522Timer *ti,
/* the timer goes down from latch to -1 (period of latch + 2) */
if (d <= (ti->counter_value + 1)) {
-counter = (ti->counter_value - d) & 0x;
+counter = ti->counter_value - d;
} else {
-counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
-counter = (ti->latch - counter) & 0x;
+int64_t d_post_reload = d - (ti->counter_value + 2);
+/* XXX this calculation assumes that ti->latch has not changed */
+counter = ti->latch - (d_post_reload % (ti->latch + 2));
}
+counter &= 0x;
/* Note: we consider the irq is raised on 0 */
if (counter == 0x) {
--
2.26.3
[PATCH v1 3/9] hw/mos6522: Remove redundant mos6522_timer1_update() calls
Reads and writes to the TL and TC registers have no immediate effect on a running timer, with the exception of a write to TCH. Hence these mos6522_timer_update() calls are not needed. Signed-off-by: Finn Thain --- hw/misc/mos6522.c | 7 --- 1 file changed, 7 deletions(-) diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c index ff246b5437..1d4a56077e 100644 --- a/hw/misc/mos6522.c +++ b/hw/misc/mos6522.c @@ -234,7 +234,6 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size) val = s->timers[0].latch & 0xff; break; case VIA_REG_T1LH: -/* XXX: check this */ val = (s->timers[0].latch >> 8) & 0xff; break; case VIA_REG_T2CL: @@ -303,8 +302,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) break; case VIA_REG_T1CL: s->timers[0].latch = (s->timers[0].latch & 0xff00) | val; -mos6522_timer1_update(s, &s->timers[0], - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); break; case VIA_REG_T1CH: s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8); @@ -313,14 +310,10 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) break; case VIA_REG_T1LL: s->timers[0].latch = (s->timers[0].latch & 0xff00) | val; -mos6522_timer1_update(s, &s->timers[0], - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); break; case VIA_REG_T1LH: s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8); s->ifr &= ~T1_INT; -mos6522_timer1_update(s, &s->timers[0], - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); break; case VIA_REG_T2CL: s->timers[1].latch = (s->timers[1].latch & 0xff00) | val; -- 2.26.3
[PATCH v1 1/9] hw/mos6522: Remove get_load_time() methods and functions
This code appears to be unnecessary.
Signed-off-by: Finn Thain
Reviewed-by: Philippe Mathieu-Daudé
---
hw/misc/mos6522.c | 22 +-
1 file changed, 1 insertion(+), 21 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 1c57332b40..a478c1ca43 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -63,17 +63,6 @@ static uint64_t get_counter_value(MOS6522State *s,
MOS6522Timer *ti)
}
}
-static uint64_t get_load_time(MOS6522State *s, MOS6522Timer *ti)
-{
-MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
-
-if (ti->index == 0) {
-return mdc->get_timer1_load_time(s, ti);
-} else {
-return mdc->get_timer2_load_time(s, ti);
-}
-}
-
static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
{
int64_t d;
@@ -98,7 +87,7 @@ static unsigned int get_counter(MOS6522State *s, MOS6522Timer
*ti)
static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val)
{
trace_mos6522_set_counter(1 + ti->index, val);
-ti->load_time = get_load_time(s, ti);
+ti->load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
ti->counter_value = val;
if (ti->index == 0) {
mos6522_timer1_update(s, ti, ti->load_time);
@@ -208,13 +197,6 @@ static uint64_t mos6522_get_counter_value(MOS6522State *s,
MOS6522Timer *ti)
ti->frequency, NANOSECONDS_PER_SECOND);
}
-static uint64_t mos6522_get_load_time(MOS6522State *s, MOS6522Timer *ti)
-{
-uint64_t load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-
-return load_time;
-}
-
static void mos6522_portA_write(MOS6522State *s)
{
qemu_log_mask(LOG_UNIMP, "portA_write unimplemented\n");
@@ -518,8 +500,6 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
mdc->update_irq = mos6522_update_irq;
mdc->get_timer1_counter_value = mos6522_get_counter_value;
mdc->get_timer2_counter_value = mos6522_get_counter_value;
-mdc->get_timer1_load_time = mos6522_get_load_time;
-mdc->get_timer2_load_time = mos6522_get_load_time;
}
static const TypeInfo mos6522_type_info = {
--
2.26.3
Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
On Fri, 10 Sep 2021, Mark Cave-Ayland wrote:
> On 01/09/2021 09:06, Mark Cave-Ayland wrote:
>
> > I'll have a go at some basic timer measurements using your patch to
> > see what sort of numbers I get for the latency here. Obviously QEMU
> > doesn't guarantee response times but over 20ms does seem high.
>
> I was able to spend some time today looking at this and came up with
> https://github.com/mcayland/qemu/tree/via-hacks with the aim of starting
> with your patches to reduce the calls to the timer update functions (to
> reduce jitter) and fixing up the places where mos6522_update_irq() isn't
> called.
>
What kind of guest was that? What impact does jitter have on that guest?
Was the jitter measurement increased, decreased or unchanged by this patch
series?
> That seemed okay, but the part I'm struggling with at the moment is
> removing the re-assertion of the timer interrupt if the timer has
> expired when any of the registers are read i.e.
>
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index 3c33cbebde..9884d7e178 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -229,16 +229,7 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned
> size)
> {
> MOS6522State *s = opaque;
> uint32_t val;
> -int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>
> -if (now >= s->timers[0].next_irq_time) {
> -mos6522_timer1_update(s, &s->timers[0], now);
> -s->ifr |= T1_INT;
> -}
> -if (now >= s->timers[1].next_irq_time) {
> -mos6522_timer2_update(s, &s->timers[1], now);
> -s->ifr |= T2_INT;
> -}
> switch (addr) {
> case VIA_REG_B:
> val = s->b;
>
> If I apply this then I see a hang in about roughly 1 in 10 boots. Poking
> it with a debugger shows that the VIA1 timer interrupts are constantly
> being fired as IER and IFR have the timer 1 bit set, but it is being
> filtered out by SR being set to 0x2700 on the CPU.
>
> The existing code above isn't correct since T1_INT should only be
> asserted by the expiry of the timer 1 via mos6522_timer1(), so I'm
> wondering if this is tickling a CPU bug somewhere? In what circumstances
> could interrupts be disabled via SR but not enabled again?
>
The code you're patching here was part of Laurent's commit cd8843ff25
("mos6522: fix T1 and T2 timers"). You've mentioned that elsewhere in this
thread. My response is the same as before: this patch series removes that
code so it's moot.
Please test the patch series I sent, unmodified. If you find a problem
with my code, please do report it here. I believe that you will see no
hangs at all.
Re: [RFC 05/10] hw/mos6522: Don't clear T1 interrupt flag on latch write
On Wed, 1 Sep 2021, Laurent Vivier wrote:
> Le 26/08/2021 à 07:21, Finn Thain a écrit :
> > On Wed, 25 Aug 2021, Mark Cave-Ayland wrote:
> >
> >> On 24/08/2021 11:09, Finn Thain wrote:
> >>
> >>> The Synertek datasheet says, "A write to T1L-H loads an 8-bit count value
> >>> into the latch. A read of T1L-H transfers the contents of the latch to
> >>> the data bus. Neither operation has an affect [sic] on the interrupt
> >>> flag."
> >>>
> >>> Signed-off-by: Finn Thain
> >>> ---
> >>> hw/misc/mos6522.c | 1 -
> >>> 1 file changed, 1 deletion(-)
> >>>
> >>> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> >>> index c0d6bee4cc..8991f4 100644
> >>> --- a/hw/misc/mos6522.c
> >>> +++ b/hw/misc/mos6522.c
> >>> @@ -313,7 +313,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t
> >>> val, unsigned size)
> >>> break;
> >>> case VIA_REG_T1LH:
> >>> s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8);
> >>> -s->ifr &= ~T1_INT;
> >>> break;
> >>> case VIA_REG_T2CL:
> >>> s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
> >>
> >> Hmmm. The reference document I used for QEMU's 6522 device is at
> >> http://archive.6502.org/datasheets/mos_6522_preliminary_nov_1977.pdf and
> >> according to page 6 and the section "Writing the Timer 1 Registers"
> >> writing to
> >> the high byte of the latch does indeed clear the T1 interrupt flag.
> >>
> >> Side note: there is reference in Gary Davidian's excellent CHM video that
> >> 6522s obtained from different manufacturers had different behaviours, and
> >> there are also web pages mentioning that 6522s integrated as part of other
> >> silicon e.g. IOSB/CUDA also had their own bugs... :/
> >>
> >
> > The MOS document you've cited is much older than the Synertek and Rockwell
> > devices. The datasheets for the Synertek and Rockwell parts disagree with
> > MOS about T1LH behaviour. Apple certainly used SY6522 devices in my Mac II
> > and I'd assumed Apple would have used compatible logic cores in the custom
> > ICs found in later models. But I don't really trust assumptions and
> > datasheets so I wrote the Linux patch below and ran it on my Quadra 630.
> >
> > diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
> > index 3d11d6219cdd..ed41f6ae2bf2 100644
> > --- a/arch/m68k/mac/via.c
> > +++ b/arch/m68k/mac/via.c
> > @@ -634,3 +634,27 @@ static u64 mac_read_clk(struct clocksource *cs)
> >
> > return ticks;
> > }
> > +
> > +static int baz(void)
> > +{
> > + u8 a, b, c;
> > +
> > + local_irq_disable();
> > +
> > + while (!(via1[vIFR] & VIA_TIMER_1_INT))
> > + continue;
> > + a = via1[vIFR] & VIA_TIMER_1_INT;
> > + via1[vT1LH] = via1[vT1LH];
> > + b = via1[vIFR] & VIA_TIMER_1_INT;
> > + via1[vT1LL] = via1[vT1LL];
> > + c = via1[vIFR] & VIA_TIMER_1_INT;
> > +
> > + printk("a == %2x\n", a);
> > + printk("b == %2x\n", b);
> > + printk("c == %2x\n", c);
> > +
> > + local_irq_enable();
> > +
> > + return 0;
> > +}
> > +late_initcall(baz);
> >
> > Based on the Synertek datasheet* one would expect to see b equal to a but
> > I got this result instead:
> >
> > [ 10.45] a == 40
> > [ 10.45] b == 0
> > [ 10.45] c == 0
> >
> > This amounts to a MOS design flaw and I doubt that this result from my
> > Quadra 630 would apply to other Mac models. So it would be great to see
> > the output from a Quadra 800. But until then, let's disregard this patch.
> >
> > * http://archive.6502.org/datasheets/synertek_sy6522.pdf
> >
>
> Tested on my Quadra 800:
>
> [4.73] a == 40
> [4.73] b == 0
> [4.73] c == 0
>
Much appreciated. I will drop this patch.
Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
On Tue, 31 Aug 2021, Mark Cave-Ayland wrote: > You mentioned that the OS may compensate for the fact that the 6522 > doesn't have an overflow flag: can you explain more as to how this works > in Linux? When running on real hardware, Linux/mac68k does so by - Elevating the interrupt priority of VIA 1 so that other drivers do not interfere with timekeeping - Constraining intervals during which the IPL is kept elevated (i.e. local_irq_disable/enable). When runing on QEMU, none of that is sufficient and the Linux/mac68k kernel can do very little to influence interrupt latency. Linux ports to other platforms typically have multiple timers and counters with which to implement reliable clocksource devices. When running on other virtualization platforms, Linux may solve the problem using a paravirtual clock device. Please see CONFIG_PARAVIRT_CLOCK, arch/x86/include/asm/pvclock-abi.h, arch/x86/kernel/pvclock.c, arch/x86/include/asm/vdso/gettimeofday.h arch/x86/xen/time.c and so on.
Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
On Tue, 31 Aug 2021, Mark Cave-Ayland wrote:
> On 28/08/2021 02:22, Finn Thain wrote:
>
> > > On 8/24/21 12:09 PM, Finn Thain wrote:
> > >
> > > > On a real Quadra, accesses to the SY6522 chips are slow because they are
> > > > synchronous with the 783360 Hz "phase 2" clock. In QEMU, they are slow
> > > > only because of the division operation in the timer count calculation.
> > > >
> > > > This patch series improves the fidelity of the emulated chip, but the
> > > > price is more division ops. I haven't tried to measure this yet.
> > > >
> > > > The emulated 6522 still deviates from the behaviour of the real thing,
> > > > however. For example, two consecutive accesses to a real 6522 timer
> > > > counter can never yield the same value. This is not true of the 6522 in
> > > > QEMU 6 wherein two consecutive accesses to a timer count register have
> > > > been observed to yield the same value.
> > > >
> > > > Linux is not particularly robust in the face of a 6522 that deviates
> > > > from the usual behaviour. The problem presently affecting a Linux guest
> > > > is that its 'via' clocksource is prone to monotonicity failure. That is,
> > > > the clocksource counter can jump backwards. This can be observed by
> > > > patching Linux like so:
> > > >
> > > > diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
> > > > --- a/arch/m68k/mac/via.c
> > > > +++ b/arch/m68k/mac/via.c
> > > > @@ -606,6 +606,8 @@ void __init via_init_clock(void)
> > > > clocksource_register_hz(&mac_clk, VIA_CLOCK_FREQ);
> > > > }
> > > > +static u32 prev_ticks;
> > > > +
> > > > static u64 mac_read_clk(struct clocksource *cs)
> > > > {
> > > > unsigned long flags;
> > > > @@ -631,6 +633,8 @@ static u64 mac_read_clk(struct clocksource *cs)
> > > > count = count_high << 8;
> > > > ticks = VIA_TIMER_CYCLES - count;
> > > > ticks += clk_offset + clk_total;
> > > > +if (ticks < prev_ticks) pr_warn("%s: %u < %u\n", __func__, ticks,
> > > > prev_ticks);
> > > > +prev_ticks = ticks;
> > > > local_irq_restore(flags);
> > > > return ticks;
> > > >
> > > > This problem can be partly blamed on a 6522 design limitation, which is
> > > > that the timer counter has no overflow register. Hence, if a timer
> > > > counter wraps around and the kernel is late to handle the subsequent
> > > > interrupt, the kernel can't account for any missed ticks.
> > > >
> > > > On a real Quadra, the kernel mitigates this limitation by minimizing
> > > > interrupt latency. But on QEMU, interrupt latency is unbounded. This
> > > > can't be mitigated by the guest kernel at all and leads to clock drift.
> > > > This can be observed by patching QEMU like so:
> > > >
> > > > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > > > --- a/hw/misc/mos6522.c
> > > > +++ b/hw/misc/mos6522.c
> > > > @@ -379,6 +379,12 @@ void mos6522_write(void *opaque, hwaddr addr,
> > > > uint64_t val, unsigned size)
> > > > s->pcr = val;
> > > > break;
> > > > case VIA_REG_IFR:
> > > > +if (val & T1_INT) {
> > > > +static int64_t last_t1_int_cleared;
> > > > +int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > > > +if (now - last_t1_int_cleared > 2000) printf("\t%s: t1
> > > > int clear is late\n", __func__);
> > > > +last_t1_int_cleared = now;
> > > > +}
> > > > /* reset bits */
> > > > s->ifr &= ~val;
> > > > mos6522_update_irq(s);
> > > >
> > > > This logic asserts that, given that Linux/m68k sets CONFIG_HZ to 100,
> > > > the emulator will theoretically see each timer 1 interrupt cleared
> > > > within 20 ms of the previous one. But that deadline is often missed on
> > > > my QEMU host [4].
> > >
> > > I wonder if using QEMU ptimer wouldn't help here, instead of
> > > calling qemu_clock_get_ns() and doing the math by hand:
> > >
> > > /* Starting to run with/se
Re: [RFC 09/10] hw/mos6522: Avoid using discrepant QEMU clock values
On Wed, 25 Aug 2021, Mark Cave-Ayland wrote:
> On 24/08/2021 11:09, Finn Thain wrote:
>
> > mos6522_read() and mos6522_write() may call various functions to determine
> > timer irq state, timer counter value and QEMUTimer deadline. All called
> > functions must use the same value for the present time.
> >
> > Signed-off-by: Finn Thain
> > ---
> > hw/misc/mos6522.c | 51 +--
> > 1 file changed, 27 insertions(+), 24 deletions(-)
> >
> > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > index 0dd3ccf945..23a440b64f 100644
> > --- a/hw/misc/mos6522.c
> > +++ b/hw/misc/mos6522.c
> > @@ -39,9 +39,9 @@
> > /* XXX: implement all timer modes */
> > static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
> > - int64_t current_time);
> > + int64_t now);
> > static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
> > - int64_t current_time);
> > + int64_t now);
> > static void mos6522_update_irq(MOS6522State *s)
> > {
> > @@ -52,12 +52,12 @@ static void mos6522_update_irq(MOS6522State *s)
> > }
> > }
> > -static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
> > +static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti, int64_t
> > now)
> > {
> > int64_t d;
> > unsigned int counter;
> > -d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
> > +d = muldiv64(now - ti->load_time,
> >ti->frequency, NANOSECONDS_PER_SECOND);
> > if (ti->index == 0) {
> > @@ -89,7 +89,7 @@ static void set_counter(MOS6522State *s, MOS6522Timer *ti,
> > unsigned int val)
> > }
> > static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
> > - int64_t current_time)
> > + int64_t now)
> > {
> > int64_t d, next_time;
> > unsigned int counter;
> > @@ -99,7 +99,7 @@ static int64_t get_next_irq_time(MOS6522State *s,
> > MOS6522Timer *ti,
> > }
> > /* current counter value */
> > -d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
> > +d = muldiv64(now - ti->load_time,
> >ti->frequency, NANOSECONDS_PER_SECOND);
> > /* the timer goes down from latch to -1 (period of latch + 2) */
> > @@ -123,20 +123,19 @@ static int64_t get_next_irq_time(MOS6522State *s,
> > MOS6522Timer *ti,
> > trace_mos6522_get_next_irq_time(ti->latch, d, next_time - d);
> > next_time = muldiv64(next_time, NANOSECONDS_PER_SECOND, ti->frequency)
> > +
> >ti->load_time;
> > -
> > -if (next_time <= current_time) {
> > -next_time = current_time + 1;
> > -}
> > return next_time;
> > }
> > static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
> > - int64_t current_time)
> > + int64_t now)
> > {
> > if (!ti->timer) {
> > return;
> > }
> > -ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> > +ti->next_irq_time = get_next_irq_time(s, ti, now);
> > +if (ti->next_irq_time <= now) {
> > +ti->next_irq_time = now + 1;
> > +}
> > if ((s->ier & T1_INT) == 0 ||
> > ((s->acr & T1MODE) == T1MODE_ONESHOT && ti->oneshot_fired)) {
> > timer_del(ti->timer);
> > @@ -146,12 +145,15 @@ static void mos6522_timer1_update(MOS6522State *s,
> > MOS6522Timer *ti,
> > }
> > static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
> > - int64_t current_time)
> > + int64_t now)
> > {
> > if (!ti->timer) {
> > return;
> > }
> > -ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> > +ti->next_irq_time = get_next_irq_time(s, ti, now);
> > +if (ti->next_irq_time <= now) {
> > +ti->next_irq_time = now + 1;
> > +}
> > if ((s->ier & T2_INT) == 0 || (s->acr & T2MODE) || ti->oneshot_fired)
> > {
> > timer_del(ti->timer);
> > } else {
> > @@
Re: [RFC 09/10] hw/mos6522: Avoid using discrepant QEMU clock values
On Tue, 24 Aug 2021, Philippe Mathieu-Daudé wrote:
> On 8/24/21 12:09 PM, Finn Thain wrote:
> > mos6522_read() and mos6522_write() may call various functions to determine
> > timer irq state, timer counter value and QEMUTimer deadline. All called
> > functions must use the same value for the present time.
> >
> > Signed-off-by: Finn Thain
> > ---
> > hw/misc/mos6522.c | 51 +--
> > 1 file changed, 27 insertions(+), 24 deletions(-)
>
> > @@ -123,20 +123,19 @@ static int64_t get_next_irq_time(MOS6522State *s,
> > MOS6522Timer *ti,
> > trace_mos6522_get_next_irq_time(ti->latch, d, next_time - d);
> > next_time = muldiv64(next_time, NANOSECONDS_PER_SECOND, ti->frequency)
> > +
> > ti->load_time;
> > -
> > -if (next_time <= current_time) {
> > -next_time = current_time + 1;
> > -}
>
> Maybe extract as an helper?
There is a small amount of code duplication here but it gets resolved in
patch 10/10.
> Otherwise:
>
> Reviewed-by: Philippe Mathieu-Daudé
>
> > return next_time;
> > }
> >
> > static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
> > - int64_t current_time)
> > + int64_t now)
> > {
> > if (!ti->timer) {
> > return;
> > }
> > -ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> > +ti->next_irq_time = get_next_irq_time(s, ti, now);
> > +if (ti->next_irq_time <= now) {
> > +ti->next_irq_time = now + 1;
> > +}
> > if ((s->ier & T1_INT) == 0 ||
> > ((s->acr & T1MODE) == T1MODE_ONESHOT && ti->oneshot_fired)) {
> > timer_del(ti->timer);
> > @@ -146,12 +145,15 @@ static void mos6522_timer1_update(MOS6522State *s,
> > MOS6522Timer *ti,
> > }
> >
> > static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
> > - int64_t current_time)
> > + int64_t now)
> > {
> > if (!ti->timer) {
> > return;
> > }
> > -ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> > +ti->next_irq_time = get_next_irq_time(s, ti, now);
> > +if (ti->next_irq_time <= now) {
> > +ti->next_irq_time = now + 1;
> > +}
>
>
Re: [RFC 06/10] hw/mos6522: Implement oneshot mode
On Wed, 25 Aug 2021, Mark Cave-Ayland wrote:
> On 24/08/2021 11:09, Finn Thain wrote:
>
> > Signed-off-by: Finn Thain
> > ---
> > hw/misc/mos6522.c | 19 ---
> > include/hw/misc/mos6522.h | 3 +++
> > 2 files changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > index 8991f4..5b1657ac0d 100644
> > --- a/hw/misc/mos6522.c
> > +++ b/hw/misc/mos6522.c
> > @@ -79,6 +79,7 @@ static void set_counter(MOS6522State *s, MOS6522Timer *ti,
> > unsigned int val)
> > trace_mos6522_set_counter(1 + ti->index, val);
> > ti->load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > ti->counter_value = val;
> > +ti->oneshot_fired = false;
> > if (ti->index == 0) {
> > mos6522_timer1_update(s, ti, ti->load_time);
> > } else {
> > @@ -133,7 +134,8 @@ static void mos6522_timer1_update(MOS6522State *s,
> > MOS6522Timer *ti,
> > return;
> > }
> > ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> > -if ((s->ier & T1_INT) == 0 || (s->acr & T1MODE) != T1MODE_CONT) {
> > +if ((s->ier & T1_INT) == 0 ||
> > +((s->acr & T1MODE) == T1MODE_ONESHOT && ti->oneshot_fired)) {
> > timer_del(ti->timer);
> > } else {
> > timer_mod(ti->timer, ti->next_irq_time);
> > @@ -147,7 +149,7 @@ static void mos6522_timer2_update(MOS6522State *s,
> > MOS6522Timer *ti,
> > return;
> > }
> > ti->next_irq_time = get_next_irq_time(s, ti, current_time);
> > -if ((s->ier & T2_INT) == 0) {
> > +if ((s->ier & T2_INT) == 0 || (s->acr & T2MODE) || ti->oneshot_fired) {
> > timer_del(ti->timer);
> > } else {
> > timer_mod(ti->timer, ti->next_irq_time);
> > @@ -159,6 +161,7 @@ static void mos6522_timer1_expired(void *opaque)
> > MOS6522State *s = opaque;
> > MOS6522Timer *ti = &s->timers[0];
> > +ti->oneshot_fired = true;
> > mos6522_timer1_update(s, ti, ti->next_irq_time);
> > s->ifr |= T1_INT;
> > mos6522_update_irq(s);
> > @@ -169,6 +172,7 @@ static void mos6522_timer2_expired(void *opaque)
> > MOS6522State *s = opaque;
> > MOS6522Timer *ti = &s->timers[1];
> > +ti->oneshot_fired = true;
> > mos6522_timer2_update(s, ti, ti->next_irq_time);
> > s->ifr |= T2_INT;
> > mos6522_update_irq(s);
>
> I was trying to understand why you need ti->oneshot_fired here since the
> mos6522_timer*_update() functions should simply not re-arm the timer if
> not in continuous mode...
>
Not so. The timer has to be re-armed with timer_mod() when
(timer interrupt enabled and timer in continuous mode) ||
(timer interrupt enabled and timer in oneshot mode and no interrupt raised)
Conversely, the timer has to be cancelled with timer_del() when
(timer interrupt disabled) ||
(timer in oneshot mode and interrupt has been raised) ||
(timer in pulse-counting mode)
> > @@ -198,10 +202,12 @@ uint64_t mos6522_read(void *opaque, hwaddr addr,
> > unsigned size)
> > int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > if (now >= s->timers[0].next_irq_time) {
> > +s->timers[0].oneshot_fired = true;
> > mos6522_timer1_update(s, &s->timers[0], now);
> > s->ifr |= T1_INT;
> > }
> > if (now >= s->timers[1].next_irq_time) {
> > +s->timers[1].oneshot_fired = true;
> > mos6522_timer2_update(s, &s->timers[1], now);
> > s->ifr |= T2_INT;
> > }
>
> ...however this block above raises the timer interrupt outside of the
> timer callback. This block isn't part of your original patch but was
> introduced as part of cd8843ff25d ("mos6522: fix T1 and T2 timers") but
> I'm wondering if it is wrong.
>
Maybe. I think a good answer would make reference to QEMU internals and
synchronization guarantees between the invocation of the callbacks and
methods in mos6522.c. I don't have a good answer, but it's moot...
> If you remove both of the above if (now ... ) {} blocks then does
> one-shot mode work by just adding the (s->acr & T2MODE) check in
> mos6522_timer2_update()?
>
Those blocks got removed in patch 10/10 because they aren't needed as long
as get_counter() gets called when necessary.
> I'm guessing that Linux/m68k does use one or both of the timers in
> one-shot mode?
>
Yes, but it's not in mainline yet. I wrote the code some months ago but
I can't push it upstream until QEMU supports it:
https://github.com/fthain/linux/commits/clockevent-oneshot
Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
On Wed, 25 Aug 2021, Mark Cave-Ayland wrote:
> On 24/08/2021 11:09, Finn Thain wrote:
>
> > This is a patch series that I started last year. The aim was to try to
> > get a monotonic clocksource for Linux/m68k guests. That aim hasn't been
> > achieved yet (for q800 machines) but I'm submitting the patch series as
> > an RFC because,
> >
> > - It does improve 6522 emulation fidelity.
> >
> > - It allows Linux/m68k to make use of the additional timer that the
> > hardware indeed offers but which QEMU omits. This has several
> > benefits for Linux guests [1].
> >
> > - I see that Mark has been working on timer emulation issues in his
> > github repo [2] and it seems likely that MacOS, NetBSD or A/UX guests
> > will also require better 6522 emulation.
> >
> > To make collaboration easier these patches can also be fetched from
> > github [3].
> >
> > On a real Quadra, accesses to the SY6522 chips are slow because they are
> > synchronous with the 783360 Hz "phase 2" clock. In QEMU, they are slow
> > only because of the division operation in the timer count calculation.
> >
> > This patch series improves the fidelity of the emulated chip, but the
> > price is more division ops. I haven't tried to measure this yet.
> >
> > The emulated 6522 still deviates from the behaviour of the real thing,
> > however. For example, two consecutive accesses to a real 6522 timer
> > counter can never yield the same value. This is not true of the 6522 in
> > QEMU 6 wherein two consecutive accesses to a timer count register have
> > been observed to yield the same value.
> >
> > Linux is not particularly robust in the face of a 6522 that deviates
> > from the usual behaviour. The problem presently affecting a Linux guest
> > is that its 'via' clocksource is prone to monotonicity failure. That is,
> > the clocksource counter can jump backwards. This can be observed by
> > patching Linux like so:
> >
> > diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
> > --- a/arch/m68k/mac/via.c
> > +++ b/arch/m68k/mac/via.c
> > @@ -606,6 +606,8 @@ void __init via_init_clock(void)
> > clocksource_register_hz(&mac_clk, VIA_CLOCK_FREQ);
> > }
> > +static u32 prev_ticks;
> > +
> > static u64 mac_read_clk(struct clocksource *cs)
> > {
> > unsigned long flags;
> > @@ -631,6 +633,8 @@ static u64 mac_read_clk(struct clocksource *cs)
> > count = count_high << 8;
> > ticks = VIA_TIMER_CYCLES - count;
> > ticks += clk_offset + clk_total;
> > +if (ticks < prev_ticks) pr_warn("%s: %u < %u\n", __func__, ticks,
> > prev_ticks);
> > +prev_ticks = ticks;
> > local_irq_restore(flags);
> > return ticks;
> >
> > This problem can be partly blamed on a 6522 design limitation, which is
> > that the timer counter has no overflow register. Hence, if a timer
> > counter wraps around and the kernel is late to handle the subsequent
> > interrupt, the kernel can't account for any missed ticks.
> >
> > On a real Quadra, the kernel mitigates this limitation by minimizing
> > interrupt latency. But on QEMU, interrupt latency is unbounded. This
> > can't be mitigated by the guest kernel at all and leads to clock drift.
> > This can be observed by patching QEMU like so:
> >
> > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > --- a/hw/misc/mos6522.c
> > +++ b/hw/misc/mos6522.c
> > @@ -379,6 +379,12 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t
> > val, unsigned size)
> > s->pcr = val;
> > break;
> > case VIA_REG_IFR:
> > +if (val & T1_INT) {
> > +static int64_t last_t1_int_cleared;
> > +int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > +if (now - last_t1_int_cleared > 2000) printf("\t%s: t1 int
> > clear is late\n", __func__);
> > +last_t1_int_cleared = now;
> > +}
> > /* reset bits */
> > s->ifr &= ~val;
> > mos6522_update_irq(s);
> >
> > This logic asserts that, given that Linux/m68k sets CONFIG_HZ to 100,
> > the emulator will theoretically see each timer 1 interrupt cleared
> > within 20 ms of the previous one. But that deadline is often missed on
> > my QEMU host [4].
> >
> > On real Mac hardware you could observe the same scenario if a high
> > priority interrupt were to sufficie
Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
On Tue, 24 Aug 2021, Philippe Mathieu-Daudé wrote:
> On 8/24/21 12:09 PM, Finn Thain wrote:
>
> > On a real Quadra, accesses to the SY6522 chips are slow because they are
> > synchronous with the 783360 Hz "phase 2" clock. In QEMU, they are slow
> > only because of the division operation in the timer count calculation.
> >
> > This patch series improves the fidelity of the emulated chip, but the
> > price is more division ops. I haven't tried to measure this yet.
> >
> > The emulated 6522 still deviates from the behaviour of the real thing,
> > however. For example, two consecutive accesses to a real 6522 timer
> > counter can never yield the same value. This is not true of the 6522 in
> > QEMU 6 wherein two consecutive accesses to a timer count register have
> > been observed to yield the same value.
> >
> > Linux is not particularly robust in the face of a 6522 that deviates
> > from the usual behaviour. The problem presently affecting a Linux guest
> > is that its 'via' clocksource is prone to monotonicity failure. That is,
> > the clocksource counter can jump backwards. This can be observed by
> > patching Linux like so:
> >
> > diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
> > --- a/arch/m68k/mac/via.c
> > +++ b/arch/m68k/mac/via.c
> > @@ -606,6 +606,8 @@ void __init via_init_clock(void)
> > clocksource_register_hz(&mac_clk, VIA_CLOCK_FREQ);
> > }
> >
> > +static u32 prev_ticks;
> > +
> > static u64 mac_read_clk(struct clocksource *cs)
> > {
> > unsigned long flags;
> > @@ -631,6 +633,8 @@ static u64 mac_read_clk(struct clocksource *cs)
> > count = count_high << 8;
> > ticks = VIA_TIMER_CYCLES - count;
> > ticks += clk_offset + clk_total;
> > +if (ticks < prev_ticks) pr_warn("%s: %u < %u\n", __func__, ticks,
> > prev_ticks);
> > +prev_ticks = ticks;
> > local_irq_restore(flags);
> >
> > return ticks;
> >
> > This problem can be partly blamed on a 6522 design limitation, which is
> > that the timer counter has no overflow register. Hence, if a timer
> > counter wraps around and the kernel is late to handle the subsequent
> > interrupt, the kernel can't account for any missed ticks.
> >
> > On a real Quadra, the kernel mitigates this limitation by minimizing
> > interrupt latency. But on QEMU, interrupt latency is unbounded. This
> > can't be mitigated by the guest kernel at all and leads to clock drift.
> > This can be observed by patching QEMU like so:
> >
> > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > --- a/hw/misc/mos6522.c
> > +++ b/hw/misc/mos6522.c
> > @@ -379,6 +379,12 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t
> > val, unsigned size)
> > s->pcr = val;
> > break;
> > case VIA_REG_IFR:
> > +if (val & T1_INT) {
> > +static int64_t last_t1_int_cleared;
> > +int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > +if (now - last_t1_int_cleared > 2000) printf("\t%s: t1 int
> > clear is late\n", __func__);
> > +last_t1_int_cleared = now;
> > +}
> > /* reset bits */
> > s->ifr &= ~val;
> > mos6522_update_irq(s);
> >
> > This logic asserts that, given that Linux/m68k sets CONFIG_HZ to 100,
> > the emulator will theoretically see each timer 1 interrupt cleared
> > within 20 ms of the previous one. But that deadline is often missed on
> > my QEMU host [4].
>
> I wonder if using QEMU ptimer wouldn't help here, instead of
> calling qemu_clock_get_ns() and doing the math by hand:
>
> /* Starting to run with/setting counter to "0" won't trigger immediately,
> * but after a one period for both oneshot and periodic modes. */
> #define PTIMER_POLICY_NO_IMMEDIATE_TRIGGER (1 << 2)
>
> /* Starting to run with/setting counter to "0" won't re-load counter
> * immediately, but after a one period. */
> #define PTIMER_POLICY_NO_IMMEDIATE_RELOAD (1 << 3)
>
> /* Make counter value of the running timer represent the actual value and
> * not the one less. */
> #define PTIMER_POLICY_NO_COUNTER_ROUND_DOWN (1 << 4)
>
It's often the case that a conversion to a new API is trivial for someone
who understands that API. But I still haven't got my head around the
implementation (hw/core/ptimer.c).
So I agree the ptimer API could simplify mos6522.c but adopting it is not
trivial for me.
QEMU's 6522 device does not attempt to model the relationship between the
"phase two" clock and timer counters and timer interrupts. I haven't
attempted to fix these deviations at all, as that's not trivial either.
But using the ptimer API could potentially make it easier to address those
fidelity issues.
Re: [RFC 01/10] hw/mos6522: Remove get_load_time() methods and functions
On Wed, 25 Aug 2021, Mark Cave-Ayland wrote:
> On 24/08/2021 11:09, Finn Thain wrote:
>
> > This code appears to be unnecessary.
> >
> > Signed-off-by: Finn Thain
> > ---
> > hw/misc/mos6522.c | 22 +-
> > 1 file changed, 1 insertion(+), 21 deletions(-)
> >
> > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > index 1c57332b40..a478c1ca43 100644
> > --- a/hw/misc/mos6522.c
> > +++ b/hw/misc/mos6522.c
> > @@ -63,17 +63,6 @@ static uint64_t get_counter_value(MOS6522State *s,
> > MOS6522Timer *ti)
> > }
> > }
> > -static uint64_t get_load_time(MOS6522State *s, MOS6522Timer *ti)
> > -{
> > -MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
> > -
> > -if (ti->index == 0) {
> > -return mdc->get_timer1_load_time(s, ti);
> > -} else {
> > -return mdc->get_timer2_load_time(s, ti);
> > -}
> > -}
> > -
> > static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
> > {
> > int64_t d;
> > @@ -98,7 +87,7 @@ static unsigned int get_counter(MOS6522State *s,
> > MOS6522Timer *ti)
> > static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int
> > val)
> > {
> > trace_mos6522_set_counter(1 + ti->index, val);
> > -ti->load_time = get_load_time(s, ti);
> > +ti->load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > ti->counter_value = val;
> > if (ti->index == 0) {
> > mos6522_timer1_update(s, ti, ti->load_time);
> > @@ -208,13 +197,6 @@ static uint64_t mos6522_get_counter_value(MOS6522State
> > *s, MOS6522Timer *ti)
> > ti->frequency, NANOSECONDS_PER_SECOND);
> > }
> > -static uint64_t mos6522_get_load_time(MOS6522State *s, MOS6522Timer *ti)
> > -{
> > -uint64_t load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > -
> > -return load_time;
> > -}
> > -
> > static void mos6522_portA_write(MOS6522State *s)
> > {
> > qemu_log_mask(LOG_UNIMP, "portA_write unimplemented\n");
> > @@ -518,8 +500,6 @@ static void mos6522_class_init(ObjectClass *oc, void
> > *data)
> > mdc->update_irq = mos6522_update_irq;
> > mdc->get_timer1_counter_value = mos6522_get_counter_value;
> > mdc->get_timer2_counter_value = mos6522_get_counter_value;
> > -mdc->get_timer1_load_time = mos6522_get_load_time;
> > -mdc->get_timer2_load_time = mos6522_get_load_time;
> > }
> > static const TypeInfo mos6522_type_info = {
>
> Both the get_counter_value() and get_load_time() callbacks are used as part of
> the CUDA emulation in hw/misc/macio/cuda.c as per the comment:
>
> /* MacOS uses timer 1 for calibration on startup, so we use
> * the timebase frequency and cuda_get_counter_value() with
> * cuda_get_load_time() to steer MacOS to calculate calibrate its timers
> * correctly for both TCG and KVM (see commit b981289c49 "PPC: Cuda: Use cuda
> * timer to expose tbfreq to guest" for more information) */
>
> Certainly for the 6522 device it is worth configuring with
> --target-list="ppc-softmmu m68k-softmmu" to make sure that you don't
> inadvertently break anything in the PPC world.
>
No build failure here. Maybe your tree is different?
> A bit of history here: the original mos6522.c was extracted from
> hw/misc/macio/cuda.c when Laurent presented his initial q800 patches since
> they also had their own implementation of the 6522, and it was better to move
> the implementation into a separate QEMU device so that the logic could be
> shared.
>
> The Darwin kernel timer calibration loop is quite hard to get right: see
> https://opensource.apple.com/source/xnu/xnu-123.5/pexpert/ppc/pe_clock_speed_asm.s.auto.html
> and
> https://opensource.apple.com/source/xnu/xnu-123.5/pexpert/ppc/pe_clock_speed.c.auto.html.
> Ben/Alex came up with the current mechanism to fool the calibration routine,
> and I simply added in those callbacks to allow it to be implemented as part of
> the now-generic 6522 device.
>
I didn't find any references to these methods (get_timer1_counter_value,
get_timer2_counter_value, get_timer1_load_time and get_timer2_load_time).
It appears to be dead code, and it adds complexity and harms readability.
The Linux kernel project has a policy that no code is added if it lacks
any in-tree usage. Does QEMU have the same policy?
Re: [RFC 07/10] hw/mos6522: Fix initial timer counter reload
On Wed, 25 Aug 2021, Mark Cave-Ayland wrote:
> On 24/08/2021 11:09, Finn Thain wrote:
>
> > The first reload of timer 1 is early by half of a clock cycle as it gets
> > measured from a falling edge. By contrast, the succeeding reloads are
> > measured from rising edge to rising edge.
> >
> > Neglecting that complication, the behaviour of the counter should be the
> > same from one reload to the next. The sequence is always:
> >
> > N, N-1, N-2, ... 2, 1, 0, -1, N, N-1, N-2, ...
> >
> > But at the first reload, the present driver does this instead:
> >
> > N, N-1, N-2, ... 2, 1, 0, -1, N-1, N-2, ...
> >
> > Fix this deviation for both timer 1 and timer 2, and allow for the fact
> > that on a real 6522 the timer 2 counter is not reloaded when it wraps.
> >
> > Signed-off-by: Finn Thain
> > ---
> > hw/misc/mos6522.c | 19 +++
> > 1 file changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > index 5b1657ac0d..0a241fe9f8 100644
> > --- a/hw/misc/mos6522.c
> > +++ b/hw/misc/mos6522.c
> > @@ -63,15 +63,16 @@ static unsigned int get_counter(MOS6522State *s,
> > MOS6522Timer *ti)
> > if (ti->index == 0) {
> > /* the timer goes down from latch to -1 (period of latch + 2) */
> > if (d <= (ti->counter_value + 1)) {
> > -counter = (ti->counter_value - d) & 0x;
> > +counter = ti->counter_value - d;
> > } else {
> > -counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
> > -counter = (ti->latch - counter) & 0x;
> > +int64_t d_post_reload = d - (ti->counter_value + 2);
> > +/* XXX this calculation assumes that ti->latch has not changed
> > */
> > +counter = ti->latch - (d_post_reload % (ti->latch + 2));
> > }
> > } else {
> > -counter = (ti->counter_value - d) & 0x;
> > +counter = ti->counter_value - d;
> > }
> > -return counter;
> > +return counter & 0x;
> > }
> > static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int
> > val)
> > @@ -103,11 +104,13 @@ static int64_t get_next_irq_time(MOS6522State *s,
> > MOS6522Timer *ti,
> > /* the timer goes down from latch to -1 (period of latch + 2) */
> > if (d <= (ti->counter_value + 1)) {
> > -counter = (ti->counter_value - d) & 0x;
> > +counter = ti->counter_value - d;
> > } else {
> > -counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
> > -counter = (ti->latch - counter) & 0x;
> > +int64_t d_post_reload = d - (ti->counter_value + 2);
> > +/* XXX this calculation assumes that ti->latch has not changed */
> > +counter = ti->latch - (d_post_reload % (ti->latch + 2));
> > }
> > +counter &= 0x;
> > /* Note: we consider the irq is raised on 0 */
> > if (counter == 0x) {
>
> I think the code looks right, but I couldn't see an explicit reference to this
> behaviour in
> http://archive.6502.org/datasheets/mos_6522_preliminary_nov_1977.pdf.
Yes, that datasheet is missing a lot of information.
> Presumably this matches what you've observed on real hardware?
>
Yes.
Re: [RFC 10/10] hw/mos6522: Synchronize timer interrupt and timer counter
On Wed, 25 Aug 2021, Mark Cave-Ayland wrote: > > Unfortunately the datasheet I was using for reference doesn't appear to > have the relevant detail here. Have you got a reference to the datasheet > you're using which shows what happens to the timers at the zero crossing > point? > The datasheets which I normally refer to (Rockwell and Synertek) agree about this. Also, I established the sequence experimentally when I was writing the clocksource drivers for Linux/m68k.
Re: [RFC 05/10] hw/mos6522: Don't clear T1 interrupt flag on latch write
On Wed, 25 Aug 2021, Mark Cave-Ayland wrote:
> On 24/08/2021 11:09, Finn Thain wrote:
>
> > The Synertek datasheet says, "A write to T1L-H loads an 8-bit count value
> > into the latch. A read of T1L-H transfers the contents of the latch to
> > the data bus. Neither operation has an affect [sic] on the interrupt
> > flag."
> >
> > Signed-off-by: Finn Thain
> > ---
> > hw/misc/mos6522.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> > index c0d6bee4cc..8991f4 100644
> > --- a/hw/misc/mos6522.c
> > +++ b/hw/misc/mos6522.c
> > @@ -313,7 +313,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t
> > val, unsigned size)
> > break;
> > case VIA_REG_T1LH:
> > s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8);
> > -s->ifr &= ~T1_INT;
> > break;
> > case VIA_REG_T2CL:
> > s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
>
> Hmmm. The reference document I used for QEMU's 6522 device is at
> http://archive.6502.org/datasheets/mos_6522_preliminary_nov_1977.pdf and
> according to page 6 and the section "Writing the Timer 1 Registers" writing to
> the high byte of the latch does indeed clear the T1 interrupt flag.
>
> Side note: there is reference in Gary Davidian's excellent CHM video that
> 6522s obtained from different manufacturers had different behaviours, and
> there are also web pages mentioning that 6522s integrated as part of other
> silicon e.g. IOSB/CUDA also had their own bugs... :/
>
The MOS document you've cited is much older than the Synertek and Rockwell
devices. The datasheets for the Synertek and Rockwell parts disagree with
MOS about T1LH behaviour. Apple certainly used SY6522 devices in my Mac II
and I'd assumed Apple would have used compatible logic cores in the custom
ICs found in later models. But I don't really trust assumptions and
datasheets so I wrote the Linux patch below and ran it on my Quadra 630.
diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
index 3d11d6219cdd..ed41f6ae2bf2 100644
--- a/arch/m68k/mac/via.c
+++ b/arch/m68k/mac/via.c
@@ -634,3 +634,27 @@ static u64 mac_read_clk(struct clocksource *cs)
return ticks;
}
+
+static int baz(void)
+{
+ u8 a, b, c;
+
+ local_irq_disable();
+
+ while (!(via1[vIFR] & VIA_TIMER_1_INT))
+ continue;
+ a = via1[vIFR] & VIA_TIMER_1_INT;
+ via1[vT1LH] = via1[vT1LH];
+ b = via1[vIFR] & VIA_TIMER_1_INT;
+ via1[vT1LL] = via1[vT1LL];
+ c = via1[vIFR] & VIA_TIMER_1_INT;
+
+ printk("a == %2x\n", a);
+ printk("b == %2x\n", b);
+ printk("c == %2x\n", c);
+
+ local_irq_enable();
+
+ return 0;
+}
+late_initcall(baz);
Based on the Synertek datasheet* one would expect to see b equal to a but
I got this result instead:
[ 10.45] a == 40
[ 10.45] b == 0
[ 10.45] c == 0
This amounts to a MOS design flaw and I doubt that this result from my
Quadra 630 would apply to other Mac models. So it would be great to see
the output from a Quadra 800. But until then, let's disregard this patch.
* http://archive.6502.org/datasheets/synertek_sy6522.pdf
[RFC 10/10] hw/mos6522: Synchronize timer interrupt and timer counter
We rely on a QEMUTimer callback to set the interrupt flag, and this races
with counter register accesses, such that the guest might see the counter
reloaded but might not see the interrupt flagged.
According to the datasheet, a real 6522 device counts down to , then
raises the relevant IRQ. After the count, the counter reloads from
the latch (for timer 1) or continues to decrement thru FFFE (for timer 2).
Therefore, the guest operating system may read zero from T1CH and infer
that the counter has not yet wrapped (given another full count hasn't
yet elapsed.)
Similarly, the guest may find the timer interrupt flag to be set and
infer that the counter is non-zero (given another full count hasn't yet
elapsed).
Synchronize the timer counter and interrupt flag such that the guest will
observe the correct sequence of states. (It's still not right, because in
reality it's not possible to access the registers more than once per
"phase 2" clock cycle.)
Eliminate the duplication of logic in get_counter() and
get_next_irq_time() by calling the former before the latter.
Note that get_counter() is called prior to changing the latch. This is
because get_counter() may need to use the old latch value in order to
reload the counter.
Signed-off-by: Finn Thain
---
hw/misc/mos6522.c | 154 --
hw/misc/trace-events | 2 +-
include/hw/misc/mos6522.h | 8 +-
3 files changed, 88 insertions(+), 76 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 23a440b64f..bd5df4963b 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -52,26 +52,58 @@ static void mos6522_update_irq(MOS6522State *s)
}
}
+static void mos6522_timer_raise_irq(MOS6522State *s, MOS6522Timer *ti)
+{
+if (ti->state == irq) {
+return;
+}
+ti->state = irq;
+if (ti->index == 0) {
+s->ifr |= T1_INT;
+} else {
+s->ifr |= T2_INT;
+}
+mos6522_update_irq(s);
+}
+
static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti, int64_t now)
{
int64_t d;
unsigned int counter;
-
+bool reload;
+
+/*
+ * Timer 1 counts down from the latch value to -1 (period of latch + 2),
+ * then raises its interrupt and reloads.
+ * Timer 2 counts down from the latch value to -1, then raises its
+ * interrupt and continues to -2 and so on without any further interrupts.
+ * (In reality, the first count should be measured from the falling edge
+ * of the "phase two" clock, making its period N + 1.5. The subsequent
+ * counts have period N + 2. This detail has been ignored here.)
+ */
d = muldiv64(now - ti->load_time,
ti->frequency, NANOSECONDS_PER_SECOND);
-if (ti->index == 0) {
-/* the timer goes down from latch to -1 (period of latch + 2) */
-if (d <= (ti->counter_value + 1)) {
-counter = ti->counter_value - d;
-} else {
-int64_t d_post_reload = d - (ti->counter_value + 2);
-/* XXX this calculation assumes that ti->latch has not changed */
-counter = ti->latch - (d_post_reload % (ti->latch + 2));
-}
-} else {
-counter = ti->counter_value - d;
+reload = (d >= ti->counter_value + 2);
+
+if (ti->index == 0 && reload) {
+int64_t more_reloads;
+
+d -= ti->counter_value + 2;
+more_reloads = d / (ti->latch + 2);
+d -= more_reloads * (ti->latch + 2);
+ti->load_time += muldiv64(ti->counter_value + 2 +
+ more_reloads * (ti->latch + 2),
+ NANOSECONDS_PER_SECOND, ti->frequency);
+ti->counter_value = ti->latch;
}
+
+counter = ti->counter_value - d;
+
+if (reload) {
+mos6522_timer_raise_irq(s, ti);
+}
+
return counter & 0x;
}
@@ -80,7 +112,7 @@ static void set_counter(MOS6522State *s, MOS6522Timer *ti,
unsigned int val)
trace_mos6522_set_counter(1 + ti->index, val);
ti->load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
ti->counter_value = val;
-ti->oneshot_fired = false;
+ti->state = decrement;
if (ti->index == 0) {
mos6522_timer1_update(s, ti, ti->load_time);
} else {
@@ -91,38 +123,15 @@ static void set_counter(MOS6522State *s, MOS6522Timer *ti,
unsigned int val)
static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
int64_t now)
{
-int64_t d, next_time;
-unsigned int counter;
+int64_t next_time;
if (ti->frequency == 0) {
return INT64_MAX;
}
-/* current counter value */
-d = muldiv64(now - ti->load_time,
- ti->frequency, NANOSECONDS_PER_SECOND);
-
-/* the timer goes down from latch to -1 (period of l
[RFC 08/10] hw/mos6522: Call mos6522_update_irq() when appropriate
It necessary to call mos6522_update_irq() when the interrupt flags
change and unnecessary when they haven't.
Signed-off-by: Finn Thain
---
hw/misc/mos6522.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 0a241fe9f8..0dd3ccf945 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -208,11 +208,13 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned
size)
s->timers[0].oneshot_fired = true;
mos6522_timer1_update(s, &s->timers[0], now);
s->ifr |= T1_INT;
+mos6522_update_irq(s);
}
if (now >= s->timers[1].next_irq_time) {
s->timers[1].oneshot_fired = true;
mos6522_timer2_update(s, &s->timers[1], now);
s->ifr |= T2_INT;
+mos6522_update_irq(s);
}
switch (addr) {
case VIA_REG_B:
@@ -237,7 +239,6 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned
size)
break;
case VIA_REG_T1CH:
val = get_counter(s, &s->timers[0]) >> 8;
-mos6522_update_irq(s);
break;
case VIA_REG_T1LL:
val = s->timers[0].latch & 0xff;
--
2.26.3
[RFC 09/10] hw/mos6522: Avoid using discrepant QEMU clock values
mos6522_read() and mos6522_write() may call various functions to determine
timer irq state, timer counter value and QEMUTimer deadline. All called
functions must use the same value for the present time.
Signed-off-by: Finn Thain
---
hw/misc/mos6522.c | 51 +--
1 file changed, 27 insertions(+), 24 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 0dd3ccf945..23a440b64f 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -39,9 +39,9 @@
/* XXX: implement all timer modes */
static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
- int64_t current_time);
+ int64_t now);
static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
- int64_t current_time);
+ int64_t now);
static void mos6522_update_irq(MOS6522State *s)
{
@@ -52,12 +52,12 @@ static void mos6522_update_irq(MOS6522State *s)
}
}
-static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
+static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti, int64_t now)
{
int64_t d;
unsigned int counter;
-d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
+d = muldiv64(now - ti->load_time,
ti->frequency, NANOSECONDS_PER_SECOND);
if (ti->index == 0) {
@@ -89,7 +89,7 @@ static void set_counter(MOS6522State *s, MOS6522Timer *ti,
unsigned int val)
}
static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
- int64_t current_time)
+ int64_t now)
{
int64_t d, next_time;
unsigned int counter;
@@ -99,7 +99,7 @@ static int64_t get_next_irq_time(MOS6522State *s,
MOS6522Timer *ti,
}
/* current counter value */
-d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
+d = muldiv64(now - ti->load_time,
ti->frequency, NANOSECONDS_PER_SECOND);
/* the timer goes down from latch to -1 (period of latch + 2) */
@@ -123,20 +123,19 @@ static int64_t get_next_irq_time(MOS6522State *s,
MOS6522Timer *ti,
trace_mos6522_get_next_irq_time(ti->latch, d, next_time - d);
next_time = muldiv64(next_time, NANOSECONDS_PER_SECOND, ti->frequency) +
ti->load_time;
-
-if (next_time <= current_time) {
-next_time = current_time + 1;
-}
return next_time;
}
static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
- int64_t current_time)
+ int64_t now)
{
if (!ti->timer) {
return;
}
-ti->next_irq_time = get_next_irq_time(s, ti, current_time);
+ti->next_irq_time = get_next_irq_time(s, ti, now);
+if (ti->next_irq_time <= now) {
+ti->next_irq_time = now + 1;
+}
if ((s->ier & T1_INT) == 0 ||
((s->acr & T1MODE) == T1MODE_ONESHOT && ti->oneshot_fired)) {
timer_del(ti->timer);
@@ -146,12 +145,15 @@ static void mos6522_timer1_update(MOS6522State *s,
MOS6522Timer *ti,
}
static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
- int64_t current_time)
+ int64_t now)
{
if (!ti->timer) {
return;
}
-ti->next_irq_time = get_next_irq_time(s, ti, current_time);
+ti->next_irq_time = get_next_irq_time(s, ti, now);
+if (ti->next_irq_time <= now) {
+ti->next_irq_time = now + 1;
+}
if ((s->ier & T2_INT) == 0 || (s->acr & T2MODE) || ti->oneshot_fired) {
timer_del(ti->timer);
} else {
@@ -163,9 +165,10 @@ static void mos6522_timer1_expired(void *opaque)
{
MOS6522State *s = opaque;
MOS6522Timer *ti = &s->timers[0];
+int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
ti->oneshot_fired = true;
-mos6522_timer1_update(s, ti, ti->next_irq_time);
+mos6522_timer1_update(s, ti, now);
s->ifr |= T1_INT;
mos6522_update_irq(s);
}
@@ -174,9 +177,10 @@ static void mos6522_timer2_expired(void *opaque)
{
MOS6522State *s = opaque;
MOS6522Timer *ti = &s->timers[1];
+int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
ti->oneshot_fired = true;
-mos6522_timer2_update(s, ti, ti->next_irq_time);
+mos6522_timer2_update(s, ti, now);
s->ifr |= T2_INT;
mos6522_update_irq(s);
}
@@ -233,12 +237,12 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned
size)
val = s->dira;
break;
case VIA_REG_T1CL:
-val = get_counter(s, &s->timers[0]) & 0xff;
+val = get_counter(s, &s->timers[0], now) & 0xff;
s->ifr &= ~T1_INT;
mos6522_update
[RFC 06/10] hw/mos6522: Implement oneshot mode
Signed-off-by: Finn Thain
---
hw/misc/mos6522.c | 19 ---
include/hw/misc/mos6522.h | 3 +++
2 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 8991f4..5b1657ac0d 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -79,6 +79,7 @@ static void set_counter(MOS6522State *s, MOS6522Timer *ti,
unsigned int val)
trace_mos6522_set_counter(1 + ti->index, val);
ti->load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
ti->counter_value = val;
+ti->oneshot_fired = false;
if (ti->index == 0) {
mos6522_timer1_update(s, ti, ti->load_time);
} else {
@@ -133,7 +134,8 @@ static void mos6522_timer1_update(MOS6522State *s,
MOS6522Timer *ti,
return;
}
ti->next_irq_time = get_next_irq_time(s, ti, current_time);
-if ((s->ier & T1_INT) == 0 || (s->acr & T1MODE) != T1MODE_CONT) {
+if ((s->ier & T1_INT) == 0 ||
+((s->acr & T1MODE) == T1MODE_ONESHOT && ti->oneshot_fired)) {
timer_del(ti->timer);
} else {
timer_mod(ti->timer, ti->next_irq_time);
@@ -147,7 +149,7 @@ static void mos6522_timer2_update(MOS6522State *s,
MOS6522Timer *ti,
return;
}
ti->next_irq_time = get_next_irq_time(s, ti, current_time);
-if ((s->ier & T2_INT) == 0) {
+if ((s->ier & T2_INT) == 0 || (s->acr & T2MODE) || ti->oneshot_fired) {
timer_del(ti->timer);
} else {
timer_mod(ti->timer, ti->next_irq_time);
@@ -159,6 +161,7 @@ static void mos6522_timer1_expired(void *opaque)
MOS6522State *s = opaque;
MOS6522Timer *ti = &s->timers[0];
+ti->oneshot_fired = true;
mos6522_timer1_update(s, ti, ti->next_irq_time);
s->ifr |= T1_INT;
mos6522_update_irq(s);
@@ -169,6 +172,7 @@ static void mos6522_timer2_expired(void *opaque)
MOS6522State *s = opaque;
MOS6522Timer *ti = &s->timers[1];
+ti->oneshot_fired = true;
mos6522_timer2_update(s, ti, ti->next_irq_time);
s->ifr |= T2_INT;
mos6522_update_irq(s);
@@ -198,10 +202,12 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned
size)
int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
if (now >= s->timers[0].next_irq_time) {
+s->timers[0].oneshot_fired = true;
mos6522_timer1_update(s, &s->timers[0], now);
s->ifr |= T1_INT;
}
if (now >= s->timers[1].next_irq_time) {
+s->timers[1].oneshot_fired = true;
mos6522_timer2_update(s, &s->timers[1], now);
s->ifr |= T2_INT;
}
@@ -279,6 +285,7 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val,
unsigned size)
{
MOS6522State *s = opaque;
MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
+int64_t now;
trace_mos6522_write(addr, val);
@@ -318,9 +325,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val,
unsigned size)
s->timers[1].latch = (s->timers[1].latch & 0xff00) | val;
break;
case VIA_REG_T2CH:
-/* To ensure T2 generates an interrupt on zero crossing with the
- common timer code, write the value directly from the latch to
- the counter */
s->timers[1].latch = (s->timers[1].latch & 0xff) | (val << 8);
s->ifr &= ~T2_INT;
set_counter(s, &s->timers[1], s->timers[1].latch);
@@ -330,8 +334,9 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val,
unsigned size)
break;
case VIA_REG_ACR:
s->acr = val;
-mos6522_timer1_update(s, &s->timers[0],
- qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+mos6522_timer1_update(s, &s->timers[0], now);
+mos6522_timer2_update(s, &s->timers[1], now);
break;
case VIA_REG_PCR:
s->pcr = val;
diff --git a/include/hw/misc/mos6522.h b/include/hw/misc/mos6522.h
index fc95d22b0f..94b1dc324c 100644
--- a/include/hw/misc/mos6522.h
+++ b/include/hw/misc/mos6522.h
@@ -50,8 +50,10 @@
#define T1_INT 0x40/* Timer 1 interrupt */
/* Bits in ACR */
+#define T2MODE 0x20/* Timer 2 mode */
#define T1MODE 0xc0/* Timer 1 mode */
#define T1MODE_CONT0x40/* continuous interrupts */
+#define T1MODE_ONESHOT 0x00/* timed interrupt */
/* VIA registers */
#define VIA_REG_B 0x00
@@ -83,6 +85,7 @@ typedef struct MOS6522Timer {
int64_t next_irq_time;
uint64_t frequency;
QEMUTimer *timer;
+bool oneshot_fired;
} MOS6522Timer;
/**
--
2.26.3
[RFC 07/10] hw/mos6522: Fix initial timer counter reload
The first reload of timer 1 is early by half of a clock cycle as it gets
measured from a falling edge. By contrast, the succeeding reloads are
measured from rising edge to rising edge.
Neglecting that complication, the behaviour of the counter should be the
same from one reload to the next. The sequence is always:
N, N-1, N-2, ... 2, 1, 0, -1, N, N-1, N-2, ...
But at the first reload, the present driver does this instead:
N, N-1, N-2, ... 2, 1, 0, -1, N-1, N-2, ...
Fix this deviation for both timer 1 and timer 2, and allow for the fact
that on a real 6522 the timer 2 counter is not reloaded when it wraps.
Signed-off-by: Finn Thain
---
hw/misc/mos6522.c | 19 +++
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 5b1657ac0d..0a241fe9f8 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -63,15 +63,16 @@ static unsigned int get_counter(MOS6522State *s,
MOS6522Timer *ti)
if (ti->index == 0) {
/* the timer goes down from latch to -1 (period of latch + 2) */
if (d <= (ti->counter_value + 1)) {
-counter = (ti->counter_value - d) & 0x;
+counter = ti->counter_value - d;
} else {
-counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
-counter = (ti->latch - counter) & 0x;
+int64_t d_post_reload = d - (ti->counter_value + 2);
+/* XXX this calculation assumes that ti->latch has not changed */
+counter = ti->latch - (d_post_reload % (ti->latch + 2));
}
} else {
-counter = (ti->counter_value - d) & 0x;
+counter = ti->counter_value - d;
}
-return counter;
+return counter & 0x;
}
static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val)
@@ -103,11 +104,13 @@ static int64_t get_next_irq_time(MOS6522State *s,
MOS6522Timer *ti,
/* the timer goes down from latch to -1 (period of latch + 2) */
if (d <= (ti->counter_value + 1)) {
-counter = (ti->counter_value - d) & 0x;
+counter = ti->counter_value - d;
} else {
-counter = (d - (ti->counter_value + 1)) % (ti->latch + 2);
-counter = (ti->latch - counter) & 0x;
+int64_t d_post_reload = d - (ti->counter_value + 2);
+/* XXX this calculation assumes that ti->latch has not changed */
+counter = ti->latch - (d_post_reload % (ti->latch + 2));
}
+counter &= 0x;
/* Note: we consider the irq is raised on 0 */
if (counter == 0x) {
--
2.26.3
[RFC 03/10] hw/mos6522: Remove redundant mos6522_timer1_update() calls
Reads and writes to the TL and TC registers have no immediate effect on a running timer, with the exception of a write to TCH. Hence these mos6522_timer_update() calls are not needed. Signed-off-by: Finn Thain --- hw/misc/mos6522.c | 7 --- 1 file changed, 7 deletions(-) diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c index ff246b5437..1d4a56077e 100644 --- a/hw/misc/mos6522.c +++ b/hw/misc/mos6522.c @@ -234,7 +234,6 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned size) val = s->timers[0].latch & 0xff; break; case VIA_REG_T1LH: -/* XXX: check this */ val = (s->timers[0].latch >> 8) & 0xff; break; case VIA_REG_T2CL: @@ -303,8 +302,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) break; case VIA_REG_T1CL: s->timers[0].latch = (s->timers[0].latch & 0xff00) | val; -mos6522_timer1_update(s, &s->timers[0], - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); break; case VIA_REG_T1CH: s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8); @@ -313,14 +310,10 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) break; case VIA_REG_T1LL: s->timers[0].latch = (s->timers[0].latch & 0xff00) | val; -mos6522_timer1_update(s, &s->timers[0], - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); break; case VIA_REG_T1LH: s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8); s->ifr &= ~T1_INT; -mos6522_timer1_update(s, &s->timers[0], - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)); break; case VIA_REG_T2CL: s->timers[1].latch = (s->timers[1].latch & 0xff00) | val; -- 2.26.3
[RFC 05/10] hw/mos6522: Don't clear T1 interrupt flag on latch write
The Synertek datasheet says, "A write to T1L-H loads an 8-bit count value into the latch. A read of T1L-H transfers the contents of the latch to the data bus. Neither operation has an affect [sic] on the interrupt flag." Signed-off-by: Finn Thain --- hw/misc/mos6522.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c index c0d6bee4cc..8991f4 100644 --- a/hw/misc/mos6522.c +++ b/hw/misc/mos6522.c @@ -313,7 +313,6 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) break; case VIA_REG_T1LH: s->timers[0].latch = (s->timers[0].latch & 0xff) | (val << 8); -s->ifr &= ~T1_INT; break; case VIA_REG_T2CL: s->timers[1].latch = (s->timers[1].latch & 0xff00) | val; -- 2.26.3
[RFC 04/10] hw/mos6522: Rename timer callback functions
This improves readability.
Signed-off-by: Finn Thain
---
hw/misc/mos6522.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 1d4a56077e..c0d6bee4cc 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -154,7 +154,7 @@ static void mos6522_timer2_update(MOS6522State *s,
MOS6522Timer *ti,
}
}
-static void mos6522_timer1(void *opaque)
+static void mos6522_timer1_expired(void *opaque)
{
MOS6522State *s = opaque;
MOS6522Timer *ti = &s->timers[0];
@@ -164,7 +164,7 @@ static void mos6522_timer1(void *opaque)
mos6522_update_irq(s);
}
-static void mos6522_timer2(void *opaque)
+static void mos6522_timer2_expired(void *opaque)
{
MOS6522State *s = opaque;
MOS6522Timer *ti = &s->timers[1];
@@ -445,8 +445,10 @@ static void mos6522_init(Object *obj)
s->timers[i].index = i;
}
-s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s);
-s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s);
+s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+ mos6522_timer1_expired, s);
+s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+ mos6522_timer2_expired, s);
}
static void mos6522_finalize(Object *obj)
--
2.26.3
[RFC 02/10] hw/mos6522: Remove get_counter_value() methods and functions
This code appears to be unnecessary.
Also, these routines don't return the counter value but a time interval
between counter values, so they are misnamed.
Signed-off-by: Finn Thain
---
hw/misc/mos6522.c | 22 ++
1 file changed, 2 insertions(+), 20 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index a478c1ca43..ff246b5437 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -52,23 +52,13 @@ static void mos6522_update_irq(MOS6522State *s)
}
}
-static uint64_t get_counter_value(MOS6522State *s, MOS6522Timer *ti)
-{
-MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
-
-if (ti->index == 0) {
-return mdc->get_timer1_counter_value(s, ti);
-} else {
-return mdc->get_timer2_counter_value(s, ti);
-}
-}
-
static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
{
int64_t d;
unsigned int counter;
-d = get_counter_value(s, ti);
+d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
+ ti->frequency, NANOSECONDS_PER_SECOND);
if (ti->index == 0) {
/* the timer goes down from latch to -1 (period of latch + 2) */
@@ -191,12 +181,6 @@ static void mos6522_set_sr_int(MOS6522State *s)
mos6522_update_irq(s);
}
-static uint64_t mos6522_get_counter_value(MOS6522State *s, MOS6522Timer *ti)
-{
-return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
-ti->frequency, NANOSECONDS_PER_SECOND);
-}
-
static void mos6522_portA_write(MOS6522State *s)
{
qemu_log_mask(LOG_UNIMP, "portA_write unimplemented\n");
@@ -498,8 +482,6 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
mdc->portB_write = mos6522_portB_write;
mdc->portA_write = mos6522_portA_write;
mdc->update_irq = mos6522_update_irq;
-mdc->get_timer1_counter_value = mos6522_get_counter_value;
-mdc->get_timer2_counter_value = mos6522_get_counter_value;
}
static const TypeInfo mos6522_type_info = {
--
2.26.3
[RFC 01/10] hw/mos6522: Remove get_load_time() methods and functions
This code appears to be unnecessary.
Signed-off-by: Finn Thain
---
hw/misc/mos6522.c | 22 +-
1 file changed, 1 insertion(+), 21 deletions(-)
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index 1c57332b40..a478c1ca43 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -63,17 +63,6 @@ static uint64_t get_counter_value(MOS6522State *s,
MOS6522Timer *ti)
}
}
-static uint64_t get_load_time(MOS6522State *s, MOS6522Timer *ti)
-{
-MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
-
-if (ti->index == 0) {
-return mdc->get_timer1_load_time(s, ti);
-} else {
-return mdc->get_timer2_load_time(s, ti);
-}
-}
-
static unsigned int get_counter(MOS6522State *s, MOS6522Timer *ti)
{
int64_t d;
@@ -98,7 +87,7 @@ static unsigned int get_counter(MOS6522State *s, MOS6522Timer
*ti)
static void set_counter(MOS6522State *s, MOS6522Timer *ti, unsigned int val)
{
trace_mos6522_set_counter(1 + ti->index, val);
-ti->load_time = get_load_time(s, ti);
+ti->load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
ti->counter_value = val;
if (ti->index == 0) {
mos6522_timer1_update(s, ti, ti->load_time);
@@ -208,13 +197,6 @@ static uint64_t mos6522_get_counter_value(MOS6522State *s,
MOS6522Timer *ti)
ti->frequency, NANOSECONDS_PER_SECOND);
}
-static uint64_t mos6522_get_load_time(MOS6522State *s, MOS6522Timer *ti)
-{
-uint64_t load_time = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-
-return load_time;
-}
-
static void mos6522_portA_write(MOS6522State *s)
{
qemu_log_mask(LOG_UNIMP, "portA_write unimplemented\n");
@@ -518,8 +500,6 @@ static void mos6522_class_init(ObjectClass *oc, void *data)
mdc->update_irq = mos6522_update_irq;
mdc->get_timer1_counter_value = mos6522_get_counter_value;
mdc->get_timer2_counter_value = mos6522_get_counter_value;
-mdc->get_timer1_load_time = mos6522_get_load_time;
-mdc->get_timer2_load_time = mos6522_get_load_time;
}
static const TypeInfo mos6522_type_info = {
--
2.26.3
[RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements
This is a patch series that I started last year. The aim was to try to
get a monotonic clocksource for Linux/m68k guests. That aim hasn't been
achieved yet (for q800 machines) but I'm submitting the patch series as
an RFC because,
- It does improve 6522 emulation fidelity.
- It allows Linux/m68k to make use of the additional timer that the
hardware indeed offers but which QEMU omits. This has several
benefits for Linux guests [1].
- I see that Mark has been working on timer emulation issues in his
github repo [2] and it seems likely that MacOS, NetBSD or A/UX guests
will also require better 6522 emulation.
To make collaboration easier these patches can also be fetched from
github [3].
On a real Quadra, accesses to the SY6522 chips are slow because they are
synchronous with the 783360 Hz "phase 2" clock. In QEMU, they are slow
only because of the division operation in the timer count calculation.
This patch series improves the fidelity of the emulated chip, but the
price is more division ops. I haven't tried to measure this yet.
The emulated 6522 still deviates from the behaviour of the real thing,
however. For example, two consecutive accesses to a real 6522 timer
counter can never yield the same value. This is not true of the 6522 in
QEMU 6 wherein two consecutive accesses to a timer count register have
been observed to yield the same value.
Linux is not particularly robust in the face of a 6522 that deviates
from the usual behaviour. The problem presently affecting a Linux guest
is that its 'via' clocksource is prone to monotonicity failure. That is,
the clocksource counter can jump backwards. This can be observed by
patching Linux like so:
diff --git a/arch/m68k/mac/via.c b/arch/m68k/mac/via.c
--- a/arch/m68k/mac/via.c
+++ b/arch/m68k/mac/via.c
@@ -606,6 +606,8 @@ void __init via_init_clock(void)
clocksource_register_hz(&mac_clk, VIA_CLOCK_FREQ);
}
+static u32 prev_ticks;
+
static u64 mac_read_clk(struct clocksource *cs)
{
unsigned long flags;
@@ -631,6 +633,8 @@ static u64 mac_read_clk(struct clocksource *cs)
count = count_high << 8;
ticks = VIA_TIMER_CYCLES - count;
ticks += clk_offset + clk_total;
+if (ticks < prev_ticks) pr_warn("%s: %u < %u\n", __func__, ticks, prev_ticks);
+prev_ticks = ticks;
local_irq_restore(flags);
return ticks;
This problem can be partly blamed on a 6522 design limitation, which is
that the timer counter has no overflow register. Hence, if a timer
counter wraps around and the kernel is late to handle the subsequent
interrupt, the kernel can't account for any missed ticks.
On a real Quadra, the kernel mitigates this limitation by minimizing
interrupt latency. But on QEMU, interrupt latency is unbounded. This
can't be mitigated by the guest kernel at all and leads to clock drift.
This can be observed by patching QEMU like so:
diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -379,6 +379,12 @@ void mos6522_write(void *opaque, hwaddr addr, uint64_t
val, unsigned size)
s->pcr = val;
break;
case VIA_REG_IFR:
+if (val & T1_INT) {
+static int64_t last_t1_int_cleared;
+int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+if (now - last_t1_int_cleared > 2000) printf("\t%s: t1 int
clear is late\n", __func__);
+last_t1_int_cleared = now;
+}
/* reset bits */
s->ifr &= ~val;
mos6522_update_irq(s);
This logic asserts that, given that Linux/m68k sets CONFIG_HZ to 100,
the emulator will theoretically see each timer 1 interrupt cleared
within 20 ms of the previous one. But that deadline is often missed on
my QEMU host [4].
On real Mac hardware you could observe the same scenario if a high
priority interrupt were to sufficiently delay the timer interrupt
handler. (This is the reason why the VIA1 interrupt priority gets
increased from level 1 to level 5 when running on Quadras.)
Anyway, for now, the clocksource monotonicity problem in Linux/mac68k
guests is still unresolved. Nonetheless, I think this patch series does
improve the situation.
[1] I've also been working on some improvements to Linux/m68k based on
Arnd Bergman's clockevent RFC patch,
https://lore.kernel.org/linux-m68k/20201008154651.1901126-14-a...@arndb.de/
The idea is to add a oneshot clockevent device by making use of the
second VIA1 timer. This approach should help mitigate the clock drift
problem as well as assist with GENERIC_CLOCKEVENTS adoption.
[2] https://github.com/mcayland/qemu/commits/q800.upstream
[3] https://github.com/fthain/qemu/commits/via-timer/
[4] This theoretical 20 ms deadline is not missed prior to every
backwards jump in the clocksource counter. AFAICT, that's because the
true deadline i
Re: [PATCH v3 0/8] dp8393x: fixes and improvements
On Mon, 12 Jul 2021, Finn Thain wrote:
> On Sun, 11 Jul 2021, Philippe Mathieu-Daudé wrote:
>
> >
> > > If I'm right that the big_endian flag should go away, commit
> > > b1600ff195 ("hw/mips/jazz: specify correct endian for dp8393x
> > > device") has already taken mainline in the wrong direction and
> > > amounts to churn.
> >
> > We might figure out with a BE guest image, the remove the endian flag.
>
> Yes, it's hard to make progress without a BE guest. However, for testing
> dp8393x we probably don't need a disk image. I think we only need
> working firmware, since the RISC/os firmware appears to implement BOOTP
> and TFTP and appears to contain a SONIC driver.
I think we probably can install RISC/os once the firmware can be made to
work.
The file "RISCos_5.01.iso", found in the Bitsavers archive, contains
several kernel binaries, one of which is "unix.r4030eb_std".
From the "r4030" in its name, and from the symbol names and string
constants it contains, this binary appears to have all the drivers for the
MIPS Magnum 4000.
Re: [PATCH v3 0/8] dp8393x: fixes and improvements
On Sun, 11 Jul 2021, Philippe Mathieu-Daudé wrote:
>
> > If I'm right that the big_endian flag should go away, commit
> > b1600ff195 ("hw/mips/jazz: specify correct endian for dp8393x device")
> > has already taken mainline in the wrong direction and amounts to
> > churn.
>
> We might figure out with a BE guest image, the remove the endian flag.
Yes, it's hard to make progress without a BE guest. However, for testing
dp8393x we probably don't need a disk image. I think we only need working
firmware, since the RISC/os firmware appears to implement BOOTP and TFTP
and appears to contain a SONIC driver.
This page discusses using the "MIPS Monitor" firmware on a Mips Magnum
3000 machine to netboot NetBSD/mipsco:
https://www.ludd.ltu.se/~ragge/htdocs/Ports/mipsco/install.html
Note that the firmware banner message looks like this:
Rx3230 MIPS Monitor: Version 5.43 OPT Mon May 13 17:31:12 PDT 1991 root
It appears that there may be a similar firmware for MIPS Magnum at,
https://gunkies.org/wiki/Installing_Windows_NT_4.0_on_Qemu(MIPS)
(I suppose this is the firmware floppy that was once found at ftp.sgi.com,
after SGI acquired MIPS?)
The file RISCOS.RAW found in SETUP.ZIP appears to contain various string
constants, both LE and BE, including:
$ swap64 < RISCOS.RAW |strings |egrep "Version|MIPS|Rx|Monitor"
...
Version 5.60 OPT-EB Wed Jun 17 11:23:28 PDT 1992 root
MIPS
Rx4230
%s %s Monitor: %s
...
This looks like the same banner, only for a different machine (as you'd
expect). Unfortunately, nothing happened when I tried to boot that
firmware:
$ ln -s RISCOS.RAW mips_bios.bin
$ qemu-system-mips64 -M magnum -L . -global ds1225y.filename=mips-nvram -serial
mon:stdio -serial null -nic bridge,model=dp83932,mac=00:00:00:aa:bb:cc
I don't know enough about this platform or about QEMU to go much further
with this so I hope that others will be able to help.
I did find this link, which talks a little about the early boot code.
https://web.archive.org/web/20180823065803/http://www.sensi.org/~alec/mips/mips-history.html
Re: [PATCH v3 0/8] dp8393x: fixes and improvements
On Sat, 10 Jul 2021, Philippe Mathieu-Daudé wrote:
>
> The last 2 patches are included for Mark, but I don't plan to merge
>
> them without Finn's Ack, and apparently they require more work.
>
I tested the patch series both with and without the last 2 patches. Both
builds worked fine with my NetBSD/arc, Linux/mipsel and Linux/m68k guests.
Tested-by: Finn Thain
I have no objection to patch 8/8 ("dp8393x: don't force 32-bit register
access"). I asked Mark to explain why it was a bug fix (since it didn't
change QEMU behaviour in my tests) but when I looked into it I found that
he is quite right, the patch does fix a theoretical bug.
My only objection to patch 7/8 ("dp8393x: Rewrite dp8393x_get() /
dp8393x_put()") was that it could be churn.
If I'm right that the big_endian flag should go away, commit b1600ff195
("hw/mips/jazz: specify correct endian for dp8393x device") has already
taken mainline in the wrong direction and amounts to churn.
I have the same reservations about patch 6/8 ("dp8393x: Store CRC using
device configured endianess"). Perhaps that should be NOTFORMERGE too
(even though it too a theoretical bug fix).
Is there a good way to avoid using big_endian for storing the CRC and the
other DMA operations?
BTW, if you see "sn0: receive buffers exhausted" occasionally logged by
the NetBSD 9.2 kernel, accompanied by packet loss, it's not a regression
in QEMU. I first observed it last year when stress testing dp8393x with
NetBSD 5.1. I believe this to be an old NetBSD sn driver bug because Linux
is unaffected.
Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put()
On Wed, 7 Jul 2021, Mark Cave-Ayland wrote: > > You don't need a rootfs to see the jazzsonic driver messages. But if > > you still want one, you could try the mipsel builds from these distros > > (not the 64-bit ones): > > > > https://ftp.jaist.ac.jp/pub/Linux/Gentoo/experimental/mips/stages/ When I tried following my own advice I ran into ABI compatibility problems. It looks like my kernel build doesn't like those binaries, but maybe it's a limitation of the Magnum CPU... ... Run /bin/sh as init process request_module: kmod_concurrent_max (0) close to 0 (max_modprobes: 50), for module binfmt-464c, throttling... request_module: modprobe binfmt-464c cannot be processed, kmod busy with 50 threads for more than 5 seconds now Kernel panic - not syncing: Requested init /bin/sh failed (error -8). > > https://landley.net/aboriginal/downloads/binaries/ The binaries from Aboriginal Linux work okay (that is, rootfs.cpio.gz found in system-image-mipsel.tar.gz). I got a shell prompt using init=/bin/sh but there's no networking support in this minimal busybox build unfortunately. Those binaries have the same ABI as the ones in my busybox build: $ file busybox busybox: ELF 32-bit LSB executable, MIPS, MIPS-I version 1 (SYSV), statically linked, stripped Whereas, Debian/mipsel binaries (like the Gentoo ones) look like this: $ file busybox busybox: ELF 32-bit LSB pie executable, MIPS, MIPS32 rel2 version 1 (SYSV), dynamically linked, interpreter /lib/ld.so.1, for GNU/Linux 3.2.0, BuildID[sha1]=febe1809f2ad8dacb067dfd74505b19c6c69ba65, stripped Eventually I found this page, https://wiki.debian.org/MIPSPort which explains that the Debian/mipsel port switched ABI between Debian 8 and Debian 9. Unfortunately, the Debian 7 and 8 installer ISO images have no initrd so they are no use. I got them from this archive: https://cdimage.debian.org/cdimage/archive/ Anyway, the Debian 8 binaries look like this, and they work too: # file bin/dash bin/dash: ELF 32-bit LSB shared object, MIPS, MIPS-II version 1 (SYSV), dynamically linked, interpreter /lib/ld.so.1, for GNU/Linux 2.6.32, BuildID[sha1]=44f7c1d61d9941db2b1de5dd9629c99e06c30ea8, stripped > > That's true, but then this wouldn't enable testing of Phil's proposed > CRC changes. Having a simple shell with ping and wget/curl is a real > help here. > To generate network traffic you can get the kernel to configure the NIC over DHCP but that does require a different kernel config (see below). > > > If you can provide me with a link to your vmlinux and rootfs with > > > busybox or similar in it, I can take a look to see what is happening > > > here. Otherwise it's almost impossible for me to understand and > > > debug the problem you are seeing on your setup. > > > > > > > Uploading kernels is a hassle (for me) as it brings a trust question > > and requires a file hosting service. I really should use PGP and > > organise a web of trust but that's very difficult given my rural > > location. > > Given that these are only running in a VM I'm not too worried about > trust. Well, untrusted images are okay as long as we are talking about debugging QEMU issues that are not exploitable... With a bit of searching I was able to find a Debian/mipsel 8 rootfs at https://github.com/jubinson/debian-rootfs I can't vouch for it though. It appears to be a page of links to tar files in someone's dropbox. $ sha256sum mipsel-rootfs-20170318T103423Z.tar.gz e6ed1871b29317c85170a07621966a013951ced1c5fb8d679b7519996b803fe8 mipsel-rootfs-20170318T103423Z.tar.gz > I also have a VPS with scp access that I could temporarily grant you > access via an SSH public key if that helps? > Thanks for the offer. But that wouldn't help anyone else reading this. In anycase, I wanted to see whether a real distro could be used. So my plan was to cross-compile a Linux kernel and debootstrap a Debian/mipsel rootfs disk image. The first stage of a debootstrap installation runs on the host... sudo -s truncate -s 1G jessie-mipsel.img mke2fs jessie-mipsel.img mount -o loop jessie-mipsel.img /mnt wget -q -O- https://ftp-master.debian.org/keys/release-8.asc | gpg --import --no-default-keyring --keyring ./debian-release-8.gpg debootstrap --keyring=./debian-release-8.gpg --foreign --arch=mipsel jessie /mnt http://archive.debian.org/debian/ umount /mnt exit git clone git://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git cd linux git checkout linux-5.10.y make ARCH=mips CROSS_COMPILE=mipsel-linux-gnu- clean jazz_defconfig scripts/config -d IPV6 -d WIRELESS -d WLAN -d DEBUG_KERNEL -d EXPERT -d CC_OPTIMIZE_FOR_PERFORMANCE -e CC_OPTIMIZE_FOR_SIZE -e ISO9660_FS -m EXT3_FS -e NFS_FS -e IP_PNP -e ROOT_NFS -e NFS_V2 -e IP_PNP_DHCP -e CMDLINE_BOOL -e MIPS_CMDLINE_BUILTIN_EXTEND --set-str CMDLINE "console=ttyS0 root=/dev/nfs rw" make ARCH=mips CROSS_COMPILE=mipsel-linux-gnu- olddefconfig vmlinux mkisofs -o vmlinux.iso -J -iso-level 3 vmlinux qemu-system-mips64
Re: [PATCH 1/4] dp8393x: don't force 32-bit register access
On Wed, 7 Jul 2021, Mark Cave-Ayland wrote: > However this conflicts with what you mention above that the SONIC is > hard-coded into little-endian mode, in which case we would still need to > keep it. > If you want to fully implement BMODE in QEMU then you'll need to abandon native endiannes for the device implementation. I was not proposing this as it implies more byte swapping. In a real Magnum the SONIC chip is connected to a bus that's not modelled by QEMU. It follows that BMODE serves different purposes than big_endian. I pointed out several semantic differences between BMODE and big_endian, but I think the most significant of those was that endianness is already a property of the memory device being accessed for DMA. Yet big_endian is a property of the dp8393x device. > Certainly we can look to improve things in the future, but without > anyone having a working big-endian MIPS image to test against, I don't > think it's worth guessing what changes are required as we can easily > double the length of this thread and still have no idea if any changes > we've made are correct. > That argument can be applied to other patches in this series also. Anyway, if we agree that the aim is ultimately to remove the big_endian flag then patch 4/4 should probably be re-evaluated in light of that.
Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put()
On Mon, 5 Jul 2021, Mark Cave-Ayland wrote: > On 05/07/2021 02:36, Finn Thain wrote: > > > > Unfortunately I don't have a test mips64el image available to see if > > > this combination works for Linux. Phil, do you have a suitable test > > > kernel and rootfs image available to allow this to be tested? > > > > > > > You can build and boot a mipsel vmlinux by following the steps I > > described previously. In the kernel messages you'll see the jazzsonic > > driver attempt to probe the device. When it succeeds, you'll see the > > MAC address reported. You can also observe the regression I reported > > with regards to patch 2/6, "dp8393x: don't force 32-bit register > > access". > > Those instructions are useful, but since I am not a MIPS developer I > don't have an existing toolchain/kernel tree and rootfs available to > test this. > You don't need a rootfs to see the jazzsonic driver messages. But if you still want one, you could try the mipsel builds from these distros (not the 64-bit ones): https://ftp.jaist.ac.jp/pub/Linux/Gentoo/experimental/mips/stages/ https://landley.net/aboriginal/downloads/binaries/ > If you can provide me with a link to your vmlinux and rootfs with > busybox or similar in it, I can take a look to see what is happening > here. Otherwise it's almost impossible for me to understand and debug > the problem you are seeing on your setup. > Uploading kernels is a hassle (for me) as it brings a trust question and requires a file hosting service. I really should use PGP and organise a web of trust but that's very difficult given my rural location.
Re: [PATCH] tests/acceptance: Test NetBSD 5.1 on the Jazz Magnum machine
On Mon, 5 Jul 2021, Philippe Mathieu-Daudé wrote:
> On 7/5/21 2:58 AM, Finn Thain wrote:
> > On Mon, 5 Jul 2021, Philippe Mathieu-Daudé wrote:
> >
> >> Test NetBSD 5.1 on the Jazz Magnum machine. As the firmware is not
> >> redistributable, it has to be extracted from the floppy configuration
> >> disk coming with a Mips Magnum 4000 system, then the NTPROM_BIN_PATH
> >> environment variable has to be set. For convenience a NVRAM pre-
> >> initialized to boot NetBSD is included. The test can be run as:
> >>
> >> $ NTPROM_BIN_PATH=/path/to/ntprom.bin \
> >> avocado --show=app,console \
> >> run -t machine:magnum tests/acceptance/
> >> Fetching asset from
> >> tests/acceptance/machine_mips_jazz.py:MipsJazz.test_magnum_netbsd_5_1
> >>(1/1)
> >> tests/acceptance/machine_mips_jazz.py:MipsJazz.test_magnum_netbsd_5_1:
> >> console: EISA Bus 0 Initialization In Progress... Direct Memory Access
> >> (DMA) System Control Port B Timer 1 OK.
> >> console: ARC Multiboot Version 174 (SGI Version 2.6)
> >> console: Copyright (c) 1991,1992 Microsoft Corporation
> >> console: Actions:
> >> console: Start Windows NT
> >> console: Run a program
> >> console: Run setup
> >> console: Use the arrow keys to select.
> >> console: Press Enter to choose.
> >> console: Program to run:
> >> console: scsi(0)cdrom(2)fdisk(0)boot scsi(0)cdrom(2)fdisk(0)netbsd
> >> console: NetBSD/arc Bootstrap, Revision 1.1
> >> console: (bui...@b7.netbsd.org, Sat Nov 6 14:06:36 UTC 2010)
> >> console: devopen: scsi(0)cdrom(2)fdisk(0) type disk file netbsd
> >> console: NetBSD 5.1 (RAMDISK) #0: Sat Nov 6 14:17:36 UTC 2010
> >> console:
> >> bui...@b7.netbsd.org:/home/builds/ab/netbsd-5-1-RELEASE/arc/201011061943Z-obj/home/builds/ab/netbsd-5-1-RELEASE/src/sys/arch/arc/compile/RAMDISK
> >> console: MIPS Magnum
>
> >> +class MipsJazz(Test):
> >> +
> >> +timeout = 60
> >> +
> >> +@skipUnless(os.getenv('NTPROM_BIN_PATH'), 'NTPROM_BIN_PATH not
> >> available')
> >> +def test_magnum_netbsd_5_1(self):
> >> +"""
> >> +:avocado: tags=arch:mips64el
> >> +:avocado: tags=machine:magnum
> >> +:avocado: tags=os:netbsd
> >> +:avocado: tags=device:sonic
> >> +:avocado: tags=device:esp
> >> +"""
> >> +drive_url = ('http://archive.netbsd.org/pub/NetBSD-archive/'
> >> + 'NetBSD-5.1/iso/arccd-5.1.iso')
> >
> > This can be updated to NetBSD 9.2 (the regressions in NetBSD have been
> > fixed).
>
> Indeed, with this change:
>
> -- >8 --
> diff --git a/tests/acceptance/machine_mips_jazz.py
> b/tests/acceptance/machine_mips_jazz.py
> index 0b6640edc12..54968959372 100644
> --- a/tests/acceptance/machine_mips_jazz.py
> +++ b/tests/acceptance/machine_mips_jazz.py
> @@ -33,13 +33,11 @@ def test_magnum_netbsd_5_1(self):
> :avocado: tags=device:sonic
> :avocado: tags=device:esp
> """
> -drive_url = ('http://archive.netbsd.org/pub/NetBSD-archive/'
> - 'NetBSD-5.1/iso/arccd-5.1.iso')
> -drive_hash = ('c91a57fb373636247d1f1ce283a610ba529e208604a'
> - 'f2a9e0237551fb3d25459c7697775af8c8d35a9764e'
> - 'fca87cfb591f363643e93417cfdb8857215ceb405e')
> +drive_url = ('http://cdn.netbsd.org/pub/NetBSD/'
> + 'NetBSD-9.2/images/NetBSD-9.2-arc.iso')
> +drive_hash = '409c61aee5459e762cdb120d2591ed2e'
> drive_path = self.fetch_asset(drive_url, asset_hash=drive_hash,
> - algorithm='sha512')
> + algorithm='md5')
> ntprom_hash = '316de17820192c89b8ee6d9936ab8364a739ca53'
> ntprom_path = self.fetch_asset('file://' +
> os.getenv('NTPROM_BIN_PATH'),
> asset_hash=ntprom_hash,
> algorithm='sha1')
> ---
>
> I get:
>
> console: [ 1.000] NetBSD 9.2 (RAMDISK) #0: Wed May 12 13:15:55 UTC
> 2021
> console: [ 1.000]
> mkre...@mkrepro.netbsd.org:/usr/src/sys/arch/arc/compile/RAMDISK
> console: [ 1.000] MIPS Magnum
> ...
> console: # [ 6.1232105]
> pmap_t
Re: [PATCH 1/4] dp8393x: don't force 32-bit register access
On Mon, 5 Jul 2021, Mark Cave-Ayland wrote:
> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" set
> .impl.min_access_size
> and .impl.max_access_size to 4 to try and fix the Linux jazzsonic driver
> which uses
> 32-bit accesses.
>
> The problem with forcing the register access to 32-bit in this way is that
> since the
> dp8393x uses 16-bit registers, a manual endian swap is required for devices
> on big
> endian machines with 32-bit accesses.
>
> For both access sizes and machine endians the QEMU memory API can do the
> right thing
> automatically: all that is needed is to set .impl.min_access_size to 2 to
> declare that
> the dp8393x implements 16-bit registers.
>
> Normally .impl.max_access_size should also be set to 2, however that doesn't
> quite
> work in this case since the register stride is specified using a (dynamic)
> it_shift
> property which is applied during the MMIO access itself. The effect of this
> is that
> for a 32-bit access the memory API performs 2 x 16-bit accesses, but the use
> of
> it_shift within the MMIO access itself causes the register value to be
> repeated in both
> the top 16-bits and bottom 16-bits. The Linux jazzsonic driver expects the
> stride to be
> zero-extended up to access size and therefore fails to correctly detect the
> dp8393x
> device due to the extra data in the top 16-bits.
>
> The solution here is to remove .impl.max_access_size so that the memory API
> will
> correctly zero-extend the 16-bit registers to the access size up to and
> including
> it_shift. Since it_shift is never greater than 2 than this will always do the
> right
> thing for both 16-bit and 32-bit accesses regardless of the machine endian,
> allowing
> the manual endian swap code to be removed.
>
IIUC, this patch replaces an explicit word swap with an implicit byte
swap. The explicit word swap was conditional on the big_endian flag.
This flag seems to work like the chip's BMODE pin which switches between
Intel and Motorola bus modes (not just byte ordering but bus signalling in
general). The BMODE pin or big_endian flag should effect a byte swap not a
word swap so there must be a bug though it's not clear how that manifests.
Regardless of this patch, the big_endian flag also controls byte swapping
during DMA by the device. IIUC, the flag is set to indicate that RAM is
big_endian, so it's not actually a property of the dp8393x but of the
RAM...
The Magnum hardware can run in big endian or little endian mode. But the
SONIC chip must remain in little endian mode always because asserting
BMODE would invoke Motorola signalling and that would contradict
Philippe's datasheet which says that the SONIC device is attached to an
"i386 compatible bus".
This seems contrary to mips_jazz_init(), which sets the dp8393x big_endian
flag whenever TARGET_WORDS_BIGENDIAN is defined, i.e. risc/os guest.
QEMU's dp8393x device has native endianness, so perhaps a big endian guest
or a big endian host could trigger the bug that's being addressed in this
patch.
Anyway, I think that this patch is heading in the right direction but
can't it go further? Shouldn't the big_endian flag disappear altogether so
that the memory API can also take care of the byte swapping needed by
dp8393x_get() and dp8393x_put() for DMA?
> Signed-off-by: Mark Cave-Ayland
> Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses")
> ---
> hw/net/dp8393x.c | 14 +-
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 11810c9b60..44a1955015 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -602,15 +602,14 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr,
> unsigned int size)
>
> trace_dp8393x_read(reg, reg_names[reg], val, size);
>
> -return s->big_endian ? val << 16 : val;
> +return val;
> }
>
> -static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
> +static void dp8393x_write(void *opaque, hwaddr addr, uint64_t val,
>unsigned int size)
> {
> dp8393xState *s = opaque;
> int reg = addr >> s->it_shift;
> -uint32_t val = s->big_endian ? data >> 16 : data;
>
> trace_dp8393x_write(reg, reg_names[reg], val, size);
>
> @@ -691,11 +690,16 @@ static void dp8393x_write(void *opaque, hwaddr addr,
> uint64_t data,
> }
> }
>
> +/*
> + * Since .impl.max_access_size is effectively controlled by the it_shift
> + * property, leave it unspecified for now to allow the memory API to
> + * correctly zero extend the 16-bit register values to the access size up to
> and
> + * including it_shift.
> + */
> static const MemoryRegionOps dp8393x_ops = {
> .read = dp8393x_read,
> .write = dp8393x_write,
> -.impl.min_access_size = 4,
> -.impl.max_access_size = 4,
> +.impl.min_access_size = 2,
> .endianness = DEVICE_NATIVE_ENDIAN,
> };
>
>
Re: [PATCH 3/4] dp8393x: Store CAM registers as 16-bit
On Mon, 5 Jul 2021, Mark Cave-Ayland wrote:
> From: Philippe Mathieu-Daudé
>
> Per the DP83932C datasheet from July 1995:
>
> 4.0 SONIC Registers
> 4.1 THE CAM UNIT
>
> The Content Addressable Memory (CAM) consists of sixteen
> 48-bit entries for complete address filtering of network
> packets. Each entry corresponds to a 48-bit destination
> address that is user programmable and can contain any
> combination of Multicast or Physical addresses. Each entry
> is partitioned into three 16-bit CAM cells accessible
> through CAM Address Ports (CAP 2, CAP 1 and CAP 0) with
> CAP0 corresponding to the least significant 16 bits of
> the Destination Address and CAP2 corresponding to the
> most significant bits.
>
> Store the CAM registers as 16-bit as it simplifies the code.
> There is no change in the migration stream.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/net/dp8393x.c | 23 ++-
> 1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index cc7c001edb..22ceea338c 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -157,7 +157,7 @@ struct dp8393xState {
> MemoryRegion mmio;
>
> /* Registers */
> -uint8_t cam[16][6];
> +uint16_t cam[16][3];
> uint16_t regs[0x40];
>
> /* Temporaries */
> @@ -280,15 +280,13 @@ static void dp8393x_do_load_cam(dp8393xState *s)
> address_space_read(&s->as, dp8393x_cdp(s),
> MEMTXATTRS_UNSPECIFIED, s->data, size);
> index = dp8393x_get(s, width, 0) & 0xf;
> -s->cam[index][0] = dp8393x_get(s, width, 1) & 0xff;
> -s->cam[index][1] = dp8393x_get(s, width, 1) >> 8;
> -s->cam[index][2] = dp8393x_get(s, width, 2) & 0xff;
> -s->cam[index][3] = dp8393x_get(s, width, 2) >> 8;
> -s->cam[index][4] = dp8393x_get(s, width, 3) & 0xff;
> -s->cam[index][5] = dp8393x_get(s, width, 3) >> 8;
> -trace_dp8393x_load_cam(index, s->cam[index][0], s->cam[index][1],
> - s->cam[index][2], s->cam[index][3],
> - s->cam[index][4], s->cam[index][5]);
> +s->cam[index][0] = dp8393x_get(s, width, 1);
> +s->cam[index][1] = dp8393x_get(s, width, 2);
> +s->cam[index][2] = dp8393x_get(s, width, 3);
> +trace_dp8393x_load_cam(index,
> + s->cam[index][0] >> 8, s->cam[index][0] &
> 0xff,
> + s->cam[index][1] >> 8, s->cam[index][1] &
> 0xff,
> + s->cam[index][2] >> 8, s->cam[index][2] &
> 0xff);
> /* Move to next entry */
> s->regs[SONIC_CDC]--;
> s->regs[SONIC_CDP] += size;
> @@ -591,8 +589,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr,
> unsigned int size)
> case SONIC_CAP1:
> case SONIC_CAP0:
> if (s->regs[SONIC_CR] & SONIC_CR_RST) {
> -val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg) +
> 1] << 8;
> -val |= s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg)];
> +val = s->cam[s->regs[SONIC_CEP] & 0xf][2 * (SONIC_CAP0 - reg)];
> }
> break;
> /* All other registers have no special contraints */
This patch incorrectly alters the behaviour of the jazzsonic.c driver
which reads the MAC address from the CAP registers in sonic_probe1().
With mainline QEMU, the driver reports:
SONIC ethernet @e0001000, MAC 00:00:00:44:33:22, IRQ 28
With this patch:
SONIC ethernet @e0001000, MAC 00:00:33:22:00:00, IRQ 28
> @@ -990,7 +987,7 @@ static const VMStateDescription vmstate_dp8393x = {
> .version_id = 0,
> .minimum_version_id = 0,
> .fields = (VMStateField []) {
> -VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 6),
> +VMSTATE_BUFFER_UNSAFE(cam, dp8393xState, 0, 16 * 3 * 2),
> VMSTATE_UINT16_ARRAY(regs, dp8393xState, 0x40),
> VMSTATE_END_OF_LIST()
> }
>
Re: [PATCH v3] dp8393x: don't force 32-bit register access
On Sun, 4 Jul 2021, Mark Cave-Ayland wrote:
> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that all
> accesses
> to the registers were 32-bit
As I said, that assumption was not made there.
If commit 3fe9a838ec is deficient it is probably because I am unaware of
the ability of the QEMU memory API to accomplish the desired result.
That's not to say that the API can't do it, just that I don't know enough
about the API.
> but this is actually not the case. The access size is determined by the
> CPU instruction used and not the number of physical address lines.
>
Again, that is an over-simplification. To explain: in Apple hardware at
least, the access size that the SONIC chip sees is a consequence of bus
sizing logic that is not part of the CPU and is not part of the SONIC chip
either.
AIUI, this logic is what Philippe alluded to when he said about this
patch, "This sounds to me like the 'QEMU doesn't model busses so we end
using kludge to hide bugs' pattern".
> The big_endian workaround applied to the register read/writes was actually
> caused
> by forcing the access size to 32-bit when the guest OS was using a 16-bit
> access.
> Since the registers are 16-bit then we can simply set .impl.min_access and
> .impl.max_accessto 2 and then the memory API will automatically do the right
> thing
> for both 16-bit accesses used by Linux and 32-bit accesses used by the MacOS
> toolbox
> ROM.
>
> Signed-off-by: Mark Cave-Ayland
> Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses")
There is a 'fixes' tag here but it's unclear what bug is being fixed. I
think this commit log entry would be more helpful if it mentioned the bug
that was observed.
> ---
> hw/net/dp8393x.c | 9 -
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 11810c9b60..d16ade2b19 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -602,15 +602,14 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr,
> unsigned int size)
>
> trace_dp8393x_read(reg, reg_names[reg], val, size);
>
> -return s->big_endian ? val << 16 : val;
> +return val;
> }
>
> -static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
> +static void dp8393x_write(void *opaque, hwaddr addr, uint64_t val,
>unsigned int size)
> {
> dp8393xState *s = opaque;
> int reg = addr >> s->it_shift;
> -uint32_t val = s->big_endian ? data >> 16 : data;
>
> trace_dp8393x_write(reg, reg_names[reg], val, size);
>
> @@ -694,8 +693,8 @@ static void dp8393x_write(void *opaque, hwaddr addr,
> uint64_t data,
> static const MemoryRegionOps dp8393x_ops = {
> .read = dp8393x_read,
> .write = dp8393x_write,
> -.impl.min_access_size = 4,
> -.impl.max_access_size = 4,
> +.impl.min_access_size = 2,
> +.impl.max_access_size = 2,
> .endianness = DEVICE_NATIVE_ENDIAN,
> };
>
>
Again, this patch breaks my Linux/mipsel guest. Perhaps you did not
receive my message about that regression? It did make it into the list
archives...
https://lore.kernel.org/qemu-devel/20210703141947.352295-1-f4...@amsat.org/T/#m8ef6d91fd8e38b01e375083058902342970b8833
Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put()
On Sun, 4 Jul 2021, Mark Cave-Ayland wrote:
> On 03/07/2021 15:19, Philippe Mathieu-Daudé wrote:
>
> > Instead of accessing N registers via a single address_space API
> > call using a temporary buffer (stored in the device state) and
> > updating each register, move the address_space call in the
> > register put/get. The load/store and word size checks are moved
> > to put/get too. This simplifies a bit, making the code easier
> > to read.
> >
> > Signed-off-by: Philippe Mathieu-Daudé
> > ---
> > hw/net/dp8393x.c | 157 ++-
> > 1 file changed, 60 insertions(+), 97 deletions(-)
> >
> > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> > index bbe241ef9db..db9cfd786f5 100644
> > --- a/hw/net/dp8393x.c
> > +++ b/hw/net/dp8393x.c
> > @@ -162,7 +162,6 @@ struct dp8393xState {
> > /* Temporaries */
> > uint8_t tx_buffer[0x1];
> > -uint16_t data[12];
> > int loopback_packet;
> > /* Memory access */
> > @@ -219,34 +218,48 @@ static uint32_t dp8393x_wt(dp8393xState *s)
> > return s->regs[SONIC_WT1] << 16 | s->regs[SONIC_WT0];
> > }
> > -static uint16_t dp8393x_get(dp8393xState *s, int width, int offset)
> > +static uint16_t dp8393x_get(dp8393xState *s, hwaddr addr, unsigned ofs16)
> > {
> > +const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
> > uint16_t val;
> > -if (s->big_endian) {
> > -val = be16_to_cpu(s->data[offset * width + width - 1]);
> > +if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
> > +addr += 2 * ofs16;
> > +if (s->big_endian) {
> > +val = address_space_ldl_be(&s->as, addr, attrs, NULL);
> > +} else {
> > +val = address_space_ldl_le(&s->as, addr, attrs, NULL);
> > +}
> > } else {
> > -val = le16_to_cpu(s->data[offset * width]);
> > +addr += 1 * ofs16;
> > +if (s->big_endian) {
> > +val = address_space_lduw_be(&s->as, addr, attrs, NULL);
> > +} else {
> > +val = address_space_lduw_le(&s->as, addr, attrs, NULL);
> > +}
> > }
> > +
> > return val;
> > }
> > -static void dp8393x_put(dp8393xState *s, int width, int offset,
> > -uint16_t val)
> > +static void dp8393x_put(dp8393xState *s,
> > +hwaddr addr, unsigned ofs16, uint16_t val)
> > {
> > -if (s->big_endian) {
> > -if (width == 2) {
> > -s->data[offset * 2] = 0;
> > -s->data[offset * 2 + 1] = cpu_to_be16(val);
> > +const MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
> > +
> > +if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
> > +addr += 2 * ofs16;
> > +if (s->big_endian) {
> > +address_space_stl_be(&s->as, addr, val, attrs, NULL);
> > } else {
> > -s->data[offset] = cpu_to_be16(val);
> > +address_space_stl_le(&s->as, addr, val, attrs, NULL);
> > }
> > } else {
> > -if (width == 2) {
> > -s->data[offset * 2] = cpu_to_le16(val);
> > -s->data[offset * 2 + 1] = 0;
> > +addr += 1 * ofs16;
> > +if (s->big_endian) {
> > +address_space_stw_be(&s->as, addr, val, attrs, NULL);
> > } else {
> > -s->data[offset] = cpu_to_le16(val);
> > +address_space_stw_le(&s->as, addr, val, attrs, NULL);
> > }
> > }
> > }
> > @@ -277,12 +290,10 @@ static void dp8393x_do_load_cam(dp8393xState *s)
> > while (s->regs[SONIC_CDC] & 0x1f) {
> > /* Fill current entry */
> > -address_space_read(&s->as, dp8393x_cdp(s),
> > - MEMTXATTRS_UNSPECIFIED, s->data, size);
> > -index = dp8393x_get(s, width, 0) & 0xf;
> > -s->cam[index][0] = dp8393x_get(s, width, 1);
> > -s->cam[index][1] = dp8393x_get(s, width, 2);
> > -s->cam[index][2] = dp8393x_get(s, width, 3);
> > +index = dp8393x_get(s, dp8393x_cdp(s), 0) & 0xf;
> > +s->cam[index][0] = dp8393x_get(s, dp8393x_cdp(s), 1);
> > +s->cam[index][1] = dp8393x_get(s, dp8393x_cdp(s), 2);
> > +s->cam[index][2] = dp8393x_get(s, dp8393x_cdp(s), 3);
> > trace_dp8393x_load_cam(index,
> > s->cam[index][0] >> 8, s->cam[index][0] &
> > 0xff,
> > s->cam[index][1] >> 8, s->cam[index][1] &
> > 0xff,
> > @@ -293,9 +304,7 @@ static void dp8393x_do_load_cam(dp8393xState *s)
> > }
> > /* Read CAM enable */
> > -address_space_read(&s->as, dp8393x_cdp(s),
> > - MEMTXATTRS_UNSPECIFIED, s->data, size);
> > -s->regs[SONIC_CE] = dp8393x_get(s, width, 0);
> > +s->regs[SONIC_CE] = dp8393x_get(s, dp8393x_cdp(s), 0);
> > trace_dp8393x_load_cam_done(s->regs[SONIC_CE]);
> > /* Done */
> > @@ -311,14 +320,12 @@ static void dp8393x_do_read_rra(dp8393xState *s)
> > /* Read memory */
> > wid
Re: [PATCH] tests/acceptance: Test NetBSD 5.1 on the Jazz Magnum machine
On Mon, 5 Jul 2021, Philippe Mathieu-Daudé wrote: > Test NetBSD 5.1 on the Jazz Magnum machine. As the firmware is not > redistributable, it has to be extracted from the floppy configuration > disk coming with a Mips Magnum 4000 system, then the NTPROM_BIN_PATH > environment variable has to be set. For convenience a NVRAM pre- > initialized to boot NetBSD is included. The test can be run as: > > $ NTPROM_BIN_PATH=/path/to/ntprom.bin \ > avocado --show=app,console \ > run -t machine:magnum tests/acceptance/ > Fetching asset from > tests/acceptance/machine_mips_jazz.py:MipsJazz.test_magnum_netbsd_5_1 >(1/1) > tests/acceptance/machine_mips_jazz.py:MipsJazz.test_magnum_netbsd_5_1: > console: EISA Bus 0 Initialization In Progress... Direct Memory Access > (DMA) System Control Port B Timer 1 OK. > console: ARC Multiboot Version 174 (SGI Version 2.6) > console: Copyright (c) 1991,1992 Microsoft Corporation > console: Actions: > console: Start Windows NT > console: Run a program > console: Run setup > console: Use the arrow keys to select. > console: Press Enter to choose. > console: Program to run: > console: scsi(0)cdrom(2)fdisk(0)boot scsi(0)cdrom(2)fdisk(0)netbsd > console: NetBSD/arc Bootstrap, Revision 1.1 > console: (bui...@b7.netbsd.org, Sat Nov 6 14:06:36 UTC 2010) > console: devopen: scsi(0)cdrom(2)fdisk(0) type disk file netbsd > console: NetBSD 5.1 (RAMDISK) #0: Sat Nov 6 14:17:36 UTC 2010 > console: > bui...@b7.netbsd.org:/home/builds/ab/netbsd-5-1-RELEASE/arc/201011061943Z-obj/home/builds/ab/netbsd-5-1-RELEASE/src/sys/arch/arc/compile/RAMDISK > console: MIPS Magnum > console: total memory = 128 MB > console: avail memory = 117 MB > console: mainbus0 (root) > console: cpu0 at mainbus0: MIPS R4000 CPU (0x400) Rev. 0.0 with MIPS R4010 > FPC Rev. 0.0 > console: cpu0: 8KB/16B direct-mapped L1 Instruction cache, 48 TLB entries > console: cpu0: 8KB/16B direct-mapped write-back L1 Data cache > console: jazzio0 at mainbus0 > console: timer0 at jazzio0 addr 0xe228 > console: mcclock0 at jazzio0 addr 0xe0004000: mc146818 compatible > time-of-day clock > console: LPT1 at jazzio0 addr 0xe0008000 intr 0 not configured > console: fdc0 at jazzio0 addr 0xe0003000 intr 1 > console: fd0 at fdc0 drive 1: 1.44MB, 80 cyl, 2 head, 18 sec > console: MAGNUM at jazzio0 addr 0xe000c000 intr 2 not configured > console: VXL at jazzio0 addr 0xe080 intr 3 not configured > console: sn0 at jazzio0 addr 0xe0001000 intr 4: SONIC Ethernet > console: sn0: Ethernet address 00:00:00:00:00:00 > console: asc0 at jazzio0 addr 0xe0002000 intr 5: NCR53C94, 25MHz, SCSI ID 7 > console: scsibus0 at asc0: 8 targets, 8 luns per target > console: pckbc0 at jazzio0 addr 0xe0005000 intr 6 > console: pckbd0 at pckbc0 (kbd slot) > console: wskbd0 at pckbd0 (mux ignored) > console: pms at jazzio0 addr 0xe0005000 intr 7 not configured > console: com0 at jazzio0 addr 0xe0006000 intr 8: ns16550a, working fifo > console: com0: txfifo disabled > console: com0: console > console: com1 at jazzio0 addr 0xe0007000 intr 9: ns16550a, working fifo > console: com1: txfifo disabled > console: jazzisabr0 at mainbus0 > console: isa0 at jazzisabr0 > console: isapnp0 at isa0 port 0x279: ISA Plug 'n Play device support > console: scsibus0: waiting 2 seconds for devices to settle... > console: cd0 at scsibus0 target 2 lun 0: cdrom > removable > console: boot device: > console: root on md0a dumps on md0b > console: root file system type: ffs > console: WARNING: preposterous TOD clock time > console: WARNING: using filesystem time > console: WARNING: CHECK AND RESET THE DATE! > console: erase ^H, werase ^W, kill ^U, intr ^C, status ^T > console: Terminal type? [vt100] > console: Erase is backspace. > console: S > console: (I)nstall, (S)hell or (H)alt ? > console: # > console: # ifconfig sn0 10.0.2.3/24 > console: # # # > console: # ping -c 3 10.0.2.2 > console: # # # # # PING 10.0.2.2 (10.0.2.2): 56 data bytes > console: 64 bytes from 10.0.2.2: icmp_seq=0 ttl=255 time=12.526 ms > console: 64 bytes from 10.0.2.2: icmp_seq=1 ttl=255 time=2.324 ms > console: 64 bytes from 10.0.2.2: icmp_seq=2 ttl=255 time=0.608 ms > console: 10.0.2.2 PING Statistics > console: 3 packets transmitted, 3 packets received, 0.0% packet loss > console: shutdown -r now > console: round-trip min/avg/max/stddev = 0.608/5.153/12.526/6.443 ms > console: # Shutdown NOW! > console: shutdown: [pid 14] > console: # sh: /usr/bin/wall: not found > console: reboot by root: > console: System shutdown time has arrived > console: About to run shutdown hooks... > console: .: Can't open /etc/rc.shutdown > console: Done running shutdown hooks. > console: syncing disks... done > console: unmounting file systems... done > console: rebooting... > PASS (39.06 s) > RESULTS: PASS 1 | ERROR 0 | FAIL 0 | SKIP 0
Re: [PATCH 2/6] dp8393x: don't force 32-bit register access
On Sat, 3 Jul 2021, Philippe Mathieu-Daudé wrote:
> From: Mark Cave-Ayland
>
> Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that all
> accesses
> to the registers were 32-bit but this is actually not the case. The access
> size is
> determined by the CPU instruction used and not the number of physical address
> lines.
>
> The big_endian workaround applied to the register read/writes was actually
> caused
> by forcing the access size to 32-bit when the guest OS was using a 16-bit
> access.
> Since the registers are 16-bit then we can simply set .impl.min_access to 2
> and
> then the memory API will automatically do the right thing for both 16-bit
> accesses
> used by Linux and 32-bit accesses used by the MacOS toolbox ROM.
>
> Signed-off-by: Mark Cave-Ayland
> Fixes: 3fe9a838ec ("dp8393x: Always use 32-bit accesses")
> Tested-by: Finn Thain
I have to retract that tested-by tag for this new version. It breaks my
Linux/mipsel guest. The jazzsonic driver now says,
SONIC ethernet controller not found (0x40004)
> Message-Id: <20210625065401.30170-9-mark.cave-ayl...@ilande.co.uk>
> [PMD: dp8393x_ops.impl.max_access_size 4 -> 2]
> Signed-off-by: Philippe Mathieu-Daudé
Re: [RFC PATCH 6/6] dp8393x: Rewrite dp8393x_get() / dp8393x_put()
On Sat, 3 Jul 2021, Philippe Mathieu-Daudé wrote: > Instead of accessing N registers via a single address_space API > call using a temporary buffer (stored in the device state) and > updating each register, move the address_space call in the > register put/get. The load/store and word size checks are moved > to put/get too. This simplifies a bit, making the code easier > to read. > > Signed-off-by: Philippe Mathieu-Daudé I tried this series with a Linux/m68k guest but network activity hanged the emulator. The cause of the problem is somewhere in this patch. BTW, I've become suspicious of the word "rewrite". In this case I think it describes a commit that is attempting to do too much and needs to be split up to make it easier to review (and debug).
Re: [RFC PATCH 0/3] dp8393x: Reviewing CRC code
On Sat, 3 Jul 2021, Philippe Mathieu-Daudé wrote:
> Hi Mark, few more patches while reviewing.
>
>
>
> Again, not tested (yet)... Simply compiled.
>
>
>
> Please tell me what you think of them.
>
>
I think these 3 patches can be reduced to this theoretical bug fix:
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index db9cfd786f..e278daebc5 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -802,7 +802,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
s->regs[SONIC_TRBA0] = s->regs[SONIC_CRBA0];
/* Calculate the ethernet checksum */
-checksum = cpu_to_le32(crc32(0, buf, pkt_size));
+checksum = crc32(0, buf, pkt_size);
/* Put packet into RBA */
trace_dp8393x_receive_packet(dp8393x_crba(s));
@@ -812,8 +812,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
address += pkt_size;
/* Put frame checksum into RBA */
-address_space_write(&s->as, address, MEMTXATTRS_UNSPECIFIED,
-&checksum, sizeof(checksum));
+if (s->big_endian) {
+address_space_stl_be(&s->as, address, checksum,
+ MEMTXATTRS_UNSPECIFIED, NULL);
+} else {
+address_space_stl_le(&s->as, address, checksum,
+ MEMTXATTRS_UNSPECIFIED, NULL);
+}
address += sizeof(checksum);
/* Pad short packets to keep pointers aligned */
>
> Regards,
>
>
>
> Phil.
>
>
>
> Philippe Mathieu-Daudé (3):
>
> dp8393x: Store CRC using address_space_stl_le()
>
> dp8393x: Do not amend CRC if it is inhibited (CRCI bit set)
>
> dp8393x: Store CRC using device configured endianess
>
>
>
> hw/net/dp8393x.c | 26 ++
>
> 1 file changed, 18 insertions(+), 8 deletions(-)
>
>
>
> --
>
> 2.31.1
>
>
>
>
Re: [RFC PATCH 2/3] dp8393x: Do not amend CRC if it is inhibited (CRCI bit set)
On Sat, 3 Jul 2021, Philippe Mathieu-Daudé wrote:
> When the CRCI (CRC INHIBIT) bit is set, the 4-byte FCS field
> is not transmitted.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/net/dp8393x.c | 22 ++
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 99e179a5e86..dee8236400c 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -472,6 +472,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> */
> } else {
> /* Remove existing FCS */
> +/* TODO check crc */
I don't understand this comment. Why would you check the CRC when it's
meant to be discarded here? (This is the CRCI enabled case.)
> tx_len -= 4;
> if (tx_len < 0) {
> trace_dp8393x_transmit_txlen_error(tx_len);
> @@ -758,7 +759,10 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
> uint8_t * buf,
> return pkt_size;
> }
>
> -rx_len = pkt_size + sizeof(checksum);
> +rx_len = pkt_size;
> +if (s->regs[SONIC_TCR] & SONIC_TCR_CRCI) {
> +rx_len += sizeof(checksum);
> +}
> if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
> padded_len = ((rx_len - 1) | 3) + 1;
> } else {
This is in dp8393x_receive(), but CRCI does not apply to the recieve side
of the chip.
> @@ -801,9 +805,6 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
> uint8_t * buf,
> s->regs[SONIC_TRBA1] = s->regs[SONIC_CRBA1];
> s->regs[SONIC_TRBA0] = s->regs[SONIC_CRBA0];
>
> -/* Calculate the ethernet checksum */
> -checksum = crc32(0, buf, pkt_size);
> -
> /* Put packet into RBA */
> trace_dp8393x_receive_packet(dp8393x_crba(s));
> address = dp8393x_crba(s);
> @@ -811,10 +812,15 @@ static ssize_t dp8393x_receive(NetClientState *nc,
> const uint8_t * buf,
> buf, pkt_size);
> address += pkt_size;
>
> -/* Put frame checksum into RBA */
> -address_space_stl_le(&s->as, address, checksum, MEMTXATTRS_UNSPECIFIED,
> - NULL);
> -address += sizeof(checksum);
> +if (s->regs[SONIC_TCR] & SONIC_TCR_CRCI) {
Same mistake here.
> +/* Calculate the ethernet checksum */
> +checksum = crc32(0, buf, pkt_size);
> +
> +/* Put frame checksum into RBA */
> +address_space_stl_le(&s->as, address, checksum,
> MEMTXATTRS_UNSPECIFIED,
> + NULL);
> +address += sizeof(checksum);
> +}
>
> /* Pad short packets to keep pointers aligned */
> if (rx_len < padded_len) {
>
Anyway, I think you are right about the FCS endianness error (which you
address in the next patch).
Re: [PATCH v2 08/10] dp8393x: don't force 32-bit register access
On Thu, 1 Jul 2021, Philippe Mathieu-Daudé wrote: > On 6/25/21 8:53 AM, Mark Cave-Ayland wrote: > > Commit 3fe9a838ec "dp8393x: Always use 32-bit accesses" assumed that > > all accesses to the registers were 32-bit No, that assumption was not made there. Just take a look at my commits in Linux that make 16-bit accesses. If commit 3fe9a838ec worked by accident, it probably just reflects my inadequate knowledge of QEMU internals. > > but this is actually not the case. The access size is determined by > > the CPU instruction used and not the number of physical address lines. > > I think that's an over-simplification (in the context of commit 3fe9a838ec). > > The big_endian workaround applied to the register read/writes was > > actually caused by forcing the access size to 32-bit when the guest OS > > was using a 16-bit access. Since the registers are 16-bit then we can > > simply set .impl.min_access to 2 and then the memory API will > > automatically do the right thing for both 16-bit accesses used by > > Linux and 32-bit accesses used by the MacOS toolbox ROM. > > Hmm I'm not sure. This sounds to me like the "QEMU doesn't model busses > so we end using kludge to hide bugs" pattern. Can you provide a QTest > (ideally) or a "-trace memory_region_ops_\*" log of your firmware > accessing the dp8393x please? > The DP83932 chip is highly configurable, so I'm not sure that the behaviour of any given firmware would resolve the question. Anyway, as far as the DP83932 hardware is concerned, the behaviour of the upper 16-bits of the data bus depends on the configuration programmed into the DP83932 registers, and whether the chip is accessed as a slave or performing DMA as a master.
Re: [PATCH v2 00/10] dp8393x: fixes for MacOS toolbox ROM
On Fri, 25 Jun 2021, Mark Cave-Ayland wrote: > Here is the next set of patches from my attempts to boot MacOS under > QEMU's Q800 machine related to the Sonic network adapter. > > Patches 1 and 2 sort out checkpatch and convert from DPRINTF macros to > trace-events. > > The discussion for the v1 patchset concluded that the dp8393x device > does NOT have its own NVRAM (there is no mention of it on the datasheet) > and so patches 3 to 5 move the generation of the PROM to the q800 and > jazz boards separately to allow the formats to diverge. > > Patch 6 adds an implementation of bitrev8 to bitops.h in preparation for > changing the q800 PROM storage format, whilst patch 7 updates the MAC > address storage and checksum for the q800 machine to match the format > expected by the MacOS toolbox ROM. > > Patch 8 ensures that the CPU loads/stores are correctly converted to > 16-bit accesses for the network card and patch 9 fixes a bug when > selecting the index specified for CAM entries. > > Finally since the MIPS magnum machine exists for both big-endian > (mips64) and little-endian (mips64el) configurations, patch 10 sets the > dp8393x big_endian property accordingly using a similar technique > already used for the MIPS malta machines. > > Migration notes: the changes to the dp8393x PROM are a migration break, > but we don't care about this for now since a) the q800 machine will have > more breaking migration changes as further MacOS toolbox ROM support is > upstreamed and b) the magnum machine migration is currently broken (and > has been for quite some time). > > Signed-off-by: Mark Cave-Ayland > Tested-by: Finn Thain My testing was much the same as last time: 'qemu-system-mips64el -M magnum' with Linux/mips and NetBSD/arc, and 'qemu-system-m68k -M q800' with Linux/m68k. In each case I checked the MAC address against the '-nic' option. The host is little-endian. I have not tested 'qemu-system-mips64 -M magnum'. It appears MIPS RISC/os is needed for that but I don't have it. > > v2: > - Move PROM generation from dp8393x to q800 and magnum machines and remove > the existing code from the device itself > - Add bitrev8 implementation to bitops.h so it can be used elsewhere in > future. Use a shift/merge technique rather than a massive table lookup > as we don't care about speed > - Add patch to set the big_endian property correctly depending upon whether > a big-endian or little-endian configuration is being used > > > Mark Cave-Ayland (10): > dp8393x: checkpatch fixes > dp8393x: convert to trace-events > hw/mips/jazz: move PROM and checksum calculation from dp8393x device > to board > hw/m68k/q800: move PROM and checksum calculation from dp8393x device > to board > dp8393x: remove onboard PROM containing MAC address and checksum > qemu/bitops.h: add bitrev8 implementation > hw/m68k/q800: fix PROM checksum and MAC address storage > dp8393x: don't force 32-bit register access > dp8393x: fix CAM descriptor entry index > hw/mips/jazz: specify correct endian for dp8393x device > > hw/m68k/q800.c| 21 ++- > hw/mips/jazz.c| 32 - > hw/net/dp8393x.c | 313 +++--- > hw/net/trace-events | 17 +++ > include/qemu/bitops.h | 22 +++ > 5 files changed, 231 insertions(+), 174 deletions(-) > >
Re: [PATCH 0/5] dp8393x: fixes for MacOS toolbox ROM
On Fri, 25 Jun 2021, Mark Cave-Ayland wrote: > On 25/06/2021 05:36, Finn Thain wrote: > > > On Thu, 24 Jun 2021, Mark Cave-Ayland wrote: > > > > > Thanks for the link and the detailed testing information. I've been > > > trying to understand why you had to set the MAC address in the ARC > > > firmware so I had a bit of an experiment here. > > > > > > The reason that you need to do this is because of the NVRAM > > > configuration in your command line, in particular -global > > > ds1225y.size=8200. That configuration also shows up here, https://virtuallyfun.com/wordpress/2013/08/30/restoring-the-mips-magnum-in-qemu-1-6-0/ with the explanation, "you'll need the NVRam stuff to add extra space for the ethernet MAC address". So it seems that the 8200 figure was just a hack and does not reflect the size of the NVRAM in an actual Magnum. > > > What this does is extend the NVRAM over the top of the dp8393x-prom > > > area where QEMU places the NIC MAC address and checksum on startup, > > > so the NVRAM captures the MAC address reads/writes instead. The net > > > effect of this is that the empty NVRAM initially reads all zeros and > > > why an initial setup is required to set the MAC address. > > > > > > This can be seen quite clearly in the "info mtree" output: > > > > > > 80009000-8000b007 (prio 0, i/o): nvram > > > 8000b000-8000bfff (prio 0, rom): dp8393x-prom > > > > > > However if you completely drop -global ds1225y.size=8200 from your > > > command line then the NVRAM doesn't overrun into the dp8393x-prom > > > area, and the ARC firmware picks up the MAC address from QEMU > > > correctly: > > > > > > 80009000-8000afff (prio 0, i/o): nvram > > > 8000b000-8000bfff (prio 0, rom): dp8393x-prom > > > > > > I've also looked over the entire SONIC datasheet to see if the PROM > > > format is documented, and according to that there is no non-volatile > > > storage available on the chip itself. > > > > Yes, that's my understanding also. The relevant National Semicondutor > > Application Notes seem to include a separate PROM. And if you closely > > examine the Linux macsonic.c driver, you'll see that the PowerBook 5x0 > > models get a random MAC address because no-one (outside of Apple) > > knows where the real MAC address is stored. > > Agreed. This means that the revised patchset should now be doing the > right thing here. > > FWIW I felt that it had changed too much in its latest form to include > your original Tested-by tag due to the extra PROM changes, so I'd be > grateful if you could give it a quick test. > Sure. > > > Testing shows that the checksum algorithm currently used for the > > > dp8393x device generates the same result as that generated by the > > > ARC firmware, which is known to be different than that used by the > > > Q800 machine. > > > > > > From this I conclude that the PROM is provided by the board and not > > > the chipset, and therefore each machine should construct its own > > > PROM accordingly. I'll send a v2 patchset shortly with these changes > > > which shall also include the proposed endian patch. > > > > > > > If you potentially have both a ds1225y NVRAM and a dp8393x PROM (for > > the magnum machine) how do you avoid ending up with conflicting state? > > Would the two storage devices have to be mutually exclusive? > > The ds1225y NVRAM is located between 0x80009000-0x8000afff and running > the nvram file through hexdump shows only the first 0x1000 bytes contain > any data, so any other changes made to NVRAM via the ARC firmware setup > will be preserved. > Perhaps '-global ds1225y.size=4096' could be used to test that assumption about ARC firmware behaviour. Anyway, the default for ds1225y.size seems to be 0x2000. And a glance at the DS1225Y datasheets agrees with that figure. (I'm going to assume that DS1225Y is the actual part found in Magnum machines even though MOS6522, for instance, was not used in Quadras.) > The existing default behaviour (without -global ds1225y.size=8200) is > that only the last few bytes at 0x8000b000 are mapped to the dp8393x > PROM, and this area is marked read-only to ensure that the MAC address > obtained by the guest OS always matches the one provided by the QEMU > configuration. > Well, I asked about conflicting state having assumed that the NVRAM in a real Magnum was used to store the MAC address. But that's probably not the case. There's probably some other chip involved and your PROM device seems like a good way to model that. (Unfortunately I don't have access to a Magnum machine so you should take what I say about that machine with a grain of salt.)
Re: [PATCH 0/5] dp8393x: fixes for MacOS toolbox ROM
On Thu, 24 Jun 2021, Mark Cave-Ayland wrote: > Thanks for the link and the detailed testing information. I've been > trying to understand why you had to set the MAC address in the ARC > firmware so I had a bit of an experiment here. > > The reason that you need to do this is because of the NVRAM > configuration in your command line, in particular -global > ds1225y.size=8200. What this does is extend the NVRAM over the top of > the dp8393x-prom area where QEMU places the NIC MAC address and checksum > on startup, so the NVRAM captures the MAC address reads/writes instead. > The net effect of this is that the empty NVRAM initially reads all zeros > and why an initial setup is required to set the MAC address. > > This can be seen quite clearly in the "info mtree" output: > > 80009000-8000b007 (prio 0, i/o): nvram > 8000b000-8000bfff (prio 0, rom): dp8393x-prom > > However if you completely drop -global ds1225y.size=8200 from your > command line then the NVRAM doesn't overrun into the dp8393x-prom area, > and the ARC firmware picks up the MAC address from QEMU correctly: > > 80009000-8000afff (prio 0, i/o): nvram > 8000b000-8000bfff (prio 0, rom): dp8393x-prom > > I've also looked over the entire SONIC datasheet to see if the PROM > format is documented, and according to that there is no non-volatile > storage available on the chip itself. Yes, that's my understanding also. The relevant National Semicondutor Application Notes seem to include a separate PROM. And if you closely examine the Linux macsonic.c driver, you'll see that the PowerBook 5x0 models get a random MAC address because no-one (outside of Apple) knows where the real MAC address is stored. > Testing shows that the checksum algorithm currently used for the dp8393x > device generates the same result as that generated by the ARC firmware, > which is known to be different than that used by the Q800 machine. > > From this I conclude that the PROM is provided by the board and not the > chipset, and therefore each machine should construct its own PROM > accordingly. I'll send a v2 patchset shortly with these changes which > shall also include the proposed endian patch. > If you potentially have both a ds1225y NVRAM and a dp8393x PROM (for the magnum machine) how do you avoid ending up with conflicting state? Would the two storage devices have to be mutually exclusive?
Re: [PATCH 0/5] dp8393x: fixes for MacOS toolbox ROM
Hi Mark,
On Wed, 16 Jun 2021, Mark Cave-Ayland wrote:
> On 16/06/2021 04:09, Finn Thain wrote:
>
> > With "qemu-system-mips -M magnum ..." I was able to boot both Linux
> > and NetBSD. That was after commit 89ae0ff9b7 ("net/dp8393x: add PROM
> > to store MAC address"). But that's not to say that the MAC address was
> > decoded correctly.
> >
> > Please see,
> > https://lore.kernel.org/qemu-devel/alpine.LNX.2.21.1.1912241504560.11@nippy.intranet/
> >
> > The Linux/mips (jazzsonic) testing that I did back in 2019 used a
> > custom busybox initramfs. The NetBSD/mips testing used the official CD
> > ISO image. I will look into reviving those test harnesses because I
> > think patch 4/5 and the proposed big-endian flag will need some
> > regression testing.
>
> Thanks for the reference - I've just discovered from the link above
> something I hadn't realised which is that -M magnum is present on both
> qemu-system-mips64 *AND* qemu-system-mips64el indicating that the endian
> needs to be set accordingly. Fortunately it should be possible to use a
> similar solution as to that used for the malta machine i.e.:
>
>
> diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
> index 1e1cf8154e..16b32d2b2c 100644
> --- a/hw/mips/jazz.c
> +++ b/hw/mips/jazz.c
> @@ -124,7 +124,7 @@ static void mips_jazz_init(MachineState *machine,
> {
> MemoryRegion *address_space = get_system_memory();
> char *filename;
> -int bios_size, n;
> +int bios_size, n, big_endian;
> Clock *cpuclk;
> MIPSCPU *cpu;
> MIPSCPUClass *mcc;
> @@ -155,6 +155,12 @@ static void mips_jazz_init(MachineState *machine,
> [JAZZ_PICA61] = {, 4},
> };
>
> +#ifdef TARGET_WORDS_BIGENDIAN
> +big_endian = 1;
> +#else
> +big_endian = 0;
> +#endif
> +
> if (machine->ram_size > 256 * MiB) {
> error_report("RAM size more than 256Mb is not supported");
> exit(EXIT_FAILURE);
> @@ -280,6 +286,7 @@ static void mips_jazz_init(MachineState *machine,
> dev = qdev_new("dp8393x");
> qdev_set_nic_properties(dev, nd);
> qdev_prop_set_uint8(dev, "it_shift", 2);
> +qdev_prop_set_bit(dev, "big_endian", big_endian);
> object_property_set_link(OBJECT(dev), "dma_mr",
> OBJECT(rc4030_dma_mr), &error_abort);
> sysbus = SYS_BUS_DEVICE(dev);
>
I was able to test this patch series successfully, using both
"qemu-system-mipsel -M magnum" and "qemu-system-m68k -M q800". With the
latter emulator I used a Linux/m68k guest and with the former I used both
Linux/mips and NetBSD/arc guests.
Basic dp8393x functionality worked fine. I don't know how the m68k guest
obtained its MAC address setting, but the macsonic driver banner message
agrees with what's on the wire, at least.
The mips guest has ARC firmware which allowed the MAC address to be
programmed manually:
JAZZ Setup Program Version 0.17Friday, 6-18-2021 5:06:02 AM
Copyright (c) 1991, 1992 Microsoft Corporation
The current Ethernet station address is: 900090001122
Enter the new station address: 900090123456
The value written to NVRAM is: 9000901234560042
Press any key to continue...
I'm guessing that the "0042" is the checksum?
BTW, the patch in the message quoted above does not seem to affect my
results. (This host is little-endian...) I don't know how to test
that one. For the others:
Tested-by: Finn Thain
>
> If you have bootable images available for -M magnum under
> qemu-system-mips64 and qemu-system-mips64el, is it possible to make them
> available to others for testing?
>
All of the ARC-compliant systems were little-endian according to the
"Advanced RISC Computing Specification" published by MIPS Technology.
There may have been some non-ARC machines but I've not explored that
question.
Regarding bootable images: for NetBSD I just used the official installer,
NetBSD-9.2-arc.iso. (The regressions I encountered in the past were fixed
and hence NetBSD 5.1 isn't needed.)
For convenience, I used the NetBSD/arc bootloader to load either the
NetBSD kernel or the Linux kernel. The ARC bios is required. For a copy of
that please see https://gunkies.org/wiki/Installing_NetBSD_ARC_on_Qemu
For Linux/mips I updated my disk images, which are 1) an extfs image
containing a minimal 32-bit mipsel busybox rootfs and 2) an ISO image
containing a Linux kernel binary.
Unfortunately, the default mainline kernel build (jazz_defconfig) seems to
be too big and crashes the bootloader (not a new pro
Re: [PATCH 0/5] dp8393x: fixes for MacOS toolbox ROM
On Mon, 14 Jun 2021, Mark Cave-Ayland wrote:
> On 14/06/2021 06:36, Philippe Mathieu-Daudé wrote:
>
> > Cc'ing Finn & Laurent.
> >
> > On 6/13/21 6:37 PM, Mark Cave-Ayland wrote:
> > > Here is the next set of patches from my attempts to boot MacOS under
> > > QEMU's Q800 machine related to the Sonic network adapter.
> > >
> > > Patches 1 and 2 sort out checkpatch and convert from DPRINTF macros
> > > to trace-events.
> > >
> > > Patch 3 fixes the PROM checksum and MAC address storage format as
> > > found by stepping through the MacOS toolbox.
> > >
> > > Patch 4 ensures that the CPU loads/stores are correctly converted to
> > > 16-bit accesses for the network card and patch 5 fixes a bug when
> > > selecting the index specified for CAM entries.
> > >
> > > NOTE TO MIPS MAINTAINERS:
> > >
> > > - The Sonic network adapter is used as part of the MIPS jazz machine,
> > > however
> > >I don't have a working kernel and system to test it with. Any
> > >pointers to test images would be appreciated.
> > >
> > > - The changes to the PROM checksum in patch 3 were determined by stepping
> > >through the MacOS toolbox, and is different from the existing
> > >algorithm. Has the current PROM checksum algorithm been validated
> > >on a MIPS guest or was it just a guess? It might be that 2
> > >different algorithms are needed for the Q800 vs. Jazz machine.
> > >
> > > - My current guess is the jazzsonic driver is broken since the last set of
> > >dp8393x changes as the MIPS jazz machine does not set the "big_endian"
> > >property on the dp8393x device. I'd expect that the following diff
> > > would
> > >be needed, but I can't confirm this without a suitable test image.
> > >
> > > diff --git a/hw/mips/jazz.c b/hw/mips/jazz.c
> > > index 1e1cf8154e..1df67035aa 100644
> > > --- a/hw/mips/jazz.c
> > > +++ b/hw/mips/jazz.c
> > > @@ -280,6 +280,7 @@ static void mips_jazz_init(MachineState *machine,
> > > dev = qdev_new("dp8393x");
> > > qdev_set_nic_properties(dev, nd);
> > > qdev_prop_set_uint8(dev, "it_shift", 2);
> > > + qdev_prop_set_bit(dev, "big_endian", true);
> > > object_property_set_link(OBJECT(dev), "dma_mr",
> > >OBJECT(rc4030_dma_mr),
> > > &error_abort);
> > > sysbus = SYS_BUS_DEVICE(dev);
> > >
> > > Signed-off-by: Mark Cave-Ayland
> > >
> > > [q800-macos-upstream patchset series: 3]
> > >
> > > Mark Cave-Ayland (5):
> > >dp8393x: checkpatch fixes
> > >dp8393x: convert to trace-events
> > >dp8393x: fix PROM checksum and MAC address storage
> > >dp8393x: don't force 32-bit register access
> > >dp8393x: fix CAM descriptor entry index
> > >
> > > hw/net/dp8393x.c| 332
> > > hw/net/trace-events | 17 +++
> > > 2 files changed, 198 insertions(+), 151 deletions(-)
>
> Just to add that I've done a large amount of testing on the q800 machine
> with Linux/MacOS so I'm happy that these patches do the right thing
> there.
>
> The part I'm struggling with is testing against MIPS jazz since I don't
> have a Linux test image to hand, and there is no documentation in the
> original commit message as to where the existing PROM checksum algorithm
> came from.
>
> Hervé, can you provide some more information on this? It looks like it
> was introduced in one of your commits:
>
> commit 89ae0ff9b73ee74c9ba707a09a07ad77b9fdccb4
> Author: Hervé Poussineau
> Date: Wed Jun 3 22:45:46 2015 +0200
>
> net/dp8393x: add PROM to store MAC address
>
> Signed-off-by: Laurent Vivier
> Signed-off-by: Hervé Poussineau
> Reviewed-by: Aurelien Jarno
> Signed-off-by: Leon Alrae
>
With "qemu-system-mips -M magnum ..." I was able to boot both Linux and
NetBSD. That was after commit 89ae0ff9b7 ("net/dp8393x: add PROM to store
MAC address"). But that's not to say that the MAC address was decoded
correctly.
Please see,
https://lore.kernel.org/qemu-devel/alpine.LNX.2.21.1.1912241504560.11@nippy.intranet/
The Linux/mips (jazzsonic) testing that I did back in 2019 used a custom
busybox initramfs. The NetBSD/mips testing used the official CD ISO image.
I will look into reviving those test harnesses because I think patch 4/5
and the proposed big-endian flag will need some regression testing.
[PATCH] .mailmap: Update my email address
Signed-off-by: Finn Thain --- .mailmap | 1 + 1 file changed, 1 insertion(+) diff --git a/.mailmap b/.mailmap index a1bd659817..772f3e16cf 100644 --- a/.mailmap +++ b/.mailmap @@ -91,6 +91,7 @@ Erik Smit Fabrice Desclaux Fernando Luis Vázquez Cao Fernando Luis Vázquez Cao +Finn Thain Gautham R. Shenoy Gautham R. Shenoy Gonglei (Arei) -- 2.26.3
Re: [PATCH 00/78] Patch Round-up for stable 4.2.1, freeze on 2020-06-22
On Sat, 20 Jun 2020, Finn Thain wrote:
>
> Thanks for picking these fixes. When the maintainer originally merged this
> series of patches, the first patch got slightly damaged. This was remedied
> in a subsequent patch[1]. That is, mainline commit a0cf4297d6 ("dp8393x:
> Mask EOL bit from descriptor addresses, take 2"). Would you also pick that
> commit for v4.2.1 please?
>
> [1]
> https://lore.kernel.org/qemu-devel/23179263-a8fb-57cc-e98a-bfe9a2ee9...@vivier.eu/
>
While we're on the subject of cherry-picking fixes for fixes, you may also
want to consider c264e5d2f9f5d73977eac8e5d084f727b3d07ea9. I didn't find
any fixes for fixes for fixes. That search probably needs to be
automated...
Re: [PATCH 00/78] Patch Round-up for stable 4.2.1, freeze on 2020-06-22
Hi Michael,
On Tue, 16 Jun 2020, Michael Roth wrote:
>
> Finn Thain (14):
> dp8393x: Mask EOL bit from descriptor addresses
> dp8393x: Always use 32-bit accesses
> dp8393x: Clean up endianness hacks
> dp8393x: Have dp8393x_receive() return the packet size
> dp8393x: Update LLFA and CRDA registers from rx descriptor
> dp8393x: Clear RRRA command register bit only when appropriate
> dp8393x: Implement packet size limit and RBAE interrupt
> dp8393x: Don't clobber packet checksum
> dp8393x: Use long-word-aligned RRA pointers in 32-bit mode
> dp8393x: Pad frames to word or long word boundary
> dp8393x: Clear descriptor in_use field to release packet
> dp8393x: Always update RRA pointers and sequence numbers
> dp8393x: Don't reset Silicon Revision register
> dp8393x: Don't stop reception upon RBE interrupt assertion
>
Thanks for picking these fixes. When the maintainer originally merged this
series of patches, the first patch got slightly damaged. This was remedied
in a subsequent patch[1]. That is, mainline commit a0cf4297d6 ("dp8393x:
Mask EOL bit from descriptor addresses, take 2"). Would you also pick that
commit for v4.2.1 please?
[1]
https://lore.kernel.org/qemu-devel/23179263-a8fb-57cc-e98a-bfe9a2ee9...@vivier.eu/
Re: [PATCH 00/22] ADB: fix autopoll issues and rework mac_via state machine
Tested-by: Finn Thain Thanks for all your work on this. I've just noticed a discrepancy between the traces from an ADB bus scan on Laurent's Apple Quadra and an ADB bus scan on your patched QEMU machine. Apple Q800: [C1f][s ][Rff-][Rff ][rff-] [C2f][s ][R61 ][R05 ][r00-] [C3f][s ][R79 ][R01 ][r00-] [C4f][s ][Rff-][Rff ][rff-] [C5f][s ][Rff-][Rff ][rff-] [C6f][s ][Rff-][Rff ][rff-] [C7f][s ][Rff-][Rff ][rff-] [C8f][s ][Rff-][Rff ][rff-] [C9f][s ][Rff-][Rff ][rff-] [Caf][s ][Rff-][Rff ][rff-] [Cbf][s ][Rff-][Rff ][rff-] [Ccf][s ][Rff-][Rff ][rff-] [Cdf][s ][Rff-][Rff ][rff-] [Cef][s ][Rff-][Rff ][rff-] [Cff][s ][Rff-][Rff ][rff-] QEMU Q800: [C1f][s ][Rff-][Rff ][rff-] [C2f][s ][R02 ][R01 ][r00-] [C3f][s ][R03 ][R02 ][r00-] [C4f][s ][R03-][R02 ][rff-] [C5f][s ][R03-][R02 ][rff-] [C6f][s ][R03-][R02 ][rff-] [C7f][s ][R03-][R02 ][rff-] [C8f][s ][R03-][R02 ][rff-] [C9f][s ][R03-][R02 ][rff-] [Caf][s ][R03-][R02 ][rff-] [Cbf][s ][R03-][R02 ][rff-] [Ccf][s ][R03-][R02 ][rff-] [Cdf][s ][R03-][R02 ][rff-] [Cef][s ][R03-][R02 ][rff-] [Cff][s ][R03-][R02 ][rff-] I think this could be easy to fix; it's probably just an uninitialized packet buffer. When you come to submit v2, you may want to look into this.
[PATCH] dp8393x: Mask EOL bit from descriptor addresses, take 2
A portion of a recent patch got lost due to a merge snafu. That patch is
now commit 88f632fbb1 ("dp8393x: Mask EOL bit from descriptor addresses").
This patch restores the portion that got lost.
Signed-off-by: Finn Thain
---
hw/net/dp8393x.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 8a3504d962..81fc13ee9f 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -525,8 +525,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
* (4 + 3 * s->regs[SONIC_TFC]),
MEMTXATTRS_UNSPECIFIED, s->data,
size);
-s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
-if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
+s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0);
+if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
/* EOL detected */
break;
}
--
2.24.1
Re: [PULL V2 01/23] dp8393x: Mask EOL bit from descriptor addresses
Hi Jason,
The patch in this pull request (since merged) differs from the patch that
I sent. In particular, the change below is missing from commit 88f632fbb1
("dp8393x: Mask EOL bit from descriptor addresses") in mainline.
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -525,8 +525,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
* (4 + 3 * s->regs[SONIC_TFC]),
MEMTXATTRS_UNSPECIFIED, s->data,
size);
-s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
-if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
+s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0);
+if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
/* EOL detected */
break;
}
Please compare with "[PATCH v4 01/14] dp8393x: Mask EOL bit from
descriptor addresses" in the mailing list archives:
https://lore.kernel.org/qemu-devel/d6e8d06ad4d02f4a30c4caa6001967f806f21a1a.1580290069.git.fth...@telegraphics.com.au/
It appears that this portion of my patch went missing when merge conflicts
were resolved. The conflicts were apparently caused by commit 19f7034773
("Avoid address_space_rw() with a constant is_write argument").
Regards,
Finn
On Tue, 3 Mar 2020, Jason Wang wrote:
> From: Finn Thain
>
> The Least Significant bit of a descriptor address register is used as
> an EOL flag. It has to be masked when the register value is to be used
> as an actual address for copying memory around. But when the registers
> are to be updated the EOL bit should not be masked.
>
> Signed-off-by: Finn Thain
> Tested-by: Laurent Vivier
> Signed-off-by: Jason Wang
> ---
> hw/net/dp8393x.c | 17 +++--
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 7045193..216d44b 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -145,6 +145,9 @@ do { printf("sonic ERROR: %s: " fmt, __func__ , ##
> __VA_ARGS__); } while (0)
> #define SONIC_ISR_PINT 0x0800
> #define SONIC_ISR_LCD0x1000
>
> +#define SONIC_DESC_EOL 0x0001
> +#define SONIC_DESC_ADDR 0xFFFE
> +
> #define TYPE_DP8393X "dp8393x"
> #define DP8393X(obj) OBJECT_CHECK(dp8393xState, (obj), TYPE_DP8393X)
>
> @@ -197,7 +200,8 @@ static uint32_t dp8393x_crba(dp8393xState *s)
>
> static uint32_t dp8393x_crda(dp8393xState *s)
> {
> -return (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA];
> +return (s->regs[SONIC_URDA] << 16) |
> + (s->regs[SONIC_CRDA] & SONIC_DESC_ADDR);
> }
>
> static uint32_t dp8393x_rbwc(dp8393xState *s)
> @@ -217,7 +221,8 @@ static uint32_t dp8393x_tsa(dp8393xState *s)
>
> static uint32_t dp8393x_ttda(dp8393xState *s)
> {
> -return (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA];
> +return (s->regs[SONIC_UTDA] << 16) |
> + (s->regs[SONIC_TTDA] & SONIC_DESC_ADDR);
> }
>
> static uint32_t dp8393x_wt(dp8393xState *s)
> @@ -509,7 +514,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> MEMTXATTRS_UNSPECIFIED, s->data,
> size);
> s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
> -if (dp8393x_get(s, width, 0) & 0x1) {
> +if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
> /* EOL detected */
> break;
> }
> @@ -765,13 +770,13 @@ static ssize_t dp8393x_receive(NetClientState *nc,
> const uint8_t * buf,
> /* XXX: Check byte ordering */
>
> /* Check for EOL */
> -if (s->regs[SONIC_LLFA] & 0x1) {
> +if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
> /* Are we still in resource exhaustion? */
> size = sizeof(uint16_t) * 1 * width;
> address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
> address_space_read(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> s->data, size);
> -if (dp8393x_get(s, width, 0) & 0x1) {
> +if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
> /* Still EOL ; stop reception */
> return -1;
> } else {
> @@ -831,7 +836,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
> uint8_t * buf,
> dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
> MEMTXATTRS_UNSPECIFIED, s->data, size);
> s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> -if (s->regs[SONIC_LLFA] & 0x1) {
> +if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
> /* EOL detected */
> s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
> } else {
>
Re: [PATCH v4 00/14] Fixes for DP8393X SONIC device emulation
On Tue, 18 Feb 2020, Aleksandar Markovic wrote: > On Wednesday, January 29, 2020, Finn Thain > wrote: > > > Hi All, > > > > There are bugs in the emulated dp8393x device that can stop packet > > reception in a Linux/m68k guest (q800 machine). > > > > With a Linux/m68k v5.5 guest (q800), it's possible to remotely trigger > > an Oops by sending ping floods. > > > > With a Linux/mips guest (magnum machine), the driver fails to probe > > the dp8393x device. > > > > With a NetBSD/arc 5.1 guest (magnum), the bugs in the device can be > > fatal to the guest kernel. > > > > Whilst debugging the device, I found that the receiver algorithm > > differs from the one described in the National Semiconductor > > datasheet. > > > > This patch series resolves these bugs. > > > > AFAIK, all bugs in the Linux sonic driver were fixed in Linux v5.5. > > --- > > > Herve, > > Do your Jazz tests pass with these changes? > AFAIK those tests did not expose the NetBSD panic that is caused by mainline QEMU (mentioned above). I have actually run the tests you requested (Hervé described them in an earlier thread). There was no regression. Quite the reverse -- it's no longer possible to remotely crash the NetBSD kernel. Apparently my testing was also the first time that the jazzsonic driver (from the Linux/mips Magnum port) was tested successfully with QEMU. It doesn't work in mainline QEMU. Anyway, more testing is always nice, and I'd certainly welcome an 'acked-by' or 'tested-by' if Hervé would like to send one. Please consider backporting this series of bug fixes to QEMU stable branch(es). Regards, Finn > Regards, > Aleksandar > > > > > Changed since v1: > > - Minor revisions as described beneath commit logs. > > - Dropped patches 4/10 and 7/10. > > - Added 5 new patches. > > > > Changed since v2: > > - Minor revisions as described beneath commit logs. > > - Dropped patch 13/13. > > - Added 2 new patches. > > > > Changed since v3: > > - Replaced patch 13/14 with patch suggested by Philippe Mathieu-Daudé. > > > > > > Finn Thain (14): > > dp8393x: Mask EOL bit from descriptor addresses > > dp8393x: Always use 32-bit accesses > > dp8393x: Clean up endianness hacks > > dp8393x: Have dp8393x_receive() return the packet size > > dp8393x: Update LLFA and CRDA registers from rx descriptor > > dp8393x: Clear RRRA command register bit only when appropriate > > dp8393x: Implement packet size limit and RBAE interrupt > > dp8393x: Don't clobber packet checksum > > dp8393x: Use long-word-aligned RRA pointers in 32-bit mode > > dp8393x: Pad frames to word or long word boundary > > dp8393x: Clear descriptor in_use field to release packet > > dp8393x: Always update RRA pointers and sequence numbers > > dp8393x: Don't reset Silicon Revision register > > dp8393x: Don't stop reception upon RBE interrupt assertion > > > > hw/net/dp8393x.c | 202 +++ > > 1 file changed, 134 insertions(+), 68 deletions(-) > > > > -- > > 2.24.1 > > > > > > >
[PATCH v4 03/14] dp8393x: Clean up endianness hacks
According to the datasheet, section 3.4.4, "in 32-bit mode ... the SONIC
always writes long words".
Therefore, use the same technique for the 'in_use' field that is used
everywhere else, and write the full long word.
Signed-off-by: Finn Thain
Tested-by: Laurent Vivier
Reviewed-by: Philippe Mathieu-Daudé
---
Changed since v1:
- Use existing 'address' variable rather than declare a new one.
Laurent tells me that a similar clean-up has been tried before.
He referred me to commit c744cf7879 ("dp8393x: fix dp8393x_receive()")
and commit 409b52bfe1 ("net/dp8393x: correctly reset in_use field").
I believe the underlying issue has been fixed by the preceding patch,
as this no longer breaks NetBSD 5.1.
---
hw/net/dp8393x.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index b2fd44bc2f..2d2ace2549 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -776,8 +776,6 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
return -1;
}
-/* XXX: Check byte ordering */
-
/* Check for EOL */
if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
/* Are we still in resource exhaustion? */
@@ -847,15 +845,12 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
/* EOL detected */
s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
} else {
-/* Clear in_use, but it is always 16bit wide */
-int offset = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
-if (s->big_endian && width == 2) {
-/* we need to adjust the offset of the 16bit field */
-offset += sizeof(uint16_t);
-}
-s->data[0] = 0;
-address_space_rw(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
- (uint8_t *)s->data, sizeof(uint16_t), 1);
+/* Clear in_use */
+size = sizeof(uint16_t) * width;
+address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
+dp8393x_put(s, width, 0, 0);
+address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)s->data, size, 1);
s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
(((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
--
2.24.1
[PATCH v4 12/14] dp8393x: Always update RRA pointers and sequence numbers
These operations need to take place regardless of whether or not
rx descriptors have been used up (that is, EOL flag was observed).
The algorithm is now the same for a packet that was withheld as for
a packet that was not.
Signed-off-by: Finn Thain
Tested-by: Laurent Vivier
---
hw/net/dp8393x.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 99c5dad7c4..1b73a8703b 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -897,12 +897,14 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
/* Move to next descriptor */
s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
-s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
(((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
+}
-if (s->regs[SONIC_RCR] & SONIC_RCR_LPKT) {
-/* Read next RRA */
-dp8393x_do_read_rra(s);
-}
+s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
+ ((s->regs[SONIC_RSC] + 1) & 0x00ff);
+
+if (s->regs[SONIC_RCR] & SONIC_RCR_LPKT) {
+/* Read next RRA */
+dp8393x_do_read_rra(s);
}
/* Done */
--
2.24.1
[PATCH v4 10/14] dp8393x: Pad frames to word or long word boundary
The existing code has a bug where the Remaining Buffer Word Count (RBWC)
is calculated with a truncating division, which gives the wrong result
for odd-sized packets.
Section 1.4.1 of the datasheet says,
Once the end of the packet has been reached, the serializer will
fill out the last word (16-bit mode) or long word (32-bit mode)
if the last byte did not end on a word or long word boundary
respectively. The fill byte will be 0FFh.
Implement buffer padding so that buffer limits are correctly enforced.
Signed-off-by: Finn Thain
Tested-by: Laurent Vivier
Reviewed-by: Philippe Mathieu-Daudé
---
hw/net/dp8393x.c | 39 ---
1 file changed, 28 insertions(+), 11 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index b052e2c854..13513986f0 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -766,16 +766,23 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
dp8393xState *s = qemu_get_nic_opaque(nc);
int packet_type;
uint32_t available, address;
-int width, rx_len = pkt_size;
+int width, rx_len, padded_len;
uint32_t checksum;
int size;
-width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
-
s->regs[SONIC_RCR] &= ~(SONIC_RCR_PRX | SONIC_RCR_LBK | SONIC_RCR_FAER |
SONIC_RCR_CRCR | SONIC_RCR_LPKT | SONIC_RCR_BC | SONIC_RCR_MC);
-if (pkt_size + 4 > dp8393x_rbwc(s) * 2) {
+rx_len = pkt_size + sizeof(checksum);
+if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
+width = 2;
+padded_len = ((rx_len - 1) | 3) + 1;
+} else {
+width = 1;
+padded_len = ((rx_len - 1) | 1) + 1;
+}
+
+if (padded_len > dp8393x_rbwc(s) * 2) {
DPRINTF("oversize packet, pkt_size is %d\n", pkt_size);
s->regs[SONIC_ISR] |= SONIC_ISR_RBAE;
dp8393x_update_irq(s);
@@ -810,22 +817,32 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
s->regs[SONIC_TRBA0] = s->regs[SONIC_CRBA0];
/* Calculate the ethernet checksum */
-checksum = cpu_to_le32(crc32(0, buf, rx_len));
+checksum = cpu_to_le32(crc32(0, buf, pkt_size));
/* Put packet into RBA */
DPRINTF("Receive packet at %08x\n", dp8393x_crba(s));
address = dp8393x_crba(s);
address_space_rw(&s->as, address,
-MEMTXATTRS_UNSPECIFIED, (uint8_t *)buf, rx_len, 1);
-address += rx_len;
+MEMTXATTRS_UNSPECIFIED, (uint8_t *)buf, pkt_size, 1);
+address += pkt_size;
+
+/* Put frame checksum into RBA */
address_space_rw(&s->as, address,
-MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, 4, 1);
-address += 4;
-rx_len += 4;
+MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, sizeof(checksum), 1);
+address += sizeof(checksum);
+
+/* Pad short packets to keep pointers aligned */
+if (rx_len < padded_len) {
+size = padded_len - rx_len;
+address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
+(uint8_t *)"\xFF\xFF\xFF", size, 1);
+address += size;
+}
+
s->regs[SONIC_CRBA1] = address >> 16;
s->regs[SONIC_CRBA0] = address & 0x;
available = dp8393x_rbwc(s);
-available -= rx_len / 2;
+available -= padded_len >> 1;
s->regs[SONIC_RBWC1] = available >> 16;
s->regs[SONIC_RBWC0] = available & 0x;
--
2.24.1
[PATCH v4 00/14] Fixes for DP8393X SONIC device emulation
Hi All, There are bugs in the emulated dp8393x device that can stop packet reception in a Linux/m68k guest (q800 machine). With a Linux/m68k v5.5 guest (q800), it's possible to remotely trigger an Oops by sending ping floods. With a Linux/mips guest (magnum machine), the driver fails to probe the dp8393x device. With a NetBSD/arc 5.1 guest (magnum), the bugs in the device can be fatal to the guest kernel. Whilst debugging the device, I found that the receiver algorithm differs from the one described in the National Semiconductor datasheet. This patch series resolves these bugs. AFAIK, all bugs in the Linux sonic driver were fixed in Linux v5.5. --- Changed since v1: - Minor revisions as described beneath commit logs. - Dropped patches 4/10 and 7/10. - Added 5 new patches. Changed since v2: - Minor revisions as described beneath commit logs. - Dropped patch 13/13. - Added 2 new patches. Changed since v3: - Replaced patch 13/14 with patch suggested by Philippe Mathieu-Daudé. Finn Thain (14): dp8393x: Mask EOL bit from descriptor addresses dp8393x: Always use 32-bit accesses dp8393x: Clean up endianness hacks dp8393x: Have dp8393x_receive() return the packet size dp8393x: Update LLFA and CRDA registers from rx descriptor dp8393x: Clear RRRA command register bit only when appropriate dp8393x: Implement packet size limit and RBAE interrupt dp8393x: Don't clobber packet checksum dp8393x: Use long-word-aligned RRA pointers in 32-bit mode dp8393x: Pad frames to word or long word boundary dp8393x: Clear descriptor in_use field to release packet dp8393x: Always update RRA pointers and sequence numbers dp8393x: Don't reset Silicon Revision register dp8393x: Don't stop reception upon RBE interrupt assertion hw/net/dp8393x.c | 202 +++ 1 file changed, 134 insertions(+), 68 deletions(-) -- 2.24.1
[PATCH v4 14/14] dp8393x: Don't stop reception upon RBE interrupt assertion
Section 3.4.7 of the datasheet explains that,
The RBE bit in the Interrupt Status register is set when the
SONIC finishes using the second to last receive buffer and reads
the last RRA descriptor. Actually, the SONIC is not truly out of
resources, but gives the system an early warning of an impending
out of resources condition.
RBE does not mean actual receive buffer exhaustion, and reception should
not be stopped. This is important because Linux will not check and clear
the RBE interrupt until it receives another packet. But that won't
happen if can_receive returns false. This bug causes the SONIC to become
deaf (until reset).
Fix this with a new flag to indicate actual receive buffer exhaustion.
Signed-off-by: Finn Thain
Tested-by: Laurent Vivier
---
Changed since v2:
- Don't use can_receive to suspend packet reception.
- Don't misuse the RBE interrupt flag as a proxy for RRP == RWP.
---
hw/net/dp8393x.c | 35 ++-
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 93eb07e6c8..cb55913a8a 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -158,6 +158,7 @@ typedef struct dp8393xState {
/* Hardware */
uint8_t it_shift;
bool big_endian;
+bool last_rba_is_full;
qemu_irq irq;
#ifdef DEBUG_SONIC
int irq_level;
@@ -347,12 +348,15 @@ static void dp8393x_do_read_rra(dp8393xState *s)
s->regs[SONIC_RRP] = s->regs[SONIC_RSA];
}
-/* Check resource exhaustion */
+/* Warn the host if CRBA now has the last available resource */
if (s->regs[SONIC_RRP] == s->regs[SONIC_RWP])
{
s->regs[SONIC_ISR] |= SONIC_ISR_RBE;
dp8393x_update_irq(s);
}
+
+/* Allow packet reception */
+s->last_rba_is_full = false;
}
static void dp8393x_do_software_reset(dp8393xState *s)
@@ -659,9 +663,6 @@ static void dp8393x_write(void *opaque, hwaddr addr,
uint64_t data,
dp8393x_do_read_rra(s);
}
dp8393x_update_irq(s);
-if (dp8393x_can_receive(s->nic->ncs)) {
-qemu_flush_queued_packets(qemu_get_queue(s->nic));
-}
break;
/* The guest is required to store aligned pointers here */
case SONIC_RSA:
@@ -721,8 +722,6 @@ static int dp8393x_can_receive(NetClientState *nc)
if (!(s->regs[SONIC_CR] & SONIC_CR_RXEN))
return 0;
-if (s->regs[SONIC_ISR] & SONIC_ISR_RBE)
-return 0;
return 1;
}
@@ -773,6 +772,10 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
s->regs[SONIC_RCR] &= ~(SONIC_RCR_PRX | SONIC_RCR_LBK | SONIC_RCR_FAER |
SONIC_RCR_CRCR | SONIC_RCR_LPKT | SONIC_RCR_BC | SONIC_RCR_MC);
+if (s->last_rba_is_full) {
+return pkt_size;
+}
+
rx_len = pkt_size + sizeof(checksum);
if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
width = 2;
@@ -786,8 +789,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
DPRINTF("oversize packet, pkt_size is %d\n", pkt_size);
s->regs[SONIC_ISR] |= SONIC_ISR_RBAE;
dp8393x_update_irq(s);
-dp8393x_do_read_rra(s);
-return pkt_size;
+s->regs[SONIC_RCR] |= SONIC_RCR_LPKT;
+goto done;
}
packet_type = dp8393x_receive_filter(s, buf, pkt_size);
@@ -899,17 +902,23 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
}
+dp8393x_update_irq(s);
+
s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
((s->regs[SONIC_RSC] + 1) & 0x00ff);
+done:
+
if (s->regs[SONIC_RCR] & SONIC_RCR_LPKT) {
-/* Read next RRA */
-dp8393x_do_read_rra(s);
+if (s->regs[SONIC_RRP] == s->regs[SONIC_RWP]) {
+/* Stop packet reception */
+s->last_rba_is_full = true;
+} else {
+/* Read next resource */
+dp8393x_do_read_rra(s);
+}
}
-/* Done */
-dp8393x_update_irq(s);
-
return pkt_size;
}
--
2.24.1
[PATCH v4 01/14] dp8393x: Mask EOL bit from descriptor addresses
The Least Significant bit of a descriptor address register is used as
an EOL flag. It has to be masked when the register value is to be used
as an actual address for copying memory around. But when the registers
are to be updated the EOL bit should not be masked.
Signed-off-by: Finn Thain
Tested-by: Laurent Vivier
Reviewed-by: Philippe Mathieu-Daudé
---
Changed since v1:
- Added macros to name constants as requested by Philippe Mathieu-Daudé.
---
hw/net/dp8393x.c | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index cdc2631c0c..14901c1445 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -145,6 +145,9 @@ do { printf("sonic ERROR: %s: " fmt, __func__ , ##
__VA_ARGS__); } while (0)
#define SONIC_ISR_PINT 0x0800
#define SONIC_ISR_LCD0x1000
+#define SONIC_DESC_EOL 0x0001
+#define SONIC_DESC_ADDR 0xFFFE
+
#define TYPE_DP8393X "dp8393x"
#define DP8393X(obj) OBJECT_CHECK(dp8393xState, (obj), TYPE_DP8393X)
@@ -197,7 +200,8 @@ static uint32_t dp8393x_crba(dp8393xState *s)
static uint32_t dp8393x_crda(dp8393xState *s)
{
-return (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA];
+return (s->regs[SONIC_URDA] << 16) |
+ (s->regs[SONIC_CRDA] & SONIC_DESC_ADDR);
}
static uint32_t dp8393x_rbwc(dp8393xState *s)
@@ -217,7 +221,8 @@ static uint32_t dp8393x_tsa(dp8393xState *s)
static uint32_t dp8393x_ttda(dp8393xState *s)
{
-return (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA];
+return (s->regs[SONIC_UTDA] << 16) |
+ (s->regs[SONIC_TTDA] & SONIC_DESC_ADDR);
}
static uint32_t dp8393x_wt(dp8393xState *s)
@@ -506,8 +511,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
sizeof(uint16_t) *
(4 + 3 * s->regs[SONIC_TFC]) * width,
MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
-s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
-if (dp8393x_get(s, width, 0) & 0x1) {
+s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0);
+if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
/* EOL detected */
break;
}
@@ -763,13 +768,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
/* XXX: Check byte ordering */
/* Check for EOL */
-if (s->regs[SONIC_LLFA] & 0x1) {
+if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
/* Are we still in resource exhaustion? */
size = sizeof(uint16_t) * 1 * width;
address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
(uint8_t *)s->data, size, 0);
-if (dp8393x_get(s, width, 0) & 0x1) {
+if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
/* Still EOL ; stop reception */
return -1;
} else {
@@ -827,7 +832,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
-if (s->regs[SONIC_LLFA] & 0x1) {
+if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
/* EOL detected */
s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
} else {
--
2.24.1
[PATCH v4 07/14] dp8393x: Implement packet size limit and RBAE interrupt
Add a bounds check to prevent a large packet from causing a buffer
overflow. This is defensive programming -- I haven't actually tried
sending an oversized packet or a jumbo ethernet frame.
The SONIC handles packets that are too big for the buffer by raising
the RBAE interrupt and dropping them. Linux uses that interrupt to
count dropped packets.
Signed-off-by: Finn Thain
Tested-by: Laurent Vivier
---
Changed since v1:
- Perform length check after Recieve Control Register initialization.
---
hw/net/dp8393x.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 2e976781e2..0309365fda 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -137,6 +137,7 @@ do { printf("sonic ERROR: %s: " fmt, __func__ , ##
__VA_ARGS__); } while (0)
#define SONIC_TCR_CRCI 0x2000
#define SONIC_TCR_PINT 0x8000
+#define SONIC_ISR_RBAE 0x0010
#define SONIC_ISR_RBE0x0020
#define SONIC_ISR_RDE0x0040
#define SONIC_ISR_TC 0x0080
@@ -770,6 +771,14 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
s->regs[SONIC_RCR] &= ~(SONIC_RCR_PRX | SONIC_RCR_LBK | SONIC_RCR_FAER |
SONIC_RCR_CRCR | SONIC_RCR_LPKT | SONIC_RCR_BC | SONIC_RCR_MC);
+if (pkt_size + 4 > dp8393x_rbwc(s) * 2) {
+DPRINTF("oversize packet, pkt_size is %d\n", pkt_size);
+s->regs[SONIC_ISR] |= SONIC_ISR_RBAE;
+dp8393x_update_irq(s);
+dp8393x_do_read_rra(s);
+return pkt_size;
+}
+
packet_type = dp8393x_receive_filter(s, buf, pkt_size);
if (packet_type < 0) {
DPRINTF("packet not for netcard\n");
--
2.24.1
[PATCH v4 13/14] dp8393x: Don't reset Silicon Revision register
The jazzsonic driver in Linux uses the Silicon Revision register value
to probe the chip. The driver fails unless the SR register contains 4.
Unfortunately, reading this register in QEMU usually returns 0 because
the s->regs[] array gets wiped after a software reset.
Fixes: bd8f1ebce4 ("net/dp8393x: fix hardware reset")
Suggested-by: Philippe Mathieu-Daudé
Signed-off-by: Finn Thain
---
Changed since v3:
- Simplified as per suggestion from Philippe Mathieu-Daudé.
---
hw/net/dp8393x.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 1b73a8703b..93eb07e6c8 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -919,6 +919,7 @@ static void dp8393x_reset(DeviceState *dev)
timer_del(s->watchdog);
memset(s->regs, 0, sizeof(s->regs));
+s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux/mips */
s->regs[SONIC_CR] = SONIC_CR_RST | SONIC_CR_STP | SONIC_CR_RXDIS;
s->regs[SONIC_DCR] &= ~(SONIC_DCR_EXBUS | SONIC_DCR_LBR);
s->regs[SONIC_RCR] &= ~(SONIC_RCR_LB0 | SONIC_RCR_LB1 | SONIC_RCR_BRD |
SONIC_RCR_RNT);
@@ -971,7 +972,6 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
-s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
memory_region_init_ram(&s->prom, OBJECT(dev),
"dp8393x-prom", SONIC_PROM_SIZE, &local_err);
--
2.24.1
[PATCH v4 11/14] dp8393x: Clear descriptor in_use field to release packet
When the SONIC receives a packet into the last available descriptor, it retains ownership of that descriptor for as long as necessary. Section 3.4.7 of the datasheet says, When the system appends more descriptors, the SONIC releases ownership of the descriptor after writing h to the RXpkt.in_use field. The packet can now be processed by the host, so raise a PKTRX interrupt, just like the normal case. Signed-off-by: Finn Thain Tested-by: Laurent Vivier --- Changed since v2: - Assert PKTRX interrupt when releasing withheld packet. --- hw/net/dp8393x.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index 13513986f0..99c5dad7c4 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -809,7 +809,17 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, return -1; } /* Link has been updated by host */ + +/* Clear in_use */ +size = sizeof(uint16_t) * width; +address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width; +dp8393x_put(s, width, 0, 0); +address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED, + (uint8_t *)s->data, size, 1); + +/* Move to next descriptor */ s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA]; +s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX; } /* Save current position */ -- 2.24.1
[PATCH v4 02/14] dp8393x: Always use 32-bit accesses
The DP83932 and DP83934 have 32 data lines. The datasheet says,
Data Bus: These bidirectional lines are used to transfer data on the
system bus. When the SONIC is a bus master, 16-bit data is transferred
on D15-D0 and 32-bit data is transferred on D31-D0. When the SONIC is
accessed as a slave, register data is driven onto lines D15-D0.
D31-D16 are held TRI-STATE if SONIC is in 16-bit mode. If SONIC is in
32-bit mode, they are driven, but invalid.
Always use 32-bit accesses both as bus master and bus slave.
Force the MSW to zero in bus master mode.
This gets the Linux 'jazzsonic' driver working, and avoids the need for
prior hacks to make the NetBSD 'sn' driver work.
Signed-off-by: Finn Thain
Tested-by: Laurent Vivier
---
hw/net/dp8393x.c | 47 +--
1 file changed, 29 insertions(+), 18 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 14901c1445..b2fd44bc2f 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -246,9 +246,19 @@ static void dp8393x_put(dp8393xState *s, int width, int
offset,
uint16_t val)
{
if (s->big_endian) {
-s->data[offset * width + width - 1] = cpu_to_be16(val);
+if (width == 2) {
+s->data[offset * 2] = 0;
+s->data[offset * 2 + 1] = cpu_to_be16(val);
+} else {
+s->data[offset] = cpu_to_be16(val);
+}
} else {
-s->data[offset * width] = cpu_to_le16(val);
+if (width == 2) {
+s->data[offset * 2] = cpu_to_le16(val);
+s->data[offset * 2 + 1] = 0;
+} else {
+s->data[offset] = cpu_to_le16(val);
+}
}
}
@@ -588,7 +598,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr,
unsigned int size)
DPRINTF("read 0x%04x from reg %s\n", val, reg_names[reg]);
-return val;
+return s->big_endian ? val << 16 : val;
}
static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
@@ -596,13 +606,14 @@ static void dp8393x_write(void *opaque, hwaddr addr,
uint64_t data,
{
dp8393xState *s = opaque;
int reg = addr >> s->it_shift;
+uint32_t val = s->big_endian ? data >> 16 : data;
-DPRINTF("write 0x%04x to reg %s\n", (uint16_t)data, reg_names[reg]);
+DPRINTF("write 0x%04x to reg %s\n", (uint16_t)val, reg_names[reg]);
switch (reg) {
/* Command register */
case SONIC_CR:
-dp8393x_do_command(s, data);
+dp8393x_do_command(s, val);
break;
/* Prevent write to read-only registers */
case SONIC_CAP2:
@@ -615,36 +626,36 @@ static void dp8393x_write(void *opaque, hwaddr addr,
uint64_t data,
/* Accept write to some registers only when in reset mode */
case SONIC_DCR:
if (s->regs[SONIC_CR] & SONIC_CR_RST) {
-s->regs[reg] = data & 0xbfff;
+s->regs[reg] = val & 0xbfff;
} else {
DPRINTF("writing to DCR invalid\n");
}
break;
case SONIC_DCR2:
if (s->regs[SONIC_CR] & SONIC_CR_RST) {
-s->regs[reg] = data & 0xf017;
+s->regs[reg] = val & 0xf017;
} else {
DPRINTF("writing to DCR2 invalid\n");
}
break;
/* 12 lower bytes are Read Only */
case SONIC_TCR:
-s->regs[reg] = data & 0xf000;
+s->regs[reg] = val & 0xf000;
break;
/* 9 lower bytes are Read Only */
case SONIC_RCR:
-s->regs[reg] = data & 0xffe0;
+s->regs[reg] = val & 0xffe0;
break;
/* Ignore most significant bit */
case SONIC_IMR:
-s->regs[reg] = data & 0x7fff;
+s->regs[reg] = val & 0x7fff;
dp8393x_update_irq(s);
break;
/* Clear bits by writing 1 to them */
case SONIC_ISR:
-data &= s->regs[reg];
-s->regs[reg] &= ~data;
-if (data & SONIC_ISR_RBE) {
+val &= s->regs[reg];
+s->regs[reg] &= ~val;
+if (val & SONIC_ISR_RBE) {
dp8393x_do_read_rra(s);
}
dp8393x_update_irq(s);
@@ -657,17 +668,17 @@ static void dp8393x_write(void *opaque, hwaddr addr,
uint64_t data,
case SONIC_REA:
case SONIC_RRP:
case SONIC_RWP:
-s->regs[reg] = data & 0xfffe;
+s->regs[reg] = val & 0xfffe;
break;
/* Invert written value for some registers */
case SONIC_CRCT:
case SONIC_FAET:
case SONIC_MPT:
-s->regs[reg]
[PATCH v4 05/14] dp8393x: Update LLFA and CRDA registers from rx descriptor
Follow the algorithm given in the National Semiconductor DP83932C
datasheet in section 3.4.7:
At the next reception, the SONIC re-reads the last RXpkt.link field,
and updates its CRDA register to point to the next descriptor.
The chip is designed to allow the host to provide a new list of
descriptors in this way.
Signed-off-by: Finn Thain
Tested-by: Laurent Vivier
---
Changed since v1:
- Update CRDA register from LLFA register after EOL is cleared.
---
hw/net/dp8393x.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index ece72cbf42..249be403af 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -784,12 +784,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
(uint8_t *)s->data, size, 0);
-if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
+s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
+if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
/* Still EOL ; stop reception */
return -1;
-} else {
-s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
}
+/* Link has been updated by host */
+s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
}
/* Save current position */
@@ -837,7 +838,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
address_space_rw(&s->as, dp8393x_crda(s),
MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
-/* Move to next descriptor */
+/* Check link field */
size = sizeof(uint16_t) * width;
address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
@@ -852,6 +853,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
dp8393x_put(s, width, 0, 0);
address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
(uint8_t *)s->data, size, 1);
+
+/* Move to next descriptor */
s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
(((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
--
2.24.1
[PATCH v4 04/14] dp8393x: Have dp8393x_receive() return the packet size
This function re-uses its 'size' argument as a scratch variable.
Instead, declare a local 'size' variable for that purpose so that the
function result doesn't get messed up.
Signed-off-by: Finn Thain
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Laurent Vivier
---
hw/net/dp8393x.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 2d2ace2549..ece72cbf42 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -757,20 +757,21 @@ static int dp8393x_receive_filter(dp8393xState *s, const
uint8_t * buf,
}
static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
- size_t size)
+ size_t pkt_size)
{
dp8393xState *s = qemu_get_nic_opaque(nc);
int packet_type;
uint32_t available, address;
-int width, rx_len = size;
+int width, rx_len = pkt_size;
uint32_t checksum;
+int size;
width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
s->regs[SONIC_RCR] &= ~(SONIC_RCR_PRX | SONIC_RCR_LBK | SONIC_RCR_FAER |
SONIC_RCR_CRCR | SONIC_RCR_LPKT | SONIC_RCR_BC | SONIC_RCR_MC);
-packet_type = dp8393x_receive_filter(s, buf, size);
+packet_type = dp8393x_receive_filter(s, buf, pkt_size);
if (packet_type < 0) {
DPRINTF("packet not for netcard\n");
return -1;
@@ -864,7 +865,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
/* Done */
dp8393x_update_irq(s);
-return size;
+return pkt_size;
}
static void dp8393x_reset(DeviceState *dev)
--
2.24.1
[PATCH v4 09/14] dp8393x: Use long-word-aligned RRA pointers in 32-bit mode
Section 3.4.1 of the datasheet says,
The alignment of the RRA is confined to either word or long word
boundaries, depending upon the data width mode. In 16-bit mode,
the RRA must be aligned to a word boundary (A0 is always zero)
and in 32-bit mode, the RRA is aligned to a long word boundary
(A0 and A1 are always zero).
This constraint has been implemented for 16-bit mode; implement it
for 32-bit mode too.
Signed-off-by: Finn Thain
Tested-by: Laurent Vivier
Reviewed-by: Philippe Mathieu-Daudé
---
hw/net/dp8393x.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 947ceef37c..b052e2c854 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -663,12 +663,16 @@ static void dp8393x_write(void *opaque, hwaddr addr,
uint64_t data,
qemu_flush_queued_packets(qemu_get_queue(s->nic));
}
break;
-/* Ignore least significant bit */
+/* The guest is required to store aligned pointers here */
case SONIC_RSA:
case SONIC_REA:
case SONIC_RRP:
case SONIC_RWP:
-s->regs[reg] = val & 0xfffe;
+if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
+s->regs[reg] = val & 0xfffc;
+} else {
+s->regs[reg] = val & 0xfffe;
+}
break;
/* Invert written value for some registers */
case SONIC_CRCT:
--
2.24.1
[PATCH v4 08/14] dp8393x: Don't clobber packet checksum
A received packet consumes pkt_size bytes in the buffer and the frame checksum that's appended to it consumes another 4 bytes. The Receive Buffer Address register takes the former quantity into account but not the latter. So the next packet written to the buffer overwrites the frame checksum. Fix this. Signed-off-by: Finn Thain Reviewed-by: Philippe Mathieu-Daudé Tested-by: Laurent Vivier --- hw/net/dp8393x.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index 0309365fda..947ceef37c 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -816,6 +816,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, address += rx_len; address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, 4, 1); +address += 4; rx_len += 4; s->regs[SONIC_CRBA1] = address >> 16; s->regs[SONIC_CRBA0] = address & 0x; -- 2.24.1
[PATCH v4 06/14] dp8393x: Clear RRRA command register bit only when appropriate
It doesn't make sense to clear the command register bit unless the
command was actually issued.
Signed-off-by: Finn Thain
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Laurent Vivier
---
hw/net/dp8393x.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 249be403af..2e976781e2 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -352,9 +352,6 @@ static void dp8393x_do_read_rra(dp8393xState *s)
s->regs[SONIC_ISR] |= SONIC_ISR_RBE;
dp8393x_update_irq(s);
}
-
-/* Done */
-s->regs[SONIC_CR] &= ~SONIC_CR_RRRA;
}
static void dp8393x_do_software_reset(dp8393xState *s)
@@ -563,8 +560,10 @@ static void dp8393x_do_command(dp8393xState *s, uint16_t
command)
dp8393x_do_start_timer(s);
if (command & SONIC_CR_RST)
dp8393x_do_software_reset(s);
-if (command & SONIC_CR_RRRA)
+if (command & SONIC_CR_RRRA) {
dp8393x_do_read_rra(s);
+s->regs[SONIC_CR] &= ~SONIC_CR_RRRA;
+}
if (command & SONIC_CR_LCAM)
dp8393x_do_load_cam(s);
}
--
2.24.1
Re: [PATCH v3 13/14] dp8393x: Don't reset Silicon Revision register
On Wed, 29 Jan 2020, Philippe Mathieu-Daudé wrote:
> >
> > This would allow the host to change the value of the Silicon Revision
> > register.
> How the guest can modify it? We have:
>
> 589 static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
> 590 unsigned int size)
> 591 {
> 592 dp8393xState *s = opaque;
> 593 int reg = addr >> s->it_shift;
> 594
> ...
> 597 switch (reg) {
> ...
> 602 /* Prevent write to read-only registers */
> ...
> 606 case SONIC_SR:
> ...
> 608 DPRINTF("writing to reg %d invalid\n", reg);
> 609 break;
>
My mistake. I had completely overlooked that logic.
I'll revise this patch in accordance with your suggestion.
Re: [PATCH v3 01/14] dp8393x: Mask EOL bit from descriptor addresses
On Tue, 28 Jan 2020, Philippe Mathieu-Daudé wrote:
> Hi Finn,
>
> On 1/19/20 11:59 PM, Finn Thain wrote:
> > The Least Significant bit of a descriptor address register is used as
> > an EOL flag. It has to be masked when the register value is to be used
> > as an actual address for copying memory around. But when the registers
> > are to be updated the EOL bit should not be masked.
> >
> > Signed-off-by: Finn Thain
> > Tested-by: Laurent Vivier
> > ---
> > Changed since v1:
> > - Added macros to name constants as requested by Philippe Mathieu-Daudé.
> > ---
> > hw/net/dp8393x.c | 19 ---
> > 1 file changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> > index cdc2631c0c..14901c1445 100644
> > --- a/hw/net/dp8393x.c
> > +++ b/hw/net/dp8393x.c
> > @@ -145,6 +145,9 @@ do { printf("sonic ERROR: %s: " fmt, __func__ , ##
> > __VA_ARGS__); } while (0)
> > #define SONIC_ISR_PINT 0x0800
> > #define SONIC_ISR_LCD0x1000
> > +#define SONIC_DESC_EOL 0x0001
> > +#define SONIC_DESC_ADDR 0xFFFE
>
> I'd rather not add SONIC_DESC_ADDR and use ~SONIC_DESC_EOL instead.
>
> Please consider it if you respin the series.
>
I chose to use 0xFFFE instead of ~SONIC_DESC_EOL because the former
correctly implies an unsigned short word, while the latter mask may
suggest some need for sign extension or longer words.
I agree that ~SONIC_DESC_EOL is easily understood as "all the other bits".
But the bits in SONIC_DESC_EOL will never change, since this value is
dictated by the hardware.
> Reviewed-by: Philippe Mathieu-Daudé
>
Thanks for reviewing this series.
> > +
> > #define TYPE_DP8393X "dp8393x"
> > #define DP8393X(obj) OBJECT_CHECK(dp8393xState, (obj), TYPE_DP8393X)
> > @@ -197,7 +200,8 @@ static uint32_t dp8393x_crba(dp8393xState *s)
> > static uint32_t dp8393x_crda(dp8393xState *s)
> > {
> > -return (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA];
> > +return (s->regs[SONIC_URDA] << 16) |
> > + (s->regs[SONIC_CRDA] & SONIC_DESC_ADDR);
> > }
> > static uint32_t dp8393x_rbwc(dp8393xState *s)
> > @@ -217,7 +221,8 @@ static uint32_t dp8393x_tsa(dp8393xState *s)
> > static uint32_t dp8393x_ttda(dp8393xState *s)
> > {
> > -return (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA];
> > +return (s->regs[SONIC_UTDA] << 16) |
> > + (s->regs[SONIC_TTDA] & SONIC_DESC_ADDR);
> > }
> > static uint32_t dp8393x_wt(dp8393xState *s)
> > @@ -506,8 +511,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
> >sizeof(uint16_t) *
> >(4 + 3 * s->regs[SONIC_TFC]) * width,
> > MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> > -s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
> > -if (dp8393x_get(s, width, 0) & 0x1) {
> > +s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0);
> > +if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
> > /* EOL detected */
> > break;
> > }
> > @@ -763,13 +768,13 @@ static ssize_t dp8393x_receive(NetClientState *nc,
> > const uint8_t * buf,
> > /* XXX: Check byte ordering */
> > /* Check for EOL */
> > -if (s->regs[SONIC_LLFA] & 0x1) {
> > +if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
> > /* Are we still in resource exhaustion? */
> > size = sizeof(uint16_t) * 1 * width;
> > address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
> > address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
> >(uint8_t *)s->data, size, 0);
> > -if (dp8393x_get(s, width, 0) & 0x1) {
> > +if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
> > /* Still EOL ; stop reception */
> > return -1;
> > } else {
> > @@ -827,7 +832,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
> > uint8_t * buf,
> > address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 *
> > width,
> > MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
> > s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
> > -if (s->regs[SONIC_LLFA] & 0x1) {
> > +if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
> > /* EOL detected */
> > s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
> > } else {
> >
>
>
Re: [PATCH v3 13/14] dp8393x: Don't reset Silicon Revision register
On Tue, 28 Jan 2020, Philippe Mathieu-Daud? wrote:
> On 1/19/20 11:59 PM, Finn Thain wrote:
> > The jazzsonic driver in Linux uses the Silicon Revision register value
> > to probe the chip. The driver fails unless the SR register contains 4.
> > Unfortunately, reading this register in QEMU usually returns 0 because
> > the s->regs[] array gets wiped after a software reset.
> >
> > Fixes: bd8f1ebce4 ("net/dp8393x: fix hardware reset")
> > Signed-off-by: Finn Thain
> > ---
> > hw/net/dp8393x.c | 5 -
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> > index 1b73a8703b..71af0fad51 100644
> > --- a/hw/net/dp8393x.c
> > +++ b/hw/net/dp8393x.c
> > @@ -591,6 +591,10 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr,
> > unsigned int size)
> > val |= s->cam[s->regs[SONIC_CEP] & 0xf][2* (SONIC_CAP0 -
> > reg)];
> > }
> > break;
> > +/* Read-only */
> > +case SONIC_SR:
> > +val = 4; /* only revision recognized by Linux/mips */
> > +break;
> > /* All other registers have no special contrainst */
> > default:
> > val = s->regs[reg];
> > @@ -971,7 +975,6 @@ static void dp8393x_realize(DeviceState *dev, Error
> > **errp)
> > qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
> > s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
> > -s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
> > memory_region_init_ram(&s->prom, OBJECT(dev),
> > "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
> >
>
> Please fix in dp8393x_reset() instead:
>
> -- >8 --
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index cdc2631c0c..65eb9c23a7 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -862,6 +862,7 @@ static void dp8393x_reset(DeviceState *dev)
> timer_del(s->watchdog);
>
> memset(s->regs, 0, sizeof(s->regs));
> +s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
> s->regs[SONIC_CR] = SONIC_CR_RST | SONIC_CR_STP | SONIC_CR_RXDIS;
> s->regs[SONIC_DCR] &= ~(SONIC_DCR_EXBUS | SONIC_DCR_LBR);
> s->regs[SONIC_RCR] &= ~(SONIC_RCR_LB0 | SONIC_RCR_LB1 | SONIC_RCR_BRD |
> SONIC_RCR_RNT);
> @@ -914,7 +915,6 @@ static void dp8393x_realize(DeviceState *dev, Error
> **errp)
> qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
>
> s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
> -s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
>
> memory_region_init_ram(&s->prom, OBJECT(dev),
> "dp8393x-prom", SONIC_PROM_SIZE, &local_err);
> ---
>
This would allow the host to change the value of the Silicon Revision
register. However, the datasheet says,
4.3.13 Silicon Revision Register
This is a 16-bit read only register. It contains information on the
current revision of the SONIC. The value of the DP83932CVF revision
register is 6h.
I haven't actually tried storing a different value in this register on
National Semiconductor hardware, but I'm willing to do that test if you
wish.
Re: [PATCH v3 00/14] Fixes for DP8393X SONIC device emulation
On Mon, 20 Jan 2020, Finn Thain wrote: > Hi All, > > There are bugs in the emulated dp8393x device that can stop packet > reception in a Linux/m68k guest (q800 machine). > > With a Linux/mips guest (magnum machine), the driver fails to probe > the dp8393x device. > > With a NetBSD/arc 5.1 guest (magnum machine), the bugs in the device > can be fatal to the guest kernel. > > Whilst debugging the device, I found that the receiver algorithm > differs from the one described in the National Semiconductor > datasheet. > > This patch series resolves these bugs. > Ironically, given the recent driver fixes in the Linux kernel, the bugs in stock QEMU make it possible to remotely trigger a kernel Oops (much like the NetBSD crash). This patch series is needed to resolve those issues. > Note that the mainline Linux sonic driver also has bugs. > I have fixed the bugs that I know of in a series of patches at, > https://github.com/fthain/linux/commits/mac68k > --- > Changed since v1: > - Minor revisions as described in commit logs. > - Dropped patches 4/10 and 7/10. > - Added 5 new patches. > > Changed since v2: > - Minor revisions as described in commit logs. > - Dropped patch 13/13. > - Added 2 new patches. > > > Finn Thain (14): > dp8393x: Mask EOL bit from descriptor addresses > dp8393x: Always use 32-bit accesses > dp8393x: Clean up endianness hacks > dp8393x: Have dp8393x_receive() return the packet size > dp8393x: Update LLFA and CRDA registers from rx descriptor > dp8393x: Clear RRRA command register bit only when appropriate > dp8393x: Implement packet size limit and RBAE interrupt > dp8393x: Don't clobber packet checksum > dp8393x: Use long-word-aligned RRA pointers in 32-bit mode > dp8393x: Pad frames to word or long word boundary > dp8393x: Clear descriptor in_use field to release packet > dp8393x: Always update RRA pointers and sequence numbers > dp8393x: Don't reset Silicon Revision register > dp8393x: Don't stop reception upon RBE interrupt assertion > > hw/net/dp8393x.c | 205 +++ > 1 file changed, 137 insertions(+), 68 deletions(-) > >
[PATCH v3 14/14] dp8393x: Don't stop reception upon RBE interrupt assertion
Section 3.4.7 of the datasheet explains that,
The RBE bit in the Interrupt Status register is set when the
SONIC finishes using the second to last receive buffer and reads
the last RRA descriptor. Actually, the SONIC is not truly out of
resources, but gives the system an early warning of an impending
out of resources condition.
RBE does not mean actual receive buffer exhaustion, and reception should
not be stopped. This is important because Linux will not check and clear
the RBE interrupt until it receives another packet. But that won't
happen if can_receive returns false. This bug causes the SONIC to become
deaf (until reset).
Fix this with a new flag to indicate actual receive buffer exhaustion.
Signed-off-by: Finn Thain
Tested-by: Laurent Vivier
---
Changed since v2:
- Don't use can_receive to suspend packet reception.
- Don't misuse the RBE interrupt flag as a proxy for RRP == RWP.
---
hw/net/dp8393x.c | 35 ++-
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 71af0fad51..1ccee0c189 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -158,6 +158,7 @@ typedef struct dp8393xState {
/* Hardware */
uint8_t it_shift;
bool big_endian;
+bool last_rba_is_full;
qemu_irq irq;
#ifdef DEBUG_SONIC
int irq_level;
@@ -347,12 +348,15 @@ static void dp8393x_do_read_rra(dp8393xState *s)
s->regs[SONIC_RRP] = s->regs[SONIC_RSA];
}
-/* Check resource exhaustion */
+/* Warn the host if CRBA now has the last available resource */
if (s->regs[SONIC_RRP] == s->regs[SONIC_RWP])
{
s->regs[SONIC_ISR] |= SONIC_ISR_RBE;
dp8393x_update_irq(s);
}
+
+/* Allow packet reception */
+s->last_rba_is_full = false;
}
static void dp8393x_do_software_reset(dp8393xState *s)
@@ -663,9 +667,6 @@ static void dp8393x_write(void *opaque, hwaddr addr,
uint64_t data,
dp8393x_do_read_rra(s);
}
dp8393x_update_irq(s);
-if (dp8393x_can_receive(s->nic->ncs)) {
-qemu_flush_queued_packets(qemu_get_queue(s->nic));
-}
break;
/* The guest is required to store aligned pointers here */
case SONIC_RSA:
@@ -725,8 +726,6 @@ static int dp8393x_can_receive(NetClientState *nc)
if (!(s->regs[SONIC_CR] & SONIC_CR_RXEN))
return 0;
-if (s->regs[SONIC_ISR] & SONIC_ISR_RBE)
-return 0;
return 1;
}
@@ -777,6 +776,10 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
s->regs[SONIC_RCR] &= ~(SONIC_RCR_PRX | SONIC_RCR_LBK | SONIC_RCR_FAER |
SONIC_RCR_CRCR | SONIC_RCR_LPKT | SONIC_RCR_BC | SONIC_RCR_MC);
+if (s->last_rba_is_full) {
+return pkt_size;
+}
+
rx_len = pkt_size + sizeof(checksum);
if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
width = 2;
@@ -790,8 +793,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
DPRINTF("oversize packet, pkt_size is %d\n", pkt_size);
s->regs[SONIC_ISR] |= SONIC_ISR_RBAE;
dp8393x_update_irq(s);
-dp8393x_do_read_rra(s);
-return pkt_size;
+s->regs[SONIC_RCR] |= SONIC_RCR_LPKT;
+goto done;
}
packet_type = dp8393x_receive_filter(s, buf, pkt_size);
@@ -903,17 +906,23 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
}
+dp8393x_update_irq(s);
+
s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
((s->regs[SONIC_RSC] + 1) & 0x00ff);
+done:
+
if (s->regs[SONIC_RCR] & SONIC_RCR_LPKT) {
-/* Read next RRA */
-dp8393x_do_read_rra(s);
+if (s->regs[SONIC_RRP] == s->regs[SONIC_RWP]) {
+/* Stop packet reception */
+s->last_rba_is_full = true;
+} else {
+/* Read next resource */
+dp8393x_do_read_rra(s);
+}
}
-/* Done */
-dp8393x_update_irq(s);
-
return pkt_size;
}
--
2.24.1
[PATCH v3 12/14] dp8393x: Always update RRA pointers and sequence numbers
These operations have to take place regardless of whether or not rx
descriptors have been used up (that is, EOL flag was observed).
The algorithm is the now the same for a packet that was withheld as
for a packet that was not.
Signed-off-by: Finn Thain
Tested-by: Laurent Vivier
---
hw/net/dp8393x.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 99c5dad7c4..1b73a8703b 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -897,12 +897,14 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
/* Move to next descriptor */
s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
-s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
(((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
+}
-if (s->regs[SONIC_RCR] & SONIC_RCR_LPKT) {
-/* Read next RRA */
-dp8393x_do_read_rra(s);
-}
+s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
+ ((s->regs[SONIC_RSC] + 1) & 0x00ff);
+
+if (s->regs[SONIC_RCR] & SONIC_RCR_LPKT) {
+/* Read next RRA */
+dp8393x_do_read_rra(s);
}
/* Done */
--
2.24.1
[PATCH v3 01/14] dp8393x: Mask EOL bit from descriptor addresses
The Least Significant bit of a descriptor address register is used as
an EOL flag. It has to be masked when the register value is to be used
as an actual address for copying memory around. But when the registers
are to be updated the EOL bit should not be masked.
Signed-off-by: Finn Thain
Tested-by: Laurent Vivier
---
Changed since v1:
- Added macros to name constants as requested by Philippe Mathieu-Daudé.
---
hw/net/dp8393x.c | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index cdc2631c0c..14901c1445 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -145,6 +145,9 @@ do { printf("sonic ERROR: %s: " fmt, __func__ , ##
__VA_ARGS__); } while (0)
#define SONIC_ISR_PINT 0x0800
#define SONIC_ISR_LCD0x1000
+#define SONIC_DESC_EOL 0x0001
+#define SONIC_DESC_ADDR 0xFFFE
+
#define TYPE_DP8393X "dp8393x"
#define DP8393X(obj) OBJECT_CHECK(dp8393xState, (obj), TYPE_DP8393X)
@@ -197,7 +200,8 @@ static uint32_t dp8393x_crba(dp8393xState *s)
static uint32_t dp8393x_crda(dp8393xState *s)
{
-return (s->regs[SONIC_URDA] << 16) | s->regs[SONIC_CRDA];
+return (s->regs[SONIC_URDA] << 16) |
+ (s->regs[SONIC_CRDA] & SONIC_DESC_ADDR);
}
static uint32_t dp8393x_rbwc(dp8393xState *s)
@@ -217,7 +221,8 @@ static uint32_t dp8393x_tsa(dp8393xState *s)
static uint32_t dp8393x_ttda(dp8393xState *s)
{
-return (s->regs[SONIC_UTDA] << 16) | s->regs[SONIC_TTDA];
+return (s->regs[SONIC_UTDA] << 16) |
+ (s->regs[SONIC_TTDA] & SONIC_DESC_ADDR);
}
static uint32_t dp8393x_wt(dp8393xState *s)
@@ -506,8 +511,8 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
sizeof(uint16_t) *
(4 + 3 * s->regs[SONIC_TFC]) * width,
MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
-s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0) & ~0x1;
-if (dp8393x_get(s, width, 0) & 0x1) {
+s->regs[SONIC_CTDA] = dp8393x_get(s, width, 0);
+if (s->regs[SONIC_CTDA] & SONIC_DESC_EOL) {
/* EOL detected */
break;
}
@@ -763,13 +768,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
/* XXX: Check byte ordering */
/* Check for EOL */
-if (s->regs[SONIC_LLFA] & 0x1) {
+if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
/* Are we still in resource exhaustion? */
size = sizeof(uint16_t) * 1 * width;
address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
(uint8_t *)s->data, size, 0);
-if (dp8393x_get(s, width, 0) & 0x1) {
+if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
/* Still EOL ; stop reception */
return -1;
} else {
@@ -827,7 +832,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
-if (s->regs[SONIC_LLFA] & 0x1) {
+if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
/* EOL detected */
s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
} else {
--
2.24.1
[PATCH v3 03/14] dp8393x: Clean up endianness hacks
According to the datasheet, section 3.4.4, "in 32-bit mode ... the SONIC
always writes long words".
Therefore, use the same technique for the 'in_use' field that is used
everywhere else, and write the full long word.
Signed-off-by: Finn Thain
Tested-by: Laurent Vivier
---
Changed since v1:
- Use existing 'address' variable rather than declare a new one.
Laurent tells me that a similar clean-up has been tried before.
He referred me to commit c744cf7879 ("dp8393x: fix dp8393x_receive()")
and commit 409b52bfe1 ("net/dp8393x: correctly reset in_use field").
I believe the underlying issue has been fixed by the preceding patch,
as this no longer breaks NetBSD 5.1.
---
hw/net/dp8393x.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index b2fd44bc2f..2d2ace2549 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -776,8 +776,6 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
return -1;
}
-/* XXX: Check byte ordering */
-
/* Check for EOL */
if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
/* Are we still in resource exhaustion? */
@@ -847,15 +845,12 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
/* EOL detected */
s->regs[SONIC_ISR] |= SONIC_ISR_RDE;
} else {
-/* Clear in_use, but it is always 16bit wide */
-int offset = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
-if (s->big_endian && width == 2) {
-/* we need to adjust the offset of the 16bit field */
-offset += sizeof(uint16_t);
-}
-s->data[0] = 0;
-address_space_rw(&s->as, offset, MEMTXATTRS_UNSPECIFIED,
- (uint8_t *)s->data, sizeof(uint16_t), 1);
+/* Clear in_use */
+size = sizeof(uint16_t) * width;
+address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width;
+dp8393x_put(s, width, 0, 0);
+address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
+ (uint8_t *)s->data, size, 1);
s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
(((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
--
2.24.1
[PATCH v3 08/14] dp8393x: Don't clobber packet checksum
A received packet consumes pkt_size bytes in the buffer and the frame checksum that's appended to it consumes another 4 bytes. The Receive Buffer Address register takes the former quantity into account but not the latter. So the next packet written to the buffer overwrites the frame checksum. Fix this. Signed-off-by: Finn Thain Reviewed-by: Philippe Mathieu-Daudé Tested-by: Laurent Vivier --- hw/net/dp8393x.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index 0309365fda..947ceef37c 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -816,6 +816,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, address += rx_len; address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, 4, 1); +address += 4; rx_len += 4; s->regs[SONIC_CRBA1] = address >> 16; s->regs[SONIC_CRBA0] = address & 0x; -- 2.24.1
[PATCH v3 10/14] dp8393x: Pad frames to word or long word boundary
The existing code has a bug where the Remaining Buffer Word Count (RBWC)
is calculated with a truncating division, which gives the wrong result
for odd-sized packets.
Section 1.4.1 of the datasheet says,
Once the end of the packet has been reached, the serializer will
fill out the last word (16-bit mode) or long word (32-bit mode)
if the last byte did not end on a word or long word boundary
respectively. The fill byte will be 0FFh.
Implement buffer padding so that buffer limits are correctly enforced.
Signed-off-by: Finn Thain
Tested-by: Laurent Vivier
---
hw/net/dp8393x.c | 39 ---
1 file changed, 28 insertions(+), 11 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index b052e2c854..13513986f0 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -766,16 +766,23 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
dp8393xState *s = qemu_get_nic_opaque(nc);
int packet_type;
uint32_t available, address;
-int width, rx_len = pkt_size;
+int width, rx_len, padded_len;
uint32_t checksum;
int size;
-width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
-
s->regs[SONIC_RCR] &= ~(SONIC_RCR_PRX | SONIC_RCR_LBK | SONIC_RCR_FAER |
SONIC_RCR_CRCR | SONIC_RCR_LPKT | SONIC_RCR_BC | SONIC_RCR_MC);
-if (pkt_size + 4 > dp8393x_rbwc(s) * 2) {
+rx_len = pkt_size + sizeof(checksum);
+if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
+width = 2;
+padded_len = ((rx_len - 1) | 3) + 1;
+} else {
+width = 1;
+padded_len = ((rx_len - 1) | 1) + 1;
+}
+
+if (padded_len > dp8393x_rbwc(s) * 2) {
DPRINTF("oversize packet, pkt_size is %d\n", pkt_size);
s->regs[SONIC_ISR] |= SONIC_ISR_RBAE;
dp8393x_update_irq(s);
@@ -810,22 +817,32 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
s->regs[SONIC_TRBA0] = s->regs[SONIC_CRBA0];
/* Calculate the ethernet checksum */
-checksum = cpu_to_le32(crc32(0, buf, rx_len));
+checksum = cpu_to_le32(crc32(0, buf, pkt_size));
/* Put packet into RBA */
DPRINTF("Receive packet at %08x\n", dp8393x_crba(s));
address = dp8393x_crba(s);
address_space_rw(&s->as, address,
-MEMTXATTRS_UNSPECIFIED, (uint8_t *)buf, rx_len, 1);
-address += rx_len;
+MEMTXATTRS_UNSPECIFIED, (uint8_t *)buf, pkt_size, 1);
+address += pkt_size;
+
+/* Put frame checksum into RBA */
address_space_rw(&s->as, address,
-MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, 4, 1);
-address += 4;
-rx_len += 4;
+MEMTXATTRS_UNSPECIFIED, (uint8_t *)&checksum, sizeof(checksum), 1);
+address += sizeof(checksum);
+
+/* Pad short packets to keep pointers aligned */
+if (rx_len < padded_len) {
+size = padded_len - rx_len;
+address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
+(uint8_t *)"\xFF\xFF\xFF", size, 1);
+address += size;
+}
+
s->regs[SONIC_CRBA1] = address >> 16;
s->regs[SONIC_CRBA0] = address & 0x;
available = dp8393x_rbwc(s);
-available -= rx_len / 2;
+available -= padded_len >> 1;
s->regs[SONIC_RBWC1] = available >> 16;
s->regs[SONIC_RBWC0] = available & 0x;
--
2.24.1
[PATCH v3 06/14] dp8393x: Clear RRRA command register bit only when appropriate
It doesn't make sense to clear the command register bit unless the
command was actually issued.
Signed-off-by: Finn Thain
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Laurent Vivier
---
hw/net/dp8393x.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 249be403af..2e976781e2 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -352,9 +352,6 @@ static void dp8393x_do_read_rra(dp8393xState *s)
s->regs[SONIC_ISR] |= SONIC_ISR_RBE;
dp8393x_update_irq(s);
}
-
-/* Done */
-s->regs[SONIC_CR] &= ~SONIC_CR_RRRA;
}
static void dp8393x_do_software_reset(dp8393xState *s)
@@ -563,8 +560,10 @@ static void dp8393x_do_command(dp8393xState *s, uint16_t
command)
dp8393x_do_start_timer(s);
if (command & SONIC_CR_RST)
dp8393x_do_software_reset(s);
-if (command & SONIC_CR_RRRA)
+if (command & SONIC_CR_RRRA) {
dp8393x_do_read_rra(s);
+s->regs[SONIC_CR] &= ~SONIC_CR_RRRA;
+}
if (command & SONIC_CR_LCAM)
dp8393x_do_load_cam(s);
}
--
2.24.1
[PATCH v3 04/14] dp8393x: Have dp8393x_receive() return the packet size
This function re-uses its 'size' argument as a scratch variable.
Instead, declare a local 'size' variable for that purpose so that the
function result doesn't get messed up.
Signed-off-by: Finn Thain
Reviewed-by: Philippe Mathieu-Daudé
Tested-by: Laurent Vivier
---
hw/net/dp8393x.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 2d2ace2549..ece72cbf42 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -757,20 +757,21 @@ static int dp8393x_receive_filter(dp8393xState *s, const
uint8_t * buf,
}
static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf,
- size_t size)
+ size_t pkt_size)
{
dp8393xState *s = qemu_get_nic_opaque(nc);
int packet_type;
uint32_t available, address;
-int width, rx_len = size;
+int width, rx_len = pkt_size;
uint32_t checksum;
+int size;
width = (s->regs[SONIC_DCR] & SONIC_DCR_DW) ? 2 : 1;
s->regs[SONIC_RCR] &= ~(SONIC_RCR_PRX | SONIC_RCR_LBK | SONIC_RCR_FAER |
SONIC_RCR_CRCR | SONIC_RCR_LPKT | SONIC_RCR_BC | SONIC_RCR_MC);
-packet_type = dp8393x_receive_filter(s, buf, size);
+packet_type = dp8393x_receive_filter(s, buf, pkt_size);
if (packet_type < 0) {
DPRINTF("packet not for netcard\n");
return -1;
@@ -864,7 +865,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
/* Done */
dp8393x_update_irq(s);
-return size;
+return pkt_size;
}
static void dp8393x_reset(DeviceState *dev)
--
2.24.1
[PATCH v3 00/14] Fixes for DP8393X SONIC device emulation
Hi All, There are bugs in the emulated dp8393x device that can stop packet reception in a Linux/m68k guest (q800 machine). With a Linux/mips guest (magnum machine), the driver fails to probe the dp8393x device. With a NetBSD/arc 5.1 guest (magnum machine), the bugs in the device can be fatal to the guest kernel. Whilst debugging the device, I found that the receiver algorithm differs from the one described in the National Semiconductor datasheet. This patch series resolves these bugs. Note that the mainline Linux sonic driver also has bugs. I have fixed the bugs that I know of in a series of patches at, https://github.com/fthain/linux/commits/mac68k --- Changed since v1: - Minor revisions as described in commit logs. - Dropped patches 4/10 and 7/10. - Added 5 new patches. Changed since v2: - Minor revisions as described in commit logs. - Dropped patch 13/13. - Added 2 new patches. Finn Thain (14): dp8393x: Mask EOL bit from descriptor addresses dp8393x: Always use 32-bit accesses dp8393x: Clean up endianness hacks dp8393x: Have dp8393x_receive() return the packet size dp8393x: Update LLFA and CRDA registers from rx descriptor dp8393x: Clear RRRA command register bit only when appropriate dp8393x: Implement packet size limit and RBAE interrupt dp8393x: Don't clobber packet checksum dp8393x: Use long-word-aligned RRA pointers in 32-bit mode dp8393x: Pad frames to word or long word boundary dp8393x: Clear descriptor in_use field to release packet dp8393x: Always update RRA pointers and sequence numbers dp8393x: Don't reset Silicon Revision register dp8393x: Don't stop reception upon RBE interrupt assertion hw/net/dp8393x.c | 205 +++ 1 file changed, 137 insertions(+), 68 deletions(-) -- 2.24.1
[PATCH v3 09/14] dp8393x: Use long-word-aligned RRA pointers in 32-bit mode
Section 3.4.1 of the datasheet says,
The alignment of the RRA is confined to either word or long word
boundaries, depending upon the data width mode. In 16-bit mode,
the RRA must be aligned to a word boundary (A0 is always zero)
and in 32-bit mode, the RRA is aligned to a long word boundary
(A0 and A1 are always zero).
This constraint has been implemented for 16-bit mode; implement it
for 32-bit mode too.
Signed-off-by: Finn Thain
Tested-by: Laurent Vivier
---
hw/net/dp8393x.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 947ceef37c..b052e2c854 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -663,12 +663,16 @@ static void dp8393x_write(void *opaque, hwaddr addr,
uint64_t data,
qemu_flush_queued_packets(qemu_get_queue(s->nic));
}
break;
-/* Ignore least significant bit */
+/* The guest is required to store aligned pointers here */
case SONIC_RSA:
case SONIC_REA:
case SONIC_RRP:
case SONIC_RWP:
-s->regs[reg] = val & 0xfffe;
+if (s->regs[SONIC_DCR] & SONIC_DCR_DW) {
+s->regs[reg] = val & 0xfffc;
+} else {
+s->regs[reg] = val & 0xfffe;
+}
break;
/* Invert written value for some registers */
case SONIC_CRCT:
--
2.24.1
[PATCH v3 02/14] dp8393x: Always use 32-bit accesses
The DP83932 and DP83934 have 32 data lines. The datasheet says,
Data Bus: These bidirectional lines are used to transfer data on the
system bus. When the SONIC is a bus master, 16-bit data is transferred
on D15-D0 and 32-bit data is transferred on D31-D0. When the SONIC is
accessed as a slave, register data is driven onto lines D15-D0.
D31-D16 are held TRI-STATE if SONIC is in 16-bit mode. If SONIC is in
32-bit mode, they are driven, but invalid.
Always use 32-bit accesses both as bus master and bus slave.
Force the MSW to zero in bus master mode.
This gets the Linux 'jazzsonic' driver working, and avoids the need for
prior hacks to make the NetBSD 'sn' driver work.
Signed-off-by: Finn Thain
---
hw/net/dp8393x.c | 47 +--
1 file changed, 29 insertions(+), 18 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 14901c1445..b2fd44bc2f 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -246,9 +246,19 @@ static void dp8393x_put(dp8393xState *s, int width, int
offset,
uint16_t val)
{
if (s->big_endian) {
-s->data[offset * width + width - 1] = cpu_to_be16(val);
+if (width == 2) {
+s->data[offset * 2] = 0;
+s->data[offset * 2 + 1] = cpu_to_be16(val);
+} else {
+s->data[offset] = cpu_to_be16(val);
+}
} else {
-s->data[offset * width] = cpu_to_le16(val);
+if (width == 2) {
+s->data[offset * 2] = cpu_to_le16(val);
+s->data[offset * 2 + 1] = 0;
+} else {
+s->data[offset] = cpu_to_le16(val);
+}
}
}
@@ -588,7 +598,7 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr,
unsigned int size)
DPRINTF("read 0x%04x from reg %s\n", val, reg_names[reg]);
-return val;
+return s->big_endian ? val << 16 : val;
}
static void dp8393x_write(void *opaque, hwaddr addr, uint64_t data,
@@ -596,13 +606,14 @@ static void dp8393x_write(void *opaque, hwaddr addr,
uint64_t data,
{
dp8393xState *s = opaque;
int reg = addr >> s->it_shift;
+uint32_t val = s->big_endian ? data >> 16 : data;
-DPRINTF("write 0x%04x to reg %s\n", (uint16_t)data, reg_names[reg]);
+DPRINTF("write 0x%04x to reg %s\n", (uint16_t)val, reg_names[reg]);
switch (reg) {
/* Command register */
case SONIC_CR:
-dp8393x_do_command(s, data);
+dp8393x_do_command(s, val);
break;
/* Prevent write to read-only registers */
case SONIC_CAP2:
@@ -615,36 +626,36 @@ static void dp8393x_write(void *opaque, hwaddr addr,
uint64_t data,
/* Accept write to some registers only when in reset mode */
case SONIC_DCR:
if (s->regs[SONIC_CR] & SONIC_CR_RST) {
-s->regs[reg] = data & 0xbfff;
+s->regs[reg] = val & 0xbfff;
} else {
DPRINTF("writing to DCR invalid\n");
}
break;
case SONIC_DCR2:
if (s->regs[SONIC_CR] & SONIC_CR_RST) {
-s->regs[reg] = data & 0xf017;
+s->regs[reg] = val & 0xf017;
} else {
DPRINTF("writing to DCR2 invalid\n");
}
break;
/* 12 lower bytes are Read Only */
case SONIC_TCR:
-s->regs[reg] = data & 0xf000;
+s->regs[reg] = val & 0xf000;
break;
/* 9 lower bytes are Read Only */
case SONIC_RCR:
-s->regs[reg] = data & 0xffe0;
+s->regs[reg] = val & 0xffe0;
break;
/* Ignore most significant bit */
case SONIC_IMR:
-s->regs[reg] = data & 0x7fff;
+s->regs[reg] = val & 0x7fff;
dp8393x_update_irq(s);
break;
/* Clear bits by writing 1 to them */
case SONIC_ISR:
-data &= s->regs[reg];
-s->regs[reg] &= ~data;
-if (data & SONIC_ISR_RBE) {
+val &= s->regs[reg];
+s->regs[reg] &= ~val;
+if (val & SONIC_ISR_RBE) {
dp8393x_do_read_rra(s);
}
dp8393x_update_irq(s);
@@ -657,17 +668,17 @@ static void dp8393x_write(void *opaque, hwaddr addr,
uint64_t data,
case SONIC_REA:
case SONIC_RRP:
case SONIC_RWP:
-s->regs[reg] = data & 0xfffe;
+s->regs[reg] = val & 0xfffe;
break;
/* Invert written value for some registers */
case SONIC_CRCT:
case SONIC_FAET:
case SONIC_MPT:
-s->regs[reg] = data ^ 0x;
+
[PATCH v3 05/14] dp8393x: Update LLFA and CRDA registers from rx descriptor
Follow the algorithm given in the National Semiconductor DP83932C
datasheet in section 3.4.7:
At the next reception, the SONIC re-reads the last RXpkt.link field,
and updates its CRDA register to point to the next descriptor.
The chip is designed to allow the host to provide a new list of
descriptors in this way.
Signed-off-by: Finn Thain
Tested-by: Laurent Vivier
---
Changed since v1:
- Update CRDA register from LLFA register after EOL is cleared.
---
hw/net/dp8393x.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index ece72cbf42..249be403af 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -784,12 +784,13 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
address = dp8393x_crda(s) + sizeof(uint16_t) * 5 * width;
address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
(uint8_t *)s->data, size, 0);
-if (dp8393x_get(s, width, 0) & SONIC_DESC_EOL) {
+s->regs[SONIC_LLFA] = dp8393x_get(s, width, 0);
+if (s->regs[SONIC_LLFA] & SONIC_DESC_EOL) {
/* Still EOL ; stop reception */
return -1;
-} else {
-s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
}
+/* Link has been updated by host */
+s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
}
/* Save current position */
@@ -837,7 +838,7 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
address_space_rw(&s->as, dp8393x_crda(s),
MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 1);
-/* Move to next descriptor */
+/* Check link field */
size = sizeof(uint16_t) * width;
address_space_rw(&s->as, dp8393x_crda(s) + sizeof(uint16_t) * 5 * width,
MEMTXATTRS_UNSPECIFIED, (uint8_t *)s->data, size, 0);
@@ -852,6 +853,8 @@ static ssize_t dp8393x_receive(NetClientState *nc, const
uint8_t * buf,
dp8393x_put(s, width, 0, 0);
address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED,
(uint8_t *)s->data, size, 1);
+
+/* Move to next descriptor */
s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA];
s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX;
s->regs[SONIC_RSC] = (s->regs[SONIC_RSC] & 0xff00) |
(((s->regs[SONIC_RSC] & 0x00ff) + 1) & 0x00ff);
--
2.24.1
[PATCH v3 11/14] dp8393x: Clear descriptor in_use field to release packet
When the SONIC receives a packet into the last available descriptor, it retains ownership of that descriptor for as long as necessary. Section 3.4.7 of the datasheet says, When the system appends more descriptors, the SONIC releases ownership of the descriptor after writing h to the RXpkt.in_use field. The packet can now be processed by the host, so raise a PKTRX interrupt, just like the normal case. Signed-off-by: Finn Thain Tested-by: Laurent Vivier --- Changed since v2: - Assert PKTRX interrupt when releasing withheld packet. --- hw/net/dp8393x.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c index 13513986f0..99c5dad7c4 100644 --- a/hw/net/dp8393x.c +++ b/hw/net/dp8393x.c @@ -809,7 +809,17 @@ static ssize_t dp8393x_receive(NetClientState *nc, const uint8_t * buf, return -1; } /* Link has been updated by host */ + +/* Clear in_use */ +size = sizeof(uint16_t) * width; +address = dp8393x_crda(s) + sizeof(uint16_t) * 6 * width; +dp8393x_put(s, width, 0, 0); +address_space_rw(&s->as, address, MEMTXATTRS_UNSPECIFIED, + (uint8_t *)s->data, size, 1); + +/* Move to next descriptor */ s->regs[SONIC_CRDA] = s->regs[SONIC_LLFA]; +s->regs[SONIC_ISR] |= SONIC_ISR_PKTRX; } /* Save current position */ -- 2.24.1
[PATCH v3 13/14] dp8393x: Don't reset Silicon Revision register
The jazzsonic driver in Linux uses the Silicon Revision register value
to probe the chip. The driver fails unless the SR register contains 4.
Unfortunately, reading this register in QEMU usually returns 0 because
the s->regs[] array gets wiped after a software reset.
Fixes: bd8f1ebce4 ("net/dp8393x: fix hardware reset")
Signed-off-by: Finn Thain
---
hw/net/dp8393x.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 1b73a8703b..71af0fad51 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -591,6 +591,10 @@ static uint64_t dp8393x_read(void *opaque, hwaddr addr,
unsigned int size)
val |= s->cam[s->regs[SONIC_CEP] & 0xf][2* (SONIC_CAP0 - reg)];
}
break;
+/* Read-only */
+case SONIC_SR:
+val = 4; /* only revision recognized by Linux/mips */
+break;
/* All other registers have no special contrainst */
default:
val = s->regs[reg];
@@ -971,7 +975,6 @@ static void dp8393x_realize(DeviceState *dev, Error **errp)
qemu_format_nic_info_str(qemu_get_queue(s->nic), s->conf.macaddr.a);
s->watchdog = timer_new_ns(QEMU_CLOCK_VIRTUAL, dp8393x_watchdog, s);
-s->regs[SONIC_SR] = 0x0004; /* only revision recognized by Linux */
memory_region_init_ram(&s->prom, OBJECT(dev),
"dp8393x-prom", SONIC_PROM_SIZE, &local_err);
--
2.24.1
