disas/m68k.c | 1 + hw/arm/pxa2xx.c | 2 +- hw/audio/cs4231a.c | 1 + hw/audio/gusemu_hal.c | 1 + hw/audio/sb16.c | 4 +--- hw/display/cg3.c | 1 + hw/display/cirrus_vga.c | 3 ++- hw/timer/sh_timer.c | 1 + target/arm/helper.c | 1 + target/i386/translate.c | 2 ++ target/mips/translate.c | 2 ++ 11 files changed, 14 insertions(+), 5 deletions(-)
Many of these are marked as "intentional/fix required" because they
just need adding a fall through comment. This is exactly what this
patch does, except for target/mips/translate.c where it is easier to
duplicate the code, and hw/audio/sb16.c where I consulted the DOSBox
sources and decide to just remove the LOG_UNIMP before the fallthrough.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
disas/m68k.c | 1 +
hw/arm/pxa2xx.c | 2 +-
hw/audio/cs4231a.c | 1 +
hw/audio/gusemu_hal.c | 1 +
hw/audio/sb16.c | 4 +---
hw/display/cg3.c | 1 +
hw/display/cirrus_vga.c | 3 ++-
hw/timer/sh_timer.c | 1 +
target/arm/helper.c | 1 +
target/i386/translate.c | 2 ++
target/mips/translate.c | 2 ++
11 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/disas/m68k.c b/disas/m68k.c
index a687df437c..0dc8aa1a3c 100644
--- a/disas/m68k.c
+++ b/disas/m68k.c
@@ -1623,6 +1623,7 @@ print_insn_arg (const char *d,
case 'X':
place = '8';
+ /* fall through */
case 'Y':
case 'Z':
case 'W':
diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index b67b0cefb6..f598a1c053 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -409,7 +409,7 @@ static uint64_t pxa2xx_mm_read(void *opaque, hwaddr addr,
case MDCNFG ... SA1110:
if ((addr & 3) == 0)
return s->mm_regs[addr >> 2];
-
+ /* fall through */
default:
printf("%s: Bad register " REG_FMT "\n", __func__, addr);
break;
diff --git a/hw/audio/cs4231a.c b/hw/audio/cs4231a.c
index aaebec1839..9089dcb47e 100644
--- a/hw/audio/cs4231a.c
+++ b/hw/audio/cs4231a.c
@@ -305,6 +305,7 @@ static void cs_reset_voices (CSState *s, uint32_t val)
case 6:
as.endianness = 1;
+ /* fall through */
case 2:
as.fmt = AUD_FMT_S16;
s->shift = as.nchannels;
diff --git a/hw/audio/gusemu_hal.c b/hw/audio/gusemu_hal.c
index 1150fc4426..ae40ca341c 100644
--- a/hw/audio/gusemu_hal.c
+++ b/hw/audio/gusemu_hal.c
@@ -261,6 +261,7 @@ void gus_write(GUSEmuState * state, int port, int size, unsigned int data)
GUSregb(IRQStatReg2x6) = 0x10;
GUS_irqrequest(state, state->gusirq, 1);
}
+ /* fall through */
case 0x20D: /* SB2xCd no IRQ */
GUSregb(SB2xCd) = (uint8_t) data;
break;
diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c
index 5a4d32364e..665a7e75c5 100644
--- a/hw/audio/sb16.c
+++ b/hw/audio/sb16.c
@@ -741,10 +741,8 @@ static void complete (SB16State *s)
ldebug ("set time const %d\n", s->time_const);
break;
- case 0x42: /* FT2 sets output freq with this, go figure */
- qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it"
- " should\n");
case 0x41:
+ case 0x42: /* FT2 sets output freq with this, go figure */
s->freq = dsp_get_hilo (s);
ldebug ("set freq %d\n", s->freq);
break;
diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index 6fff4852c5..1c199ab369 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -232,6 +232,7 @@ static void cg3_reg_write(void *opaque, hwaddr addr, uint64_t val,
s->b[s->dac_index] = regval;
/* Index autoincrement */
s->dac_index = (s->dac_index + 1) & 0xff;
+ /* fall through */
default:
s->dac_state = 0;
break;
diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 7583b18c29..04c87c8e8d 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -1426,7 +1426,8 @@ static void cirrus_vga_write_sr(CirrusVGAState * s, uint32_t val)
s->vga.hw_cursor_y = (val << 3) | (s->vga.sr_index >> 5);
break;
case 0x07: // Extended Sequencer Mode
- cirrus_update_memory_access(s);
+ cirrus_update_memory_access(s);
+ /* fall through */
case 0x08: // EEPROM Control
case 0x09: // Scratch Register 0
case 0x0a: // Scratch Register 1
diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c
index 5f8736cf10..91b18ba312 100644
--- a/hw/timer/sh_timer.c
+++ b/hw/timer/sh_timer.c
@@ -74,6 +74,7 @@ static uint32_t sh_timer_read(void *opaque, hwaddr offset)
case OFFSET_TCPR:
if (s->feat & TIMER_FEAT_CAPT)
return s->tcpr;
+ /* fall through */
default:
hw_error("sh_timer_read: Bad offset %x\n", (int)offset);
return 0;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 66afb08ee0..12735ff089 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -12277,6 +12277,7 @@ int arm_rmode_to_sf(int rmode)
/* FIXME: add support for TIEAWAY and ODD */
qemu_log_mask(LOG_UNIMP, "arm: unimplemented rounding mode: %d\n",
rmode);
+ /* fall through for now */
case FPROUNDING_TIEEVEN:
default:
rmode = float_round_nearest_even;
diff --git a/target/i386/translate.c b/target/i386/translate.c
index 07d185e7b6..1f9d1d9b24 100644
--- a/target/i386/translate.c
+++ b/target/i386/translate.c
@@ -4689,6 +4689,7 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
case 0x82:
if (CODE64(s))
goto illegal_op;
+ /* fall through */
case 0x80: /* GRP1 */
case 0x81:
case 0x83:
@@ -8292,6 +8293,7 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
case 0x10e ... 0x10f:
/* 3DNow! instructions, ignore prefixes */
s->prefix &= ~(PREFIX_REPZ | PREFIX_REPNZ | PREFIX_DATA);
+ /* fall through */
case 0x110 ... 0x117:
case 0x128 ... 0x12f:
case 0x138 ... 0x13a:
diff --git a/target/mips/translate.c b/target/mips/translate.c
index 20b43c0337..60408dcda3 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -19874,6 +19874,8 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
case OPC_MTHC1:
check_cp1_enabled(ctx);
check_insn(ctx, ISA_MIPS32R2);
+ gen_cp1(ctx, op1, rt, rd);
+ break;
case OPC_MFC1:
case OPC_CFC1:
case OPC_MTC1:
--
2.17.1
Le mer. 1 août 2018 12:15, Paolo Bonzini <pbonzini@redhat.com> a écrit :
> Many of these are marked as "intentional/fix required" because they
> just need adding a fall through comment. This is exactly what this
> patch does, except for target/mips/translate.c where it is easier to
> duplicate the code, and hw/audio/sb16.c where I consulted the DOSBox
> sources and decide to just remove the LOG_UNIMP before the fallthrough.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> disas/m68k.c | 1 +
> hw/arm/pxa2xx.c | 2 +-
> hw/audio/cs4231a.c | 1 +
> hw/audio/gusemu_hal.c | 1 +
> hw/audio/sb16.c | 4 +---
> hw/display/cg3.c | 1 +
> hw/display/cirrus_vga.c | 3 ++-
> hw/timer/sh_timer.c | 1 +
> target/arm/helper.c | 1 +
> target/i386/translate.c | 2 ++
> target/mips/translate.c | 2 ++
> 11 files changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/disas/m68k.c b/disas/m68k.c
> index a687df437c..0dc8aa1a3c 100644
> --- a/disas/m68k.c
> +++ b/disas/m68k.c
> @@ -1623,6 +1623,7 @@ print_insn_arg (const char *d,
>
> case 'X':
> place = '8';
> + /* fall through */
> case 'Y':
> case 'Z':
> case 'W':
> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> index b67b0cefb6..f598a1c053 100644
> --- a/hw/arm/pxa2xx.c
> +++ b/hw/arm/pxa2xx.c
> @@ -409,7 +409,7 @@ static uint64_t pxa2xx_mm_read(void *opaque, hwaddr
> addr,
> case MDCNFG ... SA1110:
> if ((addr & 3) == 0)
> return s->mm_regs[addr >> 2];
> -
> + /* fall through */
> default:
> printf("%s: Bad register " REG_FMT "\n", __func__, addr);
> break;
> diff --git a/hw/audio/cs4231a.c b/hw/audio/cs4231a.c
> index aaebec1839..9089dcb47e 100644
> --- a/hw/audio/cs4231a.c
> +++ b/hw/audio/cs4231a.c
> @@ -305,6 +305,7 @@ static void cs_reset_voices (CSState *s, uint32_t val)
>
> case 6:
> as.endianness = 1;
> + /* fall through */
> case 2:
> as.fmt = AUD_FMT_S16;
> s->shift = as.nchannels;
> diff --git a/hw/audio/gusemu_hal.c b/hw/audio/gusemu_hal.c
> index 1150fc4426..ae40ca341c 100644
> --- a/hw/audio/gusemu_hal.c
> +++ b/hw/audio/gusemu_hal.c
> @@ -261,6 +261,7 @@ void gus_write(GUSEmuState * state, int port, int
> size, unsigned int data)
> GUSregb(IRQStatReg2x6) = 0x10;
> GUS_irqrequest(state, state->gusirq, 1);
> }
> + /* fall through */
> case 0x20D: /* SB2xCd no IRQ */
> GUSregb(SB2xCd) = (uint8_t) data;
> break;
> diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c
> index 5a4d32364e..665a7e75c5 100644
> --- a/hw/audio/sb16.c
> +++ b/hw/audio/sb16.c
> @@ -741,10 +741,8 @@ static void complete (SB16State *s)
> ldebug ("set time const %d\n", s->time_const);
> break;
>
> - case 0x42: /* FT2 sets output freq with this, go
> figure */
> - qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think
> it"
> - " should\n");
> case 0x41:
> + case 0x42: /* FT2 sets output freq with this, go
> figure */
> s->freq = dsp_get_hilo (s);
> ldebug ("set freq %d\n", s->freq);
> break;
> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
> index 6fff4852c5..1c199ab369 100644
> --- a/hw/display/cg3.c
> +++ b/hw/display/cg3.c
> @@ -232,6 +232,7 @@ static void cg3_reg_write(void *opaque, hwaddr addr,
> uint64_t val,
> s->b[s->dac_index] = regval;
> /* Index autoincrement */
> s->dac_index = (s->dac_index + 1) & 0xff;
> + /* fall through */
> default:
> s->dac_state = 0;
> break;
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index 7583b18c29..04c87c8e8d 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -1426,7 +1426,8 @@ static void cirrus_vga_write_sr(CirrusVGAState * s,
> uint32_t val)
> s->vga.hw_cursor_y = (val << 3) | (s->vga.sr_index >> 5);
> break;
> case 0x07: // Extended Sequencer Mode
> - cirrus_update_memory_access(s);
> + cirrus_update_memory_access(s);
> + /* fall through */
> case 0x08: // EEPROM Control
> case 0x09: // Scratch Register 0
> case 0x0a: // Scratch Register 1
> diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c
> index 5f8736cf10..91b18ba312 100644
> --- a/hw/timer/sh_timer.c
> +++ b/hw/timer/sh_timer.c
> @@ -74,6 +74,7 @@ static uint32_t sh_timer_read(void *opaque, hwaddr
> offset)
> case OFFSET_TCPR:
> if (s->feat & TIMER_FEAT_CAPT)
> return s->tcpr;
> + /* fall through */
> default:
> hw_error("sh_timer_read: Bad offset %x\n", (int)offset);
> return 0;
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 66afb08ee0..12735ff089 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -12277,6 +12277,7 @@ int arm_rmode_to_sf(int rmode)
> /* FIXME: add support for TIEAWAY and ODD */
> qemu_log_mask(LOG_UNIMP, "arm: unimplemented rounding mode: %d\n",
> rmode);
> + /* fall through for now */
>
This one looks weird, hidden bug?
For the rest:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
case FPROUNDING_TIEEVEN:
> default:
> rmode = float_round_nearest_even;
> diff --git a/target/i386/translate.c b/target/i386/translate.c
> index 07d185e7b6..1f9d1d9b24 100644
> --- a/target/i386/translate.c
> +++ b/target/i386/translate.c
> @@ -4689,6 +4689,7 @@ static target_ulong disas_insn(DisasContext *s,
> CPUState *cpu)
> case 0x82:
> if (CODE64(s))
> goto illegal_op;
> + /* fall through */
> case 0x80: /* GRP1 */
> case 0x81:
> case 0x83:
> @@ -8292,6 +8293,7 @@ static target_ulong disas_insn(DisasContext *s,
> CPUState *cpu)
> case 0x10e ... 0x10f:
> /* 3DNow! instructions, ignore prefixes */
> s->prefix &= ~(PREFIX_REPZ | PREFIX_REPNZ | PREFIX_DATA);
> + /* fall through */
> case 0x110 ... 0x117:
> case 0x128 ... 0x12f:
> case 0x138 ... 0x13a:
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index 20b43c0337..60408dcda3 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -19874,6 +19874,8 @@ static void decode_opc(CPUMIPSState *env,
> DisasContext *ctx)
> case OPC_MTHC1:
> check_cp1_enabled(ctx);
> check_insn(ctx, ISA_MIPS32R2);
> + gen_cp1(ctx, op1, rt, rd);
> + break;
> case OPC_MFC1:
> case OPC_CFC1:
> case OPC_MTC1:
> --
> 2.17.1
>
>
>
On 01/08/2018 17:25, Philippe Mathieu-Daudé wrote: > > @@ -12277,6 +12277,7 @@ int arm_rmode_to_sf(int rmode) > /* FIXME: add support for TIEAWAY and ODD */ > qemu_log_mask(LOG_UNIMP, "arm: unimplemented rounding mode: > %d\n", > rmode); > + /* fall through for now */ > > > This one looks weird, hidden bug? There is already a FIXME comment. Paolo > For the rest: > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org > <mailto:f4bug@amsat.org>>
On 1 August 2018 at 16:14, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Many of these are marked as "intentional/fix required" because they
> just need adding a fall through comment. This is exactly what this
> patch does, except for target/mips/translate.c where it is easier to
> duplicate the code, and hw/audio/sb16.c where I consulted the DOSBox
> sources and decide to just remove the LOG_UNIMP before the fallthrough.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c
> index 5a4d32364e..665a7e75c5 100644
> --- a/hw/audio/sb16.c
> +++ b/hw/audio/sb16.c
> @@ -741,10 +741,8 @@ static void complete (SB16State *s)
> ldebug ("set time const %d\n", s->time_const);
> break;
>
> - case 0x42: /* FT2 sets output freq with this, go figure */
> - qemu_log_mask(LOG_UNIMP, "cmd 0x42 might not do what it think it"
> - " should\n");
> case 0x41:
> + case 0x42: /* FT2 sets output freq with this, go figure */
> s->freq = dsp_get_hilo (s);
> ldebug ("set freq %d\n", s->freq);
> break;
I agree with the outcome; see previous discussions
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02081.html
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg02079.html
and this description of what the hardware does:
http://homepages.cae.wisc.edu/~brodskye/sb16doc/sb16doc.html#SamplingRate
We could improve the somewhat cryptic comment:
/*
* 0x41 is documented as setting the output sample rate,
* and 0x42 the input sample rate, but in fact SB16 hardware
* seems to have only a single sample rate under the hood,
* and some (buggy) guest programs such as FT2 will set the
* output rate using 0x42. Compare:
* http://homepages.cae.wisc.edu/~brodskye/sb16doc/sb16doc.html#SamplingRate
*/
thanks
-- PMM
On 01/08/2018 17:59, Peter Maydell wrote: > > We could improve the somewhat cryptic comment: > /* > * 0x41 is documented as setting the output sample rate, > * and 0x42 the input sample rate, but in fact SB16 hardware > * seems to have only a single sample rate under the hood, > * and some (buggy) guest programs such as FT2 will set the > * output rate using 0x42. Compare: > * http://homepages.cae.wisc.edu/~brodskye/sb16doc/sb16doc.html#SamplingRate > */ I sometimes enjoy keeping little bits of "old-style QEMU" in these more historical devices, but your improvement is just too good, so I'll replace it. Thanks! Paolo
© 2016 - 2025 Red Hat, Inc.