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...?
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.
>> +}
>> +
>> +/* 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?
> 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.
>> +}
>> +
>> +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.
Thanks for the review,
Thomas
signature.asc
Description: OpenPGP digital signature
