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

[Alistair Francis]

On Thu, Sep 19, 2019 at 9:59 AM Jonathan Behrens <address@hidden> wrote:
>
> On Thu, Sep 19, 2019 at 10:50 AM Richard Henderson
> <address@hidden> 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);

The part I don't like about this is that it then requires all of the
RISC-V implementation to be affected by the Hypervisor extension. The
current way means that if you aren't interested in the extension you
can just ignore it and not worry about breaking anything. For ARM this
isn't as big of an issue, but RISC-V is much more modular (there will
be lots of platforms without the H extension) so I don't want people
to have to worry about it.

PS: Sorry for the delay here, I have been looking into some other ways
of doing this, but I still think the current way is the least bad.

Alistair

> }