Hi Joel, On 05/28/2018 12:22 PM, Joel Stanley wrote: > The ASPEED SoCs contain a single register that returns random data when > read. This models that register so that guests can use it. > > The random number data register has a corresponding control register, > data returns a different number regardless of the state of the enabled > bit, so the model follows this behaviour.
I have been bugging Cédric to check specs for the RNG_CTRL register and
he sent me the v1 of this patch than I missed:
>>> may be we could test bit 1 of RNG_CTRL to check if it is enabled
or not.
>>
>> The RNG is enabled by default, and I didn't find any software that
>> disables it, but it can't hurt to have that check.
>
> I did some testing on hardware, and the rng still outputs a different
> number each time I ask for one regardless of the state of the enabled
> bit.
>
I confirm that the HW doesn't really care about the enabled bit.
Let's ignore it then ?
Now your comment makes more sens, however I think it would be more
useful to add it in aspeed_scu_read() to make it obvious.
>
> Signed-off-by: Joel Stanley <j...@jms.id.au>
> ---
> v2:
> - Remove call to qcrypto_random_init as this is done in main()
> ---
> hw/misc/aspeed_scu.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index 5e6d5744eeca..29e58527793b 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -16,6 +16,7 @@
> #include "qapi/visitor.h"
> #include "qemu/bitops.h"
> #include "qemu/log.h"
> +#include "crypto/random.h"
> #include "trace.h"
>
> #define TO_REG(offset) ((offset) >> 2)
> @@ -154,6 +155,18 @@ static const uint32_t
> ast2500_a1_resets[ASPEED_SCU_NR_REGS] = {
> [BMC_DEV_ID] = 0x00002402U
> };
>
> +static uint32_t aspeed_scu_get_random(void)
> +{
> + Error *err = NULL;
> + uint32_t num;
> +
> + if (qcrypto_random_bytes((uint8_t *)&num, sizeof(num), &err)) {
> + error_report_err(err);
> + }
> +
> + return num;
> +}
This function duplicates bcm2835_rng::get_random_bytes() except it
doesn't exit(), is that on purpose?
What about refactoring, inlining bcm2835_rng::get_random_bytes() in a
new include/hw/misc/rng.h?
(This can be later patch)
> +
> static uint64_t aspeed_scu_read(void *opaque, hwaddr offset, unsigned size)
> {
> AspeedSCUState *s = ASPEED_SCU(opaque);
> @@ -167,6 +180,9 @@ static uint64_t aspeed_scu_read(void *opaque, hwaddr
> offset, unsigned size)
> }
>
> switch (reg) {
case RNG_CTRL:
/* The RNG_DATA returns a different number regardless of
* the state of the enabled bit in RNG_CTRL,
* so the model follows this behaviour.
*/
break;
With a comment:
Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>
> + case RNG_DATA:
> + s->regs[RNG_DATA] = aspeed_scu_get_random();
> + break;
> case WAKEUP_EN:
> qemu_log_mask(LOG_GUEST_ERROR,
> "%s: Read of write-only offset 0x%" HWADDR_PRIx "\n",
>
signature.asc
Description: OpenPGP digital signature
