Re: [PATCH v4 03/28] cpu: Introduce cpu_virtio_is_big_endian()

[Philippe Mathieu-Daudé]

Hi Michael,

On 3/4/21 8:51 AM, Greg Kurz wrote:
> On Wed, 3 Mar 2021 17:08:32 -0500
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
>> On Wed, Mar 03, 2021 at 10:46:43PM +0100, Philippe Mathieu-Daudé wrote:
>>> Introduce the cpu_virtio_is_big_endian() generic helper to avoid
>>> calling CPUClass internal virtio_is_big_endian() one.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> Using virtio in the name here probably because virtio wants this?
>> That doesn't sound like a good naming strategy, name should
>> tell us what function does not how it's used.
>>
> 
> I tend to agree but there was a consensus to deliberately put
> virtio in the name when this was first introduced, so that
> nobody else ever try to use it, as recorded in the commit log.
> 
> commit bf7663c4bd8f8f619d6dbb5780025d92ace250a8
> Author: Greg Kurz <groug@kaod.org>
> Date:   Tue Jun 24 19:33:21 2014 +0200
> 
>     cpu: introduce CPUClass::virtio_is_big_endian()
>     
>     If we want to support targets that can change endianness (modern PPC and
>     ARM for the moment), we need to add a per-CPU class method to be called
>     from the virtio code. The virtio_ prefix in the name is a hint for people
>     to avoid misusage (aka. anywhere but from the virtio code).
>     
>     The default behaviour is to return the compile-time default target
>     endianness.
>     
>     Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>     Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>     Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Is there something new on this front ? I'm not convinced that anything
> but legacy virtio en POWER (or any other target that can change endian
> at runtime) needs this. The next step I see for this is_big_endian()
> stuff is deprecation and removal. In the meantime, I think we should
> keep the virtio wording to prevent additional users for this.

On 3/3/21 11:15 PM, Michael S. Tsirkin wrote:
> On a more concrete proposal, how about using this change
> to rename the virtio_is_big_endian field to guest_is_big_endian(),
> and put the wrapper somewhere in a virtio header instead?

Due to Greg comment, I'll keep cpu_virtio_is_big_endian() in
the v5 respin. This doesn't seem a real blocker for the rest
of the changeset. We can settle the name and send a patch on
top.

Regards,

Phil.