diff mbox

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

Message ID 20150918110552.6487a506@bahia.local
State New
Headers show

Commit Message

Greg Kurz Sept. 18, 2015, 9:05 a.m. UTC
On Thu, 17 Sep 2015 10:49:41 +0200
Thomas Huth <thuth@redhat.com> 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 pseudo-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 back-end via the
> "rng" property of the device, e.g.:
> 
>  qemu-system-ppc64 -object rng-random,filename=/dev/hwrng,id=gid0 \
>                    -device spapr-rng,rng=gid0 ...
> 
> See http://wiki.qemu-project.org/Features-Done/VirtIORNG for
> other example of specifying RngBackends.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---

It is a good thing that the user can choose between in-kernel and backend,
and this patch does the work.

This being said, I am not sure about the use case where a user has a hwrng
capable platform and wants to run guests without any hwrng support at all is
an appropriate default behavior... I guess we will find more users that want
in-kernel being the default if it is available.

The patch below modifies yours to do just this: the pseudo-device is only
created if hwrng is present and not already created.


>  hw/ppc/Makefile.objs   |   2 +-
>  hw/ppc/spapr.c         |   8 +++
>  hw/ppc/spapr_rng.c     | 186 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h |   4 ++
>  target-ppc/kvm.c       |   9 +++
>  target-ppc/kvm_ppc.h   |   5 ++
>  6 files changed, 213 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, "could not set up rng device in the 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..ed43d5e
> --- /dev/null
> +++ b/hw/ppc/spapr_rng.c
> @@ -0,0 +1,186 @@
> +/*
> + * 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)
> +
> +struct sPAPRRngState {
> +    /*< private >*/
> +    DeviceState ds;
> +    RngBackend *backend;
> +    bool use_kvm;
> +};
> +typedef struct sPAPRRngState sPAPRRngState;
> +
> +struct HRandomData {
> +    QemuSemaphore sem;
> +    union {
> +        uint64_t v64;
> +        uint8_t v8[8];
> +    } val;
> +    int received;
> +};
> +typedef struct HRandomData 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);
> +}
> +
> +/* 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, "rng", TYPE_RNG_BACKEND,
> +                             (Object **)&rngstate->backend,
> +                             object_property_allow_set_link,
> +                             OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL);
> +    object_property_set_description(obj, "rng",
> +                                    "ID of the random number generator backend",
> +                                    NULL);
> +}
> +
> +static void spapr_rng_realize(DeviceState *dev, Error **errp)
> +{
> +
> +    sPAPRRngState *rngstate = SPAPR_RNG(dev);
> +
> +    if (rngstate->use_kvm) {
> +        if (kvmppc_enable_hwrng() == 0) {
> +            return;
> +        }
> +        /*
> +         * If user specified both, use-kvm and a backend, we fall back to
> +         * the backend now. If not, provide an appropriate error message.
> +         */
> +        if (!rngstate->backend) {
> +            error_setg(errp, "Could not initialize in-kernel H_RANDOM call!");
> +            return;
> +        }
> +    }
> +
> +    if (rngstate->backend) {
> +        spapr_register_hypercall(H_RANDOM, h_random);
> +    } else {
> +        error_setg(errp, "spapr-rng needs an RNG backend!");
> +    }
> +}
> +
> +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

Comments

David Gibson Sept. 21, 2015, 2:10 a.m. UTC | #1
On Fri, Sep 18, 2015 at 11:05:52AM +0200, Greg Kurz wrote:
> On Thu, 17 Sep 2015 10:49:41 +0200
> Thomas Huth <thuth@redhat.com> 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 pseudo-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 back-end via the
> > "rng" property of the device, e.g.:
> > 
> >  qemu-system-ppc64 -object rng-random,filename=/dev/hwrng,id=gid0 \
> >                    -device spapr-rng,rng=gid0 ...
> > 
> > See http://wiki.qemu-project.org/Features-Done/VirtIORNG for
> > other example of specifying RngBackends.
> > 
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> 
> It is a good thing that the user can choose between in-kernel and backend,
> and this patch does the work.
> 
> This being said, I am not sure about the use case where a user has a hwrng
> capable platform and wants to run guests without any hwrng support at all is
> an appropriate default behavior... I guess we will find more users that want
> in-kernel being the default if it is available.
> 
> The patch below modifies yours to do just this: the pseudo-device is only
> created if hwrng is present and not already created.

I have mixed feelings about this.  On the one hand, I agree that it
would be nice to allow H_RANDOM support by default.  On the other hand
the patch below leaves no way to turn it off for testing purposes.  It
also adds another place where the guest hardware depends on the host
configuration, which adds to the already substantial mess of ensuring
that source and destination hardware configuration matches for
migration.
Thomas Huth Sept. 21, 2015, 6 a.m. UTC | #2
On 21/09/15 04:10, David Gibson wrote:
> On Fri, Sep 18, 2015 at 11:05:52AM +0200, Greg Kurz wrote:
>> On Thu, 17 Sep 2015 10:49:41 +0200
>> Thomas Huth <thuth@redhat.com> 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 pseudo-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 back-end via the
>>> "rng" property of the device, e.g.:
>>>
>>>  qemu-system-ppc64 -object rng-random,filename=/dev/hwrng,id=gid0 \
>>>                    -device spapr-rng,rng=gid0 ...
>>>
>>> See http://wiki.qemu-project.org/Features-Done/VirtIORNG for
>>> other example of specifying RngBackends.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>
>> It is a good thing that the user can choose between in-kernel and backend,
>> and this patch does the work.
>>
>> This being said, I am not sure about the use case where a user has a hwrng
>> capable platform and wants to run guests without any hwrng support at all is
>> an appropriate default behavior... I guess we will find more users that want
>> in-kernel being the default if it is available.
>>
>> The patch below modifies yours to do just this: the pseudo-device is only
>> created if hwrng is present and not already created.
> 
> I have mixed feelings about this.  On the one hand, I agree that it
> would be nice to allow H_RANDOM support by default.  On the other hand
> the patch below leaves no way to turn it off for testing purposes.  It
> also adds another place where the guest hardware depends on the host
> configuration, which adds to the already substantial mess of ensuring
> that source and destination hardware configuration matches for
> migration.

I thought about this question on the weekend and came to the same
conclusion. I think if we want to enable this by default, it likely
should rather be done at the libvirt level instead?

 Thomas
Greg Kurz Sept. 21, 2015, 8:01 a.m. UTC | #3
On Mon, 21 Sep 2015 12:10:00 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Sep 18, 2015 at 11:05:52AM +0200, Greg Kurz wrote:
> > On Thu, 17 Sep 2015 10:49:41 +0200
> > Thomas Huth <thuth@redhat.com> 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 pseudo-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 back-end via the
> > > "rng" property of the device, e.g.:
> > > 
> > >  qemu-system-ppc64 -object rng-random,filename=/dev/hwrng,id=gid0 \
> > >                    -device spapr-rng,rng=gid0 ...
> > > 
> > > See http://wiki.qemu-project.org/Features-Done/VirtIORNG for
> > > other example of specifying RngBackends.
> > > 
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > ---
> > 
> > It is a good thing that the user can choose between in-kernel and backend,
> > and this patch does the work.
> > 
> > This being said, I am not sure about the use case where a user has a hwrng
> > capable platform and wants to run guests without any hwrng support at all is
> > an appropriate default behavior... I guess we will find more users that want
> > in-kernel being the default if it is available.
> > 
> > The patch below modifies yours to do just this: the pseudo-device is only
> > created if hwrng is present and not already created.
> 
> I have mixed feelings about this.  On the one hand, I agree that it
> would be nice to allow H_RANDOM support by default.  On the other hand
> the patch below leaves no way to turn it off for testing purposes.  It

This could be handled with a new spapr property, say 'use-hwrng', defaulting
to true.

> also adds another place where the guest hardware depends on the host
> configuration, which adds to the already substantial mess of ensuring
> that source and destination hardware configuration matches for
> migration.
> 

Yeah, describing the guest hw is really essential for migration... this
is best addressed at the libvirt level with a full XML description of
the machine... but FWIW if we are talking about running pseries on a
POWER8 or newer host, I am not aware about "hwrng-less" boards... but
I am probably missing something :)

Back to Thomas' patch, it does the job and brings H_RANDOM, which is
currently missing.

Acked-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

I could test both use-kvm and backend flavors (including migration).

Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

Thanks.

--
Greg
Thomas Huth Sept. 21, 2015, 8:26 a.m. UTC | #4
On 21/09/15 10:01, Greg Kurz wrote:
> On Mon, 21 Sep 2015 12:10:00 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
>> On Fri, Sep 18, 2015 at 11:05:52AM +0200, Greg Kurz wrote:
>>> On Thu, 17 Sep 2015 10:49:41 +0200
>>> Thomas Huth <thuth@redhat.com> 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 pseudo-device to QEMU that either
>>>> directly provides this hypercall to the guest or is able to
>>>> enable the in-kernel hypercall if available.
...
>>> It is a good thing that the user can choose between in-kernel and backend,
>>> and this patch does the work.
>>>
>>> This being said, I am not sure about the use case where a user has a hwrng
>>> capable platform and wants to run guests without any hwrng support at all is
>>> an appropriate default behavior... I guess we will find more users that want
>>> in-kernel being the default if it is available.
>>>
>>> The patch below modifies yours to do just this: the pseudo-device is only
>>> created if hwrng is present and not already created.
>>
>> I have mixed feelings about this.  On the one hand, I agree that it
>> would be nice to allow H_RANDOM support by default.  On the other hand
>> the patch below leaves no way to turn it off for testing purposes.  It
>> also adds another place where the guest hardware depends on the host
>> configuration, which adds to the already substantial mess of ensuring
>> that source and destination hardware configuration matches for
>> migration.
> 
> Yeah, describing the guest hw is really essential for migration... this
> is best addressed at the libvirt level with a full XML description of
> the machine... but FWIW if we are talking about running pseries on a
> POWER8 or newer host, I am not aware about "hwrng-less" boards... but
> I am probably missing something :)

Maybe it would be at least ok to enable it by default as long as
"-nodefaults" has not been specified as command line option?

> Back to Thomas' patch, it does the job and brings H_RANDOM, which is
> currently missing.
> 
> Acked-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> 
> I could test both use-kvm and backend flavors (including migration).
> 
> Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>

Thanks!

 Thomas
Greg Kurz Sept. 21, 2015, 8:37 a.m. UTC | #5
On Mon, 21 Sep 2015 10:26:52 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 21/09/15 10:01, Greg Kurz wrote:
> > On Mon, 21 Sep 2015 12:10:00 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> >> On Fri, Sep 18, 2015 at 11:05:52AM +0200, Greg Kurz wrote:
> >>> On Thu, 17 Sep 2015 10:49:41 +0200
> >>> Thomas Huth <thuth@redhat.com> 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 pseudo-device to QEMU that either
> >>>> directly provides this hypercall to the guest or is able to
> >>>> enable the in-kernel hypercall if available.
> ...
> >>> It is a good thing that the user can choose between in-kernel and backend,
> >>> and this patch does the work.
> >>>
> >>> This being said, I am not sure about the use case where a user has a hwrng
> >>> capable platform and wants to run guests without any hwrng support at all is
> >>> an appropriate default behavior... I guess we will find more users that want
> >>> in-kernel being the default if it is available.
> >>>
> >>> The patch below modifies yours to do just this: the pseudo-device is only
> >>> created if hwrng is present and not already created.
> >>
> >> I have mixed feelings about this.  On the one hand, I agree that it
> >> would be nice to allow H_RANDOM support by default.  On the other hand
> >> the patch below leaves no way to turn it off for testing purposes.  It
> >> also adds another place where the guest hardware depends on the host
> >> configuration, which adds to the already substantial mess of ensuring
> >> that source and destination hardware configuration matches for
> >> migration.
> > 
> > Yeah, describing the guest hw is really essential for migration... this
> > is best addressed at the libvirt level with a full XML description of
> > the machine... but FWIW if we are talking about running pseries on a
> > POWER8 or newer host, I am not aware about "hwrng-less" boards... but
> > I am probably missing something :)
> 
> Maybe it would be at least ok to enable it by default as long as
> "-nodefaults" has not been specified as command line option?
> 

It makes a lot of sense indeed. I guess David should take your patch
as it is now and the default behavior could be a follow up.

> > Back to Thomas' patch, it does the job and brings H_RANDOM, which is
> > currently missing.
> > 
> > Acked-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> > 
> > I could test both use-kvm and backend flavors (including migration).
> > 
> > Tested-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> 
> Thanks!
> 
>  Thomas
> 
>
Eric Blake Sept. 21, 2015, 3:46 p.m. UTC | #6
On 09/21/2015 12:00 AM, Thomas Huth wrote:

>>> This being said, I am not sure about the use case where a user has a hwrng
>>> capable platform and wants to run guests without any hwrng support at all is
>>> an appropriate default behavior... I guess we will find more users that want
>>> in-kernel being the default if it is available.
>>>
>>> The patch below modifies yours to do just this: the pseudo-device is only
>>> created if hwrng is present and not already created.
>>
>> I have mixed feelings about this.  On the one hand, I agree that it
>> would be nice to allow H_RANDOM support by default.  On the other hand
>> the patch below leaves no way to turn it off for testing purposes.  It
>> also adds another place where the guest hardware depends on the host
>> configuration, which adds to the already substantial mess of ensuring
>> that source and destination hardware configuration matches for
>> migration.
> 
> I thought about this question on the weekend and came to the same
> conclusion. I think if we want to enable this by default, it likely
> should rather be done at the libvirt level instead?
> 

Adding hardware by default to existing machine types has been a bane to
libvirt usage in the past.  If upgrading from an old qemu to a new one
suddenly turns on new guest-visible hardware with no change to the
command line, then libvirt has a much harder time migrating that guest.
 It's okay to have a new machine type turn on a feature by default, and
to have knobs so that the feature can be turned on even for older
machine types, but experience has shown that any new feature MUST come
with knobs and a way to learn if the feature can be turned on/off,
rather than just blindly assuming that turning it on is the right thing.
David Gibson Sept. 22, 2015, 1:38 a.m. UTC | #7
On Mon, Sep 21, 2015 at 10:37:28AM +0200, Greg Kurz wrote:
> On Mon, 21 Sep 2015 10:26:52 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
> > On 21/09/15 10:01, Greg Kurz wrote:
> > > On Mon, 21 Sep 2015 12:10:00 +1000
> > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > 
> > >> On Fri, Sep 18, 2015 at 11:05:52AM +0200, Greg Kurz wrote:
> > >>> On Thu, 17 Sep 2015 10:49:41 +0200
> > >>> Thomas Huth <thuth@redhat.com> 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 pseudo-device to QEMU that either
> > >>>> directly provides this hypercall to the guest or is able to
> > >>>> enable the in-kernel hypercall if available.
> > ...
> > >>> It is a good thing that the user can choose between in-kernel and backend,
> > >>> and this patch does the work.
> > >>>
> > >>> This being said, I am not sure about the use case where a user has a hwrng
> > >>> capable platform and wants to run guests without any hwrng support at all is
> > >>> an appropriate default behavior... I guess we will find more users that want
> > >>> in-kernel being the default if it is available.
> > >>>
> > >>> The patch below modifies yours to do just this: the pseudo-device is only
> > >>> created if hwrng is present and not already created.
> > >>
> > >> I have mixed feelings about this.  On the one hand, I agree that it
> > >> would be nice to allow H_RANDOM support by default.  On the other hand
> > >> the patch below leaves no way to turn it off for testing purposes.  It
> > >> also adds another place where the guest hardware depends on the host
> > >> configuration, which adds to the already substantial mess of ensuring
> > >> that source and destination hardware configuration matches for
> > >> migration.
> > > 
> > > Yeah, describing the guest hw is really essential for migration... this
> > > is best addressed at the libvirt level with a full XML description of
> > > the machine... but FWIW if we are talking about running pseries on a
> > > POWER8 or newer host, I am not aware about "hwrng-less" boards... but
> > > I am probably missing something :)
> > 
> > Maybe it would be at least ok to enable it by default as long as
> > "-nodefaults" has not been specified as command line option?

I like that in principle, but the -nodefaults option isn't exposed
outside vl.c

> It makes a lot of sense indeed. I guess David should take your patch
> as it is now and the default behavior could be a follow up.

That's the plan.  I've already  taken the base patch.
diff mbox

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 240fab72e7af..4b92efed2ef3 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -939,6 +939,14 @@  static int spapr_check_htab_fd(sPAPRMachineState *spapr)
     return rc;
 }
 
+static void spapr_rng_create(void)
+{
+    Object *rng = object_new(TYPE_SPAPR_RNG);
+
+    object_property_set_bool(rng, true, "use-kvm", &error_abort);
+    object_property_set_bool(rng, true, "realized", &error_abort);
+}
+
 static void ppc_spapr_reset(void)
 {
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
@@ -962,6 +970,14 @@  static void ppc_spapr_reset(void)
     spapr->rtas_addr = rtas_limit - RTAS_MAX_SIZE;
     spapr->fdt_addr = spapr->rtas_addr - FDT_MAX_SIZE;
 
+    /* Create a rng device if the user did not provide it already and
+     * KVM has hwrng support.
+     */
+    if (kvmppc_hwrng_present() &&
+        !object_resolve_path_type("", TYPE_SPAPR_RNG, NULL)) {
+        spapr_rng_create();
+    }
+
     /* Load the fdt */
     spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr,
                        spapr->rtas_size);
diff --git a/hw/ppc/spapr_rng.c b/hw/ppc/spapr_rng.c
index ed43d5e04221..ee5af302bd4d 100644
--- a/hw/ppc/spapr_rng.c
+++ b/hw/ppc/spapr_rng.c
@@ -114,7 +114,7 @@  static void spapr_rng_realize(DeviceState *dev, Error **errp)
     sPAPRRngState *rngstate = SPAPR_RNG(dev);
 
     if (rngstate->use_kvm) {
-        if (kvmppc_enable_hwrng() == 0) {
+        if (kvmppc_hwrng_present() && kvmppc_enable_hwrng() == 0) {
             return;
         }
         /*
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 42f66fea23e9..008f8a26ab17 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -2485,11 +2485,12 @@  int kvm_arch_msi_data_to_gsi(uint32_t data)
     return data & 0xffff;
 }
 
-int kvmppc_enable_hwrng(void)
+bool kvmppc_hwrng_present(void)
 {
-    if (!kvm_enabled() || !kvm_check_extension(kvm_state, KVM_CAP_PPC_HWRNG)) {
-        return -1;
-    }
+    return kvm_enabled() && kvm_check_extension(kvm_state, KVM_CAP_PPC_HWRNG);
+}
 
+int kvmppc_enable_hwrng(void)
+{
     return kvmppc_enable_hcall(kvm_state, H_RANDOM);
 }
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index 68836b401105..4b78bfe5224a 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);
+bool kvmppc_hwrng_present(void);
 int kvmppc_enable_hwrng(void);
 
 #else
@@ -249,6 +250,11 @@  static inline bool kvmppc_has_cap_fixup_hcalls(void)
     abort();
 }
 
+static inline bool kvmppc_hwrng_present(void)
+{
+    return false;
+}
+
 static inline int kvmppc_enable_hwrng(void)
 {
     return -1;