Re: [PATCH] hw/audio/c97: fix abort in audio_calloc()

[Qiang Liu <cyruscyliu@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4025 bytes --]

Hi Volker,


> Did you see my patches at
> https://lists.nongnu.org/archive/html/qemu-devel/2022-12/msg02895.html ?
> Patches 01/11 and 02/11 prevent the disturbing error message from
> audio_calloc and later patches remove the audio_calloc function.
>
Oh, I missed these. Thank you for pointing it out.


> I think the subject of your patch isn't correct. Your patch doesn't fix
> an abort in audio_calloc. In
> https://gitlab.com/qemu-project/qemu/-/issues/1393 you correctly notice
> this was already fixed.

Fair enough.


> > Section 5.10.2 of the AC97 specification (
> https://hands.com/~lkcl/ac97_r23.pdf)
> > shows the feasibility to support for rates other than 48kHZ.
> Specifically,
> > AC97_PCM_Front_DAC_Rate (reg 2Ch) should be from 8kHZ to 48kHZ.
>
> I think you misread section 5.10.2 of the AC97 Revision 2.3
> specification. Section 5.10 is about S/PDIF concurrency. It doesn't say
> anything about the lowest sample rate limit without concurrent S/PDIF
> transmission. The emulated SigmaTel STAC9700 codec doesn't even have a
> S/PDIF output. But I have an example for sample rates lower than 8kHz.
> The Texas Instruments LM4546B is an AC97 codec which supports sample
> rates from 4kHz - 48kHz.
>
Thank you for pointing it out! Now, I understand better!


> The STAC9700 is a 48kHz fixed rate codec. You won't find anything about
> the highest or lowest sample rate in its data sheet. Someone added the
> VRA and VRM modes in QEMU and the guest drivers don't seem to mind.
>
> I would like to keep the ability to select a 1Hz sample rate, as there
> is no reason to artificially limit the lowest supported sample rate. See
> https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg03987.html
>
> I would support a patch to limit the VRA and VRM modes to the highest
> supported rate of 48kHz. This is a technical limit for single data rate.
>
> > Before Volker Rümelin fixed it in 12f4abf6a245 and 0cbc8bd4694f, an
> adversary
> > could leverage this to crash QEMU.
> >
> > Fixes: e5c9a13e2670 ("PCI AC97 emulation by malc.")
> > Reported-by: Volker Rümelin<vr_qemu@t-online.de>
> > Reported-by: Qiang Liu<cyruscyliu@gmail.com>
> > Resolves:https://gitlab.com/qemu-project/qemu/-/issues/1393
> > Signed-off-by: Qiang Liu<cyruscyliu@gmail.com>
> > ---
> >   hw/audio/ac97.c | 11 ++++++++---
> >   1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/audio/ac97.c b/hw/audio/ac97.c
> > index be2dd701a4..826411e462 100644
> > --- a/hw/audio/ac97.c
> > +++ b/hw/audio/ac97.c
> > @@ -625,9 +625,14 @@ static void nam_writew(void *opaque, uint32_t addr,
> uint32_t val)
> >           break;
> >       case AC97_PCM_Front_DAC_Rate:
> >           if (mixer_load(s, AC97_Extended_Audio_Ctrl_Stat) & EACS_VRA) {
> > -            mixer_store(s, addr, val);
> > -            dolog("Set front DAC rate to %d\n", val);
> > -            open_voice(s, PO_INDEX, val);
> > +            if (val >= 8000 && val <= 48000) {
> > +                mixer_store(s, addr, val);
> > +                dolog("Set front DAC rate to %d\n", val);
> > +                open_voice(s, PO_INDEX, val);
> > +            } else {
> > +                dolog("Attempt to set front DAC rate to %d, but valid
> is"
> > +                      "8-48kHZ\n", val);
>
> This is not correct. If you limit the sample rate, you should echo back
> the closest supported sample rate. See AC'97 2.3 Section 5.8.3. It's not
> a guest error if the guest writes an unsupported sample rate to the
> Audio Sample Rate Control Registers, which means it's also not necessary
> to log this.
>
> With best regards,
> Volker
>

I rethink and want to say that a low sample rate could be a problem. Have
checked the missed patch
https://lists.nongnu.org/archive/html/qemu-devel/2022-12/msg02895.html, I
think you have addressed my concern. Thank all your suggestions again!
Let's close this discussion and I will close the issue.

Best,
Qiang

[-- Attachment #2: Type: text/html, Size: 6001 bytes --]