Re: [Qemu-devel] [PATCH v2 2/2] ppc/spapr_hcall: Implement H_RANDOM hypercall in QEMU

[Thomas Huth <thuth@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4548 bytes --]

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 <thuth@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




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]