Skip to content
This repository has been archived by the owner on Apr 13, 2019. It is now read-only.

riscv: virt machine: multiple UARTs #159

Open
wants to merge 76 commits into
base: riscv-all
Choose a base branch
from
Open

riscv: virt machine: multiple UARTs #159

wants to merge 76 commits into from

Conversation

nwf
Copy link

@nwf nwf commented Aug 24, 2018

Attach up to 16 serial devices, densely packed in the memory map and
registered in the FDT. To ensure that BBL continues to find the default
one, we register it last so that it ends up at the top of the fdt.

@nwf
Copy link
Author

nwf commented Aug 26, 2018

The CI failure seems unrelated: 'ERROR:tests/tpm-emu.c:27:tpm_emu_test_wait_cond: code should not be reached'

michaeljclark
michaeljclark reviewed Aug 26, 2018
View reviewed changes
hw/riscv/virt.c Outdated
VIRT_PLIC_CONTEXT_STRIDE,
memmap[VIRT_PLIC].size);

/* Attach UARTs */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comment indentation issue

@michaeljclark
Copy link
Collaborator

It looks okay to me, besides minor style issues.

You can use git format-patch to make a patch and then run ./scripts/checkpatch.pl which will check that the patch complies with the QEMU coding conventions. I'll check out the branch and do some local testing... I have to run checkpatch.pl before making a pull request anyhow...

Is there any reason why didn't just reverse iterate in the loop so that you don't need to special case uart0?

@nwf
Copy link
Author

nwf commented Aug 26, 2018

Will fix the style nits; thanks for pointing them out.

I wanted to use the first MAX_UARTS given, not the last, if there happened to be more. IMHO it'd be better if BBL paid attention to /chosen/stdout or some similar option rather than always taking the first UART it finds in the FDT, but given how QEMU builds the FDT, the BBL UART has to be constructed last.

@nwf
Copy link
Author

nwf commented Sep 11, 2018

Ping? As said above, the CI failure appears unrelated: "ERROR:tests/tpm-emu.c:27:tpm_emu_test_wait_cond: code should not be reached". Can this be merged?

stsquad and others added 22 commits September 17, 2018 12:37
@stsquad
tests/.gitignore: don't ignore docker tests
3cb3cd3 
The .gitignore was being a little over enthusiastic hiding files.

Signed-off-by: Alex Bennée <[email protected]>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
@stsquad
docker: base debian-tricore on qemu:debian9
2d9bf24 
We need both git and a working compiler to build the tools. Although
the qemu:debian9 image also has a bunch of extra dependencies it would
be fairly unusual for a user not to already have this layer available
for one of our many other docker images so lets not complicate things.

Signed-off-by: Alex Bennée <[email protected]>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
@stsquad
docker: par down QEMU_CONFIGURE_OPTS in debian-tricore-cross
4c5e217 
This image isn't going to build anything significant as it is just
intended for building test cases. In case it does end up getting
inadvertently included in a build lets aim for the minimal possible
product.

Signed-off-by: Alex Bennée <[email protected]>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
@stsquad
docker: fail more gracefully on docker.py check
1c2416b 
As this is called directly from the Makefile while determining
dependencies and it is possible the user was configured in one window
but not have credentials in the other. Let's catch the Exceptions and
deal with it quietly.

Signed-off-by: Alex Bennée <[email protected]>
Reported-by: Peter Maydell <[email protected]>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
@stsquad
docker: split configure_qemu from build_qemu
f72927a 
This allows some tests that just want to configure QEMU's source tree
to do so.

Signed-off-by: Alex Bennée <[email protected]>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
Tested-by: Philippe Mathieu-Daudé <[email protected]>
@stsquad
docker: move make check into check_qemu helper
4210a7f 
Not all docker images can run the check step. Let's move everything
into a common helper so we don't need to replicate checks in the
future.

Signed-off-by: Alex Bennée <[email protected]>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
Tested-by: Philippe Mathieu-Daudé <[email protected]>
@stsquad
docker: gracefully skip check_qemu
2b64400 
Not all our images are able to run the tests. Rather than use features
we can just check for the existence and run-ability of gtester. If the
image has been setup for binfmt_misc it will be able to run anyway.

Signed-off-by: Alex Bennée <[email protected]>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
Tested-by: Philippe Mathieu-Daudé <[email protected]>
@stsquad
docker: Makefile.include don't include partial images
ce06c4c 
Rename DOCKER_INTERMEDIATE_IMAGES to DOCKER_PARTIAL_IMAGES and add the
incomplete cross compiler images that can build tests but can't build
QEMU itself. We also add debian, debian-bootstrap and the tricode
images to the list.

Signed-off-by: Alex Bennée <[email protected]>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
Tested-by: Philippe Mathieu-Daudé <[email protected]>
@stsquad
docker: add test-unit runner
bbf7171 
This test doesn't even build QEMU, it just builds and runs all the
unit tests. Intended to make checking unit tests on all docker images
easier.

Signed-off-by: Alex Bennée <[email protected]>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
Tested-by: Philippe Mathieu-Daudé <[email protected]>
@stsquad
docker: add expansion for docker-test-FOO to Makefile.include
a792c98 
This allows us to run a particular test on all docker images. For
example:

  make docker-test-unit

Will run the unit tests on every supported image. At the same time
rename docker-test to docker-all-tests to be clearer.

Signed-off-by: Alex Bennée <[email protected]>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
Tested-by: Philippe Mathieu-Daudé <[email protected]>
@stsquad
docker: drop QEMU_TARGET check, fallback in EXECUTABLE not set
c655741 
The addition of QEMU_TARGET was intended to ensure we fall back to
checking for the existence of an image if the build system was not
currently configured to build it. However this breaks the direct use
of the rule for building custom binfmt_misc images. We already check
for EXECUTABLE so let us just use that as a proxy for deciding if we
are just going to check the image exits.

Signed-off-by: Alex Bennée <[email protected]>
Tested-by: Philippe Mathieu-Daudé <[email protected]>
@stsquad
docker: report hint when docker.py check fails
8706653 
When a check fails we currently just report why we failed. This is not
totally helpful to people who want to boot-strap a new image. Report a
hint as to why it failed.

Signed-off-by: Alex Bennée <[email protected]>
Suggested-by: Fam Zheng <[email protected]>
@philmd
docker: Update debootstrap script after Debian migration from Alioth …
a4af82a 
…to Salsa

This silents the following warning:

  Cloning into './debootstrap.git'...
  warning: redirecting to https://salsa.debian.org/installer-team/debootstrap.git/

See https://lists.debian.org/debian-devel-announce/2018/01/msg00004.html

Signed-off-by: Philippe Mathieu-Daudé <[email protected]>
Signed-off-by: Alex Bennée <[email protected]>
@stsquad
docker: add commentary to debian-bootstrap.docker
d879235 
This is just a note that later versions of debootstrap don't
technically need this hack.

Signed-off-by: Alex Bennée <[email protected]>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
@stsquad
docker: ignore distro versioning of debootstrap
36048c0 
We do a minimum version check for the debootstrap but if the distro
has added their own minor version tick it would fail and fall-back to
the SCM version. This is sub-optimal as the latest/greatest version
may be broken at any one particular time. We fix that with a little
sed magic on the version string before passing to our ugly shell
versioning check.

Signed-off-by: Alex Bennée <[email protected]>
Tested-by: Philippe Mathieu-Daudé <[email protected]>
@stsquad
docker: perform basic binfmt_misc validation in docker.py
b8c7544 
Setting up binfmt_misc is outside of the scope of the docker.py script
but we can at least validate it with any given executable so we have a
more useful error message than the sed line of deboostrap failing
cryptically.

Signed-off-by: Alex Bennée <[email protected]>
Reported-by: Richard Henderson <[email protected]>
@stsquad
tests/tcg: remove runcom test
3c1aed8 
The combination of being rather esoteric and needing to support mmap @
0 means this only ever worked under translation. It has now regressed
even further and is no longer useful. Kill it.

Signed-off-by: Alex Bennée <[email protected]>
Reviewed-by: Peter Maydell <[email protected]>
@nacc
block/file-posix: add bdrv_attach_aio_context callback for host dev a…
733f491 
…nd cdrom

In ed6e216 ("linux-aio: properly bubble up errors from initialzation"),
I only added a bdrv_attach_aio_context callback for the bdrv_file
driver. There are several other drivers that use the shared
aio_plug callback, though, and they will trip the assertion added to
aio_get_linux_aio because they did not call aio_setup_linux_aio first.
Add the appropriate callback definition to the affected driver
definitions.

Fixes: ed6e216 ("linux-aio: properly bubble up errors from initialization")
Reported-by: Farhan Ali <[email protected]>
Signed-off-by: Nishanth Aravamudan <[email protected]>
Reviewed-by: John Snow <[email protected]>
Message-id: [email protected]
Cc: Eric Blake <[email protected]>
Cc: Kevin Wolf <[email protected]>
Cc: John Snow <[email protected]>
Cc: Max Reitz <[email protected]>
Cc: Stefan Hajnoczi <[email protected]>
Cc: Fam Zheng <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Stefan Hajnoczi <[email protected]>
@berrange
tests: call qcrypto_init instead of gnutls_global_init
46c9f7a 
Calling qcrypto_init ensures that all relevant initialization is
done. In particular this honours the debugging settings and thread
settings.

Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
Tested-by: Philippe Mathieu-Daudé <[email protected]>
Signed-off-by: Daniel P. Berrangé <[email protected]>
@berrange
tests: don't silence error reporting for all tests
07ff3f3 
The test-vmstate test is a bit chatty because it triggers various
expected failure scenarios and the code in question uses error_report
instead of accepting 'Error **errp' parameters. To silence this test the
stubs for error_vprintf() were changed to send errors via
g_test_message() instead of stderr:

  commit 28017e0
  Author: Paolo Bonzini <[email protected]>
  Date:   Mon Oct 24 18:31:03 2016 +0200

    tests: send error_report to test log

    Implement error_vprintf to send the output of error_report to
    the test log.  This silences test-vmstate.

    Signed-off-by: Paolo Bonzini <[email protected]>
    Message-Id: <[email protected]>

Unfortunately this change has global impact across the entire test suite
and means that when tests fail for unexpected reasons, the message is
not displayed on stderr. eg when using &error_abort in a call the test
merely prints

  Unexpected error in qcrypto_tls_session_check_certificate() at crypto/tlssession.c:280:

and the actual error message is hidden, making it impossible to diagnose
the failure. This is especially problematic in CI or build systems where
it isn't possible to easily pass the --debug-log flag to tests and
re-run with the test log visible.

This change makes the previous big hammer much more nuanced, providing a
flag in the stub error_vprintf() that can used on a per-test basis to
silence the errors. Only the test-vmstate silences errors initially.

Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
Tested-by: Philippe Mathieu-Daudé <[email protected]>
Signed-off-by: Daniel P. Berrangé <[email protected]>
@berrange
tests: use error_abort in places expecting errors
a73a878 
Most of the TLS related tests are passing an in a "Error" object to
methods that are expected to fail, but then ignoring any error that is
set and instead asserting on a return value. This means that when an
error is unexpectedly raised, no information about it is printed out,
making failures hard to diagnose. Changing these tests to pass in
&error_abort will make unexpected failures print messages to stderr.

Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
Tested-by: Philippe Mathieu-Daudé <[email protected]>
Signed-off-by: Daniel P. Berrangé <[email protected]>
@berrange
tests: fix TLS handshake failure with TLS 1.3
2c0c3d5 
When gnutls negotiates TLS 1.3 instead of 1.2, the order of messages
sent by the handshake changes. This exposed a logic bug in the test
suite which caused us to wait for the server to see handshake
completion, but not wait for the client to see completion. The result
was the client didn't receive the certificate for verification and the
test failed.

This is exposed in Fedora 29 rawhide which has just enabled TLS 1.3 in
its GNUTLS builds.

Reviewed-by: Eric Blake <[email protected]>
Signed-off-by: Daniel P. Berrangé <[email protected]>
dayeol and others added 9 commits September 17, 2018 12:45
@dayeol
target/riscv/pmp.c: Fix PMP NAPOT decoding bug
3429844 
According to the RISC-V priv. v1.10 ISA document,
pmpaddr register stores (base_addr | (size/2 - 1)) >> 2 for a
NAPOT-encoded address.
However, the current code decodes (base_addr | (size - 1)) >> 3 which
leads to a wrong base address and size.

Signed-off-by: Dayeol Lee <[email protected]>
Reviewed-by: Alistair Francis <[email protected]>
Reviewed-by: Michael Clark <[email protected]>
@dayeol
target/riscv/pmp.c: Fix PMP range boundary address bug
5e346b9 
A wrong address is passed to `pmp_is_in_range` while checking if a
memory access is within a PMP range.
Since the ending address of the pmp range (i.e., pmp_state.addr[i].ea)
is set to the last address in the range (i.e., pmp base + pmp size - 1),
memory accesses containg the last address in the range will always fail.

For example, assume that a PMP range is 4KB from 0x87654000 such that
the last address within the range is 0x87654fff.
1-byte access to 0x87654fff should be considered to be fully inside the
PMP range.
However the access now fails and complains partial inclusion because
pmp_is_in_range(env, i, addr + size) returns 0 whereas
pmp_is_in_range(env, i, addr) returns 1.

Signed-off-by: Dayeol Lee <[email protected]>
Reviewed-by: Alistair Francis <[email protected]>
Reviewed-by: Michael Clark <[email protected]>
riscv: remove define cpu_init()
f816b31 
cpu_init() was removed since 2.12, so drop the define that is now unused.

Signed-off-by: Igor Mammedov <[email protected]>
Reviewed-by: Philippe Mathieu-Daudé <[email protected]>
Reviewed-by: Michael Clark <[email protected]>
hw/riscv: flatten SiFive machine bus topology
32ad714 
An SOC child object was added in a previous commit which
created a static toplogy of the SOC and apparently external
periphery, however, the intention is to make the peripheral
configuration of the 'sifive_e' and 'sifive_u' machines
dynamic so any static structs need to be removed. The first
step towards creating dynamic topology is to remove the
static topology, which in effect, created a wrapper around
riscv_hart_array.

Signed-off-by: Michael Clark <[email protected]>
target/riscv: rename RISCV_FEATURE_MISA
2ddf36f 
Renamed RISCV_FEATURE_MISA_RW to RISCV_FEATURE_MISA

Signed-off-by: Michael Clark <[email protected]>
RISC-V - Disassemble reserved compressed encodings as illegal
f37056e 
- Due to the design of the disassembler, the immediate is not
  known during decoding of the opcode; so to handle compressed
  encodings with reserved immediate values (non-zero), we need
  to add an additional check during decompression to match
  reserved encodings with zero immediates and translate them
  into the illegal instruction.

The following compressed opcodes have reserved encodings with
zero immediates: c.addi4spn, c.addi, c.lui, c.addi16sp, c.srli,
c.srai, c.andi and c.slli

Signed-off-by: Michael Clark <[email protected]>
disas/riscv: Use constant strings instead of #define for formats
9358b91 
Signed-off-by: Michael Clark <[email protected]>
disas/riscv: Fix rdinstreth constraint
5565d26 
The constraint for `rdinstreth` was comparing the csr number to 0xc80,
which is `cycleh` instead. Fix this.

Author: Wladimir J. van der Laan <[email protected]>
Signed-off-by: Michael Clark <[email protected]>
Merge branch 'qemu-for-upstream' into riscv-all
6e53247
@nwf
riscv: virt machine: multiple UARTs
61fbff5 
Attach up to 16 serial devices, densely packed in the memory map and
registered in the FDT.  To ensure that BBL continues to find the default
one, we register it last so that it ends up at the top of the fdt.

Signed-off-by: Nathaniel Wesley Filardo <[email protected]>
@nwf
Copy link
Author

nwf commented Sep 18, 2018

Rebased.

@nwf
Copy link
Author

nwf commented Sep 24, 2018

Ping?

@michaeljclark michaeljclark force-pushed the riscv-all branch 8 times, most recently from 950dc3f to 3ee161b Compare October 2, 2018 00:58
@nwf
Copy link
Author

nwf commented Oct 10, 2018

Can this please be merged? What's the hold up?

@nwf
Copy link
Author

nwf commented Oct 17, 2018

Hey look, it's been a week again. Please?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@michaeljclark michaeljclark michaeljclark left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.