From: David Hildenbrand <david@redhat.com>
To: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>, qemu-devel@nongnu.org
Cc: thuth@redhat.com, cohuck@redhat.com, rth@twiddle.net
Subject: Re: [PATCH v2] target/s390x: fix execution with icount
Date: Mon, 2 Nov 2020 10:34:54 +0100 [thread overview]
Message-ID: <af2ae869-08fc-540f-2829-89bc3fa4f5ae@redhat.com> (raw)
In-Reply-To: <160430921917.21500.1486722139653938240.stgit@pasha-ThinkPad-X280>
On 02.11.20 10:26, Pavel Dovgalyuk wrote:
> This patch adds some gen_io_start() calls to allow execution
> of s390x targets in icount mode with -smp 1.
> It enables deterministic timers and record/replay features.
Why do we have to set it for SIGP?
>
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru>
>
> ---
>
> v2:
> - added IF_IO flag to reuse icount code in translate_one()
> (suggested by Richard Henderson)
> ---
> target/s390x/insn-data.def | 70 ++++++++++++++++++++++----------------------
> target/s390x/translate.c | 15 +++++++++
> 2 files changed, 50 insertions(+), 35 deletions(-)
>
> diff --git a/target/s390x/insn-data.def b/target/s390x/insn-data.def
> index d3bcdfd67b..b95bc98d35 100644
> --- a/target/s390x/insn-data.def
> +++ b/target/s390x/insn-data.def
> @@ -379,7 +379,7 @@
> /* EXTRACT CPU ATTRIBUTE */
> C(0xeb4c, ECAG, RSY_a, GIE, 0, a2, r1, 0, ecag, 0)
> /* EXTRACT CPU TIME */
> - C(0xc801, ECTG, SSF, ECT, 0, 0, 0, 0, ectg, 0)
> + F(0xc801, ECTG, SSF, ECT, 0, 0, 0, 0, ectg, 0, IF_IO)
> /* EXTRACT FPC */
> F(0xb38c, EFPC, RRE, Z, 0, 0, new, r1_32, efpc, 0, IF_BFP)
> /* EXTRACT PSW */
> @@ -855,10 +855,10 @@
> C(0xe32f, STRVG, RXY_a, Z, la2, r1_o, new, m1_64, rev64, 0)
>
> /* STORE CLOCK */
> - C(0xb205, STCK, S, Z, la2, 0, new, m1_64, stck, 0)
> - C(0xb27c, STCKF, S, SCF, la2, 0, new, m1_64, stck, 0)
> + F(0xb205, STCK, S, Z, la2, 0, new, m1_64, stck, 0, IF_IO)
> + F(0xb27c, STCKF, S, SCF, la2, 0, new, m1_64, stck, 0, IF_IO)
> /* STORE CLOCK EXTENDED */
> - C(0xb278, STCKE, S, Z, 0, a2, 0, 0, stcke, 0)
> + F(0xb278, STCKE, S, Z, 0, a2, 0, 0, stcke, 0, IF_IO)
>
> /* STORE FACILITY LIST EXTENDED */
> C(0xb2b0, STFLE, S, SFLE, 0, a2, 0, 0, stfle, 0)
> @@ -1269,7 +1269,7 @@
> E(0xb250, CSP, RRE, Z, r1_32u, ra2, r1_P, 0, csp, 0, MO_TEUL, IF_PRIV)
> E(0xb98a, CSPG, RRE, DAT_ENH, r1_o, ra2, r1_P, 0, csp, 0, MO_TEQ, IF_PRIV)
> /* DIAGNOSE (KVM hypercall) */
> - F(0x8300, DIAG, RSI, Z, 0, 0, 0, 0, diag, 0, IF_PRIV)
> + F(0x8300, DIAG, RSI, Z, 0, 0, 0, 0, diag, 0, IF_PRIV | IF_IO)
> /* INSERT STORAGE KEY EXTENDED */
> F(0xb229, ISKE, RRE, Z, 0, r2_o, new, r1_8, iske, 0, IF_PRIV)
> /* INVALIDATE DAT TABLE ENTRY */
> @@ -1301,17 +1301,17 @@
> /* RESET REFERENCE BIT EXTENDED */
> F(0xb22a, RRBE, RRE, Z, 0, r2_o, 0, 0, rrbe, 0, IF_PRIV)
> /* SERVICE CALL LOGICAL PROCESSOR (PV hypercall) */
> - F(0xb220, SERVC, RRE, Z, r1_o, r2_o, 0, 0, servc, 0, IF_PRIV)
> + F(0xb220, SERVC, RRE, Z, r1_o, r2_o, 0, 0, servc, 0, IF_PRIV | IF_IO)
> /* SET ADDRESS SPACE CONTROL FAST */
> F(0xb279, SACF, S, Z, 0, a2, 0, 0, sacf, 0, IF_PRIV)
> /* SET CLOCK */
> - F(0xb204, SCK, S, Z, la2, 0, 0, 0, sck, 0, IF_PRIV)
> + F(0xb204, SCK, S, Z, la2, 0, 0, 0, sck, 0, IF_PRIV | IF_IO)
> /* SET CLOCK COMPARATOR */
> - F(0xb206, SCKC, S, Z, 0, m2_64a, 0, 0, sckc, 0, IF_PRIV)
> + F(0xb206, SCKC, S, Z, 0, m2_64a, 0, 0, sckc, 0, IF_PRIV | IF_IO)
> /* SET CLOCK PROGRAMMABLE FIELD */
> F(0x0107, SCKPF, E, Z, 0, 0, 0, 0, sckpf, 0, IF_PRIV)
> /* SET CPU TIMER */
> - F(0xb208, SPT, S, Z, 0, m2_64a, 0, 0, spt, 0, IF_PRIV)
> + F(0xb208, SPT, S, Z, 0, m2_64a, 0, 0, spt, 0, IF_PRIV | IF_IO)
> /* SET PREFIX */
> F(0xb210, SPX, S, Z, 0, m2_32ua, 0, 0, spx, 0, IF_PRIV)
> /* SET PSW KEY FROM ADDRESS */
> @@ -1321,7 +1321,7 @@
> /* SET SYSTEM MASK */
> F(0x8000, SSM, S, Z, 0, m2_8u, 0, 0, ssm, 0, IF_PRIV)
> /* SIGNAL PROCESSOR */
> - F(0xae00, SIGP, RS_a, Z, 0, a2, 0, 0, sigp, 0, IF_PRIV)
> + F(0xae00, SIGP, RS_a, Z, 0, a2, 0, 0, sigp, 0, IF_PRIV | IF_IO)
> /* STORE CLOCK COMPARATOR */
> F(0xb207, STCKC, S, Z, la2, 0, new, m1_64a, stckc, 0, IF_PRIV)
> /* STORE CONTROL */
> @@ -1332,7 +1332,7 @@
> /* STORE CPU ID */
> F(0xb202, STIDP, S, Z, la2, 0, new, m1_64a, stidp, 0, IF_PRIV)
> /* STORE CPU TIMER */
> - F(0xb209, STPT, S, Z, la2, 0, new, m1_64a, stpt, 0, IF_PRIV)
> + F(0xb209, STPT, S, Z, la2, 0, new, m1_64a, stpt, 0, IF_PRIV | IF_IO)
> /* STORE FACILITY LIST */
> F(0xb2b1, STFL, S, Z, 0, 0, 0, 0, stfl, 0, IF_PRIV)
> /* STORE PREFIX */
> @@ -1352,35 +1352,35 @@
> C(0xe501, TPROT, SSE, Z, la1, a2, 0, 0, tprot, 0)
>
> /* CCW I/O Instructions */
> - F(0xb276, XSCH, S, Z, 0, 0, 0, 0, xsch, 0, IF_PRIV)
> - F(0xb230, CSCH, S, Z, 0, 0, 0, 0, csch, 0, IF_PRIV)
> - F(0xb231, HSCH, S, Z, 0, 0, 0, 0, hsch, 0, IF_PRIV)
> - F(0xb232, MSCH, S, Z, 0, insn, 0, 0, msch, 0, IF_PRIV)
> - F(0xb23b, RCHP, S, Z, 0, 0, 0, 0, rchp, 0, IF_PRIV)
> - F(0xb238, RSCH, S, Z, 0, 0, 0, 0, rsch, 0, IF_PRIV)
> - F(0xb237, SAL, S, Z, 0, 0, 0, 0, sal, 0, IF_PRIV)
> - F(0xb23c, SCHM, S, Z, 0, insn, 0, 0, schm, 0, IF_PRIV)
> - F(0xb274, SIGA, S, Z, 0, 0, 0, 0, siga, 0, IF_PRIV)
> - F(0xb23a, STCPS, S, Z, 0, 0, 0, 0, stcps, 0, IF_PRIV)
> - F(0xb233, SSCH, S, Z, 0, insn, 0, 0, ssch, 0, IF_PRIV)
> - F(0xb239, STCRW, S, Z, 0, insn, 0, 0, stcrw, 0, IF_PRIV)
> - F(0xb234, STSCH, S, Z, 0, insn, 0, 0, stsch, 0, IF_PRIV)
> - F(0xb236, TPI , S, Z, la2, 0, 0, 0, tpi, 0, IF_PRIV)
> - F(0xb235, TSCH, S, Z, 0, insn, 0, 0, tsch, 0, IF_PRIV)
> + F(0xb276, XSCH, S, Z, 0, 0, 0, 0, xsch, 0, IF_PRIV | IF_IO)
> + F(0xb230, CSCH, S, Z, 0, 0, 0, 0, csch, 0, IF_PRIV | IF_IO)
> + F(0xb231, HSCH, S, Z, 0, 0, 0, 0, hsch, 0, IF_PRIV | IF_IO)
> + F(0xb232, MSCH, S, Z, 0, insn, 0, 0, msch, 0, IF_PRIV | IF_IO)
> + F(0xb23b, RCHP, S, Z, 0, 0, 0, 0, rchp, 0, IF_PRIV | IF_IO)
> + F(0xb238, RSCH, S, Z, 0, 0, 0, 0, rsch, 0, IF_PRIV | IF_IO)
> + F(0xb237, SAL, S, Z, 0, 0, 0, 0, sal, 0, IF_PRIV | IF_IO)
> + F(0xb23c, SCHM, S, Z, 0, insn, 0, 0, schm, 0, IF_PRIV | IF_IO)
> + F(0xb274, SIGA, S, Z, 0, 0, 0, 0, siga, 0, IF_PRIV | IF_IO)
> + F(0xb23a, STCPS, S, Z, 0, 0, 0, 0, stcps, 0, IF_PRIV | IF_IO)
> + F(0xb233, SSCH, S, Z, 0, insn, 0, 0, ssch, 0, IF_PRIV | IF_IO)
> + F(0xb239, STCRW, S, Z, 0, insn, 0, 0, stcrw, 0, IF_PRIV | IF_IO)
> + F(0xb234, STSCH, S, Z, 0, insn, 0, 0, stsch, 0, IF_PRIV | IF_IO)
> + F(0xb236, TPI , S, Z, la2, 0, 0, 0, tpi, 0, IF_PRIV | IF_IO)
> + F(0xb235, TSCH, S, Z, 0, insn, 0, 0, tsch, 0, IF_PRIV | IF_IO)
> /* ??? Not listed in PoO ninth edition, but there's a linux driver that
> uses it: "A CHSC subchannel is usually present on LPAR only." */
> - F(0xb25f, CHSC, RRE, Z, 0, insn, 0, 0, chsc, 0, IF_PRIV)
> + F(0xb25f, CHSC, RRE, Z, 0, insn, 0, 0, chsc, 0, IF_PRIV | IF_IO)
>
> /* zPCI Instructions */
> /* None of these instructions are documented in the PoP, so this is all
> based upon target/s390x/kvm.c and Linux code and likely incomplete */
> - F(0xebd0, PCISTB, RSY_a, PCI, la2, 0, 0, 0, pcistb, 0, IF_PRIV)
> - F(0xebd1, SIC, RSY_a, AIS, r1, r3, 0, 0, sic, 0, IF_PRIV)
> - F(0xb9a0, CLP, RRF_c, PCI, 0, 0, 0, 0, clp, 0, IF_PRIV)
> - F(0xb9d0, PCISTG, RRE, PCI, 0, 0, 0, 0, pcistg, 0, IF_PRIV)
> - F(0xb9d2, PCILG, RRE, PCI, 0, 0, 0, 0, pcilg, 0, IF_PRIV)
> - F(0xb9d3, RPCIT, RRE, PCI, 0, 0, 0, 0, rpcit, 0, IF_PRIV)
> - F(0xe3d0, MPCIFC, RXY_a, PCI, la2, 0, 0, 0, mpcifc, 0, IF_PRIV)
> - F(0xe3d4, STPCIFC, RXY_a, PCI, la2, 0, 0, 0, stpcifc, 0, IF_PRIV)
> + F(0xebd0, PCISTB, RSY_a, PCI, la2, 0, 0, 0, pcistb, 0, IF_PRIV | IF_IO)
> + F(0xebd1, SIC, RSY_a, AIS, r1, r3, 0, 0, sic, 0, IF_PRIV | IF_IO)
> + F(0xb9a0, CLP, RRF_c, PCI, 0, 0, 0, 0, clp, 0, IF_PRIV | IF_IO)
> + F(0xb9d0, PCISTG, RRE, PCI, 0, 0, 0, 0, pcistg, 0, IF_PRIV | IF_IO)
> + F(0xb9d2, PCILG, RRE, PCI, 0, 0, 0, 0, pcilg, 0, IF_PRIV | IF_IO)
> + F(0xb9d3, RPCIT, RRE, PCI, 0, 0, 0, 0, rpcit, 0, IF_PRIV | IF_IO)
> + F(0xe3d0, MPCIFC, RXY_a, PCI, la2, 0, 0, 0, mpcifc, 0, IF_PRIV | IF_IO)
> + F(0xe3d4, STPCIFC, RXY_a, PCI, la2, 0, 0, 0, stpcifc, 0, IF_PRIV | IF_IO)
>
> #endif /* CONFIG_USER_ONLY */
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index ac10f42f10..7a8ff1f2de 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -1214,6 +1214,7 @@ typedef struct {
> #define IF_DFP 0x0010 /* decimal floating point instruction */
> #define IF_PRIV 0x0020 /* privileged instruction */
> #define IF_VEC 0x0040 /* vector instruction */
> +#define IF_IO 0x0080 /* input/output instruction */
>
> struct DisasInsn {
> unsigned opc:16;
> @@ -6369,6 +6370,7 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
> const DisasInsn *insn;
> DisasJumpType ret = DISAS_NEXT;
> DisasOps o = {};
> + bool icount = false;
>
> /* Search for the insn in the table. */
> insn = extract_insn(env, s);
> @@ -6435,6 +6437,14 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
> return DISAS_NORETURN;
> }
> }
> +
> + /* input/output is the special case for icount mode */
> + if (insn->flags & IF_IO) {
I guess this is an unlikely().
> + icount = tb_cflags(s->base.tb) & CF_USE_ICOUNT;
> + if (icount) {
> + gen_io_start();
> + }
> + }
> }
>
> /* Check for insn specification exceptions. */
> @@ -6488,6 +6498,11 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
> tcg_temp_free_i64(o.addr1);
> }
>
> + /* io should be the last instruction in tb when icount is enabled */
> + if (icount && ret == DISAS_NEXT) {
> + ret = DISAS_PC_STALE;
> + }
> +
dito.
> #ifndef CONFIG_USER_ONLY
> if (s->base.tb->flags & FLAG_MASK_PER) {
> /* An exception might be triggered, save PSW if not already done. */
>
In general, looks sane to me.
--
Thanks,
David / dhildenb
next prev parent reply other threads:[~2020-11-02 9:36 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-02 9:26 [PATCH v2] target/s390x: fix execution with icount Pavel Dovgalyuk
2020-11-02 9:31 ` Philippe Mathieu-Daudé
2020-11-02 9:34 ` David Hildenbrand [this message]
2020-11-02 9:41 ` Pavel Dovgalyuk
2020-11-02 9:45 ` David Hildenbrand
2020-11-02 15:54 ` Richard Henderson
2020-11-04 17:31 ` Cornelia Huck
2020-11-05 5:52 ` Pavel Dovgalyuk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=af2ae869-08fc-540f-2829-89bc3fa4f5ae@redhat.com \
--to=david@redhat.com \
--cc=cohuck@redhat.com \
--cc=pavel.dovgalyuk@ispras.ru \
--cc=qemu-devel@nongnu.org \
--cc=rth@twiddle.net \
--cc=thuth@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).