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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.