Re: [PATCH 18/44] Add RNG200 RNG and RBG

[Peter Maydell]

On Wed, 26 Jul 2023 at 14:29, Sergey Kambalin <serg.oker@gmail.com> wrote:
>
> Signed-off-by: Sergey Kambalin <sergey.kambalin@auriga.com>
> ---
>  hw/misc/bcm2838_rng200.c | 218 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 217 insertions(+), 1 deletion(-)
>
> diff --git a/hw/misc/bcm2838_rng200.c b/hw/misc/bcm2838_rng200.c
> index a17e8f2cda..bfc40658e2 100644
> --- a/hw/misc/bcm2838_rng200.c
> +++ b/hw/misc/bcm2838_rng200.c
> @@ -8,23 +8,194 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/log.h"
>  #include "qapi/error.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/misc/bcm2838_rng200.h"
>  #include "trace.h"
>
> +#define RNG_CTRL_OFFSET                      0x00
> +#define RNG_SOFT_RESET                       0x01
> +#define RNG_SOFT_RESET_OFFSET                0x04
> +#define RBG_SOFT_RESET_OFFSET                0x08
> +#define RNG_TOTAL_BIT_COUNT_OFFSET           0x0C
> +#define RNG_TOTAL_BIT_COUNT_THRESHOLD_OFFSET 0x10
> +#define RNG_INT_STATUS_OFFSET                0x18
> +#define RNG_INT_ENABLE_OFFSET                0x1C
> +#define RNG_FIFO_DATA_OFFSET                 0x20
> +#define RNG_FIFO_COUNT_OFFSET                0x24
> +
> +#define RNG_WARM_UP_PERIOD_ELAPSED           17
> +
> +#define BCM2838_RNG200_PTIMER_POLICY         
> (PTIMER_POLICY_CONTINUOUS_TRIGGER)
> +
> +static void bcm2838_rng200_update_irq(BCM2838Rng200State *state)
> +{
> +    qemu_set_irq(state->irq, !!(state->rng_int_enable.value
> +                              & state->rng_int_status.value));

If the RNG is disabled, is the interrupt line still asserted?
If not, we need to check the rng-enabled bit here and also
make sure we do an update-irq call when that bit of the control
register changes.

> +}
> +
> +static void bcm2838_rng200_update_fifo(void *opaque, const void *buf,
> +                                       size_t size)
> +{
> +    BCM2838Rng200State *state = (BCM2838Rng200State *)opaque;
> +    Fifo8 *fifo = &state->fifo;
> +    size_t num = MIN(size, fifo8_num_free(fifo));
> +    uint32_t num_bits = num * 8;
> +    uint32_t bit_threshold_left = 0;
> +
> +    state->rng_total_bit_count += num_bits;
> +    if (state->rng_bit_count_threshold > state->rng_total_bit_count) {
> +        bit_threshold_left =
> +            state->rng_bit_count_threshold - state->rng_total_bit_count;
> +    } else {
> +        bit_threshold_left = 0;
> +    }
> +
> +    if (bit_threshold_left < num_bits) {
> +        num_bits -= bit_threshold_left;
> +    } else {
> +        num_bits = 0;
> +    }
> +
> +    num = num_bits / 8;
> +    if ((num == 0) && (num_bits > 0)) {
> +        num = 1;
> +    }
> +    if (num > 0) {
> +        fifo8_push_all(fifo, buf, num);
> +
> +        if (fifo8_num_used(fifo) > state->rng_fifo_count.thld) {
> +            state->rng_int_status.total_bits_count_irq = 1;
> +        }
> +    }
> +
> +    state->rng_fifo_count.count = fifo8_num_used(fifo) >> 2;
> +    bcm2838_rng200_update_irq(state);
> +    trace_bcm2838_rng200_update_fifo(num, fifo8_num_used(fifo));
> +}
> +
> +static void bcm2838_rng200_fill_fifo(BCM2838Rng200State *state)
> +{
> +    rng_backend_request_entropy(state->rng,
> +                                fifo8_num_free(&state->fifo),
> +                                bcm2838_rng200_update_fifo, state);
> +}
> +
> +/* state is temporary unused */
> +static void bcm2838_rng200_disable_rbg(BCM2838Rng200State *state
> +                                       __attribute__((unused)))
> +{
> +    trace_bcm2838_rng200_disable_rbg();
> +}
> +
> +static void bcm2838_rng200_enable_rbg(BCM2838Rng200State *state)
> +{
> +    state->rng_total_bit_count = RNG_WARM_UP_PERIOD_ELAPSED;
> +
> +    bcm2838_rng200_fill_fifo(state);
> +
> +    trace_bcm2838_rng200_enable_rbg();
> +}
> +
>  static void bcm2838_rng200_rng_reset(BCM2838Rng200State *state)
>  {
>      state->rng_ctrl.value = 0;
> +    state->rng_total_bit_count = 0;
> +    state->rng_bit_count_threshold = 0;
> +    state->rng_fifo_count.value = 0;
> +    state->rng_int_status.value = 0;
> +    state->rng_int_status.startup_transition_met_irq = 1;
> +    state->rng_int_enable.value = 0;
> +    fifo8_reset(&state->fifo);
>
>      trace_bcm2838_rng200_rng_soft_reset();
>  }
>
> +static void bcm2838_rng200_rbg_reset(BCM2838Rng200State *state)
> +{
> +    trace_bcm2838_rng200_rbg_soft_reset();
> +}
> +
> +static uint32_t bcm2838_rng200_read_fifo_data(BCM2838Rng200State *state)
> +{
> +    Fifo8 *fifo = &state->fifo;
> +    const uint8_t *buf;
> +    uint32_t ret = 0;
> +    uint32_t num = 0;
> +    uint32_t max = MIN(fifo8_num_used(fifo), sizeof(ret));
> +
> +    if (max > 0) {
> +        buf = fifo8_pop_buf(fifo, max, &num);
> +        if ((buf != NULL) && (num > 0)) {
> +            memcpy(&ret, buf, num);

Copying from a byte buffer into a uint32_t like this isn't
endianness-safe. fifo8_pop_buf() also won't give you all
the data in the fifo if it happens to be wrapping around the
end of the ring buffer. You can rely on it returning
you at least some data, though, so the NULL check is not
needed. So you want something like:

   uint32_t to_read = MIN(fifo8_num_used(fifo), 4);
   uint8_t byte_buf[4] = {};
   uint8_t *p = byte_buf;

   while (to_read) {
      buf = fifo8_pop_buf(fifo, to_read, &num);
      memcpy(p, buf, num);
      p += num;
      to_read -= num;
   }
   ret = ldl_le_p(byte_buf);

(assuming that the hardware spec is that the first bytes
out of the fifo go in the least-significant word of the
result when there isn't enough to fill a full word.)

> +        }
> +    } else {
> +        qemu_log_mask(
> +            LOG_GUEST_ERROR,
> +            "bcm2838_rng200_read_fifo_data: FIFO is empty\n"
> +        );
> +    }
> +
> +    state->rng_fifo_count.count = fifo8_num_used(fifo) >> 2;
> +    bcm2838_rng200_fill_fifo(state);
> +
> +    return ret;
> +}

> @@ -89,7 +305,7 @@ static Property bcm2838_rng200_properties[] = {
>      DEFINE_PROP_UINT32("rng-fifo-cap", BCM2838Rng200State, rng_fifo_cap, 
> 128),
>      DEFINE_PROP_LINK("rng", BCM2838Rng200State, rng,
>                       TYPE_RNG_BACKEND, RngBackend *),
> -    DEFINE_PROP_BOOL("use-timer", BCM2838Rng200State, use_timer, true),
> +    DEFINE_PROP_BOOL("use-timer", BCM2838Rng200State, use_timer, false),

This looks like it ought to be folded into some other patch, assuming
we keep the property at all.

thanks
-- PMM