| Message ID | 20231105172552.8405-1-vr_qemu@t-online.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/audio/hda-codec bug fixes | expand |
On Sun, Nov 5, 2023 at 9:26 PM Volker Rümelin <vr_qemu@t-online.de> wrote: > > After a relatively short time, there is an multiplication overflow > when multiplying (now - buft_start) with hda_bytes_per_second(). > While the uptime now - buft_start only overflows after 2**63 ns > = 292.27 years, this happens hda_bytes_per_second() times faster > with the multiplication. At 44100 samples/s * 2 channels > * 2 bytes/channel = 176400 bytes/s that is 14.52 hours. After the > multiplication overflow the affected audio stream stalls. > > Replace the multiplication and following division with muldiv64() > to prevent a multiplication overflow. > > Fixes: 280c1e1cdb ("audio/hda: create millisecond timers that handle IO") > Reported-by: M_O_Bz <m_o_bz@163.com> > Signed-off-by: Volker Rümelin <vr_qemu@t-online.de> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/audio/hda-codec.c | 29 +++++++++++++++++++++-------- > 1 file changed, 21 insertions(+), 8 deletions(-) > > diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c > index b9ad1f4c39..f756e419bb 100644 > --- a/hw/audio/hda-codec.c > +++ b/hw/audio/hda-codec.c > @@ -22,6 +22,7 @@ > #include "hw/qdev-properties.h" > #include "intel-hda.h" > #include "migration/vmstate.h" > +#include "qemu/host-utils.h" > #include "qemu/module.h" > #include "intel-hda-defs.h" > #include "audio/audio.h" > @@ -189,9 +190,9 @@ struct HDAAudioState { > bool use_timer; > }; > > -static inline int64_t hda_bytes_per_second(HDAAudioStream *st) > +static inline uint32_t hda_bytes_per_second(HDAAudioStream *st) > { > - return 2LL * st->as.nchannels * st->as.freq; > + return 2 * (uint32_t)st->as.nchannels * (uint32_t)st->as.freq; > } > > static inline void hda_timer_sync_adjust(HDAAudioStream *st, int64_t target_pos) > @@ -222,12 +223,18 @@ static void hda_audio_input_timer(void *opaque) > > int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > - int64_t buft_start = st->buft_start; > + int64_t uptime = now - st->buft_start; > int64_t wpos = st->wpos; > int64_t rpos = st->rpos; > + int64_t wanted_rpos; > > - int64_t wanted_rpos = hda_bytes_per_second(st) * (now - buft_start) > - / NANOSECONDS_PER_SECOND; > + if (uptime <= 0) { > + /* wanted_rpos <= 0 */ > + goto out_timer; > + } > + > + wanted_rpos = muldiv64(uptime, hda_bytes_per_second(st), > + NANOSECONDS_PER_SECOND); > wanted_rpos &= -4; /* IMPORTANT! clip to frames */ > > if (wanted_rpos <= rpos) { > @@ -286,12 +293,18 @@ static void hda_audio_output_timer(void *opaque) > > int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > - int64_t buft_start = st->buft_start; > + int64_t uptime = now - st->buft_start; > int64_t wpos = st->wpos; > int64_t rpos = st->rpos; > + int64_t wanted_wpos; > + > + if (uptime <= 0) { > + /* wanted_wpos <= 0 */ > + goto out_timer; > + } > > - int64_t wanted_wpos = hda_bytes_per_second(st) * (now - buft_start) > - / NANOSECONDS_PER_SECOND; > + wanted_wpos = muldiv64(uptime, hda_bytes_per_second(st), > + NANOSECONDS_PER_SECOND); > wanted_wpos &= -4; /* IMPORTANT! clip to frames */ > > if (wanted_wpos <= wpos) { > -- > 2.35.3 > >
diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c index b9ad1f4c39..f756e419bb 100644 --- a/hw/audio/hda-codec.c +++ b/hw/audio/hda-codec.c @@ -22,6 +22,7 @@ #include "hw/qdev-properties.h" #include "intel-hda.h" #include "migration/vmstate.h" +#include "qemu/host-utils.h" #include "qemu/module.h" #include "intel-hda-defs.h" #include "audio/audio.h" @@ -189,9 +190,9 @@ struct HDAAudioState { bool use_timer; }; -static inline int64_t hda_bytes_per_second(HDAAudioStream *st) +static inline uint32_t hda_bytes_per_second(HDAAudioStream *st) { - return 2LL * st->as.nchannels * st->as.freq; + return 2 * (uint32_t)st->as.nchannels * (uint32_t)st->as.freq; } static inline void hda_timer_sync_adjust(HDAAudioStream *st, int64_t target_pos) @@ -222,12 +223,18 @@ static void hda_audio_input_timer(void *opaque) int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); - int64_t buft_start = st->buft_start; + int64_t uptime = now - st->buft_start; int64_t wpos = st->wpos; int64_t rpos = st->rpos; + int64_t wanted_rpos; - int64_t wanted_rpos = hda_bytes_per_second(st) * (now - buft_start) - / NANOSECONDS_PER_SECOND; + if (uptime <= 0) { + /* wanted_rpos <= 0 */ + goto out_timer; + } + + wanted_rpos = muldiv64(uptime, hda_bytes_per_second(st), + NANOSECONDS_PER_SECOND); wanted_rpos &= -4; /* IMPORTANT! clip to frames */ if (wanted_rpos <= rpos) { @@ -286,12 +293,18 @@ static void hda_audio_output_timer(void *opaque) int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); - int64_t buft_start = st->buft_start; + int64_t uptime = now - st->buft_start; int64_t wpos = st->wpos; int64_t rpos = st->rpos; + int64_t wanted_wpos; + + if (uptime <= 0) { + /* wanted_wpos <= 0 */ + goto out_timer; + } - int64_t wanted_wpos = hda_bytes_per_second(st) * (now - buft_start) - / NANOSECONDS_PER_SECOND; + wanted_wpos = muldiv64(uptime, hda_bytes_per_second(st), + NANOSECONDS_PER_SECOND); wanted_wpos &= -4; /* IMPORTANT! clip to frames */ if (wanted_wpos <= wpos) {
After a relatively short time, there is an multiplication overflow when multiplying (now - buft_start) with hda_bytes_per_second(). While the uptime now - buft_start only overflows after 2**63 ns = 292.27 years, this happens hda_bytes_per_second() times faster with the multiplication. At 44100 samples/s * 2 channels * 2 bytes/channel = 176400 bytes/s that is 14.52 hours. After the multiplication overflow the affected audio stream stalls. Replace the multiplication and following division with muldiv64() to prevent a multiplication overflow. Fixes: 280c1e1cdb ("audio/hda: create millisecond timers that handle IO") Reported-by: M_O_Bz <m_o_bz@163.com> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de> --- hw/audio/hda-codec.c | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-)