Re: [PATCH 02/10] hw/audio/virtio-sound: allocate all streams in advance

[Volker Rümelin]

Am 05.01.24 um 11:54 schrieb Marc-André Lureau:
> Hi
>
> On Fri, Jan 5, 2024 at 12:34 AM Volker Rümelin <vr_qemu@t-online.de> wrote:
>> It is much easier to migrate an array of structs than individual
>> structs that are accessed via a pointer to a pointer to an array
>> of pointers to struct, where some pointers can also be NULL.
>>
>> For this reason, the audio streams are already allocated during
>> the realization phase and all stream variables that are constant
>> at runtime are initialised immediately after allocation. This is
>> a step towards being able to migrate the audio streams of the
>> virtio sound device after the next few patches.
>>
>> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>> ---
>>  hw/audio/virtio-snd.c         | 35 ++++++++++++++++++++++-------------
>>  include/hw/audio/virtio-snd.h |  1 +
>>  2 files changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
>> index 8344f61c64..36b1bb502c 100644
>> --- a/hw/audio/virtio-snd.c
>> +++ b/hw/audio/virtio-snd.c
>> @@ -447,11 +447,9 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, 
>> uint32_t stream_id)
>>
>>      stream = virtio_snd_pcm_get_stream(s, stream_id);
>>      if (stream == NULL) {
>> -        stream = g_new0(VirtIOSoundPCMStream, 1);
>> +        stream = &s->streams[stream_id];
>>          stream->active = false;
>> -        stream->id = stream_id;
>>          stream->pcm = s->pcm;
>> -        stream->s = s;
>>          QSIMPLEQ_INIT(&stream->queue);
>>          QSIMPLEQ_INIT(&stream->invalid);
> note: I can't find where s->pcm->streams[stream_id] is reset to NULL
> on pcm_release...

Hi Marc-André,

right, I'll have to remove the subordinate clause from the commit
message where I claim some pointers can be NULL. All streams get
allocated in virtio_snd_realize() with calls of virtio_snd_pcm_prepare()
and get freed in virtio_snd_unrealize().

>> @@ -463,14 +461,6 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, 
>> uint32_t stream_id)
>>      }
>>
>>      virtio_snd_get_qemu_audsettings(&as, params);
>> -    stream->info.direction = stream_id < s->snd_conf.streams / 2 +
>> -        (s->snd_conf.streams & 1) ? VIRTIO_SND_D_OUTPUT : 
>> VIRTIO_SND_D_INPUT;
>> -    stream->info.hdr.hda_fn_nid = VIRTIO_SOUND_HDA_FN_NID;
>> -    stream->info.features = 0;
>> -    stream->info.channels_min = 1;
>> -    stream->info.channels_max = as.nchannels;
>> -    stream->info.formats = supported_formats;
>> -    stream->info.rates = supported_rates;
>>      stream->params = *params;
>>
>>      stream->positions[0] = VIRTIO_SND_CHMAP_FL;
>> @@ -1074,6 +1064,24 @@ static void virtio_snd_realize(DeviceState *dev, 
>> Error **errp)
>>      vsnd->vmstate =
>>          qemu_add_vm_change_state_handler(virtio_snd_vm_state_change, vsnd);
>>
>> +    vsnd->streams = g_new0(VirtIOSoundPCMStream, vsnd->snd_conf.streams);
>> +
>> +    for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
>> +        VirtIOSoundPCMStream *stream = &vsnd->streams[i];
>> +
>> +        stream->id = i;
>> +        stream->s = vsnd;
>> +        stream->info.hdr.hda_fn_nid = VIRTIO_SOUND_HDA_FN_NID;
>> +        stream->info.features = 0;
>> +        stream->info.formats = supported_formats;
>> +        stream->info.rates = supported_rates;
>> +        stream->info.direction =
>> +            i < vsnd->snd_conf.streams / 2 + (vsnd->snd_conf.streams & 1)
>> +            ? VIRTIO_SND_D_OUTPUT : VIRTIO_SND_D_INPUT;
>> +        stream->info.channels_min = 1;
>> +        stream->info.channels_max = 2;
> Fixed max channels set to 2.. ? before this was set to
> MIN(AUDIO_MAX_CHANNELS, params->channels)

Before my patch params->channels and stream->info.max_channels were also
2 at the time the guest driver queried stream info. They got initialized
in function virtio_snd_realize().

default_params.channels = 2;
...
status = virtio_snd_set_pcm_params(vsnd, i, &default_params);
...
status = virtio_snd_pcm_prepare(vsnd, i);

The guest may not update stream->info variables. They are configuration
information set when QEMU starts. This was wrong in
virtio_snd_pcm_prepare().

>
>> +    }
>> +
>>      vsnd->pcm = g_new0(VirtIOSoundPCM, 1);
>>      vsnd->pcm->snd = vsnd;
>>      vsnd->pcm->streams =
>> @@ -1314,14 +1322,13 @@ static void virtio_snd_unrealize(DeviceState *dev)
>>      qemu_del_vm_change_state_handler(vsnd->vmstate);
>>      trace_virtio_snd_unrealize(vsnd);
>>
>> -    if (vsnd->pcm) {
>> +    if (vsnd->streams) {
>>          if (vsnd->pcm->streams) {
>>              for (uint32_t i = 0; i < vsnd->snd_conf.streams; i++) {
>>                  stream = vsnd->pcm->streams[i];
>>                  if (stream) {
>>                      virtio_snd_process_cmdq(stream->s);
>>                      virtio_snd_pcm_close(stream);
>> -                    g_free(stream);
>>                  }
>>              }
>>              g_free(vsnd->pcm->streams);
>> @@ -1329,6 +1336,8 @@ static void virtio_snd_unrealize(DeviceState *dev)
>>          g_free(vsnd->pcm->pcm_params);
>>          g_free(vsnd->pcm);
>>          vsnd->pcm = NULL;
>> +        g_free(vsnd->streams);
>> +        vsnd->streams = NULL;
>>      }
>>      AUD_remove_card(&vsnd->card);
>>      virtio_delete_queue(vsnd->queues[VIRTIO_SND_VQ_CONTROL]);
>> diff --git a/include/hw/audio/virtio-snd.h b/include/hw/audio/virtio-snd.h
>> index ea6315f59b..05b4490488 100644
>> --- a/include/hw/audio/virtio-snd.h
>> +++ b/include/hw/audio/virtio-snd.h
>> @@ -216,6 +216,7 @@ struct VirtIOSound {
>>      VirtQueue *queues[VIRTIO_SND_VQ_MAX];
>>      uint64_t features;
>>      VirtIOSoundPCM *pcm;
>> +    VirtIOSoundPCMStream *streams;
>>      QEMUSoundCard card;
>>      VMChangeStateEntry *vmstate;
>>      virtio_snd_config snd_conf;
>> --
>> 2.35.3
>>