Re: [PATCH v1 15/36] target/riscv: Convert mstatus to pointers
On Fri, Jan 31, 2020 at 9:31 AM Alistair Francis wrote:
>
> On Thu, Jan 30, 2020 at 6:48 AM Palmer Dabbelt
> wrote:
> >
> > On Tue, 21 Jan 2020 11:02:01 GMT (+), alistai...@gmail.com 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.
> >
> > OK, so in the interest of moving things forwards let's just
> >
> > Reviewed-by: Palmer Dabbelt
>
> Thanks
>
> >
> > so we can merge this -- it's too big of a patch set to wait around on
> > something
> > so small for. I think that was the last one missing a review, right?
>
> I have made one small change and dismissed your review from a patch,
> it also looks like one patch hasn't been reviewed either.
>
> I'll send a v2 later today that has been rebased on master.
After rebasing everything stopped working. While attempting to fix the
problem I remeoved this patch and managed to get it working again. So
now in v2 there are no CSR pointers, just value swapping for
everything.
I fixed a few bugs that I noticed related to FP, I have cleared your
review Palmer from a few patches. I'll send the v2 for review soon.
It's pretty similar to the v1 so should be easy to review.
Alistair
>
> Alistair
>
> >
> > >
> > > 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
Re: [PATCH v1 15/36] target/riscv: Convert mstatus to pointers
On Thu, Jan 30, 2020 at 6:48 AM Palmer Dabbelt wrote:
>
> On Tue, 21 Jan 2020 11:02:01 GMT (+), alistai...@gmail.com 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.
>
> OK, so in the interest of moving things forwards let's just
>
> Reviewed-by: Palmer Dabbelt
Thanks
>
> so we can merge this -- it's too big of a patch set to wait around on
> something
> so small for. I think that was the last one missing a review, right?
I have made one small change and dismissed your review from a patch,
it also looks like one patch hasn't been reviewed either.
I'll send a v2 later today that has been rebased on master.
Alistair
>
> >
> > 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 the "real" versions that the pointer
> >> > + * versions point to. These should never be used unless you know
> >> > what you
> >> > + * are doing. To access these use the pointer versions instead.
> >> > This is
> >> > + * required to handle the Hypervisor register swapping.
> >> > + */
> >> > +target_ulong mstatus_novirt;
> >> > +
> >> > /* Hypervisor CSRs */
> >> > target_ulong hstatus;
> >> > target_ulong hedeleg;
> >>
Re: [PATCH v1 15/36] target/riscv: Convert mstatus to pointers
On Tue, 21 Jan 2020 11:02:01 GMT (+), alistai...@gmail.com 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.
OK, so in the interest of moving things forwards let's just
Reviewed-by: Palmer Dabbelt
so we can merge this -- it's too big of a patch set to wait around on something
so small for. I think that was the last one missing a review, right?
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 the "real" versions that the pointer
> + * versions point to. These should never be used unless you know what you
> + * are doing. To access these use the pointer versions instead. This is
> + * required to handle the Hypervisor register swapping.
> + */
> +target_ulong mstatus_novirt;
> +
> /* Hypervisor CSRs */
> target_ulong hstatus;
> target_ulong hedeleg;
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index b00f66824a..9684da7f7d 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -37,8 +37,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
> #ifndef CONFIG_USER_ONLY
> static int riscv_cpu_local_irq_pending(CPURISCVState *env)
> {
> -target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE);
> -target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE);
> +target_ulong mstatus_mie = get_field(*env->mstatus, MSTATUS_MIE);
> +target_ulong mstatus_sie = get_field(*env->mstatus, MSTATUS_SIE);
> target_ulong pending = env->mip & env->mie;
> target_ulong mie = env->priv < PRV_M || (env->priv == PRV_M &&
mstatus_mie);
> target_ulong sie = env->priv < PRV_S || (env->priv ==
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 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
Re: [PATCH v1 15/36] target/riscv: Convert mstatus to pointers
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
>> 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
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
Re: [PATCH v1 15/36] target/riscv: Convert mstatus to pointers
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 the "real" versions that the pointer
> > + * versions point to. These should never be used unless you know what
> > you
> > + * are doing. To access these use the pointer versions instead. This is
> > + * required to handle the Hypervisor register swapping.
> > + */
> > +target_ulong mstatus_novirt;
> > +
> > /* Hypervisor CSRs */
> > target_ulong hstatus;
> > target_ulong hedeleg;
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index b00f66824a..9684da7f7d 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -37,8 +37,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
> > #ifndef CONFIG_USER_ONLY
> > static int riscv_cpu_local_irq_pending(CPURISCVState *env)
> > {
> > -target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE);
> > -target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE);
> > +target_ulong mstatus_mie = get_field(*env->mstatus, MSTATUS_MIE);
> > +target_ulong mstatus_sie = get_field(*env->mstatus, MSTATUS_SIE);
> > target_ulong pending = env->mip & env->mie;
> > target_ulong mie = env->priv < PRV_M || (env->priv == PRV_M &&
> > mstatus_mie);
> > target_ulong sie = env->priv < PRV_S || (env->priv == PRV_S &&
> > mstatus_sie);
> > @@ -75,7 +75,7 @@ bool riscv_cpu_exec_interrupt(CPUState *cs,
Re: [PATCH v1 15/36] target/riscv: Convert mstatus to pointers
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.
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 the "real" versions that the pointer
+ * versions point to. These should never be used unless you know what you
+ * are doing. To access these use the pointer versions instead. This is
+ * required to handle the Hypervisor register swapping.
+ */
+target_ulong mstatus_novirt;
+
/* Hypervisor CSRs */
target_ulong hstatus;
target_ulong hedeleg;
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index b00f66824a..9684da7f7d 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -37,8 +37,8 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
#ifndef CONFIG_USER_ONLY
static int riscv_cpu_local_irq_pending(CPURISCVState *env)
{
-target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE);
-target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE);
+target_ulong mstatus_mie = get_field(*env->mstatus, MSTATUS_MIE);
+target_ulong mstatus_sie = get_field(*env->mstatus, MSTATUS_SIE);
target_ulong pending = env->mip & env->mie;
target_ulong mie = env->priv < PRV_M || (env->priv == PRV_M &&
mstatus_mie);
target_ulong sie = env->priv < PRV_S || (env->priv == PRV_S &&
mstatus_sie);
@@ -75,7 +75,7 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int
interrupt_request)
/* Return true is floating point support is currently enabled */
bool riscv_cpu_fp_enabled(CPURISCVState *env)
{
-if (env->mstatus & MSTATUS_FS) {
+if (*env->mstatus & MSTATUS_FS) {
return true;
}
@@ -198,8 +198,8 @@ static int get_physical_address(CPURISCVState *env, hwaddr
*physical,
int mode = mmu_idx;
if (mode == PRV_M && access_type != MMU_INST_FETCH) {
-if (get_field(env->mstatus, MSTATUS_MPRV)) {
-mode = get_field(env->mstatus, MSTATUS_MPP);
+if (get_field(*env->mstatus, MSTATUS_MPRV)) {
+mode = get_field(*env->mstatus, MSTATUS_MPP);
}
}
@@ -213,11 +213,11 @@ static int get_physical_address(CPURISCVState *env,
hwaddr *physical,
hwaddr base;
int levels, ptidxbits, ptesize, vm, sum;
-int mxr = get_field(env->mstatus,
