Re: [Qemu-devel] [Qemu-riscv] [PATCH v1 10/28] target/riscv: Convert mie and mstatus to pointers

From: Jonathan Behrens <fintelia@gmail.com>
To: Richard Henderson <richard.henderson@linaro.org>
Cc: Palmer Dabbelt <palmer@sifive.com>,
	"open list:RISC-V" <qemu-riscv@nongnu.org>,
	Anup Patel <Anup.Patel@wdc.com>,
	"qemu-devel@nongnu.org Developers" <qemu-devel@nongnu.org>,
	Atish Patra <Atish.Patra@wdc.com>,
	Alistair Francis <Alistair.Francis@wdc.com>,
	Alistair Francis <alistair23@gmail.com>
Subject: Re: [Qemu-devel] [Qemu-riscv] [PATCH v1 10/28] target/riscv: Convert mie and mstatus to pointers
Date: Thu, 19 Sep 2019 12:58:40 -0400	[thread overview]
Message-ID: <CANnJOVEsF57FBbNxJ_WvL3O7iJ_mZijYikC_aNhxKcP856qTzw@mail.gmail.com> (raw)
In-Reply-To: <18b87754-d5b2-e4f1-bdc5-92ad26b97379@linaro.org>

On Thu, Sep 19, 2019 at 10:50 AM Richard Henderson
<richard.henderson@linaro.org> 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);
}