From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48310) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dpwS7-0000dP-Sh for qemu-devel@nongnu.org; Thu, 07 Sep 2017 09:02:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dpwRy-0006Gx-Dl for qemu-devel@nongnu.org; Thu, 07 Sep 2017 09:02:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42128) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dpwRy-0006GJ-4z for qemu-devel@nongnu.org; Thu, 07 Sep 2017 09:02:30 -0400 References: <20170904154316.4148-1-david@redhat.com> <20170904154316.4148-14-david@redhat.com> From: David Hildenbrand Message-ID: <47e0ae34-a012-366f-334c-9a2050eaf496@redhat.com> Date: Thu, 7 Sep 2017 15:02:26 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 13/19] target/s390x: use "core-id" for cpu number/address/id handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Matthew Rosato , qemu-devel@nongnu.org Cc: thuth@redhat.com, Eduardo Habkost , cohuck@redhat.com, Richard Henderson , Alexander Graf , borntraeger@de.ibm.com >> diff --git a/target/s390x/translate.c b/target/s390x/translate.c >> index 4b0db7b7bd..b8963f2fe2 100644 >> --- a/target/s390x/translate.c >> +++ b/target/s390x/translate.c >> @@ -3822,10 +3822,7 @@ static ExitStatus op_ssm(DisasContext *s, DisasOps *o) >> static ExitStatus op_stap(DisasContext *s, DisasOps *o) >> { >> check_privileged(s); >> - /* ??? Surely cpu address != cpu number. In any case the previous >> - version of this stored more than the required half-word, so it >> - is unlikely this has ever been tested. */ >> - tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, cpu_num)); >> + tcg_gen_ld32u_i64(o->out, cpu_env, offsetof(CPUS390XState, core_id)); >> return NO_EXIT; >> } >> > > Are you sure it's OK to remove this blurb in its entirety? You are > certainly collapsing the various CPU identifiers, but you aren't > changing the size of the store from when this blurb was put in > (411fea3d) So, "the previous version of this stored more than the > required half-word" seems to be still relevant -- Unless you've gone > ahead and tested it out? z13 PoP (10-132): "The CPU address by which this CPU is identified in a multiprocessing configuration is stored at the half- word location designated by the second-operand address." As far as I understand this comment, 411fea3d fixed the store to only be the required half-word, no? The previous version stored 32bit: tcg_gen_qemu_st32(tmp2, tmp, get_mem_index(s)); Now we have: C(0xb212, STAP, S, Z, la2, 0, new, m1_16, stap, 0) which, due to m1_16, will use wout_m1_16 tcg_gen_qemu_st16(o->out, o->addr1, get_mem_index(s)); So what's left is the confusion about num vs address. But core-id is certainly the CPU number, which is t be stored. There is no such thing as CPU number. So I think this comment can now safely be dropped. We have a kvm-unit-test for STAP, but we only check if anything has been stored, not if "too much" has been stored. But I am not sure if we want such checks, as the number of tests will explode. Usually, if it would be broken, other things would then go wrong in our unit tests. I will have a look. > > Outside of that nit, I like the changes. > > Reviewed-by: Matthew Rosato > -- Thanks, David