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.
>