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

[David Gibson]

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 <address@hidden>
> ---
>  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

pgpIAarP6AnOR.pgp
Description: PGP signature