Hi Igor,
On 7/14/23 22:07, Igor Mammedov wrote:
On Thu, 13 Jul 2023 15:45:00 +1000
Gavin Shan <gs...@redhat.com> wrote:
The CPU type invalidation logic in machine_run_board_init() is
independent enough. Lets factor it out into helper validate_cpu_type().
Since we're here, the relevant comments are improved a bit.
No functional change intended.
Signed-off-by: Gavin Shan <gs...@redhat.com>
---
hw/core/machine.c | 81 +++++++++++++++++++++++++----------------------
1 file changed, 43 insertions(+), 38 deletions(-)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index f0d35c6401..68b866c762 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1349,12 +1349,52 @@ out:
return r;
}
+static void validate_cpu_type(MachineState *machine)
s/validate_cpu_type/is_cpu_type_valid or better is_cpu_type_supported
Is it going to be reused elsewhere (otherwise I don't see much reason to move
code around)?
The logic of checking if the CPU type is supported is independent enough. It's
the only reason why I factored it out into a standalone helper here. It has
been explained in the commit log. Lets have an individual helper for this if
you don't have strong taste. With it, machine_run_board_init() looks a bit more
clean.
I don't have strong opinion about the function name. Shall we return 'bool'
with is_cpu_type_supported()? Something like below. The 'bool' return value
is duplicate to 'local_err' in machine_run_board_init(). So I think the
function validate_cpu_type(machine, errp) looks good to me. Igor, could you
please help to confirm?
static bool is_cpu_type_supported(MachineState *machine, Error **errp)
{
bool supported = true;
:
if (!machine_class->valid_cpu_types[i]) {
error_setg(errp, "Invalid CPU type: %s", machine->cpu_type));
error_append_hint(errp, "The valid types are: %s", model);
for (i = 1; machine_class->valid_cpu_types[i]; i++) {
error_append_hint(errp, ", %s", model);
}
error_append_hint(errp, "\n");
supported = false;
}
:
return supported;
}
void machine_run_board_init(MachineState *machine, const char *mem_path, Error
**errp)
{
Error *local_err = NULL;
:
/* These two conditions are duplicate to each other! */
if (!is_cpu_type_supported(machine, &local_err) && local_err) {
error_propagate(errp, local_err);
}
:
}
+{
+ MachineClass *machine_class = MACHINE_GET_CLASS(machine);
+ ObjectClass *oc = object_class_by_name(machine->cpu_type);
+ CPUClass *cc = CPU_CLASS(oc);
+ int i;
+
+ /*
+ * Check if the user-specified CPU type is supported when the valid
+ * CPU types have been determined. Note that the user-specified CPU
+ * type is given by '-cpu' option.
+ */
+ if (!machine->cpu_type || !machine_class->valid_cpu_types) {
+ goto out_no_check;
no goto-s please
Ok. Will be dropped in next revision.
+ }
+
+ for (i = 0; machine_class->valid_cpu_types[i]; i++) {
+ if (object_class_dynamic_cast(oc, machine_class->valid_cpu_types[i])) {
+ break;
+ }
+ }
+
+ if (!machine_class->valid_cpu_types[i]) {
+ /* The user-specified CPU type is invalid */
+ error_report("Invalid CPU type: %s", machine->cpu_type);
+ error_printf("The valid types 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]);
+ }
+ error_printf("\n");
+
+ exit(1);
since you are touching that,
turn it in errp handling, in separate patch 1st
and only then introduce your helper.
Right, it's a good idea. I will have a preparatory patch for it where
the error messages will be accumulated to @local_err and finally propagate
it to @errp of machine_run_board_init().
+ }
+
+ /* Check if CPU type is deprecated and warn if so */
+out_no_check:
+ if (cc && cc->deprecation_note) {
+ warn_report("CPU model %s is deprecated -- %s",
+ machine->cpu_type, cc->deprecation_note);
+ }
+}
void machine_run_board_init(MachineState *machine, const char *mem_path, Error **errp)
{
MachineClass *machine_class = MACHINE_GET_CLASS(machine);
- ObjectClass *oc = object_class_by_name(machine->cpu_type);
- CPUClass *cc;
/* This checkpoint is required by replay to separate prior clock
reading from the other reads, because timer polling functions query
@@ -1405,42 +1445,7 @@ void machine_run_board_init(MachineState *machine, const
char *mem_path, Error *
machine->ram = machine_consume_memdev(machine, machine->memdev);
}
- /* 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) {
- int i;
-
- for (i = 0; machine_class->valid_cpu_types[i]; i++) {
- if (object_class_dynamic_cast(oc,
- machine_class->valid_cpu_types[i])) {
- /* The user specificed CPU is in the valid field, we are
- * good to go.
- */
- break;
- }
- }
-
- 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",
- 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]);
- }
- error_printf("\n");
-
- exit(1);
- }
- }
-
- /* Check if CPU type is deprecated and warn if so */
- cc = CPU_CLASS(oc);
- if (cc && cc->deprecation_note) {
- warn_report("CPU model %s is deprecated -- %s", machine->cpu_type,
- cc->deprecation_note);
- }
+ validate_cpu_type(machine);
if (machine->cgs) {
/*
Thanks,
Gavin
