On Tue, 18 Jul 2023 16:11:42 +1000
Gavin Shan <gs...@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 <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?
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
>
