diff mbox series

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

Message ID 20210303214708.1727801-4-f4bug@amsat.org (mailing list archive)
State New, archived
Headers show
Series cpu: Introduce SysemuCPUOps structure, remove watchpoints from usermode | expand

Commit Message

Philippe Mathieu-Daudé March 3, 2021, 9:46 p.m. UTC
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>
---
 include/hw/core/cpu.h | 9 +++++++++
 hw/core/cpu.c         | 8 ++++++--
 hw/virtio/virtio.c    | 4 +---
 3 files changed, 16 insertions(+), 5 deletions(-)

Comments

Michael S. Tsirkin March 3, 2021, 10:08 p.m. UTC | #1
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.

> ---
>  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
Michael S. Tsirkin March 3, 2021, 10:15 p.m. UTC | #2
On Wed, Mar 03, 2021 at 05:08:36PM -0500, Michael S. Tsirkin 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.

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?

> > ---
> >  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
Richard Henderson March 3, 2021, 10:18 p.m. UTC | #3
On 3/3/21 2:15 PM, Michael S. Tsirkin wrote:
> On Wed, Mar 03, 2021 at 05:08:36PM -0500, Michael S. Tsirkin 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.
> 
> 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?

We already one for normal guest endianness.  This one is for when virtio 
specifically differs from that.


r~
Richard Henderson March 3, 2021, 10:24 p.m. UTC | #4
On 3/3/21 2:18 PM, Richard Henderson wrote:
> On 3/3/21 2:15 PM, Michael S. Tsirkin wrote:
>> On Wed, Mar 03, 2021 at 05:08:36PM -0500, Michael S. Tsirkin 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.
>>
>> 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?
> 
> We already one for normal guest endianness.  This one is for when virtio 
> specifically differs from that.

Hmph.  I remembered incorrectly.  I thought we had a bit of ppc weirdness in 
which endianness was reversed for virtio, but no.

I'm ok with the renaming.


r~
Greg Kurz March 4, 2021, 7:51 a.m. UTC | #5
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
> 
>
Philippe Mathieu-Daudé April 22, 2021, 10:33 a.m. UTC | #6
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.
diff mbox series

Patch

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;