All of lore.kernel.org
 help / color / mirror / Atom feed
From: Amed Magdy <amagdy.afifi@gmail.com>
To: Richard Henderson <richard.henderson@linaro.org>
Cc: qemu-devel@nongnu.org, qemu-riscv@nongnu.org,
	sagark@eecs.berkeley.edu, kbastian@mail.uni-paderborn.de,
	palmer@sifive.com, mjc@sifive.com, Alistair.Francis@wdc.com
Subject: Re: [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes
Date: Tue, 26 Feb 2019 09:58:25 +0200	[thread overview]
Message-ID: <CAKV=O7n02WMfgmfFqtC9OBgHppPYVO4RXPnKyfU8y872Lnr=zQ@mail.gmail.com> (raw)
In-Reply-To: <bf134899-fa0b-2a27-dffd-92369805fd50@linaro.org>

> > It seems to me that the C extension can be enabled at any point, since
> if C is
> > off, you know that the next insn is aligned modulo 4.
> >
>

 Ok, This is mostly right. When C extension is enabled 32-bit base
instructions can be aligned on 2 bytes boundaries instead of 4 bytes only.
So multiple enables and disables of C bit at different code areas
theoretically may require this check on C extension enable. I'm not really
sure, may be this is might not  be a practical use scenario.

> It is only if the C extension is enabled, and you want to disable it,
> that is
> > when we must check to see if the next insn is aligned mod 4.  It is
> trivial to
> > arrange for a particular instruction to be aligned, via assembler
> directives.
> > So it seems silly to make the definition of the csr write to misa any
> more
> > complicated than it is.
>
>  I completely agree with you that C extension disable should have
alignment check.

>
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 960d2b0aa9..8726ef802e 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -370,10 +370,11 @@ static int write_misa(CPURISCVState *env, int
> csrno,
> > target_ulong val)
> >          val &= ~RVD;
> >    }
>
> > -    /* Suppress 'C' if next instruction is not aligned
> > -     * TODO: this should check next_pc
> > +    /*
> > +     * Suppress 'C' if next instruction is not aligned.
> > +     * We updated env->pc to the next insn in the translator.
> >      */
> > -    if ((val & RVC) && (GETPC() & ~3) != 0) {
> > +    if ((val & RVC) && (env->pc & ~3) != 0) {
> >         val &= ~RVC;
> >     }


 Just a hint, (env->pc & 3) instead of (env->pc & ~3) , right ?

>

 Thanks,
 Ahmed

WARNING: multiple messages have this Message-ID (diff)
From: Amed Magdy <amagdy.afifi@gmail.com>
To: Richard Henderson <richard.henderson@linaro.org>
Cc: qemu-devel@nongnu.org, qemu-riscv@nongnu.org,
	sagark@eecs.berkeley.edu,  kbastian@mail.uni-paderborn.de,
	palmer@sifive.com, mjc@sifive.com,  Alistair.Francis@wdc.com
Subject: Re: [Qemu-riscv] [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes
Date: Tue, 26 Feb 2019 09:58:25 +0200	[thread overview]
Message-ID: <CAKV=O7n02WMfgmfFqtC9OBgHppPYVO4RXPnKyfU8y872Lnr=zQ@mail.gmail.com> (raw)
In-Reply-To: <bf134899-fa0b-2a27-dffd-92369805fd50@linaro.org>

[-- Attachment #1: Type: text/plain, Size: 1719 bytes --]

> > It seems to me that the C extension can be enabled at any point, since
> if C is
> > off, you know that the next insn is aligned modulo 4.
> >
>

 Ok, This is mostly right. When C extension is enabled 32-bit base
instructions can be aligned on 2 bytes boundaries instead of 4 bytes only.
So multiple enables and disables of C bit at different code areas
theoretically may require this check on C extension enable. I'm not really
sure, may be this is might not  be a practical use scenario.

> It is only if the C extension is enabled, and you want to disable it,
> that is
> > when we must check to see if the next insn is aligned mod 4.  It is
> trivial to
> > arrange for a particular instruction to be aligned, via assembler
> directives.
> > So it seems silly to make the definition of the csr write to misa any
> more
> > complicated than it is.
>
>  I completely agree with you that C extension disable should have
alignment check.

>
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 960d2b0aa9..8726ef802e 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -370,10 +370,11 @@ static int write_misa(CPURISCVState *env, int
> csrno,
> > target_ulong val)
> >          val &= ~RVD;
> >    }
>
> > -    /* Suppress 'C' if next instruction is not aligned
> > -     * TODO: this should check next_pc
> > +    /*
> > +     * Suppress 'C' if next instruction is not aligned.
> > +     * We updated env->pc to the next insn in the translator.
> >      */
> > -    if ((val & RVC) && (GETPC() & ~3) != 0) {
> > +    if ((val & RVC) && (env->pc & ~3) != 0) {
> >         val &= ~RVC;
> >     }


 Just a hint, (env->pc & 3) instead of (env->pc & ~3) , right ?

>

 Thanks,
 Ahmed

[-- Attachment #2: Type: text/html, Size: 2881 bytes --]

  reply	other threads:[~2019-02-26  7:58 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-22 16:25 [Qemu-devel] Add proper alignment check and pending 'C' extension for riscv amagdy.afifi
2019-02-22 16:25 ` [Qemu-riscv] " amagdy.afifi
2019-02-22 16:25 ` [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes amagdy.afifi
2019-02-22 16:25   ` [Qemu-riscv] " amagdy.afifi
2019-02-22 23:57   ` [Qemu-devel] " Richard Henderson
2019-02-22 23:57     ` [Qemu-riscv] " Richard Henderson
2019-02-24  7:57     ` Amed Magdy
2019-02-24  7:57       ` [Qemu-riscv] " Amed Magdy
2019-02-24 19:04       ` Richard Henderson
2019-02-24 19:04         ` [Qemu-riscv] " Richard Henderson
2019-02-26  7:58         ` Amed Magdy [this message]
2019-02-26  7:58           ` Amed Magdy
2019-02-26  8:11           ` Amed Magdy
2019-02-26  8:11             ` [Qemu-riscv] " Amed Magdy
2019-02-23 21:45   ` Eric Blake
2019-02-23 21:45     ` [Qemu-riscv] " Eric Blake
2019-02-24  8:07     ` Amed Magdy
2019-02-24  8:07       ` [Qemu-riscv] " Amed Magdy
2019-02-25 14:14       ` Eric Blake
2019-02-25 14:14         ` [Qemu-riscv] " Eric Blake

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='CAKV=O7n02WMfgmfFqtC9OBgHppPYVO4RXPnKyfU8y872Lnr=zQ@mail.gmail.com' \
    --to=amagdy.afifi@gmail.com \
    --cc=Alistair.Francis@wdc.com \
    --cc=kbastian@mail.uni-paderborn.de \
    --cc=mjc@sifive.com \
    --cc=palmer@sifive.com \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-riscv@nongnu.org \
    --cc=richard.henderson@linaro.org \
    --cc=sagark@eecs.berkeley.edu \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.