qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [patches] Re: [PATCH v6 00/26] RISC-V: Fixes and cleanu


From: Michael Clark
Subject: Re: [Qemu-devel] [patches] Re: [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12
Date: Mon, 26 Mar 2018 16:14:01 -0700

On Mon, Mar 26, 2018 at 11:07 AM, Michael Clark <address@hidden> wrote:

>
>
> On Sun, Mar 25, 2018 at 8:03 AM, Peter Maydell <address@hidden>
> wrote:
>
>> On 24 March 2018 at 18:13, Michael Clark <address@hidden> wrote:
>> > This is a series of bug fixes and code cleanups that we would
>> > like to get in before the QEMU 2.12 release. We are respinning
>> > v6 of this series to include two new bug fixes. These changes
>> > are present in the downstream riscv.org riscv-all branch:
>> >
>> > - https://github.com/riscv/riscv-qemu/commits/riscv-all
>> >
>> > This series also addresses post-merge feedback such as updating
>> > the cpu initialization model to conform with other architectures
>> > as requested by Igor Mammedov.
>>
>> Hi. It looks to me like a fair number of these patches
>> are already reviewed, so we don't need to wait on the
>> rest being reviewed to get those into master.
>>
>> My suggestion is that you send a pullrequest now for the
>> reviewed patches, and send a patchset for review for the
>> new ones or the ones that still need review. (If there
>> are patches that are reviewed but depend on earlier ones
>> that need to go in set 2 then they go in set 2 as well.)
>>
>
> Unfortunately the reviewed patches are mostly just minor cleanups. It's
> almost not worth making a PR for them as *none* of the reviewed patches are
> actually bug fixes. They are things like removing unused definitions or
> replacing hardcoded constants with enums, removing unnesscary braces, etc,
> etc
>

Apologies. There is one bug fix in the subset of reviewed patches. the -cpu
model changes.


> $ grep Reviewed outgoing/v6-00*
> outgoing/v6-0001-RISC-V-Make-virt-create_fdt-interface-consistent.patch:Reviewed-by:
> Philippe Mathieu-Daudé <address@hidden>
> outgoing/v6-0002-RISC-V-Replace-hardcoded-constants-
> with-enum-valu.patch:Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> outgoing/v6-0003-RISC-V-Make-virt-board-description-match-spike.patch:Reviewed-by:
> Philippe Mathieu-Daudé <address@hidden>
> outgoing/v6-0004-RISC-V-Use-ROM-base-address-and-size-from-memmap.patch:Reviewed-by:
> Philippe Mathieu-Daudé <address@hidden>
> outgoing/v6-0005-RISC-V-Remove-identity_translate-
> from-load_elf.patch:Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> outgoing/v6-0007-RISC-V-Remove-unused-class-definitions.patch:Reviewed-by:
> Philippe Mathieu-Daudé <address@hidden>
> outgoing/v6-0009-RISC-V-Include-intruction-hex-in-disassembly.patch:Reviewed-by:
> Philippe Mathieu-Daudé <address@hidden>
> outgoing/v6-0012-RISC-V-Make-some-header-guards-more-specific.patch:Reviewed-by:
> Philippe Mathieu-Daudé <address@hidden>
> outgoing/v6-0013-RISC-V-Make-virt-header-comment-title-consistent.patch:Reviewed-by:
> Philippe Mathieu-Daudé <address@hidden>
> outgoing/v6-0015-RISC-V-Remove-EM_RISCV-ELF_MACHINE-indirection.patch:Reviewed-by:
> Philippe Mathieu-Daudé <address@hidden>
> outgoing/v6-0017-RISC-V-Remove-braces-from-satp-case-statement.patch:Reviewed-by:
> Philippe Mathieu-Daudé <address@hidden>
> outgoing/v6-0022-RISC-V-Convert-cpu-definition-towards-future-mode.patch:Reviewed-by:
> Philippe Mathieu-Daudé <address@hidden>
> outgoing/v6-0022-RISC-V-Convert-cpu-definition-towards-future-mode.patch:Reviewed-by:
> Igor Mammedov <address@hidden>
>
> The unreviewed patches, as I've mentioned many times are the ones that
> require reading the RISC-V Privileged ISA Specification or are actual bug
> fixes and hence are harder to review. They are in the maintainer's tree and
> are what folk who are interested in using RISC-V in QEMU are actually
> running.
>
> I can drop the fix to make ROM read-only it is not critical, however it is
> a bug fix. I went through with a critical eye and reviewed them myself and
> picked up a few minor issues, but I believe the patchset as a whole should
> be fine as long as I can find someone to Ack them. Otherwise we're sort of
> in a Catch-22 situation.
>

Most of the spec compliance bug fixes are innocuous when it comes to
running Linux, nevertheless, they are bugs and will be exposed by future
verification and compliance tests. The only really critical bug fix is
26/26 mstatus.FS. Igor's fix for the cpu model 22/26 is important as it
addresses remaining review feedback with the initial port submission. 25/26
is a user-visible bugfix for the disassembler which is important for anyone
using -d in_asm. I'm pretty comfortable with the amount of testing this
series has had despite the lack of review. Testing is also important.

The FDT ROM truncation issue fixed by 8/26 isn't seen in practice but it
was seen during development as i've been working on patches to separate the
bbl bootloader from the linux kernel (currently the kernel is embedded as a
payload in embedded in the monitor firmware). I would like to change the
ports to use -bios to load firmware and -kernel can then point to a regular
linux kernel and we can indicate to the firmware where the kernel is loaded
using a device-tree chosen entry (as some other ports do). That work is not
included in this series.

C=cleanup, B=bugfix, I=important bugfix, S=spec compliance bugfix, X=critical
bugfix, R=reviewed

C R [PATCH v6 01/26] RISC-V: Make virt create_fdt interface consistent
C R [PATCH v6 02/26] RISC-V: Replace hardcoded constants with enum values
C R [PATCH v6 03/26] RISC-V: Make virt board description match spike
C R [PATCH v6 04/26] RISC-V: Use ROM base address and size from memmap
C R [PATCH v6 05/26] RISC-V: Remove identity_translate from load_elf
B - [PATCH v6 06/26] RISC-V: Mark ROM read-only after copying in code
C R [PATCH v6 07/26] RISC-V: Remove unused class definitions
B - [PATCH v6 08/26] RISC-V: Make sure rom has space for fdt
C R [PATCH v6 09/26] RISC-V: Include instruction hex in disassembly
S - [PATCH v6 10/26] RISC-V: Improve page table walker spec compliance
S - [PATCH v6 11/26] RISC-V: Update E order and I extension order
C R [PATCH v6 12/26] RISC-V: Make some header guards more specific
C R [PATCH v6 13/26] RISC-V: Make virt header comment title consistent
B - [PATCH v6 14/26] RISC-V: Use memory_region_is_ram in pte update
C R [PATCH v6 15/26] RISC-V: Remove EM_RISCV ELF_MACHINE indirection
S - [PATCH v6 16/26] RISC-V: Hardwire satp to 0 for no-mmu case
C R [PATCH v6 17/26] RISC-V: Remove braces from satp case statement
B - [PATCH v6 18/26] RISC-V: riscv-qemu port supports sv39 and sv48
S - [PATCH v6 19/26] RISC-V: vectored traps are optional
S - [PATCH v6 20/26] RISC-V: No traps on writes to misa,minstret,mcycle
C - [PATCH v6 21/26] RISC-V: Remove support for adhoc X_COP interrupt
I R [PATCH v6 22/26] RISC-V: Convert cpu definition towards future model
B - [PATCH v6 23/26] RISC-V: Clear mtval/stval on exceptions without info
C - [PATCH v6 24/26] RISC-V: Remove erroneous comment from translate.c
I - [PATCH v6 25/26] RISC-V: Fix incorrect disassembly for addiw
X - [PATCH v6 26/26] RISC-V: Workaround for critical mstatus.FS MTTCG bug



> 26 patches is a lot to still be carrying around much
>> beyond rc1, so I would like to see the size of this set
>> reducing rather than increasing. As the release process
>> moves forward the bar for "can this still go in" gradually
>> goes up -- by about rc3 it is at about "is this a
>> really critical bug or regression from the previous
>> release".
>>
>> (Also something seems to have unhelpfully decided to eat
>> or delay about half of your emails in this patchset :-(
>> Patchew only sees 14 of the 26. Our mailing list server
>> does seem to do that occasionally so that would be my
>> first guess at the culprit, but it's possible it's
>> something at your end.)
>>
>
> Phil asked that I send out only the patches that don't have review, so
> that's what I did.
>


reply via email to

[Prev in Thread] Current Thread [Next in Thread]