* [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property
@ 2018-02-02 0:42 Alistair Francis
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model Alistair Francis
` (6 more replies)
0 siblings, 7 replies; 33+ messages in thread
From: Alistair Francis @ 2018-02-02 0:42 UTC (permalink / raw)
To: qemu-devel
Cc: alistair.francis, alistair23, ehabkost, marcel, imammedo, f4bug,
peter.maydell
There are numorous QEMU machines that only have a single or a handful of
valid CPU options. To simplyfy the management of specificying which CPU
is/isn't valid let's create a property that can be set in the machine
init. We can then check to see if the user supplied CPU is in that list
or not.
I have added the valid_cpu_types for some ARM machines only at the
moment.
Here is what specifying the CPUs looks like now:
$ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m3" -S
QEMU 2.10.50 monitor - type 'help' for more information
(qemu) info cpus
* CPU #0: thread_id=24175
(qemu) q
$ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m4" -S
QEMU 2.10.50 monitor - type 'help' for more information
(qemu) q
$ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m5" -S
qemu-system-aarch64: unable to find CPU model 'cortex-m5'
$ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-a9" -S
qemu-system-aarch64: Invalid CPU type: cortex-a9-arm-cpu
The valid models are: cortex-m3, cortex-m4
V5:
- Use cpu_model instead of cpu_type
V4:
- Rebase
- Remove spaces
V3:
- Make the varialbes static
V2:
- Rebase
- Reorder patches
- Add a Raspberry Pi 2 CPU fix
V1:
- Small fixes to prepare a series instead of RFC
- Add commit messages for the commits
- Expand the machine support to ARM machines
RFC v2:
- Rebase on Igor's work
- Use more QEMUisms inside the code
- List the supported machines in a NULL terminated array
Alistair Francis (6):
machine: Convert the valid cpu types to use cpu_model
netduino2: Specify the valid CPUs
bcm2836: Use the Cortex-A7 instead of Cortex-A15
raspi: Specify the valid CPUs
xlnx-zcu102: Specify the valid CPUs
xilinx_zynq: Specify the valid CPUs
hw/arm/bcm2836.c | 2 +-
hw/arm/netduino2.c | 10 +++++++++-
hw/arm/raspi.c | 7 +++++++
hw/arm/xilinx_zynq.c | 6 ++++++
hw/arm/xlnx-zcu102.c | 17 +++++++++++++++++
hw/core/machine.c | 11 +++++------
6 files changed, 45 insertions(+), 8 deletions(-)
--
2.14.1
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2018-02-02 0:42 [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Alistair Francis
@ 2018-02-02 0:42 ` Alistair Francis
2018-02-02 18:23 ` Eduardo Habkost
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 2/6] netduino2: Specify the valid CPUs Alistair Francis
` (5 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Alistair Francis @ 2018-02-02 0:42 UTC (permalink / raw)
To: qemu-devel
Cc: alistair.francis, alistair23, ehabkost, marcel, imammedo, f4bug,
peter.maydell
As cpu_type is not a user visible string let's convert the
valid_cpu_types to compare against cpu_model instead. This way we have a
user friendly string to report back.
Once we have a cpu_type to cpu_model conversion this patch should be
reverted and we should use cpu_type instead.
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
hw/core/machine.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index cdc1163dc6..de5bac1c84 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
/* If the machine supports the valid_cpu_types check and the user
* specified a CPU with -cpu check here that the user CPU is supported.
*/
- if (machine_class->valid_cpu_types && machine->cpu_type) {
- ObjectClass *class = object_class_by_name(machine->cpu_type);
+ if (machine_class->valid_cpu_types && machine->cpu_model) {
int i;
for (i = 0; machine_class->valid_cpu_types[i]; i++) {
- if (object_class_dynamic_cast(class,
- machine_class->valid_cpu_types[i])) {
+ if (!strcmp(machine->cpu_model,
+ machine_class->valid_cpu_types[i])) {
/* The user specificed CPU is in the valid field, we are
* good to go.
*/
@@ -792,8 +791,8 @@ void machine_run_board_init(MachineState *machine)
if (!machine_class->valid_cpu_types[i]) {
/* The user specified CPU is not valid */
- error_report("Invalid CPU type: %s", machine->cpu_type);
- error_printf("The valid types are: %s",
+ error_report("Invalid CPU model: %s", machine->cpu_model);
+ error_printf("The valid models are: %s",
machine_class->valid_cpu_types[0]);
for (i = 1; machine_class->valid_cpu_types[i]; i++) {
error_printf(", %s", machine_class->valid_cpu_types[i]);
--
2.14.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 2/6] netduino2: Specify the valid CPUs
2018-02-02 0:42 [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Alistair Francis
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model Alistair Francis
@ 2018-02-02 0:42 ` Alistair Francis
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 3/6] bcm2836: Use the Cortex-A7 instead of Cortex-A15 Alistair Francis
` (4 subsequent siblings)
6 siblings, 0 replies; 33+ messages in thread
From: Alistair Francis @ 2018-02-02 0:42 UTC (permalink / raw)
To: qemu-devel
Cc: alistair.francis, alistair23, ehabkost, marcel, imammedo, f4bug,
peter.maydell
List all possible valid CPU options.
Although the board only ever has a Cortex-M3 we mark the Cortex-M4 as
supported because the Netduino2 Plus supports the Cortex-M4 and the
Netduino2 Plus is similar to the Netduino2.
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
V5:
- Use cpu_model names
V3:
- Add static property
V2:
- Fixup allignment
RFC v2:
- Use a NULL terminated list
- Add the Cortex-M4 for testing
hw/arm/netduino2.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
index f936017d4a..91c925a56c 100644
--- a/hw/arm/netduino2.c
+++ b/hw/arm/netduino2.c
@@ -34,18 +34,26 @@ static void netduino2_init(MachineState *machine)
DeviceState *dev;
dev = qdev_create(NULL, TYPE_STM32F205_SOC);
- qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3"));
+ qdev_prop_set_string(dev, "cpu-type", machine->cpu_type);
object_property_set_bool(OBJECT(dev), true, "realized", &error_fatal);
armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
FLASH_SIZE);
}
+static const char *netduino_valid_cpus[] = {
+ "cortex-m3",
+ "cortex-m4",
+ NULL
+};
+
static void netduino2_machine_init(MachineClass *mc)
{
mc->desc = "Netduino 2 Machine";
mc->init = netduino2_init;
mc->ignore_memory_transaction_failures = true;
+ mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m3");
+ mc->valid_cpu_types = netduino_valid_cpus;
}
DEFINE_MACHINE("netduino2", netduino2_machine_init)
--
2.14.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 3/6] bcm2836: Use the Cortex-A7 instead of Cortex-A15
2018-02-02 0:42 [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Alistair Francis
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model Alistair Francis
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 2/6] netduino2: Specify the valid CPUs Alistair Francis
@ 2018-02-02 0:42 ` Alistair Francis
2018-02-15 13:23 ` Philippe Mathieu-Daudé
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs Alistair Francis
` (3 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Alistair Francis @ 2018-02-02 0:42 UTC (permalink / raw)
To: qemu-devel
Cc: alistair.francis, alistair23, ehabkost, marcel, imammedo, f4bug,
peter.maydell
The BCM2836 uses a Cortex-A7 not a Cortex-A15. Update the device to use
the correct CPU.
https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/QA7_rev3.4.pdf
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
V3:
- Use ARM_CPU_TYPE_NAME() macro
V2:
- Fix the BCM2836 CPU
hw/arm/bcm2836.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
index 8c43291112..c477772484 100644
--- a/hw/arm/bcm2836.c
+++ b/hw/arm/bcm2836.c
@@ -30,7 +30,7 @@ static void bcm2836_init(Object *obj)
for (n = 0; n < BCM2836_NCPUS; n++) {
object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
- "cortex-a15-" TYPE_ARM_CPU);
+ ARM_CPU_TYPE_NAME("cortex-a7"));
object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]),
&error_abort);
}
--
2.14.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs
2018-02-02 0:42 [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Alistair Francis
` (2 preceding siblings ...)
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 3/6] bcm2836: Use the Cortex-A7 instead of Cortex-A15 Alistair Francis
@ 2018-02-02 0:42 ` Alistair Francis
2018-02-15 11:29 ` Peter Maydell
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 5/6] xlnx-zcu102: " Alistair Francis
` (2 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Alistair Francis @ 2018-02-02 0:42 UTC (permalink / raw)
To: qemu-devel
Cc: alistair.francis, alistair23, ehabkost, marcel, imammedo, f4bug,
peter.maydell
List all possible valid CPU options.
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
V5:
- Use cpu_model names
V4:
- Remove spaces
V3:
- Add static property
V2:
- Fix the indentation
hw/arm/raspi.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index cd5fa8c3dc..745a880726 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -158,6 +158,11 @@ static void raspi2_init(MachineState *machine)
setup_boot(machine, 2, machine->ram_size - vcram_size);
}
+static const char *raspi2_valid_cpus[] = {
+ "cortex-a7",
+ NULL
+};
+
static void raspi2_machine_init(MachineClass *mc)
{
mc->desc = "Raspberry Pi 2";
@@ -171,5 +176,7 @@ static void raspi2_machine_init(MachineClass *mc)
mc->default_cpus = BCM2836_NCPUS;
mc->default_ram_size = 1024 * 1024 * 1024;
mc->ignore_memory_transaction_failures = true;
+ mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a7");
+ mc->valid_cpu_types = raspi2_valid_cpus;
};
DEFINE_MACHINE("raspi2", raspi2_machine_init)
--
2.14.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 5/6] xlnx-zcu102: Specify the valid CPUs
2018-02-02 0:42 [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Alistair Francis
` (3 preceding siblings ...)
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs Alistair Francis
@ 2018-02-02 0:42 ` Alistair Francis
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 6/6] xilinx_zynq: " Alistair Francis
2018-03-13 23:13 ` [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Philippe Mathieu-Daudé
6 siblings, 0 replies; 33+ messages in thread
From: Alistair Francis @ 2018-02-02 0:42 UTC (permalink / raw)
To: qemu-devel
Cc: alistair.francis, alistair23, ehabkost, marcel, imammedo, f4bug,
peter.maydell
List all possible valid CPU options.
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
An implementation for single CPU machines is still being discussed. A
solution proposed by Eduardo is this:
1) Change the default on TYPE_MACHINE to:
mc->valid_cpu_types = { TYPE_CPU, NULL };
This will keep the existing behavior for all boards.
2) mc->valid_cpu_types=NULL be interpreted as "no CPU model
except the default is accepted" or "-cpu is not accepted" in
machine_run_board_init() (I prefer the former, but both
options would be correct)
3) Boards like xlnx_zynqmp could then just do this:
static void xxx_class_init(...) {
mc->default_cpu_type = MY_CPU_TYPE;
/* Reason: XXX_init() is hardcoded to MY_CPU_TYPE */
mc->valid_cpu_types = NULL;
}
V5:
- Use cpu_model names
V4:
- Remove spaces
V3:
- Make variable static
V2:
- Don't use the users -cpu
- Fixup allignment
hw/arm/xlnx-zcu102.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/hw/arm/xlnx-zcu102.c b/hw/arm/xlnx-zcu102.c
index b126cf148b..994b19a36f 100644
--- a/hw/arm/xlnx-zcu102.c
+++ b/hw/arm/xlnx-zcu102.c
@@ -184,6 +184,11 @@ static void xlnx_zynqmp_init(XlnxZCU102 *s, MachineState *machine)
arm_load_kernel(s->soc.boot_cpu_ptr, &xlnx_zcu102_binfo);
}
+static const char *xlnx_zynqmp_valid_cpus[] = {
+ "cortex-a53",
+ NULL
+};
+
static void xlnx_ep108_init(MachineState *machine)
{
XlnxZCU102 *s = EP108_MACHINE(machine);
@@ -216,6 +221,12 @@ static void xlnx_ep108_machine_class_init(ObjectClass *oc, void *data)
mc->ignore_memory_transaction_failures = true;
mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS;
mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS;
+ mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
+ /* The ZynqMP SoC is always a Cortex-A53. We add this here to give
+ * users a sane error if they specify a different CPU, but we never
+ * use their CPU choice.
+ */
+ mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
}
static const TypeInfo xlnx_ep108_machine_init_typeinfo = {
@@ -274,6 +285,12 @@ static void xlnx_zcu102_machine_class_init(ObjectClass *oc, void *data)
mc->ignore_memory_transaction_failures = true;
mc->max_cpus = XLNX_ZYNQMP_NUM_APU_CPUS + XLNX_ZYNQMP_NUM_RPU_CPUS;
mc->default_cpus = XLNX_ZYNQMP_NUM_APU_CPUS;
+ mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a53");
+ /* The ZynqMP SoC is always a Cortex-A53. We add this here to give
+ * users a sane error if they specify a different CPU, but we never
+ * use their CPU choice.
+ */
+ mc->valid_cpu_types = xlnx_zynqmp_valid_cpus;
}
static const TypeInfo xlnx_zcu102_machine_init_typeinfo = {
--
2.14.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [Qemu-devel] [PATCH v5 6/6] xilinx_zynq: Specify the valid CPUs
2018-02-02 0:42 [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Alistair Francis
` (4 preceding siblings ...)
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 5/6] xlnx-zcu102: " Alistair Francis
@ 2018-02-02 0:42 ` Alistair Francis
2018-03-13 23:13 ` [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Philippe Mathieu-Daudé
6 siblings, 0 replies; 33+ messages in thread
From: Alistair Francis @ 2018-02-02 0:42 UTC (permalink / raw)
To: qemu-devel
Cc: alistair.francis, alistair23, ehabkost, marcel, imammedo, f4bug,
peter.maydell
List all possible valid CPU options.
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
V5:
- Use cpu_model names
V4:
- Remove spaces
V3:
- Make variable static
V2:
- Fixup alignment
hw/arm/xilinx_zynq.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index 1836a4ed45..822e1889a5 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -313,6 +313,11 @@ static void zynq_init(MachineState *machine)
arm_load_kernel(ARM_CPU(first_cpu), &zynq_binfo);
}
+static const char *xlnx_zynq_7000_valid_cpus[] = {
+ "cortex-a9",
+ NULL
+};
+
static void zynq_machine_init(MachineClass *mc)
{
mc->desc = "Xilinx Zynq Platform Baseboard for Cortex-A9";
@@ -321,6 +326,7 @@ static void zynq_machine_init(MachineClass *mc)
mc->no_sdcard = 1;
mc->ignore_memory_transaction_failures = true;
mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a9");
+ mc->valid_cpu_types = xlnx_zynq_7000_valid_cpus;
}
DEFINE_MACHINE("xilinx-zynq-a9", zynq_machine_init)
--
2.14.1
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model Alistair Francis
@ 2018-02-02 18:23 ` Eduardo Habkost
2018-02-05 11:22 ` Igor Mammedov
0 siblings, 1 reply; 33+ messages in thread
From: Eduardo Habkost @ 2018-02-02 18:23 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel, peter.maydell, f4bug, imammedo, marcel, alistair23
On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:
> As cpu_type is not a user visible string let's convert the
> valid_cpu_types to compare against cpu_model instead. This way we have a
> user friendly string to report back.
>
> Once we have a cpu_type to cpu_model conversion this patch should be
> reverted and we should use cpu_type instead.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
> hw/core/machine.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index cdc1163dc6..de5bac1c84 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
> /* If the machine supports the valid_cpu_types check and the user
> * specified a CPU with -cpu check here that the user CPU is supported.
> */
> - if (machine_class->valid_cpu_types && machine->cpu_type) {
> - ObjectClass *class = object_class_by_name(machine->cpu_type);
> + if (machine_class->valid_cpu_types && machine->cpu_model) {
> int i;
>
> for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> - if (object_class_dynamic_cast(class,
> - machine_class->valid_cpu_types[i])) {
> + if (!strcmp(machine->cpu_model,
> + machine_class->valid_cpu_types[i])) {
I would rename valid_cpu_types to valid_cpu_models to make the
new semantics clearer.
Anyway, I have bad and good news:
The bad news is Igor already sent patches last week that remove
MachineState::cpu_model, so this conflicts with his series. Now
parse_cpu_model() will be the only place where the original CPU model name is
available, but the function needs to work on *-user too. See:
"[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
The good news is that I think we can fix this very easily if
validation is done at the same place where parse_cpu_model() is
called. e.g.:
current_machine->cpu_type = machine_class->default_cpu_type;
if (cpu_model) {
current_machine->cpu_type = parse_cpu_model(cpu_model);
if (machine_class->valid_cpu_models) {
ObjectClass *class = object_class_by_name(machine->cpu_type);
int i;
for (i = 0; machine_class->valid_cpu_models[i]; i++) {
const char *valid_model = machine_class->valid_cpu_models[i];
ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
if (object_class_dynamic_cast(class,
object_class_get_name(valid_class))) {
/* Valid CPU type, we're good to go */
break;
}
}
if (!machine_class->valid_cpu_models[i]) {
error_report("Invalid CPU model: %s", cpu_model);
error_printf("The valid CPU models are: %s",
machine_class->valid_cpu_models[0]);
for (i = 1; machine_class->valid_cpu_models[i]; i++) {
error_printf(", %s", machine_class->valid_cpu_models[i]);
}
error_printf("\n");
exit(1);
}
}
}
This can be done inside main(), or moved inside
machine_run_board_init() if main() pass cpu_model as argument to
the function.
On either case, I think it's a good idea to do validation and
printing of error messages closer to the code that parses the
command-line options. This way we separate parsing/validation
from initialization.
> /* The user specificed CPU is in the valid field, we are
> * good to go.
> */
> @@ -792,8 +791,8 @@ void machine_run_board_init(MachineState *machine)
>
> if (!machine_class->valid_cpu_types[i]) {
> /* The user specified CPU is not valid */
> - error_report("Invalid CPU type: %s", machine->cpu_type);
> - error_printf("The valid types are: %s",
> + error_report("Invalid CPU model: %s", machine->cpu_model);
> + error_printf("The valid models are: %s",
> machine_class->valid_cpu_types[0]);
> for (i = 1; machine_class->valid_cpu_types[i]; i++) {
> error_printf(", %s", machine_class->valid_cpu_types[i]);
> --
> 2.14.1
>
>
--
Eduardo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2018-02-02 18:23 ` Eduardo Habkost
@ 2018-02-05 11:22 ` Igor Mammedov
2018-02-05 13:54 ` Eduardo Habkost
0 siblings, 1 reply; 33+ messages in thread
From: Igor Mammedov @ 2018-02-05 11:22 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Alistair Francis, qemu-devel, peter.maydell, f4bug, marcel, alistair23
On Fri, 2 Feb 2018 16:23:26 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:
> > As cpu_type is not a user visible string let's convert the
> > valid_cpu_types to compare against cpu_model instead. This way we have a
> > user friendly string to report back.
> >
> > Once we have a cpu_type to cpu_model conversion this patch should be
> > reverted and we should use cpu_type instead.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > ---
> >
> > hw/core/machine.c | 11 +++++------
> > 1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > index cdc1163dc6..de5bac1c84 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
> > /* If the machine supports the valid_cpu_types check and the user
> > * specified a CPU with -cpu check here that the user CPU is supported.
> > */
> > - if (machine_class->valid_cpu_types && machine->cpu_type) {
> > - ObjectClass *class = object_class_by_name(machine->cpu_type);
> > + if (machine_class->valid_cpu_types && machine->cpu_model) {
> > int i;
> >
> > for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> > - if (object_class_dynamic_cast(class,
> > - machine_class->valid_cpu_types[i])) {
> > + if (!strcmp(machine->cpu_model,
> > + machine_class->valid_cpu_types[i])) {
>
> I would rename valid_cpu_types to valid_cpu_models to make the
> new semantics clearer.
>
> Anyway, I have bad and good news:
>
> The bad news is Igor already sent patches last week that remove
> MachineState::cpu_model, so this conflicts with his series. Now
> parse_cpu_model() will be the only place where the original CPU model name is
> available, but the function needs to work on *-user too. See:
> "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
>
> The good news is that I think we can fix this very easily if
> validation is done at the same place where parse_cpu_model() is
> called. e.g.:
>
> current_machine->cpu_type = machine_class->default_cpu_type;
> if (cpu_model) {
> current_machine->cpu_type = parse_cpu_model(cpu_model);
>
> if (machine_class->valid_cpu_models) {
> ObjectClass *class = object_class_by_name(machine->cpu_type);
> int i;
>
> for (i = 0; machine_class->valid_cpu_models[i]; i++) {
> const char *valid_model = machine_class->valid_cpu_models[i];
> ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
> if (object_class_dynamic_cast(class,
> object_class_get_name(valid_class))) {
> /* Valid CPU type, we're good to go */
> break;
> }
> }
> if (!machine_class->valid_cpu_models[i]) {
> error_report("Invalid CPU model: %s", cpu_model);
> error_printf("The valid CPU models are: %s",
> machine_class->valid_cpu_models[0]);
> for (i = 1; machine_class->valid_cpu_models[i]; i++) {
> error_printf(", %s", machine_class->valid_cpu_models[i]);
> }
> error_printf("\n");
> exit(1);
> }
> }
> }
>
> This can be done inside main(), or moved inside
> machine_run_board_init() if main() pass cpu_model as argument to
> the function.
>
> On either case, I think it's a good idea to do validation and
> printing of error messages closer to the code that parses the
> command-line options. This way we separate parsing/validation
> from initialization.
I agree it's better like you suggest as at least it prevents
ms->cpu_model creeping back into boards code.
But I still dislike (hate) an idea of new code adding non
canonized cpu_model strings back in the boards code.
It's just a matter of time when someone would use them
and cpu_model parsing will creep back into boards.
It would be much better to if we add
char *MachineClass::cpu_name_by_type_name(char *cpu_type)
callback and let machines in this patchset to set it,
something along following lines which is not much of
refactoring and allows for gradual conversion:
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9631670..85cca84 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
return cpu->el_change_hook_opaque;
}
+char *arm_cpu_name_by_type_name(const char *typename);
+
#endif
diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
index f936017..ae6adb7 100644
--- a/hw/arm/netduino2.c
+++ b/hw/arm/netduino2.c
@@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
mc->desc = "Netduino 2 Machine";
mc->init = netduino2_init;
mc->ignore_memory_transaction_failures = true;
+ mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:
}
DEFINE_MACHINE("netduino2", netduino2_machine_init)
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index cc1856c..dacc3cc 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -928,6 +928,11 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
acc->parent_realize(dev, errp);
}
+char *arm_cpu_name_by_type_name(const char *typename)
+{
+ return g_strndup(typename, strlen(typename) - strlen("-" TYPE_ARM_CPU));
+}
+
static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
{
ObjectClass *oc;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index c83c901..e27b7f0 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5373,7 +5373,7 @@ static void arm_cpu_list_entry(gpointer data, gpointer user_data)
char *name;
typename = object_class_get_name(oc);
- name = g_strndup(typename, strlen(typename) - strlen("-" TYPE_ARM_CPU));
+ name = arm_cpu_name_by_type_name(typename);
(*s->cpu_fprintf)(s->file, " %s\n",
name);
g_free(name);
@@ -5410,8 +5410,7 @@ static void arm_cpu_add_definition(gpointer data, gpointer user_data)
typename = object_class_get_name(oc);
info = g_malloc0(sizeof(*info));
- info->name = g_strndup(typename,
- strlen(typename) - strlen("-" TYPE_ARM_CPU));
+ info->name = arm_cpu_name_by_type_name(typename);
info->q_typename = g_strdup(typename);
entry = g_malloc0(sizeof(*entry));
diff --git a/vl.c b/vl.c
index c10b0f4..fda1cb2 100644
--- a/vl.c
+++ b/vl.c
@@ -4646,6 +4646,11 @@ int main(int argc, char **argv, char **envp)
if (cpu_model) {
current_machine->cpu_type =
cpu_parse_cpu_model(machine_class->default_cpu_type, cpu_model);
+
+ if !is_valid_cpu(current_machine->cpu_type)
+ print("valid cpu types: ")
+ for_each_valid_type
+ print(machine_class->cpu_name_by_type_name())
}
}
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2018-02-05 11:22 ` Igor Mammedov
@ 2018-02-05 13:54 ` Eduardo Habkost
2018-02-05 14:42 ` Igor Mammedov
0 siblings, 1 reply; 33+ messages in thread
From: Eduardo Habkost @ 2018-02-05 13:54 UTC (permalink / raw)
To: Igor Mammedov
Cc: Alistair Francis, qemu-devel, peter.maydell, f4bug, marcel, alistair23
On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote:
> On Fri, 2 Feb 2018 16:23:26 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:
> > > As cpu_type is not a user visible string let's convert the
> > > valid_cpu_types to compare against cpu_model instead. This way we have a
> > > user friendly string to report back.
> > >
> > > Once we have a cpu_type to cpu_model conversion this patch should be
> > > reverted and we should use cpu_type instead.
> > >
> > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > > ---
> > >
> > > hw/core/machine.c | 11 +++++------
> > > 1 file changed, 5 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > index cdc1163dc6..de5bac1c84 100644
> > > --- a/hw/core/machine.c
> > > +++ b/hw/core/machine.c
> > > @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
> > > /* If the machine supports the valid_cpu_types check and the user
> > > * specified a CPU with -cpu check here that the user CPU is supported.
> > > */
> > > - if (machine_class->valid_cpu_types && machine->cpu_type) {
> > > - ObjectClass *class = object_class_by_name(machine->cpu_type);
> > > + if (machine_class->valid_cpu_types && machine->cpu_model) {
> > > int i;
> > >
> > > for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> > > - if (object_class_dynamic_cast(class,
> > > - machine_class->valid_cpu_types[i])) {
> > > + if (!strcmp(machine->cpu_model,
> > > + machine_class->valid_cpu_types[i])) {
> >
> > I would rename valid_cpu_types to valid_cpu_models to make the
> > new semantics clearer.
> >
> > Anyway, I have bad and good news:
> >
> > The bad news is Igor already sent patches last week that remove
> > MachineState::cpu_model, so this conflicts with his series. Now
> > parse_cpu_model() will be the only place where the original CPU model name is
> > available, but the function needs to work on *-user too. See:
> > "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
> >
> > The good news is that I think we can fix this very easily if
> > validation is done at the same place where parse_cpu_model() is
> > called. e.g.:
> >
> > current_machine->cpu_type = machine_class->default_cpu_type;
> > if (cpu_model) {
> > current_machine->cpu_type = parse_cpu_model(cpu_model);
> >
> > if (machine_class->valid_cpu_models) {
> > ObjectClass *class = object_class_by_name(machine->cpu_type);
> > int i;
> >
> > for (i = 0; machine_class->valid_cpu_models[i]; i++) {
> > const char *valid_model = machine_class->valid_cpu_models[i];
> > ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
> > if (object_class_dynamic_cast(class,
> > object_class_get_name(valid_class))) {
> > /* Valid CPU type, we're good to go */
> > break;
> > }
> > }
> > if (!machine_class->valid_cpu_models[i]) {
> > error_report("Invalid CPU model: %s", cpu_model);
> > error_printf("The valid CPU models are: %s",
> > machine_class->valid_cpu_models[0]);
> > for (i = 1; machine_class->valid_cpu_models[i]; i++) {
> > error_printf(", %s", machine_class->valid_cpu_models[i]);
> > }
> > error_printf("\n");
> > exit(1);
> > }
> > }
> > }
> >
> > This can be done inside main(), or moved inside
> > machine_run_board_init() if main() pass cpu_model as argument to
> > the function.
> >
> > On either case, I think it's a good idea to do validation and
> > printing of error messages closer to the code that parses the
> > command-line options. This way we separate parsing/validation
> > from initialization.
> I agree it's better like you suggest as at least it prevents
> ms->cpu_model creeping back into boards code.
>
> But I still dislike (hate) an idea of new code adding non
> canonized cpu_model strings back in the boards code.
> It's just a matter of time when someone would use them
> and cpu_model parsing will creep back into boards.
>
> It would be much better to if we add
> char *MachineClass::cpu_name_by_type_name(char *cpu_type)
> callback and let machines in this patchset to set it,
> something along following lines which is not much of
> refactoring and allows for gradual conversion:
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9631670..85cca84 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
> return cpu->el_change_hook_opaque;
> }
>
> +char *arm_cpu_name_by_type_name(const char *typename);
> +
> #endif
> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> index f936017..ae6adb7 100644
> --- a/hw/arm/netduino2.c
> +++ b/hw/arm/netduino2.c
> @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
> mc->desc = "Netduino 2 Machine";
> mc->init = netduino2_init;
> mc->ignore_memory_transaction_failures = true;
> + mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:
I really don't want to introduce a new arch-specific hook just
for that. We should move CPU type lookup logic to common code
and make it unnecessary to write new hooks.
I agree it would be better if we had a cpu_name_by_type_name()
function, but I would like to have it implemented cleanly.
--
Eduardo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2018-02-05 13:54 ` Eduardo Habkost
@ 2018-02-05 14:42 ` Igor Mammedov
2018-02-05 22:42 ` Eduardo Habkost
0 siblings, 1 reply; 33+ messages in thread
From: Igor Mammedov @ 2018-02-05 14:42 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Alistair Francis, qemu-devel, peter.maydell, f4bug, marcel, alistair23
On Mon, 5 Feb 2018 11:54:01 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote:
> > On Fri, 2 Feb 2018 16:23:26 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > > On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:
> > > > As cpu_type is not a user visible string let's convert the
> > > > valid_cpu_types to compare against cpu_model instead. This way we have a
> > > > user friendly string to report back.
> > > >
> > > > Once we have a cpu_type to cpu_model conversion this patch should be
> > > > reverted and we should use cpu_type instead.
> > > >
> > > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > > > ---
> > > >
> > > > hw/core/machine.c | 11 +++++------
> > > > 1 file changed, 5 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > index cdc1163dc6..de5bac1c84 100644
> > > > --- a/hw/core/machine.c
> > > > +++ b/hw/core/machine.c
> > > > @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
> > > > /* If the machine supports the valid_cpu_types check and the user
> > > > * specified a CPU with -cpu check here that the user CPU is supported.
> > > > */
> > > > - if (machine_class->valid_cpu_types && machine->cpu_type) {
> > > > - ObjectClass *class = object_class_by_name(machine->cpu_type);
> > > > + if (machine_class->valid_cpu_types && machine->cpu_model) {
> > > > int i;
> > > >
> > > > for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> > > > - if (object_class_dynamic_cast(class,
> > > > - machine_class->valid_cpu_types[i])) {
> > > > + if (!strcmp(machine->cpu_model,
> > > > + machine_class->valid_cpu_types[i])) {
> > >
> > > I would rename valid_cpu_types to valid_cpu_models to make the
> > > new semantics clearer.
> > >
> > > Anyway, I have bad and good news:
> > >
> > > The bad news is Igor already sent patches last week that remove
> > > MachineState::cpu_model, so this conflicts with his series. Now
> > > parse_cpu_model() will be the only place where the original CPU model name is
> > > available, but the function needs to work on *-user too. See:
> > > "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
> > >
> > > The good news is that I think we can fix this very easily if
> > > validation is done at the same place where parse_cpu_model() is
> > > called. e.g.:
> > >
> > > current_machine->cpu_type = machine_class->default_cpu_type;
> > > if (cpu_model) {
> > > current_machine->cpu_type = parse_cpu_model(cpu_model);
> > >
> > > if (machine_class->valid_cpu_models) {
> > > ObjectClass *class = object_class_by_name(machine->cpu_type);
> > > int i;
> > >
> > > for (i = 0; machine_class->valid_cpu_models[i]; i++) {
> > > const char *valid_model = machine_class->valid_cpu_models[i];
> > > ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
> > > if (object_class_dynamic_cast(class,
> > > object_class_get_name(valid_class))) {
> > > /* Valid CPU type, we're good to go */
> > > break;
> > > }
> > > }
> > > if (!machine_class->valid_cpu_models[i]) {
> > > error_report("Invalid CPU model: %s", cpu_model);
> > > error_printf("The valid CPU models are: %s",
> > > machine_class->valid_cpu_models[0]);
> > > for (i = 1; machine_class->valid_cpu_models[i]; i++) {
> > > error_printf(", %s", machine_class->valid_cpu_models[i]);
> > > }
> > > error_printf("\n");
> > > exit(1);
> > > }
> > > }
> > > }
> > >
> > > This can be done inside main(), or moved inside
> > > machine_run_board_init() if main() pass cpu_model as argument to
> > > the function.
> > >
> > > On either case, I think it's a good idea to do validation and
> > > printing of error messages closer to the code that parses the
> > > command-line options. This way we separate parsing/validation
> > > from initialization.
> > I agree it's better like you suggest as at least it prevents
> > ms->cpu_model creeping back into boards code.
> >
> > But I still dislike (hate) an idea of new code adding non
> > canonized cpu_model strings back in the boards code.
> > It's just a matter of time when someone would use them
> > and cpu_model parsing will creep back into boards.
> >
> > It would be much better to if we add
> > char *MachineClass::cpu_name_by_type_name(char *cpu_type)
> > callback and let machines in this patchset to set it,
> > something along following lines which is not much of
> > refactoring and allows for gradual conversion:
> >
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 9631670..85cca84 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
> > return cpu->el_change_hook_opaque;
> > }
> >
> > +char *arm_cpu_name_by_type_name(const char *typename);
> > +
> > #endif
> > diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> > index f936017..ae6adb7 100644
> > --- a/hw/arm/netduino2.c
> > +++ b/hw/arm/netduino2.c
> > @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
> > mc->desc = "Netduino 2 Machine";
> > mc->init = netduino2_init;
> > mc->ignore_memory_transaction_failures = true;
> > + mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:
>
> I really don't want to introduce a new arch-specific hook just
> for that. We should move CPU type lookup logic to common code
> and make it unnecessary to write new hooks.
unfortunately cpu_model (cpu name part) is target specific
and it's translation to type and back is target specific mayhem.
So I'd prefer having both back and forth functions together in
one place. And common code to call them when necessary.
We could do global cpu_name_by_type_name() instead of hook,
which I'd prefer even more but then conversion can't be done
only for one target but rather for all targets at once.
> I agree it would be better if we had a cpu_name_by_type_name()
> function, but I would like to have it implemented cleanly.
In some cases(targets) it can be common helper, but in other
cases it's not so.
My suggestion though allows to do gradual conversion and
avoid putting cpu_model names back in board's code (which I just manged to remove).
Once all targets converted and relevant code is isolated
we can attempt to generalize it if it's possible or at least
make of it global per target helper to get rid of
temporary machine hook.
(seeing this series reposted with cpu_model names in boards code,
it doesn't looks like author would like to implement tree-wide
generalization first)
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2018-02-05 14:42 ` Igor Mammedov
@ 2018-02-05 22:42 ` Eduardo Habkost
2018-02-06 14:43 ` Igor Mammedov
0 siblings, 1 reply; 33+ messages in thread
From: Eduardo Habkost @ 2018-02-05 22:42 UTC (permalink / raw)
To: Igor Mammedov
Cc: Alistair Francis, qemu-devel, peter.maydell, f4bug, marcel, alistair23
On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote:
> On Mon, 5 Feb 2018 11:54:01 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote:
> > > On Fri, 2 Feb 2018 16:23:26 -0200
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >
> > > > On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:
> > > > > As cpu_type is not a user visible string let's convert the
> > > > > valid_cpu_types to compare against cpu_model instead. This way we have a
> > > > > user friendly string to report back.
> > > > >
> > > > > Once we have a cpu_type to cpu_model conversion this patch should be
> > > > > reverted and we should use cpu_type instead.
> > > > >
> > > > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > > > > ---
> > > > >
> > > > > hw/core/machine.c | 11 +++++------
> > > > > 1 file changed, 5 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > index cdc1163dc6..de5bac1c84 100644
> > > > > --- a/hw/core/machine.c
> > > > > +++ b/hw/core/machine.c
> > > > > @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
> > > > > /* If the machine supports the valid_cpu_types check and the user
> > > > > * specified a CPU with -cpu check here that the user CPU is supported.
> > > > > */
> > > > > - if (machine_class->valid_cpu_types && machine->cpu_type) {
> > > > > - ObjectClass *class = object_class_by_name(machine->cpu_type);
> > > > > + if (machine_class->valid_cpu_types && machine->cpu_model) {
> > > > > int i;
> > > > >
> > > > > for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> > > > > - if (object_class_dynamic_cast(class,
> > > > > - machine_class->valid_cpu_types[i])) {
> > > > > + if (!strcmp(machine->cpu_model,
> > > > > + machine_class->valid_cpu_types[i])) {
> > > >
> > > > I would rename valid_cpu_types to valid_cpu_models to make the
> > > > new semantics clearer.
> > > >
> > > > Anyway, I have bad and good news:
> > > >
> > > > The bad news is Igor already sent patches last week that remove
> > > > MachineState::cpu_model, so this conflicts with his series. Now
> > > > parse_cpu_model() will be the only place where the original CPU model name is
> > > > available, but the function needs to work on *-user too. See:
> > > > "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
> > > >
> > > > The good news is that I think we can fix this very easily if
> > > > validation is done at the same place where parse_cpu_model() is
> > > > called. e.g.:
> > > >
> > > > current_machine->cpu_type = machine_class->default_cpu_type;
> > > > if (cpu_model) {
> > > > current_machine->cpu_type = parse_cpu_model(cpu_model);
> > > >
> > > > if (machine_class->valid_cpu_models) {
> > > > ObjectClass *class = object_class_by_name(machine->cpu_type);
> > > > int i;
> > > >
> > > > for (i = 0; machine_class->valid_cpu_models[i]; i++) {
> > > > const char *valid_model = machine_class->valid_cpu_models[i];
> > > > ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
> > > > if (object_class_dynamic_cast(class,
> > > > object_class_get_name(valid_class))) {
> > > > /* Valid CPU type, we're good to go */
> > > > break;
> > > > }
> > > > }
> > > > if (!machine_class->valid_cpu_models[i]) {
> > > > error_report("Invalid CPU model: %s", cpu_model);
> > > > error_printf("The valid CPU models are: %s",
> > > > machine_class->valid_cpu_models[0]);
> > > > for (i = 1; machine_class->valid_cpu_models[i]; i++) {
> > > > error_printf(", %s", machine_class->valid_cpu_models[i]);
> > > > }
> > > > error_printf("\n");
> > > > exit(1);
> > > > }
> > > > }
> > > > }
> > > >
> > > > This can be done inside main(), or moved inside
> > > > machine_run_board_init() if main() pass cpu_model as argument to
> > > > the function.
> > > >
> > > > On either case, I think it's a good idea to do validation and
> > > > printing of error messages closer to the code that parses the
> > > > command-line options. This way we separate parsing/validation
> > > > from initialization.
> > > I agree it's better like you suggest as at least it prevents
> > > ms->cpu_model creeping back into boards code.
> > >
> > > But I still dislike (hate) an idea of new code adding non
> > > canonized cpu_model strings back in the boards code.
> > > It's just a matter of time when someone would use them
> > > and cpu_model parsing will creep back into boards.
> > >
> > > It would be much better to if we add
> > > char *MachineClass::cpu_name_by_type_name(char *cpu_type)
> > > callback and let machines in this patchset to set it,
> > > something along following lines which is not much of
> > > refactoring and allows for gradual conversion:
> > >
> > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > > index 9631670..85cca84 100644
> > > --- a/target/arm/cpu.h
> > > +++ b/target/arm/cpu.h
> > > @@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
> > > return cpu->el_change_hook_opaque;
> > > }
> > >
> > > +char *arm_cpu_name_by_type_name(const char *typename);
> > > +
> > > #endif
> > > diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> > > index f936017..ae6adb7 100644
> > > --- a/hw/arm/netduino2.c
> > > +++ b/hw/arm/netduino2.c
> > > @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
> > > mc->desc = "Netduino 2 Machine";
> > > mc->init = netduino2_init;
> > > mc->ignore_memory_transaction_failures = true;
> > > + mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:
> >
> > I really don't want to introduce a new arch-specific hook just
> > for that. We should move CPU type lookup logic to common code
> > and make it unnecessary to write new hooks.
> unfortunately cpu_model (cpu name part) is target specific
> and it's translation to type and back is target specific mayhem.
Why can't the model<->type translation be represented as data?
We could have simple cpu_type_name_suffix + an alias table.
We have at least 4 arches that return a constant at
class_by_name. We have at least 10 arches that simply add a
suffix to the CPU model name. We must make them use common code
instead of requiring them to implement yet another hook[1].
In addition to the ones above, we have 3 that seem to just need
an alias table (cris, superh, alpha). ppc can probably also use
an alias table for the ppc_cpu_class_by_pvr() stuff. sparc just
needs whitespaces translated to '-' (sparc), which can be done
using an alias table.
In the end I couldn't find any example that can't be represented
by a suffix + alias table.
>
> So I'd prefer having both back and forth functions together in
> one place. And common code to call them when necessary.
>
> We could do global cpu_name_by_type_name() instead of hook,
> which I'd prefer even more but then conversion can't be done
> only for one target but rather for all targets at once.
I don't mind letting a few targets override default behavior with
a hook if really necessary, but I have a problem with requiring
all targets to implement what's basically the same boilerplate
code to add/remove a string suffix and translating aliases.
>
> > I agree it would be better if we had a cpu_name_by_type_name()
> > function, but I would like to have it implemented cleanly.
> In some cases(targets) it can be common helper, but in other
> cases it's not so.
> My suggestion though allows to do gradual conversion and
> avoid putting cpu_model names back in board's code (which I just manged to remove).
> Once all targets converted and relevant code is isolated
> we can attempt to generalize it if it's possible or at least
> make of it global per target helper to get rid of
> temporary machine hook.
>
> (seeing this series reposted with cpu_model names in boards code,
> it doesn't looks like author would like to implement tree-wide
> generalization first)
Well, if nobody is willing to generalize all target-specific code
right now, I don't see the harm in having cpu_model-based tables
in a few boards in the meantime (as this patch series does). But
I do see harm in requiring all our 20 targets to implement yet
another hook and increasing the costs of cleaning up the mess
later.
---
[1] Really, this amount of duplication is insane:
static char *mips_cpu_type_name(const char *cpu_model)
{
return g_strdup_printf(MIPS_CPU_TYPE_NAME("%s"), cpu_model);
}
static ObjectClass *mips_cpu_class_by_name(const char *cpu_model)
{
ObjectClass *oc;
char *typename;
typename = mips_cpu_type_name(cpu_model);
oc = object_class_by_name(typename);
g_free(typename);
return oc;
}
static ObjectClass *tricore_cpu_class_by_name(const char *cpu_model)
{
ObjectClass *oc;
char *typename;
typename = g_strdup_printf(TRICORE_CPU_TYPE_NAME("%s"), cpu_model);
oc = object_class_by_name(typename);
g_free(typename);
if (!oc || !object_class_dynamic_cast(oc, TYPE_TRICORE_CPU) ||
object_class_is_abstract(oc)) {
return NULL;
}
return oc;
}
static ObjectClass *uc32_cpu_class_by_name(const char *cpu_model)
{
ObjectClass *oc;
char *typename;
typename = g_strdup_printf(UNICORE32_CPU_TYPE_NAME("%s"), cpu_model);
oc = object_class_by_name(typename);
g_free(typename);
if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_UNICORE32_CPU) ||
object_class_is_abstract(oc))) {
oc = NULL;
}
return oc;
}
static ObjectClass *xtensa_cpu_class_by_name(const char *cpu_model)
{
ObjectClass *oc;
char *typename;
typename = g_strdup_printf(XTENSA_CPU_TYPE_NAME("%s"), cpu_model);
oc = object_class_by_name(typename);
g_free(typename);
if (oc == NULL || !object_class_dynamic_cast(oc, TYPE_XTENSA_CPU) ||
object_class_is_abstract(oc)) {
return NULL;
}
return oc;
}
static char *x86_cpu_type_name(const char *model_name)
{
return g_strdup_printf(X86_CPU_TYPE_NAME("%s"), model_name);
}
static ObjectClass *x86_cpu_class_by_name(const char *cpu_model)
{
ObjectClass *oc;
char *typename;
if (cpu_model == NULL) {
return NULL;
}
typename = x86_cpu_type_name(cpu_model);
oc = object_class_by_name(typename);
g_free(typename);
return oc;
}
static ObjectClass *arm_cpu_class_by_name(const char *cpu_model)
{
ObjectClass *oc;
char *typename;
char **cpuname;
cpuname = g_strsplit(cpu_model, ",", 1);
typename = g_strdup_printf(ARM_CPU_TYPE_NAME("%s"), cpuname[0]);
oc = object_class_by_name(typename);
g_strfreev(cpuname);
g_free(typename);
if (!oc || !object_class_dynamic_cast(oc, TYPE_ARM_CPU) ||
object_class_is_abstract(oc)) {
return NULL;
}
return oc;
}
static ObjectClass *lm32_cpu_class_by_name(const char *cpu_model)
{
ObjectClass *oc;
char *typename;
typename = g_strdup_printf(LM32_CPU_TYPE_NAME("%s"), cpu_model);
oc = object_class_by_name(typename);
g_free(typename);
if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_LM32_CPU) ||
object_class_is_abstract(oc))) {
oc = NULL;
}
return oc;
}
static ObjectClass *m68k_cpu_class_by_name(const char *cpu_model)
{
ObjectClass *oc;
char *typename;
typename = g_strdup_printf(M68K_CPU_TYPE_NAME("%s"), cpu_model);
oc = object_class_by_name(typename);
g_free(typename);
if (oc != NULL && (object_class_dynamic_cast(oc, TYPE_M68K_CPU) == NULL ||
object_class_is_abstract(oc))) {
return NULL;
}
return oc;
}
static ObjectClass *moxie_cpu_class_by_name(const char *cpu_model)
{
ObjectClass *oc;
char *typename;
typename = g_strdup_printf(MOXIE_CPU_TYPE_NAME("%s"), cpu_model);
oc = object_class_by_name(typename);
g_free(typename);
if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_MOXIE_CPU) ||
object_class_is_abstract(oc))) {
return NULL;
}
return oc;
}
static ObjectClass *openrisc_cpu_class_by_name(const char *cpu_model)
{
ObjectClass *oc;
char *typename;
typename = g_strdup_printf(OPENRISC_CPU_TYPE_NAME("%s"), cpu_model);
oc = object_class_by_name(typename);
g_free(typename);
if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_OPENRISC_CPU) ||
object_class_is_abstract(oc))) {
return NULL;
}
return oc;
}
--
Eduardo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2018-02-05 22:42 ` Eduardo Habkost
@ 2018-02-06 14:43 ` Igor Mammedov
2019-06-17 5:09 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 33+ messages in thread
From: Igor Mammedov @ 2018-02-06 14:43 UTC (permalink / raw)
To: Eduardo Habkost
Cc: Alistair Francis, qemu-devel, peter.maydell, f4bug, marcel, alistair23
On Mon, 5 Feb 2018 20:42:05 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote:
> > On Mon, 5 Feb 2018 11:54:01 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > > On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote:
> > > > On Fri, 2 Feb 2018 16:23:26 -0200
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >
> > > > > On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:
> > > > > > As cpu_type is not a user visible string let's convert the
> > > > > > valid_cpu_types to compare against cpu_model instead. This way we have a
> > > > > > user friendly string to report back.
> > > > > >
> > > > > > Once we have a cpu_type to cpu_model conversion this patch should be
> > > > > > reverted and we should use cpu_type instead.
> > > > > >
> > > > > > Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> > > > > > ---
> > > > > >
> > > > > > hw/core/machine.c | 11 +++++------
> > > > > > 1 file changed, 5 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > > index cdc1163dc6..de5bac1c84 100644
> > > > > > --- a/hw/core/machine.c
> > > > > > +++ b/hw/core/machine.c
> > > > > > @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
> > > > > > /* If the machine supports the valid_cpu_types check and the user
> > > > > > * specified a CPU with -cpu check here that the user CPU is supported.
> > > > > > */
> > > > > > - if (machine_class->valid_cpu_types && machine->cpu_type) {
> > > > > > - ObjectClass *class = object_class_by_name(machine->cpu_type);
> > > > > > + if (machine_class->valid_cpu_types && machine->cpu_model) {
> > > > > > int i;
> > > > > >
> > > > > > for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> > > > > > - if (object_class_dynamic_cast(class,
> > > > > > - machine_class->valid_cpu_types[i])) {
> > > > > > + if (!strcmp(machine->cpu_model,
> > > > > > + machine_class->valid_cpu_types[i])) {
> > > > >
> > > > > I would rename valid_cpu_types to valid_cpu_models to make the
> > > > > new semantics clearer.
> > > > >
> > > > > Anyway, I have bad and good news:
> > > > >
> > > > > The bad news is Igor already sent patches last week that remove
> > > > > MachineState::cpu_model, so this conflicts with his series. Now
> > > > > parse_cpu_model() will be the only place where the original CPU model name is
> > > > > available, but the function needs to work on *-user too. See:
> > > > > "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
> > > > >
> > > > > The good news is that I think we can fix this very easily if
> > > > > validation is done at the same place where parse_cpu_model() is
> > > > > called. e.g.:
> > > > >
> > > > > current_machine->cpu_type = machine_class->default_cpu_type;
> > > > > if (cpu_model) {
> > > > > current_machine->cpu_type = parse_cpu_model(cpu_model);
> > > > >
> > > > > if (machine_class->valid_cpu_models) {
> > > > > ObjectClass *class = object_class_by_name(machine->cpu_type);
> > > > > int i;
> > > > >
> > > > > for (i = 0; machine_class->valid_cpu_models[i]; i++) {
> > > > > const char *valid_model = machine_class->valid_cpu_models[i];
> > > > > ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
> > > > > if (object_class_dynamic_cast(class,
> > > > > object_class_get_name(valid_class))) {
> > > > > /* Valid CPU type, we're good to go */
> > > > > break;
> > > > > }
> > > > > }
> > > > > if (!machine_class->valid_cpu_models[i]) {
> > > > > error_report("Invalid CPU model: %s", cpu_model);
> > > > > error_printf("The valid CPU models are: %s",
> > > > > machine_class->valid_cpu_models[0]);
> > > > > for (i = 1; machine_class->valid_cpu_models[i]; i++) {
> > > > > error_printf(", %s", machine_class->valid_cpu_models[i]);
> > > > > }
> > > > > error_printf("\n");
> > > > > exit(1);
> > > > > }
> > > > > }
> > > > > }
> > > > >
> > > > > This can be done inside main(), or moved inside
> > > > > machine_run_board_init() if main() pass cpu_model as argument to
> > > > > the function.
> > > > >
> > > > > On either case, I think it's a good idea to do validation and
> > > > > printing of error messages closer to the code that parses the
> > > > > command-line options. This way we separate parsing/validation
> > > > > from initialization.
> > > > I agree it's better like you suggest as at least it prevents
> > > > ms->cpu_model creeping back into boards code.
> > > >
> > > > But I still dislike (hate) an idea of new code adding non
> > > > canonized cpu_model strings back in the boards code.
> > > > It's just a matter of time when someone would use them
> > > > and cpu_model parsing will creep back into boards.
> > > >
> > > > It would be much better to if we add
> > > > char *MachineClass::cpu_name_by_type_name(char *cpu_type)
> > > > callback and let machines in this patchset to set it,
> > > > something along following lines which is not much of
> > > > refactoring and allows for gradual conversion:
> > > >
> > > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > > > index 9631670..85cca84 100644
> > > > --- a/target/arm/cpu.h
> > > > +++ b/target/arm/cpu.h
> > > > @@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
> > > > return cpu->el_change_hook_opaque;
> > > > }
> > > >
> > > > +char *arm_cpu_name_by_type_name(const char *typename);
> > > > +
> > > > #endif
> > > > diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> > > > index f936017..ae6adb7 100644
> > > > --- a/hw/arm/netduino2.c
> > > > +++ b/hw/arm/netduino2.c
> > > > @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
> > > > mc->desc = "Netduino 2 Machine";
> > > > mc->init = netduino2_init;
> > > > mc->ignore_memory_transaction_failures = true;
> > > > + mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:
> > >
> > > I really don't want to introduce a new arch-specific hook just
> > > for that. We should move CPU type lookup logic to common code
> > > and make it unnecessary to write new hooks.
> > unfortunately cpu_model (cpu name part) is target specific
> > and it's translation to type and back is target specific mayhem.
>
> Why can't the model<->type translation be represented as data?
> We could have simple cpu_type_name_suffix + an alias table.
>
> We have at least 4 arches that return a constant at
> class_by_name. We have at least 10 arches that simply add a
> suffix to the CPU model name. We must make them use common code
> instead of requiring them to implement yet another hook[1].
True, some of them could use generic hook and reduce
code duplication greatly, we should do it regardless of whether
table or target specific func approach is used.
> In addition to the ones above, we have 3 that seem to just need
> an alias table (cris, superh, alpha). ppc can probably also use
> an alias table for the ppc_cpu_class_by_pvr() stuff. sparc just
> needs whitespaces translated to '-' (sparc), which can be done
> using an alias table.
>
> In the end I couldn't find any example that can't be represented
> by a suffix + alias table.
Table based approach is possible but it won't be as simple
as you've just pictured it.
From what I recall from cpu_class_by_name cleanups table should be able
to describe cases like (sometimes combination of them):
* 1:1 mapping - where cpu_model == cpu_type
* cpu_model <==> cpu_model + suffix - most common usecase
* cpu_model <==> prefix cpu_model - riscv patches on list are trying to add such cpu types
* NULL => some_fixed type
* case (in) sensitive flag
* garbage => some_fixed type
* substitutions
* aliases (sometimes dynamic depending on --enable-kvm (PPC))
Maybe something else.
We can think about it at leisure but I can't say if new approach
complexity it's worth of the effort.
It would be nice see impl, but it's a lot of refactoring that's
clearly out of scope of this series.
I'd prefer small incremental refactoring (if possible) that
won't scare people of and easy to review vs a huge one.
> > So I'd prefer having both back and forth functions together in
> > one place. And common code to call them when necessary.
> >
> > We could do global cpu_name_by_type_name() instead of hook,
> > which I'd prefer even more but then conversion can't be done
> > only for one target but rather for all targets at once.
>
> I don't mind letting a few targets override default behavior with
> a hook if really necessary, but I have a problem with requiring
> all targets to implement what's basically the same boilerplate
> code to add/remove a string suffix and translating aliases.
it could be generic helper if target does the same plus
not mandatory at that (in case target/board doesn't care
about valid cpus).
> > > I agree it would be better if we had a cpu_name_by_type_name()
> > > function, but I would like to have it implemented cleanly.
> > In some cases(targets) it can be common helper, but in other
> > cases it's not so.
> > My suggestion though allows to do gradual conversion and
> > avoid putting cpu_model names back in board's code (which I just manged to remove).
> > Once all targets converted and relevant code is isolated
> > we can attempt to generalize it if it's possible or at least
> > make of it global per target helper to get rid of
> > temporary machine hook.
> >
> > (seeing this series reposted with cpu_model names in boards code,
> > it doesn't looks like author would like to implement tree-wide
> > generalization first)
>
> Well, if nobody is willing to generalize all target-specific code
> right now, I don't see the harm in having cpu_model-based tables
> in a few boards in the meantime (as this patch series does). But
> I do see harm in requiring all our 20 targets to implement yet
> another hook and increasing the costs of cleaning up the mess
> later.
If we use MachineClass hook then it might be done per target
on demand, so no one would require that every target should
implement it.
Also there could be a generic helper for targets that do the same.
Machine which needs to enable valid_cpus, will have to use generic
hook impl or provide target specific if it's special case.
Though I do see harm in adding cpu_model tables in boards code
vs target specific hooks on demand as that will be copy-pasted
in other boards afterwards (number of which is bigger compared
to targets count) and ultimately it would duplicate cpu_name
strings in every board vs hook approach where cpu_model could
be calculated from cpu_type by a function (generic or
target specific).
Good thing about hook is that it's non intrusive and
isolates(consolidates) existing cpu_type -> cpu_model
conversion in multiple places into one place.
Then later it would be easier to generalize if someone
decides to do it.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs Alistair Francis
@ 2018-02-15 11:29 ` Peter Maydell
2018-02-15 13:04 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2018-02-15 11:29 UTC (permalink / raw)
To: Alistair Francis
Cc: QEMU Developers, Alistair Francis, Eduardo Habkost,
Marcel Apfelbaum, Igor Mammedov, Philippe Mathieu-Daudé
On 2 February 2018 at 00:42, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> List all possible valid CPU options.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>
> V5:
> - Use cpu_model names
> V4:
> - Remove spaces
> V3:
> - Add static property
> V2:
> - Fix the indentation
>
> hw/arm/raspi.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> index cd5fa8c3dc..745a880726 100644
> --- a/hw/arm/raspi.c
> +++ b/hw/arm/raspi.c
> @@ -158,6 +158,11 @@ static void raspi2_init(MachineState *machine)
> setup_boot(machine, 2, machine->ram_size - vcram_size);
> }
>
> +static const char *raspi2_valid_cpus[] = {
> + "cortex-a7",
> + NULL
> +};
Is this definitely right? Looking at the code, the raspi2 board
creates a TYPE_BCM2836, and that creates cortex-a15 CPUs...
thanks
-- PMM
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs
2018-02-15 11:29 ` Peter Maydell
@ 2018-02-15 13:04 ` Philippe Mathieu-Daudé
2018-02-15 13:17 ` Peter Maydell
0 siblings, 1 reply; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-15 13:04 UTC (permalink / raw)
To: Peter Maydell, Alistair Francis
Cc: QEMU Developers, Alistair Francis, Eduardo Habkost,
Marcel Apfelbaum, Igor Mammedov
Hi Peter,
On 02/15/2018 08:29 AM, Peter Maydell wrote:
> On 2 February 2018 at 00:42, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> List all possible valid CPU options.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>
>> V5:
>> - Use cpu_model names
>> V4:
>> - Remove spaces
>> V3:
>> - Add static property
>> V2:
>> - Fix the indentation
>>
>> hw/arm/raspi.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
>> index cd5fa8c3dc..745a880726 100644
>> --- a/hw/arm/raspi.c
>> +++ b/hw/arm/raspi.c
>> @@ -158,6 +158,11 @@ static void raspi2_init(MachineState *machine)
>> setup_boot(machine, 2, machine->ram_size - vcram_size);
>> }
>>
>> +static const char *raspi2_valid_cpus[] = {
>> + "cortex-a7",
>> + NULL
>> +};
>
> Is this definitely right? Looking at the code, the raspi2 board
> creates a TYPE_BCM2836, and that creates cortex-a15 CPUs...
The BCM2836 use a cortex-a7 but this cpu was not available at the time
of this commit (bad5623690b) and was added later in dcf578ed8ce "The A7
is very similar to the A15."
I can prepare a patch for it to apply before this series.
>
> thanks
> -- PMM
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs
2018-02-15 13:04 ` Philippe Mathieu-Daudé
@ 2018-02-15 13:17 ` Peter Maydell
2018-02-15 17:08 ` Igor Mammedov
0 siblings, 1 reply; 33+ messages in thread
From: Peter Maydell @ 2018-02-15 13:17 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alistair Francis, QEMU Developers, Alistair Francis,
Eduardo Habkost, Marcel Apfelbaum, Igor Mammedov
On 15 February 2018 at 13:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Peter,
>
> On 02/15/2018 08:29 AM, Peter Maydell wrote:
>> On 2 February 2018 at 00:42, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>>> List all possible valid CPU options.
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>
>>> V5:
>>> - Use cpu_model names
>>> V4:
>>> - Remove spaces
>>> V3:
>>> - Add static property
>>> V2:
>>> - Fix the indentation
>>>
>>> hw/arm/raspi.c | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>>
>>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
>>> index cd5fa8c3dc..745a880726 100644
>>> --- a/hw/arm/raspi.c
>>> +++ b/hw/arm/raspi.c
>>> @@ -158,6 +158,11 @@ static void raspi2_init(MachineState *machine)
>>> setup_boot(machine, 2, machine->ram_size - vcram_size);
>>> }
>>>
>>> +static const char *raspi2_valid_cpus[] = {
>>> + "cortex-a7",
>>> + NULL
>>> +};
>>
>> Is this definitely right? Looking at the code, the raspi2 board
>> creates a TYPE_BCM2836, and that creates cortex-a15 CPUs...
>
> The BCM2836 use a cortex-a7 but this cpu was not available at the time
> of this commit (bad5623690b) and was added later in dcf578ed8ce "The A7
> is very similar to the A15."
>
> I can prepare a patch for it to apply before this series.
I noticed after I'd written that comment that patch 3 in this
series does exactly the change to cortex-a7...
thanks
-- PMM
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 3/6] bcm2836: Use the Cortex-A7 instead of Cortex-A15
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 3/6] bcm2836: Use the Cortex-A7 instead of Cortex-A15 Alistair Francis
@ 2018-02-15 13:23 ` Philippe Mathieu-Daudé
2018-02-15 22:41 ` Alistair Francis
0 siblings, 1 reply; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-02-15 13:23 UTC (permalink / raw)
To: peter.maydell
Cc: Alistair Francis, qemu-devel, alistair23, ehabkost, marcel, imammedo
Hi Peter,
On 02/01/2018 09:42 PM, Alistair Francis wrote:
> The BCM2836 uses a Cortex-A7 not a Cortex-A15. Update the device to use
> the correct CPU.
> https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/QA7_rev3.4.pdf
Can you add these lines with reference to the commits?
When the BCM2836 was introduced (bad5623690b) the Cortex-A7 was not
available, so the very similar Cortex-A15 was used.
Since dcf578ed8ce we can model the correct core.
Thanks!
Phil.
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> V3:
> - Use ARM_CPU_TYPE_NAME() macro
> V2:
> - Fix the BCM2836 CPU
>
> hw/arm/bcm2836.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
> index 8c43291112..c477772484 100644
> --- a/hw/arm/bcm2836.c
> +++ b/hw/arm/bcm2836.c
> @@ -30,7 +30,7 @@ static void bcm2836_init(Object *obj)
>
> for (n = 0; n < BCM2836_NCPUS; n++) {
> object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
> - "cortex-a15-" TYPE_ARM_CPU);
> + ARM_CPU_TYPE_NAME("cortex-a7"));
> object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]),
> &error_abort);
> }
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs
2018-02-15 13:17 ` Peter Maydell
@ 2018-02-15 17:08 ` Igor Mammedov
0 siblings, 0 replies; 33+ messages in thread
From: Igor Mammedov @ 2018-02-15 17:08 UTC (permalink / raw)
To: Peter Maydell
Cc: Philippe Mathieu-Daudé,
Alistair Francis, QEMU Developers, Alistair Francis,
Eduardo Habkost, Marcel Apfelbaum
On Thu, 15 Feb 2018 13:17:50 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:
> On 15 February 2018 at 13:04, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > Hi Peter,
> >
> > On 02/15/2018 08:29 AM, Peter Maydell wrote:
> >> On 2 February 2018 at 00:42, Alistair Francis
> >> <alistair.francis@xilinx.com> wrote:
> >>> List all possible valid CPU options.
> >>>
> >>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>> ---
> >>>
> >>> V5:
> >>> - Use cpu_model names
> >>> V4:
> >>> - Remove spaces
> >>> V3:
> >>> - Add static property
> >>> V2:
> >>> - Fix the indentation
> >>>
> >>> hw/arm/raspi.c | 7 +++++++
> >>> 1 file changed, 7 insertions(+)
> >>>
> >>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
> >>> index cd5fa8c3dc..745a880726 100644
> >>> --- a/hw/arm/raspi.c
> >>> +++ b/hw/arm/raspi.c
> >>> @@ -158,6 +158,11 @@ static void raspi2_init(MachineState *machine)
> >>> setup_boot(machine, 2, machine->ram_size - vcram_size);
> >>> }
> >>>
> >>> +static const char *raspi2_valid_cpus[] = {
> >>> + "cortex-a7",
> >>> + NULL
> >>> +};
> >>
> >> Is this definitely right? Looking at the code, the raspi2 board
> >> creates a TYPE_BCM2836, and that creates cortex-a15 CPUs...
> >
> > The BCM2836 use a cortex-a7 but this cpu was not available at the time
> > of this commit (bad5623690b) and was added later in dcf578ed8ce "The A7
> > is very similar to the A15."
> >
> > I can prepare a patch for it to apply before this series.
>
> I noticed after I'd written that comment that patch 3 in this
> series does exactly the change to cortex-a7...
anyways this series, needs not to depend MachineState::cpu_model,
removal of which is queued in Eduardo's tree.
It seems we can't find consensus on how to deal with cpu_model names
in boards code [1/6].
>
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 3/6] bcm2836: Use the Cortex-A7 instead of Cortex-A15
2018-02-15 13:23 ` Philippe Mathieu-Daudé
@ 2018-02-15 22:41 ` Alistair Francis
0 siblings, 0 replies; 33+ messages in thread
From: Alistair Francis @ 2018-02-15 22:41 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, Alistair Francis,
qemu-devel@nongnu.org Developers, Eduardo Habkost,
Marcel Apfelbaum, Igor Mammedov
On Thu, Feb 15, 2018 at 5:23 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Peter,
>
> On 02/01/2018 09:42 PM, Alistair Francis wrote:
>> The BCM2836 uses a Cortex-A7 not a Cortex-A15. Update the device to use
>> the correct CPU.
>> https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/QA7_rev3.4.pdf
>
> Can you add these lines with reference to the commits?
>
> When the BCM2836 was introduced (bad5623690b) the Cortex-A7 was not
> available, so the very similar Cortex-A15 was used.
>
> Since dcf578ed8ce we can model the correct core.
>
> Thanks!
Added!
I'm not sure when I'll send the next version out though.
Alistair
>
> Phil.
>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>> ---
>> V3:
>> - Use ARM_CPU_TYPE_NAME() macro
>> V2:
>> - Fix the BCM2836 CPU
>>
>> hw/arm/bcm2836.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c
>> index 8c43291112..c477772484 100644
>> --- a/hw/arm/bcm2836.c
>> +++ b/hw/arm/bcm2836.c
>> @@ -30,7 +30,7 @@ static void bcm2836_init(Object *obj)
>>
>> for (n = 0; n < BCM2836_NCPUS; n++) {
>> object_initialize(&s->cpus[n], sizeof(s->cpus[n]),
>> - "cortex-a15-" TYPE_ARM_CPU);
>> + ARM_CPU_TYPE_NAME("cortex-a7"));
>> object_property_add_child(obj, "cpu[*]", OBJECT(&s->cpus[n]),
>> &error_abort);
>> }
>>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property
2018-02-02 0:42 [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Alistair Francis
` (5 preceding siblings ...)
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 6/6] xilinx_zynq: " Alistair Francis
@ 2018-03-13 23:13 ` Philippe Mathieu-Daudé
2018-03-21 14:33 ` Igor Mammedov
6 siblings, 1 reply; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2018-03-13 23:13 UTC (permalink / raw)
To: Peter Maydell, alistair23, Andrew Baumann
Cc: qemu-devel, ehabkost, marcel, imammedo
On 02/02/2018 01:42 AM, Alistair Francis wrote:
>
> There are numorous QEMU machines that only have a single or a handful of
> valid CPU options. To simplyfy the management of specificying which CPU
> is/isn't valid let's create a property that can be set in the machine
> init. We can then check to see if the user supplied CPU is in that list
> or not.
Sadly this series missed the 2.12 train.
>
> I have added the valid_cpu_types for some ARM machines only at the
> moment.
>
> Here is what specifying the CPUs looks like now:
>
> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m3" -S
> QEMU 2.10.50 monitor - type 'help' for more information
> (qemu) info cpus
> * CPU #0: thread_id=24175
> (qemu) q
>
> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m4" -S
> QEMU 2.10.50 monitor - type 'help' for more information
> (qemu) q
>
> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-m5" -S
> qemu-system-aarch64: unable to find CPU model 'cortex-m5'
>
> $ aarch64-softmmu/qemu-system-aarch64 -M netduino2 -kernel ./u-boot.elf -nographic -cpu "cortex-a9" -S
> qemu-system-aarch64: Invalid CPU type: cortex-a9-arm-cpu
> The valid models are: cortex-m3, cortex-m4
>
> V5:
> - Use cpu_model instead of cpu_type
> V4:
> - Rebase
> - Remove spaces
> V3:
> - Make the varialbes static
> V2:
> - Rebase
> - Reorder patches
> - Add a Raspberry Pi 2 CPU fix
> V1:
> - Small fixes to prepare a series instead of RFC
> - Add commit messages for the commits
> - Expand the machine support to ARM machines
> RFC v2:
> - Rebase on Igor's work
> - Use more QEMUisms inside the code
> - List the supported machines in a NULL terminated array
>
>
>
> Alistair Francis (6):
> machine: Convert the valid cpu types to use cpu_model
> netduino2: Specify the valid CPUs
> bcm2836: Use the Cortex-A7 instead of Cortex-A15
> raspi: Specify the valid CPUs
> xlnx-zcu102: Specify the valid CPUs
> xilinx_zynq: Specify the valid CPUs
>
> hw/arm/bcm2836.c | 2 +-
> hw/arm/netduino2.c | 10 +++++++++-
> hw/arm/raspi.c | 7 +++++++
> hw/arm/xilinx_zynq.c | 6 ++++++
> hw/arm/xlnx-zcu102.c | 17 +++++++++++++++++
> hw/core/machine.c | 11 +++++------
> 6 files changed, 45 insertions(+), 8 deletions(-)
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property
2018-03-13 23:13 ` [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Philippe Mathieu-Daudé
@ 2018-03-21 14:33 ` Igor Mammedov
0 siblings, 0 replies; 33+ messages in thread
From: Igor Mammedov @ 2018-03-21 14:33 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Peter Maydell, alistair23, Andrew Baumann, qemu-devel, ehabkost, marcel
On Wed, 14 Mar 2018 00:13:40 +0100
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 02/02/2018 01:42 AM, Alistair Francis wrote:
> >
> > There are numorous QEMU machines that only have a single or a handful of
> > valid CPU options. To simplyfy the management of specificying which CPU
> > is/isn't valid let's create a property that can be set in the machine
> > init. We can then check to see if the user supplied CPU is in that list
> > or not.
>
> Sadly this series missed the 2.12 train.
problematic part here is 'cpu_type -> cpu_model' conversion
without introducing back cpu model names in boards code.
Each target sort of already has this code 'cpu_list' for
printing supported CPU models. What we need is to generalize
it as most of the targets implement it the same way.
And then reuse generic code for 'cpu_type -> cpu_model'
so that boards won't have to be involved in it.
So currently cpu_list needs a volunteer who would be able
to generalize it and cleanup all targets across the the tree.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2018-02-06 14:43 ` Igor Mammedov
@ 2019-06-17 5:09 ` Philippe Mathieu-Daudé
2019-06-17 14:43 ` Eduardo Habkost
2019-06-17 15:01 ` Igor Mammedov
0 siblings, 2 replies; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-06-17 5:09 UTC (permalink / raw)
To: Igor Mammedov, Eduardo Habkost
Cc: marcel, peter.maydell, qemu-devel, alistair23
Hi Igor, Eduardo,
On 2/6/18 3:43 PM, Igor Mammedov wrote:
> On Mon, 5 Feb 2018 20:42:05 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
>> On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote:
>>> On Mon, 5 Feb 2018 11:54:01 -0200
>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>
>>>> On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote:
>>>>> On Fri, 2 Feb 2018 16:23:26 -0200
>>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>>
>>>>>> On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:
>>>>>>> As cpu_type is not a user visible string let's convert the
>>>>>>> valid_cpu_types to compare against cpu_model instead. This way we have a
>>>>>>> user friendly string to report back.
>>>>>>>
>>>>>>> Once we have a cpu_type to cpu_model conversion this patch should be
>>>>>>> reverted and we should use cpu_type instead.
>>>>>>>
>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>>>> ---
>>>>>>>
>>>>>>> hw/core/machine.c | 11 +++++------
>>>>>>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>>>>>> index cdc1163dc6..de5bac1c84 100644
>>>>>>> --- a/hw/core/machine.c
>>>>>>> +++ b/hw/core/machine.c
>>>>>>> @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
>>>>>>> /* If the machine supports the valid_cpu_types check and the user
>>>>>>> * specified a CPU with -cpu check here that the user CPU is supported.
>>>>>>> */
>>>>>>> - if (machine_class->valid_cpu_types && machine->cpu_type) {
>>>>>>> - ObjectClass *class = object_class_by_name(machine->cpu_type);
>>>>>>> + if (machine_class->valid_cpu_types && machine->cpu_model) {
>>>>>>> int i;
>>>>>>>
>>>>>>> for (i = 0; machine_class->valid_cpu_types[i]; i++) {
>>>>>>> - if (object_class_dynamic_cast(class,
>>>>>>> - machine_class->valid_cpu_types[i])) {
>>>>>>> + if (!strcmp(machine->cpu_model,
>>>>>>> + machine_class->valid_cpu_types[i])) {
>>>>>>
>>>>>> I would rename valid_cpu_types to valid_cpu_models to make the
>>>>>> new semantics clearer.
>>>>>>
>>>>>> Anyway, I have bad and good news:
>>>>>>
>>>>>> The bad news is Igor already sent patches last week that remove
>>>>>> MachineState::cpu_model, so this conflicts with his series. Now
>>>>>> parse_cpu_model() will be the only place where the original CPU model name is
>>>>>> available, but the function needs to work on *-user too. See:
>>>>>> "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
>>>>>>
>>>>>> The good news is that I think we can fix this very easily if
>>>>>> validation is done at the same place where parse_cpu_model() is
>>>>>> called. e.g.:
>>>>>>
>>>>>> current_machine->cpu_type = machine_class->default_cpu_type;
>>>>>> if (cpu_model) {
>>>>>> current_machine->cpu_type = parse_cpu_model(cpu_model);
>>>>>>
>>>>>> if (machine_class->valid_cpu_models) {
>>>>>> ObjectClass *class = object_class_by_name(machine->cpu_type);
>>>>>> int i;
>>>>>>
>>>>>> for (i = 0; machine_class->valid_cpu_models[i]; i++) {
>>>>>> const char *valid_model = machine_class->valid_cpu_models[i];
>>>>>> ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
>>>>>> if (object_class_dynamic_cast(class,
>>>>>> object_class_get_name(valid_class))) {
>>>>>> /* Valid CPU type, we're good to go */
>>>>>> break;
>>>>>> }
>>>>>> }
>>>>>> if (!machine_class->valid_cpu_models[i]) {
>>>>>> error_report("Invalid CPU model: %s", cpu_model);
>>>>>> error_printf("The valid CPU models are: %s",
>>>>>> machine_class->valid_cpu_models[0]);
>>>>>> for (i = 1; machine_class->valid_cpu_models[i]; i++) {
>>>>>> error_printf(", %s", machine_class->valid_cpu_models[i]);
>>>>>> }
>>>>>> error_printf("\n");
>>>>>> exit(1);
>>>>>> }
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> This can be done inside main(), or moved inside
>>>>>> machine_run_board_init() if main() pass cpu_model as argument to
>>>>>> the function.
>>>>>>
>>>>>> On either case, I think it's a good idea to do validation and
>>>>>> printing of error messages closer to the code that parses the
>>>>>> command-line options. This way we separate parsing/validation
>>>>>> from initialization.
>>>>> I agree it's better like you suggest as at least it prevents
>>>>> ms->cpu_model creeping back into boards code.
>>>>>
>>>>> But I still dislike (hate) an idea of new code adding non
>>>>> canonized cpu_model strings back in the boards code.
>>>>> It's just a matter of time when someone would use them
>>>>> and cpu_model parsing will creep back into boards.
>>>>>
>>>>> It would be much better to if we add
>>>>> char *MachineClass::cpu_name_by_type_name(char *cpu_type)
>>>>> callback and let machines in this patchset to set it,
>>>>> something along following lines which is not much of
>>>>> refactoring and allows for gradual conversion:
>>>>>
>>>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>>>> index 9631670..85cca84 100644
>>>>> --- a/target/arm/cpu.h
>>>>> +++ b/target/arm/cpu.h
>>>>> @@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
>>>>> return cpu->el_change_hook_opaque;
>>>>> }
>>>>>
>>>>> +char *arm_cpu_name_by_type_name(const char *typename);
>>>>> +
>>>>> #endif
>>>>> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
>>>>> index f936017..ae6adb7 100644
>>>>> --- a/hw/arm/netduino2.c
>>>>> +++ b/hw/arm/netduino2.c
>>>>> @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
>>>>> mc->desc = "Netduino 2 Machine";
>>>>> mc->init = netduino2_init;
>>>>> mc->ignore_memory_transaction_failures = true;
>>>>> + mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:
>>>>
>>>> I really don't want to introduce a new arch-specific hook just
>>>> for that. We should move CPU type lookup logic to common code
>>>> and make it unnecessary to write new hooks.
>>> unfortunately cpu_model (cpu name part) is target specific
>>> and it's translation to type and back is target specific mayhem.
>>
>> Why can't the model<->type translation be represented as data?
>> We could have simple cpu_type_name_suffix + an alias table.
>>
>> We have at least 4 arches that return a constant at
>> class_by_name. We have at least 10 arches that simply add a
>> suffix to the CPU model name. We must make them use common code
>> instead of requiring them to implement yet another hook[1].
> True, some of them could use generic hook and reduce
> code duplication greatly, we should do it regardless of whether
> table or target specific func approach is used.
>
>> In addition to the ones above, we have 3 that seem to just need
>> an alias table (cris, superh, alpha). ppc can probably also use
>> an alias table for the ppc_cpu_class_by_pvr() stuff. sparc just
>> needs whitespaces translated to '-' (sparc), which can be done
>> using an alias table.
>>
>> In the end I couldn't find any example that can't be represented
>> by a suffix + alias table.
>
> Table based approach is possible but it won't be as simple
> as you've just pictured it.
>
> From what I recall from cpu_class_by_name cleanups table should be able
> to describe cases like (sometimes combination of them):
> * 1:1 mapping - where cpu_model == cpu_type
> * cpu_model <==> cpu_model + suffix - most common usecase
> * cpu_model <==> prefix cpu_model - riscv patches on list are trying to add such cpu types
> * NULL => some_fixed type
> * case (in) sensitive flag
> * garbage => some_fixed type
> * substitutions
> * aliases (sometimes dynamic depending on --enable-kvm (PPC))
> Maybe something else.
>
> We can think about it at leisure but I can't say if new approach
> complexity it's worth of the effort.
>
> It would be nice see impl, but it's a lot of refactoring that's
> clearly out of scope of this series.
> I'd prefer small incremental refactoring (if possible) that
> won't scare people of and easy to review vs a huge one.
>
>>> So I'd prefer having both back and forth functions together in
>>> one place. And common code to call them when necessary.
>>>
>>> We could do global cpu_name_by_type_name() instead of hook,
>>> which I'd prefer even more but then conversion can't be done
>>> only for one target but rather for all targets at once.
>>
>> I don't mind letting a few targets override default behavior with
>> a hook if really necessary, but I have a problem with requiring
>> all targets to implement what's basically the same boilerplate
>> code to add/remove a string suffix and translating aliases.
> it could be generic helper if target does the same plus
> not mandatory at that (in case target/board doesn't care
> about valid cpus).
>
>>>> I agree it would be better if we had a cpu_name_by_type_name()
>>>> function, but I would like to have it implemented cleanly.
>>> In some cases(targets) it can be common helper, but in other
>>> cases it's not so.
>>> My suggestion though allows to do gradual conversion and
>>> avoid putting cpu_model names back in board's code (which I just manged to remove).
>>> Once all targets converted and relevant code is isolated
>>> we can attempt to generalize it if it's possible or at least
>>> make of it global per target helper to get rid of
>>> temporary machine hook.
>>>
>>> (seeing this series reposted with cpu_model names in boards code,
>>> it doesn't looks like author would like to implement tree-wide
>>> generalization first)
>>
>> Well, if nobody is willing to generalize all target-specific code
>> right now, I don't see the harm in having cpu_model-based tables
>> in a few boards in the meantime (as this patch series does). But
>> I do see harm in requiring all our 20 targets to implement yet
>> another hook and increasing the costs of cleaning up the mess
>> later.
> If we use MachineClass hook then it might be done per target
> on demand, so no one would require that every target should
> implement it.
> Also there could be a generic helper for targets that do the same.
> Machine which needs to enable valid_cpus, will have to use generic
> hook impl or provide target specific if it's special case.
>
> Though I do see harm in adding cpu_model tables in boards code
> vs target specific hooks on demand as that will be copy-pasted
> in other boards afterwards (number of which is bigger compared
> to targets count) and ultimately it would duplicate cpu_name
> strings in every board vs hook approach where cpu_model could
> be calculated from cpu_type by a function (generic or
> target specific).
>
> Good thing about hook is that it's non intrusive and
> isolates(consolidates) existing cpu_type -> cpu_model
> conversion in multiple places into one place.
> Then later it would be easier to generalize if someone
> decides to do it.
I wonder how you want to proceed with this series, the first patch got
merged (c9cf636d48f) but after your "CPU model name" rework, this commit
seems now not very complete/usable.
Rebasing this series, i.e. with this snippet:
-- >8 --
diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
index f57fc38f92..cca4ec6648 100644
--- a/hw/arm/netduino2.c
+++ b/hw/arm/netduino2.c
@@ -34,7 +34,7 @@ static void netduino2_init(MachineState *machine)
DeviceState *dev;
dev = qdev_create(NULL, TYPE_STM32F205_SOC);
- qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3"));
+ qdev_prop_set_string(dev, "cpu-type", machine->cpu_type);
object_property_set_bool(OBJECT(dev), true, "realized", &error_fatal);
armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
@@ -43,8 +43,14 @@ static void netduino2_init(MachineState *machine)
static void netduino2_machine_init(MachineClass *mc)
{
+ static const char *valid_cpus[] = {
+ ARM_CPU_TYPE_NAME("cortex-m3"),
+ ARM_CPU_TYPE_NAME("cortex-m4"),
+ NULL
+ };
mc->desc = "Netduino 2 Machine";
mc->init = netduino2_init;
+ mc->valid_cpu_types = valid_cpus;
mc->ignore_memory_transaction_failures = true;
}
---
We get cpu names with suffix:
$ arm-softmmu/qemu-system-arm -M netduino2 -cpu arm926
qemu-system-arm: Invalid CPU type: arm926-arm-cpu
The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu
I understand you won't want a global cpu_name_by_type_name, how do you
want to do then?
Should we define an automatically expanded TARGET_CPU_TYPE_SUFFIX?
Then we could have generic machine code to parse the names.
Thanks,
Phil.
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2019-06-17 5:09 ` Philippe Mathieu-Daudé
@ 2019-06-17 14:43 ` Eduardo Habkost
2019-06-17 15:01 ` Igor Mammedov
1 sibling, 0 replies; 33+ messages in thread
From: Eduardo Habkost @ 2019-06-17 14:43 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: marcel, Igor Mammedov, qemu-devel, alistair23, peter.maydell
On Mon, Jun 17, 2019 at 07:09:59AM +0200, Philippe Mathieu-Daudé wrote:
[...]
>
> We get cpu names with suffix:
>
> $ arm-softmmu/qemu-system-arm -M netduino2 -cpu arm926
> qemu-system-arm: Invalid CPU type: arm926-arm-cpu
> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu
>
> I understand you won't want a global cpu_name_by_type_name, how do you
> want to do then?
I would like having a global cpu_name_by_type_name() function.
I wouldn't want to force each architecture to provide yet another
string conversion/parsing function, though.
>
> Should we define an automatically expanded TARGET_CPU_TYPE_SUFFIX?
> Then we could have generic machine code to parse the names.
I think we could do that, but with a CPUClass field instead of a
preprocessor macro.
For a reference on existing corner cases of CPU model name
parsing that might get in the way, see this series:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg611813.html
If you are interested, I have work in progress for a generic CPU
model listing function at:
https://github.com/ehabkost/qemu-hacks/commits/work/cpu-generic-list
https://github.com/ehabkost/qemu-hacks/commit/df122e7e47476d7e2b7b809a4b4120537f50c137
--
Eduardo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2019-06-17 5:09 ` Philippe Mathieu-Daudé
2019-06-17 14:43 ` Eduardo Habkost
@ 2019-06-17 15:01 ` Igor Mammedov
2019-06-17 15:15 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 33+ messages in thread
From: Igor Mammedov @ 2019-06-17 15:01 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: marcel, peter.maydell, Eduardo Habkost, alistair23, qemu-devel
On Mon, 17 Jun 2019 07:09:59 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> Hi Igor, Eduardo,
>
> On 2/6/18 3:43 PM, Igor Mammedov wrote:
> > On Mon, 5 Feb 2018 20:42:05 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> >> On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote:
> >>> On Mon, 5 Feb 2018 11:54:01 -0200
> >>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>>
> >>>> On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote:
> >>>>> On Fri, 2 Feb 2018 16:23:26 -0200
> >>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>>>>
> >>>>>> On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:
> >>>>>>> As cpu_type is not a user visible string let's convert the
> >>>>>>> valid_cpu_types to compare against cpu_model instead. This way we have a
> >>>>>>> user friendly string to report back.
> >>>>>>>
> >>>>>>> Once we have a cpu_type to cpu_model conversion this patch should be
> >>>>>>> reverted and we should use cpu_type instead.
> >>>>>>>
> >>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> hw/core/machine.c | 11 +++++------
> >>>>>>> 1 file changed, 5 insertions(+), 6 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
> >>>>>>> index cdc1163dc6..de5bac1c84 100644
> >>>>>>> --- a/hw/core/machine.c
> >>>>>>> +++ b/hw/core/machine.c
> >>>>>>> @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
> >>>>>>> /* If the machine supports the valid_cpu_types check and the user
> >>>>>>> * specified a CPU with -cpu check here that the user CPU is supported.
> >>>>>>> */
> >>>>>>> - if (machine_class->valid_cpu_types && machine->cpu_type) {
> >>>>>>> - ObjectClass *class = object_class_by_name(machine->cpu_type);
> >>>>>>> + if (machine_class->valid_cpu_types && machine->cpu_model) {
> >>>>>>> int i;
> >>>>>>>
> >>>>>>> for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> >>>>>>> - if (object_class_dynamic_cast(class,
> >>>>>>> - machine_class->valid_cpu_types[i])) {
> >>>>>>> + if (!strcmp(machine->cpu_model,
> >>>>>>> + machine_class->valid_cpu_types[i])) {
> >>>>>>
> >>>>>> I would rename valid_cpu_types to valid_cpu_models to make the
> >>>>>> new semantics clearer.
> >>>>>>
> >>>>>> Anyway, I have bad and good news:
> >>>>>>
> >>>>>> The bad news is Igor already sent patches last week that remove
> >>>>>> MachineState::cpu_model, so this conflicts with his series. Now
> >>>>>> parse_cpu_model() will be the only place where the original CPU model name is
> >>>>>> available, but the function needs to work on *-user too. See:
> >>>>>> "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
> >>>>>>
> >>>>>> The good news is that I think we can fix this very easily if
> >>>>>> validation is done at the same place where parse_cpu_model() is
> >>>>>> called. e.g.:
> >>>>>>
> >>>>>> current_machine->cpu_type = machine_class->default_cpu_type;
> >>>>>> if (cpu_model) {
> >>>>>> current_machine->cpu_type = parse_cpu_model(cpu_model);
> >>>>>>
> >>>>>> if (machine_class->valid_cpu_models) {
> >>>>>> ObjectClass *class = object_class_by_name(machine->cpu_type);
> >>>>>> int i;
> >>>>>>
> >>>>>> for (i = 0; machine_class->valid_cpu_models[i]; i++) {
> >>>>>> const char *valid_model = machine_class->valid_cpu_models[i];
> >>>>>> ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
> >>>>>> if (object_class_dynamic_cast(class,
> >>>>>> object_class_get_name(valid_class))) {
> >>>>>> /* Valid CPU type, we're good to go */
> >>>>>> break;
> >>>>>> }
> >>>>>> }
> >>>>>> if (!machine_class->valid_cpu_models[i]) {
> >>>>>> error_report("Invalid CPU model: %s", cpu_model);
> >>>>>> error_printf("The valid CPU models are: %s",
> >>>>>> machine_class->valid_cpu_models[0]);
> >>>>>> for (i = 1; machine_class->valid_cpu_models[i]; i++) {
> >>>>>> error_printf(", %s", machine_class->valid_cpu_models[i]);
> >>>>>> }
> >>>>>> error_printf("\n");
> >>>>>> exit(1);
> >>>>>> }
> >>>>>> }
> >>>>>> }
> >>>>>>
> >>>>>> This can be done inside main(), or moved inside
> >>>>>> machine_run_board_init() if main() pass cpu_model as argument to
> >>>>>> the function.
> >>>>>>
> >>>>>> On either case, I think it's a good idea to do validation and
> >>>>>> printing of error messages closer to the code that parses the
> >>>>>> command-line options. This way we separate parsing/validation
> >>>>>> from initialization.
> >>>>> I agree it's better like you suggest as at least it prevents
> >>>>> ms->cpu_model creeping back into boards code.
> >>>>>
> >>>>> But I still dislike (hate) an idea of new code adding non
> >>>>> canonized cpu_model strings back in the boards code.
> >>>>> It's just a matter of time when someone would use them
> >>>>> and cpu_model parsing will creep back into boards.
> >>>>>
> >>>>> It would be much better to if we add
> >>>>> char *MachineClass::cpu_name_by_type_name(char *cpu_type)
> >>>>> callback and let machines in this patchset to set it,
> >>>>> something along following lines which is not much of
> >>>>> refactoring and allows for gradual conversion:
> >>>>>
> >>>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> >>>>> index 9631670..85cca84 100644
> >>>>> --- a/target/arm/cpu.h
> >>>>> +++ b/target/arm/cpu.h
> >>>>> @@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
> >>>>> return cpu->el_change_hook_opaque;
> >>>>> }
> >>>>>
> >>>>> +char *arm_cpu_name_by_type_name(const char *typename);
> >>>>> +
> >>>>> #endif
> >>>>> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> >>>>> index f936017..ae6adb7 100644
> >>>>> --- a/hw/arm/netduino2.c
> >>>>> +++ b/hw/arm/netduino2.c
> >>>>> @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
> >>>>> mc->desc = "Netduino 2 Machine";
> >>>>> mc->init = netduino2_init;
> >>>>> mc->ignore_memory_transaction_failures = true;
> >>>>> + mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:
> >>>>
> >>>> I really don't want to introduce a new arch-specific hook just
> >>>> for that. We should move CPU type lookup logic to common code
> >>>> and make it unnecessary to write new hooks.
> >>> unfortunately cpu_model (cpu name part) is target specific
> >>> and it's translation to type and back is target specific mayhem.
> >>
> >> Why can't the model<->type translation be represented as data?
> >> We could have simple cpu_type_name_suffix + an alias table.
> >>
> >> We have at least 4 arches that return a constant at
> >> class_by_name. We have at least 10 arches that simply add a
> >> suffix to the CPU model name. We must make them use common code
> >> instead of requiring them to implement yet another hook[1].
> > True, some of them could use generic hook and reduce
> > code duplication greatly, we should do it regardless of whether
> > table or target specific func approach is used.
> >
> >> In addition to the ones above, we have 3 that seem to just need
> >> an alias table (cris, superh, alpha). ppc can probably also use
> >> an alias table for the ppc_cpu_class_by_pvr() stuff. sparc just
> >> needs whitespaces translated to '-' (sparc), which can be done
> >> using an alias table.
> >>
> >> In the end I couldn't find any example that can't be represented
> >> by a suffix + alias table.
> >
> > Table based approach is possible but it won't be as simple
> > as you've just pictured it.
> >
> > From what I recall from cpu_class_by_name cleanups table should be able
> > to describe cases like (sometimes combination of them):
> > * 1:1 mapping - where cpu_model == cpu_type
> > * cpu_model <==> cpu_model + suffix - most common usecase
> > * cpu_model <==> prefix cpu_model - riscv patches on list are trying to add such cpu types
> > * NULL => some_fixed type
> > * case (in) sensitive flag
> > * garbage => some_fixed type
> > * substitutions
> > * aliases (sometimes dynamic depending on --enable-kvm (PPC))
> > Maybe something else.
> >
> > We can think about it at leisure but I can't say if new approach
> > complexity it's worth of the effort.
> >
> > It would be nice see impl, but it's a lot of refactoring that's
> > clearly out of scope of this series.
> > I'd prefer small incremental refactoring (if possible) that
> > won't scare people of and easy to review vs a huge one.
> >
> >>> So I'd prefer having both back and forth functions together in
> >>> one place. And common code to call them when necessary.
> >>>
> >>> We could do global cpu_name_by_type_name() instead of hook,
> >>> which I'd prefer even more but then conversion can't be done
> >>> only for one target but rather for all targets at once.
> >>
> >> I don't mind letting a few targets override default behavior with
> >> a hook if really necessary, but I have a problem with requiring
> >> all targets to implement what's basically the same boilerplate
> >> code to add/remove a string suffix and translating aliases.
> > it could be generic helper if target does the same plus
> > not mandatory at that (in case target/board doesn't care
> > about valid cpus).
> >
> >>>> I agree it would be better if we had a cpu_name_by_type_name()
> >>>> function, but I would like to have it implemented cleanly.
> >>> In some cases(targets) it can be common helper, but in other
> >>> cases it's not so.
> >>> My suggestion though allows to do gradual conversion and
> >>> avoid putting cpu_model names back in board's code (which I just manged to remove).
> >>> Once all targets converted and relevant code is isolated
> >>> we can attempt to generalize it if it's possible or at least
> >>> make of it global per target helper to get rid of
> >>> temporary machine hook.
> >>>
> >>> (seeing this series reposted with cpu_model names in boards code,
> >>> it doesn't looks like author would like to implement tree-wide
> >>> generalization first)
> >>
> >> Well, if nobody is willing to generalize all target-specific code
> >> right now, I don't see the harm in having cpu_model-based tables
> >> in a few boards in the meantime (as this patch series does). But
> >> I do see harm in requiring all our 20 targets to implement yet
> >> another hook and increasing the costs of cleaning up the mess
> >> later.
> > If we use MachineClass hook then it might be done per target
> > on demand, so no one would require that every target should
> > implement it.
> > Also there could be a generic helper for targets that do the same.
> > Machine which needs to enable valid_cpus, will have to use generic
> > hook impl or provide target specific if it's special case.
> >
> > Though I do see harm in adding cpu_model tables in boards code
> > vs target specific hooks on demand as that will be copy-pasted
> > in other boards afterwards (number of which is bigger compared
> > to targets count) and ultimately it would duplicate cpu_name
> > strings in every board vs hook approach where cpu_model could
> > be calculated from cpu_type by a function (generic or
> > target specific).
> >
> > Good thing about hook is that it's non intrusive and
> > isolates(consolidates) existing cpu_type -> cpu_model
> > conversion in multiple places into one place.
> > Then later it would be easier to generalize if someone
> > decides to do it.
>
> I wonder how you want to proceed with this series, the first patch got
> merged (c9cf636d48f) but after your "CPU model name" rework, this commit
> seems now not very complete/usable.
>
> Rebasing this series, i.e. with this snippet:
>
> -- >8 --
> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> index f57fc38f92..cca4ec6648 100644
> --- a/hw/arm/netduino2.c
> +++ b/hw/arm/netduino2.c
> @@ -34,7 +34,7 @@ static void netduino2_init(MachineState *machine)
> DeviceState *dev;
>
> dev = qdev_create(NULL, TYPE_STM32F205_SOC);
> - qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3"));
> + qdev_prop_set_string(dev, "cpu-type", machine->cpu_type);
> object_property_set_bool(OBJECT(dev), true, "realized", &error_fatal);
>
> armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
> @@ -43,8 +43,14 @@ static void netduino2_init(MachineState *machine)
>
> static void netduino2_machine_init(MachineClass *mc)
> {
> + static const char *valid_cpus[] = {
> + ARM_CPU_TYPE_NAME("cortex-m3"),
> + ARM_CPU_TYPE_NAME("cortex-m4"),
> + NULL
> + };
> mc->desc = "Netduino 2 Machine";
> mc->init = netduino2_init;
> + mc->valid_cpu_types = valid_cpus;
> mc->ignore_memory_transaction_failures = true;
> }
> ---
>
> We get cpu names with suffix:
>
> $ arm-softmmu/qemu-system-arm -M netduino2 -cpu arm926
> qemu-system-arm: Invalid CPU type: arm926-arm-cpu
> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu
>
> I understand you won't want a global cpu_name_by_type_name, how do you
> want to do then?
>
> Should we define an automatically expanded TARGET_CPU_TYPE_SUFFIX?
> Then we could have generic machine code to parse the names.
It would work only for some cases,
problem is that we have a zoo of naming schemes.
Considering that cpus models are used widely we probably can't
deprecate it outright (for versioned machine types).
Instead of wasting resources on translating cpu-type => 'cpu-name',
(hook or lookup tables) how about simplifying code and making all
boards accept full typenames?
It could be handled in generic way and then printing error with
full type names would be acceptable since user would be able to feed it
to -cpu.
'-cpu help' - would need some work to display types as well (also could be generic)
Perhaps with it we could deprecate cpu_models for non versioned
machines/targets, which in most cases would allow us to drop special
suffix/prefix/nonsense/case-sensitive/substitutions and whatever else
is already existing and keep exiting translation routines (hooks) only
for versioned machine types as necessary evil.
> Thanks,
>
> Phil.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2019-06-17 15:01 ` Igor Mammedov
@ 2019-06-17 15:15 ` Philippe Mathieu-Daudé
2019-06-17 15:33 ` Igor Mammedov
0 siblings, 1 reply; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-06-17 15:15 UTC (permalink / raw)
To: Igor Mammedov
Cc: marcel, peter.maydell, Eduardo Habkost, alistair23, qemu-devel
On 6/17/19 5:01 PM, Igor Mammedov wrote:
> On Mon, 17 Jun 2019 07:09:59 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
>> Hi Igor, Eduardo,
>>
>> On 2/6/18 3:43 PM, Igor Mammedov wrote:
>>> On Mon, 5 Feb 2018 20:42:05 -0200
>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>
>>>> On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote:
>>>>> On Mon, 5 Feb 2018 11:54:01 -0200
>>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>>
>>>>>> On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote:
>>>>>>> On Fri, 2 Feb 2018 16:23:26 -0200
>>>>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>>>>
>>>>>>>> On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:
>>>>>>>>> As cpu_type is not a user visible string let's convert the
>>>>>>>>> valid_cpu_types to compare against cpu_model instead. This way we have a
>>>>>>>>> user friendly string to report back.
>>>>>>>>>
>>>>>>>>> Once we have a cpu_type to cpu_model conversion this patch should be
>>>>>>>>> reverted and we should use cpu_type instead.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> hw/core/machine.c | 11 +++++------
>>>>>>>>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>>>>>>>> index cdc1163dc6..de5bac1c84 100644
>>>>>>>>> --- a/hw/core/machine.c
>>>>>>>>> +++ b/hw/core/machine.c
>>>>>>>>> @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
>>>>>>>>> /* If the machine supports the valid_cpu_types check and the user
>>>>>>>>> * specified a CPU with -cpu check here that the user CPU is supported.
>>>>>>>>> */
>>>>>>>>> - if (machine_class->valid_cpu_types && machine->cpu_type) {
>>>>>>>>> - ObjectClass *class = object_class_by_name(machine->cpu_type);
>>>>>>>>> + if (machine_class->valid_cpu_types && machine->cpu_model) {
>>>>>>>>> int i;
>>>>>>>>>
>>>>>>>>> for (i = 0; machine_class->valid_cpu_types[i]; i++) {
>>>>>>>>> - if (object_class_dynamic_cast(class,
>>>>>>>>> - machine_class->valid_cpu_types[i])) {
>>>>>>>>> + if (!strcmp(machine->cpu_model,
>>>>>>>>> + machine_class->valid_cpu_types[i])) {
>>>>>>>>
>>>>>>>> I would rename valid_cpu_types to valid_cpu_models to make the
>>>>>>>> new semantics clearer.
>>>>>>>>
>>>>>>>> Anyway, I have bad and good news:
>>>>>>>>
>>>>>>>> The bad news is Igor already sent patches last week that remove
>>>>>>>> MachineState::cpu_model, so this conflicts with his series. Now
>>>>>>>> parse_cpu_model() will be the only place where the original CPU model name is
>>>>>>>> available, but the function needs to work on *-user too. See:
>>>>>>>> "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
>>>>>>>>
>>>>>>>> The good news is that I think we can fix this very easily if
>>>>>>>> validation is done at the same place where parse_cpu_model() is
>>>>>>>> called. e.g.:
>>>>>>>>
>>>>>>>> current_machine->cpu_type = machine_class->default_cpu_type;
>>>>>>>> if (cpu_model) {
>>>>>>>> current_machine->cpu_type = parse_cpu_model(cpu_model);
>>>>>>>>
>>>>>>>> if (machine_class->valid_cpu_models) {
>>>>>>>> ObjectClass *class = object_class_by_name(machine->cpu_type);
>>>>>>>> int i;
>>>>>>>>
>>>>>>>> for (i = 0; machine_class->valid_cpu_models[i]; i++) {
>>>>>>>> const char *valid_model = machine_class->valid_cpu_models[i];
>>>>>>>> ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
>>>>>>>> if (object_class_dynamic_cast(class,
>>>>>>>> object_class_get_name(valid_class))) {
>>>>>>>> /* Valid CPU type, we're good to go */
>>>>>>>> break;
>>>>>>>> }
>>>>>>>> }
>>>>>>>> if (!machine_class->valid_cpu_models[i]) {
>>>>>>>> error_report("Invalid CPU model: %s", cpu_model);
>>>>>>>> error_printf("The valid CPU models are: %s",
>>>>>>>> machine_class->valid_cpu_models[0]);
>>>>>>>> for (i = 1; machine_class->valid_cpu_models[i]; i++) {
>>>>>>>> error_printf(", %s", machine_class->valid_cpu_models[i]);
>>>>>>>> }
>>>>>>>> error_printf("\n");
>>>>>>>> exit(1);
>>>>>>>> }
>>>>>>>> }
>>>>>>>> }
>>>>>>>>
>>>>>>>> This can be done inside main(), or moved inside
>>>>>>>> machine_run_board_init() if main() pass cpu_model as argument to
>>>>>>>> the function.
>>>>>>>>
>>>>>>>> On either case, I think it's a good idea to do validation and
>>>>>>>> printing of error messages closer to the code that parses the
>>>>>>>> command-line options. This way we separate parsing/validation
>>>>>>>> from initialization.
>>>>>>> I agree it's better like you suggest as at least it prevents
>>>>>>> ms->cpu_model creeping back into boards code.
>>>>>>>
>>>>>>> But I still dislike (hate) an idea of new code adding non
>>>>>>> canonized cpu_model strings back in the boards code.
>>>>>>> It's just a matter of time when someone would use them
>>>>>>> and cpu_model parsing will creep back into boards.
>>>>>>>
>>>>>>> It would be much better to if we add
>>>>>>> char *MachineClass::cpu_name_by_type_name(char *cpu_type)
>>>>>>> callback and let machines in this patchset to set it,
>>>>>>> something along following lines which is not much of
>>>>>>> refactoring and allows for gradual conversion:
>>>>>>>
>>>>>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>>>>>>> index 9631670..85cca84 100644
>>>>>>> --- a/target/arm/cpu.h
>>>>>>> +++ b/target/arm/cpu.h
>>>>>>> @@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
>>>>>>> return cpu->el_change_hook_opaque;
>>>>>>> }
>>>>>>>
>>>>>>> +char *arm_cpu_name_by_type_name(const char *typename);
>>>>>>> +
>>>>>>> #endif
>>>>>>> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
>>>>>>> index f936017..ae6adb7 100644
>>>>>>> --- a/hw/arm/netduino2.c
>>>>>>> +++ b/hw/arm/netduino2.c
>>>>>>> @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
>>>>>>> mc->desc = "Netduino 2 Machine";
>>>>>>> mc->init = netduino2_init;
>>>>>>> mc->ignore_memory_transaction_failures = true;
>>>>>>> + mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:
>>>>>>
>>>>>> I really don't want to introduce a new arch-specific hook just
>>>>>> for that. We should move CPU type lookup logic to common code
>>>>>> and make it unnecessary to write new hooks.
>>>>> unfortunately cpu_model (cpu name part) is target specific
>>>>> and it's translation to type and back is target specific mayhem.
>>>>
>>>> Why can't the model<->type translation be represented as data?
>>>> We could have simple cpu_type_name_suffix + an alias table.
>>>>
>>>> We have at least 4 arches that return a constant at
>>>> class_by_name. We have at least 10 arches that simply add a
>>>> suffix to the CPU model name. We must make them use common code
>>>> instead of requiring them to implement yet another hook[1].
>>> True, some of them could use generic hook and reduce
>>> code duplication greatly, we should do it regardless of whether
>>> table or target specific func approach is used.
>>>
>>>> In addition to the ones above, we have 3 that seem to just need
>>>> an alias table (cris, superh, alpha). ppc can probably also use
>>>> an alias table for the ppc_cpu_class_by_pvr() stuff. sparc just
>>>> needs whitespaces translated to '-' (sparc), which can be done
>>>> using an alias table.
>>>>
>>>> In the end I couldn't find any example that can't be represented
>>>> by a suffix + alias table.
>>>
>>> Table based approach is possible but it won't be as simple
>>> as you've just pictured it.
>>>
>>> From what I recall from cpu_class_by_name cleanups table should be able
>>> to describe cases like (sometimes combination of them):
>>> * 1:1 mapping - where cpu_model == cpu_type
>>> * cpu_model <==> cpu_model + suffix - most common usecase
>>> * cpu_model <==> prefix cpu_model - riscv patches on list are trying to add such cpu types
>>> * NULL => some_fixed type
>>> * case (in) sensitive flag
>>> * garbage => some_fixed type
>>> * substitutions
>>> * aliases (sometimes dynamic depending on --enable-kvm (PPC))
>>> Maybe something else.
>>>
>>> We can think about it at leisure but I can't say if new approach
>>> complexity it's worth of the effort.
>>>
>>> It would be nice see impl, but it's a lot of refactoring that's
>>> clearly out of scope of this series.
>>> I'd prefer small incremental refactoring (if possible) that
>>> won't scare people of and easy to review vs a huge one.
>>>
>>>>> So I'd prefer having both back and forth functions together in
>>>>> one place. And common code to call them when necessary.
>>>>>
>>>>> We could do global cpu_name_by_type_name() instead of hook,
>>>>> which I'd prefer even more but then conversion can't be done
>>>>> only for one target but rather for all targets at once.
>>>>
>>>> I don't mind letting a few targets override default behavior with
>>>> a hook if really necessary, but I have a problem with requiring
>>>> all targets to implement what's basically the same boilerplate
>>>> code to add/remove a string suffix and translating aliases.
>>> it could be generic helper if target does the same plus
>>> not mandatory at that (in case target/board doesn't care
>>> about valid cpus).
>>>
>>>>>> I agree it would be better if we had a cpu_name_by_type_name()
>>>>>> function, but I would like to have it implemented cleanly.
>>>>> In some cases(targets) it can be common helper, but in other
>>>>> cases it's not so.
>>>>> My suggestion though allows to do gradual conversion and
>>>>> avoid putting cpu_model names back in board's code (which I just manged to remove).
>>>>> Once all targets converted and relevant code is isolated
>>>>> we can attempt to generalize it if it's possible or at least
>>>>> make of it global per target helper to get rid of
>>>>> temporary machine hook.
>>>>>
>>>>> (seeing this series reposted with cpu_model names in boards code,
>>>>> it doesn't looks like author would like to implement tree-wide
>>>>> generalization first)
>>>>
>>>> Well, if nobody is willing to generalize all target-specific code
>>>> right now, I don't see the harm in having cpu_model-based tables
>>>> in a few boards in the meantime (as this patch series does). But
>>>> I do see harm in requiring all our 20 targets to implement yet
>>>> another hook and increasing the costs of cleaning up the mess
>>>> later.
>>> If we use MachineClass hook then it might be done per target
>>> on demand, so no one would require that every target should
>>> implement it.
>>> Also there could be a generic helper for targets that do the same.
>>> Machine which needs to enable valid_cpus, will have to use generic
>>> hook impl or provide target specific if it's special case.
>>>
>>> Though I do see harm in adding cpu_model tables in boards code
>>> vs target specific hooks on demand as that will be copy-pasted
>>> in other boards afterwards (number of which is bigger compared
>>> to targets count) and ultimately it would duplicate cpu_name
>>> strings in every board vs hook approach where cpu_model could
>>> be calculated from cpu_type by a function (generic or
>>> target specific).
>>>
>>> Good thing about hook is that it's non intrusive and
>>> isolates(consolidates) existing cpu_type -> cpu_model
>>> conversion in multiple places into one place.
>>> Then later it would be easier to generalize if someone
>>> decides to do it.
>>
>> I wonder how you want to proceed with this series, the first patch got
>> merged (c9cf636d48f) but after your "CPU model name" rework, this commit
>> seems now not very complete/usable.
>>
>> Rebasing this series, i.e. with this snippet:
>>
>> -- >8 --
>> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
>> index f57fc38f92..cca4ec6648 100644
>> --- a/hw/arm/netduino2.c
>> +++ b/hw/arm/netduino2.c
>> @@ -34,7 +34,7 @@ static void netduino2_init(MachineState *machine)
>> DeviceState *dev;
>>
>> dev = qdev_create(NULL, TYPE_STM32F205_SOC);
>> - qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3"));
>> + qdev_prop_set_string(dev, "cpu-type", machine->cpu_type);
>> object_property_set_bool(OBJECT(dev), true, "realized", &error_fatal);
>>
>> armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
>> @@ -43,8 +43,14 @@ static void netduino2_init(MachineState *machine)
>>
>> static void netduino2_machine_init(MachineClass *mc)
>> {
>> + static const char *valid_cpus[] = {
>> + ARM_CPU_TYPE_NAME("cortex-m3"),
>> + ARM_CPU_TYPE_NAME("cortex-m4"),
>> + NULL
>> + };
>> mc->desc = "Netduino 2 Machine";
>> mc->init = netduino2_init;
>> + mc->valid_cpu_types = valid_cpus;
>> mc->ignore_memory_transaction_failures = true;
>> }
>> ---
>>
>> We get cpu names with suffix:
>>
>> $ arm-softmmu/qemu-system-arm -M netduino2 -cpu arm926
>> qemu-system-arm: Invalid CPU type: arm926-arm-cpu
>> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu
>>
>> I understand you won't want a global cpu_name_by_type_name, how do you
>> want to do then?
>>
>> Should we define an automatically expanded TARGET_CPU_TYPE_SUFFIX?
>> Then we could have generic machine code to parse the names.
> It would work only for some cases,
> problem is that we have a zoo of naming schemes.
> Considering that cpus models are used widely we probably can't
> deprecate it outright (for versioned machine types).
>
> Instead of wasting resources on translating cpu-type => 'cpu-name',
> (hook or lookup tables) how about simplifying code and making all
> boards accept full typenames?
>
> It could be handled in generic way and then printing error with
> full type names would be acceptable since user would be able to feed it
> to -cpu.
>
> '-cpu help' - would need some work to display types as well (also could be generic)
>
> Perhaps with it we could deprecate cpu_models for non versioned
> machines/targets, which in most cases would allow us to drop special
> suffix/prefix/nonsense/case-sensitive/substitutions and whatever else
> is already existing and keep exiting translation routines (hooks) only
> for versioned machine types as necessary evil.
Yes. Eduardo and you should write some lines to explain this, and then
we will follow :)
I feel concerned because:
1/ Alistair series is very helpful to new users, and
2/ As the RX architecture series showed, today it is not very clear how
to correctly use cpu_models.
Eduardo is working on a series, I'll wait for his work.
Regards,
Phil.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2019-06-17 15:15 ` Philippe Mathieu-Daudé
@ 2019-06-17 15:33 ` Igor Mammedov
2019-06-17 16:27 ` Eduardo Habkost
0 siblings, 1 reply; 33+ messages in thread
From: Igor Mammedov @ 2019-06-17 15:33 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: marcel, peter.maydell, Eduardo Habkost, alistair23, qemu-devel
On Mon, 17 Jun 2019 17:15:21 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> On 6/17/19 5:01 PM, Igor Mammedov wrote:
> > On Mon, 17 Jun 2019 07:09:59 +0200
> > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >
> >> Hi Igor, Eduardo,
> >>
> >> On 2/6/18 3:43 PM, Igor Mammedov wrote:
> >>> On Mon, 5 Feb 2018 20:42:05 -0200
> >>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>>
> >>>> On Mon, Feb 05, 2018 at 03:42:02PM +0100, Igor Mammedov wrote:
> >>>>> On Mon, 5 Feb 2018 11:54:01 -0200
> >>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>>>>
> >>>>>> On Mon, Feb 05, 2018 at 12:22:35PM +0100, Igor Mammedov wrote:
> >>>>>>> On Fri, 2 Feb 2018 16:23:26 -0200
> >>>>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>>>>>>
> >>>>>>>> On Thu, Feb 01, 2018 at 04:42:05PM -0800, Alistair Francis wrote:
> >>>>>>>>> As cpu_type is not a user visible string let's convert the
> >>>>>>>>> valid_cpu_types to compare against cpu_model instead. This way we have a
> >>>>>>>>> user friendly string to report back.
> >>>>>>>>>
> >>>>>>>>> Once we have a cpu_type to cpu_model conversion this patch should be
> >>>>>>>>> reverted and we should use cpu_type instead.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >>>>>>>>> ---
> >>>>>>>>>
> >>>>>>>>> hw/core/machine.c | 11 +++++------
> >>>>>>>>> 1 file changed, 5 insertions(+), 6 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
> >>>>>>>>> index cdc1163dc6..de5bac1c84 100644
> >>>>>>>>> --- a/hw/core/machine.c
> >>>>>>>>> +++ b/hw/core/machine.c
> >>>>>>>>> @@ -776,13 +776,12 @@ void machine_run_board_init(MachineState *machine)
> >>>>>>>>> /* If the machine supports the valid_cpu_types check and the user
> >>>>>>>>> * specified a CPU with -cpu check here that the user CPU is supported.
> >>>>>>>>> */
> >>>>>>>>> - if (machine_class->valid_cpu_types && machine->cpu_type) {
> >>>>>>>>> - ObjectClass *class = object_class_by_name(machine->cpu_type);
> >>>>>>>>> + if (machine_class->valid_cpu_types && machine->cpu_model) {
> >>>>>>>>> int i;
> >>>>>>>>>
> >>>>>>>>> for (i = 0; machine_class->valid_cpu_types[i]; i++) {
> >>>>>>>>> - if (object_class_dynamic_cast(class,
> >>>>>>>>> - machine_class->valid_cpu_types[i])) {
> >>>>>>>>> + if (!strcmp(machine->cpu_model,
> >>>>>>>>> + machine_class->valid_cpu_types[i])) {
> >>>>>>>>
> >>>>>>>> I would rename valid_cpu_types to valid_cpu_models to make the
> >>>>>>>> new semantics clearer.
> >>>>>>>>
> >>>>>>>> Anyway, I have bad and good news:
> >>>>>>>>
> >>>>>>>> The bad news is Igor already sent patches last week that remove
> >>>>>>>> MachineState::cpu_model, so this conflicts with his series. Now
> >>>>>>>> parse_cpu_model() will be the only place where the original CPU model name is
> >>>>>>>> available, but the function needs to work on *-user too. See:
> >>>>>>>> "[PATCH v3 23/25] Use cpu_create(type) instead of cpu_init(cpu_model)".
> >>>>>>>>
> >>>>>>>> The good news is that I think we can fix this very easily if
> >>>>>>>> validation is done at the same place where parse_cpu_model() is
> >>>>>>>> called. e.g.:
> >>>>>>>>
> >>>>>>>> current_machine->cpu_type = machine_class->default_cpu_type;
> >>>>>>>> if (cpu_model) {
> >>>>>>>> current_machine->cpu_type = parse_cpu_model(cpu_model);
> >>>>>>>>
> >>>>>>>> if (machine_class->valid_cpu_models) {
> >>>>>>>> ObjectClass *class = object_class_by_name(machine->cpu_type);
> >>>>>>>> int i;
> >>>>>>>>
> >>>>>>>> for (i = 0; machine_class->valid_cpu_models[i]; i++) {
> >>>>>>>> const char *valid_model = machine_class->valid_cpu_models[i];
> >>>>>>>> ObjectClass *valid_class = cpu_class_by_name(machine->cpu_type, valid_model);
> >>>>>>>> if (object_class_dynamic_cast(class,
> >>>>>>>> object_class_get_name(valid_class))) {
> >>>>>>>> /* Valid CPU type, we're good to go */
> >>>>>>>> break;
> >>>>>>>> }
> >>>>>>>> }
> >>>>>>>> if (!machine_class->valid_cpu_models[i]) {
> >>>>>>>> error_report("Invalid CPU model: %s", cpu_model);
> >>>>>>>> error_printf("The valid CPU models are: %s",
> >>>>>>>> machine_class->valid_cpu_models[0]);
> >>>>>>>> for (i = 1; machine_class->valid_cpu_models[i]; i++) {
> >>>>>>>> error_printf(", %s", machine_class->valid_cpu_models[i]);
> >>>>>>>> }
> >>>>>>>> error_printf("\n");
> >>>>>>>> exit(1);
> >>>>>>>> }
> >>>>>>>> }
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> This can be done inside main(), or moved inside
> >>>>>>>> machine_run_board_init() if main() pass cpu_model as argument to
> >>>>>>>> the function.
> >>>>>>>>
> >>>>>>>> On either case, I think it's a good idea to do validation and
> >>>>>>>> printing of error messages closer to the code that parses the
> >>>>>>>> command-line options. This way we separate parsing/validation
> >>>>>>>> from initialization.
> >>>>>>> I agree it's better like you suggest as at least it prevents
> >>>>>>> ms->cpu_model creeping back into boards code.
> >>>>>>>
> >>>>>>> But I still dislike (hate) an idea of new code adding non
> >>>>>>> canonized cpu_model strings back in the boards code.
> >>>>>>> It's just a matter of time when someone would use them
> >>>>>>> and cpu_model parsing will creep back into boards.
> >>>>>>>
> >>>>>>> It would be much better to if we add
> >>>>>>> char *MachineClass::cpu_name_by_type_name(char *cpu_type)
> >>>>>>> callback and let machines in this patchset to set it,
> >>>>>>> something along following lines which is not much of
> >>>>>>> refactoring and allows for gradual conversion:
> >>>>>>>
> >>>>>>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> >>>>>>> index 9631670..85cca84 100644
> >>>>>>> --- a/target/arm/cpu.h
> >>>>>>> +++ b/target/arm/cpu.h
> >>>>>>> @@ -2885,4 +2885,6 @@ static inline void *arm_get_el_change_hook_opaque(ARMCPU *cpu)
> >>>>>>> return cpu->el_change_hook_opaque;
> >>>>>>> }
> >>>>>>>
> >>>>>>> +char *arm_cpu_name_by_type_name(const char *typename);
> >>>>>>> +
> >>>>>>> #endif
> >>>>>>> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> >>>>>>> index f936017..ae6adb7 100644
> >>>>>>> --- a/hw/arm/netduino2.c
> >>>>>>> +++ b/hw/arm/netduino2.c
> >>>>>>> @@ -46,6 +46,7 @@ static void netduino2_machine_init(MachineClass *mc)
> >>>>>>> mc->desc = "Netduino 2 Machine";
> >>>>>>> mc->init = netduino2_init;
> >>>>>>> mc->ignore_memory_transaction_failures = true;
> >>>>>>> + mc->cpu_name_by_type_name = arm_cpu_name_by_type_name:
> >>>>>>
> >>>>>> I really don't want to introduce a new arch-specific hook just
> >>>>>> for that. We should move CPU type lookup logic to common code
> >>>>>> and make it unnecessary to write new hooks.
> >>>>> unfortunately cpu_model (cpu name part) is target specific
> >>>>> and it's translation to type and back is target specific mayhem.
> >>>>
> >>>> Why can't the model<->type translation be represented as data?
> >>>> We could have simple cpu_type_name_suffix + an alias table.
> >>>>
> >>>> We have at least 4 arches that return a constant at
> >>>> class_by_name. We have at least 10 arches that simply add a
> >>>> suffix to the CPU model name. We must make them use common code
> >>>> instead of requiring them to implement yet another hook[1].
> >>> True, some of them could use generic hook and reduce
> >>> code duplication greatly, we should do it regardless of whether
> >>> table or target specific func approach is used.
> >>>
> >>>> In addition to the ones above, we have 3 that seem to just need
> >>>> an alias table (cris, superh, alpha). ppc can probably also use
> >>>> an alias table for the ppc_cpu_class_by_pvr() stuff. sparc just
> >>>> needs whitespaces translated to '-' (sparc), which can be done
> >>>> using an alias table.
> >>>>
> >>>> In the end I couldn't find any example that can't be represented
> >>>> by a suffix + alias table.
> >>>
> >>> Table based approach is possible but it won't be as simple
> >>> as you've just pictured it.
> >>>
> >>> From what I recall from cpu_class_by_name cleanups table should be able
> >>> to describe cases like (sometimes combination of them):
> >>> * 1:1 mapping - where cpu_model == cpu_type
> >>> * cpu_model <==> cpu_model + suffix - most common usecase
> >>> * cpu_model <==> prefix cpu_model - riscv patches on list are trying to add such cpu types
> >>> * NULL => some_fixed type
> >>> * case (in) sensitive flag
> >>> * garbage => some_fixed type
> >>> * substitutions
> >>> * aliases (sometimes dynamic depending on --enable-kvm (PPC))
> >>> Maybe something else.
> >>>
> >>> We can think about it at leisure but I can't say if new approach
> >>> complexity it's worth of the effort.
> >>>
> >>> It would be nice see impl, but it's a lot of refactoring that's
> >>> clearly out of scope of this series.
> >>> I'd prefer small incremental refactoring (if possible) that
> >>> won't scare people of and easy to review vs a huge one.
> >>>
> >>>>> So I'd prefer having both back and forth functions together in
> >>>>> one place. And common code to call them when necessary.
> >>>>>
> >>>>> We could do global cpu_name_by_type_name() instead of hook,
> >>>>> which I'd prefer even more but then conversion can't be done
> >>>>> only for one target but rather for all targets at once.
> >>>>
> >>>> I don't mind letting a few targets override default behavior with
> >>>> a hook if really necessary, but I have a problem with requiring
> >>>> all targets to implement what's basically the same boilerplate
> >>>> code to add/remove a string suffix and translating aliases.
> >>> it could be generic helper if target does the same plus
> >>> not mandatory at that (in case target/board doesn't care
> >>> about valid cpus).
> >>>
> >>>>>> I agree it would be better if we had a cpu_name_by_type_name()
> >>>>>> function, but I would like to have it implemented cleanly.
> >>>>> In some cases(targets) it can be common helper, but in other
> >>>>> cases it's not so.
> >>>>> My suggestion though allows to do gradual conversion and
> >>>>> avoid putting cpu_model names back in board's code (which I just manged to remove).
> >>>>> Once all targets converted and relevant code is isolated
> >>>>> we can attempt to generalize it if it's possible or at least
> >>>>> make of it global per target helper to get rid of
> >>>>> temporary machine hook.
> >>>>>
> >>>>> (seeing this series reposted with cpu_model names in boards code,
> >>>>> it doesn't looks like author would like to implement tree-wide
> >>>>> generalization first)
> >>>>
> >>>> Well, if nobody is willing to generalize all target-specific code
> >>>> right now, I don't see the harm in having cpu_model-based tables
> >>>> in a few boards in the meantime (as this patch series does). But
> >>>> I do see harm in requiring all our 20 targets to implement yet
> >>>> another hook and increasing the costs of cleaning up the mess
> >>>> later.
> >>> If we use MachineClass hook then it might be done per target
> >>> on demand, so no one would require that every target should
> >>> implement it.
> >>> Also there could be a generic helper for targets that do the same.
> >>> Machine which needs to enable valid_cpus, will have to use generic
> >>> hook impl or provide target specific if it's special case.
> >>>
> >>> Though I do see harm in adding cpu_model tables in boards code
> >>> vs target specific hooks on demand as that will be copy-pasted
> >>> in other boards afterwards (number of which is bigger compared
> >>> to targets count) and ultimately it would duplicate cpu_name
> >>> strings in every board vs hook approach where cpu_model could
> >>> be calculated from cpu_type by a function (generic or
> >>> target specific).
> >>>
> >>> Good thing about hook is that it's non intrusive and
> >>> isolates(consolidates) existing cpu_type -> cpu_model
> >>> conversion in multiple places into one place.
> >>> Then later it would be easier to generalize if someone
> >>> decides to do it.
> >>
> >> I wonder how you want to proceed with this series, the first patch got
> >> merged (c9cf636d48f) but after your "CPU model name" rework, this commit
> >> seems now not very complete/usable.
> >>
> >> Rebasing this series, i.e. with this snippet:
> >>
> >> -- >8 --
> >> diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
> >> index f57fc38f92..cca4ec6648 100644
> >> --- a/hw/arm/netduino2.c
> >> +++ b/hw/arm/netduino2.c
> >> @@ -34,7 +34,7 @@ static void netduino2_init(MachineState *machine)
> >> DeviceState *dev;
> >>
> >> dev = qdev_create(NULL, TYPE_STM32F205_SOC);
> >> - qdev_prop_set_string(dev, "cpu-type", ARM_CPU_TYPE_NAME("cortex-m3"));
> >> + qdev_prop_set_string(dev, "cpu-type", machine->cpu_type);
> >> object_property_set_bool(OBJECT(dev), true, "realized", &error_fatal);
> >>
> >> armv7m_load_kernel(ARM_CPU(first_cpu), machine->kernel_filename,
> >> @@ -43,8 +43,14 @@ static void netduino2_init(MachineState *machine)
> >>
> >> static void netduino2_machine_init(MachineClass *mc)
> >> {
> >> + static const char *valid_cpus[] = {
> >> + ARM_CPU_TYPE_NAME("cortex-m3"),
> >> + ARM_CPU_TYPE_NAME("cortex-m4"),
> >> + NULL
> >> + };
> >> mc->desc = "Netduino 2 Machine";
> >> mc->init = netduino2_init;
> >> + mc->valid_cpu_types = valid_cpus;
> >> mc->ignore_memory_transaction_failures = true;
> >> }
> >> ---
> >>
> >> We get cpu names with suffix:
> >>
> >> $ arm-softmmu/qemu-system-arm -M netduino2 -cpu arm926
> >> qemu-system-arm: Invalid CPU type: arm926-arm-cpu
> >> The valid types are: cortex-m3-arm-cpu, cortex-m4-arm-cpu
> >>
> >> I understand you won't want a global cpu_name_by_type_name, how do you
> >> want to do then?
> >>
> >> Should we define an automatically expanded TARGET_CPU_TYPE_SUFFIX?
> >> Then we could have generic machine code to parse the names.
> > It would work only for some cases,
> > problem is that we have a zoo of naming schemes.
> > Considering that cpus models are used widely we probably can't
> > deprecate it outright (for versioned machine types).
> >
> > Instead of wasting resources on translating cpu-type => 'cpu-name',
> > (hook or lookup tables) how about simplifying code and making all
> > boards accept full typenames?
> >
> > It could be handled in generic way and then printing error with
> > full type names would be acceptable since user would be able to feed it
> > to -cpu.
> >
> > '-cpu help' - would need some work to display types as well (also could be generic)
> >
> > Perhaps with it we could deprecate cpu_models for non versioned
> > machines/targets, which in most cases would allow us to drop special
> > suffix/prefix/nonsense/case-sensitive/substitutions and whatever else
> > is already existing and keep exiting translation routines (hooks) only
> > for versioned machine types as necessary evil.
Maybe we don't even need to keep it versioned machine types, considering
change mapping is static and doesn't influence ABI/migration.
Management interface could just translate cpu_model to type name
for new QEMU.
That would allow us to remove quite a bit target specific code
that deals with all permutations of CPU model and replace a bunch of
'-cpu help' target specific handlers with a generic enumeration
of built-in CPU types.
> Yes. Eduardo and you should write some lines to explain this, and then
> we will follow :)
Unfortunately I don't recall details anymore. One could check out all
implementations of class_by_name callbacks to find out current state.
> I feel concerned because:
> 1/ Alistair series is very helpful to new users, and
> 2/ As the RX architecture series showed, today it is not very clear how
> to correctly use cpu_models.
That's why I'm pushing for supporting only type names whenever possible
so there won't be confusion anymore. Using types is consistent with
-device and some QMP interfaces, enforces strict error checking so
user would get error on nonsense input.
> Eduardo is working on a series, I'll wait for his work.
>
> Regards,
>
> Phil.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2019-06-17 15:33 ` Igor Mammedov
@ 2019-06-17 16:27 ` Eduardo Habkost
2019-06-18 11:34 ` Igor Mammedov
0 siblings, 1 reply; 33+ messages in thread
From: Eduardo Habkost @ 2019-06-17 16:27 UTC (permalink / raw)
To: Igor Mammedov
Cc: marcel, peter.maydell, Philippe Mathieu-Daudé,
qemu-devel, alistair23
On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote:
> On Mon, 17 Jun 2019 17:15:21 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
[...]
> > Yes. Eduardo and you should write some lines to explain this, and then
> > we will follow :)
> Unfortunately I don't recall details anymore. One could check out all
> implementations of class_by_name callbacks to find out current state.
See this message for a summary of existing class_by_name quirks:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html
Date: Wed, 08 May 2019 10:34:44 +0200
Message-ID: <877eb173a3.fsf@dusky.pond.sub.org>
Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions
I'm unsure about Igor's suggestion to get rid of CPU model names
and use only QOM type names in external interfaces. In either
case, we can still simplify the rules rules and reduce the amount
of arch-specific code.
--
Eduardo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2019-06-17 16:27 ` Eduardo Habkost
@ 2019-06-18 11:34 ` Igor Mammedov
2019-06-18 13:55 ` Eduardo Habkost
0 siblings, 1 reply; 33+ messages in thread
From: Igor Mammedov @ 2019-06-18 11:34 UTC (permalink / raw)
To: Eduardo Habkost
Cc: marcel, peter.maydell, Philippe Mathieu-Daudé,
qemu-devel, alistair23
On Mon, 17 Jun 2019 13:27:00 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote:
> > On Mon, 17 Jun 2019 17:15:21 +0200
> > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> [...]
> > > Yes. Eduardo and you should write some lines to explain this, and then
> > > we will follow :)
> > Unfortunately I don't recall details anymore. One could check out all
> > implementations of class_by_name callbacks to find out current state.
>
> See this message for a summary of existing class_by_name quirks:
>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html
> Date: Wed, 08 May 2019 10:34:44 +0200
> Message-ID: <877eb173a3.fsf@dusky.pond.sub.org>
> Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions
>
> I'm unsure about Igor's suggestion to get rid of CPU model names
> and use only QOM type names in external interfaces. In either
> case, we can still simplify the rules rules and reduce the amount
> of arch-specific code.
as far as we have cpu_class_by_name, we have to watch over that
new patches/targets won't add some custom handling/fallbac/whatnot.
On contrary -device works just with type names for all devices,
applying the same to -cpu which is basically translator
model->type[,-global type.foo,...]
would be consistent with -device and less confusing for everyone
(not counting significant code reduction).
It would certainly simplify contributing new targets as contributor
won't have to care about cpu model naming and do something about it.
This option wasn't considered before because we didn't have deprecation
back then, but now it opens possibility to simplify qemu and consolidate
naming. (we probably would be able to fold '-cpu help' into '-device help'
as well).
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2019-06-18 11:34 ` Igor Mammedov
@ 2019-06-18 13:55 ` Eduardo Habkost
2019-06-20 9:02 ` Igor Mammedov
0 siblings, 1 reply; 33+ messages in thread
From: Eduardo Habkost @ 2019-06-18 13:55 UTC (permalink / raw)
To: Igor Mammedov
Cc: marcel, peter.maydell, Philippe Mathieu-Daudé,
qemu-devel, alistair23
On Tue, Jun 18, 2019 at 01:34:10PM +0200, Igor Mammedov wrote:
> On Mon, 17 Jun 2019 13:27:00 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote:
> > > On Mon, 17 Jun 2019 17:15:21 +0200
> > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > [...]
> > > > Yes. Eduardo and you should write some lines to explain this, and then
> > > > we will follow :)
> > > Unfortunately I don't recall details anymore. One could check out all
> > > implementations of class_by_name callbacks to find out current state.
> >
> > See this message for a summary of existing class_by_name quirks:
> >
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html
> > Date: Wed, 08 May 2019 10:34:44 +0200
> > Message-ID: <877eb173a3.fsf@dusky.pond.sub.org>
> > Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions
> >
> > I'm unsure about Igor's suggestion to get rid of CPU model names
> > and use only QOM type names in external interfaces. In either
> > case, we can still simplify the rules rules and reduce the amount
> > of arch-specific code.
> as far as we have cpu_class_by_name, we have to watch over that
> new patches/targets won't add some custom handling/fallbac/whatnot.
We can get rid of CPUClass::cpu_class_by_name() without changing
the external interfaces provided by QEMU.
I don't have a strong opinion about using only QOM types at -cpu,
yet. But first we need to get rid of the arch-specific CPU model
name exceptions enumerated at the URL above (which would be very
welcome).
>
> On contrary -device works just with type names for all devices,
> applying the same to -cpu which is basically translator
> model->type[,-global type.foo,...]
> would be consistent with -device and less confusing for everyone
> (not counting significant code reduction).
> It would certainly simplify contributing new targets as contributor
> won't have to care about cpu model naming and do something about it.
>
> This option wasn't considered before because we didn't have deprecation
> back then, but now it opens possibility to simplify qemu and consolidate
> naming. (we probably would be able to fold '-cpu help' into '-device help'
> as well).
>
--
Eduardo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2019-06-18 13:55 ` Eduardo Habkost
@ 2019-06-20 9:02 ` Igor Mammedov
2019-06-20 14:43 ` Eduardo Habkost
0 siblings, 1 reply; 33+ messages in thread
From: Igor Mammedov @ 2019-06-20 9:02 UTC (permalink / raw)
To: Eduardo Habkost
Cc: marcel, peter.maydell, Philippe Mathieu-Daudé,
qemu-devel, alistair23
On Tue, 18 Jun 2019 10:55:16 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Jun 18, 2019 at 01:34:10PM +0200, Igor Mammedov wrote:
> > On Mon, 17 Jun 2019 13:27:00 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >
> > > On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote:
> > > > On Mon, 17 Jun 2019 17:15:21 +0200
> > > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > > [...]
> > > > > Yes. Eduardo and you should write some lines to explain this, and then
> > > > > we will follow :)
> > > > Unfortunately I don't recall details anymore. One could check out all
> > > > implementations of class_by_name callbacks to find out current state.
> > >
> > > See this message for a summary of existing class_by_name quirks:
> > >
> > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html
> > > Date: Wed, 08 May 2019 10:34:44 +0200
> > > Message-ID: <877eb173a3.fsf@dusky.pond.sub.org>
> > > Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions
> > >
> > > I'm unsure about Igor's suggestion to get rid of CPU model names
> > > and use only QOM type names in external interfaces. In either
> > > case, we can still simplify the rules rules and reduce the amount
> > > of arch-specific code.
> > as far as we have cpu_class_by_name, we have to watch over that
> > new patches/targets won't add some custom handling/fallbac/whatnot.
>
> We can get rid of CPUClass::cpu_class_by_name() without changing
> the external interfaces provided by QEMU.
if you mean QMP, than it is possible to keep model there where
it already exists. Based on experiment [1](x86) I did, it's local to
affected target and doesn't pollute other code.
> I don't have a strong opinion about using only QOM types at -cpu,
> yet. But first we need to get rid of the arch-specific CPU model
> name exceptions enumerated at the URL above (which would be very
> welcome).
One way to get rid of them is to deprecate them in favor of strict
match (no fallback/substitutions/aliases) to typename and when we
drop it make switch type based naming only.
1) I've just took a quick look at how much of duplicated/confusing
code we could get rid off if we drop cpu_class_by_name/*_cpu_list.
It amounts to >800LOC of trivial removal (not counting ppc/s390
that depend on model naming heavily and in need of some non
trivial refactoring)
>
> >
> > On contrary -device works just with type names for all devices,
> > applying the same to -cpu which is basically translator
> > model->type[,-global type.foo,...]
> > would be consistent with -device and less confusing for everyone
> > (not counting significant code reduction).
> > It would certainly simplify contributing new targets as contributor
> > won't have to care about cpu model naming and do something about it.
> >
> > This option wasn't considered before because we didn't have deprecation
> > back then, but now it opens possibility to simplify qemu and consolidate
> > naming. (we probably would be able to fold '-cpu help' into '-device help'
> > as well).
> >
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2019-06-20 9:02 ` Igor Mammedov
@ 2019-06-20 14:43 ` Eduardo Habkost
2020-01-23 18:48 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 33+ messages in thread
From: Eduardo Habkost @ 2019-06-20 14:43 UTC (permalink / raw)
To: Igor Mammedov
Cc: marcel, peter.maydell, Philippe Mathieu-Daudé,
qemu-devel, alistair23
On Thu, Jun 20, 2019 at 11:02:39AM +0200, Igor Mammedov wrote:
> On Tue, 18 Jun 2019 10:55:16 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
>
> > On Tue, Jun 18, 2019 at 01:34:10PM +0200, Igor Mammedov wrote:
> > > On Mon, 17 Jun 2019 13:27:00 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >
> > > > On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote:
> > > > > On Mon, 17 Jun 2019 17:15:21 +0200
> > > > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > > > [...]
> > > > > > Yes. Eduardo and you should write some lines to explain this, and then
> > > > > > we will follow :)
> > > > > Unfortunately I don't recall details anymore. One could check out all
> > > > > implementations of class_by_name callbacks to find out current state.
> > > >
> > > > See this message for a summary of existing class_by_name quirks:
> > > >
> > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html
> > > > Date: Wed, 08 May 2019 10:34:44 +0200
> > > > Message-ID: <877eb173a3.fsf@dusky.pond.sub.org>
> > > > Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions
> > > >
> > > > I'm unsure about Igor's suggestion to get rid of CPU model names
> > > > and use only QOM type names in external interfaces. In either
> > > > case, we can still simplify the rules rules and reduce the amount
> > > > of arch-specific code.
> > > as far as we have cpu_class_by_name, we have to watch over that
> > > new patches/targets won't add some custom handling/fallbac/whatnot.
> >
> > We can get rid of CPUClass::cpu_class_by_name() without changing
> > the external interfaces provided by QEMU.
> if you mean QMP, than it is possible to keep model there where
> it already exists. Based on experiment [1](x86) I did, it's local to
> affected target and doesn't pollute other code.
I mean both command line and QMP.
>
> > I don't have a strong opinion about using only QOM types at -cpu,
> > yet. But first we need to get rid of the arch-specific CPU model
> > name exceptions enumerated at the URL above (which would be very
> > welcome).
> One way to get rid of them is to deprecate them in favor of strict
> match (no fallback/substitutions/aliases) to typename and when we
> drop it make switch type based naming only.
This is true, but is it desirable? Aren't we just moving
complexity elsewhere? Management software would still need to
translate CPU models from existing VM configurations to QOM type
names.
>
> 1) I've just took a quick look at how much of duplicated/confusing
> code we could get rid off if we drop cpu_class_by_name/*_cpu_list.
> It amounts to >800LOC of trivial removal (not counting ppc/s390
> that depend on model naming heavily and in need of some non
> trivial refactoring)
Removing the code might be trivial. Coordinating with the
maintainers of all architectures to confirm this is really
something everybody wants is the difficult part.
If you really want to do it, please make sure all the
architecture maintainers (and libvirt developers) are involved in
the discussion.
>
> >
> > >
> > > On contrary -device works just with type names for all devices,
> > > applying the same to -cpu which is basically translator
> > > model->type[,-global type.foo,...]
> > > would be consistent with -device and less confusing for everyone
> > > (not counting significant code reduction).
> > > It would certainly simplify contributing new targets as contributor
> > > won't have to care about cpu model naming and do something about it.
> > >
> > > This option wasn't considered before because we didn't have deprecation
> > > back then, but now it opens possibility to simplify qemu and consolidate
> > > naming. (we probably would be able to fold '-cpu help' into '-device help'
> > > as well).
> > >
> >
>
--
Eduardo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2019-06-20 14:43 ` Eduardo Habkost
@ 2020-01-23 18:48 ` Philippe Mathieu-Daudé
2020-02-06 20:59 ` Eduardo Habkost
0 siblings, 1 reply; 33+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-01-23 18:48 UTC (permalink / raw)
To: Eduardo Habkost, Igor Mammedov, Markus Armbruster
Cc: marcel, peter.maydell, qemu-devel, alistair23
On 6/20/19 4:43 PM, Eduardo Habkost wrote:
> On Thu, Jun 20, 2019 at 11:02:39AM +0200, Igor Mammedov wrote:
>> On Tue, 18 Jun 2019 10:55:16 -0300
>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>
>>> On Tue, Jun 18, 2019 at 01:34:10PM +0200, Igor Mammedov wrote:
>>>> On Mon, 17 Jun 2019 13:27:00 -0300
>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>
>>>>> On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote:
>>>>>> On Mon, 17 Jun 2019 17:15:21 +0200
>>>>>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>>> [...]
>>>>>>> Yes. Eduardo and you should write some lines to explain this, and then
>>>>>>> we will follow :)
>>>>>> Unfortunately I don't recall details anymore. One could check out all
>>>>>> implementations of class_by_name callbacks to find out current state.
>>>>>
>>>>> See this message for a summary of existing class_by_name quirks:
>>>>>
>>>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html
>>>>> Date: Wed, 08 May 2019 10:34:44 +0200
>>>>> Message-ID: <877eb173a3.fsf@dusky.pond.sub.org>
>>>>> Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions
>>>>>
>>>>> I'm unsure about Igor's suggestion to get rid of CPU model names
>>>>> and use only QOM type names in external interfaces. In either
>>>>> case, we can still simplify the rules rules and reduce the amount
>>>>> of arch-specific code.
>>>> as far as we have cpu_class_by_name, we have to watch over that
>>>> new patches/targets won't add some custom handling/fallbac/whatnot.
>>>
>>> We can get rid of CPUClass::cpu_class_by_name() without changing
>>> the external interfaces provided by QEMU.
>> if you mean QMP, than it is possible to keep model there where
>> it already exists. Based on experiment [1](x86) I did, it's local to
>> affected target and doesn't pollute other code.
>
> I mean both command line and QMP.
>
>>
>>> I don't have a strong opinion about using only QOM types at -cpu,
>>> yet. But first we need to get rid of the arch-specific CPU model
>>> name exceptions enumerated at the URL above (which would be very
>>> welcome).
>> One way to get rid of them is to deprecate them in favor of strict
>> match (no fallback/substitutions/aliases) to typename and when we
>> drop it make switch type based naming only.
>
> This is true, but is it desirable? Aren't we just moving
> complexity elsewhere? Management software would still need to
> translate CPU models from existing VM configurations to QOM type
> names.
>
>>
>> 1) I've just took a quick look at how much of duplicated/confusing
>> code we could get rid off if we drop cpu_class_by_name/*_cpu_list.
>> It amounts to >800LOC of trivial removal (not counting ppc/s390
>> that depend on model naming heavily and in need of some non
>> trivial refactoring)
>
> Removing the code might be trivial. Coordinating with the
> maintainers of all architectures to confirm this is really
> something everybody wants is the difficult part.
>
> If you really want to do it, please make sure all the
> architecture maintainers (and libvirt developers) are involved in
> the discussion.
From the previous link (summary of existing class_by_name quirks):
https://www.mail-archive.com/qemu-devel@nongnu.org/msg615737.html
"Deprecating unwanted stuff now is likely to make a
later cleanup so much easier."
This was 8 months ago :/
IIUC we need to restart this topic addressing the libvirt community
first, then see with the QEMU one?
>
>>
>>>
>>>>
>>>> On contrary -device works just with type names for all devices,
>>>> applying the same to -cpu which is basically translator
>>>> model->type[,-global type.foo,...]
>>>> would be consistent with -device and less confusing for everyone
>>>> (not counting significant code reduction).
>>>> It would certainly simplify contributing new targets as contributor
>>>> won't have to care about cpu model naming and do something about it.
>>>>
>>>> This option wasn't considered before because we didn't have deprecation
>>>> back then, but now it opens possibility to simplify qemu and consolidate
>>>> naming. (we probably would be able to fold '-cpu help' into '-device help'
>>>> as well).
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model
2020-01-23 18:48 ` Philippe Mathieu-Daudé
@ 2020-02-06 20:59 ` Eduardo Habkost
0 siblings, 0 replies; 33+ messages in thread
From: Eduardo Habkost @ 2020-02-06 20:59 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: peter.maydell, Markus Armbruster, qemu-devel, alistair23, marcel,
Igor Mammedov
On Thu, Jan 23, 2020 at 07:48:21PM +0100, Philippe Mathieu-Daudé wrote:
> On 6/20/19 4:43 PM, Eduardo Habkost wrote:
> > On Thu, Jun 20, 2019 at 11:02:39AM +0200, Igor Mammedov wrote:
> > > On Tue, 18 Jun 2019 10:55:16 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >
> > > > On Tue, Jun 18, 2019 at 01:34:10PM +0200, Igor Mammedov wrote:
> > > > > On Mon, 17 Jun 2019 13:27:00 -0300
> > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > > On Mon, Jun 17, 2019 at 05:33:43PM +0200, Igor Mammedov wrote:
> > > > > > > On Mon, 17 Jun 2019 17:15:21 +0200
> > > > > > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > > > > > [...]
> > > > > > > > Yes. Eduardo and you should write some lines to explain this, and then
> > > > > > > > we will follow :)
> > > > > > > Unfortunately I don't recall details anymore. One could check out all
> > > > > > > implementations of class_by_name callbacks to find out current state.
> > > > > >
> > > > > > See this message for a summary of existing class_by_name quirks:
> > > > > >
> > > > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg615503.html
> > > > > > Date: Wed, 08 May 2019 10:34:44 +0200
> > > > > > Message-ID: <877eb173a3.fsf@dusky.pond.sub.org>
> > > > > > Subject: Re: [Qemu-devel] [PATCH 0/7] Delete 16 *_cpu_class_by_name() functions
> > > > > >
> > > > > > I'm unsure about Igor's suggestion to get rid of CPU model names
> > > > > > and use only QOM type names in external interfaces. In either
> > > > > > case, we can still simplify the rules rules and reduce the amount
> > > > > > of arch-specific code.
> > > > > as far as we have cpu_class_by_name, we have to watch over that
> > > > > new patches/targets won't add some custom handling/fallbac/whatnot.
> > > >
> > > > We can get rid of CPUClass::cpu_class_by_name() without changing
> > > > the external interfaces provided by QEMU.
> > > if you mean QMP, than it is possible to keep model there where
> > > it already exists. Based on experiment [1](x86) I did, it's local to
> > > affected target and doesn't pollute other code.
> >
> > I mean both command line and QMP.
> >
> > >
> > > > I don't have a strong opinion about using only QOM types at -cpu,
> > > > yet. But first we need to get rid of the arch-specific CPU model
> > > > name exceptions enumerated at the URL above (which would be very
> > > > welcome).
> > > One way to get rid of them is to deprecate them in favor of strict
> > > match (no fallback/substitutions/aliases) to typename and when we
> > > drop it make switch type based naming only.
> >
> > This is true, but is it desirable? Aren't we just moving
> > complexity elsewhere? Management software would still need to
> > translate CPU models from existing VM configurations to QOM type
> > names.
> >
> > >
> > > 1) I've just took a quick look at how much of duplicated/confusing
> > > code we could get rid off if we drop cpu_class_by_name/*_cpu_list.
> > > It amounts to >800LOC of trivial removal (not counting ppc/s390
> > > that depend on model naming heavily and in need of some non
> > > trivial refactoring)
> >
> > Removing the code might be trivial. Coordinating with the
> > maintainers of all architectures to confirm this is really
> > something everybody wants is the difficult part.
> >
> > If you really want to do it, please make sure all the
> > architecture maintainers (and libvirt developers) are involved in
> > the discussion.
>
> From the previous link (summary of existing class_by_name quirks):
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg615737.html
>
> "Deprecating unwanted stuff now is likely to make a
> later cleanup so much easier."
>
> This was 8 months ago :/
>
> IIUC we need to restart this topic addressing the libvirt community first,
> then see with the QEMU one?
The next step is to deprecate the weird and pointless exceptions
listed at the URL you mentioned. Not all exceptions will affect libvirt (maybe most won't).
In either case, we can just use qemu-deprecated.texi to
communicate this to libvirt developers. MAINTAINERS already has
a "R: libvir-list" line for qemu-deprecated.texi.
--
Eduardo
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2020-02-06 21:01 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-02 0:42 [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Alistair Francis
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 1/6] machine: Convert the valid cpu types to use cpu_model Alistair Francis
2018-02-02 18:23 ` Eduardo Habkost
2018-02-05 11:22 ` Igor Mammedov
2018-02-05 13:54 ` Eduardo Habkost
2018-02-05 14:42 ` Igor Mammedov
2018-02-05 22:42 ` Eduardo Habkost
2018-02-06 14:43 ` Igor Mammedov
2019-06-17 5:09 ` Philippe Mathieu-Daudé
2019-06-17 14:43 ` Eduardo Habkost
2019-06-17 15:01 ` Igor Mammedov
2019-06-17 15:15 ` Philippe Mathieu-Daudé
2019-06-17 15:33 ` Igor Mammedov
2019-06-17 16:27 ` Eduardo Habkost
2019-06-18 11:34 ` Igor Mammedov
2019-06-18 13:55 ` Eduardo Habkost
2019-06-20 9:02 ` Igor Mammedov
2019-06-20 14:43 ` Eduardo Habkost
2020-01-23 18:48 ` Philippe Mathieu-Daudé
2020-02-06 20:59 ` Eduardo Habkost
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 2/6] netduino2: Specify the valid CPUs Alistair Francis
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 3/6] bcm2836: Use the Cortex-A7 instead of Cortex-A15 Alistair Francis
2018-02-15 13:23 ` Philippe Mathieu-Daudé
2018-02-15 22:41 ` Alistair Francis
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 4/6] raspi: Specify the valid CPUs Alistair Francis
2018-02-15 11:29 ` Peter Maydell
2018-02-15 13:04 ` Philippe Mathieu-Daudé
2018-02-15 13:17 ` Peter Maydell
2018-02-15 17:08 ` Igor Mammedov
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 5/6] xlnx-zcu102: " Alistair Francis
2018-02-02 0:42 ` [Qemu-devel] [PATCH v5 6/6] xilinx_zynq: " Alistair Francis
2018-03-13 23:13 ` [Qemu-devel] [PATCH v5 0/6] Add a valid_cpu_types property Philippe Mathieu-Daudé
2018-03-21 14:33 ` Igor Mammedov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).