Re: [Qemu-devel] [PULL] RISC-V QEMU Port Submission v8

[Michael Clark]

On Tue, Mar 6, 2018 at 12:10 PM, Michael Clark <address@hidden> wrote:

>
>
> On Sun, Mar 4, 2018 at 11:52 AM, Peter Maydell <address@hidden>
> wrote:
>
>> On 3 March 2018 at 02:46, Michael Clark <address@hidden> wrote:
>> > On Sat, Mar 3, 2018 at 3:22 AM, Peter Maydell <address@hidden
>> >
>> > wrote:
>> >> Please don't send pull requests until after patches have been put
>> >> on list and been reviewed. A minor update to a pullreq is OK if
>> >> it's something like a trivial compiler fix or just dropping some
>> >> patches that had problems, but if you have this many changes that
>> >> deserves a fresh patchset to be sent to the list for review.
>> >>
>> >> (For the QEMU workflow, a pull request isn't a request for patch
>> >> review, it's a statement that patches have all had review and
>> >> are ready to go into master immediately.)
>> >
>> >
>> > My apoligies. I won't do this again.
>>
>> No worries, it's just a workflow thing (which differs a lot from
>> project to project), and we don't really have much documentation
>> on the submaintainer part of the process. (What we do have is here:
>> https://wiki.qemu.org/Contribute/SubmitAPullRequest  )
>>
>> The basic idea is that for us code review happens in the "patches
>> posted to list" phase, and then "pull request" is pretty much
>> the same as "commit to master". As the submaintainer you review,
>> test and accumulate in a branch patches from yourself and other
>> people, and then send them out in a pull request. In the ideal
>> case that goes straight into master without problems. Sometimes
>> it runs into trouble (like a compile issue on an oddball platform),
>> and then rather than going through the whole process again for
>> something as small as a messed up format string you can just fix
>> and resend the pullreq. (There are examples of this on list at
>> the moment, for instance.)
>> Bigger stuff it's usually easier to drop the relevant patches
>> from the pull, and then respin them and resend for review before
>> putting them in a later pull. The dividing line for what you
>> can get away with fixing up locally and what you can't is
>> kind of similar to what you can tweak without needing to drop
>> a reviewed-by: tag from a changed patch and get it re-reviewed.
>> When you get familiar with the process and what people do you
>> can take shortcuts sometimes (this is me posting what I'm
>> going to squash into a patch as a followup, to save reposting
>> a 20 patch series, for instance:
>> http://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg00310.html).
>> For getting started as a submaintainer, it's probably easiest
>> to follow the 'by the book' process: patches go to mailing
>> list as 'PATCH', get review, changes made, patch series resent,
>> reviewed patches go into pull requests. (The idea is to ensure
>> that anything that goes into master has been on list for at
>> least a few days so people who want to review can do so.)
>>
>> > I have some very very minor cleanups that do not affect logic, but
>> perhaps
>> > we could address this after getting approval to make a pull request for
>> v8.
>> >
>> > My qemu-devel branch holds changes against the latest rebase:
>> >
>> > - https://github.com/michaeljclark/riscv-qemu/tree/qemu-devel
>> >
>> > Someone raised timebase frequency on the RISC-V sw-dev and after
>> looking at
>> > the code I noticed we had a hard-coded value for a few of the constants
>> we
>> > put in device tree, and i spotted a missed rename. I'm going to have to
>> > learn about the qemu-devel process for trivial fixes...
>>
>> I would probably squash in the missed rename into the relevant
>> patch, at any rate. The rest can probably go through the post
>> patch/get review/sent pull request cycle after this first lot
>> have been applied.
>>
>
> It seems I need to rebase against master based on recent changes, so I'll
> make a v9 tag, include the minimal missed rename but exluding my queued
> changes. The changes in v8 are the minimum required to allow us to keep the
> SiFive machines as we renamed them and added the SiFive cpu models. The
> essence of the v8 patch series.
>
> The trivial fixes to v8 ended up becoming non-trivial and now includes a
> lot of specification conformance issues (essentially specification related
> bug fixes) and a fix for a potential buffer overflow wrt. device-tree, that
> doesn't appear to trigger in practice. I was also able to shed some dead
> code (machines aren't objects, so empty object boilerplate was removed):
>
> - https://github.com/riscv/riscv-qemu/pull/112
>
> We'll hold off merging these changes into the v8 branch, and will send
> them as a post merge cleanup and bugfix patch series, hopefully after
> getting the port merged.
>
> I'll get back to you when I have rebased against master...
>

I've squashed the trivial spike rename fix and rebased against master as of
commit f2bb2d14c2958f3f5aef456bd2cdb1ff99f4a562 Merge remote-tracking
branch 'remotes/stefanha/tags/block-pull-request' into staging. See here:

- https://github.com/riscv/riscv-qemu/releases/tag/riscv-qemu-upstream-v8.1

I've rebased my queued changes against the qemu-upstream-v8.1 branch (tag r
iscv-qemu-upstream-v8.1) and have them in my own tree:

- https://github.com/michaeljclark/riscv-qemu/tree/qemu-devel

I have a pending pull request against the riscv.org tree for my queued
changes, which I may re-target if we get these changes upstream:

- https://github.com/riscv/riscv-qemu/pull/115

Would appreciate it some folk could add a remote for
github.com/riscv/riscv-qemu.git and pull in riscv-qemu-upstream-v8.1, and
test... It's been pretty well tested out-of-tree already. RISC-V docs are
on the wiki, which require the riscv-toolchain, building and packaging
riscv-linux inside riscv-pk/bbl.

- https://github.com/riscv/riscv-qemu/wiki

I'm currently working on some changes locally to split the firmware from
the kernel image. Currently the kernel image is embedded in the firmware (r
iscv-pk/bbl). I want to do this as I'd like to get the riscv CI to automate
some more of the "execution environment" testing (which could be spike,
qemu, FPGA, or silicon). I'm also working on cross toolchain packages to
automate some of the test image creation. I perform various local tests
manually. i.e. booting linux on the various board combinations, and running
MCU binaries on the MMU-less SiFive E-series board, which can run HiFive1
binaries (i.e. a little more than make check). I'm pretty confident that we
won't hurt any other arches. The only core code change is to add a symbol
callback function to load_elf for our HTIF (Host Target Interface).

We have some objectives with regards to the sifive_e and sifive_u machines,
> such that we may properly abstract the SOC/board and also have a
> configurable SOC as we need to model configurable soft-core IP as well as
> actual SOCs based on this IP. This will be in the future. The machines at
> this point are relatively simple.
>
> Thanks,
> Michael.
>