[v3] ppc/spapr: Implement H_RANDOM hypercall in QEMU

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 <thuth@redhat.com>
---
 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.

 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

On 09/11/2015 03:17 AM, 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

s/pseude/pseudo/

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

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 <thuth@redhat.com>
> ---
>  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

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

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.

On (Tue) 15 Sep 2015 [14:26:06], David Gibson wrote:
> 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 ...

(snip)

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

Agreed.

		Amit
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html