Re: [PATCH v14 02/11] s390x/cpu topology: add topology entries on CPU hotplug

[Nina Schoetterl-Glausch]

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
>