Re: [PATCH] target/riscv: fix check of guest pa top bits
Yeah, I've run into the same issue. Even if you ask Gmail to treat the
email as plain text it still takes the liberty of hard wrapping lines
without asking you. It really is a shame that there isn't a good way to
submit patches via a web UI, the art of git send-email + manual SMTP isn't
as widespread as it once was.
Jonathan
On Fri, May 1, 2020 at 2:54 PM Jose Martins wrote:
> Just resubmitted version 2. Sorry. Not really used to this. I actually
> wasn't using git send-email. I was copying the patch to my email
> client which was causing the weird wrapping. I think I also fixed the
> issues raised by checkpatch. Hope everything is correct now.
>
> Jose
>
> On Thu, 30 Apr 2020 at 22:36, Alistair Francis
> wrote:
> >
> > On Thu, Apr 30, 2020 at 12:45 PM Alistair Francis
> wrote:
> > >
> > > On Fri, Apr 24, 2020 at 8:10 AM Jose Martins
> wrote:
> > > >
> > > > The spec states that on sv39x4 guest physical "address bits 63:41
> > > > must all be zeros, or else a guest-page-fault exception occurs.".
> > > > However, the check performed for these top bits of the virtual
> address
> > > > on the second stage is the same as the one performed for virtual
> > > > addresses on the first stage except with the 2-bit extension,
> > > > effectively creating the same kind of "hole" in the guest's physical
> > > > address space. I believe the following patch fixes this issue:
> > > >
> > > > Signed-off-by: Jose Martins
> > >
> > > Thanks for the patch.
> > >
> > > Reviewed-by: Alistair Francis
> > >
> > > > ---
> > > > target/riscv/cpu_helper.c | 20 +---
> > > > 1 file changed, 13 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > > index d3ba9efb02..da879f5656 100644
> > > > --- a/target/riscv/cpu_helper.c
> > > > +++ b/target/riscv/cpu_helper.c
> > > > @@ -421,15 +421,21 @@ static int get_physical_address(CPURISCVState
> > > > *env, hwaddr *physical,
> > >
> > > There seems to be a strange wrapping here, that corrupts the patch.
> > > For future patches can you please check your git send-email setup?
> > >
> > > Applied to the RISC-V tree with the above line fixed up.
> >
> > This patch also fails checkpatch.
> >
> > Do you mind resending a v2 with the check patch issues fixed?
> >
> > Alistair
> >
> > >
> > > Alistair
> > >
> > > > int va_bits = PGSHIFT + levels * ptidxbits + widened;
> > > > target_ulong mask, masked_msbs;
> > > >
> > > > -if (TARGET_LONG_BITS > (va_bits - 1)) {
> > > > -mask = (1L << (TARGET_LONG_BITS - (va_bits - 1))) - 1;
> > > > +if(!first_stage){
> > > > +if ((addr >> va_bits) != 0) {
> > > > +return TRANSLATE_FAIL;
> > > > +}
> > > > } else {
> > > > -mask = 0;
> > > > -}
> > > > -masked_msbs = (addr >> (va_bits - 1)) & mask;
> > > > +if (TARGET_LONG_BITS > (va_bits - 1)) {
> > > > +mask = (1L << (TARGET_LONG_BITS - (va_bits - 1))) - 1;
> > > > +} else {
> > > > +mask = 0;
> > > > +}
> > > > +masked_msbs = (addr >> (va_bits - 1)) & mask;
> > > >
> > > > -if (masked_msbs != 0 && masked_msbs != mask) {
> > > > -return TRANSLATE_FAIL;
> > > > +if (masked_msbs != 0 && masked_msbs != mask) {
> > > > +return TRANSLATE_FAIL;
> > > > +}
> > > > }
> > > >
> > > > int ptshift = (levels - 1) * ptidxbits;
> > > > --
> > > > 2.17.1
> > > >
> > > > Jose
> > > >
>
>
Re: [PATCH] riscv: Add semihosting support [v4]
The text you are referencing (the couple italic paragraphs below section 2.8 in the unprivileged ISA) is non-normative and "can be skipped if the reader is only interested in the specification itself". This convention of making indented italic text non-normative is described at the bottom of page 1 of the linked document. Jonathan On Wed, Jan 29, 2020 at 11:45 AM Keith Packard via wrote: > Peter Maydell writes: > > > True but irrelevant. You need to refer to a proper > > risc-v specification for your semihosting. > > The RISC-V Foundation defined semihosting as relative to the existing > ARM specification, so using a link to that is appropriate here. > > Here's the current specification of the unprivileged ISA, which includes > the definition of semihosting > > https://riscv.org/specifications/ > > While it may be nice in some abstract sense to create a "better" > semihosting spec, that's not what the RISC-V foundation has decided to > do. > > -- > -keith >
Re: [PATCH] target/riscv: Disallow WFI instruction from U-mode
Haha, fair enough. I just copied that line from one of the other functions in that file, which all use the same style. The check is actually a bit worse than it looks because PRV_S is defined to be 1. Hence, the whole thing is equivalent to just writing `env->priv == PRV_U`. I can send out a new version with that changed. Jonathan On Thu, Jan 23, 2020 at 6:35 PM Richard Henderson < richard.hender...@linaro.org> wrote: > On 1/23/20 9:52 AM, Jonathan Behrens wrote: > > +if (!(env->priv >= PRV_S) || > > For integers, !(x >= y) is a poor way to write x < y. > > > r~ > >
[PATCH] target/riscv: Disallow WFI instruction from U-mode
>From the RISC-V Priviliged Specification:
"When S-mode is implemented, then executing WFI in U-mode causes an illegal
instruction exception, unless it completes within an implementation-specific,
bounded time limit. A future revision of this specification might add a feature
that allows S-mode to selectively permit WFI in U-mode."
Signed-off-by: Jonathan Behrens
Reviewed-by: Alistair Francis
---
target/riscv/op_helper.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 331cc36232..2e5a980192 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -129,10 +129,10 @@ target_ulong helper_mret(CPURISCVState *env, target_ulong
cpu_pc_deb)
void helper_wfi(CPURISCVState *env)
{
CPUState *cs = env_cpu(env);
-
-if (env->priv == PRV_S &&
-env->priv_ver >= PRIV_VERSION_1_10_0 &&
-get_field(env->mstatus, MSTATUS_TW)) {
+if (!(env->priv >= PRV_S) ||
+(env->priv == PRV_S &&
+ env->priv_ver >= PRIV_VERSION_1_10_0 &&
+ get_field(env->mstatus, MSTATUS_TW))) {
riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
} else {
cs->halted = 1;
--
2.25.0
Re: [PATCH v1 15/36] target/riscv: Convert mstatus to pointers
> > I was trying to avoid forcing everyone to understand the Hypervisor > extension to develop for RISC-V. I agree a full understanding of the hypervisor extension shouldn't be required. But if code is going to be dealing with supervisor mode registers, then whether the machine is in HS or VS mode is going to matter in a lot of cases. The cases where it doesn't matter are particularly troubling: it wouldn't be obvious from reading the code whether the author forgot about the possibility of register swapping or if the code actually works then too. Plus, for custom forks we can always add a one line comment above the register declarations saying you can just s*xxx*[NOVIRT] to access the normal supervisor registers unless you want to make sure your code works with the H-extension. For example we don't have to check > virtualization status to dump the registers, we can just dump them and > know they are in the correct state (we do dump more is we have the > extension though). Image if someone adds a new way to dump registers, > I would like them not to care about if we are in V=1 or V=0 and they > can just dump them and know that the CSRs are correct. > This is almost a best case for being orthogonal to other features because you don't have to understand anything about how the registers impact machine execution in order to dump them. But even here it is somewhat debatable whether a naive implementation would dump the "correct" versions of these registers. One view is that what you really want would be the values that M-mode code would see since it has the most objective view of the system. Another is that they are the values that would be returned if the the current operating mode tried to access them, or undefined if that access would trap. Maybe you even want to query the value for a specific mode independent of the current privilege level. If the behavior you want happens to match up to the chosen design, then yes a naive implementation that is oblivious the hypervisor extension will work without having to understand anything. However, I'd attribute that more to luck than elegance of a particular option. Jonathan On Tue, Jan 21, 2020 at 7:01 PM Alistair Francis wrote: > On Tue, Jan 21, 2020 at 10:56 PM Jonathan Behrens > wrote: > > > > When I looked through the relevant code a few months ago, I couldn't > find anywhere that could actually be agnostic to whether the real or > virtual registers were in effect (other than emulating the actual CSR > modification instructions). For almost all state, the VS behavior is > filtered by HS-mode code. For example, you can grab satp in either mode but > to properly do address translation you also have to factor in hgatp so you > need to know the virtualization state anyway. Similarly, floating point > co-proccessor state is tracked in two places with V=1 so that both the host > and the guest can independently track dirty bits, but of course only the > one in the "real" mstatus applies in non-virtualized mode. > > So the idea is that if you aren't interested in the Hypervisor > extension you can just access the CSRs as usual (inside QEMU's source > code). That is you don't need to know anything about the Hypervisor > extension to add support for other extensions or to work on the RISC-V > target in QEMU. > > I was trying to avoid forcing everyone to understand the Hypervisor > extension to develop for RISC-V. For example we don't have to check > virtulasition status to dump the registers, we can just dump them and > know they are in the correct state (we do dump more is we have the > extension though). Image if someone adds a new way to dump registers, > I would like them not to care about if we are in V=1 or V=0 and they > can just dump them and know that the CSRs are correct. > > > > > The idea of using an array to track the two versions of registers seemed > really elegant to me. If you know you want the host version you access > element zero. If you know you want the guest version you access element > one. And if you want the version that the running code would see you index > by the virtualization mode. In any case, the choice indicates that you > thought though which was the right option to use in that instance. > > mstatus really is the only one that has any complications. The others > have a vs* version, which could be converted to an array but it > doesn't really matter as we just swap them. > > Alistair > > > > > Jonathan > > > > On Tue, Jan 21, 2020 at 6:02 AM Alistair Francis > wrote: > >> > >> On Wed, Jan 8, 2020 at 11:30 AM Palmer Dabbelt < > palmerdabb...@google.com> wrote: > >> > > >> > On Mon, 09 Dec 2019 10:11:19 PST (-0800), Alistair Francis wrote: > >> > > To hand
Re: [Qemu-devel] [PATCH] target/riscv: Disallow WFI instruction from U-mode
I don't think this was ever merged?
On Wed, Jul 3, 2019 at 10:37 PM wrote:
> Patchew URL:
> https://patchew.org/QEMU/20190703190715.5328-1-jonat...@fintelia.io/
>
>
>
> Hi,
>
> This series failed the asan build test. Please find the testing commands
> and
> their output below. If you have Docker installed, you can probably
> reproduce it
> locally.
>
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> make docker-image-fedora V=1 NETWORK=1
> time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14
> NETWORK=1
> === TEST SCRIPT END ===
>
> PASS 1 fdc-test /x86_64/fdc/cmos
> PASS 2 fdc-test /x86_64/fdc/no_media_on_start
> PASS 3 fdc-test /x86_64/fdc/read_without_media
> ==7862==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> PASS 4 fdc-test /x86_64/fdc/media_change
> PASS 5 fdc-test /x86_64/fdc/sense_interrupt
> PASS 6 fdc-test /x86_64/fdc/relative_seek
> ---
> PASS 32 test-opts-visitor /visitor/opts/range/beyond
> PASS 33 test-opts-visitor /visitor/opts/dict/unvisited
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> tests/test-coroutine -m=quick -k --tap < /dev/null | ./scripts/
> tap-driver.pl --test-name="test-coroutine"
> ==7913==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> ==7913==WARNING: ASan is ignoring requested __asan_handle_no_return: stack
> top: 0x7ffea51ee000; bottom 0x7fcaef4f8000; size: 0x0033b5cf6000
> (222093598720)
> False positive error reports may follow
> For details see https://github.com/google/sanitizers/issues/189
> PASS 1 test-coroutine /basic/no-dangling-access
> ---
> PASS 11 test-aio /aio/event/wait
> PASS 12 test-aio /aio/event/flush
> PASS 13 test-aio /aio/event/wait/no-flush-cb
> ==7929==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> PASS 14 test-aio /aio/timer/schedule
> PASS 15 test-aio /aio/coroutine/queue-chaining
> PASS 16 test-aio /aio-gsource/flush
> ---
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 QTEST_QEMU_IMG=qemu-img
> tests/ide-test -m=quick -k --tap < /dev/null | ./scripts/tap-driver.pl
> --test-name="ide-test"
> PASS 28 test-aio /aio-gsource/timer/schedule
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> tests/test-aio-multithread -m=quick -k --tap < /dev/null | ./scripts/
> tap-driver.pl --test-name="test-aio-multithread"
> ==7938==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> ==7942==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> PASS 1 test-aio-multithread /aio/multi/lifecycle
> PASS 1 ide-test /x86_64/ide/identify
> ==7959==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> PASS 2 test-aio-multithread /aio/multi/schedule
> PASS 2 ide-test /x86_64/ide/flush
> PASS 3 test-aio-multithread /aio/multi/mutex/contended
> ==7970==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> PASS 3 ide-test /x86_64/ide/bmdma/simple_rw
> ==7981==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> PASS 4 ide-test /x86_64/ide/bmdma/trim
> ==7987==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> PASS 4 test-aio-multithread /aio/multi/mutex/handoff
> PASS 5 ide-test /x86_64/ide/bmdma/short_prdt
> PASS 5 test-aio-multithread /aio/multi/mutex/mcs
> ==7998==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> PASS 6 ide-test /x86_64/ide/bmdma/one_sector_short_prdt
> PASS 6 test-aio-multithread /aio/multi/mutex/pthread
> ==8009==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> tests/test-throttle -m=quick -k --tap < /dev/null | ./scripts/
> tap-driver.pl --test-name="test-throttle"
> ==8016==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false positives in some cases!
> PASS 1 test-throttle /throttle/leak_bucket
> PASS 2 test-throttle /throttle/compute_wait
> PASS 3 test-throttle /throttle/init
> ---
> PASS 15 test-throttle /throttle/config/iops_size
> MALLOC_PERTURB_=${MALLOC_PERTURB_:-$(( ${RANDOM:-0} % 255 + 1))}
> tests/test-thread-pool -m=quick -k --tap < /dev/null | ./scripts/
> tap-driver.pl --test-name="test-thread-pool"
> PASS 7 ide-test /x86_64/ide/bmdma/long_prdt
> ==8022==WARNING: ASan doesn't fully support makecontext/swapcontext
> functions and may produce false
Re: [PATCH v1 1/1] target/riscv: Correctly implement TSR trap
Looks good to me. Though this is I think the third bug in privilege
checking in op_helper.c which is only like 150 lines long total. It would
be really good to fully double check that there aren't any more lurking
there...
Reviewed-by: Jonathan Behrens >
On Tue, Jan 21, 2020 at 12:45 AM Alistair Francis
wrote:
> As reported in: https://bugs.launchpad.net/qemu/+bug/1851939 we weren't
> correctly handling illegal instructions based on the value of MSTATUS_TSR
> and the current privledge level.
>
> This patch fixes the issue raised in the bug by raising an illegal
> instruction if TSR is set and we are in S-Mode.
>
> Signed-off-by: Alistair Francis
> ---
> target/riscv/op_helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> index 331cc36232..eed8eea6f2 100644
> --- a/target/riscv/op_helper.c
> +++ b/target/riscv/op_helper.c
> @@ -83,7 +83,7 @@ target_ulong helper_sret(CPURISCVState *env,
> target_ulong cpu_pc_deb)
> }
>
> if (env->priv_ver >= PRIV_VERSION_1_10_0 &&
> -get_field(env->mstatus, MSTATUS_TSR)) {
> +get_field(env->mstatus, MSTATUS_TSR) && !(env->priv >= PRV_M)) {
> riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> }
>
> --
> 2.24.1
>
>
>
Re: [PATCH 0/2] RISC-V TIME CSR for privileged mode
This series doesn't seem to touch mcounteren.TM which should be hardwired to zero if no callback in provided, and writable otherwise. (I had a patch to do the hardwiring unconditionally, but it seems to have been accidentally dropped.) Other than that, I think the design is quite good. Jonathan On Tue, Jan 21, 2020 at 3:59 AM Anup Patel wrote: > This series adds emulation of TIME CSRs for privileged mode. With > this series, we see approximately 25+% improvement in hackbench > numbers for non-virtualized (or Host) Linux and 40+% improvement > in hackbench numbers for Guest/VM Linux. > > These patches are based on mainline/alistair/riscv-hyp-ext-v0.5.1 > branch of https://github.com/kvm-riscv/qemu.git and can be found > in riscv_time_csr_v1 branch of same repo. > > Anup Patel (2): > target/riscv: Emulate TIME CSRs for privileged mode > hw/riscv: Provide rdtime callback for TCG in CLINT emulation > > hw/riscv/sifive_clint.c | 1 + > target/riscv/cpu.h| 5 +++ > target/riscv/cpu_helper.c | 5 +++ > target/riscv/csr.c| 80 +-- > 4 files changed, 87 insertions(+), 4 deletions(-) > > -- > 2.17.1 > > >
Re: [Qemu-devel] [PATCH v2] target/riscv: Hardwire mcounter.TM and upper bits of [m|s]counteren
I was just doubling checking the status of this patch because it conflicts with the "RISC-V TIME CSR for privileged mode" PR that was just sent out, and it seems this never got merged? In any case, perhaps these changes should be rolled into that patch? On Wed, Aug 21, 2019 at 1:37 PM Palmer Dabbelt wrote: > On Wed, 14 Aug 2019 20:19:39 PDT (-0700), jonat...@fintelia.io wrote: > > Ping! What is the status of this patch? > > Sorry, I must have lost track of it. I've added it to my patch queue. > > > > > On Wed, Jul 3, 2019 at 2:02 PM Jonathan Behrens > > wrote: > > > >> Bin, that proposal proved to be somewhat more controversial than I was > >> expecting, since it was different than how currently available hardware > >> worked. This option seemed much more likely to be accepted in the short > >> term. > >> > >> Jonathan > >> > >> On Mon, Jul 1, 2019 at 9:26 PM Bin Meng wrote: > >> > >>> On Tue, Jul 2, 2019 at 8:20 AM Alistair Francis > >>> wrote: > >>> > > >>> > On Mon, Jul 1, 2019 at 8:56 AM wrote: > >>> > > > >>> > > From: Jonathan Behrens > >>> > > > >>> > > QEMU currently always triggers an illegal instruction exception > when > >>> > > code attempts to read the time CSR. This is valid behavor, but > only if > >>> > > the TM bit in mcounteren is hardwired to zero. This change also > >>> > > corrects mcounteren and scounteren CSRs to be 32-bits on both > 32-bit > >>> > > and 64-bit targets. > >>> > > > >>> > > Signed-off-by: Jonathan Behrens > >>> > > >>> > Reviewed-by: Alistair Francis > >>> > > >>> > >>> I am a little bit lost here. I think we agreed to allow directly read > >>> to time CSR when mcounteren.TM is set, no? > >>> > >>> Regards, > >>> Bin > >>> > >> >
Re: [PATCH v1 15/36] target/riscv: Convert mstatus to pointers
When I looked through the relevant code a few months ago, I couldn't find
anywhere that could actually be agnostic to whether the real or virtual
registers were in effect (other than emulating the actual CSR modification
instructions). For almost all state, the VS behavior is filtered by HS-mode
code. For example, you can grab satp in either mode but to properly do
address translation you also have to factor in hgatp so you need to know
the virtualization state anyway. Similarly, floating point co-proccessor
state is tracked in two places with V=1 so that both the host and the guest
can independently track dirty bits, but of course only the one in the
"real" mstatus applies in non-virtualized mode.
The idea of using an array to track the two versions of registers seemed
really elegant to me. If you know you want the host version you access
element zero. If you know you want the guest version you access element
one. And if you want the version that the running code would see you index
by the virtualization mode. In any case, the choice indicates that you
thought though which was the right option to use in that instance.
Jonathan
On Tue, Jan 21, 2020 at 6:02 AM Alistair Francis
wrote:
> On Wed, Jan 8, 2020 at 11:30 AM Palmer Dabbelt
> wrote:
> >
> > On Mon, 09 Dec 2019 10:11:19 PST (-0800), Alistair Francis wrote:
> > > To handle the new Hypervisor CSR register aliasing let's use pointers.
> >
> > For some reason I thought we were making this explicit? In other words,
> > requiring that all callers provide which privilege mode they're using
> when
> > accessing these CSRs, as opposed to swapping around pointers. I don't
> actually
> > care that much, but IIRC when we were talking with the ARM guys at
> Plumbers
> > they were pretty adament that would end up being a much cleaner
> implementation
> > as they'd tried this way and later changed over.
>
> I think their implementation is different so it doesn't apply the same
> here.
>
> My main concern is that due to the modularity of RISC-V I don't expect
> all future developers to keep track of the Hypervisor extensions. This
> way we always have the correct state in the registers.
>
> There is only one pointer variable left, so we could drop the pointer
> swapping part, but for now it's still here.
>
> Alistair
>
> >
> > > Signed-off-by: Alistair Francis
> > > ---
> > > target/riscv/cpu.c| 11 +--
> > > target/riscv/cpu.h| 9 -
> > > target/riscv/cpu_helper.c | 30 +++---
> > > target/riscv/csr.c| 20 ++--
> > > target/riscv/op_helper.c | 14 +++---
> > > 5 files changed, 49 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index a07c5689b3..e61cf46a73 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -236,7 +236,7 @@ static void riscv_cpu_dump_state(CPUState *cs,
> FILE *f, int flags)
> > > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "pc ", env->pc);
> > > #ifndef CONFIG_USER_ONLY
> > > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mhartid ",
> env->mhartid);
> > > -qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ",
> env->mstatus);
> > > +qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ",
> *env->mstatus);
> > > if (riscv_has_ext(env, RVH)) {
> > > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "hstatus ",
> env->hstatus);
> > > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "vsstatus ",
> env->vsstatus);
> > > @@ -336,7 +336,7 @@ static void riscv_cpu_reset(CPUState *cs)
> > > mcc->parent_reset(cs);
> > > #ifndef CONFIG_USER_ONLY
> > > env->priv = PRV_M;
> > > -env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
> > > +*env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
> > > env->mcause = 0;
> > > env->pc = env->resetvec;
> > > #endif
> > > @@ -465,8 +465,15 @@ static void riscv_cpu_realize(DeviceState *dev,
> Error **errp)
> > > static void riscv_cpu_init(Object *obj)
> > > {
> > > RISCVCPU *cpu = RISCV_CPU(obj);
> > > +#ifndef CONFIG_USER_ONLY
> > > +CPURISCVState *env = >env;
> > > +#endif
> > >
> > > cpu_set_cpustate_pointers(cpu);
> > > +
> > > +#ifndef CONFIG_USER_ONLY
> > > +env->mstatus = >mstatus_novirt;
> > > +#endif
> > > }
> > >
> > > static const VMStateDescription vmstate_riscv_cpu = {
> > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > > index 21ae5a8b19..9dc8303c62 100644
> > > --- a/target/riscv/cpu.h
> > > +++ b/target/riscv/cpu.h
> > > @@ -122,7 +122,7 @@ struct CPURISCVState {
> > > target_ulong resetvec;
> > >
> > > target_ulong mhartid;
> > > -target_ulong mstatus;
> > > +target_ulong *mstatus;
> > >
> > > target_ulong mip;
> > > uint32_t miclaim;
> > > @@ -145,6 +145,13 @@ struct CPURISCVState {
> > > target_ulong mcause;
> > > target_ulong mtval; /* since: priv-1.10.0 */
> > >
> > > +/* The following registers are
[PATCH v4 2/3] target/riscv: Expose "priv" register for GDB for reads
This patch enables a debugger to read the current privilege level via a virtual
"priv" register. When compiled with CONFIG_USER_ONLY the register is still
visible but always reports the value zero.
Signed-off-by: Jonathan Behrens
---
configure | 4 ++--
gdb-xml/riscv-32bit-virtual.xml | 11 +++
gdb-xml/riscv-64bit-virtual.xml | 11 +++
target/riscv/gdbstub.c | 23 +++
4 files changed, 47 insertions(+), 2 deletions(-)
create mode 100644 gdb-xml/riscv-32bit-virtual.xml
create mode 100644 gdb-xml/riscv-64bit-virtual.xml
diff --git a/configure b/configure
index 30544f52e6..6118a6a045 100755
--- a/configure
+++ b/configure
@@ -7520,13 +7520,13 @@ case "$target_name" in
TARGET_BASE_ARCH=riscv
TARGET_ABI_DIR=riscv
mttcg=yes
-gdb_xml_files="riscv-32bit-cpu.xml riscv-32bit-fpu.xml riscv-32bit-csr.xml"
+gdb_xml_files="riscv-32bit-cpu.xml riscv-32bit-fpu.xml riscv-32bit-csr.xml
riscv-32bit-virtual.xml"
;;
riscv64)
TARGET_BASE_ARCH=riscv
TARGET_ABI_DIR=riscv
mttcg=yes
-gdb_xml_files="riscv-64bit-cpu.xml riscv-64bit-fpu.xml riscv-64bit-csr.xml"
+gdb_xml_files="riscv-64bit-cpu.xml riscv-64bit-fpu.xml riscv-64bit-csr.xml
riscv-64bit-virtual.xml"
;;
sh4|sh4eb)
TARGET_ARCH=sh4
diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-xml/riscv-32bit-virtual.xml
new file mode 100644
index 00..905f1c555d
--- /dev/null
+++ b/gdb-xml/riscv-32bit-virtual.xml
@@ -0,0 +1,11 @@
+
+
+
+
+
+
+
diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-xml/riscv-64bit-virtual.xml
new file mode 100644
index 00..62d86c237b
--- /dev/null
+++ b/gdb-xml/riscv-64bit-virtual.xml
@@ -0,0 +1,11 @@
+
+
+
+
+
+
+
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index cb5bfd3d50..1f71604b78 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -373,6 +373,23 @@ static int riscv_gdb_set_csr(CPURISCVState *env, uint8_t
*mem_buf, int n)
return 0;
}
+static int riscv_gdb_get_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
+{
+if (n == 0) {
+#ifdef CONFIG_USER_ONLY
+return gdb_get_regl(mem_buf, 0);
+#else
+return gdb_get_regl(mem_buf, cs->priv);
+#endif
+}
+return 0;
+}
+
+static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
+{
+return 0;
+}
+
void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
{
RISCVCPU *cpu = RISCV_CPU(cs);
@@ -385,6 +402,9 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
240, "riscv-32bit-csr.xml", 0);
+
+gdb_register_coprocessor(cs, riscv_gdb_get_virtual, riscv_gdb_set_virtual,
+ 1, "riscv-32bit-virtual.xml", 0);
#elif defined(TARGET_RISCV64)
if (env->misa & RVF) {
gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
@@ -393,5 +413,8 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
240, "riscv-64bit-csr.xml", 0);
+
+gdb_register_coprocessor(cs, riscv_gdb_get_virtual, riscv_gdb_set_virtual,
+ 1, "riscv-64bit-virtual.xml", 0);
#endif
}
--
2.23.0
[PATCH v4 1/3] target/riscv: Tell gdbstub the correct number of CSRs
If the number of registers reported to the gdbstub code does not match the
number in the associated XML file, then the register numbers used by the stub
may get out of sync with a remote GDB instance.
Signed-off-by: Jonathan Behrens
Reviewed-by: Bin Meng
Reviewed-by: Alistair Francis
---
target/riscv/gdbstub.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index ded140e8d8..cb5bfd3d50 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -384,7 +384,7 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
}
gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
- 4096, "riscv-32bit-csr.xml", 0);
+ 240, "riscv-32bit-csr.xml", 0);
#elif defined(TARGET_RISCV64)
if (env->misa & RVF) {
gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
@@ -392,6 +392,6 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
}
gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
- 4096, "riscv-64bit-csr.xml", 0);
+ 240, "riscv-64bit-csr.xml", 0);
#endif
}
--
2.23.0
[PATCH v4 0/3] target/riscv: Expose "priv" register for GDB
This series adds a new "priv" virtual register that reports the current privilege mode. This is helpful for debugging purposes because that information is not actually available in any of the real CSRs. The third patch in this series makes the priv virtual register writitable. I'm not entirely sure this is a good idea, so I split it out into its own patch. In particular, this change will conflict with the hypervisor extension work which assumes that the privilege mode does not change in unexpected cases. As pointed out in a previous version of this series, GDB actually contains some support already for the accessing the privilege mode via a virtual "priv" register, including to convert the values into human readable forms: (gdb) info reg priv priv 0x3 prv:3 [Machine] Changlog V4: - Fix typo in filename Changlog V3: - Break patch into series - Make priv a virtual register Changelog V2: - Use PRV_H and PRV_S instead of integer literals Jonathan Behrens (3) target/riscv: Tell gdbstub the correct number of CSRs target/riscv: Expose priv register for GDB for reads target/riscv: Make the priv register writable by GDB configure | 4 ++-- gdb-xml/riscv-32bit-virtual.xml | 11 +++ gdb-xml/riscv-64bit-virtual.xml | 11 +++ target/riscv/gdbstub.c | 36 ++-- 4 files changed, 58 insertions(+), 4 deletions(-)
[PATCH v4 3/3] target/riscv: Make the priv register writable by GDB
Currently only PRV_U, PRV_S and PRV_M are supported, so this patch ensures that
the privilege mode is set to one of them. Once support for the H-extension is
added, this code will also need to properly update the virtualization status
when switching between VU/VS-modes and M-mode.
Signed-off-by: Jonathan Behrens
Reviewed-by: Bin Meng
Tested-by: Bin Meng
Reviewed-by: Alistair Francis
---
target/riscv/gdbstub.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index 1f71604b78..1a7947e019 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -387,6 +387,15 @@ static int riscv_gdb_get_virtual(CPURISCVState *cs,
uint8_t *mem_buf, int n)
static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
{
+if (n == 0) {
+#ifndef CONFIG_USER_ONLY
+cs->priv = ldtul_p(mem_buf) & 0x3;
+if (cs->priv == PRV_H) {
+cs->priv = PRV_S;
+}
+#endif
+return sizeof(target_ulong);
+}
return 0;
}
--
2.23.0
Re: [PATCH V6] target/riscv: Ignore reserved bits in PTE for RV64
There is nowhere in the spec that ever says what hardware has to do if
any of those reserved bits are non-zero. Hardware is certainly not
required to ignore them and treat the PTE as being valid (which is
what this patch does). I'd argue that since only buggy code would ever
set these bits, QEMU should treat any PTE with them set as being
invalid so that programmers can realize they've made a mistake.
Jonathan
On Sat, Oct 12, 2019 at 8:16 PM Guo Ren wrote:
>
> The patch didn't wrap the physical address space directly, just follow the
> spec.
> I admit that I am trying to use the compliance specification to allow
> qemu to support some non-standard software.
> But compliance specification and wrapping the physical address space
> are different things.
> I'm preparing c910 pachset for linux riscv and you can question me there.
>
> On Sun, Oct 13, 2019 at 1:33 AM Palmer Dabbelt wrote:
> >
> > On Wed, 25 Sep 2019 17:14:21 PDT (-0700), guo...@kernel.org wrote:
> > > From: Guo Ren
> > >
> > > Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we
> > > need to ignore them. They cannot be a part of ppn.
> > >
> > > 1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
> > >4.4 Sv39: Page-Based 39-bit Virtual-Memory System
> > >4.5 Sv48: Page-Based 48-bit Virtual-Memory System
> > >
> > > Signed-off-by: Guo Ren
> > > Tested-by: Bin Meng
> > > Reviewed-by: Liu Zhiwei
> > > Reviewed-by: Bin Meng
> > > Reviewed-by: Alistair Francis
> > > ---
> > > target/riscv/cpu_bits.h | 7 +++
> > > target/riscv/cpu_helper.c | 2 +-
> > > 2 files changed, 8 insertions(+), 1 deletion(-)
> > >
> > > Changelog V6:
> > > - Add Reviewer: Alistair Francis
> > >
> > > Changelog V5:
> > > - Add Reviewer and Tester: Bin Meng
> > >
> > > Changelog V4:
> > > - Change title to Ignore not Bugfix
> > > - Use PTE_PPN_MASK for RV32 and RV64
> > >
> > > Changelog V3:
> > > - Use UUL define for PTE_RESERVED
> > > - Keep ppn >> PTE_PPN_SHIFT
> > >
> > > Changelog V2:
> > > - Bugfix pte destroyed cause boot fail
> > > - Change to AND with a mask instead of shifting both directions
> > >
> > > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> > > index e998348..399c2c6 100644
> > > --- a/target/riscv/cpu_bits.h
> > > +++ b/target/riscv/cpu_bits.h
> > > @@ -473,6 +473,13 @@
> > > /* Page table PPN shift amount */
> > > #define PTE_PPN_SHIFT 10
> > >
> > > +/* Page table PPN mask */
> > > +#if defined(TARGET_RISCV32)
> > > +#define PTE_PPN_MASK0xUL
> > > +#elif defined(TARGET_RISCV64)
> > > +#define PTE_PPN_MASK0x3fULL
> > > +#endif
> > > +
> > > /* Leaf page shift amount */
> > > #define PGSHIFT 12
> > >
> > > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > > index 87dd6a6..9961b37 100644
> > > --- a/target/riscv/cpu_helper.c
> > > +++ b/target/riscv/cpu_helper.c
> > > @@ -261,7 +261,7 @@ restart:
> > > #elif defined(TARGET_RISCV64)
> > > target_ulong pte = ldq_phys(cs->as, pte_addr);
> > > #endif
> > > -hwaddr ppn = pte >> PTE_PPN_SHIFT;
> > > +hwaddr ppn = (pte & PTE_PPN_MASK) >> PTE_PPN_SHIFT;
> > >
> > > if (!(pte & PTE_V)) {
> > > /* Invalid PTE */
> >
> > I know I'm a bit late to the party here, but I don't like this. There's
> > ample
> > evidence that wrapping the physical address space is a bad idea, and just
> > because the ISA allows implementations to do this doesn't mean we should.
>
>
>
> --
> Best Regards
> Guo Ren
>
> ML: https://lore.kernel.org/linux-csky/
>
Re: [PATCH] target/riscv: PMP violation due to wrong size parameter
How do you know that the access won't straddle a page boundary? Is there a
guarantee somewhere that size=0 means that the access is naturally aligned?
Jonathan
On Fri, Oct 11, 2019 at 7:14 PM Dayeol Lee wrote:
> riscv_cpu_tlb_fill() uses the `size` parameter to check PMP violation
> using pmp_hart_has_privs().
> However, if the size is unknown (=0), the ending address will be
> `addr - 1` as it is `addr + size - 1` in `pmp_hart_has_privs()`.
> This always causes a false PMP violation on the starting address of the
> range, as `addr - 1` is not in the range.
>
> In order to fix, we just assume that all bytes from addr to the end of
> the page will be accessed if the size is unknown.
>
> Signed-off-by: Dayeol Lee
> Reviewed-by: Richard Henderson
> ---
> target/riscv/cpu_helper.c | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index e32b6126af..7d9a22b601 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -441,6 +441,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address,
> int size,
> CPURISCVState *env = >env;
> hwaddr pa = 0;
> int prot;
> +int pmp_size = 0;
> bool pmp_violation = false;
> int ret = TRANSLATE_FAIL;
> int mode = mmu_idx;
> @@ -460,9 +461,19 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address,
> int size,
>"%s address=%" VADDR_PRIx " ret %d physical "
> TARGET_FMT_plx
>" prot %d\n", __func__, address, ret, pa, prot);
>
> +/*
> + * if size is unknown (0), assume that all bytes
> + * from addr to the end of the page will be accessed.
> + */
> +if (size == 0) {
> +pmp_size = -(address | TARGET_PAGE_MASK);
> +} else {
> +pmp_size = size;
> +}
> +
> if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> (ret == TRANSLATE_SUCCESS) &&
> -!pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
> +!pmp_hart_has_privs(env, pa, pmp_size, 1 << access_type, mode)) {
> ret = TRANSLATE_PMP_FAIL;
> }
> if (ret == TRANSLATE_PMP_FAIL) {
> --
> 2.20.1
>
>
>
Re: [PATCH v3 1/3] target/riscv: Tell gdbstub the correct number of CSRs
On Tue, Oct 8, 2019 at 5:54 AM Bin Meng wrote: > > On Tue, Oct 8, 2019 at 8:15 AM Jonathan Behrens wrote: > > > > If the number of registers reported to the gdbstub code does not match the > > number in the associated XML file, then the register numbers used by the > > stub > > may get out of sync with a remote GDB instance. > > I am not sure how to trigger the out of sync issue. Do you know how? Try applying the next patch in the series without this one. Attempting to access the priv register will return E14 because GDB will think that priv has register number 309 (since the last register with an assigned regnum is fcsr=68 and then none of the 240 CSRs have assigned numbers) while the GDB stub will think that the number is 4165 (=33 + 36 + 4096).
Re: [PATCH v3 2/3] target/riscv: Expose priv register for GDB for reads
On Tue, Oct 8, 2019 at 8:27 AM Bin Meng wrote:
>
> On Tue, Oct 8, 2019 at 8:18 AM Jonathan Behrens wrote:
> >
> > This patch enables a debugger to read the current privilege level via a
> > virtual
> > "priv" register. When compiled with CONFIG_USER_ONLY the register is still
> > visible but always reports the value zero.
> >
> > Signed-off-by: Jonathan Behrens
> > ---
> > configure | 4 ++--
> > gdb-xml/riscv-32bit-virtual.xml | 11 +++
> > gdb-xml/riscv-64bit-virtual.xml | 11 +++
> > target/riscv/gdbstub.c | 23 +++
> > 4 files changed, 47 insertions(+), 2 deletions(-)
> > create mode 100644 gdb-xml/riscv-32bit-virtual.xml
> > create mode 100644 gdb-xml/riscv-64bit-virtual.xml
> >
> > diff --git a/configure b/configure
> > index 30544f52e6..6118a6a045 100755
> > --- a/configure
> > +++ b/configure
> > @@ -7520,13 +7520,13 @@ case "$target_name" in
> > TARGET_BASE_ARCH=riscv
> > TARGET_ABI_DIR=riscv
> > mttcg=yes
> > -gdb_xml_files="riscv-32bit-cpu.xml riscv-32bit-fpu.xml
> > riscv-32bit-csr.xml"
> > +gdb_xml_files="riscv-32bit-cpu.xml riscv-32bit-fpu.xml
> > riscv-32bit-csr.xml riscv-32bit-virtual.xml"
> >;;
> >riscv64)
> > TARGET_BASE_ARCH=riscv
> > TARGET_ABI_DIR=riscv
> > mttcg=yes
> > -gdb_xml_files="riscv-64bit-cpu.xml riscv-64bit-fpu.xml
> > riscv-64bit-csr.xml"
> > +gdb_xml_files="riscv-64bit-cpu.xml riscv-64bit-fpu.xml
> > riscv-64bit-csr.xml riscv-64bit-virtual.xml"
> >;;
> >sh4|sh4eb)
> > TARGET_ARCH=sh4
> > diff --git a/gdb-xml/riscv-32bit-virtual.xml
> > b/gdb-xml/riscv-32bit-virtual.xml
> > new file mode 100644
> > index 00..905f1c555d
> > --- /dev/null
> > +++ b/gdb-xml/riscv-32bit-virtual.xml
> > @@ -0,0 +1,11 @@
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > diff --git a/gdb-xml/riscv-64bit-virtual.xml
> > b/gdb-xml/riscv-64bit-virtual.xml
> > new file mode 100644
> > index 00..62d86c237b
> > --- /dev/null
> > +++ b/gdb-xml/riscv-64bit-virtual.xml
> > @@ -0,0 +1,11 @@
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> > index cb5bfd3d50..33cf7c4c7d 100644
> > --- a/target/riscv/gdbstub.c
> > +++ b/target/riscv/gdbstub.c
> > @@ -373,6 +373,23 @@ static int riscv_gdb_set_csr(CPURISCVState *env,
> > uint8_t *mem_buf, int n)
> > return 0;
> > }
> >
> > +static int riscv_gdb_get_virtual(CPURISCVState *cs, uint8_t *mem_buf, int
> > n)
> > +{
> > +if (n == 0) {
> > +#ifdef CONFIG_USER_ONLY
> > +return gdb_get_regl(mem_buf, 0);
> > +#else
> > +return gdb_get_regl(mem_buf, cs->priv);
> > +#endif
> > +}
> > +return 0;
> > +}
> > +
> > +static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int
> > n)
> > +{
> > +return 0;
> > +}
> > +
> > void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
> > {
> > RISCVCPU *cpu = RISCV_CPU(cs);
> > @@ -385,6 +402,9 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState
> > *cs)
> >
> > gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
> > 240, "riscv-32bit-csr.xml", 0);
> > +
> > +gdb_register_coprocessor(cs, riscv_gdb_get_virtual,
> > riscv_gdb_set_virtual,
> > + 1, "riscv-32bit-csr.xml", 0);
>
> This should be riscv-32bit-virtual.xml
Good catch. I'll fix this in the next version.
>
> > #elif defined(TARGET_RISCV64)
> > if (env->misa & RVF) {
> > gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
> > @@ -393,5 +413,8 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState
> > *cs)
> >
> > gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
> > 240, "riscv-64bit-csr.xml", 0);
> > +
> > +gdb_register_coprocessor(cs, riscv_gdb_get_virtual,
> > riscv_gdb_set_virtual,
> > + 1, "riscv-64bit-virtual.xml", 0);
> > #endif
> > }
>
> Regards,
> Bin
>
Re: [PATCH v2] target/riscv: Expose "priv" register for GDB
On Mon, Oct 7, 2019 at 2:36 PM Alistair Francis wrote:
> On Fri, Oct 4, 2019 at 8:18 AM Jonathan Behrens wrote:
> > @@ -296,6 +302,14 @@ int riscv_cpu_gdb_write_register(CPUState *cs, uint8_t
> > *mem_buf, int n)
> > } else if (n == 32) {
> > env->pc = ldtul_p(mem_buf);
> > return sizeof(target_ulong);
> > +} else if (n == 33) {
> > +#ifndef CONFIG_USER_ONLY
> > +env->priv = ldtul_p(mem_buf) & 0x3;
> > +if (env->priv == PRV_H) {
> > +env->priv = PRV_S;
> > +}
>
> Why have this? There is no PRV_H so we should never be in that privilege mode.
>
> Alistair
This is hopefully more clear in the next version, but the idea is that
since GDB can try to set the privilege mode to *any* value this
function needs to make sure that it isn't set to something unsupported
like PRV_H.
Jonathan
Re: [PATCH v2] target/riscv: Expose "priv" register for GDB
On Mon, Oct 7, 2019 at 5:17 PM Jim Wilson wrote:
> On 10/4/19 8:16 AM, Jonathan Behrens wrote:
> > diff --git a/gdb-xml/riscv-32bit-cpu.xml b/gdb-xml/riscv-32bit-cpu.xml
> > index 0d07aaec85..d6d76aafd8 100644
> > --- a/gdb-xml/riscv-32bit-cpu.xml
> > +++ b/gdb-xml/riscv-32bit-cpu.xml
> > @@ -44,4 +44,5 @@
> >
> >
> >
> > +
> >
>
> Adding this to the cpu register set means that the gdb "info registers"
> command will now list a value for the mysterious undocumented "priv"
> register. That is likely to result in user confusion, and a few gdb bug
> reports.
>
> Gdb incidentally already has support for a virtual priv register. From
> gdb/riscv-tdep.c:
>
> static const struct riscv_register_feature riscv_virtual_feature =
> {
> "org.gnu.gdb.riscv.virtual",
> {
> { RISCV_PRIV_REGNUM, { "priv" }, false }
> }
> };
>
> So the correct way to fix this is to add a
> gdb-xml/riscv-32bit-virtual.xml file, along with code to handle this new
> xml file and the registers in it. Likewise for the 64-bit support.
>
> The main advantage of doing things this way is that only people that
> care about the priv register will see it, and this will interoperate
> with other RISC-V debuggers and targets (if any) that already have
> virtual priv register support.
>
> Jim
Thanks for this suggestion, I've incorporated it into the next version.
Jonathan
[PATCH v3 3/3] target/riscv: Make the priv register writable by GDB
Currently only PRV_U, PRV_S and PRV_M are supported, so this patch ensures that
the privilege mode is set to one of them. Once support for the H-extension is
added, this code will also need to properly update the virtualization status
when switching between VU/VS-modes and M-mode.
Signed-off-by: Jonathan Behrens
---
target/riscv/gdbstub.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index 33cf7c4c7d..bc84b599c2 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -387,6 +387,15 @@ static int riscv_gdb_get_virtual(CPURISCVState *cs,
uint8_t *mem_buf, int n)
static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
{
+if (n == 0) {
+#ifndef CONFIG_USER_ONLY
+cs->priv = ldtul_p(mem_buf) & 0x3;
+if (cs->priv == PRV_H) {
+cs->priv = PRV_S;
+}
+#endif
+return sizeof(target_ulong);
+}
return 0;
}
--
2.23.0
[PATCH v3 2/3] target/riscv: Expose priv register for GDB for reads
This patch enables a debugger to read the current privilege level via a virtual
"priv" register. When compiled with CONFIG_USER_ONLY the register is still
visible but always reports the value zero.
Signed-off-by: Jonathan Behrens
---
configure | 4 ++--
gdb-xml/riscv-32bit-virtual.xml | 11 +++
gdb-xml/riscv-64bit-virtual.xml | 11 +++
target/riscv/gdbstub.c | 23 +++
4 files changed, 47 insertions(+), 2 deletions(-)
create mode 100644 gdb-xml/riscv-32bit-virtual.xml
create mode 100644 gdb-xml/riscv-64bit-virtual.xml
diff --git a/configure b/configure
index 30544f52e6..6118a6a045 100755
--- a/configure
+++ b/configure
@@ -7520,13 +7520,13 @@ case "$target_name" in
TARGET_BASE_ARCH=riscv
TARGET_ABI_DIR=riscv
mttcg=yes
-gdb_xml_files="riscv-32bit-cpu.xml riscv-32bit-fpu.xml riscv-32bit-csr.xml"
+gdb_xml_files="riscv-32bit-cpu.xml riscv-32bit-fpu.xml riscv-32bit-csr.xml
riscv-32bit-virtual.xml"
;;
riscv64)
TARGET_BASE_ARCH=riscv
TARGET_ABI_DIR=riscv
mttcg=yes
-gdb_xml_files="riscv-64bit-cpu.xml riscv-64bit-fpu.xml riscv-64bit-csr.xml"
+gdb_xml_files="riscv-64bit-cpu.xml riscv-64bit-fpu.xml riscv-64bit-csr.xml
riscv-64bit-virtual.xml"
;;
sh4|sh4eb)
TARGET_ARCH=sh4
diff --git a/gdb-xml/riscv-32bit-virtual.xml b/gdb-xml/riscv-32bit-virtual.xml
new file mode 100644
index 00..905f1c555d
--- /dev/null
+++ b/gdb-xml/riscv-32bit-virtual.xml
@@ -0,0 +1,11 @@
+
+
+
+
+
+
+
diff --git a/gdb-xml/riscv-64bit-virtual.xml b/gdb-xml/riscv-64bit-virtual.xml
new file mode 100644
index 00..62d86c237b
--- /dev/null
+++ b/gdb-xml/riscv-64bit-virtual.xml
@@ -0,0 +1,11 @@
+
+
+
+
+
+
+
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index cb5bfd3d50..33cf7c4c7d 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -373,6 +373,23 @@ static int riscv_gdb_set_csr(CPURISCVState *env, uint8_t
*mem_buf, int n)
return 0;
}
+static int riscv_gdb_get_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
+{
+if (n == 0) {
+#ifdef CONFIG_USER_ONLY
+return gdb_get_regl(mem_buf, 0);
+#else
+return gdb_get_regl(mem_buf, cs->priv);
+#endif
+}
+return 0;
+}
+
+static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
+{
+return 0;
+}
+
void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
{
RISCVCPU *cpu = RISCV_CPU(cs);
@@ -385,6 +402,9 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
240, "riscv-32bit-csr.xml", 0);
+
+gdb_register_coprocessor(cs, riscv_gdb_get_virtual, riscv_gdb_set_virtual,
+ 1, "riscv-32bit-csr.xml", 0);
#elif defined(TARGET_RISCV64)
if (env->misa & RVF) {
gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
@@ -393,5 +413,8 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
240, "riscv-64bit-csr.xml", 0);
+
+gdb_register_coprocessor(cs, riscv_gdb_get_virtual, riscv_gdb_set_virtual,
+ 1, "riscv-64bit-virtual.xml", 0);
#endif
}
--
2.23.0
Re: [PATCH v3 0/3] target/riscv: Expose "priv" register for GDB
The first paragraph seems to have gone missing. Repeated below: > This series adds a new "priv" virtual register that reports the current > privilege mode. This is helpful for debugging purposes because that > information > is not actually available in any of the real CSRs. On Mon, Oct 7, 2019 at 8:15 PM Jonathan Behrens wrote: > > The third patch in this series makes the priv virtual register writitable. I'm > not entirely sure this is a good idea, so I split it out into its own patch. > In > particular, this change will conflict with the hypervisor extension work which > assumes that the privilege mode does not change in unexpected cases. > > As pointed out in a previous version of this series, GDB actually contains > some > support already for the accessing the privilege mode via a virtual "priv" > register, including to convert the values into human readable forms: > > (gdb) info reg priv > priv 0x3 prv:3 [Machine] > > Changlog V3: > - Break patch into series > - Make priv a virtual register > > Changelog V2: > - Use PRV_H and PRV_S instead of integer literals > > Jonathan Behrens (3) > target/riscv: Tell gdbstub the correct number of CSRs > target/riscv: Expose priv register for GDB for reads > target/riscv: Make the priv register writable by GDB > > configure | 4 ++-- > gdb-xml/riscv-32bit-virtual.xml | 11 +++ > gdb-xml/riscv-64bit-virtual.xml | 11 +++ > target/riscv/gdbstub.c | 36 ++-- > 4 files changed, 58 insertions(+), 4 deletions(-) > >
[PATCH v3 1/3] target/riscv: Tell gdbstub the correct number of CSRs
If the number of registers reported to the gdbstub code does not match the
number in the associated XML file, then the register numbers used by the stub
may get out of sync with a remote GDB instance.
Signed-off-by: Jonathan Behrens
---
target/riscv/gdbstub.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index ded140e8d8..cb5bfd3d50 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -384,7 +384,7 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
}
gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
- 4096, "riscv-32bit-csr.xml", 0);
+ 240, "riscv-32bit-csr.xml", 0);
#elif defined(TARGET_RISCV64)
if (env->misa & RVF) {
gdb_register_coprocessor(cs, riscv_gdb_get_fpu, riscv_gdb_set_fpu,
@@ -392,6 +392,6 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
}
gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
- 4096, "riscv-64bit-csr.xml", 0);
+ 240, "riscv-64bit-csr.xml", 0);
#endif
}
--
2.23.0
[PATCH v3 0/3] target/riscv: Expose "priv" register for GDB
The third patch in this series makes the priv virtual register writitable. I'm not entirely sure this is a good idea, so I split it out into its own patch. In particular, this change will conflict with the hypervisor extension work which assumes that the privilege mode does not change in unexpected cases. As pointed out in a previous version of this series, GDB actually contains some support already for the accessing the privilege mode via a virtual "priv" register, including to convert the values into human readable forms: (gdb) info reg priv priv 0x3 prv:3 [Machine] Changlog V3: - Break patch into series - Make priv a virtual register Changelog V2: - Use PRV_H and PRV_S instead of integer literals Jonathan Behrens (3) target/riscv: Tell gdbstub the correct number of CSRs target/riscv: Expose priv register for GDB for reads target/riscv: Make the priv register writable by GDB configure | 4 ++-- gdb-xml/riscv-32bit-virtual.xml | 11 +++ gdb-xml/riscv-64bit-virtual.xml | 11 +++ target/riscv/gdbstub.c | 36 ++-- 4 files changed, 58 insertions(+), 4 deletions(-)
[PATCH v2] target/riscv: Expose "priv" register for GDB
This patch enables a debugger to read and write the current privilege level via
a special "priv" register. When compiled with CONFIG_USER_ONLY the register is
still visible but is hardwired to zero.
Signed-off-by: Jonathan Behrens
---
gdb-xml/riscv-32bit-cpu.xml | 1 +
gdb-xml/riscv-64bit-cpu.xml | 1 +
target/riscv/cpu.c | 2 +-
target/riscv/gdbstub.c | 14 ++
4 files changed, 17 insertions(+), 1 deletion(-)
---
Changelog V2:
- Use PRV_H and PRV_S instead of integer literals
diff --git a/gdb-xml/riscv-32bit-cpu.xml b/gdb-xml/riscv-32bit-cpu.xml
index 0d07aaec85..d6d76aafd8 100644
--- a/gdb-xml/riscv-32bit-cpu.xml
+++ b/gdb-xml/riscv-32bit-cpu.xml
@@ -44,4 +44,5 @@
+
diff --git a/gdb-xml/riscv-64bit-cpu.xml b/gdb-xml/riscv-64bit-cpu.xml
index b8aa424ae4..0758d1b5fe 100644
--- a/gdb-xml/riscv-64bit-cpu.xml
+++ b/gdb-xml/riscv-64bit-cpu.xml
@@ -44,4 +44,5 @@
+
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f13e298a36..347989858f 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -475,7 +475,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
cc->synchronize_from_tb = riscv_cpu_synchronize_from_tb;
cc->gdb_read_register = riscv_cpu_gdb_read_register;
cc->gdb_write_register = riscv_cpu_gdb_write_register;
-cc->gdb_num_core_regs = 33;
+cc->gdb_num_core_regs = 34;
#if defined(TARGET_RISCV32)
cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
#elif defined(TARGET_RISCV64)
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index ded140e8d8..7e0822145d 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -278,6 +278,12 @@ int riscv_cpu_gdb_read_register(CPUState *cs, uint8_t
*mem_buf, int n)
return gdb_get_regl(mem_buf, env->gpr[n]);
} else if (n == 32) {
return gdb_get_regl(mem_buf, env->pc);
+} else if (n == 33) {
+#ifdef CONFIG_USER_ONLY
+return gdb_get_regl(mem_buf, 0);
+#else
+return gdb_get_regl(mem_buf, env->priv);
+#endif
}
return 0;
}
@@ -296,6 +302,14 @@ int riscv_cpu_gdb_write_register(CPUState *cs, uint8_t
*mem_buf, int n)
} else if (n == 32) {
env->pc = ldtul_p(mem_buf);
return sizeof(target_ulong);
+} else if (n == 33) {
+#ifndef CONFIG_USER_ONLY
+env->priv = ldtul_p(mem_buf) & 0x3;
+if (env->priv == PRV_H) {
+env->priv = PRV_S;
+}
+#endif
+return sizeof(target_ulong);
}
return 0;
}
--
2.23.0
[PATCH] target/riscv: Expose "priv" register for GDB
This patch enables a debugger to read and write the current privilege level via
a special "priv" register. When compiled with CONFIG_USER_ONLY the register is
still visible but is hardwired to zero.
Signed-off-by: Jonathan Behrens
---
gdb-xml/riscv-32bit-cpu.xml | 1 +
gdb-xml/riscv-64bit-cpu.xml | 1 +
target/riscv/cpu.c | 2 +-
target/riscv/gdbstub.c | 14 ++
4 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/gdb-xml/riscv-32bit-cpu.xml b/gdb-xml/riscv-32bit-cpu.xml
index 0d07aaec85..d6d76aafd8 100644
--- a/gdb-xml/riscv-32bit-cpu.xml
+++ b/gdb-xml/riscv-32bit-cpu.xml
@@ -44,4 +44,5 @@
+
diff --git a/gdb-xml/riscv-64bit-cpu.xml b/gdb-xml/riscv-64bit-cpu.xml
index b8aa424ae4..0758d1b5fe 100644
--- a/gdb-xml/riscv-64bit-cpu.xml
+++ b/gdb-xml/riscv-64bit-cpu.xml
@@ -44,4 +44,5 @@
+
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f13e298a36..347989858f 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -475,7 +475,7 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
cc->synchronize_from_tb = riscv_cpu_synchronize_from_tb;
cc->gdb_read_register = riscv_cpu_gdb_read_register;
cc->gdb_write_register = riscv_cpu_gdb_write_register;
-cc->gdb_num_core_regs = 33;
+cc->gdb_num_core_regs = 34;
#if defined(TARGET_RISCV32)
cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
#elif defined(TARGET_RISCV64)
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index ded140e8d8..dc8cb4d26c 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -278,6 +278,12 @@ int riscv_cpu_gdb_read_register(CPUState *cs, uint8_t
*mem_buf, int n)
return gdb_get_regl(mem_buf, env->gpr[n]);
} else if (n == 32) {
return gdb_get_regl(mem_buf, env->pc);
+} else if (n == 33) {
+#ifdef CONFIG_USER_ONLY
+return gdb_get_regl(mem_buf, 0);
+#else
+return gdb_get_regl(mem_buf, env->priv);
+#endif
}
return 0;
}
@@ -296,6 +302,14 @@ int riscv_cpu_gdb_write_register(CPUState *cs, uint8_t
*mem_buf, int n)
} else if (n == 32) {
env->pc = ldtul_p(mem_buf);
return sizeof(target_ulong);
+} else if (n == 33) {
+#ifndef CONFIG_USER_ONLY
+env->priv = ldtul_p(mem_buf) & 0x3;
+if (env->priv == 2) {
+env->priv = 1;
+}
+#endif
+return sizeof(target_ulong);
}
return 0;
}
--
2.23.0
Re: [PATCH V3] target/riscv: Bugfix reserved bits in PTE for RV64
> The specification is very clear: these bits are not part of ppn, not > part of the translation target address. The current code is against > the riscv-privilege specification. If all of the reserved bits are zero then the patch changes nothing. Further the only normative mention of the reserved bits in the spec says they must be: "Bits 63–54 are reserved for future use and must be zeroed by software for forward compatibility." Provided that software follows the spec current QEMU will behave properly. For software that ignores that directive an sets some of those bits, the spec says nothing about what hardware should do, so both the old an the new behavior are fine. Jonathan
Re: [PATCH V3] target/riscv: Bugfix reserved bits in PTE for RV64
Any code whose behavior is changed by this patch is wrong, so it doesn't seem like it matters much whether this is merged or not. Personally I'd lean towards making sure that attempts to use PTEs with the reserved bit set would always fault, rather than wrapping around to low memory and perhaps silently succeeding. Jonathan On Wed, Sep 25, 2019 at 8:29 AM Guo Ren wrote: > On Wed, Sep 25, 2019 at 1:19 PM Alistair Francis > wrote: > > > > On Tue, Sep 24, 2019 at 9:48 PM wrote: > > > > > > From: Guo Ren > > > > > > Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so > we > > > need to ignore them. They can not be a part of ppn. > > > > > > 1: The RISC-V Instruction Set Manual, Volume II: Privileged > Architecture > > >4.4 Sv39: Page-Based 39-bit Virtual-Memory System > > >4.5 Sv48: Page-Based 48-bit Virtual-Memory System > > > > Hey, > > > > As I mentioned on patch 2 I don't think this is right. It isn't up to > > HW to clear these bits, software should keep them clear. > > These bits are not part of ppn in spec, so we still need to ignore them > for ppn. > > The patch is reasonable. > > -- > Best Regards > Guo Ren > > ML: https://lore.kernel.org/linux-csky/ > >
Re: [Qemu-devel] [Qemu-riscv] [PATCH v1 10/28] target/riscv: Convert mie and mstatus to pointers
On Thu, Sep 19, 2019 at 10:50 AM Richard Henderson
wrote:
>
> On 9/18/19 4:47 PM, Alistair Francis wrote:
> > I'm not a fan of the pointer method that I'm using, but to me it seems
> > the least worst in terms of handling future code, keeping everythign
> > consistnent and avoiding complex access rules.
>
> FWIW, I prefer the "banked" register method used by ARM.
>
> enum {
> M_REG_NS = 0,/* non-secure mode */
> M_REG_S = 1, /* secure mode */
> M_REG_NUM_BANKS = 2,
> };
>
> ...
>
> uint32_t vecbase[M_REG_NUM_BANKS];
> uint32_t basepri[M_REG_NUM_BANKS];
> uint32_t control[M_REG_NUM_BANKS];
>
> The major difference that I see is that a pointer can only represent a single
> state at a single time. With an index, different parts of the code can ask
> different questions that may have different states. E.g. "are we currently in
> secure mode" vs "will the exception return to secure mode".
This makes a lot of sense to me. It means that any individual control register
has an unambiguous name that doesn't change based on context. They aren't quite
the same names as used in the architecture specification (mie & vsie
vs. mie[NOVIRT] & mie[VIRT]), but they are reasonably close. It also means other
parts of the code can't ignore that there are two different versions of the
registers in play. Perhaps the biggest benefit though is that you can sidestep
swapping on mode changes *and* avoid needing any super fancy logic in the access
functions:
int read_mstatus(...) {
target_ulong novirt_mask = ...;
*val = env->mstatus[NOVIRT] & novirt_mask | env->mstatus[virt_mode()];
}
int read_vsstatus(...) {
*val = env->mstatus[VIRT];
}
int write_mstatus(...) {
...
target_ulong novirt_mask = ...;
env->mstatus[NOVIRT] = (env->mstatus[NOVIRT] & ~novirt_mask) |
(newval & novirt_mask);
env->mstatus[virt_mode()] = (env->mstatus[virt_mode()] & novirt_mask) |
(newval & ~novirt_mask);
}
Re: [Qemu-devel] [Qemu-riscv] [PATCH v1 10/28] target/riscv: Convert mie and mstatus to pointers
I went through the uses of mie in the entire hypervisor patch series and it
seems like it would be much simpler to just have two non-pointer fields in
the CPU struct: mie and vsie. To if an interrupt is pending, you are either
running with V=0 in which case the contents of vsie can be ignored, or you
are running with V=1 and have to check both anyway. And the
read_sie/write_sie would just need a single extra branch return proper
results. The read_mie and write_mie function wouldn't need to be changed at
all: if M-mode is running to access them, then V=0. (When queried from a
debugger while V=1, I think the result would actually be more correct this
way: the bits of mie are supposed to always reflect sie rather than vsie).
Jonathan
On Tue, Sep 17, 2019 at 7:37 PM Alistair Francis
wrote:
> On Wed, Sep 11, 2019 at 7:55 AM Jonathan Behrens
> wrote:
> >
> > Version 0.4 of the hypervisor spec no longer talks about swapping
> registers. Instead when running in VS-mode some of the supervisor registers
> are "aliased" and actually refer to alternate versions. Implementations are
> of course still allowed to do swapping internally if desired, but it adds
> complexity compared to a more straightforward implementation and isn't
> obvious to me whether QEMU would get any benefit from it. At least, it is
> probably worth fleshing out the rest of the v0.4 implementation before
> deciding on this patch.
>
> This patch is to handle the aliasing. The commit message isn't clear
> (I'll fix that up) but this patch is required to handle the new alias
> method instead of the previous swapping.
>
> Alistair
>
> > Jonathan
> >
> > On Wed, Sep 11, 2019 at 4:24 AM Palmer Dabbelt
> wrote:
> >>
> >> On Fri, 23 Aug 2019 16:38:15 PDT (-0700), Alistair Francis wrote:
> >> > To handle the new Hypervisor CSR register swapping let's use pointers.
> >> >
> >> > We only need to convert the MIE and MSTATUS CSRs. With the exception
> of
> >> > MIP all of the other CSRs that swap with virtulsation changes are
> S-Mode
> >> > only, so we can just do a lazy switch. This because more challenging
> for
> >> > the M-Mode registers so it ends up being easier to use pointers.
> >> >
> >> > As the MIP CSR is always accessed atomicly the pointer swap doesn't
> work
> >> > so we leave that as is.
> >> >
> >> > Signed-off-by: Alistair Francis
> >> > ---
> >> > target/riscv/cpu.c| 16
> >> > target/riscv/cpu.h| 12 ++--
> >> > target/riscv/cpu_helper.c | 32
> >> > target/riscv/csr.c| 28 ++--
> >> > target/riscv/op_helper.c | 14 +++---
> >> > 5 files changed, 59 insertions(+), 43 deletions(-)
> >> >
> >> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> > index be8f643fc2..371d5845af 100644
> >> > --- a/target/riscv/cpu.c
> >> > +++ b/target/riscv/cpu.c
> >> > @@ -228,7 +228,7 @@ static void riscv_cpu_dump_state(CPUState *cs,
> FILE *f, int flags)
> >> > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "pc ", env->pc);
> >> > #ifndef CONFIG_USER_ONLY
> >> > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mhartid ",
> env->mhartid);
> >> > -qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ",
> env->mstatus);
> >> > +qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ",
> *env->mstatus);
> >> > if (riscv_has_ext(env, RVH)) {
> >> > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "hstatus ",
> env->hstatus);
> >> > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "bstatus ",
> env->vsstatus);
> >> > @@ -239,7 +239,7 @@ static void riscv_cpu_dump_state(CPUState *cs,
> FILE *f, int flags)
> >> > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "vsip",
> >> > (target_ulong)atomic_read(>vsip));
> >> > }
> >> > -qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mie ", env->mie);
> >> > +qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mie ",
> *env->mie);
> >> > if (riscv_has_ext(env, RVH)) {
> >> > qemu_fprintf(f, " %s " TARGE
Re: [Qemu-devel] [Qemu-riscv] [PATCH v8 18/32] riscv: sifive_u: Set the minimum number of cpus to 2
Has there been testing with "-smp 2"? A while back I thought I read that
the included uboot firmware was using a hard-coded device tree that
indicated 4+1 CPUs, which I would have expected to cause Linux boot issues?
Jonathan
On Sun, Sep 15, 2019 at 1:31 PM Palmer Dabbelt wrote:
> On Sun, 15 Sep 2019 06:07:18 PDT (-0700), bmeng...@gmail.com wrote:
> > Hi Palmer,
> >
> > On Sun, Sep 15, 2019 at 3:00 AM Palmer Dabbelt
> wrote:
> >>
> >> On Fri, 13 Sep 2019 08:25:21 PDT (-0700), bmeng...@gmail.com wrote:
> >> > Hi Palmer,
> >> >
> >> > On Fri, Sep 13, 2019 at 10:33 PM Palmer Dabbelt
> wrote:
> >> >>
> >> >> On Fri, 06 Sep 2019 09:20:05 PDT (-0700), bmeng...@gmail.com wrote:
> >> >> > It is not useful if we only have one management CPU.
> >> >> >
> >> >> > Signed-off-by: Bin Meng
> >> >> > Reviewed-by: Alistair Francis
> >> >> >
> >> >> > ---
> >> >> >
> >> >> > Changes in v8: None
> >> >> > Changes in v7: None
> >> >> > Changes in v6: None
> >> >> > Changes in v5: None
> >> >> > Changes in v4: None
> >> >> > Changes in v3:
> >> >> > - use management cpu count + 1 for the min_cpus
> >> >> >
> >> >> > Changes in v2:
> >> >> > - update the file header to indicate at least 2 harts are created
> >> >> >
> >> >> > hw/riscv/sifive_u.c | 4 +++-
> >> >> > include/hw/riscv/sifive_u.h | 2 ++
> >> >> > 2 files changed, 5 insertions(+), 1 deletion(-)
> >> >> >
> >> >> > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> >> >> > index 2947e06..2023b71 100644
> >> >> > --- a/hw/riscv/sifive_u.c
> >> >> > +++ b/hw/riscv/sifive_u.c
> >> >> > @@ -10,7 +10,8 @@
> >> >> > * 1) CLINT (Core Level Interruptor)
> >> >> > * 2) PLIC (Platform Level Interrupt Controller)
> >> >> > *
> >> >> > - * This board currently uses a hardcoded devicetree that
> indicates one hart.
> >> >> > + * This board currently generates devicetree dynamically that
> indicates at least
> >> >> > + * two harts.
> >> >> > *
> >> >> > * This program is free software; you can redistribute it and/or
> modify it
> >> >> > * under the terms and conditions of the GNU General Public
> License,
> >> >> > @@ -433,6 +434,7 @@ static void
> riscv_sifive_u_machine_init(MachineClass *mc)
> >> >> > * management CPU.
> >> >> > */
> >> >> > mc->max_cpus = 4;
> >> >> > +mc->min_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + 1;
> >> >> > }
> >> >> >
> >> >> > DEFINE_MACHINE("sifive_u", riscv_sifive_u_machine_init)
> >> >> > diff --git a/include/hw/riscv/sifive_u.h
> b/include/hw/riscv/sifive_u.h
> >> >> > index f25bad8..6d22741 100644
> >> >> > --- a/include/hw/riscv/sifive_u.h
> >> >> > +++ b/include/hw/riscv/sifive_u.h
> >> >> > @@ -69,6 +69,8 @@ enum {
> >> >> > SIFIVE_U_GEM_CLOCK_FREQ = 12500
> >> >> > };
> >> >> >
> >> >> > +#define SIFIVE_U_MANAGEMENT_CPU_COUNT 1
> >> >> > +
> >> >> > #define SIFIVE_U_PLIC_HART_CONFIG "MS"
> >> >> > #define SIFIVE_U_PLIC_NUM_SOURCES 54
> >> >> > #define SIFIVE_U_PLIC_NUM_PRIORITIES 7
> >> >>
> >> >> This fails "make check", so I'm going to squash in this
> >> >>
> >> >> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> >> >> index ca9f7fea41..adecbf1dd9 100644
> >> >> --- a/hw/riscv/sifive_u.c
> >> >> +++ b/hw/riscv/sifive_u.c
> >> >> @@ -528,6 +528,7 @@ static void
> riscv_sifive_u_machine_init(MachineClass *mc)
> >> >> mc->init = riscv_sifive_u_init;
> >> >> mc->max_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT +
> SIFIVE_U_COMPUTE_CPU_COUNT;
> >> >> mc->min_cpus = SIFIVE_U_MANAGEMENT_CPU_COUNT + 1;
> >> >> +mc->default_cpus = mc->max_cpus;
> >> >
> >> > Thank you for fixing the 'make check'. Shouldn't it be:
> >> >
> >> > mc->default_cpus = mc->min_cpus;
> >>
> >> We have 5 harts on the board that this matches, so I figured that'd be
> the
> >> better default.
> >>
> >
> > Per my understanding mc->default_cpus is used when invoking QEMU
> > without passing '-smp n' (that's what 'make check' uses), and with the
> > updated sifive_u machine, '-smp 2' is the actual useful configuration
> > to boot Linux. For consistency with user experience on other machines,
> > without '-smp' means we want a uni-processor machine hence I would
> > suggest we set "mc->default_cpus = mc->min_cpus".
>
> OK, I've spun a v3. I never sent out v2 but I had tagged it, unless
> there's
> any opposition I'll send this out when I'm back on normal internet.
>
>
Re: [Qemu-devel] [Qemu-riscv] [PATCH v1 10/28] target/riscv: Convert mie and mstatus to pointers
Version 0.4 of the hypervisor spec no longer talks about swapping
registers. Instead when running in VS-mode some of the supervisor registers
are "aliased" and actually refer to alternate versions. Implementations are
of course still allowed to do swapping internally if desired, but it adds
complexity compared to a more straightforward implementation and isn't
obvious to me whether QEMU would get any benefit from it. At least, it is
probably worth fleshing out the rest of the v0.4 implementation before
deciding on this patch.
Jonathan
On Wed, Sep 11, 2019 at 4:24 AM Palmer Dabbelt wrote:
> On Fri, 23 Aug 2019 16:38:15 PDT (-0700), Alistair Francis wrote:
> > To handle the new Hypervisor CSR register swapping let's use pointers.
> >
> > We only need to convert the MIE and MSTATUS CSRs. With the exception of
> > MIP all of the other CSRs that swap with virtulsation changes are S-Mode
> > only, so we can just do a lazy switch. This because more challenging for
> > the M-Mode registers so it ends up being easier to use pointers.
> >
> > As the MIP CSR is always accessed atomicly the pointer swap doesn't work
> > so we leave that as is.
> >
> > Signed-off-by: Alistair Francis
> > ---
> > target/riscv/cpu.c| 16
> > target/riscv/cpu.h| 12 ++--
> > target/riscv/cpu_helper.c | 32
> > target/riscv/csr.c| 28 ++--
> > target/riscv/op_helper.c | 14 +++---
> > 5 files changed, 59 insertions(+), 43 deletions(-)
> >
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index be8f643fc2..371d5845af 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -228,7 +228,7 @@ static void riscv_cpu_dump_state(CPUState *cs, FILE
> *f, int flags)
> > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "pc ", env->pc);
> > #ifndef CONFIG_USER_ONLY
> > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mhartid ",
> env->mhartid);
> > -qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ",
> env->mstatus);
> > +qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mstatus ",
> *env->mstatus);
> > if (riscv_has_ext(env, RVH)) {
> > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "hstatus ",
> env->hstatus);
> > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "bstatus ",
> env->vsstatus);
> > @@ -239,7 +239,7 @@ static void riscv_cpu_dump_state(CPUState *cs, FILE
> *f, int flags)
> > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "vsip",
> > (target_ulong)atomic_read(>vsip));
> > }
> > -qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mie ", env->mie);
> > +qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "mie ", *env->mie);
> > if (riscv_has_ext(env, RVH)) {
> > qemu_fprintf(f, " %s " TARGET_FMT_lx "\n", "vsie",
> env->vsie);
> > }
> > @@ -309,7 +309,7 @@ static bool riscv_cpu_has_work(CPUState *cs)
> > * Definition of the WFI instruction requires it to ignore the
> privilege
> > * mode and delegation registers, but respect individual enables
> > */
> > -return (atomic_read(>mip) & env->mie) != 0;
> > +return (atomic_read(>mip) & *env->mie) != 0;
> > #else
> > return true;
> > #endif
> > @@ -330,7 +330,7 @@ static void riscv_cpu_reset(CPUState *cs)
> > mcc->parent_reset(cs);
> > #ifndef CONFIG_USER_ONLY
> > env->priv = PRV_M;
> > -env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
> > +*env->mstatus &= ~(MSTATUS_MIE | MSTATUS_MPRV);
> > env->mcause = 0;
> > env->pc = env->resetvec;
> > #endif
> > @@ -459,8 +459,16 @@ static void riscv_cpu_realize(DeviceState *dev,
> Error **errp)
> > static void riscv_cpu_init(Object *obj)
> > {
> > RISCVCPU *cpu = RISCV_CPU(obj);
> > +#ifndef CONFIG_USER_ONLY
> > +CPURISCVState *env = >env;
> > +#endif
> >
> > cpu_set_cpustate_pointers(cpu);
> > +
> > +#ifndef CONFIG_USER_ONLY
> > +env->mie = >mie_novirt;
> > +env->mstatus = >mstatus_novirt;
> > +#endif
> > }
> >
> > static const VMStateDescription vmstate_riscv_cpu = {
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 4c342e7a79..680592cb60 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -122,7 +122,7 @@ struct CPURISCVState {
> > target_ulong resetvec;
> >
> > target_ulong mhartid;
> > -target_ulong mstatus;
> > +target_ulong *mstatus;
> >
> > /*
> > * CAUTION! Unlike the rest of this struct, mip is accessed
> asynchonously
> > @@ -136,7 +136,7 @@ struct CPURISCVState {
> > uint32_t mip;
> > uint32_t miclaim;
> >
> > -target_ulong mie;
> > +target_ulong *mie;
> > target_ulong mideleg;
> >
> > target_ulong sptbr; /* until: priv-1.9.1 */
> > @@ -154,6 +154,14 @@ struct CPURISCVState {
> > target_ulong mcause;
> > target_ulong mtval; /* since: priv-1.10.0 */
> >
> > +/* The following registers are the "real" versions that the pointer
>
Re: [Qemu-devel] [Qemu-riscv] RISC-V: Vector && DSP Extension
Is there a reason to guarantee support of a particular draft extension version once it has been superseded by a subsequent version? I understand why it was done for priv-1.9.1, but going forward I'm skeptical there will be much/any code out in the wild that depends on old draft versions of extensions. The main reason people seem interested in implementing extensions in QEMU is to test them before going through the trouble of manufacturing hardware, and I don't really see why anyone would want to test a design that is no longer under consideration. Jonathan On Wed, Aug 21, 2019 at 3:31 PM Palmer Dabbelt wrote: > On Thu, 15 Aug 2019 14:37:52 PDT (-0700), alistai...@gmail.com wrote: > > On Thu, Aug 15, 2019 at 2:07 AM Peter Maydell > wrote: > >> > >> On Thu, 15 Aug 2019 at 09:53, Aleksandar Markovic > >> wrote: > >> > > >> > > We can accept draft > >> > > extensions in QEMU as long as they are disabled by default. > >> > >> > Hi, Alistair, Palmer, > >> > > >> > Is this an official stance of QEMU community, or perhaps Alistair's > >> > personal judgement, or maybe a rule within risv subcomunity? > >> > >> Alistair asked on a previous thread; my view was: > >> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg03364.html > >> and nobody else spoke up disagreeing (summary: should at least be > >> disabled-by-default and only enabled by setting an explicit > >> property whose name should start with the 'x-' prefix). > > > > Agreed! > > > >> > >> In general QEMU does sometimes introduce experimental extensions > >> (we've had them in the block layer, for example) and so the 'x-' > >> property to enable them is a reasonably established convention. > >> I think it's a reasonable compromise to allow this sort of work > >> to start and not have to live out-of-tree for a long time, without > >> confusing users or getting into a situation where some QEMU > >> versions behave differently or to obsolete drafts of a spec > >> without it being clear from the command line that experimental > >> extensions are being enabled. > >> > >> There is also an element of "submaintainer judgement" to be applied > >> here -- upstream is probably not the place for a draft extension > >> to be implemented if it is: > >> * still fast moving or subject to major changes of design direction > >> * major changes to the codebase (especially if it requires > >>changes to core code) that might later need to be redone > >>entirely differently > >> * still experimental > > > > Yep, agreed. For RISC-V I think this would extend to only allowing > > extensions that have backing from the foundation and are under active > > discussion. > > My general philosophy here is that we'll take anything written down in an > official RISC-V ISA manual (ie, the ones actually released by the > foundation). > This provides a single source of truth for what an extension name / > version > means, which is important to avoid confusion. If it's a ratified > extension > then I see no reason not to support it on my end. For frozen extensions > we > should probably just wait the 45 days until they go up for a ratification > vote, > but I'd be happy to start reviewing patches then (or earlier :)). > > If the spec is a draft in the ISA manual then we need to worry about the > support burden, which I don't have a fixed criteria for -- generally there > shouldn't be issues here, but early drafts can be in a state where they're > going to change extensively and are unlikely to be used by anyone. > There's > also the question of "what is an official release of a draft > specification?". > > That's a bit awkward right now: the current ratified ISA manual contains > version 0.3 of the hypervisor extension, but I just talked to Andrew and > the > plan is to remove the draft extensions from the ratified manuals because > these > drafts are old and the official manuals update slowly. For now I guess > we'll > need an an-hoc way of determining if a draft extension has been officially > versioned or not, which is a bit of a headache. > > We already have examples of supporting draft extensions, including > priv-1.9.1. > This does cause some pain for us on the QEMU side (CSR bits have different > semantics between the specs), but there's 1.9.1 hardware out there and the > port > continues to be useful so I'd be in favor of keeping it around for now. I > suppose there is an implicit risk that draft extensions will be > deprecated, but > the "x-" prefix, draft status, and long deprecation period should be > sufficient > to inform users of the risk. I wouldn't be opposed to adding a "this is a > draft ISA" warning, but I feel like it might be a bit overkill. > > > > > Alistair > > > >> > >> thanks > >> -- PMM > >
Re: [Qemu-devel] [PATCH v2] target/riscv: Hardwire mcounter.TM and upper bits of [m|s]counteren
Ping! What is the status of this patch? On Wed, Jul 3, 2019 at 2:02 PM Jonathan Behrens wrote: > Bin, that proposal proved to be somewhat more controversial than I was > expecting, since it was different than how currently available hardware > worked. This option seemed much more likely to be accepted in the short > term. > > Jonathan > > On Mon, Jul 1, 2019 at 9:26 PM Bin Meng wrote: > >> On Tue, Jul 2, 2019 at 8:20 AM Alistair Francis >> wrote: >> > >> > On Mon, Jul 1, 2019 at 8:56 AM wrote: >> > > >> > > From: Jonathan Behrens >> > > >> > > QEMU currently always triggers an illegal instruction exception when >> > > code attempts to read the time CSR. This is valid behavor, but only if >> > > the TM bit in mcounteren is hardwired to zero. This change also >> > > corrects mcounteren and scounteren CSRs to be 32-bits on both 32-bit >> > > and 64-bit targets. >> > > >> > > Signed-off-by: Jonathan Behrens >> > >> > Reviewed-by: Alistair Francis >> > >> >> I am a little bit lost here. I think we agreed to allow directly read >> to time CSR when mcounteren.TM is set, no? >> >> Regards, >> Bin >> >
Re: [Qemu-devel] [Qemu-riscv] [PATCH 26/28] riscv: hw: Update PLIC device tree
I was a little surprised to see the "riscv,max-priority" element removed, but there is no mention of it in the kernel documentation <https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/interrupt-controller/sifive%2Cplic-1.0.0.txt> so I guess that max-priority=7 is now assumed. Reviewed-by: Jonathan Behrens On Mon, Aug 5, 2019 at 12:10 PM Bin Meng wrote: > This removes "reg-names" and "riscv,max-priority" properties of the > PLIC node from device tree, and updates its compatible string, to > keep in sync with the Linux kernel device tree. > > Signed-off-by: Bin Meng > --- > > hw/riscv/sifive_u.c | 4 +--- > hw/riscv/virt.c | 4 +--- > 2 files changed, 2 insertions(+), 6 deletions(-) > > diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c > index d77b3c3..5ded3a0 100644 > --- a/hw/riscv/sifive_u.c > +++ b/hw/riscv/sifive_u.c > @@ -229,15 +229,13 @@ static void create_fdt(SiFiveUState *s, const struct > MemmapEntry *memmap, > (long)memmap[SIFIVE_U_PLIC].base); > qemu_fdt_add_subnode(fdt, nodename); > qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 1); > -qemu_fdt_setprop_string(fdt, nodename, "compatible", "riscv,plic0"); > +qemu_fdt_setprop_string(fdt, nodename, "compatible", > "sifive,plic-1.0.0"); > qemu_fdt_setprop(fdt, nodename, "interrupt-controller", NULL, 0); > qemu_fdt_setprop(fdt, nodename, "interrupts-extended", > cells, s->soc.cpus.num_harts * sizeof(uint32_t) * 4); > qemu_fdt_setprop_cells(fdt, nodename, "reg", > 0x0, memmap[SIFIVE_U_PLIC].base, > 0x0, memmap[SIFIVE_U_PLIC].size); > -qemu_fdt_setprop_string(fdt, nodename, "reg-names", "control"); > -qemu_fdt_setprop_cell(fdt, nodename, "riscv,max-priority", 7); > qemu_fdt_setprop_cell(fdt, nodename, "riscv,ndev", 0x35); > qemu_fdt_setprop_cell(fdt, nodename, "phandle", plic_phandle); > plic_phandle = qemu_fdt_get_phandle(fdt, nodename); > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c > index 127f005..f662100 100644 > --- a/hw/riscv/virt.c > +++ b/hw/riscv/virt.c > @@ -237,15 +237,13 @@ static void *create_fdt(RISCVVirtState *s, const > struct MemmapEntry *memmap, >FDT_PLIC_ADDR_CELLS); > qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", >FDT_PLIC_INT_CELLS); > -qemu_fdt_setprop_string(fdt, nodename, "compatible", "riscv,plic0"); > +qemu_fdt_setprop_string(fdt, nodename, "compatible", > "sifive,plic-1.0.0"); > qemu_fdt_setprop(fdt, nodename, "interrupt-controller", NULL, 0); > qemu_fdt_setprop(fdt, nodename, "interrupts-extended", > cells, s->soc.num_harts * sizeof(uint32_t) * 4); > qemu_fdt_setprop_cells(fdt, nodename, "reg", > 0x0, memmap[VIRT_PLIC].base, > 0x0, memmap[VIRT_PLIC].size); > -qemu_fdt_setprop_string(fdt, nodename, "reg-names", "control"); > -qemu_fdt_setprop_cell(fdt, nodename, "riscv,max-priority", 7); > qemu_fdt_setprop_cell(fdt, nodename, "riscv,ndev", VIRTIO_NDEV); > qemu_fdt_setprop_cell(fdt, nodename, "phandle", plic_phandle); > plic_phandle = qemu_fdt_get_phandle(fdt, nodename); > -- > 2.7.4 > > >
Re: [Qemu-devel] [Qemu-riscv] [PATCH 09/28] riscv: sifive_u: Update UART base addresses
Reviewed-by: Jonathan Behrens
On Mon, Aug 5, 2019 at 12:07 PM Bin Meng wrote:
> This updates the UART base address to match the hardware.
>
> Signed-off-by: Bin Meng
> ---
>
> hw/riscv/sifive_u.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index b235f29..9f05e09 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -60,8 +60,8 @@ static const struct MemmapEntry {
> [SIFIVE_U_MROM] = { 0x1000,0x11000 },
> [SIFIVE_U_CLINT] ={ 0x200,0x1 },
> [SIFIVE_U_PLIC] = { 0xc00, 0x400 },
> -[SIFIVE_U_UART0] ={ 0x10013000, 0x1000 },
> -[SIFIVE_U_UART1] ={ 0x10023000, 0x1000 },
> +[SIFIVE_U_UART0] ={ 0x1001, 0x1000 },
> +[SIFIVE_U_UART1] ={ 0x10011000, 0x1000 },
> [SIFIVE_U_DRAM] = { 0x8000,0x0 },
> [SIFIVE_U_GEM] = { 0x100900FC, 0x2000 },
> };
> --
> 2.7.4
>
>
>
Re: [Qemu-devel] [Qemu-riscv] [PATCH 07/28] riscv: sifive_u: Set the minimum number of cpus to 2
I'm not familiar with QEMU conventions on this, but would it make sense to
require having exactly 5 CPUs to match the real board?
Jonathan
On Mon, Aug 5, 2019 at 12:05 PM Bin Meng wrote:
> It is not useful if we only have one management CPU.
>
> Signed-off-by: Bin Meng
> ---
>
> hw/riscv/sifive_u.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/hw/riscv/sifive_u.c b/hw/riscv/sifive_u.c
> index 08d406f..206eccc 100644
> --- a/hw/riscv/sifive_u.c
> +++ b/hw/riscv/sifive_u.c
> @@ -428,6 +428,8 @@ static void riscv_sifive_u_machine_init(MachineClass
> *mc)
> * management CPU.
> */
> mc->max_cpus = 5;
> +/* It is not useful if we only have one management CPU */
> +mc->min_cpus = 2;
> }
>
> DEFINE_MACHINE("sifive_u", riscv_sifive_u_machine_init)
> --
> 2.7.4
>
>
>
Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W
The remaining checks are not sufficient. If you look at the bottom of
csr.c, you'll see that for most of the M-mode CSRs the predicate is set to
"any" which unconditionally allows access regardless of privilege mode. The
S-mode CSR predicates similarly only check that supervisor mode exists, but
not that the processor is current running in that mode. I do agree with you
that using the register address to determine the required privilege mode is
a little strange, but RISC-V is designed so that bits 8 and 9 of the CSR
address encode that information: 0=U-mode, 1=S-mode, 2=HS-mode, 3=M-mode.
I also tested by booting RVirt <https://github.com/mit-pdos/RVirt> with a
Linux guest and found that this patch caused it to instantly crash because
guest CSR accesses weren't intercepted.
Jonathan
On Fri, Jul 26, 2019 at 4:28 PM Alistair Francis
wrote:
> On Thu, Jul 25, 2019 at 2:48 PM Jonathan Behrens
> wrote:
> >
> > Unless I'm missing something, this is the only place that QEMU checks
> the privilege level for read and writes to CSRs. The exact computation used
> here won't work with the hypervisor extension, but we also can't just get
> rid of privilege checking entirely...
>
> The csr_ops struct contains a checker function, so there are still
> some checks occurring. I haven't done negative testing on this patch,
> but the current check doesn't seem to make any sense so it should be
> removed. We can separately discuss adding more checks but this current
> way base don CSR address just seems strange.
>
> Alistair
>
> >
> > Jonathan
> >
> > On Thu, Jul 25, 2019 at 2:56 PM Alistair Francis <
> alistair.fran...@wdc.com> wrote:
> >>
> >> The privledge check based on the CSR address mask 0x300 doesn't work
> >> when using Hypervisor extensions so remove the check
> >>
> >> Signed-off-by: Alistair Francis
> >> ---
> >> target/riscv/csr.c | 3 +--
> >> 1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> >> index e0d4586760..af3b762c8b 100644
> >> --- a/target/riscv/csr.c
> >> +++ b/target/riscv/csr.c
> >> @@ -797,9 +797,8 @@ int riscv_csrrw(CPURISCVState *env, int csrno,
> target_ulong *ret_value,
> >>
> >> /* check privileges and return -1 if check fails */
> >> #if !defined(CONFIG_USER_ONLY)
> >> -int csr_priv = get_field(csrno, 0x300);
> >> int read_only = get_field(csrno, 0xC00) == 3;
> >> -if ((write_mask && read_only) || (env->priv < csr_priv)) {
> >> +if (write_mask && read_only) {
> >> return -1;
> >> }
> >> #endif
> >> --
> >> 2.22.0
> >>
> >>
>
Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 3/6] riscv: plic: Remove unused interrupt functions
Reviewed-by: Jonathan Behrens
On Thu, Jul 25, 2019 at 2:56 PM Alistair Francis
wrote:
> Signed-off-by: Alistair Francis
> ---
> hw/riscv/sifive_plic.c | 12
> include/hw/riscv/sifive_plic.h | 3 ---
> 2 files changed, 15 deletions(-)
>
> diff --git a/hw/riscv/sifive_plic.c b/hw/riscv/sifive_plic.c
> index 0950e89e15..864a1bed42 100644
> --- a/hw/riscv/sifive_plic.c
> +++ b/hw/riscv/sifive_plic.c
> @@ -161,18 +161,6 @@ static void sifive_plic_update(SiFivePLICState *plic)
> }
> }
>
> -void sifive_plic_raise_irq(SiFivePLICState *plic, uint32_t irq)
> -{
> -sifive_plic_set_pending(plic, irq, true);
> -sifive_plic_update(plic);
> -}
> -
> -void sifive_plic_lower_irq(SiFivePLICState *plic, uint32_t irq)
> -{
> -sifive_plic_set_pending(plic, irq, false);
> -sifive_plic_update(plic);
> -}
> -
> static uint32_t sifive_plic_claim(SiFivePLICState *plic, uint32_t addrid)
> {
> int i, j;
> diff --git a/include/hw/riscv/sifive_plic.h
> b/include/hw/riscv/sifive_plic.h
> index ce8907f6aa..3b8a623919 100644
> --- a/include/hw/riscv/sifive_plic.h
> +++ b/include/hw/riscv/sifive_plic.h
> @@ -69,9 +69,6 @@ typedef struct SiFivePLICState {
> uint32_t aperture_size;
> } SiFivePLICState;
>
> -void sifive_plic_raise_irq(SiFivePLICState *plic, uint32_t irq);
> -void sifive_plic_lower_irq(SiFivePLICState *plic, uint32_t irq);
> -
> DeviceState *sifive_plic_create(hwaddr addr, char *hart_config,
> uint32_t num_sources, uint32_t num_priorities,
> uint32_t priority_base, uint32_t pending_base,
> --
> 2.22.0
>
>
>
Re: [Qemu-devel] [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W
Unless I'm missing something, this is the only place that QEMU checks the
privilege level for read and writes to CSRs. The exact computation used
here won't work with the hypervisor extension, but we also can't just get
rid of privilege checking entirely...
Jonathan
On Thu, Jul 25, 2019 at 2:56 PM Alistair Francis
wrote:
> The privledge check based on the CSR address mask 0x300 doesn't work
> when using Hypervisor extensions so remove the check
>
> Signed-off-by: Alistair Francis
> ---
> target/riscv/csr.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index e0d4586760..af3b762c8b 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -797,9 +797,8 @@ int riscv_csrrw(CPURISCVState *env, int csrno,
> target_ulong *ret_value,
>
> /* check privileges and return -1 if check fails */
> #if !defined(CONFIG_USER_ONLY)
> -int csr_priv = get_field(csrno, 0x300);
> int read_only = get_field(csrno, 0xC00) == 3;
> -if ((write_mask && read_only) || (env->priv < csr_priv)) {
> +if (write_mask && read_only) {
> return -1;
> }
> #endif
> --
> 2.22.0
>
>
>
[Qemu-devel] [PATCH] target/riscv: Disallow WFI instruction from U-mode
Signed-off-by: Jonathan Behrens
---
target/riscv/op_helper.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 331cc36232..2e5a980192 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -129,10 +129,10 @@ target_ulong helper_mret(CPURISCVState *env, target_ulong
cpu_pc_deb)
void helper_wfi(CPURISCVState *env)
{
CPUState *cs = env_cpu(env);
-
-if (env->priv == PRV_S &&
-env->priv_ver >= PRIV_VERSION_1_10_0 &&
-get_field(env->mstatus, MSTATUS_TW)) {
+if (!(env->priv >= PRV_S) ||
+(env->priv == PRV_S &&
+ env->priv_ver >= PRIV_VERSION_1_10_0 &&
+ get_field(env->mstatus, MSTATUS_TW))) {
riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
} else {
cs->halted = 1;
--
2.22.0
Re: [Qemu-devel] [PATCH v2] target/riscv: Hardwire mcounter.TM and upper bits of [m|s]counteren
Bin, that proposal proved to be somewhat more controversial than I was expecting, since it was different than how currently available hardware worked. This option seemed much more likely to be accepted in the short term. Jonathan On Mon, Jul 1, 2019 at 9:26 PM Bin Meng wrote: > On Tue, Jul 2, 2019 at 8:20 AM Alistair Francis > wrote: > > > > On Mon, Jul 1, 2019 at 8:56 AM wrote: > > > > > > From: Jonathan Behrens > > > > > > QEMU currently always triggers an illegal instruction exception when > > > code attempts to read the time CSR. This is valid behavor, but only if > > > the TM bit in mcounteren is hardwired to zero. This change also > > > corrects mcounteren and scounteren CSRs to be 32-bits on both 32-bit > > > and 64-bit targets. > > > > > > Signed-off-by: Jonathan Behrens > > > > Reviewed-by: Alistair Francis > > > > I am a little bit lost here. I think we agreed to allow directly read > to time CSR when mcounteren.TM is set, no? > > Regards, > Bin >
Re: [Qemu-devel] [PATCH] target/riscv: Hardwire mcounter.TM and upper bits of [m|s]counteren
> Can you wrap your commit message at ~70 lines?
Sure.
> Isn't CSR_TIME & 31 just 0? Can this just be changed 1 << 1 or even
better add a macro?
Any of those options work. Unless anyone feels strongly otherwise, I'll add
macros for the bits associated with the three named counters but not the
remaining 29 unnamed "high performance counters".
On Fri, Jun 28, 2019 at 5:03 PM Alistair Francis
wrote:
> On Fri, Jun 28, 2019 at 1:12 PM wrote:
> >
> > From: Jonathan Behrens
> >
> > QEMU currently always triggers an illegal instruction exception when code
> > attempts to read the time CSR. This is valid behavor, but only if the TM
> bit in
> > mcounteren is hardwired to zero. This change also corrects mcounteren
> and scounteren CSRs to be 32-bits on both
> > 32-bit and 64-bit targets.
>
> Thanks for the patch.
>
> Can you wrap your commit message at ~70 lines?
>
> >
> > Signed-off-by: Jonathan Behrens
> > ---
> > target/riscv/cpu.h | 4 ++--
> > target/riscv/csr.c | 3 ++-
> > 2 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 0adb307f32..2d0cbe9c78 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -151,8 +151,8 @@ struct CPURISCVState {
> > target_ulong mcause;
> > target_ulong mtval; /* since: priv-1.10.0 */
> >
> > -target_ulong scounteren;
> > -target_ulong mcounteren;
> > +uint32_t scounteren;
> > +uint32_t mcounteren;
> >
> > target_ulong sscratch;
> > target_ulong mscratch;
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index e0d4586760..89cf9734c3 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -473,7 +473,8 @@ static int write_mcounteren(CPURISCVState *env, int
> csrno, target_ulong val)
> > if (env->priv_ver < PRIV_VERSION_1_10_0) {
> > return -1;
> > }
> > -env->mcounteren = val;
> > +/* mcounteren.TM is hardwired to zero, all other bits are writable
> */
> > +env->mcounteren = val & ~(1 << (CSR_TIME & 31));
>
> Isn't CSR_TIME & 31 just 0? Can this just be changed 1 << 1 or even
> better add a macro?
>
> Otherwise this patch looks good :)
>
> Alistair
>
> > return 0;
> > }
> >
> > --
> > 2.22.0
> >
>
Re: [Qemu-devel] [PATCH for 4.1 v3] target/riscv: Expose time CSRs when allowed by [m|s]counteren
> Taking a patch that matches the out-of-spec FU540 behavior doesn't make any > sense, I don't want to implement errata in QEMU :) QEMU actually currently matches the out of spec behavior of the FU540, I just sent out a patch to fix this. Jonathan On Fri, Jun 28, 2019 at 2:26 PM Alistair Francis wrote: > On Thu, Jun 27, 2019 at 11:12 PM Palmer Dabbelt wrote: > > > > On Thu, 27 Jun 2019 12:56:57 PDT (-0700), alistai...@gmail.com wrote: > > > On Wed, Jun 26, 2019 at 1:25 AM Palmer Dabbelt > wrote: > > >> > > >> On Tue, 25 Jun 2019 23:58:34 PDT (-0700), bmeng...@gmail.com wrote: > > >> > On Wed, Jun 26, 2019 at 4:23 AM Jonathan Behrens < > finte...@gmail.com> wrote: > > >> >> > > >> >> I just did some testing on a HiFive Unleashed board and can > confirm what > > >> >> you are saying. The low 5 bits of both mcounteren and scounteren > are > > >> >> writable (if you try to write 0x to them, they'll take on > the value > > >> >> 0x1F) but even with the TM bit set in both mcounteren and > scounteren the > > >> >> rdtime instruction always generates an illegal instruction > exception. > > >> >> > > >> > > > >> > Then I would think the FU540 is not spec complaint :) > > >> > > >> Ya, it's an errata. There's a handful of them :) > > >> > > >> >> Reading through the relevant chapter of the spec, I still think > that having > > >> >> mcounteren.TM be writable but making rdtime unconditionally trap is > > >> >> non-conformant. If other people feel strongly that rdtime should > always > > >> > > > >> > Agree. To test hardware (FU540) compatibility in QEMU, maybe we can > > >> > add a cpu property to allow hard-wiring mcounteren.TM to zero? > > >> > > >> In theory we should have properties to control the behavior of all > WARL fields, > > >> but it's a lot of work. I'd be happy to take a patch for any of them. > > > > > > Hmmm... We should avoid taking patches that don't adhere to the spec > > > just to match some hardware. In the case that core/popular software > > > doesn't work it probably makes sense, but in general it's probably not > > > the best move. > > > > WARL is Write Any Read Legal. Essentially it means that the hardware > gets to > > decide what sort of behavior that field has, and is the mechanism for > optional > > features in the ISA. In this case it'd be entirely within spec, > specifically: > > > > Registers mcounteren and scounteren are WARL registers that must be > > implemented if U-mode and S-mode are implemented. Any of the bits may > > contain a hardwired value of zero, indicating reads to the > corresponding > > counter will cause an illegal instruction exception when executing > in a > > less-privileged mode. > > > > Taking a patch that matches the out-of-spec FU540 behavior doesn't make > any > > sense, I don't want to implement errata in QEMU :) > > Ah, I misread your email. That is fine then :) > > Alistair > > > > > > > > > Alistair > > > > > >> > > >> >> require trapping into firmware then the natural change would be to > simply > > >> >> hardwire mcounteren.TM to zero (the value in scounteren wouldn't > matter in > > >> >> that case so it could be left writable). My own (biased) personal > feeling > > >> >> is that this full implementation makes sense at least for the > `virt` > > >> >> machine type because it represents a clear case where deviating > from > > >> >> current hardware enables a performance boost, and would not break > > >> >> compatibility with any current software: both OpenSBI and BBL try > to enable > > >> >> hardware handling of rdtime when the platform claims to support it. > > >> >> > > >> > > > >> > Regards, > > >> > Bin > > >> >
Re: [Qemu-devel] [Qemu-riscv] [PULL 10/34] RISC-V: Fix a PMP check with the correct access size
I think this patch is slightly incorrect. If the PMP region is valid for
the size of the access, but not the rest of the page then a few lines down
in this function the entire page should not be placed into the TLB. Instead
only the portion of the page that passed the access check should be
included. To give an example of where this goes wrong, in the code below
access to address 0x9008 should always fail due to PMP rules, but if
the TLB has already been primed by loading the (allowed) address 0x9000
then no fault will be triggered. Notably, this code also executes
improperly without the patch because the first `ld` instruction traps when
it shouldn't.
li t0, 0x2400 // region[0]: 0x9000..0x9007
csrw pmpaddr0, t0
li t0, 0x240001FF // region[1]: 0x9000..0x9fff
csrw pmpaddr1, t0
li t0, 0x1F00989F // cfg[0] = LXRW, cfg[1] = L
csrw pmpcfg0, t0
sfence.vma
li t0, 0x9000
ld s0, 0(t0)
ld s1, 8(t0) // NO TRAP: address is incorrectly in TLB!
sfence.vma
ld s1, 8(t0) // TRAP: TLB has been flushed!
I think that the proper fix would be to first do a PMP check for the full
PAGE_SIZE and execute normally if it passes. Then in the event the full
page fails, there could be a more granular PMP check with only the accessed
region inserted as an entry in the TLB.
Unrelated question: should I be sending "Reviewed By" lines if I read
through patches that seem reasonable? Or there some formal process I'd have
to go through first to be approved?
Jonathan
On Thu, Jun 27, 2019 at 11:43 AM Palmer Dabbelt wrote:
> From: Hesham Almatary
>
> The PMP check should be of the memory access size rather
> than TARGET_PAGE_SIZE.
>
> Signed-off-by: Hesham Almatary
> Reviewed-by: Alistair Francis
> Signed-off-by: Palmer Dabbelt
> ---
> target/riscv/cpu_helper.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 66be83210f11..e1b079e69c60 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -452,8 +452,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address,
> int size,
>
> if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> (ret == TRANSLATE_SUCCESS) &&
> -!pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type,
> -mode)) {
> +!pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
> ret = TRANSLATE_PMP_FAIL;
> }
> if (ret == TRANSLATE_PMP_FAIL) {
> --
> 2.21.0
>
>
>
Re: [Qemu-devel] [PATCH for 4.1 v3] target/riscv: Expose time CSRs when allowed by [m|s]counteren
I just did some testing on a HiFive Unleashed board and can confirm what you are saying. The low 5 bits of both mcounteren and scounteren are writable (if you try to write 0x to them, they'll take on the value 0x1F) but even with the TM bit set in both mcounteren and scounteren the rdtime instruction always generates an illegal instruction exception. Reading through the relevant chapter of the spec, I still think that having mcounteren.TM be writable but making rdtime unconditionally trap is non-conformant. If other people feel strongly that rdtime should always require trapping into firmware then the natural change would be to simply hardwire mcounteren.TM to zero (the value in scounteren wouldn't matter in that case so it could be left writable). My own (biased) personal feeling is that this full implementation makes sense at least for the `virt` machine type because it represents a clear case where deviating from current hardware enables a performance boost, and would not break compatibility with any current software: both OpenSBI and BBL try to enable hardware handling of rdtime when the platform claims to support it. On Tue, Jun 25, 2019 at 5:56 AM Palmer Dabbelt wrote: > On Mon, 24 Jun 2019 16:03:20 PDT (-0700), finte...@gmail.com wrote: > > Apparently my previous message didn't make it out onto the list (sorry > > about all these email glitches!). I've included the message again below. > > Hopefully either a patch like this one or something simpler that just > hard > > codes mcounteren.TM to zero (so QEMU is at least conformant) can be > merged > > in time for 4.1. > > > > On Fri, Jun 14, 2019 at 8:55 AM Jonathan Behrens > > wrote: > > > >> I'm not sure that is accurate. Based on the discussion here > >> <https://forums.sifive.com/t/possible-bug-in-counteren-csrs/2123> the > >> HiFive Unleashed actually does support reading the timer CSR from > >> unprivileged modes (from that discussion it does so a little too well... > >> but it should presumably be fixed in later iterations of the processor). > >> And even if no real hardware supported this capability, it still might > make > >> sense to provide it in QEMU as an optimization. > > time and cycle are different registers: rdtime should trap, but rdcycle > should > work. The hardware traps rdtime into machine mode and emulates it via a > memory > mapped register. The bug in the FU540-C000 appears to be related to the > counter enables, not the presence of the counters. > > >> > >> On Fri, Jun 14, 2019 at 7:52 AM Palmer Dabbelt > wrote: > >> > >>> On Tue, 28 May 2019 11:30:20 PDT (-0700), jonat...@fintelia.io wrote: > >>> > Currently mcounteren.TM acts as though it is hardwired to zero, even > >>> though QEMU allows it to be set. This change resolves the issue by > allowing > >>> reads to the time and timeh control registers when running in a > privileged > >>> mode where such accesses are allowed. > >>> > > >>> > The frequency of the time register is stored in the time_freq field > of > >>> each hart so that it is accessible during CSR reads, but must be the > same > >>> across all harts. Each board can initialize it to a custom value, > although > >>> all currently use a 10 MHz frequency. > >>> > > >>> > Signed-off-by: Jonathan Behrens > >>> > --- > >>> > hw/riscv/riscv_hart.c | 4 > >>> > hw/riscv/sifive_clint.c | 30 ++ > >>> > hw/riscv/sifive_e.c | 2 ++ > >>> > hw/riscv/sifive_u.c | 4 +++- > >>> > hw/riscv/spike.c| 6 +- > >>> > hw/riscv/virt.c | 4 +++- > >>> > include/hw/riscv/riscv_hart.h | 1 + > >>> > include/hw/riscv/sifive_clint.h | 4 > >>> > include/hw/riscv/sifive_e.h | 4 > >>> > include/hw/riscv/sifive_u.h | 1 + > >>> > include/hw/riscv/spike.h| 1 + > >>> > include/hw/riscv/virt.h | 1 + > >>> > target/riscv/cpu.h | 2 ++ > >>> > target/riscv/csr.c | 17 +++-- > >>> > 14 files changed, 60 insertions(+), 21 deletions(-) > >>> > > >>> > diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c > >>> > index e34a26a0ef..c39cd55330 100644 > >>> > --- a/hw/riscv/riscv_hart.c > >>> > +++ b/hw/riscv/riscv_hart.c > >>> > @@ -19,6 +19,7 @
Re: [Qemu-devel] [PATCH for 4.1 v3] target/riscv: Expose time CSRs when allowed by [m|s]counteren
Apparently my previous message didn't make it out onto the list (sorry
about all these email glitches!). I've included the message again below.
Hopefully either a patch like this one or something simpler that just hard
codes mcounteren.TM to zero (so QEMU is at least conformant) can be merged
in time for 4.1.
On Fri, Jun 14, 2019 at 8:55 AM Jonathan Behrens
wrote:
> I'm not sure that is accurate. Based on the discussion here
> <https://forums.sifive.com/t/possible-bug-in-counteren-csrs/2123> the
> HiFive Unleashed actually does support reading the timer CSR from
> unprivileged modes (from that discussion it does so a little too well...
> but it should presumably be fixed in later iterations of the processor).
> And even if no real hardware supported this capability, it still might make
> sense to provide it in QEMU as an optimization.
>
> On Fri, Jun 14, 2019 at 7:52 AM Palmer Dabbelt wrote:
>
>> On Tue, 28 May 2019 11:30:20 PDT (-0700), jonat...@fintelia.io wrote:
>> > Currently mcounteren.TM acts as though it is hardwired to zero, even
>> though QEMU allows it to be set. This change resolves the issue by allowing
>> reads to the time and timeh control registers when running in a privileged
>> mode where such accesses are allowed.
>> >
>> > The frequency of the time register is stored in the time_freq field of
>> each hart so that it is accessible during CSR reads, but must be the same
>> across all harts. Each board can initialize it to a custom value, although
>> all currently use a 10 MHz frequency.
>> >
>> > Signed-off-by: Jonathan Behrens
>> > ---
>> > hw/riscv/riscv_hart.c | 4
>> > hw/riscv/sifive_clint.c | 30 ++
>> > hw/riscv/sifive_e.c | 2 ++
>> > hw/riscv/sifive_u.c | 4 +++-
>> > hw/riscv/spike.c| 6 +-
>> > hw/riscv/virt.c | 4 +++-
>> > include/hw/riscv/riscv_hart.h | 1 +
>> > include/hw/riscv/sifive_clint.h | 4
>> > include/hw/riscv/sifive_e.h | 4
>> > include/hw/riscv/sifive_u.h | 1 +
>> > include/hw/riscv/spike.h| 1 +
>> > include/hw/riscv/virt.h | 1 +
>> > target/riscv/cpu.h | 2 ++
>> > target/riscv/csr.c | 17 +++--
>> > 14 files changed, 60 insertions(+), 21 deletions(-)
>> >
>> > diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
>> > index e34a26a0ef..c39cd55330 100644
>> > --- a/hw/riscv/riscv_hart.c
>> > +++ b/hw/riscv/riscv_hart.c
>> > @@ -19,6 +19,7 @@
>> > */
>> >
>> > #include "qemu/osdep.h"
>> > +#include "qemu/timer.h"
>> > #include "qapi/error.h"
>> > #include "hw/sysbus.h"
>> > #include "target/riscv/cpu.h"
>> > @@ -27,6 +28,8 @@
>> > static Property riscv_harts_props[] = {
>> > DEFINE_PROP_UINT32("num-harts", RISCVHartArrayState, num_harts, 1),
>> > DEFINE_PROP_STRING("cpu-type", RISCVHartArrayState, cpu_type),
>> > +DEFINE_PROP_UINT64("timebase-frequency", RISCVHartArrayState,
>> time_freq,
>> > + NANOSECONDS_PER_SECOND),
>> > DEFINE_PROP_END_OF_LIST(),
>> > };
>> >
>> > @@ -49,6 +52,7 @@ static void riscv_harts_realize(DeviceState *dev,
>> Error **errp)
>> > sizeof(RISCVCPU), s->cpu_type,
>> > _abort, NULL);
>> > s->harts[n].env.mhartid = n;
>> > +s->harts[n].env.time_freq = s->time_freq;
>> > qemu_register_reset(riscv_harts_cpu_reset, >harts[n]);
>> > object_property_set_bool(OBJECT(>harts[n]), true,
>> > "realized", );
>> > diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
>> > index d4c159e937..71edf4dcc6 100644
>> > --- a/hw/riscv/sifive_clint.c
>> > +++ b/hw/riscv/sifive_clint.c
>> > @@ -26,10 +26,10 @@
>> > #include "hw/riscv/sifive_clint.h"
>> > #include "qemu/timer.h"
>> >
>> > -static uint64_t cpu_riscv_read_rtc(void)
>> > +static uint64_t cpu_riscv_read_rtc(CPURISCVState *env)
>> > {
>> > return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
>> > -SIFIVE_CLINT_TIMEBASE_FREQ, NANOSECONDS_PER_SECOND);
>> > +env->time_freq, NANOSECONDS_PER_
Re: [Qemu-devel] [Qemu-riscv] [RFC v1 2/5] hw/riscv: Add support for loading a firmware
I was actually just writing up the same thing. Shifting all the addresses
in the ELF file by 2 or 4MB is somewhat surprising behavior, especially
because this will apply to segments that are mapped even at much higher
addresses. If you want a segment aligned to a 1GB superpage boundary you
now need to place it slightly below so that it will be bumped up to the
right place. Further, ANDing all addresses with 0x7fff makes it
impossible to map anything beyond the first 2GB of RAM.
Jonathan
On Wed, Jun 19, 2019 at 11:16 AM Bin Meng wrote:
> On Wed, Jun 19, 2019 at 8:53 AM Alistair Francis
> wrote:
> >
> > Add support for loading a firmware file for the virt machine and the
> > SiFive U. This can be run with the following command:
> >
> > qemu-system-riscv64 -machine virt -bios fw_jump.elf -kernel vmlinux
> >
> > Signed-off-by: Alistair Francis
> > ---
> > hw/riscv/boot.c | 41 +++--
> > hw/riscv/sifive_e.c | 2 +-
> > hw/riscv/sifive_u.c | 6 +-
> > hw/riscv/spike.c| 6 +++---
> > hw/riscv/virt.c | 7 ++-
> > include/hw/riscv/boot.h | 4 +++-
> > 6 files changed, 57 insertions(+), 9 deletions(-)
> >
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index 62f94aaf8a..392ca0cb2e 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -23,13 +23,50 @@
> > #include "exec/cpu-defs.h"
> > #include "hw/loader.h"
> > #include "hw/riscv/boot.h"
> > +#include "hw/boards.h"
> > #include "elf.h"
> >
> > -target_ulong riscv_load_kernel(const char *kernel_filename)
> > +#if defined(TARGET_RISCV32)
> > +# define KERNEL_BOOT_ADDRESS 0x8040
> > +#else
> > +# define KERNEL_BOOT_ADDRESS 0x8020
> > +#endif
> > +
> > +static uint64_t kernel_translate(void *opaque, uint64_t addr)
> > +{
> > +MachineState *machine = opaque;
> > +
> > +/*
> > + * If the user specified a firmware move the kernel to the offset
> > + * start address.
> > + */
>
> Why?
>
> > +if (machine->firmware) {
> > +return (addr & 0x7fff) + KERNEL_BOOT_ADDRESS;
>
> So with both "-bios" and "-kernel", the kernel address will be moved
> to another address other than 0x8020 (for 64-bit). This does not
> look good to me.
>
> > +} else {
> > +return addr;
> > +}
> > +}
> > +
>
> [snip]
>
> Regards,
> Bin
>
>
Re: [Qemu-devel] [PATCH for-4.1 2/2] target/riscv: Add support for -bios "firmware_filename" flag
I've thought some more about this issue, and long term I think there are a couple different useful configurations: - For end users, having default firmware that loaded the OS from a block device would be easiest - Current invocation: impossible - Proposed: empty command line (i.e. pass neither -bios nor -kernel) - Custom firmware support would be good to test possible firmware improvements or if the default is missing something - Current invocation: -kernel firmware.elf - Proposed: -bios firmware.elf - A kernel developer may want to test a kernel binary without having to make a full disk image or bundle firmware (on x86 and perhaps other architectures this is done with the -kernel parameter, but for RISC-V that invocation currently is used to load M-mode code rather than supervisor code) - Current invocation: impossible - Proposed: -bios firmware.elf -kernel kernel.bin - Ideally `-kernel kernel.bin` be the same except using default firmware, but I don't know if QEMU would be willing to deprecate the current syntax to allow it For now, it is probably too early to add default firmware (but perhaps not?) which would leave only the firmware only and firmware + kernel variants. What do other people think about this? Jonathan On Mon, May 20, 2019 at 12:56 PM Alistair Francis wrote: > On Sat, May 18, 2019 at 2:57 PM Jonathan Behrens > wrote: > > > > > I've never been fully convinced of this, why not just use the generic > loader? > > > > If I understand you are proposing passing bbl (or other firmware) with > the -kernel flag, and then vmlinux (or another kernel) with the -initrd > flag? Wouldn't this result in losing the ability to pass a real init > ramdisk to Linux? It also seems to open the possibility for strange > bugs/compatibility issues later if firmware starts recognizing any "initrd" > entries in the device tree as kernel code to jump into. > > No I mean passing in OpenSBI (or some other boot loader) via the > -kernel option and then passing in the kernel with QEMU's generic > device loader. This is documented as part of the OpenSBI boot flow: > https://github.com/riscv/opensbi/blob/master/docs/platform/qemu_virt.md > > The only disadvantage with that is that we don't get debug symbols > from the kernel, but it does mean that the boot loader in QEMU is much > simpler. > > > > > I do wonder though how compatible the current design is with providing > default firmware for riscv in the future. > > > > > This should be in a generic boot.c file and support added to all > RISC-V boards. > > > > I can do this for v2. > > Thanks > > Alistair > > > > > Jonathan >
Re: [Qemu-devel] [PATCH for 4.1 v2] target/riscv: Expose time CSRs when allowed by [m|s]counteren
Sure, I've just sent a corrected version.
Thanks,
Jonathan
On Tue, May 28, 2019 at 1:09 PM Palmer Dabbelt wrote:
> On Tue, 30 Apr 2019 10:36:01 PDT (-0700), finte...@gmail.com wrote:
> > Currently mcounteren.TM acts as though it is hardwired to zero, even
> though
> > QEMU allows it to be set. This change resolves the issue by allowing
> reads
> > to the time and timeh control registers when running in a privileged mode
> > where such accesses are allowed.
> >
> > The frequency of the time register is stored in the time_freq field of
> each
> > hart so that it is accessible during CSR reads, but must be the same
> across
> > all harts. Each board can initialize it to a custom value, although all
> > currently use a 10 MHz frequency.
> > ---
> > hw/riscv/riscv_hart.c | 4
> > hw/riscv/sifive_clint.c | 30 ++
> > hw/riscv/sifive_e.c | 2 ++
> > hw/riscv/sifive_u.c | 4 +++-
> > hw/riscv/spike.c| 6 +-
> > hw/riscv/virt.c | 4 +++-
> > include/hw/riscv/riscv_hart.h | 1 +
> > include/hw/riscv/sifive_clint.h | 4
> > include/hw/riscv/sifive_e.h | 4
> > include/hw/riscv/sifive_u.h | 1 +
> > include/hw/riscv/spike.h| 1 +
> > include/hw/riscv/virt.h | 1 +
> > target/riscv/cpu.h | 2 ++
> > target/riscv/csr.c | 17 +++--
> > 14 files changed, 60 insertions(+), 21 deletions(-)
> >
> > diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
> > index e34a26a0ef..c39cd55330 100644
> > --- a/hw/riscv/riscv_hart.c
> > +++ b/hw/riscv/riscv_hart.c
> > @@ -19,6 +19,7 @@
> > */
> >
> > #include "qemu/osdep.h"
> > +#include "qemu/timer.h"
> > #include "qapi/error.h"
> > #include "hw/sysbus.h"
> > #include "target/riscv/cpu.h"
> > @@ -27,6 +28,8 @@
> > static Property riscv_harts_props[] = {
> > DEFINE_PROP_UINT32("num-harts", RISCVHartArrayState, num_harts, 1),
> > DEFINE_PROP_STRING("cpu-type", RISCVHartArrayState, cpu_type),
> > +DEFINE_PROP_UINT64("timebase-frequency", RISCVHartArrayState,
> > time_freq,
> > + NANOSECONDS_PER_SECOND),
> > DEFINE_PROP_END_OF_LIST(),
> > };
> >
> > @@ -49,6 +52,7 @@ static void riscv_harts_realize(DeviceState *dev, Error
> > **errp)
> > sizeof(RISCVCPU), s->cpu_type,
> > _abort, NULL);
> > s->harts[n].env.mhartid = n;
> > +s->harts[n].env.time_freq = s->time_freq;
> > qemu_register_reset(riscv_harts_cpu_reset, >harts[n]);
> > object_property_set_bool(OBJECT(>harts[n]), true,
> > "realized", );
> > diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
> > index d4c159e937..71edf4dcc6 100644
> > --- a/hw/riscv/sifive_clint.c
> > +++ b/hw/riscv/sifive_clint.c
> > @@ -26,10 +26,10 @@
> > #include "hw/riscv/sifive_clint.h"
> > #include "qemu/timer.h"
> >
> > -static uint64_t cpu_riscv_read_rtc(void)
> > +static uint64_t cpu_riscv_read_rtc(CPURISCVState *env)
> > {
> > return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> > -SIFIVE_CLINT_TIMEBASE_FREQ, NANOSECONDS_PER_SECOND);
> > +env->time_freq, NANOSECONDS_PER_SECOND);
> > }
> >
> > /*
> > @@ -41,7 +41,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
> > uint64_t value)
> > uint64_t next;
> > uint64_t diff;
> >
> > -uint64_t rtc_r = cpu_riscv_read_rtc();
> > +uint64_t rtc_r = cpu_riscv_read_rtc(>env);
> >
> > cpu->env.timecmp = value;
> > if (cpu->env.timecmp <= rtc_r) {
> > @@ -56,7 +56,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
> > uint64_t value)
> > diff = cpu->env.timecmp - rtc_r;
> > /* back to ns (note args switched in muldiv64) */
> > next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> > -muldiv64(diff, NANOSECONDS_PER_SECOND,
> SIFIVE_CLINT_TIMEBASE_FREQ);
> > +muldiv64(diff, NANOSECONDS_PER_SECOND, cpu->env.time_freq);
> > timer_mod(cpu->env.timer, next);
> > }
> >
> > @@ -107,11 +107,25 @@ static uint64_t sifive_clint_read(void *opaque,
> > hwaddr addr, unsigned size)
> > return 0;
> > }
> > } else if (addr == clint->time_base) {
> > -/* time_lo */
> > -return cpu_riscv_read_rtc() & 0x;
> > +/* All harts must have the same time frequency, so just use
> hart 0
> > */
> > +CPUState *cpu = qemu_get_cpu(0);
> > +CPURISCVState *env = cpu ? cpu->env_ptr : NULL;
> > +if (!env) {
> > +error_report("clint: hart 0 not valid?!");
> > +} else {
> > +/* time_lo */
> > +return cpu_riscv_read_rtc(env) & 0x;
> > +}
> > } else if (addr == clint->time_base + 4) {
> > -/* time_hi */
> > -return (cpu_riscv_read_rtc() >> 32) & 0x;
> > +/* All harts must have the
[Qemu-devel] [PATCH for 4.1 v3] target/riscv: Expose time CSRs when allowed by [m|s]counteren
Currently mcounteren.TM acts as though it is hardwired to zero, even though
QEMU allows it to be set. This change resolves the issue by allowing reads to
the time and timeh control registers when running in a privileged mode where
such accesses are allowed.
The frequency of the time register is stored in the time_freq field of each
hart so that it is accessible during CSR reads, but must be the same across all
harts. Each board can initialize it to a custom value, although all currently
use a 10 MHz frequency.
Signed-off-by: Jonathan Behrens
---
hw/riscv/riscv_hart.c | 4
hw/riscv/sifive_clint.c | 30 ++
hw/riscv/sifive_e.c | 2 ++
hw/riscv/sifive_u.c | 4 +++-
hw/riscv/spike.c| 6 +-
hw/riscv/virt.c | 4 +++-
include/hw/riscv/riscv_hart.h | 1 +
include/hw/riscv/sifive_clint.h | 4
include/hw/riscv/sifive_e.h | 4
include/hw/riscv/sifive_u.h | 1 +
include/hw/riscv/spike.h| 1 +
include/hw/riscv/virt.h | 1 +
target/riscv/cpu.h | 2 ++
target/riscv/csr.c | 17 +++--
14 files changed, 60 insertions(+), 21 deletions(-)
diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
index e34a26a0ef..c39cd55330 100644
--- a/hw/riscv/riscv_hart.c
+++ b/hw/riscv/riscv_hart.c
@@ -19,6 +19,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/timer.h"
#include "qapi/error.h"
#include "hw/sysbus.h"
#include "target/riscv/cpu.h"
@@ -27,6 +28,8 @@
static Property riscv_harts_props[] = {
DEFINE_PROP_UINT32("num-harts", RISCVHartArrayState, num_harts, 1),
DEFINE_PROP_STRING("cpu-type", RISCVHartArrayState, cpu_type),
+DEFINE_PROP_UINT64("timebase-frequency", RISCVHartArrayState, time_freq,
+ NANOSECONDS_PER_SECOND),
DEFINE_PROP_END_OF_LIST(),
};
@@ -49,6 +52,7 @@ static void riscv_harts_realize(DeviceState *dev, Error
**errp)
sizeof(RISCVCPU), s->cpu_type,
_abort, NULL);
s->harts[n].env.mhartid = n;
+s->harts[n].env.time_freq = s->time_freq;
qemu_register_reset(riscv_harts_cpu_reset, >harts[n]);
object_property_set_bool(OBJECT(>harts[n]), true,
"realized", );
diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
index d4c159e937..71edf4dcc6 100644
--- a/hw/riscv/sifive_clint.c
+++ b/hw/riscv/sifive_clint.c
@@ -26,10 +26,10 @@
#include "hw/riscv/sifive_clint.h"
#include "qemu/timer.h"
-static uint64_t cpu_riscv_read_rtc(void)
+static uint64_t cpu_riscv_read_rtc(CPURISCVState *env)
{
return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-SIFIVE_CLINT_TIMEBASE_FREQ, NANOSECONDS_PER_SECOND);
+env->time_freq, NANOSECONDS_PER_SECOND);
}
/*
@@ -41,7 +41,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
uint64_t value)
uint64_t next;
uint64_t diff;
-uint64_t rtc_r = cpu_riscv_read_rtc();
+uint64_t rtc_r = cpu_riscv_read_rtc(>env);
cpu->env.timecmp = value;
if (cpu->env.timecmp <= rtc_r) {
@@ -56,7 +56,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
uint64_t value)
diff = cpu->env.timecmp - rtc_r;
/* back to ns (note args switched in muldiv64) */
next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-muldiv64(diff, NANOSECONDS_PER_SECOND, SIFIVE_CLINT_TIMEBASE_FREQ);
+muldiv64(diff, NANOSECONDS_PER_SECOND, cpu->env.time_freq);
timer_mod(cpu->env.timer, next);
}
@@ -107,11 +107,25 @@ static uint64_t sifive_clint_read(void *opaque, hwaddr
addr, unsigned size)
return 0;
}
} else if (addr == clint->time_base) {
-/* time_lo */
-return cpu_riscv_read_rtc() & 0x;
+/* All harts must have the same time frequency, so just use hart 0 */
+CPUState *cpu = qemu_get_cpu(0);
+CPURISCVState *env = cpu ? cpu->env_ptr : NULL;
+if (!env) {
+error_report("clint: hart 0 not valid?!");
+} else {
+/* time_lo */
+return cpu_riscv_read_rtc(env) & 0x;
+}
} else if (addr == clint->time_base + 4) {
-/* time_hi */
-return (cpu_riscv_read_rtc() >> 32) & 0x;
+/* All harts must have the same time frequency, so just use hart 0 */
+CPUState *cpu = qemu_get_cpu(0);
+CPURISCVState *env = cpu ? cpu->env_ptr : NULL;
+if (!env) {
+error_report("clint: hart 0 not valid?!");
+} else {
+/* time_hi */
+return (cpu_riscv_read_rtc(env) >> 32) & 0x;
+}
}
error_report("cl
Re: [Qemu-devel] [Qemu-riscv] [PATCHv3 5/5] RISC-V: Fix a PMP check with the correct access size
Hesham,
I don't think this is quite right. If I understand correctly, PMP
permissions are only validated on TLB fills, not on all accesses. (Is
anyone able to confirm this?) If so, this function can't just validate the
range of a single access and then place the entire page into the TLB.
However, the current code is also wrong because an access should
succeed/fail based on the permissions only for the range it actually
touches even regardless of the permissions on the rest of the page. Now
that I think about it, I'd also expect that somewhere in the PMP logic
would flush the TLB every time any of the related control registers change
though I can't find anywhere that this is happening...
Sorry to keep raising complaints about this patch set, the interaction
between physical memory protection and paging is very subtle. Even some
real hardware has had errata related to it!
Jonathan
On Tue, May 21, 2019 at 6:33 PM Alistair Francis
wrote:
> On Tue, May 21, 2019 at 3:45 AM Hesham Almatary
> wrote:
> >
> > The PMP check should be of the memory access size rather
> > than TARGET_PAGE_SIZE.
> >
> > Signed-off-by: Hesham Almatary
>
> Reviewed-by: Alistair Francis
>
> Alistair
>
> > ---
> > target/riscv/cpu_helper.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index d0b0f9cf88..ce1f47e4e3 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -410,7 +410,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address,
> int size,
> >
> > if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> > (ret == TRANSLATE_SUCCESS) &&
> > -!pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 <<
> access_type)) {
> > +!pmp_hart_has_privs(env, pa, size, 1 << access_type)) {
> > ret = TRANSLATE_PMP_FAIL;
> > }
> > if (ret == TRANSLATE_PMP_FAIL) {
> > --
> > 2.17.1
> >
> >
>
>
Re: [Qemu-devel] [PATCH for-4.1 2/2] target/riscv: Add support for -bios "firmware_filename" flag
> I've never been fully convinced of this, why not just use the generic loader? If I understand you are proposing passing bbl (or other firmware) with the -kernel flag, and then vmlinux (or another kernel) with the -initrd flag? Wouldn't this result in losing the ability to pass a real init ramdisk to Linux? It also seems to open the possibility for strange bugs/compatibility issues later if firmware starts recognizing any "initrd" entries in the device tree as kernel code to jump into. I do wonder though how compatible the current design is with providing default firmware for riscv in the future. > This should be in a generic boot.c file and support added to all RISC-V boards. I can do this for v2. Jonathan
Re: [Qemu-devel] [Qemu-riscv] [PATCH 1/2] RISC-V: Raise access fault exceptions on PMP violations
This patch assumes that translation failure should always raise a paging
fault, but it should be possible for it to raise an access fault as well
(since according to the spec "PMP checks are also applied to
page-table accesses for virtual-address translation, for which the
effective privilege mode is S."). I think the code to actually do the PMP
checking during page table walks is currently unimplemented though...
Jonathan
On Sat, May 18, 2019 at 3:14 PM Hesham Almatary <
hesham.almat...@cl.cam.ac.uk> wrote:
> Section 3.6 in RISC-V v1.10 privilege specification states that PMP
> violations
> report "access exceptions." The current PMP implementation has
> a bug which wrongly reports "page exceptions" on PMP violations.
>
> This patch fixes this bug by reporting the correct PMP access exceptions
> trap values.
>
> Signed-off-by: Hesham Almatary
> ---
> target/riscv/cpu_helper.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 41d6db41c3..b48de36114 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -318,12 +318,13 @@ restart:
> }
>
> static void raise_mmu_exception(CPURISCVState *env, target_ulong address,
> -MMUAccessType access_type)
> +MMUAccessType access_type, bool
> pmp_violation)
> {
> CPUState *cs = CPU(riscv_env_get_cpu(env));
> int page_fault_exceptions =
> (env->priv_ver >= PRIV_VERSION_1_10_0) &&
> -get_field(env->satp, SATP_MODE) != VM_1_10_MBARE;
> +get_field(env->satp, SATP_MODE) != VM_1_10_MBARE &&
> +!pmp_violation;
> switch (access_type) {
> case MMU_INST_FETCH:
> cs->exception_index = page_fault_exceptions ?
> @@ -389,6 +390,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address,
> int size,
> CPURISCVState *env = >env;
> hwaddr pa = 0;
> int prot;
> +bool pmp_violation = false;
> int ret = TRANSLATE_FAIL;
>
> qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
> @@ -402,6 +404,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address,
> int size,
>
> if (riscv_feature(env, RISCV_FEATURE_PMP) &&
> !pmp_hart_has_privs(env, pa, TARGET_PAGE_SIZE, 1 << access_type))
> {
> +pmp_violation = true;
> ret = TRANSLATE_FAIL;
> }
> if (ret == TRANSLATE_SUCCESS) {
> @@ -411,7 +414,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address,
> int size,
> } else if (probe) {
> return false;
> } else {
> -raise_mmu_exception(env, address, access_type);
> +raise_mmu_exception(env, address, access_type, pmp_violation);
> riscv_raise_exception(env, cs->exception_index, retaddr);
> }
> #else
> --
> 2.17.1
>
>
>
[Qemu-devel] [PATCH for-4.1 1/2] target/riscv: virt machine shouldn't assume ELF entry is DRAM base
There are two components of this:
* The reset vector now uses the ELF entry point instead of the DRAM base
address.
* Initrd now uses the DRAM base address to figure out the half way point of
RAM. This is more in line with the comment in load_initrd, but an alternative
strategy would be to somehow use the kernel_high address in that computation
instead.
Signed-off-by: Jonathan Behrens
---
hw/riscv/virt.c | 33 +
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index fc4c6b306e..87cc08016b 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -64,10 +64,10 @@ static const struct MemmapEntry {
static target_ulong load_kernel(const char *kernel_filename)
{
-uint64_t kernel_entry, kernel_high;
+uint64_t kernel_entry;
if (load_elf(kernel_filename, NULL, NULL, NULL,
- _entry, NULL, _high,
+ _entry, NULL, NULL,
0, EM_RISCV, 1, 0) < 0) {
error_report("could not load kernel '%s'", kernel_filename);
exit(1);
@@ -75,8 +75,8 @@ static target_ulong load_kernel(const char *kernel_filename)
return kernel_entry;
}
-static hwaddr load_initrd(const char *filename, uint64_t mem_size,
- uint64_t kernel_entry, hwaddr *start)
+static uint64_t load_initrd(const char *filename, uint64_t mem_base,
+uint64_t mem_size, uint64_t *start)
{
int size;
@@ -85,12 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t
mem_size,
* on boards without much RAM we must ensure that we still leave
* enough room for a decent sized initrd, and on boards with large
* amounts of RAM we must avoid the initrd being so far up in RAM
- * that it is outside lowmem and inaccessible to the kernel.
- * So for boards with less than 256MB of RAM we put the initrd
- * halfway into RAM, and for boards with 256MB of RAM or more we put
- * the initrd at 128MB.
+ * that it is outside lowmem and inaccessible to the kernel. So
+ * for boards with less than 256MB of RAM we put the initrd
+ * halfway into RAM, and for boards with 256MB of RAM or more we
+ * put the initrd at 128MB.
*/
-*start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
+*start = mem_base + MIN(mem_size / 2, 128 * MiB);
size = load_ramdisk(filename, *start, mem_size - *start);
if (size == -1) {
@@ -422,14 +422,15 @@ static void riscv_virt_board_init(MachineState *machine)
memory_region_add_subregion(system_memory, memmap[VIRT_MROM].base,
mask_rom);
+uint64_t entry = memmap[VIRT_DRAM].base;
if (machine->kernel_filename) {
-uint64_t kernel_entry = load_kernel(machine->kernel_filename);
+entry = load_kernel(machine->kernel_filename);
if (machine->initrd_filename) {
-hwaddr start;
-hwaddr end = load_initrd(machine->initrd_filename,
- machine->ram_size, kernel_entry,
- );
+uint64_t start;
+uint64_t end = load_initrd(machine->initrd_filename,
+ memmap[VIRT_DRAM].base,
machine->ram_size,
+ );
qemu_fdt_setprop_cell(fdt, "/chosen",
"linux,initrd-start", start);
qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end",
@@ -449,8 +450,8 @@ static void riscv_virt_board_init(MachineState *machine)
#endif
0x00028067, /* jr t0 */
0x,
-memmap[VIRT_DRAM].base, /* start: .dword memmap[VIRT_DRAM].base */
-0x,
+entry & 0x, /* start: .qword entry */
+entry >> 32,
/* dtb: */
};
--
2.20.1
[Qemu-devel] [PATCH for-4.1 2/2] target/riscv: Add support for -bios "firmware_filename" flag
QEMU does not have any default firmware for RISC-V. However, it isn't possible
to run a normal kernel binary without firmware. Thus it has previously been
necessary to compile the two together into a single binary to pass with the
-kernel flag. This patch allows passing separate firmware and kernel binaries by
passing both the -bios and -kernel flags.
This is based on a previously proposed patch by Michael Clark:
https://patchwork.kernel.org/patch/10419975/
Signed-off-by: Jonathan Behrens
---
hw/riscv/virt.c | 66 -
1 file changed, 55 insertions(+), 11 deletions(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 87cc08016b..d7b1792b58 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -62,6 +62,40 @@ static const struct MemmapEntry {
[VIRT_PCIE_ECAM] = { 0x3000,0x1000 },
};
+
+static target_ulong load_firmware_and_kernel(const char *firmware_filename,
+ const char *kernel_filename,
+ uint64_t mem_size,
+ uint64_t* kernel_start,
+ uint64_t* kernel_end)
+{
+uint64_t firmware_entry, firmware_end;
+int size;
+
+if (load_elf(firmware_filename, NULL, NULL, NULL,
+ _entry, NULL, _end,
+ 0, EM_RISCV, 1, 0) < 0) {
+error_report("could not load firmware '%s'", firmware_filename);
+exit(1);
+}
+
+/* align kernel load address to the megapage after the firmware */
+#if defined(TARGET_RISCV32)
+*kernel_start = (firmware_end + 0x3f) & ~0x3f;
+#else
+*kernel_start = (firmware_end + 0x1f) & ~0x1f;
+#endif
+
+size = load_image_targphys(kernel_filename, *kernel_start,
+ mem_size - *kernel_start);
+if (size == -1) {
+error_report("could not load kernel '%s'", kernel_filename);
+exit(1);
+}
+*kernel_end = *kernel_start + size;
+return firmware_entry;
+}
+
static target_ulong load_kernel(const char *kernel_filename)
{
uint64_t kernel_entry;
@@ -423,19 +457,29 @@ static void riscv_virt_board_init(MachineState *machine)
mask_rom);
uint64_t entry = memmap[VIRT_DRAM].base;
-if (machine->kernel_filename) {
+if (machine->firmware && machine->kernel_filename) {
+uint64_t kernel_start, kernel_end;
+entry = load_firmware_and_kernel(machine->firmware,
+ machine->kernel_filename,
+ machine->ram_size, _start,
+ _end);
+
+qemu_fdt_setprop_cells(fdt, "/chosen", "riscv,kernel-end",
+ kernel_end >> 32, kernel_end);
+qemu_fdt_setprop_cells(fdt, "/chosen", "riscv,kernel-start",
+ kernel_start >> 32, kernel_start);
+} else if (machine->kernel_filename) {
entry = load_kernel(machine->kernel_filename);
+}
-if (machine->initrd_filename) {
-uint64_t start;
-uint64_t end = load_initrd(machine->initrd_filename,
- memmap[VIRT_DRAM].base,
machine->ram_size,
- );
-qemu_fdt_setprop_cell(fdt, "/chosen",
- "linux,initrd-start", start);
-qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end",
- end);
-}
+if (machine->kernel_filename && machine->initrd_filename) {
+uint64_t start;
+uint64_t end = load_initrd(machine->initrd_filename,
+ memmap[VIRT_DRAM].base, machine->ram_size,
+ );
+
+qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-start", start);
+qemu_fdt_setprop_cell(fdt, "/chosen", "linux,initrd-end", end);
}
/* reset vector */
--
2.20.1
[Qemu-devel] [PATCH for-4.1 0/2] target/riscv: Improve virt machine kernel handling
Jonathan Behrens (2): target/riscv: virt machine shouldn't assume ELF entry is DRAM base target/riscv: Add support for -bios "firmware_filename" flag hw/riscv/virt.c | 93 - 1 file changed, 69 insertions(+), 24 deletions(-) -- 2.20.1
[Qemu-devel] [PATCH] cadence_gem: Don't define GEM_INT_Q1_MASK twice
Signed-off-by: Jonathan Behrens --- hw/net/cadence_gem.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index 7f63411430..37cb8a4e5c 100644 --- a/hw/net/cadence_gem.c +++ b/hw/net/cadence_gem.c @@ -146,7 +146,6 @@ #define GEM_DESCONF7 (0x0298/4) #define GEM_INT_Q1_STATUS (0x0400 / 4) -#define GEM_INT_Q1_MASK (0x0640 / 4) #define GEM_TRANSMIT_Q1_PTR (0x0440 / 4) #define GEM_TRANSMIT_Q7_PTR (GEM_TRANSMIT_Q1_PTR + 6) -- 2.20.1
[Qemu-devel] [PATCH] target/riscv: Only flush TLB if SATP.ASID changes
There is an analogous change for ARM here:
https://patchwork.kernel.org/patch/10649857
Signed-off-by: Jonathan Behrens
---
target/riscv/csr.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 6083c782a1..1ec1222da1 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -732,7 +732,9 @@ static int write_satp(CPURISCVState *env, int csrno,
target_ulong val)
if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
return -1;
} else {
-tlb_flush(CPU(riscv_env_get_cpu(env)));
+if((val ^ env->satp) & SATP_ASID) {
+tlb_flush(CPU(riscv_env_get_cpu(env)));
+}
env->satp = val;
}
}
--
2.20.1
Re: [Qemu-devel] [PATCH for 4.1] target/riscv: More accurate handling of `sip` CSR
Yes, I was pasting the output of `git format-patch`. Gmail displays
properly for me, but seems to have hard-wrapped the plaintext version of my
outgoing message to 78 characters. I've tried re-sending from a different
address where I can use `git send-email` directly, please let me know if it
works and I'll resend the other patch the same way. Sorry about this!
Jonathan
On Tue, May 7, 2019 at 1:52 PM Palmer Dabbelt wrote:
> On Mon, 06 May 2019 08:52:43 PDT (-0700), finte...@gmail.com wrote:
> > According to the spec, "All bits besides SSIP, USIP, and UEIP in the sip
> > register are read-only." Further, if an interrupt is not delegated to
> mode
> > x,
> > then "the corresponding bits in xip [...] should appear to be hardwired
> to
> > zero. This patch implements both of those requirements.
> >
> > Signed-off-by: Jonathan Behrens
> > ---
> > target/riscv/csr.c | 7 +--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> > index 1ec1222da1..fff7d834e8 100644
> > --- a/target/riscv/csr.c
> > +++ b/target/riscv/csr.c
> > @@ -246,6 +246,7 @@ static const target_ulong sstatus_v1_9_mask =
> > SSTATUS_SIE | SSTATUS_SPIE |
> > static const target_ulong sstatus_v1_10_mask = SSTATUS_SIE |
> SSTATUS_SPIE |
> > SSTATUS_UIE | SSTATUS_UPIE | SSTATUS_SPP | SSTATUS_FS | SSTATUS_XS |
> > SSTATUS_SUM | SSTATUS_MXR | SSTATUS_SD;
> > +static const target_ulong sip_writable_mask = SIP_SSIP | MIP_USIP |
> > MIP_UEIP;
> >
> > #if defined(TARGET_RISCV32)
> > static const char valid_vm_1_09[16] = {
> > @@ -694,8 +695,10 @@ static int write_sbadaddr(CPURISCVState *env, int
> > csrno, target_ulong val)
> > static int rmw_sip(CPURISCVState *env, int csrno, target_ulong
> *ret_value,
> > target_ulong new_value, target_ulong write_mask)
> > {
> > -return rmw_mip(env, CSR_MSTATUS, ret_value, new_value,
> > - write_mask & env->mideleg);
> > +int ret = rmw_mip(env, CSR_MSTATUS, ret_value, new_value,
> > + write_mask & env->mideleg & sip_writable_mask);
> > +*ret_value &= env->mideleg;
> > +return ret;
> > }
> >
> > /* Supervisor Protection and Translation */
>
> This patch (and your previous one) don't apply for me. I don't see the
> git-send-email tags in your messages, are you trying to do something like
> paste
> them into gmail? If so I think they're getting line wrapped.
>
[Qemu-devel] [PATCH] target/riscv: More accurate handling of `sip` CSR
According to the spec, "All bits besides SSIP, USIP, and UEIP in the sip
register are read-only." Further, if an interrupt is not delegated to mode x,
then "the corresponding bits in xip [...] should appear to be hardwired to
zero. This patch implements both of those requirements.
Signed-off-by: Jonathan Behrens
---
target/riscv/csr.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 1ec1222da1..fff7d834e8 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -246,6 +246,7 @@ static const target_ulong sstatus_v1_9_mask = SSTATUS_SIE |
SSTATUS_SPIE |
static const target_ulong sstatus_v1_10_mask = SSTATUS_SIE | SSTATUS_SPIE |
SSTATUS_UIE | SSTATUS_UPIE | SSTATUS_SPP | SSTATUS_FS | SSTATUS_XS |
SSTATUS_SUM | SSTATUS_MXR | SSTATUS_SD;
+static const target_ulong sip_writable_mask = SIP_SSIP | MIP_USIP | MIP_UEIP;
#if defined(TARGET_RISCV32)
static const char valid_vm_1_09[16] = {
@@ -694,8 +695,10 @@ static int write_sbadaddr(CPURISCVState *env, int csrno,
target_ulong val)
static int rmw_sip(CPURISCVState *env, int csrno, target_ulong *ret_value,
target_ulong new_value, target_ulong write_mask)
{
-return rmw_mip(env, CSR_MSTATUS, ret_value, new_value,
- write_mask & env->mideleg);
+int ret = rmw_mip(env, CSR_MSTATUS, ret_value, new_value,
+ write_mask & env->mideleg & sip_writable_mask);
+*ret_value &= env->mideleg;
+return ret;
}
/* Supervisor Protection and Translation */
--
2.20.1
[Qemu-devel] [PATCH for 4.1] target/riscv: More accurate handling of `sip` CSR
According to the spec, "All bits besides SSIP, USIP, and UEIP in the sip
register are read-only." Further, if an interrupt is not delegated to mode
x,
then "the corresponding bits in xip [...] should appear to be hardwired to
zero. This patch implements both of those requirements.
Signed-off-by: Jonathan Behrens
---
target/riscv/csr.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 1ec1222da1..fff7d834e8 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -246,6 +246,7 @@ static const target_ulong sstatus_v1_9_mask =
SSTATUS_SIE | SSTATUS_SPIE |
static const target_ulong sstatus_v1_10_mask = SSTATUS_SIE | SSTATUS_SPIE |
SSTATUS_UIE | SSTATUS_UPIE | SSTATUS_SPP | SSTATUS_FS | SSTATUS_XS |
SSTATUS_SUM | SSTATUS_MXR | SSTATUS_SD;
+static const target_ulong sip_writable_mask = SIP_SSIP | MIP_USIP |
MIP_UEIP;
#if defined(TARGET_RISCV32)
static const char valid_vm_1_09[16] = {
@@ -694,8 +695,10 @@ static int write_sbadaddr(CPURISCVState *env, int
csrno, target_ulong val)
static int rmw_sip(CPURISCVState *env, int csrno, target_ulong *ret_value,
target_ulong new_value, target_ulong write_mask)
{
-return rmw_mip(env, CSR_MSTATUS, ret_value, new_value,
- write_mask & env->mideleg);
+int ret = rmw_mip(env, CSR_MSTATUS, ret_value, new_value,
+ write_mask & env->mideleg & sip_writable_mask);
+*ret_value &= env->mideleg;
+return ret;
}
/* Supervisor Protection and Translation */
--
2.20.1
Re: [Qemu-devel] [PATCH for 4.1] target/riscv: Only flush TLB if SATP.ASID changes
Argh, meant to include a signed off by line:
Signed-off-by: Jonathan Behrens
On Mon, May 6, 2019 at 11:31 AM Jonathan Behrens wrote:
> There is an analogous change for ARM here:
> https://patchwork.kernel.org/patch/10649857
> ---
> target/riscv/csr.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/csr.c b/target/riscv/csr.c
> index 6083c782a1..1ec1222da1 100644
> --- a/target/riscv/csr.c
> +++ b/target/riscv/csr.c
> @@ -732,7 +732,9 @@ static int write_satp(CPURISCVState *env, int csrno,
> target_ulong val)
> if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
> return -1;
> } else {
> -tlb_flush(CPU(riscv_env_get_cpu(env)));
> +if((val ^ env->satp) & SATP_ASID) {
> +tlb_flush(CPU(riscv_env_get_cpu(env)));
> +}
> env->satp = val;
> }
> }
> --
> 2.20.1
>
>
[Qemu-devel] [PATCH for 4.1] target/riscv: Only flush TLB if SATP.ASID changes
There is an analogous change for ARM here:
https://patchwork.kernel.org/patch/10649857
---
target/riscv/csr.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index 6083c782a1..1ec1222da1 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -732,7 +732,9 @@ static int write_satp(CPURISCVState *env, int csrno,
target_ulong val)
if (env->priv == PRV_S && get_field(env->mstatus, MSTATUS_TVM)) {
return -1;
} else {
-tlb_flush(CPU(riscv_env_get_cpu(env)));
+if((val ^ env->satp) & SATP_ASID) {
+tlb_flush(CPU(riscv_env_get_cpu(env)));
+}
env->satp = val;
}
}
--
2.20.1
[Qemu-devel] [PATCH for 4.1 v2] target/riscv: Expose time CSRs when allowed by [m|s]counteren
Currently mcounteren.TM acts as though it is hardwired to zero, even though
QEMU allows it to be set. This change resolves the issue by allowing reads
to the time and timeh control registers when running in a privileged mode
where such accesses are allowed.
The frequency of the time register is stored in the time_freq field of each
hart so that it is accessible during CSR reads, but must be the same across
all harts. Each board can initialize it to a custom value, although all
currently use a 10 MHz frequency.
---
hw/riscv/riscv_hart.c | 4
hw/riscv/sifive_clint.c | 30 ++
hw/riscv/sifive_e.c | 2 ++
hw/riscv/sifive_u.c | 4 +++-
hw/riscv/spike.c| 6 +-
hw/riscv/virt.c | 4 +++-
include/hw/riscv/riscv_hart.h | 1 +
include/hw/riscv/sifive_clint.h | 4
include/hw/riscv/sifive_e.h | 4
include/hw/riscv/sifive_u.h | 1 +
include/hw/riscv/spike.h| 1 +
include/hw/riscv/virt.h | 1 +
target/riscv/cpu.h | 2 ++
target/riscv/csr.c | 17 +++--
14 files changed, 60 insertions(+), 21 deletions(-)
diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c
index e34a26a0ef..c39cd55330 100644
--- a/hw/riscv/riscv_hart.c
+++ b/hw/riscv/riscv_hart.c
@@ -19,6 +19,7 @@
*/
#include "qemu/osdep.h"
+#include "qemu/timer.h"
#include "qapi/error.h"
#include "hw/sysbus.h"
#include "target/riscv/cpu.h"
@@ -27,6 +28,8 @@
static Property riscv_harts_props[] = {
DEFINE_PROP_UINT32("num-harts", RISCVHartArrayState, num_harts, 1),
DEFINE_PROP_STRING("cpu-type", RISCVHartArrayState, cpu_type),
+DEFINE_PROP_UINT64("timebase-frequency", RISCVHartArrayState,
time_freq,
+ NANOSECONDS_PER_SECOND),
DEFINE_PROP_END_OF_LIST(),
};
@@ -49,6 +52,7 @@ static void riscv_harts_realize(DeviceState *dev, Error
**errp)
sizeof(RISCVCPU), s->cpu_type,
_abort, NULL);
s->harts[n].env.mhartid = n;
+s->harts[n].env.time_freq = s->time_freq;
qemu_register_reset(riscv_harts_cpu_reset, >harts[n]);
object_property_set_bool(OBJECT(>harts[n]), true,
"realized", );
diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
index d4c159e937..71edf4dcc6 100644
--- a/hw/riscv/sifive_clint.c
+++ b/hw/riscv/sifive_clint.c
@@ -26,10 +26,10 @@
#include "hw/riscv/sifive_clint.h"
#include "qemu/timer.h"
-static uint64_t cpu_riscv_read_rtc(void)
+static uint64_t cpu_riscv_read_rtc(CPURISCVState *env)
{
return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-SIFIVE_CLINT_TIMEBASE_FREQ, NANOSECONDS_PER_SECOND);
+env->time_freq, NANOSECONDS_PER_SECOND);
}
/*
@@ -41,7 +41,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
uint64_t value)
uint64_t next;
uint64_t diff;
-uint64_t rtc_r = cpu_riscv_read_rtc();
+uint64_t rtc_r = cpu_riscv_read_rtc(>env);
cpu->env.timecmp = value;
if (cpu->env.timecmp <= rtc_r) {
@@ -56,7 +56,7 @@ static void sifive_clint_write_timecmp(RISCVCPU *cpu,
uint64_t value)
diff = cpu->env.timecmp - rtc_r;
/* back to ns (note args switched in muldiv64) */
next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-muldiv64(diff, NANOSECONDS_PER_SECOND, SIFIVE_CLINT_TIMEBASE_FREQ);
+muldiv64(diff, NANOSECONDS_PER_SECOND, cpu->env.time_freq);
timer_mod(cpu->env.timer, next);
}
@@ -107,11 +107,25 @@ static uint64_t sifive_clint_read(void *opaque,
hwaddr addr, unsigned size)
return 0;
}
} else if (addr == clint->time_base) {
-/* time_lo */
-return cpu_riscv_read_rtc() & 0x;
+/* All harts must have the same time frequency, so just use hart 0
*/
+CPUState *cpu = qemu_get_cpu(0);
+CPURISCVState *env = cpu ? cpu->env_ptr : NULL;
+if (!env) {
+error_report("clint: hart 0 not valid?!");
+} else {
+/* time_lo */
+return cpu_riscv_read_rtc(env) & 0x;
+}
} else if (addr == clint->time_base + 4) {
-/* time_hi */
-return (cpu_riscv_read_rtc() >> 32) & 0x;
+/* All harts must have the same time frequency, so just use hart 0
*/
+CPUState *cpu = qemu_get_cpu(0);
+CPURISCVState *env = cpu ? cpu->env_ptr : NULL;
+if (!env) {
+error_report("clint: hart 0 not valid?!");
+} else {
+/* time_hi */
+return (cpu_riscv_read_rtc(env) >> 32) & 0x;
+}
}
error_report("clint: invalid read: %08x", (uint32_t)addr);
diff --git a/hw/riscv/sifive_e.c b/hw/riscv/sifive_e.c
index b1cd11363c..0a6bb629b6 100644
--- a/hw/riscv/sifive_e.c
+++ b/hw/riscv/sifive_e.c
@@ -146,6 +146,8 @@ static void riscv_sifive_e_soc_init(Object *obj)
Re: [Qemu-devel] [PATCH] target/riscv: Expose time CSRs when allowed by [m|s]counteren
No, I've still been meaning to send it. After thinking about this some more
I realized that it didn't actually make sense for the CLINT to decide the
timer frequency and that it should instead be a property of the board
itself. I got a bit sidetracked in the process of making those changes, but
I should have a new version out in the next few days.
On Thu, Apr 25, 2019 at 5:44 PM Palmer Dabbelt wrote:
> On Fri, 19 Apr 2019 16:05:35 PDT (-0700), alistai...@gmail.com wrote:
> > On Mon, Apr 15, 2019 at 5:46 PM Jonathan Behrens
> wrote:
> >>
> >> For any chip that has a CLINT, we want the frequency of the time
> register and the frequency of the CLINT to match. That frequency,
> SIFIVE_CLINT_TIMEBASE_FREQ (=10MHz) is currently defined in
> hw/riscv/sifive_clint.h and so isn't visible to target/riscv/cpu.c where
> the CPURISCVState is first created. Instead, I first initialize the
> frequency to a reasonable default (1GHz) and then let the CLINT override
> the value if one is attached. Phrased differently, the values produced by
> the `sifive_clint.c: cpu_riscv_read_rtc()` and `csr.c: read_time()` must
> match, and this is one way of doing that.
> >
> > Ah that seems fine. Can you add a comment in the code to indicate that
> > it will be overwritten later?
>
> I don't see a v2, did I miss something?
>
> >
> > Alistair
> >
> >>
> >> I'd be open to other suggestions.
> >>
> >> Jonathan
> >>
> >> On Mon, Apr 15, 2019 at 8:23 PM Alistair Francis
> wrote:
> >>>
> >>> On Fri, Apr 12, 2019 at 12:04 PM Jonathan Behrens
> wrote:
> >>> >
> >>> > Currently mcounteren.TM acts as though it is hardwired to zero, even
> though
> >>> > QEMU
> >>> > allows it to be set. This change resolves the issue by allowing
> reads to the
> >>> > time and timeh control registers when running in a privileged mode
> where
> >>> > such
> >>> > accesses are allowed.
> >>> >
> >>> > Signed-off-by: Jonathan Behrens
> >>> > ---
> >>> > hw/riscv/sifive_clint.c | 1 +
> >>> > target/riscv/cpu.c | 14 ++
> >>> > target/riscv/cpu.h | 2 ++
> >>> > target/riscv/csr.c | 17 +++--
> >>> > 4 files changed, 28 insertions(+), 6 deletions(-)
> >>> >
> >>> > diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
> >>> > index d4c159e937..3ad4fe6139 100644
> >>> > --- a/hw/riscv/sifive_clint.c
> >>> > +++ b/hw/riscv/sifive_clint.c
> >>> > @@ -237,6 +237,7 @@ DeviceState *sifive_clint_create(hwaddr addr,
> hwaddr
> >>> > size, uint32_t num_harts,
> >>> > env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> >>> >_clint_timer_cb, cpu);
> >>> > env->timecmp = 0;
> >>> > +env->time_freq = SIFIVE_CLINT_TIMEBASE_FREQ;
> >>>
> >>> Why do you need to set this here?
> >>>
> >>> Alistair
> >>>
> >>> > }
> >>> >
> >>> > DeviceState *dev = qdev_create(NULL, TYPE_SIFIVE_CLINT);
> >>> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >>> > index d61bce6d55..ff17d54691 100644
> >>> > --- a/target/riscv/cpu.c
> >>> > +++ b/target/riscv/cpu.c
> >>> > @@ -103,12 +103,20 @@ static void set_resetvec(CPURISCVState *env,
> int
> >>> > resetvec)
> >>> > #endif
> >>> > }
> >>> >
> >>> > +static void set_time_freq(CPURISCVState *env, uint64_t freq)
> >>> > +{
> >>> > +#ifndef CONFIG_USER_ONLY
> >>> > +env->time_freq = freq;
> >>> > +#endif
> >>> > +}
> >>> > +
> >>> > static void riscv_any_cpu_init(Object *obj)
> >>> > {
> >>> > CPURISCVState *env = _CPU(obj)->env;
> >>> > set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> >>> > set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0);
> >>> > set_resetvec(env, DEFAULT_RSTVEC);
> >>> > +set_time_freq(env, NANOSECONDS_PER_SECOND);
> >>> > }
> >>> >
> >>> > #if defined(TARGET_RISCV32)
> >>> > @@ -121,6 +129,7 @@ static void rv32gcsu_priv1_09_1_cpu_init(Obj
Re: [Qemu-devel] [PATCH] target/riscv: Expose time CSRs when allowed by [m|s]counteren
For any chip that has a CLINT, we want the frequency of the time register
and the frequency of the CLINT to match. That frequency,
SIFIVE_CLINT_TIMEBASE_FREQ
(=10MHz) is currently defined in hw/riscv/sifive_clint.h and so isn't
visible to target/riscv/cpu.c where the CPURISCVState is first created.
Instead, I first initialize the frequency to a reasonable default (1GHz)
and then let the CLINT override the value if one is attached. Phrased
differently, the values produced by the `sifive_clint.c:
cpu_riscv_read_rtc()` and `csr.c: read_time()` must match, and this is one
way of doing that.
I'd be open to other suggestions.
Jonathan
On Mon, Apr 15, 2019 at 8:23 PM Alistair Francis
wrote:
> On Fri, Apr 12, 2019 at 12:04 PM Jonathan Behrens
> wrote:
> >
> > Currently mcounteren.TM acts as though it is hardwired to zero, even
> though
> > QEMU
> > allows it to be set. This change resolves the issue by allowing reads to
> the
> > time and timeh control registers when running in a privileged mode where
> > such
> > accesses are allowed.
> >
> > Signed-off-by: Jonathan Behrens
> > ---
> > hw/riscv/sifive_clint.c | 1 +
> > target/riscv/cpu.c | 14 ++
> > target/riscv/cpu.h | 2 ++
> > target/riscv/csr.c | 17 +++--
> > 4 files changed, 28 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
> > index d4c159e937..3ad4fe6139 100644
> > --- a/hw/riscv/sifive_clint.c
> > +++ b/hw/riscv/sifive_clint.c
> > @@ -237,6 +237,7 @@ DeviceState *sifive_clint_create(hwaddr addr, hwaddr
> > size, uint32_t num_harts,
> > env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> >_clint_timer_cb, cpu);
> > env->timecmp = 0;
> > +env->time_freq = SIFIVE_CLINT_TIMEBASE_FREQ;
>
> Why do you need to set this here?
>
> Alistair
>
> > }
> >
> > DeviceState *dev = qdev_create(NULL, TYPE_SIFIVE_CLINT);
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index d61bce6d55..ff17d54691 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -103,12 +103,20 @@ static void set_resetvec(CPURISCVState *env, int
> > resetvec)
> > #endif
> > }
> >
> > +static void set_time_freq(CPURISCVState *env, uint64_t freq)
> > +{
> > +#ifndef CONFIG_USER_ONLY
> > +env->time_freq = freq;
> > +#endif
> > +}
> > +
> > static void riscv_any_cpu_init(Object *obj)
> > {
> > CPURISCVState *env = _CPU(obj)->env;
> > set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
> > set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0);
> > set_resetvec(env, DEFAULT_RSTVEC);
> > +set_time_freq(env, NANOSECONDS_PER_SECOND);
> > }
> >
> > #if defined(TARGET_RISCV32)
> > @@ -121,6 +129,7 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
> > set_resetvec(env, DEFAULT_RSTVEC);
> > set_feature(env, RISCV_FEATURE_MMU);
> > set_feature(env, RISCV_FEATURE_PMP);
> > +set_time_freq(env, NANOSECONDS_PER_SECOND);
> > }
> >
> > static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
> > @@ -131,6 +140,7 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
> > set_resetvec(env, DEFAULT_RSTVEC);
> > set_feature(env, RISCV_FEATURE_MMU);
> > set_feature(env, RISCV_FEATURE_PMP);
> > +set_time_freq(env, NANOSECONDS_PER_SECOND);
> > }
> >
> > static void rv32imacu_nommu_cpu_init(Object *obj)
> > @@ -140,6 +150,7 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
> > set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0);
> > set_resetvec(env, DEFAULT_RSTVEC);
> > set_feature(env, RISCV_FEATURE_PMP);
> > +set_time_freq(env, NANOSECONDS_PER_SECOND);
> > }
> >
> > #elif defined(TARGET_RISCV64)
> > @@ -152,6 +163,7 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
> > set_resetvec(env, DEFAULT_RSTVEC);
> > set_feature(env, RISCV_FEATURE_MMU);
> > set_feature(env, RISCV_FEATURE_PMP);
> > +set_time_freq(env, NANOSECONDS_PER_SECOND);
> > }
> >
> > static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
> > @@ -162,6 +174,7 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
> > set_resetvec(env, DEFAULT_RSTVEC);
> > set_feature(env, RISCV_FEATURE_MMU);
> > set_feature(env, RISCV_FEATURE_PMP);
> > +set_time_freq(env, NANOSECONDS_PER_SECOND);
> > }
&g
Re: [Qemu-devel] [PATCH] target/riscv: Do not allow sfence.vma from user mode
Just to double check, nothing further on this is need from me, right? It is
set to be merged onto the master branch once the 4.0 release is out?
Jonathan
On Wed, Apr 3, 2019 at 7:11 PM Alistair Francis
wrote:
> On Mon, Apr 1, 2019 at 1:39 PM Jonathan Behrens
> wrote:
> >
> > The 'sfence.vma' instruction is privileged, and should only ever be
> allowed
> > when executing in supervisor mode or higher.
> >
> > Jonathan
> >
> > Signed-off-by: Jonathan Behrens
>
> Reviewed-by: Alistair Francis
>
> Alistair
>
> > ---
> > target/riscv/op_helper.c | 7 ---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
> > index b7dc18a41e..644d0fb35f 100644
> > --- a/target/riscv/op_helper.c
> > +++ b/target/riscv/op_helper.c
> > @@ -145,9 +145,10 @@ void helper_tlb_flush(CPURISCVState *env)
> > {
> > RISCVCPU *cpu = riscv_env_get_cpu(env);
> > CPUState *cs = CPU(cpu);
> > -if (env->priv == PRV_S &&
> > -env->priv_ver >= PRIV_VERSION_1_10_0 &&
> > -get_field(env->mstatus, MSTATUS_TVM)) {
> > +if (!(env->priv >= PRV_S) ||
> > +(env->priv == PRV_S &&
> > + env->priv_ver >= PRIV_VERSION_1_10_0 &&
> > + get_field(env->mstatus, MSTATUS_TVM))) {
> > riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> > } else {
> > tlb_flush(cs);
> > --
> > 2.20.1
>
[Qemu-devel] [PATCH] target/riscv: Expose time CSRs when allowed by [m|s]counteren
Currently mcounteren.TM acts as though it is hardwired to zero, even though
QEMU
allows it to be set. This change resolves the issue by allowing reads to the
time and timeh control registers when running in a privileged mode where
such
accesses are allowed.
Signed-off-by: Jonathan Behrens
---
hw/riscv/sifive_clint.c | 1 +
target/riscv/cpu.c | 14 ++
target/riscv/cpu.h | 2 ++
target/riscv/csr.c | 17 +++--
4 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
index d4c159e937..3ad4fe6139 100644
--- a/hw/riscv/sifive_clint.c
+++ b/hw/riscv/sifive_clint.c
@@ -237,6 +237,7 @@ DeviceState *sifive_clint_create(hwaddr addr, hwaddr
size, uint32_t num_harts,
env->timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
_clint_timer_cb, cpu);
env->timecmp = 0;
+env->time_freq = SIFIVE_CLINT_TIMEBASE_FREQ;
}
DeviceState *dev = qdev_create(NULL, TYPE_SIFIVE_CLINT);
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index d61bce6d55..ff17d54691 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -103,12 +103,20 @@ static void set_resetvec(CPURISCVState *env, int
resetvec)
#endif
}
+static void set_time_freq(CPURISCVState *env, uint64_t freq)
+{
+#ifndef CONFIG_USER_ONLY
+env->time_freq = freq;
+#endif
+}
+
static void riscv_any_cpu_init(Object *obj)
{
CPURISCVState *env = _CPU(obj)->env;
set_misa(env, RVXLEN | RVI | RVM | RVA | RVF | RVD | RVC | RVU);
set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0);
set_resetvec(env, DEFAULT_RSTVEC);
+set_time_freq(env, NANOSECONDS_PER_SECOND);
}
#if defined(TARGET_RISCV32)
@@ -121,6 +129,7 @@ static void rv32gcsu_priv1_09_1_cpu_init(Object *obj)
set_resetvec(env, DEFAULT_RSTVEC);
set_feature(env, RISCV_FEATURE_MMU);
set_feature(env, RISCV_FEATURE_PMP);
+set_time_freq(env, NANOSECONDS_PER_SECOND);
}
static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
@@ -131,6 +140,7 @@ static void rv32gcsu_priv1_10_0_cpu_init(Object *obj)
set_resetvec(env, DEFAULT_RSTVEC);
set_feature(env, RISCV_FEATURE_MMU);
set_feature(env, RISCV_FEATURE_PMP);
+set_time_freq(env, NANOSECONDS_PER_SECOND);
}
static void rv32imacu_nommu_cpu_init(Object *obj)
@@ -140,6 +150,7 @@ static void rv32imacu_nommu_cpu_init(Object *obj)
set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0);
set_resetvec(env, DEFAULT_RSTVEC);
set_feature(env, RISCV_FEATURE_PMP);
+set_time_freq(env, NANOSECONDS_PER_SECOND);
}
#elif defined(TARGET_RISCV64)
@@ -152,6 +163,7 @@ static void rv64gcsu_priv1_09_1_cpu_init(Object *obj)
set_resetvec(env, DEFAULT_RSTVEC);
set_feature(env, RISCV_FEATURE_MMU);
set_feature(env, RISCV_FEATURE_PMP);
+set_time_freq(env, NANOSECONDS_PER_SECOND);
}
static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
@@ -162,6 +174,7 @@ static void rv64gcsu_priv1_10_0_cpu_init(Object *obj)
set_resetvec(env, DEFAULT_RSTVEC);
set_feature(env, RISCV_FEATURE_MMU);
set_feature(env, RISCV_FEATURE_PMP);
+set_time_freq(env, NANOSECONDS_PER_SECOND);
}
static void rv64imacu_nommu_cpu_init(Object *obj)
@@ -171,6 +184,7 @@ static void rv64imacu_nommu_cpu_init(Object *obj)
set_versions(env, USER_VERSION_2_02_0, PRIV_VERSION_1_10_0);
set_resetvec(env, DEFAULT_RSTVEC);
set_feature(env, RISCV_FEATURE_PMP);
+set_time_freq(env, NANOSECONDS_PER_SECOND);
}
#endif
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 20bce8742e..67b1769ad3 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -173,7 +173,9 @@ struct CPURISCVState {
/* temporary htif regs */
uint64_t mfromhost;
uint64_t mtohost;
+
uint64_t timecmp;
+uint64_t time_freq;
/* physical memory protection */
pmp_table_t pmp_state;
diff --git a/target/riscv/csr.c b/target/riscv/csr.c
index e1d91b6c60..6083c782a1 100644
--- a/target/riscv/csr.c
+++ b/target/riscv/csr.c
@@ -191,22 +191,31 @@ static int read_instreth(CPURISCVState *env, int
csrno, target_ulong *val)
}
#endif /* TARGET_RISCV32 */
-#if defined(CONFIG_USER_ONLY)
static int read_time(CPURISCVState *env, int csrno, target_ulong *val)
{
+#if !defined(CONFIG_USER_ONLY)
+*val = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+env->time_freq, NANOSECONDS_PER_SECOND);
+#else
*val = cpu_get_host_ticks();
+#endif
return 0;
}
#if defined(TARGET_RISCV32)
static int read_timeh(CPURISCVState *env, int csrno, target_ulong *val)
{
+#if !defined(CONFIG_USER_ONLY)
+*val = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+env->time_freq, NANOSECONDS_PER_SECOND) >> 32;
+#else
*val = cpu_get_host_ticks() >> 32;
+#endif
return 0;
}
#endif
-#else /* CONFIG_USER_ONLY */
+#if !defined(CONFIG_USER_ONLY)
/* Machine constants */
@@ -854,14 +863,
[Qemu-devel] [PATCH v2] target/riscv: Remove unused include of riscv_htif.h for virt board riscv
Signed-off-by: Jonathan Behrens --- hw/riscv/virt.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index fc4c6b306e..3526463034 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -29,7 +29,6 @@ #include "hw/sysbus.h" #include "hw/char/serial.h" #include "target/riscv/cpu.h" -#include "hw/riscv/riscv_htif.h" #include "hw/riscv/riscv_hart.h" #include "hw/riscv/sifive_plic.h" #include "hw/riscv/sifive_clint.h" -- 2.20.1
[Qemu-devel] [PATCH] target/riscv: Remove unused include of riscv_htif.h for virt board
Unless I'm missing something, the virt board doesn't support HTIF and should not be including this header. Jonathan Signed-off-by: Jonathan Behrens --- hw/riscv/virt.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c index fc4c6b306e..3526463034 100644 --- a/hw/riscv/virt.c +++ b/hw/riscv/virt.c @@ -29,7 +29,6 @@ #include "hw/sysbus.h" #include "hw/char/serial.h" #include "target/riscv/cpu.h" -#include "hw/riscv/riscv_htif.h" #include "hw/riscv/riscv_hart.h" #include "hw/riscv/sifive_plic.h" #include "hw/riscv/sifive_clint.h" -- 2.20.1
[Qemu-devel] [PATCH] target/riscv: Do not allow sfence.vma from user mode
The 'sfence.vma' instruction is privileged, and should only ever be allowed
when executing in supervisor mode or higher.
Jonathan
Signed-off-by: Jonathan Behrens
---
target/riscv/op_helper.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index b7dc18a41e..644d0fb35f 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -145,9 +145,10 @@ void helper_tlb_flush(CPURISCVState *env)
{
RISCVCPU *cpu = riscv_env_get_cpu(env);
CPUState *cs = CPU(cpu);
-if (env->priv == PRV_S &&
-env->priv_ver >= PRIV_VERSION_1_10_0 &&
-get_field(env->mstatus, MSTATUS_TVM)) {
+if (!(env->priv >= PRV_S) ||
+(env->priv == PRV_S &&
+ env->priv_ver >= PRIV_VERSION_1_10_0 &&
+ get_field(env->mstatus, MSTATUS_TVM))) {
riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
} else {
tlb_flush(cs);
--
2.20.1
[Qemu-devel] [Bug 1820686] [NEW] risc-v: 'c.unimp' instruction decoded as 'c.addi4spn fp, 0'
Public bug reported: QEMU 3.1 incorrectly decodes the "c.unimp" instruction (opcode 0x) as an "addi4spn fp, 0" when either of the two following bytes are non- zero. This is because the ctx->opcode value used when decoding the instruction is actually filled with a 32-bit load (to handle normal uncompressed instructions) but when a compressed instruction is found only the low 16 bits are valid. Other reserved/illegal bit patterns with the addi4spn opcode are also incorrectly decoded. I believe that the switch to decodetree on master happened to fix this issue, but hopefully it is helpful to have this recorded somewhere. I've included a simple one line patch if anyone wants to backport this. ** Affects: qemu Importance: Undecided Status: New ** Patch added: "decode.patch" https://bugs.launchpad.net/bugs/1820686/+attachment/5247338/+files/decode.patch -- You received this bug notification because you are a member of qemu- devel-ml, which is subscribed to QEMU. https://bugs.launchpad.net/bugs/1820686 Title: risc-v: 'c.unimp' instruction decoded as 'c.addi4spn fp, 0' Status in QEMU: New Bug description: QEMU 3.1 incorrectly decodes the "c.unimp" instruction (opcode 0x) as an "addi4spn fp, 0" when either of the two following bytes are non- zero. This is because the ctx->opcode value used when decoding the instruction is actually filled with a 32-bit load (to handle normal uncompressed instructions) but when a compressed instruction is found only the low 16 bits are valid. Other reserved/illegal bit patterns with the addi4spn opcode are also incorrectly decoded. I believe that the switch to decodetree on master happened to fix this issue, but hopefully it is helpful to have this recorded somewhere. I've included a simple one line patch if anyone wants to backport this. To manage notifications about this bug go to: https://bugs.launchpad.net/qemu/+bug/1820686/+subscriptions
[Qemu-devel] [Bug 1814343] [NEW] Initrd not loaded on riscv32
Public bug reported:
I attempted to run qemu with a ram disk. However, when reading the
contents of the disk from within the VM I only get back zeros.
I was able to trace the issue to a mismatch of expectations on line 93
of hw/riscv/virt.c. Specifically, when running in 32-bit mode the value
of kernel_entry is sign extended to 64-bits, but load_image_targphys
expects the start address to not be sign extended.
Straw man patch (works for 32-bit but would probably break 64-bit VMs?):
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index e7f0716fb6..32216f993c 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -90,7 +90,7 @@ static hwaddr load_initrd(const char *filename, uint64_t
mem_size,
* halfway into RAM, and for boards with 256MB of RAM or more we put
* the initrd at 128MB.
*/
-*start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
+*start = (kernel_entry & 0x) + MIN(mem_size / 2, 128 * MiB);
size = load_ramdisk(filename, *start, mem_size - *start);
if (size == -1) {
Run command:
$ qemu/build/riscv32-softmmu/qemu-system-riscv32 -machine virt -kernel
mykernel.elf -nographic -initrd payload
Commit hash:
3a183e330dbd7dbcac3841737ac874979552cca2
** Affects: qemu
Importance: Undecided
Status: New
--
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1814343
Title:
Initrd not loaded on riscv32
Status in QEMU:
New
Bug description:
I attempted to run qemu with a ram disk. However, when reading the
contents of the disk from within the VM I only get back zeros.
I was able to trace the issue to a mismatch of expectations on line 93
of hw/riscv/virt.c. Specifically, when running in 32-bit mode the
value of kernel_entry is sign extended to 64-bits, but
load_image_targphys expects the start address to not be sign extended.
Straw man patch (works for 32-bit but would probably break 64-bit
VMs?):
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index e7f0716fb6..32216f993c 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -90,7 +90,7 @@ static hwaddr load_initrd(const char *filename, uint64_t
mem_size,
* halfway into RAM, and for boards with 256MB of RAM or more we put
* the initrd at 128MB.
*/
-*start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
+*start = (kernel_entry & 0x) + MIN(mem_size / 2, 128 * MiB);
size = load_ramdisk(filename, *start, mem_size - *start);
if (size == -1) {
Run command:
$ qemu/build/riscv32-softmmu/qemu-system-riscv32 -machine virt -kernel
mykernel.elf -nographic -initrd payload
Commit hash:
3a183e330dbd7dbcac3841737ac874979552cca2
To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1814343/+subscriptions
