| Message ID | 20210303214708.1727801-4-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | cpu: Introduce SysemuCPUOps structure, remove watchpoints from usermode | expand |
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
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
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~
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~
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 > >
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 --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;
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(-)