From: Peter Maydell <peter.maydell@linaro.org>
To: Alistair Francis <alistair.francis@opensource.wdc.com>
Cc: qemu-devel@nongnu.org, alistair23@gmail.com,
"Wilfred Mallawa" <wilfred.mallawa@wdc.com>,
"Alistair Francis" <alistair.francis@wdc.com>,
"Philippe Mathieu-Daudé" <f4bug@amsat.org>
Subject: Re: [PULL v2 01/31] hw/ssi: Add Ibex SPI device model
Date: Thu, 12 May 2022 17:37:43 +0100 [thread overview]
Message-ID: <CAFEAcA9i97-N8dwscyZ+dVotitSTptg_5-xoBTZ2JuRn3p5nig@mail.gmail.com> (raw)
In-Reply-To: <20220422003656.1648121-2-alistair.francis@opensource.wdc.com>
On Fri, 22 Apr 2022 at 01:40, Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Wilfred Mallawa <wilfred.mallawa@wdc.com>
>
> Adds the SPI_HOST device model for ibex. The device specification is as per
> [1]. The model has been tested on opentitan with spi_host unit tests
> written for TockOS.
>
> [1] https://docs.opentitan.org/hw/ip/spi_host/doc/
Hi; Coverity points out a bug in this code (CID 1488107):
> +REG32(STATUS, 0x14)
> + FIELD(STATUS, TXQD, 0, 8)
> + FIELD(STATUS, RXQD, 18, 8)
RXQD isn't at the bottom of this register, so the R_STATUS_RXQD_MASK
define is a shifted-up mask...
> +static void ibex_spi_host_transfer(IbexSPIHostState *s)
> +{
> + uint32_t rx, tx;
> + /* Get num of one byte transfers */
> + uint8_t segment_len = ((s->regs[IBEX_SPI_HOST_COMMAND] & R_COMMAND_LEN_MASK)
> + >> R_COMMAND_LEN_SHIFT);
> + while (segment_len > 0) {
> + if (fifo8_is_empty(&s->tx_fifo)) {
> + /* Assert Stall */
> + s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_TXSTALL_MASK;
> + break;
> + } else if (fifo8_is_full(&s->rx_fifo)) {
> + /* Assert Stall */
> + s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXSTALL_MASK;
> + break;
> + } else {
> + tx = fifo8_pop(&s->tx_fifo);
> + }
> +
> + rx = ssi_transfer(s->ssi, tx);
> +
> + trace_ibex_spi_host_transfer(tx, rx);
> +
> + if (!fifo8_is_full(&s->rx_fifo)) {
> + fifo8_push(&s->rx_fifo, rx);
> + } else {
> + /* Assert RXFULL */
> + s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_RXFULL_MASK;
> + }
> + --segment_len;
> + }
> +
> + /* Assert Ready */
> + s->regs[IBEX_SPI_HOST_STATUS] |= R_STATUS_READY_MASK;
> + /* Set RXQD */
> + s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_RXQD_MASK;
> + s->regs[IBEX_SPI_HOST_STATUS] |= (R_STATUS_RXQD_MASK
> + & div4_round_up(segment_len));
...but here we don't shift div4_round_up(segment_len) up to the
right place before ORing it with the MASK, so Coverity points
out that the result is always zero.
> + /* Set TXQD */
> + s->regs[IBEX_SPI_HOST_STATUS] &= ~R_STATUS_TXQD_MASK;
> + s->regs[IBEX_SPI_HOST_STATUS] |= (fifo8_num_used(&s->tx_fifo) / 4)
> + & R_STATUS_TXQD_MASK;
This has the same issue, but avoids it by luck because TXQD
does start at bit 0.
Since we're using the FIELD() macros, it would be clearer to
write all this to use FIELD_DP32() rather than manual
bit operations to clear the bits and then OR in the new ones.
(True here and also in what looks like several other places
through out the file, for deposit and extract operations.)
thanks
-- PMM
next prev parent reply other threads:[~2022-05-12 16:42 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-22 0:36 [PULL v2 00/31] riscv-to-apply queue Alistair Francis
2022-04-22 0:36 ` [PULL v2 01/31] hw/ssi: Add Ibex SPI device model Alistair Francis
2022-05-12 16:37 ` Peter Maydell [this message]
2022-07-20 5:33 ` Alistair Francis
2022-04-22 0:36 ` [PULL v2 02/31] riscv: opentitan: Connect opentitan SPI Host Alistair Francis
2022-04-22 0:36 ` [PULL v2 03/31] target/riscv: Define simpler privileged spec version numbering Alistair Francis
2022-04-22 0:36 ` [PULL v2 04/31] target/riscv: Add the privileged spec version 1.12.0 Alistair Francis
2022-04-22 0:36 ` [PULL v2 05/31] target/riscv: Introduce privilege version field in the CSR ops Alistair Francis
2022-04-22 0:36 ` [PULL v2 06/31] target/riscv: Add support for mconfigptr Alistair Francis
2022-04-22 0:36 ` [PULL v2 07/31] target/riscv: Add *envcfg* CSRs support Alistair Francis
2022-04-22 0:36 ` [PULL v2 08/31] target/riscv: Enable privileged spec version 1.12 Alistair Francis
2022-04-22 0:36 ` [PULL v2 09/31] target/riscv: cpu: Fixup indentation Alistair Francis
2022-04-22 0:36 ` [PULL v2 10/31] target/riscv: Allow software access to MIP SEIP Alistair Francis
2022-04-22 0:36 ` [PULL v2 11/31] target/riscv: Add initial support for the Sdtrig extension Alistair Francis
2022-04-22 0:36 ` [PULL v2 12/31] target/riscv: optimize condition assign for scale < 0 Alistair Francis
2022-04-22 0:36 ` [PULL v2 13/31] target/riscv: optimize helper for vmv<nr>r.v Alistair Francis
2022-04-22 0:36 ` [PULL v2 14/31] target/riscv: misa to ISA string conversion fix Alistair Francis
2022-04-22 0:36 ` [PULL v2 15/31] target/riscv: Add isa extenstion strings to the device tree Alistair Francis
2022-04-22 0:36 ` [PULL v2 16/31] target/riscv: fix start byte for vmv<nf>r.v when vstart != 0 Alistair Francis
2022-04-22 0:36 ` [PULL v2 17/31] target/riscv: Use cpu_loop_exit_restore directly from mmu faults Alistair Francis
2022-04-22 0:36 ` [PULL v2 18/31] hw/riscv: virt: Exit if the user provided -bios in combination with KVM Alistair Francis
2022-04-22 0:36 ` [PULL v2 19/31] target/riscv/pmp: fix NAPOT range computation overflow Alistair Francis
2022-04-22 0:36 ` [PULL v2 20/31] hw/riscv: virt: fix DT property mmu-type when CPU mmu option is disabled Alistair Francis
2022-04-22 0:36 ` [PULL v2 21/31] hw/intc: Add .impl.[min|max]_access_size declaration in RISC-V ACLINT Alistair Francis
2022-04-22 0:36 ` [PULL v2 22/31] hw/intc: Support 32/64-bit mtimecmp and mtime accesses " Alistair Francis
2022-04-22 0:36 ` [PULL v2 23/31] hw/intc: Make RISC-V ACLINT mtime MMIO register writable Alistair Francis
2022-04-22 0:36 ` [PULL v2 24/31] hw/intc: riscv_aclint: Add reset function of ACLINT devices Alistair Francis
2022-04-22 0:36 ` [PULL v2 25/31] target/riscv: debug: Implement debug related TCGCPUOps Alistair Francis
2022-04-22 0:36 ` [PULL v2 26/31] target/riscv: cpu: Add a config option for native debug Alistair Francis
2022-04-22 0:36 ` [PULL v2 27/31] target/riscv: csr: Hook debug CSR read/write Alistair Francis
2022-04-22 0:36 ` [PULL v2 28/31] target/riscv: machine: Add debug state description Alistair Francis
2022-04-22 0:36 ` [PULL v2 29/31] target/riscv: cpu: Enable native debug feature Alistair Francis
2022-04-22 0:36 ` [PULL v2 30/31] hw/core: tcg-cpu-ops.h: Update comments of debug_check_watchpoint() Alistair Francis
2022-04-22 0:36 ` [PULL v2 31/31] hw/riscv: boot: Support 64bit fdt address Alistair Francis
2022-04-22 10:54 ` [PULL v2 00/31] riscv-to-apply queue Richard Henderson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFEAcA9i97-N8dwscyZ+dVotitSTptg_5-xoBTZ2JuRn3p5nig@mail.gmail.com \
--to=peter.maydell@linaro.org \
--cc=alistair.francis@opensource.wdc.com \
--cc=alistair.francis@wdc.com \
--cc=alistair23@gmail.com \
--cc=f4bug@amsat.org \
--cc=qemu-devel@nongnu.org \
--cc=wilfred.mallawa@wdc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.