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