On 19/12/2019 20:14, Laurent Vivier wrote: > The command byte is not decoded correctly. > > This patch reworks the RTC/PRAM interface and fixes the problem. > It adds a comment before the function to explain how are encoded commands > and some trace-events to ease debugging. > > Bug: https://bugs.launchpad.net/qemu/+bug/1856549 > Fixes: 6dca62a000 ("hw/m68k: add VIA support") > Signed-off-by: Laurent Vivier <laur...@vivier.eu> > --- > hw/misc/mac_via.c | 274 ++++++++++++++++++++++++++++++------------- > hw/misc/trace-events | 19 +++ > 2 files changed, 210 insertions(+), 83 deletions(-) > > diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c > index f3f130ad96..e5658af922 100644 > --- a/hw/misc/mac_via.c > +++ b/hw/misc/mac_via.c > @@ -27,7 +27,7 @@ > #include "sysemu/runstate.h" > #include "qapi/error.h" > #include "qemu/cutils.h" > - > +#include "trace.h" > > /* > * VIAs: There are two in every machine, > @@ -278,6 +278,21 @@ > /* VIA returns time offset from Jan 1, 1904, not 1970 */ > #define RTC_OFFSET 2082844800 > > +enum { > + REG_0, > + REG_1, > + REG_2, > + REG_3, > + REG_TEST, > + REG_WPROTECT, > + REG_PRAM_ADDR, > + REG_PRAM_ADDR_LAST = REG_PRAM_ADDR + 19, > + REG_PRAM_SECT, > + REG_PRAM_SECT_LAST = REG_PRAM_SECT + 7, > + REG_INVALID, > + REG_EMPTY = 0xff, > +}; > + > static void via1_VBL_update(MOS6522Q800VIA1State *v1s) > { > MOS6522State *s = MOS6522(v1s); > @@ -360,10 +375,62 @@ static void via2_irq_request(void *opaque, int irq, int > level) > mdc->update_irq(s); > } > > +/* > + * RTC Commands > + * > + * Command byte Register addressed by the command > + * > + * z0000001 Seconds register 0 (lowest-order byte) > + * z0000101 Seconds register 1 > + * z0001001 Seconds register 2 > + * z0001101 Seconds register 3 (highest-order byte) > + * 00110001 Test register (write-only) > + * 00110101 Write-Protect Register (write-only) > + * z010aa01 RAM address 100aa ($10-$13) (first 20 bytes only) > + * z1aaaa01 RAM address 0aaaa ($00-$0F) (first 20 bytes only) > + * z0111aaa Extended memory designator and sector number > + * > + * For a read request, z=1, for a write z=0 > + * The letter a indicates bits whose value depend on what parameter > + * RAM byte you want to address > + */ > +static int via1_rtc_compact_cmd(uint8_t value) > +{ > + uint8_t read = value & 0x80; > + > + value &= 0x7f; > + > + /* the last 2 bits of a command byte must always be 0b01 ... */ > + if ((value & 0x78) == 0x38) { > + /* except for the extended memory designator */ > + return read | (REG_PRAM_SECT + (value & 0x07)); > + } > + if ((value & 0x03) == 0x01) { > + value >>= 2; > + if ((value & 0x1c) == 0) { > + /* seconds registers */ > + return read | (REG_0 + (value & 0x03)); > + } else if ((value == 0x0c) && !read) { > + return REG_TEST; > + } else if ((value == 0x0d) && !read) { > + return REG_WPROTECT; > + } else if ((value & 0x1c) == 0x08) { > + /* RAM address 0x10 to 0x13 */ > + return read | (REG_PRAM_ADDR + 0x10 + (value & 0x03)); > + } else if ((value & 0x43) == 0x41) { > + /* RAM address 0x00 to 0x0f */ > + return read | (REG_PRAM_ADDR + (value & 0x0f)); > + } > + } > + return REG_INVALID; > +} > + > static void via1_rtc_update(MacVIAState *m) > { > MOS6522Q800VIA1State *v1s = &m->mos6522_via1; > MOS6522State *s = MOS6522(v1s); > + int cmd, sector, addr; > + uint32_t time; > > if (s->b & VIA1B_vRTCEnb) { > return; > @@ -376,7 +443,9 @@ static void via1_rtc_update(MacVIAState *m) > m->data_out |= s->b & VIA1B_vRTCData; > m->data_out_cnt++; > } > + trace_via1_rtc_update_data_out(m->data_out_cnt, m->data_out); > } else { > + trace_via1_rtc_update_data_in(m->data_in_cnt, m->data_in); > /* receive bits from the RTC */ > if ((v1s->last_b & VIA1B_vRTCClk) && > !(s->b & VIA1B_vRTCClk) && > @@ -386,96 +455,132 @@ static void via1_rtc_update(MacVIAState *m) > m->data_in <<= 1; > m->data_in_cnt--; > } > + return; > } > > - if (m->data_out_cnt == 8) { > - m->data_out_cnt = 0; > - > - if (m->cmd == 0) { > - if (m->data_out & 0x80) { > - /* this is a read command */ > - uint32_t time = m->tick_offset + > - (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) / > - NANOSECONDS_PER_SECOND); > - if (m->data_out == 0x81) { /* seconds register 0 */ > - m->data_in = time & 0xff; > - m->data_in_cnt = 8; > - } else if (m->data_out == 0x85) { /* seconds register 1 */ > - m->data_in = (time >> 8) & 0xff; > - m->data_in_cnt = 8; > - } else if (m->data_out == 0x89) { /* seconds register 2 */ > - m->data_in = (time >> 16) & 0xff; > - m->data_in_cnt = 8; > - } else if (m->data_out == 0x8d) { /* seconds register 3 */ > - m->data_in = (time >> 24) & 0xff; > - m->data_in_cnt = 8; > - } else if ((m->data_out & 0xf3) == 0xa1) { > - /* PRAM address 0x10 -> 0x13 */ > - int addr = (m->data_out >> 2) & 0x03; > - m->data_in = v1s->PRAM[addr]; > - m->data_in_cnt = 8; > - } else if ((m->data_out & 0xf3) == 0xa1) { > - /* PRAM address 0x00 -> 0x0f */ > - int addr = (m->data_out >> 2) & 0x0f; > - m->data_in = v1s->PRAM[addr]; > - m->data_in_cnt = 8; > - } else if ((m->data_out & 0xf8) == 0xb8) { > - /* extended memory designator and sector number */ > - m->cmd = m->data_out; > - } > - } else { > - /* this is a write command */ > - m->cmd = m->data_out; > + if (m->data_out_cnt != 8) { > + return; > + } > + > + m->data_out_cnt = 0; > + > + trace_via1_rtc_internal_status(m->cmd, m->alt, m->data_out); > + /* first byte: it's a command */ > + if (m->cmd == REG_EMPTY) { > + > + cmd = via1_rtc_compact_cmd(m->data_out); > + trace_via1_rtc_internal_cmd(cmd); > + > + if (cmd == REG_INVALID) { > + trace_via1_rtc_cmd_invalid(m->data_out); > + return; > + } > + > + if (cmd & 0x80) { /* this is a read command */ > + switch (cmd & 0x7f) { > + case REG_0...REG_3: /* seconds registers */ > + /* > + * register 0 is lowest-order byte > + * register 3 is highest-order byte > + */ > + > + time = m->tick_offset + > (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) > + / NANOSECONDS_PER_SECOND); > + trace_via1_rtc_internal_time(time); > + m->data_in = (time >> ((cmd & 0x03) << 3)) & 0xff; > + m->data_in_cnt = 8; > + trace_via1_rtc_cmd_seconds_read((cmd & 0x7f) - REG_0, > + m->data_in); > + break; > + case REG_PRAM_ADDR...REG_PRAM_ADDR_LAST: > + /* PRAM address 0x00 -> 0x13 */ > + m->data_in = v1s->PRAM[(cmd & 0x7f) - REG_PRAM_ADDR]; > + m->data_in_cnt = 8; > + trace_via1_rtc_cmd_pram_read((cmd & 0x7f) - REG_PRAM_ADDR, > + m->data_in); > + break; > + case REG_PRAM_SECT...REG_PRAM_SECT_LAST: > + /* > + * extended memory designator and sector number > + * the only two-byte read command > + */ > + trace_via1_rtc_internal_set_cmd(cmd); > + m->cmd = cmd; > + break; > + default: > + g_assert_not_reached(); > + break; > } > + return; > + } > + > + /* this is a write command, needs a parameter */ > + if (cmd == REG_WPROTECT || !m->wprotect) { > + trace_via1_rtc_internal_set_cmd(cmd); > + m->cmd = cmd; > } else { > + trace_via1_rtc_internal_ignore_cmd(cmd); > + } > + return; > + } > + > + /* second byte: it's a parameter */ > + if (m->alt == REG_EMPTY) { > + switch (m->cmd & 0x7f) { > + case REG_0...REG_3: /* seconds register */ > + /* FIXME */ > + trace_via1_rtc_cmd_seconds_write(m->cmd - REG_0, m->data_out); > + m->cmd = REG_EMPTY; > + break; > + case REG_TEST: > + /* device control: nothing to do */ > + trace_via1_rtc_cmd_test_write(m->data_out); > + m->cmd = REG_EMPTY; > + break; > + case REG_WPROTECT: > + /* Write Protect register */ > + trace_via1_rtc_cmd_wprotect_write(m->data_out); > + m->wprotect = !!(m->data_out & 0x80); > + m->cmd = REG_EMPTY; > + break; > + case REG_PRAM_ADDR...REG_PRAM_ADDR_LAST: > + /* PRAM address 0x00 -> 0x13 */ > + trace_via1_rtc_cmd_pram_write(m->cmd - REG_PRAM_ADDR, > m->data_out); > + v1s->PRAM[m->cmd - REG_PRAM_ADDR] = m->data_out; > + m->cmd = REG_EMPTY; > + break; > + case REG_PRAM_SECT...REG_PRAM_SECT_LAST: > + addr = (m->data_out >> 2) & 0x1f; > + sector = (m->cmd & 0x7f) - REG_PRAM_SECT; > if (m->cmd & 0x80) { > - if ((m->cmd & 0xf8) == 0xb8) { > - /* extended memory designator and sector number */ > - int sector = m->cmd & 0x07; > - int addr = (m->data_out >> 2) & 0x1f; > - > - m->data_in = v1s->PRAM[sector * 8 + addr]; > - m->data_in_cnt = 8; > - } > - } else if (!m->wprotect) { > - /* this is a write command */ > - if (m->alt != 0) { > - /* extended memory designator and sector number */ > - int sector = m->cmd & 0x07; > - int addr = (m->alt >> 2) & 0x1f; > - > - v1s->PRAM[sector * 8 + addr] = m->data_out; > - > - m->alt = 0; > - } else if (m->cmd == 0x01) { /* seconds register 0 */ > - /* FIXME */ > - } else if (m->cmd == 0x05) { /* seconds register 1 */ > - /* FIXME */ > - } else if (m->cmd == 0x09) { /* seconds register 2 */ > - /* FIXME */ > - } else if (m->cmd == 0x0d) { /* seconds register 3 */ > - /* FIXME */ > - } else if (m->cmd == 0x31) { > - /* Test Register */ > - } else if (m->cmd == 0x35) { > - /* Write Protect register */ > - m->wprotect = m->data_out & 1; > - } else if ((m->cmd & 0xf3) == 0xa1) { > - /* PRAM address 0x10 -> 0x13 */ > - int addr = (m->cmd >> 2) & 0x03; > - v1s->PRAM[addr] = m->data_out; > - } else if ((m->cmd & 0xf3) == 0xa1) { > - /* PRAM address 0x00 -> 0x0f */ > - int addr = (m->cmd >> 2) & 0x0f; > - v1s->PRAM[addr] = m->data_out; > - } else if ((m->cmd & 0xf8) == 0xb8) { > - /* extended memory designator and sector number */ > - m->alt = m->cmd; > - } > + /* it's a read */ > + m->data_in = v1s->PRAM[sector * 32 + addr]; > + m->data_in_cnt = 8; > + trace_via1_rtc_cmd_pram_sect_read(sector, addr, > + sector * 32 + addr, > + m->data_in); > + m->cmd = REG_EMPTY; > + } else { > + /* it's a write, we need one more parameter */ > + trace_via1_rtc_internal_set_alt(addr, sector, addr); > + m->alt = addr; > } > + break; > + default: > + g_assert_not_reached(); > + break; > } > - m->data_out = 0; > + return; > } > + > + /* third byte: it's the data of a REG_PRAM_SECT write */ > + g_assert(REG_PRAM_SECT <= m->cmd && m->cmd <= REG_PRAM_SECT_LAST); > + sector = m->cmd - REG_PRAM_SECT; > + v1s->PRAM[sector * 32 + m->alt] = m->data_out; > + trace_via1_rtc_cmd_pram_sect_write(sector, m->alt, sector * 32 + m->alt, > + m->data_out); > + m->alt = REG_EMPTY; > + m->cmd = REG_EMPTY; > } > > static int adb_via_poll(MacVIAState *s, int state, uint8_t *data) > @@ -742,6 +847,9 @@ static void mac_via_reset(DeviceState *dev) > v1s->next_VBL = 0; > timer_del(v1s->one_second_timer); > v1s->next_second = 0; > + > + m->cmd = REG_EMPTY; > + m->alt = REG_EMPTY; > } > > static void mac_via_realize(DeviceState *dev, Error **errp) > diff --git a/hw/misc/trace-events b/hw/misc/trace-events > index 1deb1d08c1..2e0c820834 100644 > --- a/hw/misc/trace-events > +++ b/hw/misc/trace-events > @@ -149,3 +149,22 @@ bcm2835_mbox_write(unsigned int size, uint64_t addr, > uint64_t value) "mbox write > bcm2835_mbox_read(unsigned int size, uint64_t addr, uint64_t value) "mbox > read sz:%u addr:0x%"PRIx64" data:0x%"PRIx64 > bcm2835_mbox_irq(unsigned level) "mbox irq:ARM level:%u" > bcm2835_mbox_property(uint32_t tag, uint32_t bufsize, size_t resplen) "mbox > property tag:0x%08x in_sz:%u out_sz:%zu" > + > +# mac_via.c > +via1_rtc_update_data_out(int count, int value) "count=%d value=0x%02x" > +via1_rtc_update_data_in(int count, int value) "count=%d value=0x%02x" > +via1_rtc_internal_status(int cmd, int alt, int value) "cmd=0x%02x alt=0x%02x > value=0x%02x" > +via1_rtc_internal_cmd(int cmd) "cmd=0x%02x" > +via1_rtc_cmd_invalid(int value) "value=0x%02x" > +via1_rtc_internal_time(uint32_t time) "time=0x%08x" > +via1_rtc_internal_set_cmd(int cmd) "cmd=0x%02x" > +via1_rtc_internal_ignore_cmd(int cmd) "cmd=0x%02x" > +via1_rtc_internal_set_alt(int alt, int sector, int offset) "alt=0x%02x > sector=%u offset=%u" > +via1_rtc_cmd_seconds_read(int reg, int value) "reg=%d value=0x%02x" > +via1_rtc_cmd_seconds_write(int reg, int value) "reg=%d value=0x%02x" > +via1_rtc_cmd_test_write(int value) "value=0x%02x" > +via1_rtc_cmd_wprotect_write(int value) "value=0x%02x" > +via1_rtc_cmd_pram_read(int addr, int value) "addr=%u value=0x%02x" > +via1_rtc_cmd_pram_write(int addr, int value) "addr=%u value=0x%02x" > +via1_rtc_cmd_pram_sect_read(int sector, int offset, int addr, int value) > "sector=%u offset=%u addr=%d value=0x%02x" > +via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) > "sector=%u offset=%u addr=%d value=0x%02x"
Seems like a good improvement to me. Reviewed-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> ATB, Mark.
