Re: [PATCH] HDA codec: Fix wanted_r/w position overflow

["Volker Rümelin" <vr_qemu@t-online.de>]

Hi,

> From: zeroway <linwei6@ruijie.com.cn>
>
> when the duration now - buft_start reach to some kind of value,
> which will get the multiply hda_bytes_per_second(st) * (now - buft_start) overflow,
> instead of calculate the wanted_r/wpos from start time to current time,
> here calculate the each timer tick delta data first in wanted_r/wpos_delta,
> and sum it all to wanted_r/wpos to avoid the overflow

you could avoid the multiplication overflow with the following code

#include "qemu/host-utils.h"

     int64_t wanted_rpos = muldiv64(now - buft_start, 
hda_bytes_per_second(st),
                                    NANOSECONDS_PER_SECOND);

and

     int64_t wanted_wpos = muldiv64(now - buft_start, 
hda_bytes_per_second(st),
                                    NANOSECONDS_PER_SECOND);

This would be a less intrusive change. The wanted_pos with your code 
will grow slower than with the original code because you sum up 
truncated results from the division by NANOSECONDS_PER_SECONDS rounded 
down to next multiple of 4.

With best regards,
Volker

>
> Signed-off-by: zeroway <linwei6@ruijie.com.cn>
> ---
>   hw/audio/hda-codec.c | 24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
> index c51d8ba617..747188221a 100644
> --- a/hw/audio/hda-codec.c
> +++ b/hw/audio/hda-codec.c
> @@ -169,6 +169,8 @@ struct HDAAudioStream {
>       uint8_t buf[8192]; /* size must be power of two */
>       int64_t rpos;
>       int64_t wpos;
> +    int64_t wanted_rpos;
> +    int64_t wanted_wpos;
>       QEMUTimer *buft;
>       int64_t buft_start;
>   };
> @@ -226,16 +228,18 @@ static void hda_audio_input_timer(void *opaque)
>       int64_t wpos = st->wpos;
>       int64_t rpos = st->rpos;
>   
> -    int64_t wanted_rpos = hda_bytes_per_second(st) * (now - buft_start)
> +    int64_t wanted_rpos_delta = hda_bytes_per_second(st) * (now - buft_start)
>                             / NANOSECONDS_PER_SECOND;
> -    wanted_rpos &= -4; /* IMPORTANT! clip to frames */
> +    st->wanted_rpos += wanted_rpos_delta;
> +    st->wanted_rpos &= -4; /* IMPORTANT! clip to frames */
>   
> -    if (wanted_rpos <= rpos) {
> +    st->buft_start = now;
> +    if (st->wanted_rpos <= rpos) {
>           /* we already transmitted the data */
>           goto out_timer;
>       }
>   
> -    int64_t to_transfer = MIN(wpos - rpos, wanted_rpos - rpos);
> +    int64_t to_transfer = MIN(wpos - rpos, st->wanted_rpos - rpos);
>       while (to_transfer) {
>           uint32_t start = (rpos & B_MASK);
>           uint32_t chunk = MIN(B_SIZE - start, to_transfer);
> @@ -290,16 +294,18 @@ static void hda_audio_output_timer(void *opaque)
>       int64_t wpos = st->wpos;
>       int64_t rpos = st->rpos;
>   
> -    int64_t wanted_wpos = hda_bytes_per_second(st) * (now - buft_start)
> +    int64_t wanted_wpos_delta = hda_bytes_per_second(st) * (now - buft_start)
>                             / NANOSECONDS_PER_SECOND;
> -    wanted_wpos &= -4; /* IMPORTANT! clip to frames */
> +    st->wanted_wpos += wanted_wpos_delta;
> +    st->wanted_wpos &= -4; /* IMPORTANT! clip to frames */
>   
> -    if (wanted_wpos <= wpos) {
> +    st->buft_start = now;
> +    if (st->wanted_wpos <= wpos) {
>           /* we already received the data */
>           goto out_timer;
>       }
>   
> -    int64_t to_transfer = MIN(B_SIZE - (wpos - rpos), wanted_wpos - wpos);
> +    int64_t to_transfer = MIN(B_SIZE - (wpos - rpos), st->wanted_wpos - wpos);
>       while (to_transfer) {
>           uint32_t start = (wpos & B_MASK);
>           uint32_t chunk = MIN(B_SIZE - start, to_transfer);
> @@ -420,6 +426,8 @@ static void hda_audio_set_running(HDAAudioStream *st, bool running)
>               int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>               st->rpos = 0;
>               st->wpos = 0;
> +            st->wanted_rpos = 0;
> +            st->wanted_wpos = 0;
>               st->buft_start = now;
>               timer_mod_anticipate_ns(st->buft, now + HDA_TIMER_TICKS);
>           } else {