[{"id":1764485,"web_url":"http://patchwork.ozlabs.org/comment/1764485/","msgid":"","list_archive_url":null,"date":"2017-09-07T03:15:39","subject":"Re: [Qemu-devel] [PATCH v2 13/19] target/s390x: use \"core-id\" for\n\tcpu number/address/id handling","submitter":{"id":26065,"url":"http://patchwork.ozlabs.org/api/people/26065/","name":"Matthew Rosato","email":"mjrosato@linux.vnet.ibm.com"},"content":"On 09/04/2017 11:43 AM, David Hildenbrand wrote:\n> Some time ago we discussed that using \"id\" as property name is not the\n> right thing to do, as it is a reserved property for other devices and\n> will not work with device_add.\n> \n> Switch to the term \"core-id\" instead, and use it as an equivalent to\n> \"CPU address\" mentioned in the PoP. There is no such thing as cpu number,\n> so rename env.cpu_num to env.core_id. We use \"core-id\" as this is the\n> common term to use for device_add later on (x86 and ppc).\n> \n> We can get rid of cpu->id now. Keep cpu_index and env->core_id in sync.\n> cpu_index was already implicitly used by e.g. cpu_exists(), so keeping\n> both in sync seems to be the right thing to do.\n> \n\nRename & removal of 'id' is fine w/ me - in retrospect, it's a shame we\nused 'id' in the first place.\n\n> cpu_index will now no longer automatically get set via\n> cpu_exec_realizefn(). For now, we were lucky that both implicitly stayed\n> in sync.>\n\nGood catch.\n\n> Our new cpu property \"core-id\" can be a static property. Range checks can\n> be avoided by using the correct type and the \"setting after realized\"\n> check is done implicitly.\n> \n> device_add will later need the reserved \"id\" property. Hotplugging a CPU\n> on s390x will then be: \"device_add host-s390-cpu,id=cpu2,core-id=2\".\n> \n> Signed-off-by: David Hildenbrand \n> ---\n\n[...]\n\n> diff --git a/target/s390x/translate.c b/target/s390x/translate.c\n> index 4b0db7b7bd..b8963f2fe2 100644\n> --- a/target/s390x/translate.c\n> +++ b/target/s390x/translate.c\n> @@ -3822,10 +3822,7 @@ static ExitStatus op_ssm(DisasContext *s, DisasOps *o)\n> static ExitStatus op_stap(DisasContext *s, DisasOps *o)\n> {\n> check_privileged(s);\n> - /* ??? Surely cpu address != cpu number. In any case the previous\n> - version of this stored more than the required half-word, so it\n> - is unlikely this has ever been tested. */\n> - tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, cpu_num));\n> + tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, core_id));\n> return NO_EXIT;\n> }\n> \n\nAre you sure it's OK to remove this blurb in its entirety? You are\ncertainly collapsing the various CPU identifiers, but you aren't\nchanging the size of the store from when this blurb was put in\n(411fea3d) So, \"the previous version of this stored more than the\nrequired half-word\" seems to be still relevant -- Unless you've gone\nahead and tested it out?\n\nOutside of that nit, I like the changes.\n\nReviewed-by: Matthew Rosato ","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xnlwM3wBTz9s7c\n\tfor ;\n\tThu, 7 Sep 2017 13:16:16 +1000 (AEST)","from localhost ([::1]:38661 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpnIc-000639-3P\n\tfor incoming@patchwork.ozlabs.org; Wed, 06 Sep 2017 23:16:14 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:51633)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpnIC-00062r-VU\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 23:15:50 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpnI8-0004Zu-Ti\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 23:15:48 -0400","from mx0b-001b2d01.pphosted.com ([148.163.158.5]:55495\n\thelo=mx0a-001b2d01.pphosted.com)\n\tby eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from )\n\tid 1dpnI8-0004Zg-Nq\n\tfor qemu-devel@nongnu.org; Wed, 06 Sep 2017 23:15:44 -0400","from pps.filterd (m0098417.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv873DfX3095086\n\tfor ; Wed, 6 Sep 2017 23:15:44 -0400","from e13.ny.us.ibm.com (e13.ny.us.ibm.com [129.33.205.203])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2ctq57tncj-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Wed, 06 Sep 2017 23:15:43 -0400","from localhost\n\tby e13.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ;\n\tWed, 6 Sep 2017 23:15:43 -0400","from b01cxnp23033.gho.pok.ibm.com (9.57.198.28)\n\tby e13.ny.us.ibm.com (146.89.104.200) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tWed, 6 Sep 2017 23:15:41 -0400","from b01ledav001.gho.pok.ibm.com (b01ledav001.gho.pok.ibm.com\n\t[9.57.199.106])\n\tby b01cxnp23033.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP\n\tid v873Feqo60293176; Thu, 7 Sep 2017 03:15:40 GMT","from b01ledav001.gho.pok.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id E01932803A;\n\tWed, 6 Sep 2017 23:15:33 -0400 (EDT)","from [9.85.195.170] (unknown [9.85.195.170])\n\tby b01ledav001.gho.pok.ibm.com (Postfix) with ESMTP id 7244B2803F;\n\tWed, 6 Sep 2017 23:15:33 -0400 (EDT)"],"To":"David Hildenbrand , qemu-devel@nongnu.org","References":"<20170904154316.4148-1-david@redhat.com>\n\t<20170904154316.4148-14-david@redhat.com>","From":"Matthew Rosato ","Date":"Wed, 6 Sep 2017 23:15:39 -0400","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170904154316.4148-14-david@redhat.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","X-TM-AS-GCONF":"00","x-cbid":"17090703-0008-0000-0000-0000027B781D","X-IBM-SpamModules-Scores":"","X-IBM-SpamModules-Versions":"BY=3.00007680; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000226; SDB=6.00913362; UDB=6.00458407;\n\tIPR=6.00693625; \n\tBA=6.00005574; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009;\n\tZB=6.00000000; \n\tZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017043;\n\tXFM=3.00000015; UTC=2017-09-07 03:15:43","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17090703-0009-0000-0000-0000369D9B4C","Message-Id":"","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-09-07_03:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tspamscore=0 suspectscore=0\n\tmalwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam\n\tadjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000\n\tdefinitions=main-1709070046","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 3.x [generic] [fuzzy]","X-Received-From":"148.163.158.5","Subject":"Re: [Qemu-devel] [PATCH v2 13/19] target/s390x: use \"core-id\" for\n\tcpu number/address/id handling","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"thuth@redhat.com, Eduardo Habkost ,\n\tcohuck@redhat.com, Richard Henderson ,\n\tAlexander Graf , borntraeger@de.ibm.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}},{"id":1764709,"web_url":"http://patchwork.ozlabs.org/comment/1764709/","msgid":"<47e0ae34-a012-366f-334c-9a2050eaf496@redhat.com>","list_archive_url":null,"date":"2017-09-07T13:02:26","subject":"Re: [Qemu-devel] [PATCH v2 13/19] target/s390x: use \"core-id\" for\n\tcpu number/address/id handling","submitter":{"id":70402,"url":"http://patchwork.ozlabs.org/api/people/70402/","name":"David Hildenbrand","email":"david@redhat.com"},"content":">> diff --git a/target/s390x/translate.c b/target/s390x/translate.c\n>> index 4b0db7b7bd..b8963f2fe2 100644\n>> --- a/target/s390x/translate.c\n>> +++ b/target/s390x/translate.c\n>> @@ -3822,10 +3822,7 @@ static ExitStatus op_ssm(DisasContext *s, DisasOps *o)\n>> static ExitStatus op_stap(DisasContext *s, DisasOps *o)\n>> {\n>> check_privileged(s);\n>> - /* ??? Surely cpu address != cpu number. In any case the previous\n>> - version of this stored more than the required half-word, so it\n>> - is unlikely this has ever been tested. */\n>> - tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, cpu_num));\n>> + tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, core_id));\n>> return NO_EXIT;\n>> }\n>>\n> \n> Are you sure it's OK to remove this blurb in its entirety? You are\n> certainly collapsing the various CPU identifiers, but you aren't\n> changing the size of the store from when this blurb was put in\n> (411fea3d) So, \"the previous version of this stored more than the\n> required half-word\" seems to be still relevant -- Unless you've gone\n> ahead and tested it out?\n\nz13 PoP (10-132):\n\n\"The CPU address by which this CPU is identified in a\nmultiprocessing configuration is stored at the half-\nword location designated by the second-operand\naddress.\"\n\nAs far as I understand this comment, 411fea3d fixed the store to only be\nthe required half-word, no?\n\nThe previous version stored 32bit:\n tcg_gen_qemu_st32(tmp2, tmp, get_mem_index(s));\n\nNow we have:\nC(0xb212, STAP, S, Z, la2, 0, new, m1_16, stap, 0)\n\nwhich, due to m1_16, will use wout_m1_16\n tcg_gen_qemu_st16(o->out, o->addr1, get_mem_index(s));\n\nSo what's left is the confusion about num vs address. But core-id is\ncertainly the CPU number, which is t be stored. There is no such thing\nas CPU number.\n\nSo I think this comment can now safely be dropped.\n\nWe have a kvm-unit-test for STAP, but we only check if anything has been\nstored, not if \"too much\" has been stored. But I am not sure if we want\nsuch checks, as the number of tests will explode. Usually, if it would\nbe broken, other things would then go wrong in our unit tests. I will\nhave a look.\n\n> \n> Outside of that nit, I like the changes.\n> \n> Reviewed-by: Matthew Rosato \n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=nongnu.org\n\t(client-ip=2001:4830:134:3::11; helo=lists.gnu.org;\n\tenvelope-from=qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org;\n\treceiver=)","ext-mx04.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx04.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=david@redhat.com"],"Received":["from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11])\n\t(using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xp0xW3Krfz9s7h\n\tfor ;\n\tThu, 7 Sep 2017 23:03:11 +1000 (AEST)","from localhost ([::1]:40421 helo=lists.gnu.org)\n\tby lists.gnu.org with esmtp (Exim 4.71) (envelope-from\n\t)\n\tid 1dpwSb-0000dn-LV\n\tfor incoming@patchwork.ozlabs.org; Thu, 07 Sep 2017 09:03:09 -0400","from eggs.gnu.org ([2001:4830:134:3::10]:48310)\n\tby lists.gnu.org with esmtp (Exim 4.71)\n\t(envelope-from ) id 1dpwS7-0000dP-Sh\n\tfor qemu-devel@nongnu.org; Thu, 07 Sep 2017 09:02:47 -0400","from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)\n\t(envelope-from ) id 1dpwRy-0006Gx-Dl\n\tfor qemu-devel@nongnu.org; Thu, 07 Sep 2017 09:02:39 -0400","from mx1.redhat.com ([209.132.183.28]:42128)\n\tby eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)\n\t(Exim 4.71) (envelope-from ) id 1dpwRy-0006GJ-4z\n\tfor qemu-devel@nongnu.org; Thu, 07 Sep 2017 09:02:30 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 0357985542;\n\tThu, 7 Sep 2017 13:02:29 +0000 (UTC)","from [10.36.116.130] (ovpn-116-130.ams2.redhat.com [10.36.116.130])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 3C6374F07C;\n\tThu, 7 Sep 2017 13:02:27 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 0357985542","To":"Matthew Rosato , qemu-devel@nongnu.org","References":"<20170904154316.4148-1-david@redhat.com>\n\t<20170904154316.4148-14-david@redhat.com>\n\t","From":"David Hildenbrand ","Organization":"Red Hat GmbH","Message-ID":"<47e0ae34-a012-366f-334c-9a2050eaf496@redhat.com>","Date":"Thu, 7 Sep 2017 15:02:26 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.28]);\n\tThu, 07 Sep 2017 13:02:29 +0000 (UTC)","X-detected-operating-system":"by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic]\n\t[fuzzy]","X-Received-From":"209.132.183.28","Subject":"Re: [Qemu-devel] [PATCH v2 13/19] target/s390x: use \"core-id\" for\n\tcpu number/address/id handling","X-BeenThere":"qemu-devel@nongnu.org","X-Mailman-Version":"2.1.21","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"thuth@redhat.com, Eduardo Habkost ,\n\tcohuck@redhat.com, Richard Henderson ,\n\tAlexander Graf , borntraeger@de.ibm.com","Errors-To":"qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org","Sender":"\"Qemu-devel\"\n\t"}}]