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

[David Gibson]

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.
> 
> Signed-off-by: Thomas Huth <address@hidden>
> ---
>  v3:
>  - Completely reworked the patch set accordingly to discussion
>    on the mailing list, so that the code is now encapsulated
>    as a QEMU device in a separate file.

Looking good..

> 
>  hw/ppc/Makefile.objs   |   2 +-
>  hw/ppc/spapr.c         |   8 +++
>  hw/ppc/spapr_rng.c     | 178 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |   4 ++
>  target-ppc/kvm.c       |   9 +++
>  target-ppc/kvm_ppc.h   |   5 ++
>  6 files changed, 205 insertions(+), 1 deletion(-)
>  create mode 100644 hw/ppc/spapr_rng.c
> 
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index c8ab06e..c1ffc77 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -3,7 +3,7 @@ obj-y += ppc.o ppc_booke.o
>  # IBM pSeries (sPAPR)
>  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> -obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o
> +obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>  obj-y += spapr_pci_vfio.o
>  endif
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bf0c64f..34e7d24 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -768,6 +768,14 @@ static void spapr_finalize_fdt(sPAPRMachineState *spapr,
>          exit(1);
>      }
>  
> +    if (object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)) {
> +        ret = spapr_rng_populate_dt(fdt);
> +        if (ret < 0) {
> +            fprintf(stderr, "couldn't setup rng device in fdt\n");
> +            exit(1);
> +        }
> +    }
> +
>      QLIST_FOREACH(phb, &spapr->phbs, list) {
>          ret = spapr_populate_pci_dt(phb, PHANDLE_XICP, fdt);
>      }
> diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c
> new file mode 100644
> index 0000000..d4923bc
> --- /dev/null
> +++ b/hw/ppc/spapr_rng.c
> @@ -0,0 +1,178 @@
> +/*
> + * QEMU sPAPR random number generator "device" for H_RANDOM hypercall
> + *
> + * Copyright 2015 Thomas Huth, Red Hat Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License,
> + * or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#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.

> +}
> +
> +/* 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.

More importantly, this should probably be called "rng" not "backend"
to match virtio-rng.

> +}
> +
> +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".

> +    spapr_register_hypercall(H_RANDOM, h_random);
> +}
> +
> +int spapr_rng_populate_dt(void *fdt)
> +{
> +    int node;
> +    int ret;
> +
> +    node = qemu_fdt_add_subnode(fdt, "/ibm,platform-facilities");
> +    if (node <= 0) {
> +        return -1;
> +    }
> +    ret = fdt_setprop_string(fdt, node, "device_type",
> +                             "ibm,platform-facilities");
> +    ret |= fdt_setprop_cell(fdt, node, "#address-cells", 0x1);
> +    ret |= fdt_setprop_cell(fdt, node, "#size-cells", 0x0);
> +
> +    node = fdt_add_subnode(fdt, node, "ibm,random-v1");
> +    if (node <= 0) {
> +        return -1;
> +    }
> +    ret |= fdt_setprop_string(fdt, node, "compatible", "ibm,random");
> +
> +    return ret ? -1 : 0;
> +}
> +
> +static Property spapr_rng_properties[] = {
> +    DEFINE_PROP_BOOL("use-kvm", sPAPRRngState, use_kvm, false),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void spapr_rng_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = spapr_rng_realize;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +    dc->props = spapr_rng_properties;
> +}
> +
> +static const TypeInfo spapr_rng_info = {
> +    .name          = TYPE_SPAPR_RNG,
> +    .parent        = TYPE_DEVICE,
> +    .instance_size = sizeof(sPAPRRngState),
> +    .instance_init = spapr_rng_instance_init,
> +    .class_init    = spapr_rng_class_init,
> +};
> +
> +static void spapr_rng_register_type(void)
> +{
> +    type_register_static(&spapr_rng_info);
> +}
> +type_init(spapr_rng_register_type)
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 91a61ab..4e8aa2d 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -331,6 +331,7 @@ struct sPAPRMachineState {
>  #define H_SET_MPP               0x2D0
>  #define H_GET_MPP               0x2D4
>  #define H_XIRR_X                0x2FC
> +#define H_RANDOM                0x300
>  #define H_SET_MODE              0x31C
>  #define MAX_HCALL_OPCODE        H_SET_MODE
>  
> @@ -603,10 +604,13 @@ struct sPAPRConfigureConnectorState {
>  void spapr_ccs_reset_hook(void *opaque);
>  
>  #define TYPE_SPAPR_RTC "spapr-rtc"
> +#define TYPE_SPAPR_RNG "spapr-rng"
>  
>  void spapr_rtc_read(DeviceState *dev, struct tm *tm, uint32_t *ns);
>  int spapr_rtc_import_offset(DeviceState *dev, int64_t legacy_offset);
>  
> +int spapr_rng_populate_dt(void *fdt);
> +
>  #define SPAPR_MEMORY_BLOCK_SIZE (1 << 28) /* 256MB */
>  
>  #endif /* !defined (__HW_SPAPR_H__) */
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 110436d..42f66fe 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -2484,3 +2484,12 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>      return data & 0xffff;
>  }
> +
> +int kvmppc_enable_hwrng(void)
> +{
> +    if (!kvm_enabled() || !kvm_check_extension(kvm_state, 
> KVM_CAP_PPC_HWRNG)) {
> +        return -1;
> +    }
> +
> +    return kvmppc_enable_hcall(kvm_state, H_RANDOM);
> +}
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 4d30e27..68836b4 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -55,6 +55,7 @@ void kvmppc_hash64_free_pteg(uint64_t token);
>  void kvmppc_hash64_write_pte(CPUPPCState *env, target_ulong pte_index,
>                               target_ulong pte0, target_ulong pte1);
>  bool kvmppc_has_cap_fixup_hcalls(void);
> +int kvmppc_enable_hwrng(void);
>  
>  #else
>  
> @@ -248,6 +249,10 @@ static inline bool kvmppc_has_cap_fixup_hcalls(void)
>      abort();
>  }
>  
> +static inline int kvmppc_enable_hwrng(void)
> +{
> +    return -1;
> +}
>  #endif
>  
>  #ifndef CONFIG_KVM

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

pgpX_MhesEAxL.pgp
Description: PGP signature