qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 19/44] Add RNG200 timer


From: Peter Maydell
Subject: Re: [PATCH 19/44] Add RNG200 timer
Date: Fri, 4 Aug 2023 15:31:44 +0100

On Wed, 26 Jul 2023 at 15:18, Sergey Kambalin <serg.oker@gmail.com> wrote:
>
> Signed-off-by: Sergey Kambalin <sergey.kambalin@auriga.com>
> ---
>  hw/arm/bcm2838_peripherals.c         |  14 ++++
>  hw/arm/raspi4b.c                     |   1 -
>  hw/misc/bcm2838_rng200.c             | 105 ++++++++++++++++++++++++---
>  include/hw/arm/bcm2838_peripherals.h |   2 +
>  4 files changed, 111 insertions(+), 11 deletions(-)
>
> diff --git a/hw/arm/bcm2838_peripherals.c b/hw/arm/bcm2838_peripherals.c
> index 41b321b254..df97953bbe 100644
> --- a/hw/arm/bcm2838_peripherals.c
> +++ b/hw/arm/bcm2838_peripherals.c
> @@ -34,6 +34,9 @@ static void bcm2838_peripherals_init(Object *obj)
>                         bc->peri_low_size);
>      sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->peri_low_mr);
>
> +    /* Random Number Generator */
> +    object_initialize_child(obj, "rng200", &s->rng200, TYPE_BCM2838_RNG200);
> +
>      /* PCIe Host Bridge */
>      object_initialize_child(obj, "pcie-host", &s->pcie_host,
>                              TYPE_BCM2838_PCIE_HOST);
> @@ -84,6 +87,17 @@ static void bcm2838_peripherals_realize(DeviceState *dev, 
> Error **errp)
>                                          BCM2838_VC_PERI_LOW_BASE,
>                                          &s->peri_low_mr_alias, 1);
>
> +    /* Random Number Generator */
> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->rng200), errp)) {
> +        return;
> +    }
> +    memory_region_add_subregion(
> +        &s_base->peri_mr, RNG_OFFSET,
> +        sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->rng200), 0));
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->rng200), 0,
> +        qdev_get_gpio_in_named(DEVICE(&s_base->ic), BCM2835_IC_GPU_IRQ,
> +                               INTERRUPT_RNG));
> +
>      /* Extended Mass Media Controller 2 */
>      object_property_set_uint(OBJECT(&s->emmc2), "sd-spec-version", 3,
>                               &error_abort);
> diff --git a/hw/arm/raspi4b.c b/hw/arm/raspi4b.c
> index 115359e451..82b6b48d3c 100644
> --- a/hw/arm/raspi4b.c
> +++ b/hw/arm/raspi4b.c
> @@ -66,7 +66,6 @@ static void raspi4_modify_dtb(const struct arm_boot_info 
> *info, void *fdt)
>
>      /* Temporary disable following devices until they are implemented*/
>      const char *to_be_removed_from_dt_as_wa[] = {
> -        "brcm,bcm2711-rng200",
>          "brcm,bcm2711-thermal",
>          "brcm,bcm2711-genet-v5",
>      };

These "add the rng device to the SoC" changes should be in their
own patch.

> diff --git a/hw/misc/bcm2838_rng200.c b/hw/misc/bcm2838_rng200.c
> index bfc40658e2..face1e9579 100644
> --- a/hw/misc/bcm2838_rng200.c
> +++ b/hw/misc/bcm2838_rng200.c
> @@ -9,6 +9,7 @@
>
>  #include "qemu/osdep.h"
>  #include "qemu/log.h"
> +#include "qom/object_interfaces.h"
>  #include "qapi/error.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/misc/bcm2838_rng200.h"
> @@ -35,6 +36,15 @@ static void bcm2838_rng200_update_irq(BCM2838Rng200State 
> *state)
>                                & state->rng_int_status.value));
>  }
>
> +static void bcm2838_rng200_update_rbg_period(void *opaque, ClockEvent event)
> +{
> +    BCM2838Rng200State *s = (BCM2838Rng200State *)opaque;
> +
> +    ptimer_transaction_begin(s->ptimer);
> +    ptimer_set_period_from_clock(s->ptimer, s->clock, s->rng_fifo_cap * 8);
> +    ptimer_transaction_commit(s->ptimer);
> +}

What is this timer being used for, and why do we need it?
(This is a good example of the kind of thing to put into a
commit message.) It looks like it's trying to simulate the
way that real hardware only slowly fills up the RNG fifo.
But do we really need the extra complication? Does guest
software fall over if it finds that the FIFO is
full very quickly rather than taking a longer time?

> +
>  static void bcm2838_rng200_update_fifo(void *opaque, const void *buf,
>                                         size_t size)
>  {
> @@ -62,10 +72,11 @@ static void bcm2838_rng200_update_fifo(void *opaque, 
> const void *buf,
>      if ((num == 0) && (num_bits > 0)) {
>          num = 1;
>      }
> -    if (num > 0) {
> +    if (!state->use_timer || (num > 0)) {
>          fifo8_push_all(fifo, buf, num);
>
> -        if (fifo8_num_used(fifo) > state->rng_fifo_count.thld) {
> +        if (!state->use_timer
> +                || (fifo8_num_used(fifo) > state->rng_fifo_count.thld)) {
>              state->rng_int_status.total_bits_count_irq = 1;
>          }
>      }
> @@ -82,10 +93,14 @@ static void bcm2838_rng200_fill_fifo(BCM2838Rng200State 
> *state)
>                                  bcm2838_rng200_update_fifo, state);
>  }
>
> -/* state is temporary unused */
> -static void bcm2838_rng200_disable_rbg(BCM2838Rng200State *state
> -                                       __attribute__((unused)))
> +static void bcm2838_rng200_disable_rbg(BCM2838Rng200State *state)
>  {
> +    if (state->use_timer) {
> +        ptimer_transaction_begin(state->ptimer);
> +        ptimer_stop(state->ptimer);
> +        ptimer_transaction_commit(state->ptimer);
> +    }
> +
>      trace_bcm2838_rng200_disable_rbg();
>  }
>
> @@ -93,11 +108,38 @@ static void bcm2838_rng200_enable_rbg(BCM2838Rng200State 
> *state)
>  {
>      state->rng_total_bit_count = RNG_WARM_UP_PERIOD_ELAPSED;
>
> -    bcm2838_rng200_fill_fifo(state);
> +    if (state->use_timer) {
> +        uint32_t div = state->rng_ctrl.div + 1;
> +
> +        ptimer_transaction_begin(state->ptimer);
> +        ptimer_set_limit(state->ptimer, div, 1);
> +        ptimer_set_count(state->ptimer, div);
> +        ptimer_run(state->ptimer, 0);
> +        ptimer_transaction_commit(state->ptimer);
> +    } else {
> +        bcm2838_rng200_fill_fifo(state);
> +    }
>
>      trace_bcm2838_rng200_enable_rbg();
>  }
>
> +static void bcm2838_rng200_ptimer_cb(void *arg)
> +{
> +    BCM2838Rng200State *state = (BCM2838Rng200State *)arg;
> +    Fifo8 *fifo = &state->fifo;
> +    size_t size = fifo8_num_free(fifo);
> +
> +    assert(state->rng_ctrl.rbg_enable);
> +
> +    if (size > 0) {
> +        rng_backend_request_entropy(state->rng, size,
> +                                    bcm2838_rng200_update_fifo, state);
> +    } else {
> +        ptimer_stop(state->ptimer);
> +        trace_bcm2838_rng200_fifo_full();
> +    }
> +}
> +
>  static void bcm2838_rng200_rng_reset(BCM2838Rng200State *state)
>  {
>      state->rng_ctrl.value = 0;
> @@ -129,6 +171,12 @@ static uint32_t 
> bcm2838_rng200_read_fifo_data(BCM2838Rng200State *state)
>          buf = fifo8_pop_buf(fifo, max, &num);
>          if ((buf != NULL) && (num > 0)) {
>              memcpy(&ret, buf, num);
> +
> +            if (state->rng_ctrl.rbg_enable && state->use_timer) {
> +                ptimer_transaction_begin(state->ptimer);
> +                ptimer_run(state->ptimer, 0);
> +                ptimer_transaction_commit(state->ptimer);
> +            }
>          }
>      } else {
>          qemu_log_mask(
> @@ -138,7 +186,10 @@ static uint32_t 
> bcm2838_rng200_read_fifo_data(BCM2838Rng200State *state)
>      }
>
>      state->rng_fifo_count.count = fifo8_num_used(fifo) >> 2;
> -    bcm2838_rng200_fill_fifo(state);
> +
> +    if (!state->use_timer) {
> +        bcm2838_rng200_fill_fifo(state);
> +    }
>
>      return ret;
>  }
> @@ -237,7 +288,11 @@ static void bcm2838_rng200_write(void *opaque, hwaddr 
> offset,
>          break;
>      case RNG_TOTAL_BIT_COUNT_THRESHOLD_OFFSET:
>          s->rng_bit_count_threshold = value;
> -        s->rng_total_bit_count = value + 1;
> +        if (s->use_timer) {
> +            s->rng_total_bit_count = 0;
> +        } else {
> +            s->rng_total_bit_count = value + 1;
> +        }
>          break;
>      default:
>          qemu_log_mask(
> @@ -253,15 +308,39 @@ static const MemoryRegionOps bcm2838_rng200_ops = {
>      .read = bcm2838_rng200_read,
>      .write = bcm2838_rng200_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
> +    .impl = {
> +        .max_access_size = 4,
> +        .min_access_size = 4,
> +    },
> +    .valid = {
> +        .max_access_size = 4,
> +        .min_access_size = 4
> +    },

This should probably have been in a previous patch.

>  };
>
>  static void bcm2838_rng200_realize(DeviceState *dev, Error **errp)
>  {
>      BCM2838Rng200State *s = BCM2838_RNG200(dev);
>
> +    if (s->use_timer) {
> +        s->ptimer = ptimer_init(bcm2838_rng200_ptimer_cb, s,
> +                                BCM2838_RNG200_PTIMER_POLICY);
> +        if (s->ptimer == NULL) {
> +            error_setg(&error_fatal, "Failed to init RBG timer");
> +            return;
> +        }
> +    }
> +
>      if (s->rng == NULL) {
>          Object *default_backend = object_new(TYPE_RNG_BUILTIN);
>
> +        if (!user_creatable_complete(USER_CREATABLE(default_backend),
> +                                     errp)) {
> +            object_unref(default_backend);
> +            error_setg(errp, "Failed to create user creatable RNG backend");
> +            return;
> +        }
> +
>          object_property_add_child(OBJECT(dev), "default-backend",
>                                    default_backend);
>          object_unref(default_backend);
> @@ -270,6 +349,12 @@ static void bcm2838_rng200_realize(DeviceState *dev, 
> Error **errp)
>                                   errp);
>      }
>
> +    if (s->use_timer && !clock_has_source(s->clock)) {
> +        ptimer_transaction_begin(s->ptimer);
> +        ptimer_set_period(s->ptimer, s->rbg_period * s->rng_fifo_cap * 8);
> +        ptimer_transaction_commit(s->ptimer);
> +    }
> +
>      fifo8_create(&s->fifo, s->rng_fifo_cap);
>      sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
>  }
> @@ -280,7 +365,7 @@ static void bcm2838_rng200_init(Object *obj)
>      SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>
>      s->clock = qdev_init_clock_in(DEVICE(s), "rbg-clock",
> -                                  NULL, s,
> +                                  bcm2838_rng200_update_rbg_period, s,
>                                    ClockPreUpdate);
>      if (s->clock == NULL) {
>          error_setg(&error_fatal, "Failed to init RBG clock");
> @@ -305,7 +390,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, false),
> +    DEFINE_PROP_BOOL("use-timer", BCM2838Rng200State, use_timer, true),

...property has flipped back to true again.

>      DEFINE_PROP_END_OF_LIST(),
>  };


thanks
-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]