Re: [PATCH v13 1/7] s390x/cpu topology: Creating CPU topology device

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

On 12/9/22 14:50, Thomas Huth wrote:
> On 08/12/2022 10.44, Pierre Morel wrote:
>> We will need a Topology device to transfer the topology
>> during migration and to implement machine reset.
>>
>> The device creation is fenced by s390_has_topology().
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
> [...]
>> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
>> new file mode 100644
>> index 0000000000..b3e59873f6
>> --- /dev/null
>> +++ b/hw/s390x/cpu-topology.c
>> @@ -0,0 +1,149 @@
>> +/*
>> + * CPU Topology
>> + *
>> + * Copyright IBM Corp. 2022
>> + * Author(s): Pierre Morel <pmorel@linux.ibm.com>
>> +
>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
>> (at
>> + * your option) any later version. See the COPYING file in the top-level
>> + * directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/error.h"
>> +#include "qemu/error-report.h"
>> +#include "hw/qdev-properties.h"
>> +#include "hw/boards.h"
>> +#include "qemu/typedefs.h"
>> +#include "target/s390x/cpu.h"
>> +#include "hw/s390x/s390-virtio-ccw.h"
>> +#include "hw/s390x/cpu-topology.h"
>> +
>> +/**
>> + * s390_has_topology
>> + *
>> + * Return false until the commit activating the topology is
>> + * commited.
>> + */
>> +bool s390_has_topology(void)
>> +{
>> +    return false;
>> +}
>> +
>> +/**
>> + * s390_get_topology
>> + *
>> + * Returns a pointer to the topology.
>> + *
>> + * This function is called when we know the topology exist.
>> + * Testing if the topology exist is done with s390_has_topology()
>> + */
>> +S390Topology *s390_get_topology(void)
>> +{
>> +    static S390Topology *s390Topology;
>> +
>> +    if (!s390Topology) {
>> +        s390Topology = S390_CPU_TOPOLOGY(
>> +            object_resolve_path(TYPE_S390_CPU_TOPOLOGY, NULL));
>> +    }
>> +
>> +    assert(s390Topology);
> 
> I think you can move the assert() into the body of the if-statement.

Yes, clearly.

> 
>> +    return s390Topology;
>> +}
>> +
>> +/**
>> + * s390_init_topology
>> + * @machine: The Machine state, used to retrieve the SMP parameters
>> + * @errp: the error pointer in case of problem
>> + *
>> + * This function creates and initialize the S390Topology with
>> + * the QEMU -smp parameters we will use during adding cores to the
>> + * topology.
>> + */
>> +void s390_init_topology(MachineState *machine, Error **errp)
>> +{
>> +    DeviceState *dev;
>> +
>> +    if (machine->smp.threads > 1) {
>> +        error_setg(errp, "CPU Topology do not support multithreading");
> 
> s/CPU Toplogy do/CPU topology does/

Yes, thanks.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen