| Message ID | 20180528124621.22977-1-joel@jms.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | aspeed_scu: Implement RNG register | expand |
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 = { >
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
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?
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 --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 = {
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(+)