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.
> > > > >>> (ed134c9d-6e6f-465b-900f-e39ca4e09876@linaro.org/">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