Re: [Qemu-devel] [PATCH v1 00/21] RISC-V QEMU Port Submission v1

[Michael Clark]

On Thu, Jan 4, 2018 at 12:35 AM, Richard W.M. Jones <address@hidden>
wrote:

> Just a few small points:
>
> (1) I've built Fedora RPMs from this patch set [approximately - I'm
> using a very slightly different / slightly older version, but it's not
> substantively different]:
>
>   http://copr-fe.cloud.fedoraproject.org/coprs/rjones/riscv/
>
> It works well for me building plenty of Fedora packages over the past
> few weeks, except for a possible Linux kernel bug which interacts with
> the patch set not handling EBREAK correctly, which I had to patch
> around (in the kernel):
>
>   https://groups.google.com/a/groups.riscv.org/forum/#!msg/
> sw-dev/v05FjcGC1EI/atXXUAcsCgAJ


Do you have any evidence that the RISC-V QEMU port is not handling ebreak
correctly? I've reviewed the code and looked at your kernel oops and QEMU
appears to be correctly trapping with a breapoint exception. At this point
I think the issue is a riscv linux-kernel bug until we have any evidence to
the contrary. I suspect from your ooops message that you are encountering
an ebreak in kernel code given mepc contained a kernel address. GCC 7.x now
inserts ebreaks in certain conditions i.e. if there is a null dereference
that should not be reached.

There is an erroneous fprintf that should perhaps be removed, as the debug
mode support is not required to handle breakpoint exceptions.

-
https://github.com/riscv/riscv-qemu/blob/qemu-upstream-v1/target/riscv/helper.c#L394-L396

Debug mode in the fprintf is referring to JTAG debug as specified in the
Draft RISC-V External Debug Support specification. We don't intend to
implement JTAG debug emulation in RISC-V QEMU any time soon, so ebreak
should just trap and the printf can be removed. We could potentially add
trace support for exceptions. The RISC-V support doesn't yet implement any
tracing hooks. I've been trying to adjust stray printf's to use
error_report but this is one I missed. I would like to add trace support
for interrupts and exceptions which would be more generally useful.

(2) I'm worried that this patch starts off using virtio-mmio instead
> of virtio-pci.  virtio-pci is better in every respect than
> virtio-mmio, and while it may be a good interim solution I think we
> need to have a plan to get rid of it eventually, and should make it
> clear that virtio-mmio is not a permanent ABI so we don't get into the
> same situation that we did with -M virt on ARM.
>

I've noted in [PATCH v1 00/00] that we have a goal to add GPEX to the
SiFive RISC-V virt board. The idea with the initial version of the virt
board is to provide enough infrastructure so that distro builders can use
RISC-V QEMU. Prior to the work to implement PLIC, device-tree and VirtIO,
there was no network and block device support so it was not possible to use
QEMU for distro bring up. Now RISC-V QEMU is usable. Data point: the arm
'virt' board still supports VirtIO MMIO. I don't think this should be a
blocking issue. I've looked at device support in other ports and it seems
unreasonable to set such a high bar for inclusion of a port. We would, as
mentioned in [PATCH v1 00/00], like to shift our development focus to
upstream QEMU versus maintaining an out-of-tree port so I don't think some
as yet unsupported feature should be a barrier to inclusion. We also don't
/yet/ support live migration. We currently only support 8 CPUs due to a
hardcoded limit in BBL. There is also RISC-V FPGA Soft Core IP that uses
the XILINX PCIe controller, and given that we have a goal to maintain
compatibility with Soft Core IP, there may be RISC-V machines that use the
XILINX PCIe controller.

You can be certain that we are aware of the benefits of VirtIO PCI.

If PCI is a blocking issue we can remove the 'virt' machine from the
patchset. It's not necessary for the RISC-V port to be usable. i.e. we can
maintain it in the riscv.org tree.

(3) poweroff doesn't work if you use -M virt (and hence don't use
> HTIF).  I wrote a dummy poweroff device to get around this, but I
> think it could raise a bigger point: Why bother to support HTIF
> machine types at all?  The reason given in the patch set is because
> spike (the cycle-accurate RISC-V emulator) only supports HTIF but
> is that a good reason?
>

Spike (aka 'riscv-isa-sim') is not a cycle-accurate simulator. It's the
RISC-V "golden reference" ISA simulator. i.e. is the canonical behavioural
simulator for the RISC-V ISA.

We are going to support HTIF on the Spike machines and this patch set is
consistent with that policy. Currently HTIF is the only IO mechanism
supported by Spike. It is our goal to maintain Spike compatible machines.
This is important for binary compatibility and verification reasons that we
can emulate 'riscv-isa-sim'. HTIF is not appropriate for the other machines
for reasons already discussed on the RISC-V Software Development mailing
list however HTIF will be maintained and supported while it remains the
only supported IO mechanism on Spike. If it was removed, we would need to
remove the Spike machines which would be quite unfortunate.

The plan for power off and reset is to implement SiFive's AON/PRCI block
(Always On / Power, Reset, Interrupt). We may indeed rename 'virt' to
'sifive_virt' to avoid any comparisons with the arm 'virt' machine. The
SiFive 'virt' machine is derived from SiFive's Freedom U500 machine and
implements the SiFive CLINT (Core Local Interruptor) and PLIC (Platform
Level Interrupt Controller). It's device-tree is unique to the device-tree
used by SiFive's hardware.

If HTIF is a blocking issue we can remove the 'spike' machines from the
patchset. It's not necessary for the RISC-V port to be usable. i.e. we can
maintain it in the riscv.org tree.

The sooner we can get through patch review for the bulk of the port, the
sooner we can shift our attention to future development. i.e. AON/PRCI for
power off and reset and GPEX for virtio-pci.

There is already ~14,000 LOC for review so I don't think deferring
inclusion based on as yet undeveloped features is a very good rationale.
RISC-V has not been around for 30+ years so comparison to arm features at
this point is a little bit unfair. EM_MOXIE 223 and EM_RISCV 243 are
perhaps more fair comparisons, although moxie is already in QEMU.