Re: [Qemu-devel] [PATCH v3] ppc/spapr: Implement H_RANDOM hypercall in QEMU

[David Gibson]

On Mon, Sep 14, 2015 at 08:32:36AM +0200, Thomas Huth wrote:
> On 14/09/15 04:15, David Gibson wrote:
> > On Fri, Sep 11, 2015 at 11:17:01AM +0200, Thomas Huth wrote:
> >> The PAPR interface defines a hypercall to pass high-quality
> >> hardware generated random numbers to guests. Recent kernels can
> >> already provide this hypercall to the guest if the right hardware
> >> random number generator is available. But in case the user wants
> >> to use another source like EGD, or QEMU is running with an older
> >> kernel, we should also have this call in QEMU, so that guests that
> >> do not support virtio-rng yet can get good random numbers, too.
> >>
> >> This patch now adds a new pseude-device to QEMU that either
> >> directly provides this hypercall to the guest or is able to
> >> enable the in-kernel hypercall if available. The in-kernel
> >> hypercall can be enabled with the use-kvm property, e.g.:
> >>
> >>  qemu-system-ppc64 -device spapr-rng,use-kvm=true
> >>
> >> For handling the hypercall in QEMU instead, a RngBackend is required
> >> since the hypercall should provide "good" random data instead of
> >> pseudo-random (like from 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 "backend"
> >> property of the device, e.g.:
> >>
> >>  qemu-system-ppc64 -object rng-random,filename=/dev/hwrng,id=rng0 \
> >>                    -device spapr-rng,backend=rng0 ...
> >>
> >> See http://wiki.qemu-project.org/Features-Done/VirtIORNG for
> >> other example of specifying RngBackends.
> ...
> >> +
> >> +#include "qemu/error-report.h"
> >> +#include "sysemu/sysemu.h"
> >> +#include "sysemu/device_tree.h"
> >> +#include "sysemu/rng.h"
> >> +#include "hw/ppc/spapr.h"
> >> +#include "kvm_ppc.h"
> >> +
> >> +#define SPAPR_RNG(obj) \
> >> +    OBJECT_CHECK(sPAPRRngState, (obj), TYPE_SPAPR_RNG)
> >> +
> >> +typedef struct sPAPRRngState {
> >> +    /*< private >*/
> >> +    DeviceState ds;
> >> +    RngBackend *backend;
> >> +    bool use_kvm;
> >> +} sPAPRRngState;
> >> +
> >> +typedef struct HRandomData {
> >> +    QemuSemaphore sem;
> >> +    union {
> >> +        uint64_t v64;
> >> +        uint8_t v8[8];
> >> +    } val;
> >> +    int received;
> >> +} HRandomData;
> >> +
> >> +/* Callback function for the RngBackend */
> >> +static void random_recv(void *dest, const void *src, size_t size)
> >> +{
> >> +    HRandomData *hrdp = dest;
> >> +
> >> +    if (src && size > 0) {
> >> +        assert(size + hrdp->received <= sizeof(hrdp->val.v8));
> >> +        memcpy(&hrdp->val.v8[hrdp->received], src, size);
> >> +        hrdp->received += size;
> >> +    }
> >> +
> >> +    qemu_sem_post(&hrdp->sem);
> > 
> > I'm assuming qemu_sem_post() includes the necessary memory barrier to
> > make sure the requesting thread actually sees the data.
> 
> Not sure whether I fully got your point here... both callback function
> and main thread are calling an extern C-function, so the compiler should
> not assume that the memory stays the same in the main thread...?

I'm not talking about a compiler barrier: the callback will likely be
invoked on a different CPU from the vcpu thread that invoked H_RANDOM,
so on a weakly ordered arch like Power we need a real CPU memory barrier.

> Anyway, I've tested the hypercall by implementing it in SLOF and calling
> it a couple of times there to see that all bits in the result behave
> randomly, so for me this is working fine.

Right, I'd be almost certain anyway that qemu_sem_post() (actually
likely the pthreads functions it invokes) will include the necessary
barriers to stop accesses leaking outside the locked region.

> 
> >> +}
> >> +
> >> +/* Handler for the H_RANDOM hypercall */
> >> +static target_ulong h_random(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >> +                             target_ulong opcode, target_ulong *args)
> >> +{
> >> +    sPAPRRngState *rngstate;
> >> +    HRandomData hrdata;
> >> +
> >> +    rngstate = SPAPR_RNG(object_resolve_path_type("", TYPE_SPAPR_RNG, 
> >> NULL));
> >> +
> >> +    if (!rngstate || !rngstate->backend) {
> >> +        return H_HARDWARE;
> >> +    }
> >> +
> >> +    qemu_sem_init(&hrdata.sem, 0);
> >> +    hrdata.val.v64 = 0;
> >> +    hrdata.received = 0;
> >> +
> >> +    qemu_mutex_unlock_iothread();
> >> +    while (hrdata.received < 8) {
> >> +        rng_backend_request_entropy(rngstate->backend, 8 - 
> >> hrdata.received,
> >> +                                    random_recv, &hrdata);
> >> +        qemu_sem_wait(&hrdata.sem);
> >> +    }
> >> +    qemu_mutex_lock_iothread();
> >> +
> >> +    qemu_sem_destroy(&hrdata.sem);
> >> +    args[0] = hrdata.val.v64;
> >> +
> >> +    return H_SUCCESS;
> >> +}
> >> +
> >> +static void spapr_rng_instance_init(Object *obj)
> >> +{
> >> +    sPAPRRngState *rngstate = SPAPR_RNG(obj);
> >> +
> >> +    if (object_resolve_path_type("", TYPE_SPAPR_RNG, NULL) != NULL) {
> >> +        error_report("spapr-rng can not be instantiated twice!");
> >> +        return;
> >> +    }
> >> +
> >> +    object_property_add_link(obj, "backend", TYPE_RNG_BACKEND,
> >> +                             (Object **)&rngstate->backend,
> >> +                             object_property_allow_set_link,
> >> +                             OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);
> >> +    object_property_set_description(obj, "backend",
> >> +                                    "ID of the random number generator 
> >> backend",
> >> +                                    NULL);
> > 
> > Since virtio-rng does it the same way, I'm assuming there's a reason
> > this is constructed with object_propery_add() rather than listing it
> > in spapr_rng_properties, but it's not obvious what the reason is.
> 
> I did not spot a macro a la "DEFINE_PROP_LINK" that could be used for
> this. Do you see a possibility to define a link that way?

No, like I say since virtio-rng does it that way I assume there's a reason.

> > More importantly, this should probably be called "rng" not "backend"
> > to match virtio-rng.
> 
> Since the device is already called "spapr-rng", i.e. has "rng" in its
> name, I'd rather like to keep this as "backend" to make it clear that
> you specify the backend this way.

Hm, personally I'd weigh consistency with virtio-rng higher than the
slightly confusing name.

> 
> >> +}
> >> +
> >> +static void spapr_rng_realize(DeviceState *dev, Error **errp)
> >> +{
> >> +
> >> +    sPAPRRngState *rngstate = SPAPR_RNG(dev);
> >> +
> >> +    if (rngstate->use_kvm) {
> >> +        if (kvmppc_enable_hwrng() != 0) {
> >> +            error_setg(errp, "Could not initialize in-kernel H_RANDOM 
> >> call!");
> >> +        }
> >> +        return;
> >> +    }
> >> +
> >> +    if (!rngstate->backend) {
> >> +        error_setg(errp, "spapr-rng needs a RNG backend!");
> >> +        return;
> >> +    }
> > 
> > So, the logic here means you have to explicitly choose whether to use
> > the kernel implementation or the qemu imeplementation.
> > 
> > It seems to me it might be useful to be able to specify "use the
> > kernel implementation if available, otherwise fall back to qemu".
> 
> Right, makes sense, I'll update this logic.

Ok.

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

pgp5RZDvnTt1F.pgp
Description: PGP signature