On 01/09/15 02:47, David Gibson wrote: > On Mon, Aug 31, 2015 at 08:46:02PM +0200, Thomas Huth wrote: >> The PAPR interface provides a hypercall to pass high-quality >> hardware generated random numbers to guests. So let's provide >> this call in QEMU, too, so that guests that do not support >> virtio-rnd yet can get good random numbers, too. >> Please note that this hypercall should provide "good" random data >> instead of pseudo-random, so the function uses the RngBackend to >> retrieve the values instead of using a "simple" library function >> like rand() or g_random_int(). Since there are multiple RngBackends >> available, the user must select an appropriate backend via the >> "h-random" property of the the machine state to enable it, e.g. >> >> qemu-system-ppc64 -M pseries,h-random=rng-random ... > > I think it would be a better idea to require that the backend RNG be > constructed with -object, then give its id to the pseries code. That > matches what's needed for virtio-rng more closely, and also makes it > simpler to supply parameters to the RNG backend, if necessary.
Ok, sounds like a good idea, will rework my patch accordingly.
> There's also a wart in that whatever the user specifies by way of
> backend, it will be silently overridden if KVM does implement the
> H_RANDOM call. I'm not sure how best to handle that.
We could either print a warning message if we detect that the in-kernel
implementation is available and the user still tries to use the QEMU
implementation - or we simply do not enable the in-kernel hypercall via
kvmppc_enable_hcall() if the user tried to use the QEMU implementation.
What would you prefer?
>> to use the /dev/random backend, or "h-random=rng-egd" to use the
>> Entropy Gathering Daemon instead.
>>
>> Signed-off-by: Thomas Huth <th...@redhat.com>
>> ---
>> hw/ppc/spapr.c | 36 +++++++++++++++++++++---
>> hw/ppc/spapr_hcall.c | 75
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>> include/hw/ppc/spapr.h | 2 ++
>> 3 files changed, 109 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index bc3a112..3db87b5 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
...
>> @@ -1472,7 +1473,7 @@ static void ppc_spapr_init(MachineState *machine)
>> uint32_t initrd_base = 0;
>> long kernel_size = 0, initrd_size = 0;
>> long load_limit, fw_size;
>> - bool kernel_le = false;
>> + bool kernel_le = false, enable_h_random;
>
> I'd prefer to have the new variable on a new line - this way makes it
> very easy to miss the initializer on the old one.
Ok.
...
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 652ddf6..ff9d4fd 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1,4 +1,8 @@
>> #include "sysemu/sysemu.h"
>> +#include "sysemu/rng.h"
>> +#include "sysemu/rng-random.h"
>> +#include "qom/object_interfaces.h"
>> +#include "qemu/error-report.h"
>> #include "cpu.h"
>> #include "helper_regs.h"
>> #include "hw/ppc/spapr.h"
>> @@ -929,6 +933,77 @@ static target_ulong
>> h_client_architecture_support(PowerPCCPU *cpu_,
>> return H_SUCCESS;
>> }
>>
>> +typedef struct HRandomData {
>> + QemuSemaphore sem;
>> + union {
>> + uint64_t v64;
>> + uint8_t v8[8];
>> + } val;
>> + int received;
>> +} HRandomData;
>> +
>> +static RndRandom *hrandom_rng;
>
> Couldn't you avoid the new global by looking this up through the
> sPAPRMachineState?
Sure.
>> +static void random_recv(void *dest, const void *src, size_t size)
>> +{
>> + HRandomData *hrcrdp = dest;
>> +
>> + if (src && size > 0) {
>> + memcpy(&hrcrdp->val.v8[hrcrdp->received], src, size);
>
> I'd be happier with an assert() ensuring that size doesn't exceed the
> buffer space we have left.
That's a different issue - the if-statement above seems to be necassary
because the callback sometimes seems to be called with size = 0 or src =
NULL, so I had to add this here to avoid crashes.
But I can also add an assert(hrcrdp->received + size <= 8) here if you
like, just to be sure.
>> + hrcrdp->received += size;
>> + }
>> + qemu_sem_post(&hrcrdp->sem);
>
> Could you avoid a few wakeups by only posting the semaphore once the
> buffer is filled?
Then the callback functions has to trigger the next
rng_backend_request_entropy() ... not sure yet whether I like that
idea... but I can have a try.
Thomas
signature.asc
Description: OpenPGP digital signature
