Re: [PATCH v3 07/18] machine: Add a new function init_apicid_fn in MachineClass

[Eduardo Habkost]

On Tue, Jan 28, 2020 at 01:45:31PM -0600, Babu Moger wrote:
> 
> 
> On 1/28/20 10:29 AM, Igor Mammedov wrote:
> > On Tue, 03 Dec 2019 18:37:42 -0600
> > Babu Moger <address@hidden> wrote:
> > 
> >> Add a new function init_apicid_fn in MachineClass to initialize the mode
> >> specific handlers to decode the apic ids.
> >>
> >> Signed-off-by: Babu Moger <address@hidden>
> >> ---
> >>  include/hw/boards.h |    1 +
> >>  vl.c                |    3 +++
> >>  2 files changed, 4 insertions(+)
> >>
> >> diff --git a/include/hw/boards.h b/include/hw/boards.h
> >> index d4fab218e6..ce5aa365cb 100644
> >> --- a/include/hw/boards.h
> >> +++ b/include/hw/boards.h
> >> @@ -238,6 +238,7 @@ struct MachineClass {
> >>                                                           unsigned 
> >> cpu_index);
> >>      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
> >>      int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx);
> >> +    void (*init_apicid_fn)(MachineState *ms);
> > it's x86 specific, so why it wasn put into PCMachineClass?
> 
> Yes. It is x86 specific for now. I tried to make it generic function so
> other OSes can use it if required(like we have done in
> possible_cpu_arch_ids). It initializes functions required to build the
> apicid for each CPUs. We need these functions much early in the
> initialization. It should be initialized before parse_numa_opts or
> machine_run_board_init(in v1.c) which are called from generic context. We
> cannot use PCMachineClass at this time.

Even if the only user of the new hook will be x86, you are
introducing a generic API, so a x86-specific name doesn't seem
appropriate.

I suggest using a generic name and documenting its rules and
intended usage explicitly.  Something like "pre_init" might be
good enough, as long as the rules documented clearly (e.g. it
will be called before NUMA initialization, but after CPU model
lookup).

However, I believe we can implement the same functionality
without a new generic initialization hook.  See my reply to patch
16/18.

> 
> > 
> > 
> >>  };
> >>  
> >>  /**
> >> diff --git a/vl.c b/vl.c
> >> index a42c24a77f..b6af604e11 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> >> @@ -4318,6 +4318,9 @@ int main(int argc, char **argv, char **envp)
> >>      current_machine->cpu_type = machine_class->default_cpu_type;
> >>      if (cpu_option) {
> >>          current_machine->cpu_type = parse_cpu_option(cpu_option);
> >> +        if (machine_class->init_apicid_fn) {
> >> +            machine_class->init_apicid_fn(current_machine);
> >> +        }
> >>      }
> >>      parse_numa_opts(current_machine);
> >>  
> >>
> >>
> > 
> 

-- 
Eduardo