| Message ID | 20170228131710.10593-1-mbenes@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 28.02.2017 14:17, Miroslav Benes wrote: > Implement MVCOS instruction, which the Linux kernel uses in user access > functions. > > Signed-off-by: Miroslav Benes <mbenes@suse.cz> > --- > I tried to do my best to follow the specification but it is quite > possible that I got something wrong because of my lack of > understanding. Especially I am not sure about all those bit ops :/. > > Anyway, there is one piece missing. The actual use of keys and > address-space-control during the move. I used fast_memmove, but > it is not correct. Is there a helper which I could use? I looked at > other instructions which should implement access control, but there were > silently ignore it :). I'm not aware of a function that could deal with two address spaces already (but that does not mean that there is no such function already) ... still, I guess, you likely need to write your own memmove helper function that can deal with two different address spaces. > target/s390x/helper.h | 1 + > target/s390x/insn-data.def | 2 ++ > target/s390x/mem_helper.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++ > target/s390x/translate.c | 12 +++++++ > 4 files changed, 95 insertions(+) > > diff --git a/target/s390x/helper.h b/target/s390x/helper.h > index 9102071d0aa4..bc5dfccc3d7e 100644 > --- a/target/s390x/helper.h > +++ b/target/s390x/helper.h > @@ -104,6 +104,7 @@ DEF_HELPER_FLAGS_2(iske, TCG_CALL_NO_RWG_SE, i64, env, i64) > DEF_HELPER_FLAGS_3(sske, TCG_CALL_NO_RWG, void, env, i64, i64) > DEF_HELPER_FLAGS_2(rrbe, TCG_CALL_NO_RWG, i32, env, i64) > DEF_HELPER_3(csp, i32, env, i32, i64) > +DEF_HELPER_5(mvcos, i32, env, i64, i64, i64, i64) > DEF_HELPER_4(mvcs, i32, env, i64, i64, i64) > DEF_HELPER_4(mvcp, i32, env, i64, i64, i64) > DEF_HELPER_4(sigp, i32, env, i64, i32, i64) > diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def > index 075ff597c3de..a1e6d735d090 100644 > --- a/target/s390x/insn-data.def > +++ b/target/s390x/insn-data.def > @@ -854,6 +854,8 @@ > /* LOAD USING REAL ADDRESS */ > C(0xb24b, LURA, RRE, Z, 0, r2, new, r1_32, lura, 0) > C(0xb905, LURAG, RRE, Z, 0, r2, r1, 0, lurag, 0) > +/* MOVE WITH OPTIONAL SPECIFICATION */ > + C(0xc800, MVCOS, SSF, MVCOS, la1, a2, 0, 0, mvcos, 0) > /* MOVE TO PRIMARY */ > C(0xda00, MVCP, SS_d, Z, la1, a2, 0, 0, mvcp, 0) > /* MOVE TO SECONDARY */ > diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c > index 675aba2e44d4..ca8f7c49250c 100644 > --- a/target/s390x/mem_helper.c > +++ b/target/s390x/mem_helper.c > @@ -1089,6 +1089,86 @@ uint32_t HELPER(mvcp)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2) > return cc; > } > > +uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t r0, uint64_t dest, > + uint64_t src, uint64_t len) > +{ > + int cc; > + int key1, as1, abit1, kbit1; > + int key2, as2, abit2, kbit2; > + > + HELPER_LOG("%s dest %" PRIx64 ", src %" PRIx64 ", len %" PRIx64 "\n", > + __func__, dest, src, len); > + > + /* check DAT */ > + if (!(env->psw.mask & PSW_MASK_DAT)) { > + program_interrupt(env, PGM_SPECIAL_OP, 2); Length of the opcode is 6 bytes, not 2. > + } > + > + /* access control for the first operand */ > + abit1 = (r0 & 0x0010000ULL) >> 16; > + kbit1 = (r0 & 0x0020000ULL) >> 17; > + as1 = (r0 & 0x00c00000ULL) >> 22; > + key1 = (r0 & 0xf0000000ULL) >> 28; > + > + if (!kbit1) { > + key1 = (env->psw.mask & PSW_MASK_KEY) >> (PSW_SHIFT_KEY - 4); I wonder whether it would make sense to define PSW_SHIFT_KEY directly to 52 instead ... I can't see the reason for 56 here. The only other spot that uses it also subtracts 4. > + } > + > + if (!abit1) { > + as1 = (env->psw.mask & PSW_MASK_ASC) >> 46; > + } > + > + /* > + * abit1 is set, as1 designates the home-space mode, psw is in the problem > + * state. > + * */ Cosmetic nit: Make the closing comment just "*/" instead of "* */". > + if (abit1 && (as1 == 3) && (env->psw.mask & PSW_MASK_PSTATE)) { > + program_interrupt(env, PGM_SPECIAL_OP, 2); set instruction length = 6 again > + } > + > + /* access control for the second operand */ > + abit2 = (r0 & 0x0010000ULL); > + kbit2 = (r0 & 0x0020000ULL) >> 1; > + as2 = (r0 & 0x00c00000ULL) >> 6; > + key2 = (r0 & 0xf0000000ULL) >> 12; The above four lines look wrong ... are you sure that you've got the masks right? If I read the POP correctly, the values should be in the lowest two bytes instead? > + if (!kbit2) { > + key2 = (env->psw.mask & PSW_MASK_KEY) >> (PSW_SHIFT_KEY - 4); > + } > + > + if (!abit2) { > + as2 = (env->psw.mask & PSW_MASK_ASC) >> 46; > + } > + > + /* > + * Secondary-space control bit is zero (bit 37 of r0) and either as > + * designates secondary-space mode. > + */ > + if (!(r0 & 0x2000000000ULL) && (as1 == 2 || as2 == 2)) { You mixed up "control register 0" with "general purpose register 0" here, i.e. you must not use r0 but rather env->cregs[0] here. > + program_interrupt(env, PGM_SPECIAL_OP, 2); s/2/6/ > + } > + > + /* psw is in the problem state and either key is invalid */ > + if ((env->psw.mask & PSW_MASK_PSTATE) && > + (!(env->cregs[3] & (1 << (31 - key1))) || > + !(env->cregs[3] & (1 << (31 - key2))))) { > + program_interrupt(env, PGM_PRIVILEGED, 2); s/2/6/ > + } > + > + if (len <= 4096) { > + cc = 0; > + } else { > + cc = 3; > + len = 4096; > + } > + > + /* move */ > + /* XXX use keys and as during the move */ > + fast_memmove(env, dest, src, len); > + > + return cc; > +} > + > /* invalidate pte */ > void HELPER(ipte)(CPUS390XState *env, uint64_t pte_addr, uint64_t vaddr) > { > diff --git a/target/s390x/translate.c b/target/s390x/translate.c > index 01c62176bf70..ac90b758d312 100644 > --- a/target/s390x/translate.c > +++ b/target/s390x/translate.c > @@ -1194,6 +1194,7 @@ typedef enum DisasFacility { > FAC_SCF, /* store clock fast */ > FAC_SFLE, /* store facility list extended */ > FAC_ILA, /* interlocked access facility 1 */ > + FAC_MVCOS, /* move-with-optional-specification */ > } DisasFacility; > > struct DisasInsn { > @@ -2877,6 +2878,17 @@ static ExitStatus op_mvcs(DisasContext *s, DisasOps *o) > set_cc_static(s); > return NO_EXIT; > } > + > +static ExitStatus op_mvcos(DisasContext *s, DisasOps *o) > +{ > + int r3 = get_field(s->fields, r3); > + > + check_privileged(s); I think it is possible to use this instruction in PRoblem state, so the general check_privileged() check seems to be wrong here. The instruction only generates a PRIVILEGE exception if the keys are invalid. > + potential_page_fault(s); > + gen_helper_mvcos(cc_op, cpu_env, regs[0], o->addr1, o->in2, regs[r3]); > + set_cc_static(s); > + return NO_EXIT; > +} > #endif > > static ExitStatus op_mvpg(DisasContext *s, DisasOps *o) >
On Wed, 1 Mar 2017, Thomas Huth wrote: > On 28.02.2017 14:17, Miroslav Benes wrote: > > Implement MVCOS instruction, which the Linux kernel uses in user access > > functions. > > > > Signed-off-by: Miroslav Benes <mbenes@suse.cz> > > --- > > I tried to do my best to follow the specification but it is quite > > possible that I got something wrong because of my lack of > > understanding. Especially I am not sure about all those bit ops :/. > > > > Anyway, there is one piece missing. The actual use of keys and > > address-space-control during the move. I used fast_memmove, but > > it is not correct. Is there a helper which I could use? I looked at > > other instructions which should implement access control, but there were > > silently ignore it :). > > I'm not aware of a function that could deal with two address spaces > already (but that does not mean that there is no such function already) > ... still, I guess, you likely need to write your own memmove helper > function that can deal with two different address spaces. Ok, I thought that was the case. I'll try to come up with something. > > target/s390x/helper.h | 1 + > > target/s390x/insn-data.def | 2 ++ > > target/s390x/mem_helper.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++ > > target/s390x/translate.c | 12 +++++++ > > 4 files changed, 95 insertions(+) > > > > diff --git a/target/s390x/helper.h b/target/s390x/helper.h > > index 9102071d0aa4..bc5dfccc3d7e 100644 > > --- a/target/s390x/helper.h > > +++ b/target/s390x/helper.h > > @@ -104,6 +104,7 @@ DEF_HELPER_FLAGS_2(iske, TCG_CALL_NO_RWG_SE, i64, env, i64) > > DEF_HELPER_FLAGS_3(sske, TCG_CALL_NO_RWG, void, env, i64, i64) > > DEF_HELPER_FLAGS_2(rrbe, TCG_CALL_NO_RWG, i32, env, i64) > > DEF_HELPER_3(csp, i32, env, i32, i64) > > +DEF_HELPER_5(mvcos, i32, env, i64, i64, i64, i64) > > DEF_HELPER_4(mvcs, i32, env, i64, i64, i64) > > DEF_HELPER_4(mvcp, i32, env, i64, i64, i64) > > DEF_HELPER_4(sigp, i32, env, i64, i32, i64) > > diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def > > index 075ff597c3de..a1e6d735d090 100644 > > --- a/target/s390x/insn-data.def > > +++ b/target/s390x/insn-data.def > > @@ -854,6 +854,8 @@ > > /* LOAD USING REAL ADDRESS */ > > C(0xb24b, LURA, RRE, Z, 0, r2, new, r1_32, lura, 0) > > C(0xb905, LURAG, RRE, Z, 0, r2, r1, 0, lurag, 0) > > +/* MOVE WITH OPTIONAL SPECIFICATION */ > > + C(0xc800, MVCOS, SSF, MVCOS, la1, a2, 0, 0, mvcos, 0) > > /* MOVE TO PRIMARY */ > > C(0xda00, MVCP, SS_d, Z, la1, a2, 0, 0, mvcp, 0) > > /* MOVE TO SECONDARY */ > > diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c > > index 675aba2e44d4..ca8f7c49250c 100644 > > --- a/target/s390x/mem_helper.c > > +++ b/target/s390x/mem_helper.c > > @@ -1089,6 +1089,86 @@ uint32_t HELPER(mvcp)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2) > > return cc; > > } > > > > +uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t r0, uint64_t dest, > > + uint64_t src, uint64_t len) > > +{ > > + int cc; > > + int key1, as1, abit1, kbit1; > > + int key2, as2, abit2, kbit2; > > + > > + HELPER_LOG("%s dest %" PRIx64 ", src %" PRIx64 ", len %" PRIx64 "\n", > > + __func__, dest, src, len); > > + > > + /* check DAT */ > > + if (!(env->psw.mask & PSW_MASK_DAT)) { > > + program_interrupt(env, PGM_SPECIAL_OP, 2); > > Length of the opcode is 6 bytes, not 2. True. Sorry, I don't know where 2 came from. It does not make sense. > > + } > > + > > + /* access control for the first operand */ > > + abit1 = (r0 & 0x0010000ULL) >> 16; > > + kbit1 = (r0 & 0x0020000ULL) >> 17; > > + as1 = (r0 & 0x00c00000ULL) >> 22; > > + key1 = (r0 & 0xf0000000ULL) >> 28; > > + > > + if (!kbit1) { > > + key1 = (env->psw.mask & PSW_MASK_KEY) >> (PSW_SHIFT_KEY - 4); > > I wonder whether it would make sense to define PSW_SHIFT_KEY directly to > 52 instead ... I can't see the reason for 56 here. The only other spot > that uses it also subtracts 4. Right, I'll make a patch to change that. > > + } > > + > > + if (!abit1) { > > + as1 = (env->psw.mask & PSW_MASK_ASC) >> 46; > > + } > > + > > + /* > > + * abit1 is set, as1 designates the home-space mode, psw is in the problem > > + * state. > > + * */ > > Cosmetic nit: Make the closing comment just "*/" instead of "* */". Oh, sure. > > + if (abit1 && (as1 == 3) && (env->psw.mask & PSW_MASK_PSTATE)) { > > + program_interrupt(env, PGM_SPECIAL_OP, 2); > > set instruction length = 6 again > > > + } > > + > > + /* access control for the second operand */ > > + abit2 = (r0 & 0x0010000ULL); > > + kbit2 = (r0 & 0x0020000ULL) >> 1; > > + as2 = (r0 & 0x00c00000ULL) >> 6; > > + key2 = (r0 & 0xf0000000ULL) >> 12; > > The above four lines look wrong ... are you sure that you've got the > masks right? If I read the POP correctly, the values should be in the > lowest two bytes instead? You're right. Stupid copy-paste error, I only changed shift constants. Sorry for that. > > + if (!kbit2) { > > + key2 = (env->psw.mask & PSW_MASK_KEY) >> (PSW_SHIFT_KEY - 4); > > + } > > + > > + if (!abit2) { > > + as2 = (env->psw.mask & PSW_MASK_ASC) >> 46; > > + } > > + > > + /* > > + * Secondary-space control bit is zero (bit 37 of r0) and either as > > + * designates secondary-space mode. > > + */ > > + if (!(r0 & 0x2000000000ULL) && (as1 == 2 || as2 == 2)) { > > You mixed up "control register 0" with "general purpose register 0" > here, i.e. you must not use r0 but rather env->cregs[0] here. Correct. Will fix it. > > + program_interrupt(env, PGM_SPECIAL_OP, 2); > > s/2/6/ > > > + } > > + > > + /* psw is in the problem state and either key is invalid */ > > + if ((env->psw.mask & PSW_MASK_PSTATE) && > > + (!(env->cregs[3] & (1 << (31 - key1))) || > > + !(env->cregs[3] & (1 << (31 - key2))))) { > > + program_interrupt(env, PGM_PRIVILEGED, 2); > > s/2/6/ > > > + } > > + > > + if (len <= 4096) { > > + cc = 0; > > + } else { > > + cc = 3; > > + len = 4096; > > + } > > + > > + /* move */ > > + /* XXX use keys and as during the move */ > > + fast_memmove(env, dest, src, len); > > + > > + return cc; > > +} > > + > > /* invalidate pte */ > > void HELPER(ipte)(CPUS390XState *env, uint64_t pte_addr, uint64_t vaddr) > > { > > diff --git a/target/s390x/translate.c b/target/s390x/translate.c > > index 01c62176bf70..ac90b758d312 100644 > > --- a/target/s390x/translate.c > > +++ b/target/s390x/translate.c > > @@ -1194,6 +1194,7 @@ typedef enum DisasFacility { > > FAC_SCF, /* store clock fast */ > > FAC_SFLE, /* store facility list extended */ > > FAC_ILA, /* interlocked access facility 1 */ > > + FAC_MVCOS, /* move-with-optional-specification */ > > } DisasFacility; > > > > struct DisasInsn { > > @@ -2877,6 +2878,17 @@ static ExitStatus op_mvcs(DisasContext *s, DisasOps *o) > > set_cc_static(s); > > return NO_EXIT; > > } > > + > > +static ExitStatus op_mvcos(DisasContext *s, DisasOps *o) > > +{ > > + int r3 = get_field(s->fields, r3); > > + > > + check_privileged(s); > > I think it is possible to use this instruction in PRoblem state, so the > general check_privileged() check seems to be wrong here. The instruction > only generates a PRIVILEGE exception if the keys are invalid. Ok. Thanks for the feedback! I'm new to qemu, so everything is more than useful. Miroslav
On 01.03.2017 13:19, Miroslav Benes wrote: > On Wed, 1 Mar 2017, Thomas Huth wrote: > >> On 28.02.2017 14:17, Miroslav Benes wrote: >>> Implement MVCOS instruction, which the Linux kernel uses in user access >>> functions. >>> >>> Signed-off-by: Miroslav Benes <mbenes@suse.cz> >>> --- >>> I tried to do my best to follow the specification but it is quite >>> possible that I got something wrong because of my lack of >>> understanding. Especially I am not sure about all those bit ops :/. >>> >>> Anyway, there is one piece missing. The actual use of keys and >>> address-space-control during the move. I used fast_memmove, but >>> it is not correct. Is there a helper which I could use? I looked at >>> other instructions which should implement access control, but there were >>> silently ignore it :). >> >> I'm not aware of a function that could deal with two address spaces >> already (but that does not mean that there is no such function already) >> ... still, I guess, you likely need to write your own memmove helper >> function that can deal with two different address spaces. > > Ok, I thought that was the case. I'll try to come up with something. > >>> target/s390x/helper.h | 1 + >>> target/s390x/insn-data.def | 2 ++ >>> target/s390x/mem_helper.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++ >>> target/s390x/translate.c | 12 +++++++ >>> 4 files changed, 95 insertions(+) >>> >>> diff --git a/target/s390x/helper.h b/target/s390x/helper.h >>> index 9102071d0aa4..bc5dfccc3d7e 100644 >>> --- a/target/s390x/helper.h >>> +++ b/target/s390x/helper.h >>> @@ -104,6 +104,7 @@ DEF_HELPER_FLAGS_2(iske, TCG_CALL_NO_RWG_SE, i64, env, i64) >>> DEF_HELPER_FLAGS_3(sske, TCG_CALL_NO_RWG, void, env, i64, i64) >>> DEF_HELPER_FLAGS_2(rrbe, TCG_CALL_NO_RWG, i32, env, i64) >>> DEF_HELPER_3(csp, i32, env, i32, i64) >>> +DEF_HELPER_5(mvcos, i32, env, i64, i64, i64, i64) >>> DEF_HELPER_4(mvcs, i32, env, i64, i64, i64) >>> DEF_HELPER_4(mvcp, i32, env, i64, i64, i64) >>> DEF_HELPER_4(sigp, i32, env, i64, i32, i64) >>> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def >>> index 075ff597c3de..a1e6d735d090 100644 >>> --- a/target/s390x/insn-data.def >>> +++ b/target/s390x/insn-data.def >>> @@ -854,6 +854,8 @@ >>> /* LOAD USING REAL ADDRESS */ >>> C(0xb24b, LURA, RRE, Z, 0, r2, new, r1_32, lura, 0) >>> C(0xb905, LURAG, RRE, Z, 0, r2, r1, 0, lurag, 0) >>> +/* MOVE WITH OPTIONAL SPECIFICATION */ >>> + C(0xc800, MVCOS, SSF, MVCOS, la1, a2, 0, 0, mvcos, 0) >>> /* MOVE TO PRIMARY */ >>> C(0xda00, MVCP, SS_d, Z, la1, a2, 0, 0, mvcp, 0) >>> /* MOVE TO SECONDARY */ >>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c >>> index 675aba2e44d4..ca8f7c49250c 100644 >>> --- a/target/s390x/mem_helper.c >>> +++ b/target/s390x/mem_helper.c >>> @@ -1089,6 +1089,86 @@ uint32_t HELPER(mvcp)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2) >>> return cc; >>> } >>> >>> +uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t r0, uint64_t dest, >>> + uint64_t src, uint64_t len) >>> +{ >>> + int cc; >>> + int key1, as1, abit1, kbit1; >>> + int key2, as2, abit2, kbit2; >>> + >>> + HELPER_LOG("%s dest %" PRIx64 ", src %" PRIx64 ", len %" PRIx64 "\n", >>> + __func__, dest, src, len); >>> + >>> + /* check DAT */ >>> + if (!(env->psw.mask & PSW_MASK_DAT)) { >>> + program_interrupt(env, PGM_SPECIAL_OP, 2); >> >> Length of the opcode is 6 bytes, not 2. > > True. Sorry, I don't know where 2 came from. It does not make sense. As I recently had to learn it the hard way (while implementing the TEST BLOCK instruction), you should use ILEN_LATER_INC here instead of 2 (or 6), since the Special operation exception is suppressing, too, i.e. the program counter should be increased afterwards to the next instruction. BTW, are you still working on a new version of this patch? Thomas
On 12.06.2017 18:44, Thomas Huth wrote: > On 01.03.2017 13:19, Miroslav Benes wrote: >> On Wed, 1 Mar 2017, Thomas Huth wrote: >> >>> On 28.02.2017 14:17, Miroslav Benes wrote: >>>> Implement MVCOS instruction, which the Linux kernel uses in user access >>>> functions. >>>> >>>> Signed-off-by: Miroslav Benes <mbenes@suse.cz> >>>> --- >>>> I tried to do my best to follow the specification but it is quite >>>> possible that I got something wrong because of my lack of >>>> understanding. Especially I am not sure about all those bit ops :/. >>>> >>>> Anyway, there is one piece missing. The actual use of keys and >>>> address-space-control during the move. I used fast_memmove, but >>>> it is not correct. Is there a helper which I could use? I looked at >>>> other instructions which should implement access control, but there were >>>> silently ignore it :). >>> >>> I'm not aware of a function that could deal with two address spaces >>> already (but that does not mean that there is no such function already) >>> ... still, I guess, you likely need to write your own memmove helper >>> function that can deal with two different address spaces. >> >> Ok, I thought that was the case. I'll try to come up with something. >> >>>> target/s390x/helper.h | 1 + >>>> target/s390x/insn-data.def | 2 ++ >>>> target/s390x/mem_helper.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++ >>>> target/s390x/translate.c | 12 +++++++ >>>> 4 files changed, 95 insertions(+) >>>> >>>> diff --git a/target/s390x/helper.h b/target/s390x/helper.h >>>> index 9102071d0aa4..bc5dfccc3d7e 100644 >>>> --- a/target/s390x/helper.h >>>> +++ b/target/s390x/helper.h >>>> @@ -104,6 +104,7 @@ DEF_HELPER_FLAGS_2(iske, TCG_CALL_NO_RWG_SE, i64, env, i64) >>>> DEF_HELPER_FLAGS_3(sske, TCG_CALL_NO_RWG, void, env, i64, i64) >>>> DEF_HELPER_FLAGS_2(rrbe, TCG_CALL_NO_RWG, i32, env, i64) >>>> DEF_HELPER_3(csp, i32, env, i32, i64) >>>> +DEF_HELPER_5(mvcos, i32, env, i64, i64, i64, i64) >>>> DEF_HELPER_4(mvcs, i32, env, i64, i64, i64) >>>> DEF_HELPER_4(mvcp, i32, env, i64, i64, i64) >>>> DEF_HELPER_4(sigp, i32, env, i64, i32, i64) >>>> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def >>>> index 075ff597c3de..a1e6d735d090 100644 >>>> --- a/target/s390x/insn-data.def >>>> +++ b/target/s390x/insn-data.def >>>> @@ -854,6 +854,8 @@ >>>> /* LOAD USING REAL ADDRESS */ >>>> C(0xb24b, LURA, RRE, Z, 0, r2, new, r1_32, lura, 0) >>>> C(0xb905, LURAG, RRE, Z, 0, r2, r1, 0, lurag, 0) >>>> +/* MOVE WITH OPTIONAL SPECIFICATION */ >>>> + C(0xc800, MVCOS, SSF, MVCOS, la1, a2, 0, 0, mvcos, 0) >>>> /* MOVE TO PRIMARY */ >>>> C(0xda00, MVCP, SS_d, Z, la1, a2, 0, 0, mvcp, 0) >>>> /* MOVE TO SECONDARY */ >>>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c >>>> index 675aba2e44d4..ca8f7c49250c 100644 >>>> --- a/target/s390x/mem_helper.c >>>> +++ b/target/s390x/mem_helper.c >>>> @@ -1089,6 +1089,86 @@ uint32_t HELPER(mvcp)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2) >>>> return cc; >>>> } >>>> >>>> +uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t r0, uint64_t dest, >>>> + uint64_t src, uint64_t len) >>>> +{ >>>> + int cc; >>>> + int key1, as1, abit1, kbit1; >>>> + int key2, as2, abit2, kbit2; >>>> + >>>> + HELPER_LOG("%s dest %" PRIx64 ", src %" PRIx64 ", len %" PRIx64 "\n", >>>> + __func__, dest, src, len); >>>> + >>>> + /* check DAT */ >>>> + if (!(env->psw.mask & PSW_MASK_DAT)) { >>>> + program_interrupt(env, PGM_SPECIAL_OP, 2); >>> >>> Length of the opcode is 6 bytes, not 2. >> >> True. Sorry, I don't know where 2 came from. It does not make sense. > > As I recently had to learn it the hard way (while implementing the TEST > BLOCK instruction), you should use ILEN_LATER_INC here instead of 2 (or > 6), since the Special operation exception is suppressing, too, i.e. the > program counter should be increased afterwards to the next instruction. > This is no longer needed with https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02631.html You can either use 6 or ILEN_AUTO here, suppression will be handled internally. > BTW, are you still working on a new version of this patch? > > Thomas >
On Mon, 12 Jun 2017, David Hildenbrand wrote: > On 12.06.2017 18:44, Thomas Huth wrote: > > On 01.03.2017 13:19, Miroslav Benes wrote: > >> On Wed, 1 Mar 2017, Thomas Huth wrote: > >> > >>> On 28.02.2017 14:17, Miroslav Benes wrote: > >>>> Implement MVCOS instruction, which the Linux kernel uses in user access > >>>> functions. > >>>> > >>>> Signed-off-by: Miroslav Benes <mbenes@suse.cz> > >>>> --- > >>>> I tried to do my best to follow the specification but it is quite > >>>> possible that I got something wrong because of my lack of > >>>> understanding. Especially I am not sure about all those bit ops :/. > >>>> > >>>> Anyway, there is one piece missing. The actual use of keys and > >>>> address-space-control during the move. I used fast_memmove, but > >>>> it is not correct. Is there a helper which I could use? I looked at > >>>> other instructions which should implement access control, but there were > >>>> silently ignore it :). > >>> > >>> I'm not aware of a function that could deal with two address spaces > >>> already (but that does not mean that there is no such function already) > >>> ... still, I guess, you likely need to write your own memmove helper > >>> function that can deal with two different address spaces. > >> > >> Ok, I thought that was the case. I'll try to come up with something. > >> > >>>> target/s390x/helper.h | 1 + > >>>> target/s390x/insn-data.def | 2 ++ > >>>> target/s390x/mem_helper.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++ > >>>> target/s390x/translate.c | 12 +++++++ > >>>> 4 files changed, 95 insertions(+) > >>>> > >>>> diff --git a/target/s390x/helper.h b/target/s390x/helper.h > >>>> index 9102071d0aa4..bc5dfccc3d7e 100644 > >>>> --- a/target/s390x/helper.h > >>>> +++ b/target/s390x/helper.h > >>>> @@ -104,6 +104,7 @@ DEF_HELPER_FLAGS_2(iske, TCG_CALL_NO_RWG_SE, i64, env, i64) > >>>> DEF_HELPER_FLAGS_3(sske, TCG_CALL_NO_RWG, void, env, i64, i64) > >>>> DEF_HELPER_FLAGS_2(rrbe, TCG_CALL_NO_RWG, i32, env, i64) > >>>> DEF_HELPER_3(csp, i32, env, i32, i64) > >>>> +DEF_HELPER_5(mvcos, i32, env, i64, i64, i64, i64) > >>>> DEF_HELPER_4(mvcs, i32, env, i64, i64, i64) > >>>> DEF_HELPER_4(mvcp, i32, env, i64, i64, i64) > >>>> DEF_HELPER_4(sigp, i32, env, i64, i32, i64) > >>>> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def > >>>> index 075ff597c3de..a1e6d735d090 100644 > >>>> --- a/target/s390x/insn-data.def > >>>> +++ b/target/s390x/insn-data.def > >>>> @@ -854,6 +854,8 @@ > >>>> /* LOAD USING REAL ADDRESS */ > >>>> C(0xb24b, LURA, RRE, Z, 0, r2, new, r1_32, lura, 0) > >>>> C(0xb905, LURAG, RRE, Z, 0, r2, r1, 0, lurag, 0) > >>>> +/* MOVE WITH OPTIONAL SPECIFICATION */ > >>>> + C(0xc800, MVCOS, SSF, MVCOS, la1, a2, 0, 0, mvcos, 0) > >>>> /* MOVE TO PRIMARY */ > >>>> C(0xda00, MVCP, SS_d, Z, la1, a2, 0, 0, mvcp, 0) > >>>> /* MOVE TO SECONDARY */ > >>>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c > >>>> index 675aba2e44d4..ca8f7c49250c 100644 > >>>> --- a/target/s390x/mem_helper.c > >>>> +++ b/target/s390x/mem_helper.c > >>>> @@ -1089,6 +1089,86 @@ uint32_t HELPER(mvcp)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2) > >>>> return cc; > >>>> } > >>>> > >>>> +uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t r0, uint64_t dest, > >>>> + uint64_t src, uint64_t len) > >>>> +{ > >>>> + int cc; > >>>> + int key1, as1, abit1, kbit1; > >>>> + int key2, as2, abit2, kbit2; > >>>> + > >>>> + HELPER_LOG("%s dest %" PRIx64 ", src %" PRIx64 ", len %" PRIx64 "\n", > >>>> + __func__, dest, src, len); > >>>> + > >>>> + /* check DAT */ > >>>> + if (!(env->psw.mask & PSW_MASK_DAT)) { > >>>> + program_interrupt(env, PGM_SPECIAL_OP, 2); > >>> > >>> Length of the opcode is 6 bytes, not 2. > >> > >> True. Sorry, I don't know where 2 came from. It does not make sense. > > > > As I recently had to learn it the hard way (while implementing the TEST > > BLOCK instruction), you should use ILEN_LATER_INC here instead of 2 (or > > 6), since the Special operation exception is suppressing, too, i.e. the > > program counter should be increased afterwards to the next instruction. > > > > This is no longer needed with > https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02631.html > > You can either use 6 or ILEN_AUTO here, suppression will be handled > internally. ok, good to know. > > BTW, are you still working on a new version of this patch? Unfortunately, I haven't touched it since the submission. It is only a side project at work and I only started reading about s390x address spaces to implement a proper memmove helper. No implementation yet so far :/ So if anyone wants to work on this, they can of course take my patch as a basis. Thanks, Miroslav
On 13.06.2017 10:18, Miroslav Benes wrote: > On Mon, 12 Jun 2017, David Hildenbrand wrote: > >> On 12.06.2017 18:44, Thomas Huth wrote: >>> On 01.03.2017 13:19, Miroslav Benes wrote: >>>> On Wed, 1 Mar 2017, Thomas Huth wrote: >>>> >>>>> On 28.02.2017 14:17, Miroslav Benes wrote: >>>>>> Implement MVCOS instruction, which the Linux kernel uses in user access >>>>>> functions. >>>>>> >>>>>> Signed-off-by: Miroslav Benes <mbenes@suse.cz> >>>>>> --- >>>>>> I tried to do my best to follow the specification but it is quite >>>>>> possible that I got something wrong because of my lack of >>>>>> understanding. Especially I am not sure about all those bit ops :/. >>>>>> >>>>>> Anyway, there is one piece missing. The actual use of keys and >>>>>> address-space-control during the move. I used fast_memmove, but >>>>>> it is not correct. Is there a helper which I could use? I looked at >>>>>> other instructions which should implement access control, but there were >>>>>> silently ignore it :). >>>>> >>>>> I'm not aware of a function that could deal with two address spaces >>>>> already (but that does not mean that there is no such function already) >>>>> ... still, I guess, you likely need to write your own memmove helper >>>>> function that can deal with two different address spaces. >>>> >>>> Ok, I thought that was the case. I'll try to come up with something. >>>> >>>>>> target/s390x/helper.h | 1 + >>>>>> target/s390x/insn-data.def | 2 ++ >>>>>> target/s390x/mem_helper.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> target/s390x/translate.c | 12 +++++++ >>>>>> 4 files changed, 95 insertions(+) >>>>>> >>>>>> diff --git a/target/s390x/helper.h b/target/s390x/helper.h >>>>>> index 9102071d0aa4..bc5dfccc3d7e 100644 >>>>>> --- a/target/s390x/helper.h >>>>>> +++ b/target/s390x/helper.h >>>>>> @@ -104,6 +104,7 @@ DEF_HELPER_FLAGS_2(iske, TCG_CALL_NO_RWG_SE, i64, env, i64) >>>>>> DEF_HELPER_FLAGS_3(sske, TCG_CALL_NO_RWG, void, env, i64, i64) >>>>>> DEF_HELPER_FLAGS_2(rrbe, TCG_CALL_NO_RWG, i32, env, i64) >>>>>> DEF_HELPER_3(csp, i32, env, i32, i64) >>>>>> +DEF_HELPER_5(mvcos, i32, env, i64, i64, i64, i64) >>>>>> DEF_HELPER_4(mvcs, i32, env, i64, i64, i64) >>>>>> DEF_HELPER_4(mvcp, i32, env, i64, i64, i64) >>>>>> DEF_HELPER_4(sigp, i32, env, i64, i32, i64) >>>>>> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def >>>>>> index 075ff597c3de..a1e6d735d090 100644 >>>>>> --- a/target/s390x/insn-data.def >>>>>> +++ b/target/s390x/insn-data.def >>>>>> @@ -854,6 +854,8 @@ >>>>>> /* LOAD USING REAL ADDRESS */ >>>>>> C(0xb24b, LURA, RRE, Z, 0, r2, new, r1_32, lura, 0) >>>>>> C(0xb905, LURAG, RRE, Z, 0, r2, r1, 0, lurag, 0) >>>>>> +/* MOVE WITH OPTIONAL SPECIFICATION */ >>>>>> + C(0xc800, MVCOS, SSF, MVCOS, la1, a2, 0, 0, mvcos, 0) >>>>>> /* MOVE TO PRIMARY */ >>>>>> C(0xda00, MVCP, SS_d, Z, la1, a2, 0, 0, mvcp, 0) >>>>>> /* MOVE TO SECONDARY */ >>>>>> diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c >>>>>> index 675aba2e44d4..ca8f7c49250c 100644 >>>>>> --- a/target/s390x/mem_helper.c >>>>>> +++ b/target/s390x/mem_helper.c >>>>>> @@ -1089,6 +1089,86 @@ uint32_t HELPER(mvcp)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2) >>>>>> return cc; >>>>>> } >>>>>> >>>>>> +uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t r0, uint64_t dest, >>>>>> + uint64_t src, uint64_t len) >>>>>> +{ >>>>>> + int cc; >>>>>> + int key1, as1, abit1, kbit1; >>>>>> + int key2, as2, abit2, kbit2; >>>>>> + >>>>>> + HELPER_LOG("%s dest %" PRIx64 ", src %" PRIx64 ", len %" PRIx64 "\n", >>>>>> + __func__, dest, src, len); >>>>>> + >>>>>> + /* check DAT */ >>>>>> + if (!(env->psw.mask & PSW_MASK_DAT)) { >>>>>> + program_interrupt(env, PGM_SPECIAL_OP, 2); >>>>> >>>>> Length of the opcode is 6 bytes, not 2. >>>> >>>> True. Sorry, I don't know where 2 came from. It does not make sense. >>> >>> As I recently had to learn it the hard way (while implementing the TEST >>> BLOCK instruction), you should use ILEN_LATER_INC here instead of 2 (or >>> 6), since the Special operation exception is suppressing, too, i.e. the >>> program counter should be increased afterwards to the next instruction. >>> >> >> This is no longer needed with >> https://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg02631.html >> >> You can either use 6 or ILEN_AUTO here, suppression will be handled >> internally. > > ok, good to know. > >>> BTW, are you still working on a new version of this patch? > > Unfortunately, I haven't touched it since the submission. It is only a > side project at work and I only started reading about s390x address spaces > to implement a proper memmove helper. No implementation yet so far :/ > > So if anyone wants to work on this, they can of course take my patch as a > basis. I will look into it. I already have something running that almost works. For some reason, when I trick my upstream linux kernel in using mvcos for uaccess, I get my /init killed at some point, like it would be receiving some unexpected page fault. hmmm.... Thanks! > > Thanks, > Miroslav >
diff --git a/target/s390x/helper.h b/target/s390x/helper.h index 9102071d0aa4..bc5dfccc3d7e 100644 --- a/target/s390x/helper.h +++ b/target/s390x/helper.h @@ -104,6 +104,7 @@ DEF_HELPER_FLAGS_2(iske, TCG_CALL_NO_RWG_SE, i64, env, i64) DEF_HELPER_FLAGS_3(sske, TCG_CALL_NO_RWG, void, env, i64, i64) DEF_HELPER_FLAGS_2(rrbe, TCG_CALL_NO_RWG, i32, env, i64) DEF_HELPER_3(csp, i32, env, i32, i64) +DEF_HELPER_5(mvcos, i32, env, i64, i64, i64, i64) DEF_HELPER_4(mvcs, i32, env, i64, i64, i64) DEF_HELPER_4(mvcp, i32, env, i64, i64, i64) DEF_HELPER_4(sigp, i32, env, i64, i32, i64) diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def index 075ff597c3de..a1e6d735d090 100644 --- a/target/s390x/insn-data.def +++ b/target/s390x/insn-data.def @@ -854,6 +854,8 @@ /* LOAD USING REAL ADDRESS */ C(0xb24b, LURA, RRE, Z, 0, r2, new, r1_32, lura, 0) C(0xb905, LURAG, RRE, Z, 0, r2, r1, 0, lurag, 0) +/* MOVE WITH OPTIONAL SPECIFICATION */ + C(0xc800, MVCOS, SSF, MVCOS, la1, a2, 0, 0, mvcos, 0) /* MOVE TO PRIMARY */ C(0xda00, MVCP, SS_d, Z, la1, a2, 0, 0, mvcp, 0) /* MOVE TO SECONDARY */ diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c index 675aba2e44d4..ca8f7c49250c 100644 --- a/target/s390x/mem_helper.c +++ b/target/s390x/mem_helper.c @@ -1089,6 +1089,86 @@ uint32_t HELPER(mvcp)(CPUS390XState *env, uint64_t l, uint64_t a1, uint64_t a2) return cc; } +uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t r0, uint64_t dest, + uint64_t src, uint64_t len) +{ + int cc; + int key1, as1, abit1, kbit1; + int key2, as2, abit2, kbit2; + + HELPER_LOG("%s dest %" PRIx64 ", src %" PRIx64 ", len %" PRIx64 "\n", + __func__, dest, src, len); + + /* check DAT */ + if (!(env->psw.mask & PSW_MASK_DAT)) { + program_interrupt(env, PGM_SPECIAL_OP, 2); + } + + /* access control for the first operand */ + abit1 = (r0 & 0x0010000ULL) >> 16; + kbit1 = (r0 & 0x0020000ULL) >> 17; + as1 = (r0 & 0x00c00000ULL) >> 22; + key1 = (r0 & 0xf0000000ULL) >> 28; + + if (!kbit1) { + key1 = (env->psw.mask & PSW_MASK_KEY) >> (PSW_SHIFT_KEY - 4); + } + + if (!abit1) { + as1 = (env->psw.mask & PSW_MASK_ASC) >> 46; + } + + /* + * abit1 is set, as1 designates the home-space mode, psw is in the problem + * state. + * */ + if (abit1 && (as1 == 3) && (env->psw.mask & PSW_MASK_PSTATE)) { + program_interrupt(env, PGM_SPECIAL_OP, 2); + } + + /* access control for the second operand */ + abit2 = (r0 & 0x0010000ULL); + kbit2 = (r0 & 0x0020000ULL) >> 1; + as2 = (r0 & 0x00c00000ULL) >> 6; + key2 = (r0 & 0xf0000000ULL) >> 12; + + if (!kbit2) { + key2 = (env->psw.mask & PSW_MASK_KEY) >> (PSW_SHIFT_KEY - 4); + } + + if (!abit2) { + as2 = (env->psw.mask & PSW_MASK_ASC) >> 46; + } + + /* + * Secondary-space control bit is zero (bit 37 of r0) and either as + * designates secondary-space mode. + */ + if (!(r0 & 0x2000000000ULL) && (as1 == 2 || as2 == 2)) { + program_interrupt(env, PGM_SPECIAL_OP, 2); + } + + /* psw is in the problem state and either key is invalid */ + if ((env->psw.mask & PSW_MASK_PSTATE) && + (!(env->cregs[3] & (1 << (31 - key1))) || + !(env->cregs[3] & (1 << (31 - key2))))) { + program_interrupt(env, PGM_PRIVILEGED, 2); + } + + if (len <= 4096) { + cc = 0; + } else { + cc = 3; + len = 4096; + } + + /* move */ + /* XXX use keys and as during the move */ + fast_memmove(env, dest, src, len); + + return cc; +} + /* invalidate pte */ void HELPER(ipte)(CPUS390XState *env, uint64_t pte_addr, uint64_t vaddr) { diff --git a/target/s390x/translate.c b/target/s390x/translate.c index 01c62176bf70..ac90b758d312 100644 --- a/target/s390x/translate.c +++ b/target/s390x/translate.c @@ -1194,6 +1194,7 @@ typedef enum DisasFacility { FAC_SCF, /* store clock fast */ FAC_SFLE, /* store facility list extended */ FAC_ILA, /* interlocked access facility 1 */ + FAC_MVCOS, /* move-with-optional-specification */ } DisasFacility; struct DisasInsn { @@ -2877,6 +2878,17 @@ static ExitStatus op_mvcs(DisasContext *s, DisasOps *o) set_cc_static(s); return NO_EXIT; } + +static ExitStatus op_mvcos(DisasContext *s, DisasOps *o) +{ + int r3 = get_field(s->fields, r3); + + check_privileged(s); + potential_page_fault(s); + gen_helper_mvcos(cc_op, cpu_env, regs[0], o->addr1, o->in2, regs[r3]); + set_cc_static(s); + return NO_EXIT; +} #endif static ExitStatus op_mvpg(DisasContext *s, DisasOps *o)
Implement MVCOS instruction, which the Linux kernel uses in user access functions. Signed-off-by: Miroslav Benes <mbenes@suse.cz> --- I tried to do my best to follow the specification but it is quite possible that I got something wrong because of my lack of understanding. Especially I am not sure about all those bit ops :/. Anyway, there is one piece missing. The actual use of keys and address-space-control during the move. I used fast_memmove, but it is not correct. Is there a helper which I could use? I looked at other instructions which should implement access control, but there were silently ignore it :). target/s390x/helper.h | 1 + target/s390x/insn-data.def | 2 ++ target/s390x/mem_helper.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++ target/s390x/translate.c | 12 +++++++ 4 files changed, 95 insertions(+)