| Message ID | 1521584722-39133-1-git-send-email-mjc@sifive.com |
|---|---|
| State | New |
| Headers | show |
I had the branch all set up and ready for a PR, including the tag message, but after dropping the riscv_isa_string fix I noticed it was still in the tag blurb for the series. I don't think it is worth re-tagging the PR to add a note that we dropped the change from the series. These are pretty much all bug fixes that we have had in the riscv tree since the v8.2 PR from March 8th, although now they all have sign-offs and all outstanding review feedback has been addressed. Besides printing hex in the disassembler, this series contains bug fixes and code cleanup, although printing hex in the disassembler was used for debugging, so it could be considered a meta bug-fix. On Tue, Mar 20, 2018 at 3:25 PM, Michael Clark <mjc@sifive.com> 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) > > - ---------------------------------------------------------------- > 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 > > 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(-) > -----BEGIN PGP SIGNATURE----- > > iF0EARECAB0WIQR8mZMOsXzYugc9Xvpr8dezV+8+TwUCWrGJWgAKCRBr8dezV+8+ > Tw34AJ9QquvDTCdzOS6k2bfUvCppUaQ3KACghKuKfhjypUM7L5SM9CuvKPovfNc= > =SI2p > -----END PGP SIGNATURE----- >
On 20.03.2018 23:25, 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. Hi Michael, for future PULL request, could you please send out all patches again that should be pulled, and not send the cover letter alone? I.e. do a "git format-patch --subject-prefix PULL ..." and then replace the contents of the cover letter with the output of "git request-pull". ... this is how all other maintainers are sending PULL requests (at least as far as I know) and this way everybody gets a very final chance to have a look at the patches in their final state (it does not happen very often that someone replies to such patches, but in some rare cases we already found problems this way). Thank you very much for your efforts, Thomas
Le 21/03/2018 à 08:05, Thomas Huth a écrit : > On 20.03.2018 23:25, 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. > > Hi Michael, > > for future PULL request, could you please send out all patches again > that should be pulled, and not send the cover letter alone? I.e. do a > "git format-patch --subject-prefix PULL ..." and then replace the > contents of the cover letter with the output of "git request-pull". > > ... this is how all other maintainers are sending PULL requests (at > least as far as I know) and this way everybody gets a very final chance > to have a look at the patches in their final state (it does not happen > very often that someone replies to such patches, but in some rare cases > we already found problems this way). You can also use git-publish that is a very simple tool to manage patch series revision and to send pull request in an easy way. https://github.com/stefanha/git-publish Thanks, Laurent
On 21/03/2018 08:05, Thomas Huth wrote: > On 20.03.2018 23:25, 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. > > Hi Michael, > > for future PULL request, could you please send out all patches again > that should be pulled, and not send the cover letter alone? I.e. do a > "git format-patch --subject-prefix PULL ..." and then replace the > contents of the cover letter with the output of "git request-pull". > > ... this is how all other maintainers are sending PULL requests (at > least as far as I know) and this way everybody gets a very final chance > to have a look at the patches in their final state (it does not happen > very often that someone replies to such patches, but in some rare cases > we already found problems this way). In fact, I already explained on the mailing list that this patch: RISC-V: Hold rcu_read_lock when accessing memory is fixing the wrong place, so please remove it from the pull request too. Thanks, Paolo
On Wed, Mar 21, 2018 at 4:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 21/03/2018 08:05, Thomas Huth wrote: > > On 20.03.2018 23:25, Michael Clark wrote: > >> 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 e1a247183ac0816284dfda61423f70 > 30b6686679: > >> > >> 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. > > > > Hi Michael, > > > > for future PULL request, could you please send out all patches again > > that should be pulled, and not send the cover letter alone? I.e. do a > > "git format-patch --subject-prefix PULL ..." and then replace the > > contents of the cover letter with the output of "git request-pull". > > > > ... this is how all other maintainers are sending PULL requests (at > > least as far as I know) and this way everybody gets a very final chance > > to have a look at the patches in their final state (it does not happen > > very often that someone replies to such patches, but in some rare cases > > we already found problems this way). > > In fact, I already explained on the mailing list that this patch: > > RISC-V: Hold rcu_read_lock when accessing memory > > is fixing the wrong place, so please remove it from the pull request too. > Okay. I really wasn't sure whether it was wrong at the time but now I know. I'll drop that patch.
On Wed, Mar 21, 2018 at 12:05 AM, Thomas Huth <thuth@redhat.com> wrote: > On 20.03.2018 23:25, Michael Clark wrote: > > 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) > > > > ---------------------------------------------------------------- > > 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. > > Hi Michael, > > for future PULL request, could you please send out all patches again > that should be pulled, and not send the cover letter alone? I.e. do a > "git format-patch --subject-prefix PULL ..." and then replace the > contents of the cover letter with the output of "git request-pull". > No problem. Thanks for the advice. I was grappling with this part of the process. This is now very clear and makes complete sense. > ... this is how all other maintainers are sending PULL requests (at > least as far as I know) and this way everybody gets a very final chance > to have a look at the patches in their final state (it does not happen > very often that someone replies to such patches, but in some rare cases > we already found problems this way). > > Thank you very much for your efforts, > Thomas > >
On 03/21/2018 01:27 PM, Michael Clark wrote: >> for future PULL request, could you please send out all patches again >> that should be pulled, and not send the cover letter alone? I.e. do a >> "git format-patch --subject-prefix PULL ..." and then replace the >> contents of the cover letter with the output of "git request-pull". >> > > No problem. Thanks for the advice. I was grappling with this part of the > process. This is now very clear and makes complete sense. You'll notice that some maintainers send a full PULL request the first time, but if they have to send a v2, they only include the patches that changed from v1. That's also okay (there's still an email for each patch in the series as it eventually got committed, even if it is now split across threads; but that is sufficient for someone performing a bisect to have an easy email to reply to when reporting a regression caused by the final version that ended up getting pulled). For an example of that, see my QAPI v4 request: https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05160.html and the regression report against the v1 request (since that particular patch did not change between v1 and v4): https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg03611.html https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05709.html
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(-) >
On Thu, Mar 22, 2018 at 2:56 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Hi Michael, > > On 03/20/2018 07:25 PM, Michael Clark wrote: > > 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) > > > > ---------------------------------------------------------------- > > 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." > Re 2). I am listed as a maintainer for hw/riscv and target/riscv which is the only area these patches touch and these patches have been in the riscv.org tree for quite some time. Indeed, most folk are running RISC-V QEMU from the riscv.org tree as it has previously been the only way to access the port before it very recently made it upstream. $ sed -n '/RISC-V/,/^$/p' MAINTAINERS RISC-V M: Michael Clark <mjc@sifive.com> M: Palmer Dabbelt <palmer@sifive.com> M: Sagar Karandikar <sagark@eecs.berkeley.edu> M: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> S: Maintained F: target/riscv/ F: hw/riscv/ F: include/hw/riscv/ F: disas/riscv.c Besides some trivial cleanups (erroneous comments, dead-code), and the cpu init work we were asked to work on by Peter Maydell, the focus of the changes are specification conformance. e.g. cases where we were trapping on CSR accesses when we should return 0 or cases where the page walker didn't comply with the specification. To review these, it will require a thorough reading of the RISC-V Privileged ISA Specification: - https://riscv.org/specifications/privileged-isa/ Given i'm the primary active RISC-V QEMU maintainer then its mostly my responsibility that we conform with the RISC-V specifications. In time we will have tests for these corner cases in page walker, and control and status registers on processor variants that implement different subsets of the specification (such as SiFive's E series that has no MMU and no S mode, versus the SiFive U series which implements S mode and has an MMU), but at this stage it requires a very careful reading of the RISC-V ISA Privileged ISA Specification. We will more changes and the patch queue will only grow longer. Soon we will be trapping on s* CSRs if misa.S is not set and we'll add RISCV_FEATURE_MMU_HW_AD so that we can distinguish processors that handle PTE AD bits in hardware vs processors that defer PTE AD bit handling to software. These can only be reviewed by someone who is familar with the RISC-V Privileged ISA specification. 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. > Sure take your time, however do consider that we should be able to fix bugs as a maintainer for RISC-V. I'm very able to separate well-tested changes from experimental code. e.g - https://github.com/michaeljclark/riscv-qemu/tree/wip-riscv-tcg-backend It will be some time before we submit patches for the RISC-V backend. It needs to be rock-solid before I would consider submitting it. In any case, folk are mostly using the riscv-all branch on the riscv.org tree for the RISC-V QEMU port, so it's not an immediate concern. It just means that riscv.org QEMU is more likely to be RISC-V compliant than qemu.org QEMU, assuming specification compliance fixes aren't accepted upstream. That said, i'd appreciate careful external review, but the most appropriate source are the RISC-V instruction set architects. That was my two reviewer's cents. Thanks for your help with review. I really appreciate it. The only problem we have that is not your fault, nor your responsibility, is that the really hard to review patches, that don't have review other than by the "active" maintainer, may never actually receive review unless someone dedicates the time to finding the specific paragraphs within the RISC-V ISA Privileged ISA Specification. The focus of the series, after all, besides the cleanups is on specification conformance bugs. So they may need to be accepted under category 2), i.e. are in the maintainers tree. I'm going to ask around for some folk with RISC-V domain knowledge to review the remaining patches. Thanks for your help so far... > > 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(-) > > > >
On 22 March 2018 at 18:26, Michael Clark <mjc@sifive.com> wrote: > Besides some trivial cleanups (erroneous comments, dead-code), and the cpu > init work we were asked to work on by Peter Maydell, the focus of the > changes are specification conformance. e.g. cases where we were trapping on > CSR accesses when we should return 0 or cases where the page walker didn't > comply with the specification. To review these, it will require a thorough > reading of the RISC-V Privileged ISA Specification: > > - https://riscv.org/specifications/privileged-isa/ > > Given i'm the primary active RISC-V QEMU maintainer then its mostly my > responsibility that we conform with the RISC-V specifications. Right, but it is also your responsibility to make the best effort you can to make your code available for review. Sometimes there are patches that go out on the list and just don't get any review, and you have to make a judgement call about whether you think they're solid enough to go in anyway. But the ideal is that those are not very common, and you should allow plenty of time and several "pings for review" before doing that. I am at the moment allowing you some extra slack because risc-v support is new to QEMU and delay at this point would risk these things not getting into 2.12. If we were not currently in freeze I would take the approach of asking you to submit just the reviewed patches and let the others have more time on-list for review. > In time we > will have tests for these corner cases in page walker, and control and > status registers on processor variants that implement different subsets of > the specification (such as SiFive's E series that has no MMU and no S mode, > versus the SiFive U series which implements S mode and has an MMU), but at > this stage it requires a very careful reading of the RISC-V ISA Privileged > ISA Specification. > > We will more changes and the patch queue will only grow longer. Soon we will > be trapping on s* CSRs if misa.S is not set and we'll add > RISCV_FEATURE_MMU_HW_AD so that we can distinguish processors that handle > PTE AD bits in hardware vs processors that defer PTE AD bit handling to > software. These can only be reviewed by someone who is familar with the > RISC-V Privileged ISA specification. Pretty much all of our target frontends require at least some familiarity with the relevant ISA specifications, yes. (You can get a long way with knowing ISAs in general and having the manual, though. We have several developers who have reviewed code for multiple different supported guest architectures.) Code review isn't only about "does this behave as the secification requires". It can also catch: * simple logic bugs * places where the code is more complicated than it needs to be * style issues * places where a QEMU utility routine could have been used * misunderstandings about QEMU internal APIs * memory leaks or memory corruption bugs Review by somebody who is familiar with QEMU is just as useful as review by somebody familiar with the target ISA. >> 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. > > > Sure take your time, however do consider that we should be able to fix bugs > as a maintainer for RISC-V. I'm very able to separate well-tested changes > from experimental code. e.g > > - https://github.com/michaeljclark/riscv-qemu/tree/wip-riscv-tcg-backend > > It will be some time before we submit patches for the RISC-V backend. It > needs to be rock-solid before I would consider submitting it. This is perhaps not the best possible approach. You will find that it's easier to get code into QEMU (and many upstream projects) if you do the development as a part of that upstream. So for instance you can submit "RFC" patches if they're work in progress, or initial versions of a patchset which implement a basic working version with optimizations to follow. (You just need to be clear in a patchset cover letter what is/isn't implemented, what you're asking for review on, and so on.) If you do that then you won't find yourself in the position of submitting what you think is a complete finished feature and being asked to do significant rework. You also are less likely find yourself two weeks before a codefreeze with a large pile of unreviewed and uncommitted changes and having huge difficulty getting your code into the tree. In my experience the best way to work is "upstream first" and incrementally, so that large features and bugfixes all go onto the list, get reviewed and then go into upstream in small parts over the whole QEMU development cycle. It's often possible to put not-yet-finished work behind a feature-bit gate that means that it doesn't appear to guests until it's finished and the feature-bit is enabled. Working incrementally also significantly improves the odds of your code getting prompt review. A 20 patch patchset is a large chunk of work to review, and is likely to be put off or ignored. Four separate 5 patch sets sent out over a week or two are much easier for reviewers to digest, and for people to review even if risc-v stuff isn't their primary focus. If your queue of unsubmitted or unreviewed patches is steadily getting longer then something is going wrong; you should be aiming for it to be as short as possible. thanks -- PMM
On 03/22/2018 02:10 PM, Peter Maydell wrote: [snip lots of good advice] > Code review isn't only about "does this behave as the > secification requires". It can also catch: > * simple logic bugs > * places where the code is more complicated than it needs to be > * style issues > * places where a QEMU utility routine could have been used > * misunderstandings about QEMU internal APIs > * memory leaks or memory corruption bugs * grammar/typo fixes in comments/documentation/user-visible error strings > Review by somebody who is familiar with QEMU is just as useful > as review by somebody familiar with the target ISA. Indeed. Not everyone is an expert on every subject, but the more people that get involved, each in their own area of expertise, the wider the review coverage can be for a better end product. And I'm very much okay with a reviewer stating up front "I'm only commenting on aspect XYZ, but will leave the rest of your patch to someone more familiar with ABC" - even that admission of a partial review is still helpful. > If your queue of unsubmitted or unreviewed patches is steadily > getting longer then something is going wrong; you should be > aiming for it to be as short as possible. More on this point - typically, the review process is OFTEN the long pole in open source projects, where patches are submitted faster than reviewers can keep up. While the following advice is by no means a requirement that everyone should follow, my personal goal is that I review at least two patches for every one that I submit, to help alleviate the review bottleneck. And doing so has made ME a better programmer, as I get to explore coding paradigms and portions of the tree that I was previously unfamiliar with. Furthermore, providing reviews to other's patches in addition to writing your own patches has some nice benefits: you gain some name recognition in the community, and it is obvious that you care more about the project as a whole than just about getting your stuff merged. That in turn tends to have an interesting effect that your own patches get reviewed faster.
On 20 March 2018 at 22:25, Michael Clark <mjc@sifive.com> wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > 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) 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. thanks -- PMM
On Fri, Mar 23, 2018 at 3:20 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 20 March 2018 at 22:25, Michael Clark <mjc@sifive.com> 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.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 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 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(-) -----BEGIN PGP SIGNATURE----- iF0EARECAB0WIQR8mZMOsXzYugc9Xvpr8dezV+8+TwUCWrGJWgAKCRBr8dezV+8+ Tw34AJ9QquvDTCdzOS6k2bfUvCppUaQ3KACghKuKfhjypUM7L5SM9CuvKPovfNc= =SI2p -----END PGP SIGNATURE-----