[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [Qemu-devel] [PATCH v7 01/16] hw/cpu: introduce CPU clust
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-arm] [Qemu-devel] [PATCH v7 01/16] hw/cpu: introduce CPU clusters |
Date: |
Sun, 25 Nov 2018 22:27:04 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.0 |
Hi Eduardo,
On 23/11/18 19:10, Eduardo Habkost wrote:
> Hi,
>
> Sorry for not reviewing this series earlier. I just stumbled
> upon this part of the code:
>
> On Fri, Nov 23, 2018 at 10:17:14AM +0100, Luc Michel wrote:
>> This commit adds the cpu-cluster type. It aims at gathering CPUs from
>> the same cluster in a machine.
>>
>> For now it only has a `cluster-id` property.
>>
>> Signed-off-by: Luc Michel <address@hidden>
>> Reviewed-by: Alistair Francis <address@hidden>
>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>> Tested-by: Philippe Mathieu-Daudé <address@hidden>
>> Reviewed-by: Edgar E. Iglesias <address@hidden>
> [...]
>> +static void cpu_cluster_init(Object *obj)
>> +{
>> + static uint32_t cluster_id_auto_increment;
>> + CPUClusterState *cluster = CPU_CLUSTER(obj);
>> +
>> + cluster->cluster_id = cluster_id_auto_increment++;
>
> I see that you implemented this after a suggestion from Philippe,
> but I'm worried about this kind of side-effect on object/device
> code. I'm afraid this will bite us back in the future. We were
> bitten by problems caused by automatic cpu_index assignment on
> CPU instance_init, and we took a while to fix that.
Oops, thanks for catching this. I'm not aware of the cpu_index past issue.
>
> If you really want to do this and assign cluster_id
> automatically, please do it on realize, where it won't have
> unexpected side-effects after a simple `qom-list-properties` QMP
> command.
This looks clean enough to me.
Do you prefer we don't use static auto_increment at all?
>
> I would also add a huge warning above the cluster_id field
> declaration, mentioning that the field is not supposed to be used
> for anything except debugging. I think there's a large risk of
> people trying to reuse the field incorrectly, just like cpu_index
> was reused for multiple (conflicting) purposes in the past.
>
>
>> +}
>> +
>> +static Property cpu_cluster_properties[] = {
>> + DEFINE_PROP_UINT32("cluster-id", CPUClusterState, cluster_id, 0),
>> + DEFINE_PROP_END_OF_LIST()
>> +};
> [...]
>
[Qemu-arm] [PATCH v7 03/16] gdbstub: add multiprocess support to '?' packets, Luc Michel, 2018/11/23
[Qemu-arm] [PATCH v7 05/16] gdbstub: add multiprocess support to vCont packets, Luc Michel, 2018/11/23
[Qemu-arm] [PATCH v7 12/16] gdbstub: add support for vAttach packets, Luc Michel, 2018/11/23
[Qemu-arm] [PATCH v7 06/16] gdbstub: add multiprocess support to 'sC' packets, Luc Michel, 2018/11/23
[Qemu-arm] [PATCH v7 04/16] gdbstub: add multiprocess support to 'H' and 'T' packets, Luc Michel, 2018/11/23