From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id CAFEAE7F144 for ; Tue, 26 Sep 2023 23:58:34 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qlHvv-0007lP-Fi; Tue, 26 Sep 2023 19:57:39 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qlHvu-0007kx-CK for qemu-devel@nongnu.org; Tue, 26 Sep 2023 19:57:38 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qlHvs-0006ci-Dj for qemu-devel@nongnu.org; Tue, 26 Sep 2023 19:57:38 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1695772654; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=YojiN4iQj8J/kXQ6FhBhpSyy/ep2bnLMTpRtktnAGj8=; b=aVzQxVhacgd5MndYv8oza6oktCjEi+i7Z82mGtQiYe6JYFtHRUDx+lAOIwdgEYCVtWb/Je EDAIoCTUXXPKNFxJIPANO8r/fNfEozv0xJ46d7hmkTgfNiGIJH19dLoDb4lLmmEQyM+5yC eUqb7jW+rOPbYGU66Y35UZ5MShb4eEk= Received: from mail-pg1-f198.google.com (mail-pg1-f198.google.com [209.85.215.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-696-AmGXq_kJO8q1un191YIllQ-1; Tue, 26 Sep 2023 19:57:33 -0400 X-MC-Unique: AmGXq_kJO8q1un191YIllQ-1 Received: by mail-pg1-f198.google.com with SMTP id 41be03b00d2f7-570096f51acso11079189a12.0 for ; Tue, 26 Sep 2023 16:57:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695772652; x=1696377452; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=YojiN4iQj8J/kXQ6FhBhpSyy/ep2bnLMTpRtktnAGj8=; b=LCaRtPZy+feGNRLmxcHYemn6wXybqOYBhYh+ut/rXz5DezoLaOIwUi3MJQBgkKS72w QNx8yiY7XEp6Rg3T+YkEpFIMQbGHz9OBPeAiYLYFOxPJSb2jP/c4Wr9ZpgqvwUDHwW/f YkJJTPyjjfMUY3I9T2844LBFHct27oylOovb7V0t1ZYgV/zM4ffnkJOM2MRuyYSXm44J 1M1yIkdIC4we28afTLa1w89+p+VaNojODmy4bJ8sg1SCL2IpAn95N2fPHPEKKXa7xPEg mfKYyZxibFjsGXXZoentEx1jxOtvz9CCOSrvPYKG14JG6YI852QOa1Dliu/X00SW8pmd M0lA== X-Gm-Message-State: AOJu0YzN7E2s5wys/h42eWkaAGySxQUP+vtLWA4Xn28HN1YK9361RBBk MOpOkb8B8cKdeIoIhNXMH7ZC2gPYUKbFbs6hmcAPbdYIz99tKg6fzue4E21+yi30NUFiAT1ckMJ LggaGN0iFYJFyjLo= X-Received: by 2002:a05:6a20:2443:b0:15d:b407:b0a0 with SMTP id t3-20020a056a20244300b0015db407b0a0mr617103pzc.26.1695772651867; Tue, 26 Sep 2023 16:57:31 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFzygL6Q7QUBdl6x1BRNY2f2EFQ6bbegy08gd5QK22aH4LbQnDw/+6bqDomQKb/Her/+Qby1w== X-Received: by 2002:a05:6a20:2443:b0:15d:b407:b0a0 with SMTP id t3-20020a056a20244300b0015db407b0a0mr617046pzc.26.1695772651374; Tue, 26 Sep 2023 16:57:31 -0700 (PDT) Received: from ?IPV6:2001:8003:e5b0:9f00:dbbc:1945:6e65:ec5? ([2001:8003:e5b0:9f00:dbbc:1945:6e65:ec5]) by smtp.gmail.com with ESMTPSA id fm1-20020a056a002f8100b00679a4b56e41sm10580839pfb.43.2023.09.26.16.57.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 26 Sep 2023 16:57:30 -0700 (PDT) Message-ID: <6cd28639-2cfa-f233-c6d9-d5d2ec5b1c58@redhat.com> Date: Wed, 27 Sep 2023 09:57:18 +1000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: [PATCH RFC V2 01/37] arm/virt,target/arm: Add new ARMCPU {socket,cluster,core,thread}-id property Content-Language: en-US To: Salil Mehta , qemu-devel@nongnu.org, qemu-arm@nongnu.org Cc: maz@kernel.org, jean-philippe@linaro.org, jonathan.cameron@huawei.com, lpieralisi@kernel.org, peter.maydell@linaro.org, richard.henderson@linaro.org, imammedo@redhat.com, andrew.jones@linux.dev, david@redhat.com, philmd@linaro.org, eric.auger@redhat.com, will@kernel.org, ardb@kernel.org, oliver.upton@linux.dev, pbonzini@redhat.com, mst@redhat.com, rafael@kernel.org, borntraeger@linux.ibm.com, alex.bennee@linaro.org, linux@armlinux.org.uk, darren@os.amperecomputing.com, ilkka@os.amperecomputing.com, vishnu@os.amperecomputing.com, karl.heubaum@oracle.com, miguel.luis@oracle.com, salil.mehta@opnsrc.net, zhukeqian1@huawei.com, wangxiongfeng2@huawei.com, wangyanan55@huawei.com, jiakernel2@gmail.com, maobibo@loongson.cn, lixianglai@loongson.cn References: <20230926100436.28284-1-salil.mehta@huawei.com> <20230926100436.28284-2-salil.mehta@huawei.com> From: Gavin Shan In-Reply-To: <20230926100436.28284-2-salil.mehta@huawei.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Received-SPF: pass client-ip=170.10.133.124; envelope-from=gshan@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -35 X-Spam_score: -3.6 X-Spam_bar: --- X-Spam_report: (-3.6 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, NICE_REPLY_A=-1.473, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Hi Salil, On 9/26/23 20:04, Salil Mehta wrote: > This shall be used to store user specified topology{socket,cluster,core,thread} > and shall be converted to a unique 'vcpu-id' which is used as slot-index during > hot(un)plug of vCPU. > Note that we don't have 'vcpu-id' property. It's actually the index to the array ms->possible_cpus->cpus[] and cpu->cpu_index. Please improve the commit log if it makes sense. > Co-developed-by: Salil Mehta > Signed-off-by: Salil Mehta > Co-developed-by: Keqian Zhu > Signed-off-by: Keqian Zhu > Signed-off-by: Salil Mehta > --- > hw/arm/virt.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++ > target/arm/cpu.c | 4 +++ > target/arm/cpu.h | 4 +++ > 3 files changed, 71 insertions(+) > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index 7d9dbc2663..57fe97c242 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -221,6 +221,11 @@ static const char *valid_cpus[] = { > ARM_CPU_TYPE_NAME("max"), > }; > > +static int virt_get_socket_id(const MachineState *ms, int cpu_index); > +static int virt_get_cluster_id(const MachineState *ms, int cpu_index); > +static int virt_get_core_id(const MachineState *ms, int cpu_index); > +static int virt_get_thread_id(const MachineState *ms, int cpu_index); > + > static bool cpu_type_valid(const char *cpu) > { > int i; > @@ -2168,6 +2173,14 @@ static void machvirt_init(MachineState *machine) > &error_fatal); > > aarch64 &= object_property_get_bool(cpuobj, "aarch64", NULL); > + object_property_set_int(cpuobj, "socket-id", > + virt_get_socket_id(machine, n), NULL); > + object_property_set_int(cpuobj, "cluster-id", > + virt_get_cluster_id(machine, n), NULL); > + object_property_set_int(cpuobj, "core-id", > + virt_get_core_id(machine, n), NULL); > + object_property_set_int(cpuobj, "thread-id", > + virt_get_thread_id(machine, n), NULL); > > if (!vms->secure) { > object_property_set_bool(cpuobj, "has_el3", false, NULL); > @@ -2652,10 +2665,59 @@ static int64_t virt_get_default_cpu_node_id(const MachineState *ms, int idx) > return socket_id % ms->numa_state->num_nodes; > } > It seems it's not unnecessary to keep virt_get_{socket, cluster, core, thread}_id() because they're called for once. I would suggest to figure out the socket, cluster, core and thread ID through @possible_cpus in machvirt_init(), like below. Besides, we can't always expose property "cluster-id" since cluster in the CPU topology isn't always supported, seeing MachineClass::smp_props. Some users may want to hide cluster for unknown reasons. 'cluster-id' shouldn't be exposed in this case. Otherwise, users may be confused by 'cluster-id' property while it has been disabled. For example, a VM is started with the following command lines and 'cluster-id' shouldn't be supported in vCPU hot-add. -cpu host -smp=maxcpus=2,cpus=1,sockets=2,cores=1,threads=1 (qemu) device_add host,id=cpu1,socket-id=1,cluster-id=0,core-id=0,thread-id=0 object_property_set_int(cpuobj, "socket-id", possible_cpus->cpus[i].props.socket_id, NULL); if (mc->smp_props.cluster_supported && mc->smp_props.has_clusters) { object_property_set_int(cpuobj, "cluster-id", possible_cpus->cpus[i].props.cluster_id, NULL); } object_property_set_int(cpuobj, "core-id", possible_cpus->cpus[i].props.core_id, NULL); object_property_set_int(cpuobj, "thread-id", possible_cpus->cpus[i].props.thread_id, NULL); > +static int virt_get_socket_id(const MachineState *ms, int cpu_index) > +{ > + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); > + > + return ms->possible_cpus->cpus[cpu_index].props.socket_id; > +} > + > +static int virt_get_cluster_id(const MachineState *ms, int cpu_index) > +{ > + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); > + > + return ms->possible_cpus->cpus[cpu_index].props.cluster_id; > +} > + > +static int virt_get_core_id(const MachineState *ms, int cpu_index) > +{ > + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); > + > + return ms->possible_cpus->cpus[cpu_index].props.core_id; > +} > + > +static int virt_get_thread_id(const MachineState *ms, int cpu_index) > +{ > + assert(cpu_index >= 0 && cpu_index < ms->possible_cpus->len); > + > + return ms->possible_cpus->cpus[cpu_index].props.thread_id; > +} > + > +static int > +virt_get_cpu_id_from_cpu_topo(const MachineState *ms, DeviceState *dev) > +{ > + int cpu_id, sock_vcpu_num, clus_vcpu_num, core_vcpu_num; > + ARMCPU *cpu = ARM_CPU(dev); > + > + /* calculate total logical cpus across socket/cluster/core */ > + sock_vcpu_num = cpu->socket_id * (ms->smp.threads * ms->smp.cores * > + ms->smp.clusters); > + clus_vcpu_num = cpu->cluster_id * (ms->smp.threads * ms->smp.cores); > + core_vcpu_num = cpu->core_id * ms->smp.threads; > + > + /* get vcpu-id(logical cpu index) for this vcpu from this topology */ > + cpu_id = (sock_vcpu_num + clus_vcpu_num + core_vcpu_num) + cpu->thread_id; > + > + assert(cpu_id >= 0 && cpu_id < ms->possible_cpus->len); > + > + return cpu_id; > +} > + This function is called for once in PATCH[04/37]. I think it needs to be moved around to PATCH[04/37]. [PATCH RFC V2 04/37] arm/virt,target/arm: Machine init time change common to vCPU {cold|hot}-plug The function name can be shortened because I don't see the suffix "_from_cpu_topo" is too much helpful. I think virt_get_cpu_index() would be good enough since it's called for once to return the index in array MachineState::possible_cpus::cpus[] and the return value is stored to CPUState::cpu_index. static int virt_get_cpu_index(const MachineState *ms, ARMCPU *cpu) { int index, cpus_in_socket, cpus_in_cluster, cpus_in_core; /* * It's fine to take cluster into account even it's not supported. In this * case, ms->smp.clusters is always one. */ } > static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms) > { > int n; > unsigned int max_cpus = ms->smp.max_cpus; > + unsigned int smp_threads = ms->smp.threads; > VirtMachineState *vms = VIRT_MACHINE(ms); > MachineClass *mc = MACHINE_GET_CLASS(vms); > > @@ -2669,6 +2731,7 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms) > ms->possible_cpus->len = max_cpus; > for (n = 0; n < ms->possible_cpus->len; n++) { > ms->possible_cpus->cpus[n].type = ms->cpu_type; > + ms->possible_cpus->cpus[n].vcpus_count = smp_threads; > ms->possible_cpus->cpus[n].arch_id = > virt_cpu_mp_affinity(vms, n); > This initialization seems to accomodate HMP command "info hotpluggable-cpus". It would be nice if it can be mentioned in the commit log. > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 93c28d50e5..1376350416 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -2277,6 +2277,10 @@ static Property arm_cpu_properties[] = { > DEFINE_PROP_UINT64("mp-affinity", ARMCPU, > mp_affinity, ARM64_AFFINITY_INVALID), > DEFINE_PROP_INT32("node-id", ARMCPU, node_id, CPU_UNSET_NUMA_NODE_ID), > + DEFINE_PROP_INT32("socket-id", ARMCPU, socket_id, 0), > + DEFINE_PROP_INT32("cluster-id", ARMCPU, cluster_id, 0), > + DEFINE_PROP_INT32("core-id", ARMCPU, core_id, 0), > + DEFINE_PROP_INT32("thread-id", ARMCPU, thread_id, 0), > DEFINE_PROP_INT32("core-count", ARMCPU, core_count, -1), > DEFINE_PROP_END_OF_LIST() > }; All those 4 properties are used for vCPU hot-add, meaning they're not needed when vCPU hotplug isn't supported on the specific board. Even for hw/virt board, cluster isn't always supported and 'cluster-id' shouldn't always be exposed, as explained above. How about to register the properties dynamically only when they're needed by vCPU hotplug? > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index 88e5accda6..d51d39f621 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -1094,6 +1094,10 @@ struct ArchCPU { > QLIST_HEAD(, ARMELChangeHook) el_change_hooks; > > int32_t node_id; /* NUMA node this CPU belongs to */ > + int32_t socket_id; > + int32_t cluster_id; > + int32_t core_id; > + int32_t thread_id; It would be fine to keep those fields even the corresponding properties are dynamically registered, but a little bit memory overhead incurred :) > > /* Used to synchronize KVM and QEMU in-kernel device levels */ > uint8_t device_irq_level; Thanks, Gavin