Re: [Qemu-devel] [patches] [PATCH v8 10/35] RISC-V: Remove erroneous comment from translate.c
On Wed, 25 Apr 2018 16:45:13 PDT (-0700), Michael Clark wrote: Cc: Sagar KarandikarCc: Bastian Koppelmann Cc: Palmer Dabbelt Cc: Alistair Francis Signed-off-by: Michael Clark --- target/riscv/translate.c | 1 - 1 file changed, 1 deletion(-) diff --git a/target/riscv/translate.c b/target/riscv/translate.c index 808eab7..c3a029a 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -280,7 +280,6 @@ static void gen_arith(DisasContext *ctx, uint32_t opc, int rd, int rs1, tcg_gen_andi_tl(source2, source2, 0x1F); tcg_gen_sar_tl(source1, source1, source2); break; -/* fall through to SRA */ #endif case OPC_RISC_SRA: tcg_gen_andi_tl(source2, source2, TARGET_LONG_BITS - 1); Reviewed-by: Palmer Dabbelt
Re: [Qemu-devel] [patches] [PATCH v1 1/1] RISC-V: Workaround for critical mstatus.FS MTTCG bug
On Tue, 27 Mar 2018 12:54:47 PDT (-0700), Michael Clark wrote: This change is a workaround for a bug where mstatus.FS is not correctly reporting dirty when MTTCG and SMP are enabled which results in the floating point register file not being saved during context switches. This a critical bug for RISC-V in QEMU as it results in floating point register file corruption when running SMP Linux in the RISC-V 'virt' machine. This workaround will return dirty if mstatus.FS is switched from off to initial or clean. We have checked the specification and it is legal for an implementation to return either off, or dirty, if set to initial or clean. This workaround will result in unnecessary floating point save restore. When mstatus.FS is off, floating point instruction trap to indicate the process is using the FPU. The OS can then save floating-point state of the previous process using the FPU and set mstatus.FS to initial or clean. With this workaround, mstatus.FS will always return dirty if set to a non-zero value, indicating floating point save restore is necessary, versus misreporting mstatus.FS resulting in floating point register file corruption. Cc: Palmer DabbeltCc: Sagar Karandikar Cc: Bastian Koppelmann Cc: Peter Maydell Tested-by: Richard W.M. Jones Signed-off-by: Michael Clark --- target/riscv/op_helper.c | 19 +-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c index e34715d..7281b98 100644 --- a/target/riscv/op_helper.c +++ b/target/riscv/op_helper.c @@ -144,8 +144,23 @@ void csr_write_helper(CPURISCVState *env, target_ulong val_to_write, } mstatus = (mstatus & ~mask) | (val_to_write & mask); -int dirty = (mstatus & MSTATUS_FS) == MSTATUS_FS; -dirty |= (mstatus & MSTATUS_XS) == MSTATUS_XS; + +/* Note: this is a workaround for an issue where mstatus.FS + does not report dirty when SMP and MTTCG is enabled. This + workaround is technically compliant with the RISC-V Privileged + specification as it is legal to return only off, or dirty, + however this may cause unnecessary saves of floating point state. + Without this workaround, floating point state is not saved and + restored correctly when SMP and MTTCG is enabled, */ +if (qemu_tcg_mttcg_enabled()) { +/* FP is always dirty or off */ +if (mstatus & MSTATUS_FS) { +mstatus |= MSTATUS_FS; +} +} + +int dirty = ((mstatus & MSTATUS_FS) == MSTATUS_FS) | +((mstatus & MSTATUS_XS) == MSTATUS_XS); mstatus = set_field(mstatus, MSTATUS_SD, dirty); env->mstatus = mstatus; break; FWIW, this isn't just "technically compliant with the RISC-V Privileged specification" but it's actually an intended design point. We're considering making this a bit more explicit in the ISA manual -- well, unless Andrew decides I'm being too pedantic in one of my possible readings of the spec :). Reviewed-By: Palmer Dabbelt
Re: [Qemu-devel] [patches] Re: [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12
On Tue, Mar 27, 2018 at 11:39 AM, Michael Clarkwrote: > > I will divide the series up into 3 branches, and move through them in > order of priority, with correctness ahead of tidyness: > > 1). riscv-qemu-2.12-critical-fixes > 2). riscv-qemu-2.13-bug-fixes > 3). riscv-qemu-2.13-tidy-ups > I think we need 4 categories: 1). riscv-qemu-2.12-critical-fixes - floating point register file corruption mstatus.FS 2). riscv-qemu-2.12-important-fixes - user visible bugs, e.g. Igor's -cpu list bug, wrong dissassembly for sext.w/addiw bug 3). riscv-qemu-2.13-bug-fixes - spec bugs and other innocuous bug fixes that are not /yet/ user visible. i.e. not exercised by RISC-V Linux 4). riscv-qemu-2.13-tidy-ups - code cleanups
Re: [Qemu-devel] [patches] Re: [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12
On Tue, Mar 27, 2018 at 2:42 AM, Peter Maydellwrote: > On 26 March 2018 at 19:07, Michael Clark wrote: > > On Sun, Mar 25, 2018 at 8:03 AM, Peter Maydell > > > wrote: > >> 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 > > No, what I'm saying is that it is very much worth it. You > want to shorten the size of your set of uncommitted patches. > Large pull requests increase the chances that some > random thing in there hits a compile issue or other minor > problem that means I have to bounce the whole thing and > you need to respin it. Smaller ones are more likely to > go in. This is especially true during the freeze part > of the release cycle, when we do an RC every week -- having > patches in earlier RCs reduces the risk. I do not want > to still see a 26 patch set unapplied by the time we get > to RC3 or RC4. > > Or if you don't think the minor cleanups are worth putting > into 2.12, that's fine too (it's a submaintainer judgement > you can make). In that case you can put those to one side > and trim down the size of the patchset you're sending out > (ie make it an 01/11...11/11 patchset or whatever). I'm not sure whether maintainer or submaintainer is really that relavant. Active maintainership is probably more relevant. i.e. responding to RISC-V related emails, PRs, issues on the riscv.org qemu repo, testing PRs before merging them, etc, etc. I'm going to focus on getting the critical bug fixes in for QEMU 2.12 i.e. the ones that break RISC-V Linux in QEMU 2.12 e.g. the mstatus.FS fix. User visible bugs like the disassembler bug and the -cpu list bug. I'm going to make a 3 patch series... possibly a 2 patch series... we can leave the disassembler bug there and just include Igor's change and the mstatus.FS workaround. I don't think writable ROM is really that critical, and bounds checks for potential device-tree truncation are just nice-to-haves. Spec conformance is nice-to-have if we are triaging against critical issues. Once we can ensure we have a working RISC-V port for QEMU 2.12 we can then worry about spec conformance bug fixes and tidy ups, perhaps for QEMU 2.13. My pesonal opinion is that Tested-by: should carry more weight than Reviewed-by: assuming Reviewed-by: only means someone has reviewed the code versus checking out and testing that a critical bug has been resolved by said patch. That said, if all QEMU patches need Reviewed-by: then there is not much we can do. In GCC and Linux, subsystem maintainers are allowed to make judgements over the inclusion of critical bug fixes. i.e. Reviewed-by: is not mandatory if the change is a critical fix. I will divide the series up into 3 branches, and move through them in order of priority, with correctness ahead of tidyness: 1). riscv-qemu-2.12-critical-fixes 2). riscv-qemu-2.13-bug-fixes 3). riscv-qemu-2.13-tidy-ups Expect to see riscv-qemu-2.12-critical-fixes very soon... > 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. > > I think that was a miscommunication. You should always > send out entire patchsets, not just parts of one. > Philippe said: > https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06038.html > "You could have sent a PR of the reviewed patches, and > respin the unreviewed patches separately.", which is the > same thing I'm suggesting here. My mistake.
Re: [Qemu-devel] [patches] Re: [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12
On Mon, Mar 26, 2018 at 04:45:34PM -0700, Michael Clark wrote: > I've made a tag for the series including the fixes from my own review > during the weekend (one logic fix and 2 comment of commit log typos, and a > patch hunk in the wrong commit): > > - https://github.com/riscv/riscv-qemu/releases/tag/riscv-qemu-2.12-fixes-v7 > > That said, if you want important or critical fixes only, then i'd suggest > these at least these 3: > > I R [PATCH v6 22/26] RISC-V: Convert cpu definition towards future model > 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 > > The other patches are the 23 patches which were accidentially included in > the initial pull on March 8, now with Signed-off-by, less the one that > Paolo asked me to drop. > > Most of the bugs are relatively innocuous except for the mstatus.FS fix. > Linux boots on upstream QEMU but has FPU register file corruption when SMP > is enabled. Even innocuous bug fixes are still welcome before rc2 is released. > I need advice as to which fixes you want to have included in QEMU 2.12. I'm > not sure if the reviewed set is the right set as it excludes 25/26 and more > importantly 26/26. We've got rc1 now, but you can still send pull requests for pretty much any kind of bug fix to get into the rc2 release, assuming the patches have a Reviewed-by, and are not excessively complicated or likely to cause regressions. After rc2 is out you want to only be sending important bug fixes, that users cannot wait for. After rc3 is out, ideally we won't find any more important bugs that need fixing. A bug would have to be really severe to justify including at that stage. Based on your description, patches 22/25/26 definitely still welcome in a pull request before rc2 or rc3. The more innocuous patches can also still be sent if you can do send a pull request for them pretty much straightaway before rc2. You'll obviously need to chase reviews for the few that miss them still. We're slightly unlucky in this release that the time between rc1 and rc2 falls over Easter weekend and Peter is on holiday, so has pretty limited time for dealing with pull requests. So you want to do what you can to minimize the work that Peter has to do, and minimize risk that a patch will fail build or automated testing, as that risks having the PR rejected. This is why it is worth sending patches in smaller groups. If you sent 20 patches at once and the last one fails, all 20 patches get rejected. If you send two batches of 10 you would at least get some of them off your plate, even if they are innocuous fixes. Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [patches] Re: [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12
On 26 March 2018 at 19:07, Michael Clarkwrote: > On Sun, Mar 25, 2018 at 8:03 AM, Peter Maydell > wrote: >> 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 No, what I'm saying is that it is very much worth it. You want to shorten the size of your set of uncommitted patches. Large pull requests increase the chances that some random thing in there hits a compile issue or other minor problem that means I have to bounce the whole thing and you need to respin it. Smaller ones are more likely to go in. This is especially true during the freeze part of the release cycle, when we do an RC every week -- having patches in earlier RCs reduces the risk. I do not want to still see a 26 patch set unapplied by the time we get to RC3 or RC4. Or if you don't think the minor cleanups are worth putting into 2.12, that's fine too (it's a submaintainer judgement you can make). In that case you can put those to one side and trim down the size of the patchset you're sending out (ie make it an 01/11...11/11 patchset or whatever). > 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. I think that was a miscommunication. You should always send out entire patchsets, not just parts of one. Philippe said: https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06038.html "You could have sent a PR of the reviewed patches, and respin the unreviewed patches separately.", which is the same thing I'm suggesting here. thanks -- PMM
Re: [Qemu-devel] [patches] Re: [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12
On Mon, Mar 26, 2018 at 4:14 PM, Michael Clarkwrote: > > > On Mon, Mar 26, 2018 at 11:07 AM, Michael Clark wrote: > >> >> >> On Sun, Mar 25, 2018 at 8:03 AM, Peter Maydell >> wrote: >> >>> On 24 March 2018 at 18:13, Michael Clark 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é >> outgoing/v6-0002-RISC-V-Replace-hardcoded-constants-with-enum-valu.patch:Reviewed-by: >> Philippe Mathieu-Daudé >> outgoing/v6-0003-RISC-V-Make-virt-board-description-match-spike.patch:Reviewed-by: >> Philippe Mathieu-Daudé >> outgoing/v6-0004-RISC-V-Use-ROM-base-address-and-size-from-memmap.patch:Reviewed-by: >> Philippe Mathieu-Daudé >> outgoing/v6-0005-RISC-V-Remove-identity_translate-from-load_elf.patch:Reviewed-by: >> Philippe Mathieu-Daudé >> outgoing/v6-0007-RISC-V-Remove-unused-class-definitions.patch:Reviewed-by: >> Philippe Mathieu-Daudé >> outgoing/v6-0009-RISC-V-Include-intruction-hex-in-disassembly.patch:Reviewed-by: >> Philippe Mathieu-Daudé >> outgoing/v6-0012-RISC-V-Make-some-header-guards-more-specific.patch:Reviewed-by: >> Philippe Mathieu-Daudé >> outgoing/v6-0013-RISC-V-Make-virt-header-comment-title-consistent.patch:Reviewed-by: >> Philippe Mathieu-Daudé >> outgoing/v6-0015-RISC-V-Remove-EM_RISCV-ELF_MACHINE-indirection.patch:Reviewed-by: >> Philippe Mathieu-Daudé >> outgoing/v6-0017-RISC-V-Remove-braces-from-satp-case-statement.patch:Reviewed-by: >> Philippe Mathieu-Daudé >> outgoing/v6-0022-RISC-V-Convert-cpu-definition-towards-future-mode.patch:Reviewed-by: >> Philippe Mathieu-Daudé >> outgoing/v6-0022-RISC-V-Convert-cpu-definition-towards-future-mode.patch:Reviewed-by: >> Igor Mammedov >> >> 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
Re: [Qemu-devel] [patches] Re: [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12
On Mon, Mar 26, 2018 at 11:07 AM, Michael Clarkwrote: > > > On Sun, Mar 25, 2018 at 8:03 AM, Peter Maydell > wrote: > >> On 24 March 2018 at 18:13, Michael Clark 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é > outgoing/v6-0002-RISC-V-Replace-hardcoded-constants- > with-enum-valu.patch:Reviewed-by: Philippe Mathieu-Daudé > outgoing/v6-0003-RISC-V-Make-virt-board-description-match-spike.patch:Reviewed-by: > Philippe Mathieu-Daudé > outgoing/v6-0004-RISC-V-Use-ROM-base-address-and-size-from-memmap.patch:Reviewed-by: > Philippe Mathieu-Daudé > outgoing/v6-0005-RISC-V-Remove-identity_translate- > from-load_elf.patch:Reviewed-by: Philippe Mathieu-Daudé > outgoing/v6-0007-RISC-V-Remove-unused-class-definitions.patch:Reviewed-by: > Philippe Mathieu-Daudé > outgoing/v6-0009-RISC-V-Include-intruction-hex-in-disassembly.patch:Reviewed-by: > Philippe Mathieu-Daudé > outgoing/v6-0012-RISC-V-Make-some-header-guards-more-specific.patch:Reviewed-by: > Philippe Mathieu-Daudé > outgoing/v6-0013-RISC-V-Make-virt-header-comment-title-consistent.patch:Reviewed-by: > Philippe Mathieu-Daudé > outgoing/v6-0015-RISC-V-Remove-EM_RISCV-ELF_MACHINE-indirection.patch:Reviewed-by: > Philippe Mathieu-Daudé > outgoing/v6-0017-RISC-V-Remove-braces-from-satp-case-statement.patch:Reviewed-by: > Philippe Mathieu-Daudé > outgoing/v6-0022-RISC-V-Convert-cpu-definition-towards-future-mode.patch:Reviewed-by: > Philippe Mathieu-Daudé > outgoing/v6-0022-RISC-V-Convert-cpu-definition-towards-future-mode.patch:Reviewed-by: > Igor Mammedov > > 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
Re: [Qemu-devel] [patches] Re: [PATCH v6 00/26] RISC-V: Fixes and cleanups for QEMU 2.12
On Sun, Mar 25, 2018 at 8:03 AM, Peter Maydellwrote: > On 24 March 2018 at 18:13, Michael Clark 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 $ grep Reviewed outgoing/v6-00* outgoing/v6-0001-RISC-V-Make-virt-create_fdt-interface-consistent.patch:Reviewed-by: Philippe Mathieu-Daudé outgoing/v6-0002-RISC-V-Replace-hardcoded-constants-with-enum-valu.patch:Reviewed-by: Philippe Mathieu-Daudé outgoing/v6-0003-RISC-V-Make-virt-board-description-match-spike.patch:Reviewed-by: Philippe Mathieu-Daudé outgoing/v6-0004-RISC-V-Use-ROM-base-address-and-size-from-memmap.patch:Reviewed-by: Philippe Mathieu-Daudé outgoing/v6-0005-RISC-V-Remove-identity_translate-from-load_elf.patch:Reviewed-by: Philippe Mathieu-Daudé outgoing/v6-0007-RISC-V-Remove-unused-class-definitions.patch:Reviewed-by: Philippe Mathieu-Daudé outgoing/v6-0009-RISC-V-Include-intruction-hex-in-disassembly.patch:Reviewed-by: Philippe Mathieu-Daudé outgoing/v6-0012-RISC-V-Make-some-header-guards-more-specific.patch:Reviewed-by: Philippe Mathieu-Daudé outgoing/v6-0013-RISC-V-Make-virt-header-comment-title-consistent.patch:Reviewed-by: Philippe Mathieu-Daudé outgoing/v6-0015-RISC-V-Remove-EM_RISCV-ELF_MACHINE-indirection.patch:Reviewed-by: Philippe Mathieu-Daudé outgoing/v6-0017-RISC-V-Remove-braces-from-satp-case-statement.patch:Reviewed-by: Philippe Mathieu-Daudé outgoing/v6-0022-RISC-V-Convert-cpu-definition-towards-future-mode.patch:Reviewed-by: Philippe Mathieu-Daudé outgoing/v6-0022-RISC-V-Convert-cpu-definition-towards-future-mode.patch:Reviewed-by: Igor Mammedov 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. 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.
Re: [Qemu-devel] [patches] Re: [PATCH v4 26/26] RISC-V: Fix riscv_isa_string memory size bug
On Tue, Mar 20, 2018 at 1:51 PM, Philippe Mathieu-Daudéwrote: > Le mar. 20 mars 2018 12:52, Peter Maydell a > écrit : > >> On 19 March 2018 at 21:18, Michael Clark wrote: >> > This version uses a constant size memory buffer sized for >> > the maximum possible ISA string length. It also uses g_new >> > instead of g_new0, uses more efficient logic to append >> > extensions and adds manual zero termination of the string. >> > >> > Cc: Palmer Dabbelt >> > Cc: Peter Maydell >> > Signed-off-by: Michael Clark >> > Reviewed-by: Philippe Mathieu-Daudé >> > --- >> > target/riscv/cpu.c | 12 ++-- >> > 1 file changed, 6 insertions(+), 6 deletions(-) >> > >> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> > index 1f25968..c82359f 100644 >> > --- a/target/riscv/cpu.c >> > +++ b/target/riscv/cpu.c >> > @@ -360,16 +360,16 @@ static void riscv_cpu_class_init(ObjectClass *c, >> void *data) >> > char *riscv_isa_string(RISCVCPU *cpu) >> > { >> > int i; >> > -size_t maxlen = 5 + ctz32(cpu->env.misa); >> > -char *isa_string = g_new0(char, maxlen); >> > -snprintf(isa_string, maxlen, "rv%d", TARGET_LONG_BITS); >> > +const size_t maxlen = sizeof("rv128") + sizeof(riscv_exts) + 1; >> > +char *isa_str = g_new(char, maxlen); >> > +char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", >> TARGET_LONG_BITS); >> > for (i = 0; i < sizeof(riscv_exts); i++) { >> > if (cpu->env.misa & RV(riscv_exts[i])) { >> > -isa_string[strlen(isa_string)] = riscv_exts[i] - 'A' + 'a'; >> > - >> > +*p++ = tolower(riscv_exts[i]); >> >> This should be qemu_tolower(). Plain tolower() has an awkward problem >> where if the 'char' type is signed and you hand tolower() a char that >> happens to be a negative value you get undefined behaviour. This means >> you need to cast the argument to 'unsigned char', which is what our >> > > Oh, good to know. > > wrapper does. In this specific case the inputs are known constant >> ASCII, so tolower() happens to be safe, > > > Yep. > > but for consistency with >> the rest of the codebase we should use our wrapper function. >> > > Ok. > > >> > } >> > } >> > -return isa_string; >> > +*p = '\0'; >> > +return isa_str; >> > } >> > >> > typedef struct RISCVCPUListState { >> >> Since this bug is causing the build tests to intermittently fail, >> I'm going to apply this patch directly to master, with tolower() >> replaced with qemu_tolower(). >> > > Thanks Peter! > Okay. I'll drop the patch from my post-merge spec conformance fixes series. I've addressed all feedback on the post-merge spec conformance series so i'll make a PR for it... ... these are the extra commits that were erroneously included in the v8.2 pull. They now all have sign-offs... they've been in the riscv tree for a while and have had a lot of testing...
Re: [Qemu-devel] [patches] Re: [PATCH v3 00/24] RISC-V Post-merge spec conformance and cleanup
On Fri, Mar 16, 2018 at 1:06 PM,wrote: > Hi, > > This series seems to have some coding style problems. See output below for > more information: > > Type: series > Message-id: 1521229281-73637-1-git-send-email-...@sifive.com > Subject: [Qemu-devel] [PATCH v3 00/24] RISC-V Post-merge spec conformance > and cleanup > > === TEST SCRIPT BEGIN === > #!/bin/bash > > BASE=base > n=1 > total=$(git log --oneline $BASE.. | wc -l) > failed=0 > > git config --local diff.renamelimit 0 > git config --local diff.renames True > git config --local diff.algorithm histogram > > commits="$(git log --format=%H --reverse $BASE..)" > for c in $commits; do > echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." > if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback > -; then > failed=1 > echo > fi > n=$((n+1)) > done > > exit $failed > === TEST SCRIPT END === > > Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 > From https://github.com/patchew-project/qemu > * [new tag] patchew/1521229281-73637-1- > git-send-email-...@sifive.com -> patchew/1521229281-73637-1- > git-send-email-...@sifive.com > Switched to a new branch 'test' > cf9ad8471c RISC-V: Clear mtval/stval on exceptions without info > e0eb5ad03f RISC-V: Convert cpu definition towards future model > cb986bd661 RISC-V: Remove support for adhoc X_COP interrupt > c1bea999fe RISC-V: No traps on writes to misa, minstret, mcycle > c45005f71d RISC-V: vectored traps are optional > 23dccf73aa RISC-V: riscv-qemu port supports sv39 and sv48 > 9beecddea0 RISC-V: Remove braces from satp case statement > db362a77b9 RISC-V: Hardwire satp to 0 for no-mmu case > c696060959 RISC-V: Remove EM_RISCV ELF_MACHINE indirection > e48c09d25a RISC-V: Use memory_region_is_ram in pte update > eaa01d4e61 RISC-V: Make virt header comment title consistent > 9435045fd8 RISC-V: Make some header guards more specific > 23973a84f7 RISC-V: Update E order and I extension order > 6efbb781e4 RISC-V: Improve page table walker spec compliance > 924e46b0b8 RISC-V: Hold rcu_read_lock when accessing memory > eef95b067f RISC-V: Include intruction hex in disassembly > 3884a14086 RISC-V: Make sure rom has space for fdt > 49ad774612 RISC-V: Remove unused class definitions > 4b771876d8 RISC-V: Mark ROM read-only after copying in code > 646a8d2508 RISC-V: Remove identity_translate from load_elf > c582b7bbf0 RISC-V: Use ROM base address and size from memmap > 8a3ef54b0d RISC-V: Make virt board description match spike > ba0cbace2d RISC-V: Replace hardcoded constants with enum values > ec0c0eea8c RISC-V: Make virt create_fdt interface consistent > > === OUTPUT BEGIN === > Checking PATCH 1/24: RISC-V: Make virt create_fdt interface consistent... > Checking PATCH 2/24: RISC-V: Replace hardcoded constants with enum > values... > Checking PATCH 3/24: RISC-V: Make virt board description match spike... > Checking PATCH 4/24: RISC-V: Use ROM base address and size from memmap... > Checking PATCH 5/24: RISC-V: Remove identity_translate from load_elf... > Checking PATCH 6/24: RISC-V: Mark ROM read-only after copying in code... > Checking PATCH 7/24: RISC-V: Remove unused class definitions... > Checking PATCH 8/24: RISC-V: Make sure rom has space for fdt... > Checking PATCH 9/24: RISC-V: Include intruction hex in disassembly... > Checking PATCH 10/24: RISC-V: Hold rcu_read_lock when accessing memory... > ERROR: switch and case should be at the same indent > #50: FILE: target/riscv/helper.c:240: > +switch (action) { > +case success: break; > +case translate_fail: return TRANSLATE_FAIL; > +case restart_walk: goto restart; > > ERROR: trailing statements should be on next line > #51: FILE: target/riscv/helper.c:241: > +case success: break; > > ERROR: trailing statements should be on next line > #53: FILE: target/riscv/helper.c:243: > +case restart_walk: goto restart; > > total: 3 errors, 0 warnings, 32 lines checked > > Your patch has style problems, please review. If any of these errors > are false positives report them to the maintainer, see > CHECKPATCH in MAINTAINERS. > > Checking PATCH 11/24: RISC-V: Improve page table walker spec compliance... > Checking PATCH 12/24: RISC-V: Update E order and I extension order... > Checking PATCH 13/24: RISC-V: Make some header guards more specific... > Checking PATCH 14/24: RISC-V: Make virt header comment title consistent... > Checking PATCH 15/24: RISC-V: Use memory_region_is_ram in pte update... > Checking PATCH 16/24: RISC-V: Remove EM_RISCV ELF_MACHINE indirection... > Checking PATCH 17/24: RISC-V: Hardwire satp to 0 for no-mmu case... > Checking PATCH 18/24: RISC-V: Remove braces from satp case statement... > Checking PATCH 19/24: RISC-V: riscv-qemu port supports sv39 and sv48... > Checking PATCH 20/24: RISC-V: vectored traps are optional... > ERROR: trailing whitespace > #27:
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2
On Sat, Mar 10, 2018 at 9:11 AM, Michael Clarkwrote: > > > On Sat, Mar 10, 2018 at 5:49 AM, Peter Maydell > wrote: > >> On 9 March 2018 at 14:28, Peter Maydell wrote: >> > NB: there was a test failure on OpenBSD host: >> > >> > TEST: tests/qom-test... (pid=64016) >> > /riscv32/qom/spike_v1.9.1: >> ** >> > ERROR:/home/qemu/tests/qom-test.c:64:test_properties: assertion >> > failed: (qdict_haskey(response, "return")) >> > FAIL >> > >> > but this seems to have been intermittent -- it was only on that one >> > host, and I reran the test suite there and it passed fine the second >> > time. So it may be nothing to do with your code; we'll see if it >> > comes up again. >> >> On a later test run I got this different one; openbsd again: >> >> TEST: tests/test-hmp... (pid=45236) >> /riscv32/hmp/spike_v1.9.1: ** >> ERROR:/home/qemu/qom/object.c:488:object_new_with_type: assertion >> failed: (type != NULL) >> Broken pipe >> FAIL >> >> My current best theory is that OpenBSD libc's memory allocator >> happens to be more sensitive to a memory corruption bug in the risc >> code, resulting in intermittent failures if the allocations happen >> to come out the wrong way. You do have at least one invalid-write >> off the end of a block according to valgrind: >> >> ==17441== Invalid write of size 1 >> ==17441==at 0x26517F: riscv_isa_string (cpu.c:399) >> ==17441==by 0x25C14D: create_fdt (spike.c:125) >> ==17441==by 0x25C14D: spike_v1_10_0_board_init (spike.c:199) >> ==17441==by 0x2CCE0A: machine_run_board_init (machine.c:807) >> ==17441==by 0x1BFF28: main (vl.c:4597) >> ==17441== Address 0x3055c425 is 0 bytes after a block of size 5 alloc'd >> ==17441==at 0x4C2FB55: calloc (in >> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) >> ==17441==by 0x70C8770: g_malloc0 (in >> /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2) >> ==17441==by 0x26511E: riscv_isa_string (cpu.c:395) >> ==17441==by 0x25C14D: create_fdt (spike.c:125) >> ==17441==by 0x25C14D: spike_v1_10_0_board_init (spike.c:199) >> ==17441==by 0x2CCE0A: machine_run_board_init (machine.c:807) >> ==17441==by 0x1BFF28: main (vl.c:4597) >> >> If you can prioritise a patch that fixes the bug in riscv_isa_string() >> I'll apply that and hopefully these intermittent failures will go away. >> > > I'm looking at this right now. > It's a glaringly obvious logic bug. The use of the wrong bit manipulation instrinsic. I just sent a patch.
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2
On Sat, Mar 10, 2018 at 5:49 AM, Peter Maydellwrote: > On 9 March 2018 at 14:28, Peter Maydell wrote: > > NB: there was a test failure on OpenBSD host: > > > > TEST: tests/qom-test... (pid=64016) > > /riscv32/qom/spike_v1.9.1: ** > > ERROR:/home/qemu/tests/qom-test.c:64:test_properties: assertion > > failed: (qdict_haskey(response, "return")) > > FAIL > > > > but this seems to have been intermittent -- it was only on that one > > host, and I reran the test suite there and it passed fine the second > > time. So it may be nothing to do with your code; we'll see if it > > comes up again. > > On a later test run I got this different one; openbsd again: > > TEST: tests/test-hmp... (pid=45236) > /riscv32/hmp/spike_v1.9.1: ** > ERROR:/home/qemu/qom/object.c:488:object_new_with_type: assertion > failed: (type != NULL) > Broken pipe > FAIL > > My current best theory is that OpenBSD libc's memory allocator > happens to be more sensitive to a memory corruption bug in the risc > code, resulting in intermittent failures if the allocations happen > to come out the wrong way. You do have at least one invalid-write > off the end of a block according to valgrind: > > ==17441== Invalid write of size 1 > ==17441==at 0x26517F: riscv_isa_string (cpu.c:399) > ==17441==by 0x25C14D: create_fdt (spike.c:125) > ==17441==by 0x25C14D: spike_v1_10_0_board_init (spike.c:199) > ==17441==by 0x2CCE0A: machine_run_board_init (machine.c:807) > ==17441==by 0x1BFF28: main (vl.c:4597) > ==17441== Address 0x3055c425 is 0 bytes after a block of size 5 alloc'd > ==17441==at 0x4C2FB55: calloc (in > /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ==17441==by 0x70C8770: g_malloc0 (in > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2) > ==17441==by 0x26511E: riscv_isa_string (cpu.c:395) > ==17441==by 0x25C14D: create_fdt (spike.c:125) > ==17441==by 0x25C14D: spike_v1_10_0_board_init (spike.c:199) > ==17441==by 0x2CCE0A: machine_run_board_init (machine.c:807) > ==17441==by 0x1BFF28: main (vl.c:4597) > > If you can prioritise a patch that fixes the bug in riscv_isa_string() > I'll apply that and hopefully these intermittent failures will go away. > I'm looking at this right now.
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2
On 9 March 2018 at 14:28, Peter Maydellwrote: > NB: there was a test failure on OpenBSD host: > > TEST: tests/qom-test... (pid=64016) > /riscv32/qom/spike_v1.9.1: ** > ERROR:/home/qemu/tests/qom-test.c:64:test_properties: assertion > failed: (qdict_haskey(response, "return")) > FAIL > > but this seems to have been intermittent -- it was only on that one > host, and I reran the test suite there and it passed fine the second > time. So it may be nothing to do with your code; we'll see if it > comes up again. On a later test run I got this different one; openbsd again: TEST: tests/test-hmp... (pid=45236) /riscv32/hmp/spike_v1.9.1: ** ERROR:/home/qemu/qom/object.c:488:object_new_with_type: assertion failed: (type != NULL) Broken pipe FAIL My current best theory is that OpenBSD libc's memory allocator happens to be more sensitive to a memory corruption bug in the risc code, resulting in intermittent failures if the allocations happen to come out the wrong way. You do have at least one invalid-write off the end of a block according to valgrind: ==17441== Invalid write of size 1 ==17441==at 0x26517F: riscv_isa_string (cpu.c:399) ==17441==by 0x25C14D: create_fdt (spike.c:125) ==17441==by 0x25C14D: spike_v1_10_0_board_init (spike.c:199) ==17441==by 0x2CCE0A: machine_run_board_init (machine.c:807) ==17441==by 0x1BFF28: main (vl.c:4597) ==17441== Address 0x3055c425 is 0 bytes after a block of size 5 alloc'd ==17441==at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==17441==by 0x70C8770: g_malloc0 (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2) ==17441==by 0x26511E: riscv_isa_string (cpu.c:395) ==17441==by 0x25C14D: create_fdt (spike.c:125) ==17441==by 0x25C14D: spike_v1_10_0_board_init (spike.c:199) ==17441==by 0x2CCE0A: machine_run_board_init (machine.c:807) ==17441==by 0x1BFF28: main (vl.c:4597) If you can prioritise a patch that fixes the bug in riscv_isa_string() I'll apply that and hopefully these intermittent failures will go away. thanks -- PMM
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2
On 9 March 2018 at 15:15, Alex Bennéewrote: > > Michael Clark writes: > > >> >> BTW - I've integrated the following 3 branches into the riscv tree: >> >> - https://github.com/riscv/riscv-qemu/tree/softfloat-snan-abort-fix >> - https://github.com/riscv/riscv-qemu/tree/riscv-qemu-upstream-v8.2 >> - https://github.com/michaeljclark/riscv-qemu/tree/qemu-devel >> >> into our `riscv-all` integration branch, and we're now passing all FPU >> tests, interestingly, including the NaN-boxing of single precision values >> into doubles. We'll need to check that the riscv-tests testsuite is >> exhastive enough... Suprised! I think Richard might have thought about >> our NaN-boxing >> issue or some other sort of magic is going on :-) >> >> - https://github.com/riscv/riscv-qemu/tree/riscv-all > > Is the SNaN patch going to get re-posted now it has had a review? I'm just in the process of applying that to master now... -- PMM
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2
Michael Clarkwrites: > > BTW - I've integrated the following 3 branches into the riscv tree: > > - https://github.com/riscv/riscv-qemu/tree/softfloat-snan-abort-fix > - https://github.com/riscv/riscv-qemu/tree/riscv-qemu-upstream-v8.2 > - https://github.com/michaeljclark/riscv-qemu/tree/qemu-devel > > into our `riscv-all` integration branch, and we're now passing all FPU > tests, interestingly, including the NaN-boxing of single precision values > into doubles. We'll need to check that the riscv-tests testsuite is > exhastive enough... Suprised! I think Richard might have thought about > our NaN-boxing > issue or some other sort of magic is going on :-) > > - https://github.com/riscv/riscv-qemu/tree/riscv-all Is the SNaN patch going to get re-posted now it has had a review? -- Alex Bennée
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2
On Sat, Mar 10, 2018 at 3:28 AM, Peter Maydellwrote: > On 8 March 2018 at 19:53, Michael Clark wrote: > > I re-iterate Palmer's apology. > > > > I shouldn't be polling git.qemu.org/qemu.git and answering emails near > to > > 3am in the morning after 4 months of working on trying to get the RISC-V > > port in shape to go upstream. > > > > It appears it is completely my mistake and I had tagged early deltas on > top > > of v8.2 instead of the tip of v8.2. > > > > I've force pushed the 'riscv-qemu-upstream-v8.2' so only the mailing list > > will hold the history of my mistake. > > Thank you for the apology. On my side, I regret not starting this > email thread by just asking if you'd pushed the wrong tag by mistake, > since in retrospect that was certainly the most likely situation. > No worries. It was very late at night. I was tired and a little anxious. Thanks very much for bearing with me after my comments. > I've now merged and tested the revised tag, and pushed it upstream. > That's great news. > NB: there was a test failure on OpenBSD host: > > TEST: tests/qom-test... (pid=64016) > /riscv32/qom/spike_v1.9.1: ** > ERROR:/home/qemu/tests/qom-test.c:64:test_properties: assertion > failed: (qdict_haskey(response, "return")) > FAIL > > but this seems to have been intermittent -- it was only on that one > host, and I reran the test suite there and it passed fine the second > time. So it may be nothing to do with your code; we'll see if it > comes up again. > > I also had a look at running the port under valgrind, which shows > what looks like a bug in riscv_isa_string(): > > $ valgrind ./build/all/riscv32-softmmu/qemu-system-riscv32 > [...] > ==24805== Invalid read of size 1 > ==24805==at 0x4C30F74: strlen (in > /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ==24805==by 0x26518E: riscv_isa_string (cpu.c:399) > ==24805==by 0x25C15D: create_fdt (spike.c:125) > ==24805==by 0x25C15D: spike_v1_10_0_board_init (spike.c:199) > ==24805==by 0x2CCE1A: machine_run_board_init (machine.c:807) > ==24805==by 0x1BFF28: main (vl.c:4597) > ==24805== Address 0x3055be55 is 0 bytes after a block of size 5 alloc'd > ==24805==at 0x4C2FB55: calloc (in > /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ==24805==by 0x70C8770: g_malloc0 (in > /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2) > ==24805==by 0x26512E: riscv_isa_string (cpu.c:395) > ==24805==by 0x25C15D: create_fdt (spike.c:125) > ==24805==by 0x25C15D: spike_v1_10_0_board_init (spike.c:199) > ==24805==by 0x2CCE1A: machine_run_board_init (machine.c:807) > ==24805==by 0x1BFF28: main (vl.c:4597) > > I haven't looked too hard at the code, but I suspect you're > miscalculating the length of the string and/or not writing the > trailing NUL to the string. I recommend you have a look at that, > and perhaps try running some other tests under valgrind. > Thanks very much! I'll valgrind locally when I get time. It's very late at night here. 3.45am. We may very well have a memory issue but it's likely restricted to the RISC-V port and shouldn't cause any issues for other ports. BTW - I've integrated the following 3 branches into the riscv tree: - https://github.com/riscv/riscv-qemu/tree/softfloat-snan-abort-fix - https://github.com/riscv/riscv-qemu/tree/riscv-qemu-upstream-v8.2 - https://github.com/michaeljclark/riscv-qemu/tree/qemu-devel into our `riscv-all` integration branch, and we're now passing all FPU tests, interestingly, including the NaN-boxing of single precision values into doubles. We'll need to check that the riscv-tests testsuite is exhastive enough... Suprised! I think Richard might have thought about our NaN-boxing issue or some other sort of magic is going on :-) - https://github.com/riscv/riscv-qemu/tree/riscv-all Michael. -- $ sh qemu-images/run-riscv-tests.sh rv64ua-v-amoadd_d rv64ua-v-amoadd_w rv64ua-v-amoand_d rv64ua-v-amoand_w rv64ua-v-amomax_d rv64ua-v-amomax_w rv64ua-v-amomaxu_d rv64ua-v-amomaxu_w rv64ua-v-amomin_d rv64ua-v-amomin_w rv64ua-v-amominu_d rv64ua-v-amominu_w rv64ua-v-amoor_d rv64ua-v-amoor_w rv64ua-v-amoswap_d rv64ua-v-amoswap_w rv64ua-v-amoxor_d rv64ua-v-amoxor_w rv64ua-v-lrsc rv64uc-v-rvc rv64ud-v-fadd rv64ud-v-fclass rv64ud-v-fcmp rv64ud-v-fcvt rv64ud-v-fcvt_w rv64ud-v-fdiv rv64ud-v-fmadd rv64ud-v-fmin rv64ud-v-ldst rv64ud-v-move rv64ud-v-recoding rv64ud-v-structural rv64uf-v-fadd rv64uf-v-fclass rv64uf-v-fcmp rv64uf-v-fcvt rv64uf-v-fcvt_w rv64uf-v-fdiv rv64uf-v-fmadd rv64uf-v-fmin rv64uf-v-ldst rv64uf-v-move rv64uf-v-recoding rv64ui-v-add rv64ui-v-addi rv64ui-v-addiw rv64ui-v-addw rv64ui-v-and rv64ui-v-andi rv64ui-v-auipc rv64ui-v-beq rv64ui-v-bge rv64ui-v-bgeu rv64ui-v-blt rv64ui-v-bltu rv64ui-v-bne rv64ui-v-fence_i rv64ui-v-jal rv64ui-v-jalr rv64ui-v-lb rv64ui-v-lbu rv64ui-v-ld rv64ui-v-lh rv64ui-v-lhu rv64ui-v-lui rv64ui-v-lw rv64ui-v-lwu rv64ui-v-or rv64ui-v-ori
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2
On 8 March 2018 at 19:53, Michael Clarkwrote: > I re-iterate Palmer's apology. > > I shouldn't be polling git.qemu.org/qemu.git and answering emails near to > 3am in the morning after 4 months of working on trying to get the RISC-V > port in shape to go upstream. > > It appears it is completely my mistake and I had tagged early deltas on top > of v8.2 instead of the tip of v8.2. > > I've force pushed the 'riscv-qemu-upstream-v8.2' so only the mailing list > will hold the history of my mistake. Thank you for the apology. On my side, I regret not starting this email thread by just asking if you'd pushed the wrong tag by mistake, since in retrospect that was certainly the most likely situation. I've now merged and tested the revised tag, and pushed it upstream. NB: there was a test failure on OpenBSD host: TEST: tests/qom-test... (pid=64016) /riscv32/qom/spike_v1.9.1: ** ERROR:/home/qemu/tests/qom-test.c:64:test_properties: assertion failed: (qdict_haskey(response, "return")) FAIL but this seems to have been intermittent -- it was only on that one host, and I reran the test suite there and it passed fine the second time. So it may be nothing to do with your code; we'll see if it comes up again. I also had a look at running the port under valgrind, which shows what looks like a bug in riscv_isa_string(): $ valgrind ./build/all/riscv32-softmmu/qemu-system-riscv32 [...] ==24805== Invalid read of size 1 ==24805==at 0x4C30F74: strlen (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==24805==by 0x26518E: riscv_isa_string (cpu.c:399) ==24805==by 0x25C15D: create_fdt (spike.c:125) ==24805==by 0x25C15D: spike_v1_10_0_board_init (spike.c:199) ==24805==by 0x2CCE1A: machine_run_board_init (machine.c:807) ==24805==by 0x1BFF28: main (vl.c:4597) ==24805== Address 0x3055be55 is 0 bytes after a block of size 5 alloc'd ==24805==at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==24805==by 0x70C8770: g_malloc0 (in /lib/x86_64-linux-gnu/libglib-2.0.so.0.4800.2) ==24805==by 0x26512E: riscv_isa_string (cpu.c:395) ==24805==by 0x25C15D: create_fdt (spike.c:125) ==24805==by 0x25C15D: spike_v1_10_0_board_init (spike.c:199) ==24805==by 0x2CCE1A: machine_run_board_init (machine.c:807) ==24805==by 0x1BFF28: main (vl.c:4597) I haven't looked too hard at the code, but I suspect you're miscalculating the length of the string and/or not writing the trailing NUL to the string. I recommend you have a look at that, and perhaps try running some other tests under valgrind. thanks -- PMM
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2
On Fri, Mar 9, 2018 at 8:29 AM, Palmer Dabbeltwrote: > On Thu, 08 Mar 2018 03:41:33 PST (-0800), Michael Clark wrote: > >> On Fri, 9 Mar 2018 at 12:18 AM, Michael Clark wrote: >> >>> On Fri, 9 Mar 2018 at 12:10 AM, Michael Clark wrote: >>> On Thu, 8 Mar 2018 at 11:02 PM, Peter Maydell wrote: > On 6 March 2018 at 19:46, Michael Clark wrote: > You are making this very hard. Do you work for Arm perchance? I really >>> wouldn’t be surprised if our port is being sandbagged by Arm. Apologies >>> for >>> being so direct about this, but things like this happen... >>> >>> I have complied with practically every review request and the sign-offs >>> are there. It’s a bit ridiculous. >>> >>> It would be nice to find someone neutral, unrelated to Arm, to merge our >>> PR >>> >>> >> Some history on the origins of RISC to put things in perspective: >> >> https://en.m.wikipedia.org/wiki/Berkeley_RISC >> >> David Patterson worked with Andrew Waterman and Krste Asanovic on the >> design of RISC-V. Sagar did most of the work on the QEMU port and he >> agreeded to sign off on all patches. The SiFive patches only have >> sign-offs >> from SiFive because SiFive was the sole contributor for its hardware >> model, >> beside the SiFiveUART which has Stefan’s sign-off. >> >> In any case it seems there is not enough review bandwidth in the QEMU >> project as a whole and the policy to accept contributions is too strict to >> be reasonable, given earnest attempts to comply with *all* review >> feedback. >> Not impressed. >> > > On behalf of the rest of the RISC-V QEMU team I'd like to apologize for > Michael's comments. That's a pretty insulting thing to say, and the whole > thing comes off as a bit entitled: we've asked the QEMU community to do a > lot of work for us in reviewing our port, and seeing as how none of us are > QEMU contributors we certainly don't have any grounds to ask someone to > stop reviewing it -- that's pretty absurd. > > While I haven't been following the upstreaming process as closely as I > should have been, as far as I can tell there's no grounds to accuse Peter, > or anyone else, of trying to shoot down our port for any reason. Peter, I > can understand if you're upset, as I certainly would be. If you don't want > to help out with our port any more then I can understand, but I'd just like > to assure you that we value the time you've spent on our port and hope you > continue to help out! > > Hopefully this doesn't derail our chances of moving forward with > submitting the RISC-V port upstream. > > Sorry! > I re-iterate Palmer's apology. I shouldn't be polling git.qemu.org/qemu.git and answering emails near to 3am in the morning after 4 months of working on trying to get the RISC-V port in shape to go upstream. It appears it is completely my mistake and I had tagged early deltas on top of v8.2 instead of the tip of v8.2. I've force pushed the 'riscv-qemu-upstream-v8.2' so only the mailing list will hold the history of my mistake. $ git push -f --tags Counting objects: 1, done. Writing objects: 100% (1/1), 6.31 KiB | 0 bytes/s, done. Total 1 (delta 0), reused 0 (delta 0) To g...@github.com:riscv/riscv-qemu.git + feb8f5e...2c7f042 riscv-qemu-upstream-v8.2 -> riscv-qemu-upstream-v8.2 (forced update)
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission v8.2
On Thu, 08 Mar 2018 03:41:33 PST (-0800), Michael Clark wrote: On Fri, 9 Mar 2018 at 12:18 AM, Michael Clarkwrote: On Fri, 9 Mar 2018 at 12:10 AM, Michael Clark wrote: On Thu, 8 Mar 2018 at 11:02 PM, Peter Maydell wrote: On 6 March 2018 at 19:46, Michael Clark wrote: You are making this very hard. Do you work for Arm perchance? I really wouldn’t be surprised if our port is being sandbagged by Arm. Apologies for being so direct about this, but things like this happen... I have complied with practically every review request and the sign-offs are there. It’s a bit ridiculous. It would be nice to find someone neutral, unrelated to Arm, to merge our PR Some history on the origins of RISC to put things in perspective: https://en.m.wikipedia.org/wiki/Berkeley_RISC David Patterson worked with Andrew Waterman and Krste Asanovic on the design of RISC-V. Sagar did most of the work on the QEMU port and he agreeded to sign off on all patches. The SiFive patches only have sign-offs from SiFive because SiFive was the sole contributor for its hardware model, beside the SiFiveUART which has Stefan’s sign-off. In any case it seems there is not enough review bandwidth in the QEMU project as a whole and the policy to accept contributions is too strict to be reasonable, given earnest attempts to comply with *all* review feedback. Not impressed. On behalf of the rest of the RISC-V QEMU team I'd like to apologize for Michael's comments. That's a pretty insulting thing to say, and the whole thing comes off as a bit entitled: we've asked the QEMU community to do a lot of work for us in reviewing our port, and seeing as how none of us are QEMU contributors we certainly don't have any grounds to ask someone to stop reviewing it -- that's pretty absurd. While I haven't been following the upstreaming process as closely as I should have been, as far as I can tell there's no grounds to accuse Peter, or anyone else, of trying to shoot down our port for any reason. Peter, I can understand if you're upset, as I certainly would be. If you don't want to help out with our port any more then I can understand, but I'd just like to assure you that we value the time you've spent on our port and hope you continue to help out! Hopefully this doesn't derail our chances of moving forward with submitting the RISC-V port upstream. Sorry!
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission
On Tue, Mar 6, 2018 at 8:00 AM, Emilio G. Cotawrote: > On Sat, Mar 03, 2018 at 02:26:12 +1300, Michael Clark wrote: > > It was qemu-2.7.50 (late 2016). The benchmarks were generated mid last > year. > > > > I can run the benchmarks again... Has it doubled in speed? > > It depends on the benchmarks. Small-ish benchmarks such as rv8-bench > show about a 1.5x speedup since QEMU v2.6.0 for Aarch64: > > Aarch64 rv8-bench performance under QEMU user-mode > Host: Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz > > 4.5 +-++--+--+--+--+--+--+--+--- > ---++-+ > | ++ > | > 4 +-+..v2.8.0.v2.9.0v2.10.0.%%. > v2.11.0+-+ > 3.5 +-+...%%@... > +-+ > | %%@ > | > 3 +-+...%%@... > +-+ > 2.5 +-+...%%@... > +-+ > | ++$$$%@ > | > 2 +-+.$$$%@..##.$%@... > +-+ > | ##+$%@ ##$$%@ ## $%@ > | > 1.5 +-+..+++%%@...**#.$%@.##.$%@%%@##.$%@... > .##$$%@.+-+ > 1 +-+.**#$$%@+##$$%@**#.$%@**#.$%@**#$$%@**#$$%@**#.$%@**#$$%@ > **#.$%@.+-+ > | **# $%@**# $%@**# $%@**# $%@**# $%@**#+$%@**# $%@**# $%@**# $%@ > | > 0.5 +-+-**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@ > **#$$%@-+-+ > aes bigidhrystone miniz norx primes qsort sha512geomean > png: https://imgur.com/Agr5CJd > > SPEC06int shows a larger improvement, up to ~2x avg speedup for the train > set: > Aarch64 SPEC06int (train set) performance under QEMU user-mode > Host: Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz > > 4 +-+--+++++++++++ > ++--+-+ > |%% ++ > | > 3.5 +-+..%%@.v2.8.0.v2.9.0v2.10.0.%%+ > v2.11.0+-+ > |%%@ %%@ ++ > | > 3 +-+..%%@...++.%%@... > %%+.+-+ > | +$%@|+%%@ %%@ > | > 2.5 +-+.##%@...%%@...+$%@... > %%@.+-+ > 2 +-+.##%@..+%%@...%%@.%%@.##%@... > %%@..++.+-+ > | ##%@ %%@ ##%@ +$%@ %%@ %%@ ##%@ $%@ > %%@ | > 1.5 +-+**#%@.##%@.##%@..##%@...$%@...$%@.##%@..%%+.# > #%@.##%@+-+ > | **#%@**#%@**#%@ +++**#%@ ##%@ ++ ##%@**#%@ ##%@ ##%@ > ##%@ | > 1 +-+**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@+##%@**#%@+##%@** > #%@**#%@+-+ > | **#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@ > | > 0.5 +-+**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@** > #%@**#%@+-+ >401.bzi403.g429445.g456.h462.libq464.h471.omn4483.xalancbgeomean > png: https://imgur.com/JknVT5H > > Note that the test set is less sensitive to the changes: > https://imgur.com/W7CT0eO > > Running small benchmarks (such as SPEC "test" or rv8-bench) is > very useful to get quick feedback on optimizations. However, some > of these runs are still dominated by parts of the code that aren't > that relevant -- for instance, some of them take so little time to > run that the major contributor to execution time is memory allocation. > Therefore, when publishing results it's best to stick with larger > benchmarks that run for longer (e.g. SPEC "train" set), which are more > sensitive to DBT performance. > > I tried running some other benchmarks, such as nbench[1], under rv-jit. > I quickly get a "bus error" though -- don't know if I'm doing anything > wrong, or maybe compiling with the glibc cross-compiler I used > to build riscv linux isn't supported. > I managed though to run rv8-bench on both rv-jit and qemu (v8 patchset); > rv-jit is 1.30x faster on average for those, although note I dropped > qsort because it wasn't working properly on rv-jit: > That's interesting. I know from some analysis that the current slow-down in rv8 is mostly from accessing statically spilled registers (which in many cases we embed in x86 memory operands to keep up code density, and make use of the instruction cracker and uop cache in Intel's front-end). The slowdown is mostly L1 cache latency vs register access latency given we are emulating 31 registers on a 16 register host with a static register allocation (based on the compiler register allocation order which optimizes for the RVC accessible registers). With the addition of a register allocator, I am sure I can make rv8 substantially faster. perhaps 1.7x The user-mode emulation in rv8 is very limited, and
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission
On Sat, Mar 03, 2018 at 02:26:12 +1300, Michael Clark wrote: > It was qemu-2.7.50 (late 2016). The benchmarks were generated mid last year. > > I can run the benchmarks again... Has it doubled in speed? It depends on the benchmarks. Small-ish benchmarks such as rv8-bench show about a 1.5x speedup since QEMU v2.6.0 for Aarch64: Aarch64 rv8-bench performance under QEMU user-mode Host: Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz 4.5 +-++--+--+--+--+--+--+--+--++-+ | ++ | 4 +-+..v2.8.0.v2.9.0v2.10.0.%%.v2.11.0+-+ 3.5 +-+...%%@...+-+ | %%@ | 3 +-+...%%@...+-+ 2.5 +-+...%%@...+-+ | ++$$$%@ | 2 +-+.$$$%@..##.$%@...+-+ | ##+$%@ ##$$%@ ## $%@ | 1.5 +-+..+++%%@...**#.$%@.##.$%@%%@##.$%@##$$%@.+-+ 1 +-+.**#$$%@+##$$%@**#.$%@**#.$%@**#$$%@**#$$%@**#.$%@**#$$%@**#.$%@.+-+ | **# $%@**# $%@**# $%@**# $%@**# $%@**#+$%@**# $%@**# $%@**# $%@ | 0.5 +-+-**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@**#$$%@-+-+ aes bigidhrystone miniz norx primes qsort sha512geomean png: https://imgur.com/Agr5CJd SPEC06int shows a larger improvement, up to ~2x avg speedup for the train set: Aarch64 SPEC06int (train set) performance under QEMU user-mode Host: Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz 4 +-+--+++++++++++++--+-+ |%% ++ | 3.5 +-+..%%@.v2.8.0.v2.9.0v2.10.0.%%+v2.11.0+-+ |%%@ %%@ ++| 3 +-+..%%@...++.%%@...%%+.+-+ | +$%@|+%%@ %%@ | 2.5 +-+.##%@...%%@...+$%@...%%@.+-+ 2 +-+.##%@..+%%@...%%@.%%@.##%@...%%@..++.+-+ | ##%@ %%@ ##%@ +$%@ %%@ %%@ ##%@ $%@ %%@ | 1.5 +-+**#%@.##%@.##%@..##%@...$%@...$%@.##%@..%%+.##%@.##%@+-+ | **#%@**#%@**#%@ +++**#%@ ##%@ ++ ##%@**#%@ ##%@ ##%@ ##%@ | 1 +-+**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@+##%@**#%@+##%@**#%@**#%@+-+ | **#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@ | 0.5 +-+**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@**#%@+-+ 401.bzi403.g429445.g456.h462.libq464.h471.omn4483.xalancbgeomean png: https://imgur.com/JknVT5H Note that the test set is less sensitive to the changes: https://imgur.com/W7CT0eO Running small benchmarks (such as SPEC "test" or rv8-bench) is very useful to get quick feedback on optimizations. However, some of these runs are still dominated by parts of the code that aren't that relevant -- for instance, some of them take so little time to run that the major contributor to execution time is memory allocation. Therefore, when publishing results it's best to stick with larger benchmarks that run for longer (e.g. SPEC "train" set), which are more sensitive to DBT performance. I tried running some other benchmarks, such as nbench[1], under rv-jit. I quickly get a "bus error" though -- don't know if I'm doing anything wrong, or maybe compiling with the glibc cross-compiler I used to build riscv linux isn't supported. I managed though to run rv8-bench on both rv-jit and qemu (v8 patchset); rv-jit is 1.30x faster on average for those, although note I dropped qsort because it wasn't working properly on rv-jit: rv8-bench performance under rv-jit and QEMU user-mode Host: Intel(R) Core(TM) i7-6700K CPU @ 4.00GHz [qsort does not finish cleanly for rv8, so I dropped it.] 3 +-+-+---+--+---+---+---+--+---+-+-+ 2.5 +-+..*..+-+ |*-+-* b1bae23b7c2| 2
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission
On Thu, Mar 1, 2018 at 9:40 AM, Michael Clarkwrote: > > > On Thu, Mar 1, 2018 at 12:53 AM, Peter Maydell > wrote: > >> On 28 February 2018 at 00:09, Michael Clark wrote: >> > I've just talked to SiFive about this. They have agreed that we can >> remove >> > the sifive_e300 and sifive_u500 boards from the patch series that we are >> > going to submit upstream again later this week or early next week. These >> > machines and their devices are pretty easy for us to maintain in the >> riscv >> > or a sifive repo. This trims the number of machines from 5 to 3 and >> lets us >> > remove the SiFiveUART and SiFivePRCI from the next patch series we are >> > going to submit. e.g. v8 >> >> Models of boards which people actively want and are using are fine >> (though indeed you can save them for a later round of patches if you >> like). And it sounds like the 1.9.1 stuff is genuinely wanted, so >> thanks for the clarification there. What I want to avoid is boards >> going into QEMU just because you happened to have them already. Once >> a board model goes into QEMU it's a commitment to supporting it for >> the long term, and getting rid of it again is hard. > > > Most folk in the community at large are interested in the 'spike' and > 'virt' machines. Well, the linux distributors are currently targetting > 'virt' as it has working network and block storage. > > It's mostly SiFive customers that are interested in the SiFive machines. > > SiFive do intend to submit their machines upstream however we've decided > that we want to review the naming of the machine/board/SOC in light of > several new models, as there are infact more MCU/SOC models than are > currently represented in the RISC-V QEMU port (E21, E31, E51, U54, U54MC). > Those are the SOCs and then there are boards like the HiFive1, the LoFive, > the HiFive Unleashed and several others (the 7th RISC-V Foundation Workshop > had an electronic badge with the FE310 designed by antmicro). SiFive might > like to review the naming and re-jig the SOC / board relationship. > Presently we created two generic boards to represent the MMU-less E-series > (sifive_e300) and the U-series (sifive_u500) however that doesn't cover the > E21 and E51. As well as silicon, there are soft-cores, including soft-cores > from other vendors (who have not yet submitted anything to the QEMU port). > After reflecting on this we don't think the naming of the two sifive > machines is quite right and they are not yet complete emulations. The > sifive_u500 > is supposed to model the HiFive Unleashed however we don't have device > emulation for all of the hardware widgets yet, and some of the drivers are > not yet in upstream Linux. Linus essentially accepted the generic core, > which is what we are now suggesting for QEMU. We are totally happy to > submit them if folk don't mind us potentially renaming them later. This > is 2 patches for 2 machines and 2 or 3 devices. It would reduce the patch > series from 23 patches to 18 patches and we'd maintain the 5 files and > associated headers out-of tree until things firm up. The core of the port > is pretty solid as Fedora have build stage 4 images using SMP on the 'virt' > machine. > We had internal discussions about the SiFive machines and SiFive decided to rename SiFive E300 (sifive_e300.c) and SiFive U500 (sifive_u500.c) to SiFive E Series (sifive_e.c) and SiFive U Series (sifive_u.c). This was the result of synchronising with the product marketing folk. - sifive_e will instantiate a e31 nommu core when run in qemu-system-riscv32 - sifive_e will instantiate a e51 nommu core when run in qemu-system-riscv64 - sifive_u will instantiate a u34 mmu core when run in qemu-system-riscv32 - sifive_u will instantiate a u54 mmu core when run in qemu-system-riscv64 The riscv port is interesting because the machines are designed to be instantiated with either 32-bit or 64-bit cores. SiFive decided they wanted to get this right before we contributed the SiFive boards. We think we now have a model we are satisfied with submittting. We did some last minute changes to strengthen the riscv-qemu port specificaiton compliance. The nommu machines worked previously but the lack of an mmu was not enforced. We've now disabled mmu on cores that lack mmu (SiFive E series). We've also made some changes to strengthen specification compliance. It is possible for a core to implement S mode and U mode (Supervisor and User privilege mode) either with or without an mmu. We've now matches the hardware behaviour and discard mstatus.mpp writes for modes that are not supported by the core, making it not possible to enter S mode on the E series cores. We are pretty confident that these are low risk changes, and we have tested that Linux still works fine in the 'virt' machine. The 'virt' machine is likely to be the most popular machine for use by the Linux community. Given SiFive has SOCs and Soft-core IP, we would
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission
On Thu, Mar 1, 2018 at 11:26 AM, Emilio G. Cotawrote: > On Wed, Feb 28, 2018 at 13:09:11 +1300, Michael Clark wrote: > > BTW somewhat coincidentally, the binary translator I wrote; RV8, which is > > practicaly twice as fast as QEMU only supports privileged ISA v1.9.1 and > I > > personally want to keep binary compatiblity with it. > (snip) > > - https://rv8.io/ > > - https://rv8.io/bench > > - https://anarch128.org/~mclark/rv8-carrv.pdf > > - https://anarch128.org/~mclark/rv8-slides.pdf > > What QEMU versions did you use for those comparisons? I wonder if > the recent indirect branch handling improvements were included in those > (this work was merged in 2.10 for aarch64). Also, 2.7 is quite a bit > faster than previous versions for user-mode due to the use of QHT, > although you probably used a later version. > Yes I noticed indirect branch handling was very slow in the QEMU verison I tested. I have highly optimised assembly stubs that implement a direct mapped translation cache and translation cache miss fallback to C code that does a fast hash map lookup. > BTW after the merge you might want to look into optimizing indirect > branches (and cross-page direct jumps in softmmu) for riscv in qemu. > See examples with > $ git log -Stcg_gen_lookup_and_goto_ptr > It was qemu-2.7.50 (late 2016). The benchmarks were generated mid last year. I can run the benchmarks again... Has it doubled in speed? Note: I don't even have a register allocator. I've assigned RISC-V RVC register to hard registers (compiler optimises to choose compressable registers first) and the remainder are in spill slots that in many cases can be embedded as memory operands in the x86_64 translation. i.e. no explicit reload, we let the micro-architecture crack these into micro-ops, as they help to keep up code density. I think I can get close to double again with tiered optimization and a good register allocator (lift RISC-V asm to SSA form). It's also a hotspot interpreter, which is definately faster than compiling all code, as I benchmarked it. It profiles and only translates hot paths, so code that only runs a few iterations is not translated. When I did eager transaltion I got a slow-down.
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission
On Wed, Feb 28, 2018 at 13:09:11 +1300, Michael Clark wrote: > BTW somewhat coincidentally, the binary translator I wrote; RV8, which is > practicaly twice as fast as QEMU only supports privileged ISA v1.9.1 and I > personally want to keep binary compatiblity with it. (snip) > - https://rv8.io/ > - https://rv8.io/bench > - https://anarch128.org/~mclark/rv8-carrv.pdf > - https://anarch128.org/~mclark/rv8-slides.pdf What QEMU versions did you use for those comparisons? I wonder if the recent indirect branch handling improvements were included in those (this work was merged in 2.10 for aarch64). Also, 2.7 is quite a bit faster than previous versions for user-mode due to the use of QHT, although you probably used a later version. BTW after the merge you might want to look into optimizing indirect branches (and cross-page direct jumps in softmmu) for riscv in qemu. See examples with $ git log -Stcg_gen_lookup_and_goto_ptr Cheers, Emilio
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission
On Thu, Mar 1, 2018 at 12:53 AM, Peter Maydellwrote: > On 28 February 2018 at 00:09, Michael Clark wrote: > > I've just talked to SiFive about this. They have agreed that we can > remove > > the sifive_e300 and sifive_u500 boards from the patch series that we are > > going to submit upstream again later this week or early next week. These > > machines and their devices are pretty easy for us to maintain in the > riscv > > or a sifive repo. This trims the number of machines from 5 to 3 and lets > us > > remove the SiFiveUART and SiFivePRCI from the next patch series we are > > going to submit. e.g. v8 > > Models of boards which people actively want and are using are fine > (though indeed you can save them for a later round of patches if you > like). And it sounds like the 1.9.1 stuff is genuinely wanted, so > thanks for the clarification there. What I want to avoid is boards > going into QEMU just because you happened to have them already. Once > a board model goes into QEMU it's a commitment to supporting it for > the long term, and getting rid of it again is hard. Most folk in the community at large are interested in the 'spike' and 'virt' machines. Well, the linux distributors are currently targetting 'virt' as it has working network and block storage. It's mostly SiFive customers that are interested in the SiFive machines. SiFive do intend to submit their machines upstream however we've decided that we want to review the naming of the machine/board/SOC in light of several new models, as there are infact more MCU/SOC models than are currently represented in the RISC-V QEMU port (E21, E31, E51, U54, U54MC). Those are the SOCs and then there are boards like the HiFive1, the LoFive, the HiFive Unleashed and several others (the 7th RISC-V Foundation Workshop had an electronic badge with the FE310 designed by antmicro). SiFive might like to review the naming and re-jig the SOC / board relationship. Presently we created two generic boards to represent the MMU-less E-series (sifive_e300) and the U-series (sifive_u500) however that doesn't cover the E21 and E51. As well as silicon, there are soft-cores, including soft-cores from other vendors (who have not yet submitted anything to the QEMU port). After reflecting on this we don't think the naming of the two sifive machines is quite right and they are not yet complete emulations. The sifive_u500 is supposed to model the HiFive Unleashed however we don't have device emulation for all of the hardware widgets yet, and some of the drivers are not yet in upstream Linux. Linus essentially accepted the generic core, which is what we are now suggesting for QEMU. We are totally happy to submit them if folk don't mind us potentially renaming them later. This is 2 patches for 2 machines and 2 or 3 devices. It would reduce the patch series from 23 patches to 18 patches and we'd maintain the 5 files and associated headers out-of tree until things firm up. The core of the port is pretty solid as Fedora have build stage 4 images using SMP on the 'virt' machine. > > However I'm inclined to leave it as it is, at this point. It is not > > something that we can't change in the future once the code is in-tree. > > With my 'upstream dev' hat on, I tend to be suspicious of this > line of argument, because in a lot of cases what tends to happen > is that the code for some new target or device goes in-tree, and > then the people who worked on submitting it disappear, or never > actually do get round to refactoring[*]. You get more leeway for > making this argument the longer you've been around and participating > in QEMU development, because then you have a track record of > following up on things. > > [*] in fact we're currently discussing deleting support for > a couple of target architectures that were basically "once the > code went into mainline nothing further was ever done to it except > global-refactorings and other tree wide maintenance by other > QEMU developers". I might explore coalescing the two spike boards before the v8 spin then. I was thinking about using a global to enable the backwards compatibility mode. It seems -global is for driver properties? Is it appropriate to use a global property to change the behaviour of a machine? Okay, I see raspi.c contains raspi, raspi 2, raspi 3, and it just defines multiple machines in the same file and passes a version number to routines that are shared. We could go with that approach.
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission
On 28 February 2018 at 11:53, Peter Maydellwrote: > With my 'upstream dev' hat on, I tend to be suspicious of this > line of argument, because in a lot of cases what tends to happen > is that the code for some new target or device goes in-tree, and > then the people who worked on submitting it disappear, or never > actually do get round to refactoring[*]. You get more leeway for > making this argument the longer you've been around and participating > in QEMU development, because then you have a track record of > following up on things. That said, I can probably live with it in this particular instance, given when the 2.12 codefreeze deadline is. thanks -- PMM
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission
On 28 February 2018 at 00:09, Michael Clarkwrote: > I've just talked to SiFive about this. They have agreed that we can remove > the sifive_e300 and sifive_u500 boards from the patch series that we are > going to submit upstream again later this week or early next week. These > machines and their devices are pretty easy for us to maintain in the riscv > or a sifive repo. This trims the number of machines from 5 to 3 and lets us > remove the SiFiveUART and SiFivePRCI from the next patch series we are > going to submit. e.g. v8 Models of boards which people actively want and are using are fine (though indeed you can save them for a later round of patches if you like). And it sounds like the 1.9.1 stuff is genuinely wanted, so thanks for the clarification there. What I want to avoid is boards going into QEMU just because you happened to have them already. Once a board model goes into QEMU it's a commitment to supporting it for the long term, and getting rid of it again is hard. > However I'm inclined to leave it as it is, at this point. It is not > something that we can't change in the future once the code is in-tree. With my 'upstream dev' hat on, I tend to be suspicious of this line of argument, because in a lot of cases what tends to happen is that the code for some new target or device goes in-tree, and then the people who worked on submitting it disappear, or never actually do get round to refactoring[*]. You get more leeway for making this argument the longer you've been around and participating in QEMU development, because then you have a track record of following up on things. [*] in fact we're currently discussing deleting support for a couple of target architectures that were basically "once the code went into mainline nothing further was ever done to it except global-refactorings and other tree wide maintenance by other QEMU developers". thanks -- PMM
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission
On Wed, 28 Feb 2018 13:41:25 +1300 Michael Clarkwrote: > On Wed, Feb 28, 2018 at 5:00 AM, Igor Mammedov wrote: > > > On Tue, 27 Feb 2018 14:01:05 + > > Peter Maydell wrote: > > > > > On 27 February 2018 at 00:15, Michael Clark wrote: > > > > -BEGIN PGP SIGNED MESSAGE- > > > > Hash: SHA1 > > > > > > > > The following changes since commit 0a773d55ac76c5aa89ed9187a3bc5a > > f8c5c2a6d0: > > > > > > > > maintainers: Add myself as a OpenBSD maintainer (2018-02-23 12:05:07 > > +) > > > > > > > > are available in the git repository at: > > > > > > > > https://github.com/riscv/riscv-qemu.git tags/riscv-qemu-upstream-v7 > > > > > > > > for you to fetch changes up to 170a9d412ca1eb3b7ae6f6c1ff86dc > > bdff0fd7a8: > > > > > > > > RISC-V Build Infrastructure (2018-02-27 11:09:43 +1300) > > > > > > > > - > > > > QEMU RISC-V Emulation Support (RV64GC, RV32GC) > > > > > > Hi; thanks for this pull request. Unfortunately it seems to > > > be missing Signed-off-by: tags. Every commit needs to have > > > the Signed-off-by: tags from the people who contributed code to > > > it, indicating that they're OK with the code going into QEMU. > > > (If the work was done by and copyright a company then you don't > > > need to provide signoffs from every person at the company who > > > worked on the code if you don't want to.) > > > > > > > The spike_v1.9 > > > > machine has been renamed to spike_v1.9.1 to match the privileged ISA > > > > version and spike_v1.10 has been made the default machine. > > > > > > I'm confused about this. Generally QEMU boards should model > > > hardware, and the board shouldn't care about the ISA versions. > > > Versioned board names in QEMU generally follow _QEMU_'s versioning, > > > and indicate that a board is identical to whatever we modelled > > > in that earlier QEMU version, for VM migration compatibility. > > > Board renames for minor ISA version bumps sounds like there's going > > > to be a lot of churn and breakage -- is this stuff really ready? > > > (Also, should we really have two different board source files > > > for two different ISA versions? I would have expected these to > > > share a source file to share code.) > > > > > > I did a test build and there are some compile errors: > > > > > > /home/pm215/qemu/linux-user/main.c:38:24: fatal error: target_elf.h: > > > No such file or directory > > > #include "target_elf.h" > > > ^ > > > compilation terminated. > > > > > > This is because your patchset has a clash with commit 542ca4349878a2e > > > which has just merged to master, and refactors out an ifdef ladder, > > > so now all targets supporting linux-user need to provide a > > > linux-user/$ARCH/target_elf.h file. Could you fix that up and rebase, > > > please? > > also '[PATCH v7 03/23] RISC-V CPU Core Definition' still hasn't addressed > > comment http://lists.gnu.org/archive/html/qemu-devel/2018-02/msg06412.html > > which isn't fixed since it was first pointed out (v4). > > > > I'd prefer that being fixed before merge so another people > > won't have to clean it up later after original authors, > > When they try to generalize cpu_type -> cpu_model conversion. > > > > I re-read the email and it doesn't seem clear what you want us to do. > I changed the CPU suffix to a prefix as you requested. The rest of the CPU > initialisation is "modelled" on arm not sh4. Not addressed comment is not related to CPU suffix, it's a separate issue. I haven't had time to clean up that part of ARM cpu initialization yet, so it still uses old approach, but you've been pointed to right approach rather early (v4) but ignored it. There is no point to use legacy approach with new patches that could block other people and would force them to cleanup before doing what they intended to do. I've pointed to sh4 as example that you should use for new CPU types internalization. Beside making RISC-V consistent with direction that part of code moves to, it will also simplify patch. Please ask if something still unclear. > If you want to make a pull request, please use this branch: > > - https://github.com/riscv/riscv-qemu/tree/qemu-upstream-v7
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission
On Wed, Feb 28, 2018 at 5:00 AM, Igor Mammedovwrote: > On Tue, 27 Feb 2018 14:01:05 + > Peter Maydell wrote: > > > On 27 February 2018 at 00:15, Michael Clark wrote: > > > -BEGIN PGP SIGNED MESSAGE- > > > Hash: SHA1 > > > > > > The following changes since commit 0a773d55ac76c5aa89ed9187a3bc5a > f8c5c2a6d0: > > > > > > maintainers: Add myself as a OpenBSD maintainer (2018-02-23 12:05:07 > +) > > > > > > are available in the git repository at: > > > > > > https://github.com/riscv/riscv-qemu.git tags/riscv-qemu-upstream-v7 > > > > > > for you to fetch changes up to 170a9d412ca1eb3b7ae6f6c1ff86dc > bdff0fd7a8: > > > > > > RISC-V Build Infrastructure (2018-02-27 11:09:43 +1300) > > > > > > - > > > QEMU RISC-V Emulation Support (RV64GC, RV32GC) > > > > Hi; thanks for this pull request. Unfortunately it seems to > > be missing Signed-off-by: tags. Every commit needs to have > > the Signed-off-by: tags from the people who contributed code to > > it, indicating that they're OK with the code going into QEMU. > > (If the work was done by and copyright a company then you don't > > need to provide signoffs from every person at the company who > > worked on the code if you don't want to.) > > > > > The spike_v1.9 > > > machine has been renamed to spike_v1.9.1 to match the privileged ISA > > > version and spike_v1.10 has been made the default machine. > > > > I'm confused about this. Generally QEMU boards should model > > hardware, and the board shouldn't care about the ISA versions. > > Versioned board names in QEMU generally follow _QEMU_'s versioning, > > and indicate that a board is identical to whatever we modelled > > in that earlier QEMU version, for VM migration compatibility. > > Board renames for minor ISA version bumps sounds like there's going > > to be a lot of churn and breakage -- is this stuff really ready? > > (Also, should we really have two different board source files > > for two different ISA versions? I would have expected these to > > share a source file to share code.) > > > > I did a test build and there are some compile errors: > > > > /home/pm215/qemu/linux-user/main.c:38:24: fatal error: target_elf.h: > > No such file or directory > > #include "target_elf.h" > > ^ > > compilation terminated. > > > > This is because your patchset has a clash with commit 542ca4349878a2e > > which has just merged to master, and refactors out an ifdef ladder, > > so now all targets supporting linux-user need to provide a > > linux-user/$ARCH/target_elf.h file. Could you fix that up and rebase, > > please? > also '[PATCH v7 03/23] RISC-V CPU Core Definition' still hasn't addressed > comment http://lists.gnu.org/archive/html/qemu-devel/2018-02/msg06412.html > which isn't fixed since it was first pointed out (v4). > > I'd prefer that being fixed before merge so another people > won't have to clean it up later after original authors, > When they try to generalize cpu_type -> cpu_model conversion. > I re-read the email and it doesn't seem clear what you want us to do. I changed the CPU suffix to a prefix as you requested. The rest of the CPU initialisation is "modelled" on arm not sh4. If you want to make a pull request, please use this branch: - https://github.com/riscv/riscv-qemu/tree/qemu-upstream-v7
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission
On Wed, Feb 28, 2018 at 6:50 AM, Peter Maydellwrote: > On 27 February 2018 at 15:50, Stef O'Rear wrote: > > On Tue, Feb 27, 2018 at 6:01 AM, Peter Maydell > wrote: > >> On 27 February 2018 at 00:15, Michael Clark wrote: > >>> The spike_v1.9 > >>> machine has been renamed to spike_v1.9.1 to match the privileged ISA > >>> version and spike_v1.10 has been made the default machine. > >> > >> I'm confused about this. Generally QEMU boards should model > >> hardware, and the board shouldn't care about the ISA versions. > > > > The spike boards model the Berkeley architectural simulator "spike" > > (https://github.com/riscv/riscv-isa-sim), which does not have a formal > > release process or version numbers so we are using the ISA version as > > a proxy for spike's version. > > > > When physical boards are released with full documentation I presume we > > will be adding board definitions for them, and they will imply > > specific ISA versions. > > > >> Versioned board names in QEMU generally follow _QEMU_'s versioning, > >> and indicate that a board is identical to whatever we modelled > >> in that earlier QEMU version, for VM migration compatibility. > > > > In this case we're handling two logically distinct boards. We could > > combine them and implement a parameter; I was having trouble finding a > > suitable example to follow earlier but it looks like gic-version in > > hw/arm/virt.c is one. This seems like a bad thing to change this late > > in the review though? > > You don't need to make them one board with a command line option > if that doesn't suit -- for instance hw/arm/vexpress.c defines > multiple board models that are variants on each other and > share a lot of code. That said, see below... > > >> Board renames for minor ISA version bumps sounds like there's going > >> to be a lot of churn and breakage -- is this stuff really ready? > >> (Also, should we really have two different board source files > >> for two different ISA versions? I would have expected these to > >> share a source file to share code.) > > > > 1.10 is the version we have committed to long term support for; it > > matches all public hardware the upstream Linux port, so it seems > > appropriate to use for QEMU. > > > > 1.9.1 was the version supported by riscv-qemu at the time Michael > > Clark took over maintainership; we have not removed support for it > > because we cannot prove that there is nobody depending on it, although > > I do not use it myself and do not know anyone else who does, so I > > would not personably object to removing it if that were required. > > I would rather not have stray legacy old versions in QEMU just > because we think maybe somebody might be using them. If 1.10 > is the long-term-support committed version, then I think we > should just have a model of that. Anybody who for some reason is > still stuck on an older unsupported version gets to find out > what "unsupported" means; they can always keep using whatever > old QEMU code base they've been using up til now, presumably. > SiFive are happy to support privileged ISA v1.9.1. I don't think the branch we maintain will easily merge with a branch that has privileged ISA v1.9.1 torn out (the only version that actually worked 4 months ago). If we can't submit our port with privileged ISA v1.9.1 suport then thats going to put a big spanner in the works. We've made a pretty strong choice to not break backwards compatibility going forward and privileged ISA v1.9.1 is the line in the sand so to speak. i.e. the QEMU port is still compatible with binaries from the v1.9.1 ISA spec published in November 2016 which has been implemented by many folk. We have to have a much more reasonable deprecation period. Software such as GDB and OpenOCD continue to support privileged ISA v1.9.1 and have specific fallback code paths, as well as the OpenOCD port having support for two versions of the debug spec.
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission
On Wed, Feb 28, 2018 at 4:50 AM, Stef O'Rearwrote: > On Tue, Feb 27, 2018 at 6:01 AM, Peter Maydell > wrote: > > On 27 February 2018 at 00:15, Michael Clark wrote: > >> -BEGIN PGP SIGNED MESSAGE- > >> Hash: SHA1 > >> > >> The following changes since commit 0a773d55ac76c5aa89ed9187a3bc5a > f8c5c2a6d0: > >> > >> maintainers: Add myself as a OpenBSD maintainer (2018-02-23 12:05:07 > +) > >> > >> are available in the git repository at: > >> > >> https://github.com/riscv/riscv-qemu.git tags/riscv-qemu-upstream-v7 > >> > >> for you to fetch changes up to 170a9d412ca1eb3b7ae6f6c1ff86dc > bdff0fd7a8: > >> > >> RISC-V Build Infrastructure (2018-02-27 11:09:43 +1300) > >> > >> - > >> QEMU RISC-V Emulation Support (RV64GC, RV32GC) > > > > Hi; thanks for this pull request. Unfortunately it seems to > > be missing Signed-off-by: tags. Every commit needs to have > > the Signed-off-by: tags from the people who contributed code to > > it, indicating that they're OK with the code going into QEMU. > > (If the work was done by and copyright a company then you don't > > need to provide signoffs from every person at the company who > > worked on the code if you don't want to.) > > I'll add mine. > > >> The spike_v1.9 > >> machine has been renamed to spike_v1.9.1 to match the privileged ISA > >> version and spike_v1.10 has been made the default machine. > > > > I'm confused about this. Generally QEMU boards should model > > hardware, and the board shouldn't care about the ISA versions. > > The spike boards model the Berkeley architectural simulator "spike" > (https://github.com/riscv/riscv-isa-sim), which does not have a formal > release process or version numbers so we are using the ISA version as > a proxy for spike's version. > > When physical boards are released with full documentation I presume we > will be adding board definitions for them, and they will imply > specific ISA versions. > > > Versioned board names in QEMU generally follow _QEMU_'s versioning, > > and indicate that a board is identical to whatever we modelled > > in that earlier QEMU version, for VM migration compatibility. > > In this case we're handling two logically distinct boards. We could > combine them and implement a parameter; I was having trouble finding a > suitable example to follow earlier but it looks like gic-version in > hw/arm/virt.c is one. This seems like a bad thing to change this late > in the review though? > > > Board renames for minor ISA version bumps sounds like there's going > > to be a lot of churn and breakage -- is this stuff really ready? > > (Also, should we really have two different board source files > > for two different ISA versions? I would have expected these to > > share a source file to share code.) > > 1.10 is the version we have committed to long term support for; it > matches all public hardware the upstream Linux port, so it seems > appropriate to use for QEMU. > > 1.9.1 was the version supported by riscv-qemu at the time Michael > Clark took over maintainership; we have not removed support for it > because we cannot prove that there is nobody depending on it, although > I do not use it myself and do not know anyone else who does, so I > would not personably object to removing it if that were required. > > Combining spike_v1.10 and spike_v1.9.1 would also be an option amenable to > us. > I've just talked to SiFive about this. They have agreed that we can remove the sifive_e300 and sifive_u500 boards from the patch series that we are going to submit upstream again later this week or early next week. These machines and their devices are pretty easy for us to maintain in the riscv or a sifive repo. This trims the number of machines from 5 to 3 and lets us remove the SiFiveUART and SiFivePRCI from the next patch series we are going to submit. e.g. v8 SiFive have indicated that they would like to keep privileged ISA v1.9.1 support. It's likely the RISC-V foundation would also like us to start supporting backwards compatibility from this point. Removing support for a specification version only 4 months after the latest specification has been implemented is too severe of a deprecation period. They have said they would like QEMU to support at least 2 specification versions so we won't consider removing privileged ISA v1.9.1 support until privileged ISA v1.11 has been released and implemented. There are still several OS ports and private tape-outs and test chips that target privileged ISA v1.9.1. In fact, someone may very well add privileged ISA v1.9 and privileged ISA v1.7 support, perhaps as a computing history project. The published specifications are all available however the chips implementing these versions of the spec are mostly test chips. Nevertheless, they are part of RISC-V history. With respect to combining them, we could investigate triggering the config
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission
On 27 February 2018 at 15:50, Stef O'Rearwrote: > On Tue, Feb 27, 2018 at 6:01 AM, Peter Maydell > wrote: >> On 27 February 2018 at 00:15, Michael Clark wrote: >>> The spike_v1.9 >>> machine has been renamed to spike_v1.9.1 to match the privileged ISA >>> version and spike_v1.10 has been made the default machine. >> >> I'm confused about this. Generally QEMU boards should model >> hardware, and the board shouldn't care about the ISA versions. > > The spike boards model the Berkeley architectural simulator "spike" > (https://github.com/riscv/riscv-isa-sim), which does not have a formal > release process or version numbers so we are using the ISA version as > a proxy for spike's version. > > When physical boards are released with full documentation I presume we > will be adding board definitions for them, and they will imply > specific ISA versions. > >> Versioned board names in QEMU generally follow _QEMU_'s versioning, >> and indicate that a board is identical to whatever we modelled >> in that earlier QEMU version, for VM migration compatibility. > > In this case we're handling two logically distinct boards. We could > combine them and implement a parameter; I was having trouble finding a > suitable example to follow earlier but it looks like gic-version in > hw/arm/virt.c is one. This seems like a bad thing to change this late > in the review though? You don't need to make them one board with a command line option if that doesn't suit -- for instance hw/arm/vexpress.c defines multiple board models that are variants on each other and share a lot of code. That said, see below... >> Board renames for minor ISA version bumps sounds like there's going >> to be a lot of churn and breakage -- is this stuff really ready? >> (Also, should we really have two different board source files >> for two different ISA versions? I would have expected these to >> share a source file to share code.) > > 1.10 is the version we have committed to long term support for; it > matches all public hardware the upstream Linux port, so it seems > appropriate to use for QEMU. > > 1.9.1 was the version supported by riscv-qemu at the time Michael > Clark took over maintainership; we have not removed support for it > because we cannot prove that there is nobody depending on it, although > I do not use it myself and do not know anyone else who does, so I > would not personably object to removing it if that were required. I would rather not have stray legacy old versions in QEMU just because we think maybe somebody might be using them. If 1.10 is the long-term-support committed version, then I think we should just have a model of that. Anybody who for some reason is still stuck on an older unsupported version gets to find out what "unsupported" means; they can always keep using whatever old QEMU code base they've been using up til now, presumably. thanks -- PMM
Re: [Qemu-devel] [patches] Re: [PULL] RISC-V QEMU Port Submission
On Tue, Feb 27, 2018 at 6:01 AM, Peter Maydellwrote: > On 27 February 2018 at 00:15, Michael Clark wrote: >> -BEGIN PGP SIGNED MESSAGE- >> Hash: SHA1 >> >> The following changes since commit 0a773d55ac76c5aa89ed9187a3bc5af8c5c2a6d0: >> >> maintainers: Add myself as a OpenBSD maintainer (2018-02-23 12:05:07 +) >> >> are available in the git repository at: >> >> https://github.com/riscv/riscv-qemu.git tags/riscv-qemu-upstream-v7 >> >> for you to fetch changes up to 170a9d412ca1eb3b7ae6f6c1ff86dcbdff0fd7a8: >> >> RISC-V Build Infrastructure (2018-02-27 11:09:43 +1300) >> >> - >> QEMU RISC-V Emulation Support (RV64GC, RV32GC) > > Hi; thanks for this pull request. Unfortunately it seems to > be missing Signed-off-by: tags. Every commit needs to have > the Signed-off-by: tags from the people who contributed code to > it, indicating that they're OK with the code going into QEMU. > (If the work was done by and copyright a company then you don't > need to provide signoffs from every person at the company who > worked on the code if you don't want to.) I'll add mine. >> The spike_v1.9 >> machine has been renamed to spike_v1.9.1 to match the privileged ISA >> version and spike_v1.10 has been made the default machine. > > I'm confused about this. Generally QEMU boards should model > hardware, and the board shouldn't care about the ISA versions. The spike boards model the Berkeley architectural simulator "spike" (https://github.com/riscv/riscv-isa-sim), which does not have a formal release process or version numbers so we are using the ISA version as a proxy for spike's version. When physical boards are released with full documentation I presume we will be adding board definitions for them, and they will imply specific ISA versions. > Versioned board names in QEMU generally follow _QEMU_'s versioning, > and indicate that a board is identical to whatever we modelled > in that earlier QEMU version, for VM migration compatibility. In this case we're handling two logically distinct boards. We could combine them and implement a parameter; I was having trouble finding a suitable example to follow earlier but it looks like gic-version in hw/arm/virt.c is one. This seems like a bad thing to change this late in the review though? > Board renames for minor ISA version bumps sounds like there's going > to be a lot of churn and breakage -- is this stuff really ready? > (Also, should we really have two different board source files > for two different ISA versions? I would have expected these to > share a source file to share code.) 1.10 is the version we have committed to long term support for; it matches all public hardware the upstream Linux port, so it seems appropriate to use for QEMU. 1.9.1 was the version supported by riscv-qemu at the time Michael Clark took over maintainership; we have not removed support for it because we cannot prove that there is nobody depending on it, although I do not use it myself and do not know anyone else who does, so I would not personably object to removing it if that were required. Combining spike_v1.10 and spike_v1.9.1 would also be an option amenable to us. -s
Re: [Qemu-devel] Patches
On Wed, Jul 15, 2015 at 12:26:44PM +0100, Peter Maydell wrote: On 15 July 2015 at 09:52, Stefan Hajnoczi stefa...@redhat.com wrote: On Tue, Jul 14, 2015 at 10:58:41AM +0100, Peter Maydell wrote: On 14 July 2015 at 09:12, Peter Crosthwaite peter.crosthwa...@xilinx.com wrote: Hi Stefan, Peter, Is patches down? I don't see any patches for the last 3 days and there is definitely new things on list: $ ./patches fetch http://vmsplice.net/~patches/patches.json Fetched info on 689 patch series Fetching mboxes... $ ./patches list | more Message-id: 1436556159-3002-1-git-send-email...@weilnetz.de From: Stefan Weil s...@weilnetz.de Date: 2015-07-10 Tags: for-2.4 [1/1] tci: Fix regression with INDEX_op_qemu_st_i32, INDEX_op_qemu_st_i64 The other patches instance also seems to have got wedged (http://wiki.qemu.org/patches/patches.json): that 2015-07-10 patch is the last one it knows about too. Strange, that is the same file as http://qemu-project.org/patches/patches.json. It unwedged itself later that same day. I experienced something weird today as well. patches fetch downloaded an incomplete patches.json file :(. I'll have to keep an eye on the server... Stefan pgpDqjotpK7wm.pgp Description: PGP signature
Re: [Qemu-devel] Patches
On Tue, Jul 14, 2015 at 10:58:41AM +0100, Peter Maydell wrote: On 14 July 2015 at 09:12, Peter Crosthwaite peter.crosthwa...@xilinx.com wrote: Hi Stefan, Peter, Is patches down? I don't see any patches for the last 3 days and there is definitely new things on list: $ ./patches fetch http://vmsplice.net/~patches/patches.json Fetched info on 689 patch series Fetching mboxes... $ ./patches list | more Message-id: 1436556159-3002-1-git-send-email...@weilnetz.de From: Stefan Weil s...@weilnetz.de Date: 2015-07-10 Tags: for-2.4 [1/1] tci: Fix regression with INDEX_op_qemu_st_i32, INDEX_op_qemu_st_i64 The other patches instance also seems to have got wedged (http://wiki.qemu.org/patches/patches.json): that 2015-07-10 patch is the last one it knows about too. Strange, that is the same file as http://qemu-project.org/patches/patches.json. When I wget http://wiki.qemu.org/patches/patches.json I see more recent patches in the JSON. Can you double-check that you're getting a stale file? Stefan pgpa7M9z0Hmgs.pgp Description: PGP signature
Re: [Qemu-devel] Patches
On 15 July 2015 at 09:52, Stefan Hajnoczi stefa...@redhat.com wrote: On Tue, Jul 14, 2015 at 10:58:41AM +0100, Peter Maydell wrote: On 14 July 2015 at 09:12, Peter Crosthwaite peter.crosthwa...@xilinx.com wrote: Hi Stefan, Peter, Is patches down? I don't see any patches for the last 3 days and there is definitely new things on list: $ ./patches fetch http://vmsplice.net/~patches/patches.json Fetched info on 689 patch series Fetching mboxes... $ ./patches list | more Message-id: 1436556159-3002-1-git-send-email...@weilnetz.de From: Stefan Weil s...@weilnetz.de Date: 2015-07-10 Tags: for-2.4 [1/1] tci: Fix regression with INDEX_op_qemu_st_i32, INDEX_op_qemu_st_i64 The other patches instance also seems to have got wedged (http://wiki.qemu.org/patches/patches.json): that 2015-07-10 patch is the last one it knows about too. Strange, that is the same file as http://qemu-project.org/patches/patches.json. It unwedged itself later that same day. -- PMM
Re: [Qemu-devel] Patches
On 14 July 2015 at 09:12, Peter Crosthwaite peter.crosthwa...@xilinx.com wrote: Hi Stefan, Peter, Is patches down? I don't see any patches for the last 3 days and there is definitely new things on list: $ ./patches fetch http://vmsplice.net/~patches/patches.json Fetched info on 689 patch series Fetching mboxes... $ ./patches list | more Message-id: 1436556159-3002-1-git-send-email...@weilnetz.de From: Stefan Weil s...@weilnetz.de Date: 2015-07-10 Tags: for-2.4 [1/1] tci: Fix regression with INDEX_op_qemu_st_i32, INDEX_op_qemu_st_i64 The other patches instance also seems to have got wedged (http://wiki.qemu.org/patches/patches.json): that 2015-07-10 patch is the last one it knows about too. -- PMM
Re: [Qemu-devel] Patches
On Tue, Jul 14, 2015 at 01:12:22AM -0700, Peter Crosthwaite wrote: Hi Stefan, Peter, Is patches down? I don't see any patches for the last 3 days and there is definitely new things on list: $ ./patches fetch http://vmsplice.net/~patches/patches.json Please switch ~/.patchesrc to this new official URL: http://qemu-project.org/patches/patches.json I'll investigate why the old URL isn't working anymore but the official URL is best anyway. Stefan pgp_gyv5yZ4Wj.pgp Description: PGP signature
Re: [Qemu-devel] [PATCHES for 1.0] various spice usb-redir integration patches
On 11/19/2011 03:22 AM, Hans de Goede wrote: Hi All, Sorry for sending these in so late, I send most of them in before a long time ago, but then they got stuck on waiting for the big chardev rewrite. Since it seems clear now that the big chardev rewrite won't happen before 1.0, I would like to get these into 1.0 as is. Applied all. Thanks. Regards, Anthony Liguori Thanks Regards, Hans
Re: [Qemu-devel] [PATCHES for 1.0] various spice usb-redir integration patches
On 11/21/11 22:05, Anthony Liguori wrote: On 11/19/2011 03:22 AM, Hans de Goede wrote: Hi All, Sorry for sending these in so late, I send most of them in before a long time ago, but then they got stuck on waiting for the big chardev rewrite. Since it seems clear now that the big chardev rewrite won't happen before 1.0, I would like to get these into 1.0 as is. This seems pretty well isolated. I'm not going to put this in -rc3 because I'd like to see Gerd at least Ack 2/5. Once it has an Ack, I'll take it either for -rc4 or for 1.0.z. Series looks good. Acked-by: Gerd Hoffmann kra...@redhat.com cheers, Gerd
Re: [Qemu-devel] [PATCHES for 1.0] various spice usb-redir integration patches
On 11/19/2011 03:22 AM, Hans de Goede wrote: Hi All, Sorry for sending these in so late, I send most of them in before a long time ago, but then they got stuck on waiting for the big chardev rewrite. Since it seems clear now that the big chardev rewrite won't happen before 1.0, I would like to get these into 1.0 as is. This seems pretty well isolated. I'm not going to put this in -rc3 because I'd like to see Gerd at least Ack 2/5. Once it has an Ack, I'll take it either for -rc4 or for 1.0.z. Regards, Anthony Liguori Thanks Regards, Hans
Re: [Qemu-devel] Patches for SMSC LAN911X driver
Hi, Am 11.11.2011 14:44, schrieb Cachet Bertrand: Patch is contained in the following commit : https://bitbucket.org/bca/qemu-linaro/changeset/0aa1f76e5141 I have sent this patch to qemu-linaro but Peter Maydell (pm215 on #qemu IRC channel) told me to send it here because it is against upstream qemu. That's probably not what Peter meant. :) Please see http://wiki.qemu.org/Contribute/SubmitAPatch In particular the patch needs to be sent with git-send-email, for review and git-am compatibility; there should be a short summary line showing what part of code (file) it applies to (before the in-depth description); and it needs Signed-off-by(s). Regards, Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] Patches for SMSC LAN911X driver
On 11 November 2011 13:44, Cachet Bertrand bertrand.cac...@heig-vd.ch wrote: In the driver for the SMSC LAN9118 device (hw/lan9118.c), I modify the code to update the PM_CTRL register (switch PM_MODE bits to D0 and set (1b) READY bit ) when writing to the BYTE_TEST register. As Andreas says, if you can submit your patches in line with the guidelines in http://wiki.qemu.org/Contribute/SubmitAPatch I'd appreciate it. (In particular we can't apply any patch without a Signed-off-by: line.) On the meat of the patch: s-pmt_ctrl = ~0x03000; s-pmt_ctrl |= 0x01; At the moment these two lines will always have no effect, because pmt_ctrl is set to 0x1 on initialisation and the register-writing code never allows bits 0x3000 to be set or bit 0x1 to be cleared. So I'm wondering if we should implement the other power modes in a slightly more serious way. (Or perhaps we should just have a comment that that code currently does nothing but is there for if/when we do do the other power modes properly. Dunno.) More generally, the data sheet is clear that read only means writes ignored and write only means reads as zero so we should make sure we implement that for all registers (not just this one) rather than giving a hw_error(). -- PMM
Re: [Qemu-devel] Patches for OpenBSD support.
On Sun, Dec 19, 2010 at 8:59 PM, Brad b...@comstyle.com wrote: Here are a few patches for OpenBSD support. Thanks. Could you resend each patch separately and add Signed-off-by: Brad Smith b...@comstyle.com line before '---' line?
Re: [Qemu-devel] Patches in bug tracker also sent to mailing list?
On Thursday 22 April 2010 09:44:33 Kraai, Matt wrote: Hi, I've submitted two trivial patches against QEMU in its bug tracker: https://bugs.launchpad.net/qemu/+bug/568053 https://bugs.launchpad.net/qemu/+bug/568442 Is this sufficient or should I also submit them to the mailing list? You'll want to send them to the list as well. That's where devs review patches. Matt
Re: [Qemu-devel] Patches in bug tracker also sent to mailing list?
On Thu, Apr 22, 2010 at 9:44 AM, Kraai, Matt matt.kr...@amo.abbott.com wrote: I’ve submitted two trivial patches against QEMU in its bug tracker: https://bugs.launchpad.net/qemu/+bug/568053 https://bugs.launchpad.net/qemu/+bug/568442 Is this sufficient or should I also submit them to the mailing list? Anthony has said that he can only accept patches sent to the mailing list, with a Signed-off-by line. Patches can land in Launchpad for discussion within the bug report, but he really must have the patches on the mailing list in order to be accepted upstream. :-Dustin
Re: [Qemu-devel] Patches?
On Monday 17 March 2008 07:19:18 am Gary Thomas wrote: I've fixed a number of problems with the user mode code. Where's the best place to send/propose/discuss these? What's the preferred format? The best place to send patches is this mailing list. I think they have to be a unified diff, i.e. diff -u. signature.asc Description: This is a digitally signed message part.
Re: [Qemu-devel] Patches?
On Mon, Mar 17, 2008 at 07:19:18AM -0600, Gary Thomas wrote: I've fixed a number of problems with the user mode code. Where's the best place to send/propose/discuss these? What's the preferred format? Hello Gary, Just send the patches to this list as unified diffs, preferably inlined to the email to ease reviewing. If the patch is large consider splitting it into several patches and emails. Thanks -- Edgar E. Iglesias Axis Communications AB
Re: [Qemu-devel] Patches?
On Mon, Mar 17, 2008 at 10:42:51AM -0600, C.W. Betts wrote: The best place to send patches is this mailing list. I think they have to be a unified diff, i.e. diff -u. I'd also recommend using the '-p' option, which shows the name of the function that each change is in. Cheers, -- Stuart Brady
Re: [Qemu-devel] patches for qemu-cvs on NetBSD
Hetz Ben Hamo wrote: Have you tried to see if the same problem appears with KQEMU installed on Linux host? Yes, I can confirm that the same thing happens with kqemu installed on Linux host with Linux guest. -- Jason Brittain ___ Qemu-devel mailing list Qemu-devel@nongnu.org http://lists.nongnu.org/mailman/listinfo/qemu-devel