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

[David Gibson]

On Mon, Sep 07, 2015 at 05:05:48PM +0200, Thomas Huth wrote:
> 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 ...
> ...
> >> 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?
> > 
> >> +
> >> +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.
> > 
> >> +        hrcrdp->received += size;
> >> +    }
> >> +    qemu_sem_post(&hrcrdp->sem);
> > 
> > Could you avoid a few wakeups by only posting the semaphore once the
> > buffer is filled?
> 
> I tried that now, but calling rng_backend_request_entropy() from within
> the callback function does not work (since entropy_available() in
> rng-random.c clears the callback function variable after having called
> the callback).

Ah, ok.  I'd missed the fact that rng_backend_request_entropy() was
being called again - I'd assumed that after the first call the backend
would just keep notifying until you had as much data as requested.

> And since you normally seem get 8 bytes in the first shot already anyway
> when using a good random number generator source, I think it's best to
> simply keep the logic as I've currently got it - at least that's easiest
> to understand when reading the source code.

Yes, I concur.

> 
> >> +}
> >> +
> >> +static target_ulong h_random(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >> +                             target_ulong opcode, target_ulong *args)
> >> +{
> >> +    HRandomData hrcrd;
> >> +
> >> +    if (!hrandom_rng) {
> >> +        return H_HARDWARE;
> >> +    }
> >> +
> >> +    qemu_sem_init(&hrcrd.sem, 0);
> >> +    hrcrd.val.v64 = 0;
> >> +    hrcrd.received = 0;
> >> +
> >> +    qemu_mutex_unlock_iothread();
> >> +    while (hrcrd.received < 8) {
> >> +        rng_backend_request_entropy((RngBackend *)hrandom_rng,
> >> +                                    8 - hrcrd.received, random_recv, 
> >> &hrcrd);
> >> +        qemu_sem_wait(&hrcrd.sem);
> >> +    }
> >> +    qemu_mutex_lock_iothread();
> >> +
> >> +    qemu_sem_destroy(&hrcrd.sem);
> >> +    args[0] = hrcrd.val.v64;
> >> +
> >> +    return H_SUCCESS;
> >> +}
> 
> I'll post a new version with the other changes soon.
> 
>  Thomas
> 
> 



-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

pgpW26ml64yMB.pgp
Description: PGP signature