On Tue, 2023-01-17 at 14:55 +0100, Pierre Morel wrote:
>
> On 1/13/23 19:15, Nina Schoetterl-Glausch wrote:
> >
[...]
> > > +/**
> > > + * s390_topology_set_entry:
> > > + * @entry: Topology entry to setup
> > > + * @id: topology id to use for the setup
> > > + *
> > > + * Set the core bit inside the topology mask and
> > > + * increments the number of cores for the socket.
> > > + */
> > > +static void s390_topology_set_entry(S390TopologyEntry *entry,
> >
> > Not sure if I like the name, what it does is to add a cpu to the entry.
>
> s390_topology_add_cpu_to_entry() ?
Yeah, that's better.
[...]
>
> > > +/**
> > > + * s390_topology_set_cpu:
> > > + * @ms: MachineState used to initialize the topology structure on
> > > + * first call.
> > > + * @cpu: the new S390CPU to insert in the topology structure
> > > + * @errp: the error pointer
> > > + *
> > > + * Called from CPU Hotplug to check and setup the CPU attributes
> > > + * before to insert the CPU in the topology.
> > > + */
> > > +void s390_topology_set_cpu(MachineState *ms, S390CPU *cpu, Error **errp)
> > > +{
> > > + Error *local_error = NULL;
> >
> > Can't you just use ERRP_GUARD ?
>
> I do not think it is necessary and I find it obfuscating.
> So, should I?
/*
* Propagate error object (if any) from @local_err to @dst_errp.
[...]
* Please use ERRP_GUARD() instead when possible.
* Please don't error_propagate(&error_fatal, ...), use
* error_report_err() and exit(), because that's more obvious.
*/
void error_propagate(Error **dst_errp, Error *local_err);
So I'd say yes.
>
> >
> > > + s390_topology_id id;
> > > +
> > > + /*
> > > + * We do not want to initialize the topology if the cpu model
> > > + * does not support topology consequently, we have to wait for
> >
> > ", consequently," I think. Could you do the initialization some where else,
> > after you know what the cpu model is? Not that I object to doing it this way.
> >
>
> I did not find a better place, it must be done after the CPU model is
> initialize and before the first CPU is created.
> The cpu model is initialized during the early creation of the first cpu.
>
> Any idea?
>
> Thanks.
>
> Regards,
> Pierre
>
