Re: [Qemu-devel] [PATCH v3] hw/misc: Add Exynos4210 Pseudo Random Number Generator

[Peter Maydell]

On 25 April 2017 at 19:06, Krzysztof Kozlowski <address@hidden> wrote:
> Add emulation for Exynos4210 Pseudo Random Number Generator which could
> work on fixed seeds or with seeds provided by True Random Number
> Generator block inside the SoC.
>
> Implement only the fixed seeds part of it in polling mode (no
> interrupts).
>
> Emulation tested with two independent Linux kernel exynos-rng drivers:
> 1. New kcapi-rng interface (targeting Linux v4.12),
> 2. Old hwrng inteface
>    # echo "exynos" > /sys/class/misc/hw_random/rng_current
>    # dd if=/dev/hwrng of=/dev/null bs=1 count=16
>
> Signed-off-by: Krzysztof Kozlowski <address@hidden>
>
> ---
>
> Changes since v2:
> 1. Drop GRand in favor of qcrypto_random_bytes() after discussion with
>    Peter Maydell.
> 2. Because of above, ignore the seed value but mark if each one of five
>    seed registers were seeded.
> 3. Fix memset of s->randr_value (copy paste error, also shorter sizeof
>    can be used).
>
> Changes since v1:
> 1. Use GRand-like functions to fix build on MingW32 (this adds also
>    finalize).
> 2. Add DPRINTF macro.
> 3. Use HWADDR_PRIx and family for printing values.
> ---

> +#define EXYNOS4210_RNG_STATUS_WRITE_MASK        
> (EXYNOS4210_RNG_STATUS_PRNG_DONE \
> +                                                    | 
> EXYNOS4210_RNG_STATUS_MSG_DONE \
> +                                                    | 
> EXYNOS4210_RNG_STATUS_PARTIAL_DONE)
> +
> +#define EXYNOS4210_RNG_CONTROL_1                  0x0
> +#define EXYNOS4210_RNG_STATUS                    0x10
> +#define EXYNOS4210_RNG_SEED_IN                  0x140
> +#define EXYNOS4210_RNG_SEED_IN_OFFSET(n)        (EXYNOS4210_RNG_SEED_IN + (n 
> * 0x4))
> +#define EXYNOS4210_RNG_PRNG                     0x160
> +#define EXYNOS4210_RNG_PRNG_OFFSET(n)           (EXYNOS4210_RNG_PRNG + (n * 
> 0x4))

Some of these lines are slightly overlong.

> +
> +#define EXYNOS4210_RNG_PRNG_NUM                 5
> +
> +#define EXYNOS4210_RNG_REGS_MEM_SIZE            0x200
> +
> +typedef struct Exynos4210RngState {
> +    SysBusDevice parent_obj;
> +    MemoryRegion iomem;
> +
> +    int32_t randr_value[EXYNOS4210_RNG_PRNG_NUM];
> +    /* bits from 0 to EXYNOS4210_RNG_PRNG_NUM if given seed register was set 
> */
> +    uint32_t seed_set;
> +
> +    /* Register values */
> +    uint32_t reg_control;
> +    uint32_t reg_status;
> +} Exynos4210RngState;
> +
> +static bool exynos4210_rng_seed_ready(const Exynos4210RngState *s)
> +{
> +    unsigned int i;
> +
> +    for (i = 0; i < EXYNOS4210_RNG_PRNG_NUM; i++) {
> +        if ((s->seed_set & BIT(i)) == 0) {
> +            return false;
> +        }
> +    }
> +
> +    return true;

This can be more efficiently written as:

    uint32_t mask = MAKE_64BIT_MASK(0, EXYNOS4210_RNG_PRNG_NUM);

    /* Return true if all the seed-set bits are set. */
    return (s->seed_set & mask) == mask;


Unless you object, I propose to make those minor tweaks
locally and commit to target-arm.next, rather than making
you spin another version of this.

thanks
-- PMM