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.