diff mbox series

aspeed_scu: Implement RNG register

Message ID 20180528124621.22977-1-joel@jms.id.au
State Superseded
Headers show
Series aspeed_scu: Implement RNG register | expand

Commit Message

Joel Stanley May 28, 2018, 12:46 p.m. UTC 
The ASPEED SoCs contain a single register that returns random data when
read. This models that register so that guests can use it.

Signed-off-by: Joel Stanley <joel@jms.id.au>

---
 hw/misc/aspeed_scu.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

-- 
2.17.0

Comments

Cédric Le Goater May 28, 2018, 1:47 p.m. UTC | #1 
Hello Joel, 

On 05/28/2018 02:46 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.

> 

> Signed-off-by: Joel Stanley <joel@jms.id.au>

> ---

>  hw/misc/aspeed_scu.c | 19 +++++++++++++++++++

>  1 file changed, 19 insertions(+)

> 

> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c

> index 5e6d5744eeca..8fa0cecf0fa1 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;

> +}

> +

>  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_DATA:

> +        return aspeed_scu_get_random();


may be we could test bit 1 of RNG_CTRL to check if it is enabled or not.

> +        break;

>      case WAKEUP_EN:

>          qemu_log_mask(LOG_GUEST_ERROR,

>                        "%s: Read of write-only offset 0x%" HWADDR_PRIx "\n",

> @@ -287,6 +303,9 @@ static void aspeed_scu_realize(DeviceState *dev, Error **errp)

>                            TYPE_ASPEED_SCU, SCU_IO_REGION_SIZE);

>  

>      sysbus_init_mmio(sbd, &s->iomem);

> +

> +    if (qcrypto_random_init(errp))

> +        return;

>  }


Isn't this routine called from main() already ? 

C. 

>  

>  static const VMStateDescription vmstate_aspeed_scu = {

>
Joel Stanley May 28, 2018, 2:03 p.m. UTC | #2 
On 28 May 2018 at 23:17, Cédric Le Goater <clg@kaod.org> wrote:
> Hello Joel,

>

> On 05/28/2018 02:46 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.

>>

>> Signed-off-by: Joel Stanley <joel@jms.id.au>

>> ---

>>  hw/misc/aspeed_scu.c | 19 +++++++++++++++++++

>>  1 file changed, 19 insertions(+)

>>

>> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c

>> index 5e6d5744eeca..8fa0cecf0fa1 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;

>> +}

>> +

>>  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_DATA:

>> +        return aspeed_scu_get_random();

>

> 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'll send a v2 with that check.

>

>> +        break;

>>      case WAKEUP_EN:

>>          qemu_log_mask(LOG_GUEST_ERROR,

>>                        "%s: Read of write-only offset 0x%" HWADDR_PRIx "\n",

>> @@ -287,6 +303,9 @@ static void aspeed_scu_realize(DeviceState *dev, Error **errp)

>>                            TYPE_ASPEED_SCU, SCU_IO_REGION_SIZE);

>>

>>      sysbus_init_mmio(sbd, &s->iomem);

>> +

>> +    if (qcrypto_random_init(errp))

>> +        return;

>>  }

>

> Isn't this routine called from main() already ?


It is indirectly called, yes. I'll remove this.

Cheers,

Joel
Joel Stanley May 28, 2018, 2:29 p.m. UTC | #3 
On 28 May 2018 at 23:33, Joel Stanley <joel@jms.id.au> wrote:
> On 28 May 2018 at 23:17, Cédric Le Goater <clg@kaod.org> wrote:

>> Hello Joel,

>>

>> On 05/28/2018 02:46 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.

>>>

>>> Signed-off-by: Joel Stanley <joel@jms.id.au>

>>> ---

>>>  hw/misc/aspeed_scu.c | 19 +++++++++++++++++++

>>>  1 file changed, 19 insertions(+)

>>>

>>> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c

>>> index 5e6d5744eeca..8fa0cecf0fa1 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;

>>> +}

>>> +

>>>  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_DATA:

>>> +        return aspeed_scu_get_random();

>>

>> 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.

How should we model that?
Cédric Le Goater May 28, 2018, 2:57 p.m. UTC | #4 
On 05/28/2018 04:29 PM, Joel Stanley wrote:
> On 28 May 2018 at 23:33, Joel Stanley <joel@jms.id.au> wrote:

>> On 28 May 2018 at 23:17, Cédric Le Goater <clg@kaod.org> wrote:

>>> Hello Joel,

>>>

>>> On 05/28/2018 02:46 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.

>>>>

>>>> Signed-off-by: Joel Stanley <joel@jms.id.au>

>>>> ---

>>>>  hw/misc/aspeed_scu.c | 19 +++++++++++++++++++

>>>>  1 file changed, 19 insertions(+)

>>>>

>>>> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c

>>>> index 5e6d5744eeca..8fa0cecf0fa1 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;

>>>> +}

>>>> +

>>>>  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_DATA:

>>>> +        return aspeed_scu_get_random();

>>>

>>> 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.

> 

> How should we model that?

> 


I confirm that the HW doesn't really care about the enabled bit.
Let's ignore it then ?

C.
diff mbox series

Patch

diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index 5e6d5744eeca..8fa0cecf0fa1 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;
+}
+
 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_DATA:
+        return aspeed_scu_get_random();
+        break;
     case WAKEUP_EN:
         qemu_log_mask(LOG_GUEST_ERROR,
                       "%s: Read of write-only offset 0x%" HWADDR_PRIx "\n",
@@ -287,6 +303,9 @@  static void aspeed_scu_realize(DeviceState *dev, Error **errp)
                           TYPE_ASPEED_SCU, SCU_IO_REGION_SIZE);
 
     sysbus_init_mmio(sbd, &s->iomem);
+
+    if (qcrypto_random_init(errp))
+        return;
 }
 
 static const VMStateDescription vmstate_aspeed_scu = {