Re: [PATCH v19 02/21] s390x/cpu topology: add topology entries on CPU hotplug

[Pierre Morel <pmorel@linux.ibm.com>]

On 4/4/23 09:31, Cédric Le Goater wrote:
> On 4/3/23 18:28, Pierre Morel wrote:
>>

[...]


>> +
>> +/**
>> + * s390_socket_nb:
>> + * @cpu: s390x CPU
>> + *
>> + * Returns the socket number used inside the cores_per_socket array
>> + * for a cpu.
>> + */
>> +int s390_socket_nb(S390CPU *cpu)
>
> s390_socket_nb() doesn't seem to be used anywhere else than in
> hw/s390x/cpu-topology.c. It should be static.


right

[...]
>> +/**
>> + * s390_topology_add_core_to_socket:
>> + * @cpu: the new S390CPU to insert in the topology structure
>> + * @drawer_id: new drawer_id
>> + * @book_id: new book_id
>> + * @socket_id: new socket_id
>> + * @creation: if is true the CPU is a new CPU and there is no old 
>> socket
>> + *            to handle.
>> + *            if is false, this is a moving the CPU and old socket 
>> count
>> + *            must be decremented.
>> + * @errp: the error pointer
>> + *
>> + */
>> +static void s390_topology_add_core_to_socket(S390CPU *cpu, int 
>> drawer_id,
>> +                                             int book_id, int 
>> socket_id,
>> +                                             bool creation, Error 
>> **errp)
>> +{
>
> Since this routine is called twice, in s390_topology_setup_cpu() for
> creation, and in s390_change_topology() for socket migration, we could
> duplicate the code in two distinct routines.
>
> I think this would simplify a bit each code path and avoid the 'creation'
> parameter which is confusing.


right

Thanks.


>
>
>> +    int old_socket_entry = s390_socket_nb(cpu);
>> +    int new_socket_entry;
>> +
>> +    if (creation) {
>> +        new_socket_entry = old_socket_entry;
>> +    } else {
>> +        new_socket_entry = (drawer_id * s390_topology.smp->books + 
>> book_id) *
>> +                            s390_topology.smp->sockets + socket_id;
>
> A helper common routine that s390_socket_nb() could use also would be 
> a plus.


Yes, thanks


>
>> +    }
>> +
>> +    /* Check for space on new socket */
>> +    if ((new_socket_entry != old_socket_entry) &&
>> +        (s390_topology.cores_per_socket[new_socket_entry] >=
>> +         s390_topology.smp->cores)) {
>> +        error_setg(errp, "No more space on this socket");
>> +        return;
>> +    }
>> +
>> +    /* Update the count of cores in sockets */
>> +    s390_topology.cores_per_socket[new_socket_entry] += 1;
>> +    if (!creation) {
>> +        s390_topology.cores_per_socket[old_socket_entry] -= 1;
>> +    }
>> +}
>> +
>> +/**
>> + * s390_update_cpu_props:
>> + * @ms: the machine state
>> + * @cpu: the CPU for which to update the properties from the 
>> environment.
>> + *
>> + */
>> +static void s390_update_cpu_props(MachineState *ms, S390CPU *cpu)
>> +{
>> +    CpuInstanceProperties *props;
>> +
>> +    props = &ms->possible_cpus->cpus[cpu->env.core_id].props;
>> +
>> +    props->socket_id = cpu->env.socket_id;
>> +    props->book_id = cpu->env.book_id;
>> +    props->drawer_id = cpu->env.drawer_id;
>> +}
>> +
>> +/**
>> + * s390_topology_setup_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.
>
> ... before the CPU is inserted in the topology.
OK
>
>> + * There is no use to update the MTCR explicitely here because it
>
> ... is no need ... sounds better.
OK
>
>> + * will be updated by KVM on creation of the new vCPU.
>
> "CPU" is used everywhere else.
OK
>
>> + */
>> +void s390_topology_setup_cpu(MachineState *ms, S390CPU *cpu, Error 
>> **errp)
>> +{
>> +    ERRP_GUARD();
>> +
>> +    /*
>> +     * We do not want to initialize the topology if the cpu model
>> +     * does not support topology, consequently, we have to wait for
>> +     * the first CPU to be realized, which realizes the CPU model
>> +     * to initialize the topology structures.
>> +     *
>> +     * s390_topology_setup_cpu() is called from the cpu hotplug.
>> +     */
>> +    if (!s390_topology.cores_per_socket) {
>> +        s390_topology_init(ms);
>> +    }
>> +
>> +    s390_topology_cpu_default(cpu, errp);
>> +    if (*errp) {
>
> May be having s390_topology_cpu_default() return a bool would be cleaner.
> Same comment for the routines below. This is minor.


Yes and it is more readable. I do it.


Thanks for the comments.

Regards,

Pierre