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.
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.
> 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
> @@ -312,7 +312,8 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
> hwaddr kernel_size,
> bool little_endian,
> const char *kernel_cmdline,
> - uint32_t epow_irq)
> + uint32_t epow_irq,
> + bool enable_h_random)
> {
> void *fdt;
> uint32_t start_prop = cpu_to_be32(initrd_base);
> @@ -466,7 +467,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>
> _FDT((fdt_end_node(fdt)));
>
> - if (kvmppc_hwrng_present()) {
> + if (enable_h_random) {
> _FDT(fdt_begin_node(fdt, "ibm,platform-facilities"));
>
> _FDT(fdt_property_string(fdt, "name", "ibm,platform-facilities"));
> @@ -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.
> char *filename;
>
> msi_supported = true;
> @@ -1699,6 +1700,10 @@ static void ppc_spapr_init(MachineState *machine)
> }
> g_free(filename);
>
> + /* Enable H_RANDOM hypercall if requested by user or supported by kernel
> */
> + enable_h_random = kvmppc_hwrng_present() ||
> + !spapr_h_random_init(spapr->h_random_type);
> +
> /* FIXME: Should register things through the MachineState's qdev
> * interface, this is a legacy from the sPAPREnvironment structure
> * which predated MachineState but had a similar function */
> @@ -1710,7 +1715,8 @@ static void ppc_spapr_init(MachineState *machine)
> spapr->fdt_skel = spapr_create_fdt_skel(initrd_base, initrd_size,
> kernel_size, kernel_le,
> kernel_cmdline,
> - spapr->check_exception_irq);
> + spapr->check_exception_irq,
> + enable_h_random);
> assert(spapr->fdt_skel != NULL);
>
> /* used by RTAS */
> @@ -1810,6 +1816,21 @@ static void spapr_set_kvm_type(Object *obj, const char
> *value, Error **errp)
> spapr->kvm_type = g_strdup(value);
> }
>
> +static char *spapr_get_h_random_type(Object *obj, Error **errp)
> +{
> + sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +
> + return g_strdup(spapr->h_random_type);
> +}
> +
> +static void spapr_set_h_random_type(Object *obj, const char *val, Error
> **errp)
> +{
> + sPAPRMachineState *spapr = SPAPR_MACHINE(obj);
> +
> + g_free(spapr->h_random_type);
> + spapr->h_random_type = g_strdup(val);
> +}
> +
> static void spapr_machine_initfn(Object *obj)
> {
> object_property_add_str(obj, "kvm-type",
> @@ -1817,6 +1838,13 @@ static void spapr_machine_initfn(Object *obj)
> object_property_set_description(obj, "kvm-type",
> "Specifies the KVM virtualization mode
> (HV, PR)",
> NULL);
> +
> + object_property_add_str(obj, "h-random", spapr_get_h_random_type,
> + spapr_set_h_random_type, NULL);
> + object_property_set_description(obj, "h-random",
> + "Select RNG backend for H_RANDOM
> hypercall "
> + "(rng-random, rng-egd)",
> + NULL);
> }
>
> static void ppc_cpu_do_nmi_on_cpu(void *arg)
> 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?
> +}
> +
> +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;
> +}
> +
> +int spapr_h_random_init(const char *backend_type)
> +{
> + Error *local_err = NULL;
> +
> + if (!backend_type) {
> + return -1;
> + }
> +
> + hrandom_rng = RNG_RANDOM(object_new(backend_type));
> + user_creatable_complete(OBJECT(hrandom_rng), &local_err);
> + if (local_err) {
> + error_printf("Failed to initialize backend for H_RANDOM:\n %s\n",
> + error_get_pretty(local_err));
> + object_unref(OBJECT(hrandom_rng));
> + return -1;
> + }
> +
> + spapr_register_hypercall(H_RANDOM, h_random);
> +
> + return 0;
> +}
> +
> static spapr_hcall_fn papr_hypercall_table[(MAX_HCALL_OPCODE / 4) + 1];
> static spapr_hcall_fn kvmppc_hypercall_table[KVMPPC_HCALL_MAX -
> KVMPPC_HCALL_BASE + 1];
>
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index ab8906f..de17624 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -76,6 +76,7 @@ struct sPAPRMachineState {
>
> /*< public >*/
> char *kvm_type;
> + char *h_random_type;
> };
>
> #define H_SUCCESS 0
> @@ -592,6 +593,7 @@ int spapr_tcet_dma_dt(void *fdt, int node_off, const char
> *propname,
> void spapr_pci_switch_vga(bool big_endian);
> void spapr_hotplug_req_add_event(sPAPRDRConnector *drc);
> void spapr_hotplug_req_remove_event(sPAPRDRConnector *drc);
> +int spapr_h_random_init(const char *backend_type);
>
> /* rtas-configure-connector state */
> struct sPAPRConfigureConnectorState {
--
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
pgpiK_muPmAWs.pgp
Description: PGP signature
