| Message ID | 20240104203422.12308-2-vr_qemu@t-online.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-sound migration part 1 | expand |
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... > > @@ -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) > + } > + > 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 >
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 >>
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); @@ -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; + } + 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;
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(-)