| Message ID | 20230713054502.410911-2-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/virt: Use generic CPU invalidation | expand |
On Thu, 13 Jul 2023 15:45:00 +1000 Gavin Shan <gshan@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 <gshan@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)? > +{ > + 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 > + } > + > + 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. > + } > + > + /* 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) { > /*
Hi Igor, On 7/14/23 22:07, Igor Mammedov wrote: > On Thu, 13 Jul 2023 15:45:00 +1000 > Gavin Shan <gshan@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 <gshan@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
On Tue, 18 Jul 2023 16:11:42 +1000 Gavin Shan <gshan@redhat.com> wrote: > Hi Igor, > > On 7/14/23 22:07, Igor Mammedov wrote: > > On Thu, 13 Jul 2023 15:45:00 +1000 > > Gavin Shan <gshan@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 <gshan@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? I'd check errp and drop bool return, otherwise looks fine to me > > 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 >
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) +{ + 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; + } + + 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); + } + + /* 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) { /*
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 <gshan@redhat.com> --- hw/core/machine.c | 81 +++++++++++++++++++++++++---------------------- 1 file changed, 43 insertions(+), 38 deletions(-)