Re: [Qemu-devel] [PULL 00/25] RISC-V Post-merge spec conformance and cleanup

[Michael Clark]

On Fri, Mar 23, 2018 at 3:20 AM, Peter Maydell <address@hidden>
wrote:

> On 20 March 2018 at 22:25, Michael Clark <address@hidden> wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> > The following changes since commit f1a63fcfcd92c88be8942b5ae71aef
> 9749a4f135:
> >
> >   Update version for v2.12.0-rc0 release (2018-03-20 19:04:22 +0000)
> >
> > are available in the git repository at:
> >
> >   https://github.com/riscv/riscv-qemu.git tags/riscv-qemu-2.12-fixes
> >
> > for you to fetch changes up to e1a247183ac0816284dfda61423f7030b6686679:
> >
> >   RISC-V: Remove erroneous comment from translate.c (2018-03-20 14:34:13
> -0700)
>
> I know this pull request needs a respin, but I gave it a test build
> run anyway to see if there were any issues thrown up by that (there
> weren't, which is great).
>
> I did notice that the tag name you quote here,
> "tags/riscv-qemu-2.12-fixes",
> doesn't seem to be a tag pushed to your git repo. I tested with
> "tags/riscv-qemu-2.12-fixes-v5" which I'm guessing is what you meant.
> You probably want to have a look into the workflow which generates
> these pullreq emails though to ensure it's creating them with the
> tag names that you intend, though.
>

I'll make sure I have the version number on the tag in future PRs.

I just sent a v6 series which includes two new bug fixes, and all of the
unreviewed patches as requested by Phil.

It's interesting to note that the reviewed patches are the cleanups which
are not critical and the unreviewed patches are mostly the actual bug
fixes, and likely require a reading of the specification. The bug fixes are
of course harder to review. I might ask around to see if we can find a
reviewer familar with RISC-V. The CSR return 0 vs trap behaviour is
something I discussed with Andrew Waterman and there were issues raised on
the riscv-isa-manual github repo as we are trying to clarify the behaviour
or MMU, no-MMU; S-mode vs no S-mode, etc. I have more fixes planned as we
try to more accurately emulate the MCU class cores with no-MMU but I don't
have the bandwidth at present to make all the required changes. i.e. cores
that don't report misa.S should trap on all s* CSRs but cores with misa.S
with no-MMU should just return 0 for satp (Supervisor Address Translation
Pointer) and ignore writes. The page walker changes are also made after a
closer reading of the specification i.e. handling mstatus.MXR permissions
and various other subtleties. There are also bounds checks for device-tree,
which don't actually show up in practice likely because the physical memory
write routines appear to silently handle bounds overruns. I added some
printfs and discovered the device tree size was larger than the ROM, so the
ROM space has been increased and bounds checks have been added.

And of course we have added the critical workaround for the mstatus.FS
problem, after checking that the workaround behaviour is legal, which it is.

26/26 in the series is what I would refer to as a critical fix. Of course
we'd like to get all of the changes in, as I believe the Fedora folk have
been using QEMU with this series applied. Indeed 26/26 is a derivative of
Richard W.M. Jones fix for the fedora rpm, however I've made it checkpatch
compliant and added comments describing the workaround.

I've cc'd Jim, while he works on GCC, he has helped with some spec
conformance bug fixes in QEMU, although not to do with the page walker. I
had added Palmer's sign-off to some of the patches after speaking with him,
as I sit near to him, so I guess he can't review them, but he could review
the latest fixes which don't have his sign-off. I'll try and find somebody
next week who doesn't mind looking at the Privileged Spec and
riscv-isa-manual issue tracker discussions to review the spec related bug
fixes... It would be nice to get this series in to QEMU 2.12...

Thanks,
Michael.