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

[Greg Kurz]

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.

> > ---
> >  include/hw/core/cpu.h | 9 +++++++++
> >  hw/core/cpu.c         | 8 ++++++--
> >  hw/virtio/virtio.c    | 4 +---
> >  3 files changed, 16 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > index 2d43f78819f..b12028c3c03 100644
> > --- a/include/hw/core/cpu.h
> > +++ b/include/hw/core/cpu.h
> > @@ -602,6 +602,15 @@ hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr 
> > addr);
> >   */
> >  int cpu_asidx_from_attrs(CPUState *cpu, MemTxAttrs attrs);
> >  
> > +/**
> > + * cpu_virtio_is_big_endian:
> > + * @cpu: CPU
> > +
> > + * Returns %true if a CPU which supports runtime configurable endianness
> > + * is currently big-endian.
> > + */
> > +bool cpu_virtio_is_big_endian(CPUState *cpu);
> > +
> >  #endif /* CONFIG_USER_ONLY */
> >  
> >  /**
> > diff --git a/hw/core/cpu.c b/hw/core/cpu.c
> > index 4dce35f832f..daaff56a79e 100644
> > --- a/hw/core/cpu.c
> > +++ b/hw/core/cpu.c
> > @@ -218,8 +218,13 @@ static int cpu_common_gdb_write_register(CPUState 
> > *cpu, uint8_t *buf, int reg)
> >      return 0;
> >  }
> >  
> > -static bool cpu_common_virtio_is_big_endian(CPUState *cpu)
> > +bool cpu_virtio_is_big_endian(CPUState *cpu)
> >  {
> > +    CPUClass *cc = CPU_GET_CLASS(cpu);
> > +
> > +    if (cc->virtio_is_big_endian) {
> > +        return cc->virtio_is_big_endian(cpu);
> > +    }
> >      return target_words_bigendian();
> >  }
> >  
> > @@ -438,7 +443,6 @@ static void cpu_class_init(ObjectClass *klass, void 
> > *data)
> >      k->write_elf64_note = cpu_common_write_elf64_note;
> >      k->gdb_read_register = cpu_common_gdb_read_register;
> >      k->gdb_write_register = cpu_common_gdb_write_register;
> > -    k->virtio_is_big_endian = cpu_common_virtio_is_big_endian;
> >      set_bit(DEVICE_CATEGORY_CPU, dc->categories);
> >      dc->realize = cpu_common_realizefn;
> >      dc->unrealize = cpu_common_unrealizefn;
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 1fd1917ca0f..fe6a4be99e4 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1973,9 +1973,7 @@ static enum virtio_device_endian 
> > virtio_default_endian(void)
> >  
> >  static enum virtio_device_endian virtio_current_cpu_endian(void)
> >  {
> > -    CPUClass *cc = CPU_GET_CLASS(current_cpu);
> > -
> > -    if (cc->virtio_is_big_endian(current_cpu)) {
> > +    if (cpu_virtio_is_big_endian(current_cpu)) {
> >          return VIRTIO_DEVICE_ENDIAN_BIG;
> >      } else {
> >          return VIRTIO_DEVICE_ENDIAN_LITTLE;
> > -- 
> > 2.26.2
> 
>