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

[Philippe Mathieu-Daudé]

Hi Michael,

On 03/20/2018 07:25 PM, Michael Clark wrote:
> The following changes since commit f1a63fcfcd92c88be8942b5ae71aef9749a4f135:
> 
>   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)
> 
> ----------------------------------------------------------------
> This is a series of spec conformance bug fixes and code cleanups
> that we would like to get in before the QEMU 2.12 release.
> 
> * Implements WARL behavior for CSRs that don't support writes
> * Improves specification conformance of the page table walker
>   * Change access checks from ternary operator to if statements
>   * Checks for misaligned PPNs
>   * Disallow M-mode or S-mode from fetching from User pages
>   * Adds reserved PTE flag check: W or W|X
>   * Set READ flag for PTE X flag if mstatus.mxr is in effect
>   * Improves page walker comments and general readability
> * Several trivial code cleanups to hw/riscv
>   * Replacing hard coded constants with reference to enums
>     or the machine memory maps.
>   * Remove unnecessary class initialization boilerplate
> * Adds bounds checks when writing device-tree to ROM
> * Updates the cpu model to use a more modern interface
> * Sets mtval/stval to zero on exceptions without addresses
> * Fixes memory allocation bug in riscv_isa_string
> 
> v4
> 
> * added fix for memory allocation bug in riscv_isa_string
> 
> v3
> 
> * refactor rcu_read_lock in PTE update to use single unlock
> * mstatus.mxr is in effect regardless of privilege mode
> * remove unnecessary class init from riscv_hart
> * set mtval/stval to zero on exceptions without addresses
> 
> v2
> 
> * remove unused class boilerplate retains qom parent_obj
> * convert cpu definition towards future model
> * honor mstatus.mxr flag in page table walker
> 
> v1
> 
> * initial post merge cleanup patch series
> 
> ----------------------------------------------------------------
> Michael Clark (25):
>       RISC-V: Make virt create_fdt interface consistent
>       RISC-V: Replace hardcoded constants with enum values
>       RISC-V: Make virt board description match spike
>       RISC-V: Use ROM base address and size from memmap
>       RISC-V: Remove identity_translate from load_elf
>       RISC-V: Mark ROM read-only after copying in code
>       RISC-V: Remove unused class definitions
>       RISC-V: Make sure rom has space for fdt
>       RISC-V: Include intruction hex in disassembly
>       RISC-V: Hold rcu_read_lock when accessing memory
>       RISC-V: Improve page table walker spec compliance
>       RISC-V: Update E order and I extension order
>       RISC-V: Make some header guards more specific
>       RISC-V: Make virt header comment title consistent
>       RISC-V: Use memory_region_is_ram in pte update
>       RISC-V: Remove EM_RISCV ELF_MACHINE indirection
>       RISC-V: Hardwire satp to 0 for no-mmu case
>       RISC-V: Remove braces from satp case statement
>       RISC-V: riscv-qemu port supports sv39 and sv48
>       RISC-V: vectored traps are optional
>       RISC-V: No traps on writes to misa,minstret,mcycle
>       RISC-V: Remove support for adhoc X_COP interrupt
>       RISC-V: Convert cpu definition towards future model
>       RISC-V: Clear mtval/stval on exceptions without info
>       RISC-V: Remove erroneous comment from translate.c


I'm not a maintainer, so I'll just give my reviewer point of view, since
I'm willing to review the RISC-V patches.

From https://wiki.qemu.org/Contribute/FAQ:

  "In order for your patch to be merged, it must either
  (1) receive a Reviewed-by by a trusted reviewer on qemu-devel or
  (2) be reviewed by a maintainer and accepted into their tree."

As I understand the review process, reviewers can:
- directly agree with your patches and add their R-b tag so you can pull
your patches,
- ask changes.
If the change is easy/obvious they can:
- give you the option to add their R-b tag if you agree and do their change,
- wait you respin another version, and review again.
(Normally when a reviewer commented on a patch, he will track your
respin, but it is safer to Cc: him due to the mass amount of traffic).

There are sometime misunderstanding and if the updated patch is not what
the reviewer suggested, he can still notify you in time.


In your 25 matches, only 12 have R-b tag.

You could have sent a PR of the reviewed patches, and respin the
unreviewed patches separately.

I also see you addressed some of my comments, so I'd have liked be able
to take time to review and eventually test your series.


That was my two reviewer's cents.

Regards,

Phil.

> 
>  disas/riscv.c                   |  39 +++++++------
>  hw/riscv/riscv_hart.c           |   6 --
>  hw/riscv/sifive_clint.c         |   9 +--
>  hw/riscv/sifive_e.c             |  34 +----------
>  hw/riscv/sifive_u.c             |  65 +++++++--------------
>  hw/riscv/spike.c                |  65 ++++++++-------------
>  hw/riscv/virt.c                 |  77 +++++++++----------------
>  include/hw/riscv/sifive_clint.h |   4 ++
>  include/hw/riscv/sifive_e.h     |   5 --
>  include/hw/riscv/sifive_u.h     |   9 ++-
>  include/hw/riscv/spike.h        |  15 ++---
>  include/hw/riscv/virt.h         |  17 +++---
>  target/riscv/cpu.c              | 125 
> ++++++++++++++++++++++------------------
>  target/riscv/cpu.h              |   6 +-
>  target/riscv/cpu_bits.h         |   3 -
>  target/riscv/helper.c           |  84 ++++++++++++++++++++-------
>  target/riscv/op_helper.c        |  52 ++++++++---------
>  target/riscv/translate.c        |   1 -
>  18 files changed, 281 insertions(+), 335 deletions(-)
>

signature.asc
Description: OpenPGP digital signature