Re: [PATCH v3] hw/audio/virtio-snd: Use device endianness instead of target one

[Manos Pitsidianakis]

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