>> 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 <mjros...@linux.vnet.ibm.com>
>
--
Thanks,
David
