[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: |
Eduardo Habkost |
Subject: |
Re: [Qemu-arm] [Qemu-devel] [PATCH v7 01/16] hw/cpu: introduce CPU clusters |
Date: |
Fri, 23 Nov 2018 16:10:59 -0200 |
User-agent: |
Mutt/1.9.2 (2017-12-15) |
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.
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.
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()
> +};
[...]
--
Eduardo
- [Qemu-arm] [PATCH v7 00/16] gdbstub: support for the multiprocess extension, Luc Michel, 2018/11/23
- Re: [Qemu-arm] [Qemu-devel] [PATCH v7 01/16] hw/cpu: introduce CPU clusters, Philippe Mathieu-Daudé, 2018/11/25
- Re: [Qemu-arm] [Qemu-devel] [PATCH v7 01/16] hw/cpu: introduce CPU clusters, Eduardo Habkost, 2018/11/26
- Re: [Qemu-arm] [Qemu-devel] [PATCH v7 01/16] hw/cpu: introduce CPU clusters, Peter Maydell, 2018/11/30
- Re: [Qemu-arm] [Qemu-devel] [PATCH v7 01/16] hw/cpu: introduce CPU clusters, Eduardo Habkost, 2018/11/30
[Qemu-arm] [PATCH v7 03/16] gdbstub: add multiprocess support to '?' packets, Luc Michel, 2018/11/23