* [Qemu-devel] [RFC PATCH] target-s390x: Implement mvcos instruction
@ 2017-02-28 13:17 Miroslav Benes
2017-03-01 11:11 ` Thomas Huth
0 siblings, 1 reply; 7+ messages in thread
From: Miroslav Benes @ 2017-02-28 13:17 UTC (permalink / raw)
To: rth, agraf; +Cc: qemu-devel, mmarek, ebischoff, thuth, Miroslav Benes
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(+)
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)
--
2.12.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] target-s390x: Implement mvcos instruction
2017-02-28 13:17 [Qemu-devel] [RFC PATCH] target-s390x: Implement mvcos instruction Miroslav Benes
@ 2017-03-01 11:11 ` Thomas Huth
2017-03-01 12:19 ` Miroslav Benes
0 siblings, 1 reply; 7+ messages in thread
From: Thomas Huth @ 2017-03-01 11:11 UTC (permalink / raw)
To: Miroslav Benes, rth, agraf; +Cc: qemu-devel, mmarek, ebischoff
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)
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] target-s390x: Implement mvcos instruction
2017-03-01 11:11 ` Thomas Huth
@ 2017-03-01 12:19 ` Miroslav Benes
2017-06-12 16:44 ` Thomas Huth
0 siblings, 1 reply; 7+ messages in thread
From: Miroslav Benes @ 2017-03-01 12:19 UTC (permalink / raw)
To: Thomas Huth; +Cc: rth, agraf, qemu-devel, mmarek, ebischoff
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] target-s390x: Implement mvcos instruction
2017-03-01 12:19 ` Miroslav Benes
@ 2017-06-12 16:44 ` Thomas Huth
2017-06-12 20:03 ` David Hildenbrand
0 siblings, 1 reply; 7+ messages in thread
From: Thomas Huth @ 2017-06-12 16:44 UTC (permalink / raw)
To: Miroslav Benes
Cc: qemu-devel, ebischoff, mmarek, agraf, rth, Aurelien Jarno,
David Hildenbrand
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] target-s390x: Implement mvcos instruction
2017-06-12 16:44 ` Thomas Huth
@ 2017-06-12 20:03 ` David Hildenbrand
2017-06-13 8:18 ` Miroslav Benes
0 siblings, 1 reply; 7+ messages in thread
From: David Hildenbrand @ 2017-06-12 20:03 UTC (permalink / raw)
To: Thomas Huth, Miroslav Benes
Cc: qemu-devel, ebischoff, mmarek, agraf, rth, Aurelien Jarno
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
>
--
Thanks,
David
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] target-s390x: Implement mvcos instruction
2017-06-12 20:03 ` David Hildenbrand
@ 2017-06-13 8:18 ` Miroslav Benes
2017-06-13 16:21 ` David Hildenbrand
0 siblings, 1 reply; 7+ messages in thread
From: Miroslav Benes @ 2017-06-13 8:18 UTC (permalink / raw)
To: David Hildenbrand
Cc: Thomas Huth, qemu-devel, ebischoff, mmarek, agraf, rth,
Aurelien Jarno
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC PATCH] target-s390x: Implement mvcos instruction
2017-06-13 8:18 ` Miroslav Benes
@ 2017-06-13 16:21 ` David Hildenbrand
0 siblings, 0 replies; 7+ messages in thread
From: David Hildenbrand @ 2017-06-13 16:21 UTC (permalink / raw)
To: Miroslav Benes
Cc: Thomas Huth, qemu-devel, ebischoff, mmarek, agraf, rth,
Aurelien Jarno
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
>
--
Thanks,
David
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-06-13 16:21 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-02-28 13:17 [Qemu-devel] [RFC PATCH] target-s390x: Implement mvcos instruction Miroslav Benes
2017-03-01 11:11 ` Thomas Huth
2017-03-01 12:19 ` Miroslav Benes
2017-06-12 16:44 ` Thomas Huth
2017-06-12 20:03 ` David Hildenbrand
2017-06-13 8:18 ` Miroslav Benes
2017-06-13 16:21 ` David Hildenbrand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).