Re: [Qemu-riscv] [Qemu-devel] [PATCH for 4.1 v3] target/riscv: Expose time CSRs when allowed by [m|s]counteren

[Palmer Dabbelt]

On Thu, 27 Jun 2019 12:56:57 PDT (-0700), address@hidden wrote:
> On Wed, Jun 26, 2019 at 1:25 AM Palmer Dabbelt <address@hidden> wrote:
> > On Tue, 25 Jun 2019 23:58:34 PDT (-0700), address@hidden wrote:
> > > On Wed, Jun 26, 2019 at 4:23 AM Jonathan Behrens <address@hidden> 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 0xFFFFFFFF 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 :)

> 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