[PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Philippe Mathieu-Daudé]

Since VirtIO devices can change endianness at runtime,
we need to use the device endianness, not the target
one.

Cc: qemu-stable@nongnu.org
Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
v2: Use virtio_is_big_endian()
v3: Remove "hw/core/cpu.h
---
 hw/audio/virtio-snd.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
index c80b58bf5d..939cd78026 100644
--- a/hw/audio/virtio-snd.c
+++ b/hw/audio/virtio-snd.c
@@ -24,7 +24,6 @@
 #include "trace.h"
 #include "qapi/error.h"
 #include "hw/audio/virtio-snd.h"
-#include "hw/core/cpu.h"
 
 #define VIRTIO_SOUND_VM_VERSION 1
 #define VIRTIO_SOUND_JACK_DEFAULT 0
@@ -395,13 +394,15 @@ static uint32_t virtio_snd_get_qemu_freq(uint32_t rate)
  * Get QEMU Audiosystem compatible audsettings from virtio based pcm stream
  * params.
  */
-static void virtio_snd_get_qemu_audsettings(audsettings *as,
+static void virtio_snd_get_qemu_audsettings(VirtIOSound *s, audsettings *as,
                                             virtio_snd_pcm_set_params *params)
 {
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
     as->nchannels = MIN(AUDIO_MAX_CHANNELS, params->channels);
     as->fmt = virtio_snd_get_qemu_format(params->format);
     as->freq = virtio_snd_get_qemu_freq(params->rate);
-    as->endianness = target_words_bigendian() ? 1 : 0;
+    as->endianness = virtio_is_big_endian(vdev) ? 1 : 0;
 }
 
 /*
@@ -464,7 +465,7 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, uint32_t stream_id)
         s->pcm->streams[stream_id] = stream;
     }
 
-    virtio_snd_get_qemu_audsettings(&as, params);
+    virtio_snd_get_qemu_audsettings(s, &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;
-- 
2.41.0

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Manos Pitsidianakis]

On Mon, 22 Apr 2024 17:20, Philippe Mathieu-Daudé <philmd@linaro.org> 
wrote:
>Since VirtIO devices can change endianness at runtime,
>we need to use the device endianness, not the target
>one.
>
>Cc: qemu-stable@nongnu.org
>Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
>Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>---

Reviewed-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>

Thanks for the explanation on v2 btw. virtio_is_big_endian()'s doc 
should probably reflect it's not just about legacy devices (virtio sound 
isn't legacy) but about target originating data streams too

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Michael S. Tsirkin]

On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
> Since VirtIO devices can change endianness at runtime,
> we need to use the device endianness, not the target
> one.
> 
> Cc: qemu-stable@nongnu.org
> Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>



This is all completely bogus. Virtio SND is from Virtio 1.0 only.
It is unconditionally little endian.
If it's not it's a guest bug pls just fix it there.


> ---
> v2: Use virtio_is_big_endian()
> v3: Remove "hw/core/cpu.h
> ---
>  hw/audio/virtio-snd.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
> index c80b58bf5d..939cd78026 100644
> --- a/hw/audio/virtio-snd.c
> +++ b/hw/audio/virtio-snd.c
> @@ -24,7 +24,6 @@
>  #include "trace.h"
>  #include "qapi/error.h"
>  #include "hw/audio/virtio-snd.h"
> -#include "hw/core/cpu.h"
>  
>  #define VIRTIO_SOUND_VM_VERSION 1
>  #define VIRTIO_SOUND_JACK_DEFAULT 0
> @@ -395,13 +394,15 @@ static uint32_t virtio_snd_get_qemu_freq(uint32_t rate)
>   * Get QEMU Audiosystem compatible audsettings from virtio based pcm stream
>   * params.
>   */
> -static void virtio_snd_get_qemu_audsettings(audsettings *as,
> +static void virtio_snd_get_qemu_audsettings(VirtIOSound *s, audsettings *as,
>                                              virtio_snd_pcm_set_params *params)
>  {
> +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> +
>      as->nchannels = MIN(AUDIO_MAX_CHANNELS, params->channels);
>      as->fmt = virtio_snd_get_qemu_format(params->format);
>      as->freq = virtio_snd_get_qemu_freq(params->rate);
> -    as->endianness = target_words_bigendian() ? 1 : 0;
> +    as->endianness = virtio_is_big_endian(vdev) ? 1 : 0;
>  }
>  
>  /*
> @@ -464,7 +465,7 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, uint32_t stream_id)
>          s->pcm->streams[stream_id] = stream;
>      }
>  
> -    virtio_snd_get_qemu_audsettings(&as, params);
> +    virtio_snd_get_qemu_audsettings(s, &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;
> -- 
> 2.41.0

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Philippe Mathieu-Daudé]

On 22/4/24 23:02, Michael S. Tsirkin wrote:
> On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
>> Since VirtIO devices can change endianness at runtime,
>> we need to use the device endianness, not the target
>> one.
>>
>> Cc: qemu-stable@nongnu.org
>> Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> 
> 
> This is all completely bogus. Virtio SND is from Virtio 1.0 only.
> It is unconditionally little endian.

Oh, then the fix is as simple as:

  -    as->endianness = target_words_bigendian() ? 1 : 0;
  +    as->endianness = 0; /* VIRTIO 1.0: always LE. */

> If it's not it's a guest bug pls just fix it there.
> 
> 
>> ---
>> v2: Use virtio_is_big_endian()
>> v3: Remove "hw/core/cpu.h
>> ---
>>   hw/audio/virtio-snd.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
>> index c80b58bf5d..939cd78026 100644
>> --- a/hw/audio/virtio-snd.c
>> +++ b/hw/audio/virtio-snd.c
>> @@ -24,7 +24,6 @@
>>   #include "trace.h"
>>   #include "qapi/error.h"
>>   #include "hw/audio/virtio-snd.h"
>> -#include "hw/core/cpu.h"
>>   
>>   #define VIRTIO_SOUND_VM_VERSION 1
>>   #define VIRTIO_SOUND_JACK_DEFAULT 0
>> @@ -395,13 +394,15 @@ static uint32_t virtio_snd_get_qemu_freq(uint32_t rate)
>>    * Get QEMU Audiosystem compatible audsettings from virtio based pcm stream
>>    * params.
>>    */
>> -static void virtio_snd_get_qemu_audsettings(audsettings *as,
>> +static void virtio_snd_get_qemu_audsettings(VirtIOSound *s, audsettings *as,
>>                                               virtio_snd_pcm_set_params *params)
>>   {
>> +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
>> +
>>       as->nchannels = MIN(AUDIO_MAX_CHANNELS, params->channels);
>>       as->fmt = virtio_snd_get_qemu_format(params->format);
>>       as->freq = virtio_snd_get_qemu_freq(params->rate);
>> -    as->endianness = target_words_bigendian() ? 1 : 0;
>> +    as->endianness = virtio_is_big_endian(vdev) ? 1 : 0;
>>   }
>>   
>>   /*
>> @@ -464,7 +465,7 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, uint32_t stream_id)
>>           s->pcm->streams[stream_id] = stream;
>>       }
>>   
>> -    virtio_snd_get_qemu_audsettings(&as, params);
>> +    virtio_snd_get_qemu_audsettings(s, &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;
>> -- 
>> 2.41.0
> 

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Michael S. Tsirkin]

On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
> On 22/4/24 23:02, Michael S. Tsirkin wrote:
> > On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
> > > Since VirtIO devices can change endianness at runtime,
> > > we need to use the device endianness, not the target
> > > one.
> > > 
> > > Cc: qemu-stable@nongnu.org
> > > Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > 
> > 
> > 
> > This is all completely bogus. Virtio SND is from Virtio 1.0 only.
> > It is unconditionally little endian.
> 
> Oh, then the fix is as simple as:
> 
>  -    as->endianness = target_words_bigendian() ? 1 : 0;
>  +    as->endianness = 0; /* VIRTIO 1.0: always LE. */

Makes sense. Pls clarify in commit log whether the incorrect
value leads to any failures or this was found by code review.

> > If it's not it's a guest bug pls just fix it there.
> > 
> > 
> > > ---
> > > v2: Use virtio_is_big_endian()
> > > v3: Remove "hw/core/cpu.h
> > > ---
> > >   hw/audio/virtio-snd.c | 9 +++++----
> > >   1 file changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/audio/virtio-snd.c b/hw/audio/virtio-snd.c
> > > index c80b58bf5d..939cd78026 100644
> > > --- a/hw/audio/virtio-snd.c
> > > +++ b/hw/audio/virtio-snd.c
> > > @@ -24,7 +24,6 @@
> > >   #include "trace.h"
> > >   #include "qapi/error.h"
> > >   #include "hw/audio/virtio-snd.h"
> > > -#include "hw/core/cpu.h"
> > >   #define VIRTIO_SOUND_VM_VERSION 1
> > >   #define VIRTIO_SOUND_JACK_DEFAULT 0
> > > @@ -395,13 +394,15 @@ static uint32_t virtio_snd_get_qemu_freq(uint32_t rate)
> > >    * Get QEMU Audiosystem compatible audsettings from virtio based pcm stream
> > >    * params.
> > >    */
> > > -static void virtio_snd_get_qemu_audsettings(audsettings *as,
> > > +static void virtio_snd_get_qemu_audsettings(VirtIOSound *s, audsettings *as,
> > >                                               virtio_snd_pcm_set_params *params)
> > >   {
> > > +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> > > +
> > >       as->nchannels = MIN(AUDIO_MAX_CHANNELS, params->channels);
> > >       as->fmt = virtio_snd_get_qemu_format(params->format);
> > >       as->freq = virtio_snd_get_qemu_freq(params->rate);
> > > -    as->endianness = target_words_bigendian() ? 1 : 0;
> > > +    as->endianness = virtio_is_big_endian(vdev) ? 1 : 0;
> > >   }
> > >   /*
> > > @@ -464,7 +465,7 @@ static uint32_t virtio_snd_pcm_prepare(VirtIOSound *s, uint32_t stream_id)
> > >           s->pcm->streams[stream_id] = stream;
> > >       }
> > > -    virtio_snd_get_qemu_audsettings(&as, params);
> > > +    virtio_snd_get_qemu_audsettings(s, &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;
> > > -- 
> > > 2.41.0
> > 

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Manos Pitsidianakis]

On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
> > On 22/4/24 23:02, Michael S. Tsirkin wrote:
> > > On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
> > > > Since VirtIO devices can change endianness at runtime,
> > > > we need to use the device endianness, not the target
> > > > one.
> > > >
> > > > Cc: qemu-stable@nongnu.org
> > > > Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
> > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > >
> > >
> > >
> > > This is all completely bogus. Virtio SND is from Virtio 1.0 only.
> > > It is unconditionally little endian.


This part of the code is for PCM frames (raw bytes), not virtio spec
fields (which indeed must be LE in modern VIRTIO).

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Manos Pitsidianakis]

On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
> > > On 22/4/24 23:02, Michael S. Tsirkin wrote:
> > > > On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
> > > > > Since VirtIO devices can change endianness at runtime,
> > > > > we need to use the device endianness, not the target
> > > > > one.
> > > > >
> > > > > Cc: qemu-stable@nongnu.org
> > > > > Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
> > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > >
> > > >
> > > >
> > > > This is all completely bogus. Virtio SND is from Virtio 1.0 only.
> > > > It is unconditionally little endian.
>
>
> This part of the code is for PCM frames (raw bytes), not virtio spec
> fields (which indeed must be LE in modern VIRTIO).

Thought a little more about it. We should keep the target's endianness
here, if it's mutable then we should query the machine the device is
attached to somehow. the virtio device should never change endianness
like Michael says since it's not legacy.

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Philippe Mathieu-Daudé]

On 23/4/24 11:18, Manos Pitsidianakis wrote:
> On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
> <manos.pitsidianakis@linaro.org> wrote:
>>
>> On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>
>>> On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
>>>> On 22/4/24 23:02, Michael S. Tsirkin wrote:
>>>>> On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
>>>>>> Since VirtIO devices can change endianness at runtime,
>>>>>> we need to use the device endianness, not the target
>>>>>> one.
>>>>>>
>>>>>> Cc: qemu-stable@nongnu.org
>>>>>> Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>
>>>>>
>>>>>
>>>>> This is all completely bogus. Virtio SND is from Virtio 1.0 only.
>>>>> It is unconditionally little endian.
>>
>>
>> This part of the code is for PCM frames (raw bytes), not virtio spec
>> fields (which indeed must be LE in modern VIRTIO).
> 
> Thought a little more about it. We should keep the target's endianness
> here, if it's mutable then we should query the machine the device is
> attached to somehow. the virtio device should never change endianness
> like Michael says since it's not legacy.

Grr. So as Richard suggested, this need to be pass as a device
property then.
(https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09876@linaro.org/)

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Mark Cave-Ayland]

On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:

> On 23/4/24 11:18, Manos Pitsidianakis wrote:
>> On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
>> <manos.pitsidianakis@linaro.org> wrote:
>>>
>>> On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>
>>>> On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
>>>>> On 22/4/24 23:02, Michael S. Tsirkin wrote:
>>>>>> On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
>>>>>>> Since VirtIO devices can change endianness at runtime,
>>>>>>> we need to use the device endianness, not the target
>>>>>>> one.
>>>>>>>
>>>>>>> Cc: qemu-stable@nongnu.org
>>>>>>> Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>
>>>>>>
>>>>>>
>>>>>> This is all completely bogus. Virtio SND is from Virtio 1.0 only.
>>>>>> It is unconditionally little endian.
>>>
>>>
>>> This part of the code is for PCM frames (raw bytes), not virtio spec
>>> fields (which indeed must be LE in modern VIRTIO).
>>
>> Thought a little more about it. We should keep the target's endianness
>> here, if it's mutable then we should query the machine the device is
>> attached to somehow. the virtio device should never change endianness
>> like Michael says since it's not legacy.
> 
> Grr. So as Richard suggested, this need to be pass as a device
> property then.
> (https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09876@linaro.org/)

It feels to me that the endianness is something that should be negotiated as part of 
the frame format, since the endianness of the audio hardware can be different from 
that of the CPU (think PReP machines where it was common that a big endian CPU is 
driving little endian hardware as found on x86).

My feeling is that either the VIRTIO_SND_PCM_FMT_* constants should be extended to 
have both _LE and _BE variants, or all frame formats are defined to always be little 
endian.


ATB,

Mark.

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Manos Pitsidianakis]

On Wed, 24 Apr 2024 at 13:31, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:
>
> > On 23/4/24 11:18, Manos Pitsidianakis wrote:
> >> On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
> >> <manos.pitsidianakis@linaro.org> wrote:
> >>>
> >>> On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>
> >>>> On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
> >>>>> On 22/4/24 23:02, Michael S. Tsirkin wrote:
> >>>>>> On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
> >>>>>>> Since VirtIO devices can change endianness at runtime,
> >>>>>>> we need to use the device endianness, not the target
> >>>>>>> one.
> >>>>>>>
> >>>>>>> Cc: qemu-stable@nongnu.org
> >>>>>>> Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
> >>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> This is all completely bogus. Virtio SND is from Virtio 1.0 only.
> >>>>>> It is unconditionally little endian.
> >>>
> >>>
> >>> This part of the code is for PCM frames (raw bytes), not virtio spec
> >>> fields (which indeed must be LE in modern VIRTIO).
> >>
> >> Thought a little more about it. We should keep the target's endianness
> >> here, if it's mutable then we should query the machine the device is
> >> attached to somehow. the virtio device should never change endianness
> >> like Michael says since it's not legacy.
> >
> > Grr. So as Richard suggested, this need to be pass as a device
> > property then.
> > (https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09876@linaro.org/)
>
> It feels to me that the endianness is something that should be negotiated as part of
> the frame format, since the endianness of the audio hardware can be different from
> that of the CPU (think PReP machines where it was common that a big endian CPU is
> driving little endian hardware as found on x86).

But that is the job of the hardware drivers, isn't it? Here we are
taking frames passed from the guest to its virtio driver in the format
specified in the target cpu's endianness and QEMU as the device passes
it to host ALSA/Pipewire/etc which in turn passes it to the actual
audio hardware driver..



> My feeling is that either the VIRTIO_SND_PCM_FMT_* constants should be extended to
> have both _LE and _BE variants, or all frame formats are defined to always be little
> endian.
>
>
> ATB,
>
> Mark.
>

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Mark Cave-Ayland]

On 25/04/2024 07:30, Manos Pitsidianakis wrote:

> On Wed, 24 Apr 2024 at 13:31, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:
>>
>>> On 23/4/24 11:18, Manos Pitsidianakis wrote:
>>>> On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
>>>> <manos.pitsidianakis@linaro.org> wrote:
>>>>>
>>>>> On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>
>>>>>> On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
>>>>>>> On 22/4/24 23:02, Michael S. Tsirkin wrote:
>>>>>>>> On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
>>>>>>>>> Since VirtIO devices can change endianness at runtime,
>>>>>>>>> we need to use the device endianness, not the target
>>>>>>>>> one.
>>>>>>>>>
>>>>>>>>> Cc: qemu-stable@nongnu.org
>>>>>>>>> Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
>>>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> This is all completely bogus. Virtio SND is from Virtio 1.0 only.
>>>>>>>> It is unconditionally little endian.
>>>>>
>>>>>
>>>>> This part of the code is for PCM frames (raw bytes), not virtio spec
>>>>> fields (which indeed must be LE in modern VIRTIO).
>>>>
>>>> Thought a little more about it. We should keep the target's endianness
>>>> here, if it's mutable then we should query the machine the device is
>>>> attached to somehow. the virtio device should never change endianness
>>>> like Michael says since it's not legacy.
>>>
>>> Grr. So as Richard suggested, this need to be pass as a device
>>> property then.
>>> (https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09876@linaro.org/)
>>
>> It feels to me that the endianness is something that should be negotiated as part of
>> the frame format, since the endianness of the audio hardware can be different from
>> that of the CPU (think PReP machines where it was common that a big endian CPU is
>> driving little endian hardware as found on x86).
> 
> But that is the job of the hardware drivers, isn't it? Here we are
> taking frames passed from the guest to its virtio driver in the format
> specified in the target cpu's endianness and QEMU as the device passes
> it to host ALSA/Pipewire/etc which in turn passes it to the actual
> audio hardware driver..

The problem is that the notion of target CPU endian is not fixed. For example the 
PowerPC CPU starts off in big-endian mode, but these days most systems will switch 
the CPU to little-endian mode on startup to run ppc64le. There's also the ILE bit 
which can be configured so that a big-endian PowerPC CPU can dynamically switch to 
little-endian mode when processing an interrupt, so you could potentially end up with 
either depending upon the current mode of the CPU.

These are the kinds of issues that led to the later virtio specifications simply 
using little-endian for everything, since then there is zero ambiguity over what 
endian is required for the virtio configuration space accesses.

It feels to me that assuming a target CPU endian is fixed for the PCM frame formats 
is simply repeating the mistakes of the past - and even the fact that we are 
discussing this within this thread suggests that at a very minimum the virtio-snd 
specification needs to be updated to clarify the byte ordering of the PCM frame formats.


ATB,

Mark.

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Manos Pitsidianakis]

On Thu, 25 Apr 2024 at 10:49, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 25/04/2024 07:30, Manos Pitsidianakis wrote:
>
> > On Wed, 24 Apr 2024 at 13:31, Mark Cave-Ayland
> > <mark.cave-ayland@ilande.co.uk> wrote:
> >>
> >> On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:
> >>
> >>> On 23/4/24 11:18, Manos Pitsidianakis wrote:
> >>>> On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
> >>>> <manos.pitsidianakis@linaro.org> wrote:
> >>>>>
> >>>>> On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>>>
> >>>>>> On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
> >>>>>>> On 22/4/24 23:02, Michael S. Tsirkin wrote:
> >>>>>>>> On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
> >>>>>>>>> Since VirtIO devices can change endianness at runtime,
> >>>>>>>>> we need to use the device endianness, not the target
> >>>>>>>>> one.
> >>>>>>>>>
> >>>>>>>>> Cc: qemu-stable@nongnu.org
> >>>>>>>>> Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
> >>>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> This is all completely bogus. Virtio SND is from Virtio 1.0 only.
> >>>>>>>> It is unconditionally little endian.
> >>>>>
> >>>>>
> >>>>> This part of the code is for PCM frames (raw bytes), not virtio spec
> >>>>> fields (which indeed must be LE in modern VIRTIO).
> >>>>
> >>>> Thought a little more about it. We should keep the target's endianness
> >>>> here, if it's mutable then we should query the machine the device is
> >>>> attached to somehow. the virtio device should never change endianness
> >>>> like Michael says since it's not legacy.
> >>>
> >>> Grr. So as Richard suggested, this need to be pass as a device
> >>> property then.
> >>> (https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09876@linaro.org/)
> >>
> >> It feels to me that the endianness is something that should be negotiated as part of
> >> the frame format, since the endianness of the audio hardware can be different from
> >> that of the CPU (think PReP machines where it was common that a big endian CPU is
> >> driving little endian hardware as found on x86).
> >
> > But that is the job of the hardware drivers, isn't it? Here we are
> > taking frames passed from the guest to its virtio driver in the format
> > specified in the target cpu's endianness and QEMU as the device passes
> > it to host ALSA/Pipewire/etc which in turn passes it to the actual
> > audio hardware driver..
>
> The problem is that the notion of target CPU endian is not fixed. For example the
> PowerPC CPU starts off in big-endian mode, but these days most systems will switch
> the CPU to little-endian mode on startup to run ppc64le. There's also the ILE bit
> which can be configured so that a big-endian PowerPC CPU can dynamically switch to
> little-endian mode when processing an interrupt, so you could potentially end up with
> either depending upon the current mode of the CPU.
>
> These are the kinds of issues that led to the later virtio specifications simply
> using little-endian for everything, since then there is zero ambiguity over what
> endian is required for the virtio configuration space accesses.
>
> It feels to me that assuming a target CPU endian is fixed for the PCM frame formats
> is simply repeating the mistakes of the past - and even the fact that we are
> discussing this within this thread suggests that at a very minimum the virtio-snd
> specification needs to be updated to clarify the byte ordering of the PCM frame formats.
>
>
> ATB,
>
> Mark.
>


Agreed, I think we are saying approximately the same thing here.

 We need a mechanism to retrieve the vCPUs endianness and a way to
notify subscribed devices when it changes.

 I think that then, since the virtio device is mostly certain of the
correct target endianness and completely certain of the host
endianness, it can perform the necessary conversions.

 I don't recall seeing a restriction on the byte ordering of PCM
formats other than the CPU order; except for the ones which have
explicit endianness in their definitions. Please correct me if I am
wrong!

A straightforward solution would be to set an endianness change notify
callback in DeviceClass. What do you think?

-- 
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Michael S. Tsirkin]

On Thu, Apr 25, 2024 at 01:04:31PM +0300, Manos Pitsidianakis wrote:
> On Thu, 25 Apr 2024 at 10:49, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
> >
> > On 25/04/2024 07:30, Manos Pitsidianakis wrote:
> >
> > > On Wed, 24 Apr 2024 at 13:31, Mark Cave-Ayland
> > > <mark.cave-ayland@ilande.co.uk> wrote:
> > >>
> > >> On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:
> > >>
> > >>> On 23/4/24 11:18, Manos Pitsidianakis wrote:
> > >>>> On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
> > >>>> <manos.pitsidianakis@linaro.org> wrote:
> > >>>>>
> > >>>>> On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >>>>>>
> > >>>>>> On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
> > >>>>>>> On 22/4/24 23:02, Michael S. Tsirkin wrote:
> > >>>>>>>> On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
> > >>>>>>>>> Since VirtIO devices can change endianness at runtime,
> > >>>>>>>>> we need to use the device endianness, not the target
> > >>>>>>>>> one.
> > >>>>>>>>>
> > >>>>>>>>> Cc: qemu-stable@nongnu.org
> > >>>>>>>>> Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
> > >>>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> This is all completely bogus. Virtio SND is from Virtio 1.0 only.
> > >>>>>>>> It is unconditionally little endian.
> > >>>>>
> > >>>>>
> > >>>>> This part of the code is for PCM frames (raw bytes), not virtio spec
> > >>>>> fields (which indeed must be LE in modern VIRTIO).
> > >>>>
> > >>>> Thought a little more about it. We should keep the target's endianness
> > >>>> here, if it's mutable then we should query the machine the device is
> > >>>> attached to somehow. the virtio device should never change endianness
> > >>>> like Michael says since it's not legacy.
> > >>>
> > >>> Grr. So as Richard suggested, this need to be pass as a device
> > >>> property then.
> > >>> (https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09876@linaro.org/)
> > >>
> > >> It feels to me that the endianness is something that should be negotiated as part of
> > >> the frame format, since the endianness of the audio hardware can be different from
> > >> that of the CPU (think PReP machines where it was common that a big endian CPU is
> > >> driving little endian hardware as found on x86).
> > >
> > > But that is the job of the hardware drivers, isn't it? Here we are
> > > taking frames passed from the guest to its virtio driver in the format
> > > specified in the target cpu's endianness and QEMU as the device passes
> > > it to host ALSA/Pipewire/etc which in turn passes it to the actual
> > > audio hardware driver..
> >
> > The problem is that the notion of target CPU endian is not fixed. For example the
> > PowerPC CPU starts off in big-endian mode, but these days most systems will switch
> > the CPU to little-endian mode on startup to run ppc64le. There's also the ILE bit
> > which can be configured so that a big-endian PowerPC CPU can dynamically switch to
> > little-endian mode when processing an interrupt, so you could potentially end up with
> > either depending upon the current mode of the CPU.
> >
> > These are the kinds of issues that led to the later virtio specifications simply
> > using little-endian for everything, since then there is zero ambiguity over what
> > endian is required for the virtio configuration space accesses.
> >
> > It feels to me that assuming a target CPU endian is fixed for the PCM frame formats
> > is simply repeating the mistakes of the past - and even the fact that we are
> > discussing this within this thread suggests that at a very minimum the virtio-snd
> > specification needs to be updated to clarify the byte ordering of the PCM frame formats.
> >
> >
> > ATB,
> >
> > Mark.
> >
> 
> 
> Agreed, I think we are saying approximately the same thing here.
> 
>  We need a mechanism to retrieve the vCPUs endianness and a way to
> notify subscribed devices when it changes.

I don't think I agree, it's not the same thing.
Guest should just convert and send data in LE format.
Host should then convert from LE format.
Target endian-ness does not come into it.



>  I think that then, since the virtio device is mostly certain of the
> correct target endianness and completely certain of the host
> endianness, it can perform the necessary conversions.
> 
>  I don't recall seeing a restriction on the byte ordering of PCM
> formats other than the CPU order; except for the ones which have
> explicit endianness in their definitions. Please correct me if I am
> wrong!
> 
> A straightforward solution would be to set an endianness change notify
> callback in DeviceClass. What do you think?
> 
> -- 
> Manos Pitsidianakis
> Emulation and Virtualization Engineer at Linaro Ltd

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Manos Pitsidianakis]

On Thu, 25 Apr 2024 at 13:24, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Apr 25, 2024 at 01:04:31PM +0300, Manos Pitsidianakis wrote:
> > On Thu, 25 Apr 2024 at 10:49, Mark Cave-Ayland
> > <mark.cave-ayland@ilande.co.uk> wrote:
> > >
> > > On 25/04/2024 07:30, Manos Pitsidianakis wrote:
> > >
> > > > On Wed, 24 Apr 2024 at 13:31, Mark Cave-Ayland
> > > > <mark.cave-ayland@ilande.co.uk> wrote:
> > > >>
> > > >> On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:
> > > >>
> > > >>> On 23/4/24 11:18, Manos Pitsidianakis wrote:
> > > >>>> On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
> > > >>>> <manos.pitsidianakis@linaro.org> wrote:
> > > >>>>>
> > > >>>>> On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >>>>>>
> > > >>>>>> On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
> > > >>>>>>> On 22/4/24 23:02, Michael S. Tsirkin wrote:
> > > >>>>>>>> On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
> > > >>>>>>>>> Since VirtIO devices can change endianness at runtime,
> > > >>>>>>>>> we need to use the device endianness, not the target
> > > >>>>>>>>> one.
> > > >>>>>>>>>
> > > >>>>>>>>> Cc: qemu-stable@nongnu.org
> > > >>>>>>>>> Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
> > > >>>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>>
> > > >>>>>>>> This is all completely bogus. Virtio SND is from Virtio 1.0 only.
> > > >>>>>>>> It is unconditionally little endian.
> > > >>>>>
> > > >>>>>
> > > >>>>> This part of the code is for PCM frames (raw bytes), not virtio spec
> > > >>>>> fields (which indeed must be LE in modern VIRTIO).
> > > >>>>
> > > >>>> Thought a little more about it. We should keep the target's endianness
> > > >>>> here, if it's mutable then we should query the machine the device is
> > > >>>> attached to somehow. the virtio device should never change endianness
> > > >>>> like Michael says since it's not legacy.
> > > >>>
> > > >>> Grr. So as Richard suggested, this need to be pass as a device
> > > >>> property then.
> > > >>> (https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09876@linaro.org/)
> > > >>
> > > >> It feels to me that the endianness is something that should be negotiated as part of
> > > >> the frame format, since the endianness of the audio hardware can be different from
> > > >> that of the CPU (think PReP machines where it was common that a big endian CPU is
> > > >> driving little endian hardware as found on x86).
> > > >
> > > > But that is the job of the hardware drivers, isn't it? Here we are
> > > > taking frames passed from the guest to its virtio driver in the format
> > > > specified in the target cpu's endianness and QEMU as the device passes
> > > > it to host ALSA/Pipewire/etc which in turn passes it to the actual
> > > > audio hardware driver..
> > >
> > > The problem is that the notion of target CPU endian is not fixed. For example the
> > > PowerPC CPU starts off in big-endian mode, but these days most systems will switch
> > > the CPU to little-endian mode on startup to run ppc64le. There's also the ILE bit
> > > which can be configured so that a big-endian PowerPC CPU can dynamically switch to
> > > little-endian mode when processing an interrupt, so you could potentially end up with
> > > either depending upon the current mode of the CPU.
> > >
> > > These are the kinds of issues that led to the later virtio specifications simply
> > > using little-endian for everything, since then there is zero ambiguity over what
> > > endian is required for the virtio configuration space accesses.
> > >
> > > It feels to me that assuming a target CPU endian is fixed for the PCM frame formats
> > > is simply repeating the mistakes of the past - and even the fact that we are
> > > discussing this within this thread suggests that at a very minimum the virtio-snd
> > > specification needs to be updated to clarify the byte ordering of the PCM frame formats.
> > >
> > >
> > > ATB,
> > >
> > > Mark.
> > >
> >
> >
> > Agreed, I think we are saying approximately the same thing here.
> >
> >  We need a mechanism to retrieve the vCPUs endianness and a way to
> > notify subscribed devices when it changes.
>
> I don't think I agree, it's not the same thing.
> Guest should just convert and send data in LE format.
> Host should then convert from LE format.
> Target endian-ness does not come into it.

That's not in the VIRTIO 1.2 spec. We are talking about supporting
things as they currently stand, not as they could have been.

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Michael S. Tsirkin]

On Thu, Apr 25, 2024 at 01:26:38PM +0300, Manos Pitsidianakis wrote:
> On Thu, 25 Apr 2024 at 13:24, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Apr 25, 2024 at 01:04:31PM +0300, Manos Pitsidianakis wrote:
> > > On Thu, 25 Apr 2024 at 10:49, Mark Cave-Ayland
> > > <mark.cave-ayland@ilande.co.uk> wrote:
> > > >
> > > > On 25/04/2024 07:30, Manos Pitsidianakis wrote:
> > > >
> > > > > On Wed, 24 Apr 2024 at 13:31, Mark Cave-Ayland
> > > > > <mark.cave-ayland@ilande.co.uk> wrote:
> > > > >>
> > > > >> On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:
> > > > >>
> > > > >>> On 23/4/24 11:18, Manos Pitsidianakis wrote:
> > > > >>>> On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
> > > > >>>> <manos.pitsidianakis@linaro.org> wrote:
> > > > >>>>>
> > > > >>>>> On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >>>>>>
> > > > >>>>>> On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
> > > > >>>>>>> On 22/4/24 23:02, Michael S. Tsirkin wrote:
> > > > >>>>>>>> On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
> > > > >>>>>>>>> Since VirtIO devices can change endianness at runtime,
> > > > >>>>>>>>> we need to use the device endianness, not the target
> > > > >>>>>>>>> one.
> > > > >>>>>>>>>
> > > > >>>>>>>>> Cc: qemu-stable@nongnu.org
> > > > >>>>>>>>> Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
> > > > >>>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > >>>>>>>>
> > > > >>>>>>>>
> > > > >>>>>>>>
> > > > >>>>>>>> This is all completely bogus. Virtio SND is from Virtio 1.0 only.
> > > > >>>>>>>> It is unconditionally little endian.
> > > > >>>>>
> > > > >>>>>
> > > > >>>>> This part of the code is for PCM frames (raw bytes), not virtio spec
> > > > >>>>> fields (which indeed must be LE in modern VIRTIO).
> > > > >>>>
> > > > >>>> Thought a little more about it. We should keep the target's endianness
> > > > >>>> here, if it's mutable then we should query the machine the device is
> > > > >>>> attached to somehow. the virtio device should never change endianness
> > > > >>>> like Michael says since it's not legacy.
> > > > >>>
> > > > >>> Grr. So as Richard suggested, this need to be pass as a device
> > > > >>> property then.
> > > > >>> (https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09876@linaro.org/)
> > > > >>
> > > > >> It feels to me that the endianness is something that should be negotiated as part of
> > > > >> the frame format, since the endianness of the audio hardware can be different from
> > > > >> that of the CPU (think PReP machines where it was common that a big endian CPU is
> > > > >> driving little endian hardware as found on x86).
> > > > >
> > > > > But that is the job of the hardware drivers, isn't it? Here we are
> > > > > taking frames passed from the guest to its virtio driver in the format
> > > > > specified in the target cpu's endianness and QEMU as the device passes
> > > > > it to host ALSA/Pipewire/etc which in turn passes it to the actual
> > > > > audio hardware driver..
> > > >
> > > > The problem is that the notion of target CPU endian is not fixed. For example the
> > > > PowerPC CPU starts off in big-endian mode, but these days most systems will switch
> > > > the CPU to little-endian mode on startup to run ppc64le. There's also the ILE bit
> > > > which can be configured so that a big-endian PowerPC CPU can dynamically switch to
> > > > little-endian mode when processing an interrupt, so you could potentially end up with
> > > > either depending upon the current mode of the CPU.
> > > >
> > > > These are the kinds of issues that led to the later virtio specifications simply
> > > > using little-endian for everything, since then there is zero ambiguity over what
> > > > endian is required for the virtio configuration space accesses.
> > > >
> > > > It feels to me that assuming a target CPU endian is fixed for the PCM frame formats
> > > > is simply repeating the mistakes of the past - and even the fact that we are
> > > > discussing this within this thread suggests that at a very minimum the virtio-snd
> > > > specification needs to be updated to clarify the byte ordering of the PCM frame formats.
> > > >
> > > >
> > > > ATB,
> > > >
> > > > Mark.
> > > >
> > >
> > >
> > > Agreed, I think we are saying approximately the same thing here.
> > >
> > >  We need a mechanism to retrieve the vCPUs endianness and a way to
> > > notify subscribed devices when it changes.
> >
> > I don't think I agree, it's not the same thing.
> > Guest should just convert and send data in LE format.
> > Host should then convert from LE format.
> > Target endian-ness does not come into it.
> 
> That's not in the VIRTIO 1.2 spec. We are talking about supporting
> things as they currently stand, not as they could have been.

Okay it's a spec bug then.
I do worry that if we just work around it now no one will bother
fixing it properly though.

How do other drivers handle this? Could you check please?


-- 
MST

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Mark Cave-Ayland]

On 25/04/2024 11:26, Manos Pitsidianakis wrote:

> On Thu, 25 Apr 2024 at 13:24, Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Thu, Apr 25, 2024 at 01:04:31PM +0300, Manos Pitsidianakis wrote:
>>> On Thu, 25 Apr 2024 at 10:49, Mark Cave-Ayland
>>> <mark.cave-ayland@ilande.co.uk> wrote:
>>>>
>>>> On 25/04/2024 07:30, Manos Pitsidianakis wrote:
>>>>
>>>>> On Wed, 24 Apr 2024 at 13:31, Mark Cave-Ayland
>>>>> <mark.cave-ayland@ilande.co.uk> wrote:
>>>>>>
>>>>>> On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:
>>>>>>
>>>>>>> On 23/4/24 11:18, Manos Pitsidianakis wrote:
>>>>>>>> On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
>>>>>>>> <manos.pitsidianakis@linaro.org> wrote:
>>>>>>>>>
>>>>>>>>> On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
>>>>>>>>>>> On 22/4/24 23:02, Michael S. Tsirkin wrote:
>>>>>>>>>>>> On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
>>>>>>>>>>>>> Since VirtIO devices can change endianness at runtime,
>>>>>>>>>>>>> we need to use the device endianness, not the target
>>>>>>>>>>>>> one.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Cc: qemu-stable@nongnu.org
>>>>>>>>>>>>> Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
>>>>>>>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> This is all completely bogus. Virtio SND is from Virtio 1.0 only.
>>>>>>>>>>>> It is unconditionally little endian.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This part of the code is for PCM frames (raw bytes), not virtio spec
>>>>>>>>> fields (which indeed must be LE in modern VIRTIO).
>>>>>>>>
>>>>>>>> Thought a little more about it. We should keep the target's endianness
>>>>>>>> here, if it's mutable then we should query the machine the device is
>>>>>>>> attached to somehow. the virtio device should never change endianness
>>>>>>>> like Michael says since it's not legacy.
>>>>>>>
>>>>>>> Grr. So as Richard suggested, this need to be pass as a device
>>>>>>> property then.
>>>>>>> (https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09876@linaro.org/)
>>>>>>
>>>>>> It feels to me that the endianness is something that should be negotiated as part of
>>>>>> the frame format, since the endianness of the audio hardware can be different from
>>>>>> that of the CPU (think PReP machines where it was common that a big endian CPU is
>>>>>> driving little endian hardware as found on x86).
>>>>>
>>>>> But that is the job of the hardware drivers, isn't it? Here we are
>>>>> taking frames passed from the guest to its virtio driver in the format
>>>>> specified in the target cpu's endianness and QEMU as the device passes
>>>>> it to host ALSA/Pipewire/etc which in turn passes it to the actual
>>>>> audio hardware driver..
>>>>
>>>> The problem is that the notion of target CPU endian is not fixed. For example the
>>>> PowerPC CPU starts off in big-endian mode, but these days most systems will switch
>>>> the CPU to little-endian mode on startup to run ppc64le. There's also the ILE bit
>>>> which can be configured so that a big-endian PowerPC CPU can dynamically switch to
>>>> little-endian mode when processing an interrupt, so you could potentially end up with
>>>> either depending upon the current mode of the CPU.
>>>>
>>>> These are the kinds of issues that led to the later virtio specifications simply
>>>> using little-endian for everything, since then there is zero ambiguity over what
>>>> endian is required for the virtio configuration space accesses.
>>>>
>>>> It feels to me that assuming a target CPU endian is fixed for the PCM frame formats
>>>> is simply repeating the mistakes of the past - and even the fact that we are
>>>> discussing this within this thread suggests that at a very minimum the virtio-snd
>>>> specification needs to be updated to clarify the byte ordering of the PCM frame formats.
>>>>
>>>>
>>>> ATB,
>>>>
>>>> Mark.
>>>>
>>>
>>>
>>> Agreed, I think we are saying approximately the same thing here.
>>>
>>>   We need a mechanism to retrieve the vCPUs endianness and a way to
>>> notify subscribed devices when it changes.
>>
>> I don't think I agree, it's not the same thing.
>> Guest should just convert and send data in LE format.
>> Host should then convert from LE format.
>> Target endian-ness does not come into it.
> 
> That's not in the VIRTIO 1.2 spec. We are talking about supporting
> things as they currently stand, not as they could have been.

Can you also clarify the particular case that you're trying to fix - is it big-endian 
on ARM, or something else?


ATB,

Mark.

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Philippe Mathieu-Daudé]

On 25/4/24 12:40, Mark Cave-Ayland wrote:
> On 25/04/2024 11:26, Manos Pitsidianakis wrote:
> 
>> On Thu, 25 Apr 2024 at 13:24, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>
>>> On Thu, Apr 25, 2024 at 01:04:31PM +0300, Manos Pitsidianakis wrote:
>>>> On Thu, 25 Apr 2024 at 10:49, Mark Cave-Ayland
>>>> <mark.cave-ayland@ilande.co.uk> wrote:
>>>>>
>>>>> On 25/04/2024 07:30, Manos Pitsidianakis wrote:
>>>>>
>>>>>> On Wed, 24 Apr 2024 at 13:31, Mark Cave-Ayland
>>>>>> <mark.cave-ayland@ilande.co.uk> wrote:
>>>>>>>
>>>>>>> On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:
>>>>>>>
>>>>>>>> On 23/4/24 11:18, Manos Pitsidianakis wrote:
>>>>>>>>> On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
>>>>>>>>> <manos.pitsidianakis@linaro.org> wrote:
>>>>>>>>>>
>>>>>>>>>> On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin 
>>>>>>>>>> <mst@redhat.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe 
>>>>>>>>>>> Mathieu-Daudé wrote:
>>>>>>>>>>>> On 22/4/24 23:02, Michael S. Tsirkin wrote:
>>>>>>>>>>>>> On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe 
>>>>>>>>>>>>> Mathieu-Daudé wrote:
>>>>>>>>>>>>>> Since VirtIO devices can change endianness at runtime,
>>>>>>>>>>>>>> we need to use the device endianness, not the target
>>>>>>>>>>>>>> one.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Cc: qemu-stable@nongnu.org
>>>>>>>>>>>>>> Fixes: eb9ad377bb ("virtio-sound: handle control messages 
>>>>>>>>>>>>>> and streams")
>>>>>>>>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> This is all completely bogus. Virtio SND is from Virtio 1.0 
>>>>>>>>>>>>> only.
>>>>>>>>>>>>> It is unconditionally little endian.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This part of the code is for PCM frames (raw bytes), not 
>>>>>>>>>> virtio spec
>>>>>>>>>> fields (which indeed must be LE in modern VIRTIO).
>>>>>>>>>
>>>>>>>>> Thought a little more about it. We should keep the target's 
>>>>>>>>> endianness
>>>>>>>>> here, if it's mutable then we should query the machine the 
>>>>>>>>> device is
>>>>>>>>> attached to somehow. the virtio device should never change 
>>>>>>>>> endianness
>>>>>>>>> like Michael says since it's not legacy.
>>>>>>>>
>>>>>>>> Grr. So as Richard suggested, this need to be pass as a device
>>>>>>>> property then.
>>>>>>>> (https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09876@linaro.org/)
>>>>>>>
>>>>>>> It feels to me that the endianness is something that should be 
>>>>>>> negotiated as part of
>>>>>>> the frame format, since the endianness of the audio hardware can 
>>>>>>> be different from
>>>>>>> that of the CPU (think PReP machines where it was common that a 
>>>>>>> big endian CPU is
>>>>>>> driving little endian hardware as found on x86).
>>>>>>
>>>>>> But that is the job of the hardware drivers, isn't it? Here we are
>>>>>> taking frames passed from the guest to its virtio driver in the 
>>>>>> format
>>>>>> specified in the target cpu's endianness and QEMU as the device 
>>>>>> passes
>>>>>> it to host ALSA/Pipewire/etc which in turn passes it to the actual
>>>>>> audio hardware driver..
>>>>>
>>>>> The problem is that the notion of target CPU endian is not fixed. 
>>>>> For example the
>>>>> PowerPC CPU starts off in big-endian mode, but these days most 
>>>>> systems will switch
>>>>> the CPU to little-endian mode on startup to run ppc64le. There's 
>>>>> also the ILE bit
>>>>> which can be configured so that a big-endian PowerPC CPU can 
>>>>> dynamically switch to
>>>>> little-endian mode when processing an interrupt, so you could 
>>>>> potentially end up with
>>>>> either depending upon the current mode of the CPU.
>>>>>
>>>>> These are the kinds of issues that led to the later virtio 
>>>>> specifications simply
>>>>> using little-endian for everything, since then there is zero 
>>>>> ambiguity over what
>>>>> endian is required for the virtio configuration space accesses.
>>>>>
>>>>> It feels to me that assuming a target CPU endian is fixed for the 
>>>>> PCM frame formats
>>>>> is simply repeating the mistakes of the past - and even the fact 
>>>>> that we are
>>>>> discussing this within this thread suggests that at a very minimum 
>>>>> the virtio-snd
>>>>> specification needs to be updated to clarify the byte ordering of 
>>>>> the PCM frame formats.
>>>>>
>>>>>
>>>>> ATB,
>>>>>
>>>>> Mark.
>>>>>
>>>>
>>>>
>>>> Agreed, I think we are saying approximately the same thing here.
>>>>
>>>>   We need a mechanism to retrieve the vCPUs endianness and a way to
>>>> notify subscribed devices when it changes.
>>>
>>> I don't think I agree, it's not the same thing.
>>> Guest should just convert and send data in LE format.
>>> Host should then convert from LE format.
>>> Target endian-ness does not come into it.
>>
>> That's not in the VIRTIO 1.2 spec. We are talking about supporting
>> things as they currently stand, not as they could have been.
> 
> Can you also clarify the particular case that you're trying to fix - is 
> it big-endian on ARM, or something else?

I'm only aware of big-endian on ARM.

Regards,

Phil.

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Michael S. Tsirkin]

On Thu, Apr 25, 2024 at 01:15:43PM +0200, Philippe Mathieu-Daudé wrote:
> On 25/4/24 12:40, Mark Cave-Ayland wrote:
> > On 25/04/2024 11:26, Manos Pitsidianakis wrote:
> > 
> > > On Thu, 25 Apr 2024 at 13:24, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > 
> > > > On Thu, Apr 25, 2024 at 01:04:31PM +0300, Manos Pitsidianakis wrote:
> > > > > On Thu, 25 Apr 2024 at 10:49, Mark Cave-Ayland
> > > > > <mark.cave-ayland@ilande.co.uk> wrote:
> > > > > > 
> > > > > > On 25/04/2024 07:30, Manos Pitsidianakis wrote:
> > > > > > 
> > > > > > > On Wed, 24 Apr 2024 at 13:31, Mark Cave-Ayland
> > > > > > > <mark.cave-ayland@ilande.co.uk> wrote:
> > > > > > > > 
> > > > > > > > On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:
> > > > > > > > 
> > > > > > > > > On 23/4/24 11:18, Manos Pitsidianakis wrote:
> > > > > > > > > > On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
> > > > > > > > > > <manos.pitsidianakis@linaro.org> wrote:
> > > > > > > > > > > 
> > > > > > > > > > > On Tue, 23 Apr 2024 at 00:11,
> > > > > > > > > > > Michael S. Tsirkin <mst@redhat.com>
> > > > > > > > > > > wrote:
> > > > > > > > > > > > 
> > > > > > > > > > > > On Mon, Apr 22, 2024 at
> > > > > > > > > > > > 11:07:21PM +0200, Philippe
> > > > > > > > > > > > Mathieu-Daudé wrote:
> > > > > > > > > > > > > On 22/4/24 23:02, Michael S. Tsirkin wrote:
> > > > > > > > > > > > > > On Mon, Apr 22, 2024 at
> > > > > > > > > > > > > > 04:20:56PM +0200,
> > > > > > > > > > > > > > Philippe Mathieu-Daudé
> > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > Since VirtIO devices can change endianness at runtime,
> > > > > > > > > > > > > > > we need to use the device endianness, not the target
> > > > > > > > > > > > > > > one.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Cc: qemu-stable@nongnu.org
> > > > > > > > > > > > > > > Fixes: eb9ad377bb
> > > > > > > > > > > > > > > ("virtio-sound:
> > > > > > > > > > > > > > > handle control
> > > > > > > > > > > > > > > messages and
> > > > > > > > > > > > > > > streams")
> > > > > > > > > > > > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > This is all completely
> > > > > > > > > > > > > > bogus. Virtio SND is
> > > > > > > > > > > > > > from Virtio 1.0 only.
> > > > > > > > > > > > > > It is unconditionally little endian.
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > This part of the code is for PCM
> > > > > > > > > > > frames (raw bytes), not virtio spec
> > > > > > > > > > > fields (which indeed must be LE in modern VIRTIO).
> > > > > > > > > > 
> > > > > > > > > > Thought a little more about it. We
> > > > > > > > > > should keep the target's endianness
> > > > > > > > > > here, if it's mutable then we should
> > > > > > > > > > query the machine the device is
> > > > > > > > > > attached to somehow. the virtio device
> > > > > > > > > > should never change endianness
> > > > > > > > > > like Michael says since it's not legacy.
> > > > > > > > > 
> > > > > > > > > Grr. So as Richard suggested, this need to be pass as a device
> > > > > > > > > property then.
> > > > > > > > > (https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09876@linaro.org/)
> > > > > > > > 
> > > > > > > > It feels to me that the endianness is something
> > > > > > > > that should be negotiated as part of
> > > > > > > > the frame format, since the endianness of the
> > > > > > > > audio hardware can be different from
> > > > > > > > that of the CPU (think PReP machines where it
> > > > > > > > was common that a big endian CPU is
> > > > > > > > driving little endian hardware as found on x86).
> > > > > > > 
> > > > > > > But that is the job of the hardware drivers, isn't it? Here we are
> > > > > > > taking frames passed from the guest to its virtio
> > > > > > > driver in the format
> > > > > > > specified in the target cpu's endianness and QEMU as
> > > > > > > the device passes
> > > > > > > it to host ALSA/Pipewire/etc which in turn passes it to the actual
> > > > > > > audio hardware driver..
> > > > > > 
> > > > > > The problem is that the notion of target CPU endian is
> > > > > > not fixed. For example the
> > > > > > PowerPC CPU starts off in big-endian mode, but these
> > > > > > days most systems will switch
> > > > > > the CPU to little-endian mode on startup to run ppc64le.
> > > > > > There's also the ILE bit
> > > > > > which can be configured so that a big-endian PowerPC CPU
> > > > > > can dynamically switch to
> > > > > > little-endian mode when processing an interrupt, so you
> > > > > > could potentially end up with
> > > > > > either depending upon the current mode of the CPU.
> > > > > > 
> > > > > > These are the kinds of issues that led to the later
> > > > > > virtio specifications simply
> > > > > > using little-endian for everything, since then there is
> > > > > > zero ambiguity over what
> > > > > > endian is required for the virtio configuration space accesses.
> > > > > > 
> > > > > > It feels to me that assuming a target CPU endian is
> > > > > > fixed for the PCM frame formats
> > > > > > is simply repeating the mistakes of the past - and even
> > > > > > the fact that we are
> > > > > > discussing this within this thread suggests that at a
> > > > > > very minimum the virtio-snd
> > > > > > specification needs to be updated to clarify the byte
> > > > > > ordering of the PCM frame formats.
> > > > > > 
> > > > > > 
> > > > > > ATB,
> > > > > > 
> > > > > > Mark.
> > > > > > 
> > > > > 
> > > > > 
> > > > > Agreed, I think we are saying approximately the same thing here.
> > > > > 
> > > > >   We need a mechanism to retrieve the vCPUs endianness and a way to
> > > > > notify subscribed devices when it changes.
> > > > 
> > > > I don't think I agree, it's not the same thing.
> > > > Guest should just convert and send data in LE format.
> > > > Host should then convert from LE format.
> > > > Target endian-ness does not come into it.
> > > 
> > > That's not in the VIRTIO 1.2 spec. We are talking about supporting
> > > things as they currently stand, not as they could have been.
> > 
> > Can you also clarify the particular case that you're trying to fix - is
> > it big-endian on ARM, or something else?
> 
> I'm only aware of big-endian on ARM.
> 
> Regards,
> 
> Phil.

But of course this applies to any BE emulated on LE
or vice versa.

-- 
MST

Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Mark Cave-Ayland]

On 25/04/2024 11:04, Manos Pitsidianakis wrote:

> On Thu, 25 Apr 2024 at 10:49, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> On 25/04/2024 07:30, Manos Pitsidianakis wrote:
>>
>>> On Wed, 24 Apr 2024 at 13:31, Mark Cave-Ayland
>>> <mark.cave-ayland@ilande.co.uk> wrote:
>>>>
>>>> On 23/04/2024 12:05, Philippe Mathieu-Daudé wrote:
>>>>
>>>>> On 23/4/24 11:18, Manos Pitsidianakis wrote:
>>>>>> On Tue, 23 Apr 2024 at 11:47, Manos Pitsidianakis
>>>>>> <manos.pitsidianakis@linaro.org> wrote:
>>>>>>>
>>>>>>> On Tue, 23 Apr 2024 at 00:11, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>
>>>>>>>> On Mon, Apr 22, 2024 at 11:07:21PM +0200, Philippe Mathieu-Daudé wrote:
>>>>>>>>> On 22/4/24 23:02, Michael S. Tsirkin wrote:
>>>>>>>>>> On Mon, Apr 22, 2024 at 04:20:56PM +0200, Philippe Mathieu-Daudé wrote:
>>>>>>>>>>> Since VirtIO devices can change endianness at runtime,
>>>>>>>>>>> we need to use the device endianness, not the target
>>>>>>>>>>> one.
>>>>>>>>>>>
>>>>>>>>>>> Cc: qemu-stable@nongnu.org
>>>>>>>>>>> Fixes: eb9ad377bb ("virtio-sound: handle control messages and streams")
>>>>>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This is all completely bogus. Virtio SND is from Virtio 1.0 only.
>>>>>>>>>> It is unconditionally little endian.
>>>>>>>
>>>>>>>
>>>>>>> This part of the code is for PCM frames (raw bytes), not virtio spec
>>>>>>> fields (which indeed must be LE in modern VIRTIO).
>>>>>>
>>>>>> Thought a little more about it. We should keep the target's endianness
>>>>>> here, if it's mutable then we should query the machine the device is
>>>>>> attached to somehow. the virtio device should never change endianness
>>>>>> like Michael says since it's not legacy.
>>>>>
>>>>> Grr. So as Richard suggested, this need to be pass as a device
>>>>> property then.
>>>>> (https://lore.kernel.org/qemu-devel/ed134c9d-6e6f-465b-900f-e39ca4e09876@linaro.org/)
>>>>
>>>> It feels to me that the endianness is something that should be negotiated as part of
>>>> the frame format, since the endianness of the audio hardware can be different from
>>>> that of the CPU (think PReP machines where it was common that a big endian CPU is
>>>> driving little endian hardware as found on x86).
>>>
>>> But that is the job of the hardware drivers, isn't it? Here we are
>>> taking frames passed from the guest to its virtio driver in the format
>>> specified in the target cpu's endianness and QEMU as the device passes
>>> it to host ALSA/Pipewire/etc which in turn passes it to the actual
>>> audio hardware driver..
>>
>> The problem is that the notion of target CPU endian is not fixed. For example the
>> PowerPC CPU starts off in big-endian mode, but these days most systems will switch
>> the CPU to little-endian mode on startup to run ppc64le. There's also the ILE bit
>> which can be configured so that a big-endian PowerPC CPU can dynamically switch to
>> little-endian mode when processing an interrupt, so you could potentially end up with
>> either depending upon the current mode of the CPU.
>>
>> These are the kinds of issues that led to the later virtio specifications simply
>> using little-endian for everything, since then there is zero ambiguity over what
>> endian is required for the virtio configuration space accesses.
>>
>> It feels to me that assuming a target CPU endian is fixed for the PCM frame formats
>> is simply repeating the mistakes of the past - and even the fact that we are
>> discussing this within this thread suggests that at a very minimum the virtio-snd
>> specification needs to be updated to clarify the byte ordering of the PCM frame formats.
>
> Agreed, I think we are saying approximately the same thing here.
> 
>   We need a mechanism to retrieve the vCPUs endianness and a way to
> notify subscribed devices when it changes.
> 
>   I think that then, since the virtio device is mostly certain of the
> correct target endianness and completely certain of the host
> endianness, it can perform the necessary conversions.
> 
>   I don't recall seeing a restriction on the byte ordering of PCM
> formats other than the CPU order; except for the ones which have
> explicit endianness in their definitions. Please correct me if I am
> wrong!
> 
> A straightforward solution would be to set an endianness change notify
> callback in DeviceClass. What do you think?

Well, I guess... but why propose such a complex solution when you can simply specify 
the endianness of the PCM frame? If you think back to pre-1.0 virtio where everything 
was done using target vCPU endianness, why did they not implement this solution back 
then to solve the problem instead of mandating that virtio-1.0 onwards should simply 
use little-endian? Clearly specifying that all implementations should use 
little-endian was the deemed to be the best way to solve all these issues.

Have you raised this issue on the virtio-dev mailing list? It's worth explaining the 
problem you're trying to solve there, since hopefully the person or persons who 
designed and proposed the virtio-snd specification will be able to give you the 
correct advice.


ATB,

Mark.