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