[Qemu-devel] [PATCH v2 0/2] ppc/spapr_hcall: Implement H_RANDOM hypercall

[Qemu-devel] [PATCH v2 0/2] ppc/spapr_hcall: Implement H_RANDOM hypercall

[Thomas Huth]

The H_RANDOM sPAPR hypercall is used to pass high-quality random
numbers to guests (similar to virtio-rnd). Since the Linux guest
kernels already support this hypercall, we should provide this
interface in QEMU, too, in case virtio-rnd is not available in
the guest yet.

First patch is an older patch by Michael Ellerman which sets up
the device tree for this interface (which is already used when
KVM in the host kernel supports the H_RANDOM hypercall). The
second patch then implements the hypercall for plain QEMU, too,
by using the RngBackends of QEMU.

Note: Since I do not have access to the SPAPR spec, I figured out
the calling conventions of the hypercall by looking at the way
the Linux kernel is using this call to retriev random data. So
there might be some bugs or missing pieces here... - if you spot
something, please tell me!

v2:
- Added Michael's patch for setting up the device tree information
- After discussing this hypercall with some people at KVM
  forum 2015, I've now reworked the code to use the RngBackend
  of QEMU instead of using /dev/random directly.

Michael Ellerman (1):
  spapr: Add support for hwrng when available

Thomas Huth (1):
  ppc/spapr_hcall: Implement H_RANDOM hypercall in QEMU

 hw/ppc/spapr.c         | 50 +++++++++++++++++++++++++++++++--
 hw/ppc/spapr_hcall.c   | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ppc/spapr.h |  3 ++
 target-ppc/kvm.c       |  5 ++++
 target-ppc/kvm_ppc.h   |  5 ++++
 5 files changed, 135 insertions(+), 3 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/2] spapr: Add support for hwrng when available

[Thomas Huth]

From: Michael Ellerman <michael@ellerman.id.au>

Some powerpc systems have support for a hardware random number generator
(hwrng). If such a hwrng is present the host kernel can provide access
to it via the H_RANDOM hcall.

The kernel advertises the presence of a hwrng with the KVM_CAP_PPC_HWRNG
capability. If this is detected we add the appropriate device tree bits
to advertise the presence of the hwrng to the guest kernel.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
[thuth: Refreshed patch so it applies to QEMU master branch]
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c         | 16 ++++++++++++++++
 include/hw/ppc/spapr.h |  1 +
 target-ppc/kvm.c       |  5 +++++
 target-ppc/kvm_ppc.h   |  5 +++++
 4 files changed, 27 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bf0c64f..bc3a112 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -466,6 +466,22 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
 
     _FDT((fdt_end_node(fdt)));
 
+    if (kvmppc_hwrng_present()) {
+        _FDT(fdt_begin_node(fdt, "ibm,platform-facilities"));
+
+        _FDT(fdt_property_string(fdt, "name", "ibm,platform-facilities"));
+        _FDT(fdt_property_string(fdt, "device_type",
+                                 "ibm,platform-facilities"));
+        _FDT(fdt_property_cell(fdt, "#address-cells", 0x1));
+        _FDT(fdt_property_cell(fdt, "#size-cells", 0x0));
+        _FDT(fdt_begin_node(fdt, "ibm,random-v1"));
+        _FDT(fdt_property_string(fdt, "name", "ibm,random-v1"));
+        _FDT(fdt_property_string(fdt, "compatible", "ibm,random"));
+        _FDT((fdt_end_node(fdt)));
+    }
+
+    _FDT((fdt_end_node(fdt)));
+
     /* event-sources */
     spapr_events_fdt_skel(fdt, epow_irq);
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 91a61ab..ab8906f 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
 
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 110436d..7317f8f 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -2484,3 +2484,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
 {
     return data & 0xffff;
 }
+
+bool kvmppc_hwrng_present(void)
+{
+    return kvm_enabled() && kvm_check_extension(kvm_state, KVM_CAP_PPC_HWRNG);
+}
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index 4d30e27..62ff601 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);
 
 #else
 
@@ -248,6 +249,10 @@ static inline bool kvmppc_has_cap_fixup_hcalls(void)
     abort();
 }
 
+static inline bool kvmppc_hwrng_present(void)
+{
+    return false;
+}
 #endif
 
 #ifndef CONFIG_KVM
-- 
1.8.3.1

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

[Thomas Huth]

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

to use the /dev/random backend, or "h-random=rng-egd" to use the
Entropy Gathering Daemon instead.

Signed-off-by: Thomas Huth <thuth@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;
     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;
+
+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);
+        hrcrdp->received += size;
+    }
+    qemu_sem_post(&hrcrdp->sem);
+}
+
+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 {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 1/2] spapr: Add support for hwrng when available

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4016 bytes --]

On Mon, Aug 31, 2015 at 08:46:01PM +0200, Thomas Huth wrote:
> From: Michael Ellerman <michael@ellerman.id.au>
> 
> Some powerpc systems have support for a hardware random number generator
> (hwrng). If such a hwrng is present the host kernel can provide access
> to it via the H_RANDOM hcall.
> 
> The kernel advertises the presence of a hwrng with the KVM_CAP_PPC_HWRNG
> capability. If this is detected we add the appropriate device tree bits
> to advertise the presence of the hwrng to the guest kernel.
> 
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> [thuth: Refreshed patch so it applies to QEMU master branch]
> Signed-off-by: Thomas Huth <thuth@redhat.com>

So, I'm confused by one thing.

I thought new kernel handled hcalls were supposed to be disabled by
default, but I don't see any calls to kvmppc_enable_hcall() to turn on
H_RANDOM.

> ---
>  hw/ppc/spapr.c         | 16 ++++++++++++++++
>  include/hw/ppc/spapr.h |  1 +
>  target-ppc/kvm.c       |  5 +++++
>  target-ppc/kvm_ppc.h   |  5 +++++
>  4 files changed, 27 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bf0c64f..bc3a112 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -466,6 +466,22 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
>  
>      _FDT((fdt_end_node(fdt)));
>  
> +    if (kvmppc_hwrng_present()) {
> +        _FDT(fdt_begin_node(fdt, "ibm,platform-facilities"));
> +
> +        _FDT(fdt_property_string(fdt, "name", "ibm,platform-facilities"));

Also, "name" properties don't need to be set in flattened trees -
that's implicit in the node name itself.

> +        _FDT(fdt_property_string(fdt, "device_type",
> +                                 "ibm,platform-facilities"));
> +        _FDT(fdt_property_cell(fdt, "#address-cells", 0x1));
> +        _FDT(fdt_property_cell(fdt, "#size-cells", 0x0));
> +        _FDT(fdt_begin_node(fdt, "ibm,random-v1"));
> +        _FDT(fdt_property_string(fdt, "name", "ibm,random-v1"));
> +        _FDT(fdt_property_string(fdt, "compatible", "ibm,random"));
> +        _FDT((fdt_end_node(fdt)));
> +    }
> +
> +    _FDT((fdt_end_node(fdt)));
> +
>      /* event-sources */
>      spapr_events_fdt_skel(fdt, epow_irq);
>  
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 91a61ab..ab8906f 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
>  
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 110436d..7317f8f 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -2484,3 +2484,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>      return data & 0xffff;
>  }
> +
> +bool kvmppc_hwrng_present(void)
> +{
> +    return kvm_enabled() && kvm_check_extension(kvm_state, KVM_CAP_PPC_HWRNG);
> +}
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 4d30e27..62ff601 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);
>  
>  #else
>  
> @@ -248,6 +249,10 @@ static inline bool kvmppc_has_cap_fixup_hcalls(void)
>      abort();
>  }
>  
> +static inline bool kvmppc_hwrng_present(void)
> +{
> +    return false;
> +}
>  #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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 9532 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2 1/2] spapr: Add support for hwrng when available

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 1339 bytes --]

On 01/09/15 02:38, David Gibson wrote:
> On Mon, Aug 31, 2015 at 08:46:01PM +0200, Thomas Huth wrote:
>> From: Michael Ellerman <michael@ellerman.id.au>
>>
>> Some powerpc systems have support for a hardware random number generator
>> (hwrng). If such a hwrng is present the host kernel can provide access
>> to it via the H_RANDOM hcall.
>>
>> The kernel advertises the presence of a hwrng with the KVM_CAP_PPC_HWRNG
>> capability. If this is detected we add the appropriate device tree bits
>> to advertise the presence of the hwrng to the guest kernel.
>>
>> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
>> [thuth: Refreshed patch so it applies to QEMU master branch]
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> So, I'm confused by one thing.
> 
> I thought new kernel handled hcalls were supposed to be disabled by
> default, but I don't see any calls to kvmppc_enable_hcall() to turn on
> H_RANDOM.

Michael's patch was from 2013, the kvmppc_enable_hcall() stuff seems to
be from 2014 ... so the enablement is likely missing in this patch,
indeed. I didn't test the in-kernel hypercall yet, just my QEMU
implementation so far, that's why I did not notice this yet.

Michael, do you want to rework your patch? Or shall I add an additional
enablement patch to my queue?

 Thomas



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 4548 bytes --]

On 01/09/15 02:47, David Gibson wrote:
> 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.

Ok, sounds like a good idea, will rework my patch accordingly.

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

We could either print a warning message if we detect that the in-kernel
implementation is available and the user still tries to use the QEMU
implementation - or we simply do not enable the in-kernel hypercall via
kvmppc_enable_hcall() if the user tried to use the QEMU implementation.
What would you prefer?

>> to use the /dev/random backend, or "h-random=rng-egd" to use the
>> Entropy Gathering Daemon instead.
>>
>> Signed-off-by: Thomas Huth <thuth@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
...
>> @@ -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.

Ok.

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

Sure.

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

That's a different issue - the if-statement above seems to be necassary
because the callback sometimes seems to be called with size = 0 or src =
NULL, so I had to add this here to avoid crashes.
But I can also add an assert(hrcrdp->received + size <= 8) here if you
like, just to be sure.

>> +        hrcrdp->received += size;
>> +    }
>> +    qemu_sem_post(&hrcrdp->sem);
> 
> Could you avoid a few wakeups by only posting the semaphore once the
> buffer is filled?

Then the callback functions has to trigger the next
rng_backend_request_entropy() ... not sure yet whether I like that
idea... but I can have a try.

 Thomas




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Amit Shah]

On (Mon) 31 Aug 2015 [20:46:02], 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.

virtio-rng, not rnd.

Can you elaborate what you mean by 'guests that do not support
virtio-rng yet'?  The Linux kernel has had the virtio-rng driver since
2.6.26, so I'm assuming that's not the thing you're alluding to.

Not saying this hypercall isn't a good idea, just asking why.  I think
there's are valid reasons like the driver fails to load, or the driver
is compiled out, or simply is loaded too late in the boot cycle.

> 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 ...
> 
> to use the /dev/random backend, or "h-random=rng-egd" to use the
> Entropy Gathering Daemon instead.

I was going to suggest using -object here, but already I see you and
David have reached an agreement for that.

Out of curiosity: what does the host kernel use for its source when
going the hypercall route?

> +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);
> +        hrcrdp->received += size;
> +    }
> +    qemu_sem_post(&hrcrdp->sem);
> +}
> +
> +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);
> +    }

Is it possible for a second hypercall to arrive while the first is
waiting for the backend to provide data?

> +    qemu_mutex_lock_iothread();
> +
> +    qemu_sem_destroy(&hrcrd.sem);
> +    args[0] = hrcrd.val.v64;
> +
> +    return H_SUCCESS;
> +}

		Amit

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4048 bytes --]

On Wed, Sep 02, 2015 at 11:04:12AM +0530, Amit Shah wrote:
> On (Mon) 31 Aug 2015 [20:46:02], 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.
> 
> virtio-rng, not rnd.
> 
> Can you elaborate what you mean by 'guests that do not support
> virtio-rng yet'?  The Linux kernel has had the virtio-rng driver since
> 2.6.26, so I'm assuming that's not the thing you're alluding to.
> 
> Not saying this hypercall isn't a good idea, just asking why.  I think
> there's are valid reasons like the driver fails to load, or the driver
> is compiled out, or simply is loaded too late in the boot cycle.

Yeah, I think we'd be talking about guests that just don't have it
configured, although I suppose it's possible someone out there is
using something earlier than 2.6.26 as well.  Note that H_RANDOM has
been supported under PowerVM for a long time, and PowerVM doesn't have
any virtio support.  So it is plausible that there are guests out
there with with H_RANDOM support but no virtio-rng support, although I
don't know of any examples specifically.  RHEL6 had virtio support,
including virtio-rng more or less by accident (since it was only
supported under PowerVM).  SLES may not have made the same fortunate
error - I don't have a system handy to check.

> > 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 ...
> > 
> > to use the /dev/random backend, or "h-random=rng-egd" to use the
> > Entropy Gathering Daemon instead.
> 
> I was going to suggest using -object here, but already I see you and
> David have reached an agreement for that.
> 
> Out of curiosity: what does the host kernel use for its source when
> going the hypercall route?

I believe it draws from the same entropy pool as /dev/random.

> > +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);
> > +        hrcrdp->received += size;
> > +    }
> > +    qemu_sem_post(&hrcrdp->sem);
> > +}
> > +
> > +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);
> > +    }
> 
> Is it possible for a second hypercall to arrive while the first is
> waiting for the backend to provide data?

Yes it is.  The hypercall itself is synchronous, but you could get
concurrent calls from different guest CPUs.  Hence the need for
iothread unlocking.

> > +    qemu_mutex_lock_iothread();
> > +
> > +    qemu_sem_destroy(&hrcrd.sem);
> > +    args[0] = hrcrd.val.v64;
> > +
> > +    return H_SUCCESS;
> > +}
> 
> 		Amit
> 

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 4378 bytes --]

On 02/09/15 09:48, David Gibson wrote:
> On Wed, Sep 02, 2015 at 11:04:12AM +0530, Amit Shah wrote:
>> On (Mon) 31 Aug 2015 [20:46:02], 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.
>>
>> virtio-rng, not rnd.

Oh, sorry, I'll fix the description.

>> Can you elaborate what you mean by 'guests that do not support
>> virtio-rng yet'?  The Linux kernel has had the virtio-rng driver since
>> 2.6.26, so I'm assuming that's not the thing you're alluding to.
>>
>> Not saying this hypercall isn't a good idea, just asking why.  I think
>> there's are valid reasons like the driver fails to load, or the driver
>> is compiled out, or simply is loaded too late in the boot cycle.
> 
> Yeah, I think we'd be talking about guests that just don't have it
> configured, although I suppose it's possible someone out there is
> using something earlier than 2.6.26 as well.  Note that H_RANDOM has
> been supported under PowerVM for a long time, and PowerVM doesn't have
> any virtio support.  So it is plausible that there are guests out
> there with with H_RANDOM support but no virtio-rng support, although I
> don't know of any examples specifically.  RHEL6 had virtio support,
> including virtio-rng more or less by accident (since it was only
> supported under PowerVM).  SLES may not have made the same fortunate
> error - I don't have a system handy to check.

Right, thanks David, I couldn't have explained it better.

>>> 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 ...
>>>
>>> to use the /dev/random backend, or "h-random=rng-egd" to use the
>>> Entropy Gathering Daemon instead.
>>
>> I was going to suggest using -object here, but already I see you and
>> David have reached an agreement for that.
>>
>> Out of curiosity: what does the host kernel use for its source when
>> going the hypercall route?
> 
> I believe it draws from the same entropy pool as /dev/random.

The H_RANDOM handler in the kernel uses powernv_get_random_real_mode()
in arch/powerpc/platforms/powernv/rng.c ... that seems to be a
powernv-only pool (but it is also used to feed the normal kernel entropy
pool, I think), but I am not an expert here so I might be wrong.

>>> +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);
>>> +        hrcrdp->received += size;
>>> +    }
>>> +    qemu_sem_post(&hrcrdp->sem);
>>> +}
>>> +
>>> +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);
>>> +    }
>>
>> Is it possible for a second hypercall to arrive while the first is
>> waiting for the backend to provide data?
> 
> Yes it is.  The hypercall itself is synchronous, but you could get
> concurrent calls from different guest CPUs.  Hence the need for
> iothread unlocking.

BQL and semaphore handling should be ok, I think, but one remaining
question is: Can the RngBackend deal with multiple requests in flight
from different vCPUs? Or is it limited to one consumer only? Amit, do
you know this?

 Thomas



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Amit Shah]

On (Wed) 02 Sep 2015 [17:48:01], David Gibson wrote:
> On Wed, Sep 02, 2015 at 11:04:12AM +0530, Amit Shah wrote:
> > On (Mon) 31 Aug 2015 [20:46:02], 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.
> > 
> > virtio-rng, not rnd.
> > 
> > Can you elaborate what you mean by 'guests that do not support
> > virtio-rng yet'?  The Linux kernel has had the virtio-rng driver since
> > 2.6.26, so I'm assuming that's not the thing you're alluding to.
> > 
> > Not saying this hypercall isn't a good idea, just asking why.  I think
> > there's are valid reasons like the driver fails to load, or the driver
> > is compiled out, or simply is loaded too late in the boot cycle.
> 
> Yeah, I think we'd be talking about guests that just don't have it
> configured, although I suppose it's possible someone out there is
> using something earlier than 2.6.26 as well.  Note that H_RANDOM has
> been supported under PowerVM for a long time, and PowerVM doesn't have
> any virtio support.  So it is plausible that there are guests out
> there with with H_RANDOM support but no virtio-rng support, although I
> don't know of any examples specifically.  RHEL6 had virtio support,
> including virtio-rng more or less by accident (since it was only
> supported under PowerVM).  SLES may not have made the same fortunate
> error - I don't have a system handy to check.

RHEL6 also used 2.6.32, which means it inherited from upstream.  But
you're right that x86 didn't have a device for virtio-rng then.

> > > 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 ...
> > > 
> > > to use the /dev/random backend, or "h-random=rng-egd" to use the
> > > Entropy Gathering Daemon instead.
> > 
> > I was going to suggest using -object here, but already I see you and
> > David have reached an agreement for that.
> > 
> > Out of curiosity: what does the host kernel use for its source when
> > going the hypercall route?
> 
> I believe it draws from the same entropy pool as /dev/random.

OK - I'll take a look there as well.

> > > +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);
> > > +        hrcrdp->received += size;
> > > +    }
> > > +    qemu_sem_post(&hrcrdp->sem);
> > > +}
> > > +
> > > +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);
> > > +    }
> > 
> > Is it possible for a second hypercall to arrive while the first is
> > waiting for the backend to provide data?
> 
> Yes it is.  The hypercall itself is synchronous, but you could get
> concurrent calls from different guest CPUs.  Hence the need for
> iothread unlocking.

OK, thanks!


		Amit

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

[Amit Shah]

On (Wed) 02 Sep 2015 [10:58:57], Thomas Huth wrote:
> On 02/09/15 09:48, David Gibson wrote:
> > On Wed, Sep 02, 2015 at 11:04:12AM +0530, Amit Shah wrote:
> >> On (Mon) 31 Aug 2015 [20:46:02], 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.
> >>
> >> virtio-rng, not rnd.
> 
> Oh, sorry, I'll fix the description.

Thanks.  (It's that way in patch 0 too.)

> >> Can you elaborate what you mean by 'guests that do not support
> >> virtio-rng yet'?  The Linux kernel has had the virtio-rng driver since
> >> 2.6.26, so I'm assuming that's not the thing you're alluding to.
> >>
> >> Not saying this hypercall isn't a good idea, just asking why.  I think
> >> there's are valid reasons like the driver fails to load, or the driver
> >> is compiled out, or simply is loaded too late in the boot cycle.
> > 
> > Yeah, I think we'd be talking about guests that just don't have it
> > configured, although I suppose it's possible someone out there is
> > using something earlier than 2.6.26 as well.  Note that H_RANDOM has
> > been supported under PowerVM for a long time, and PowerVM doesn't have
> > any virtio support.  So it is plausible that there are guests out
> > there with with H_RANDOM support but no virtio-rng support, although I
> > don't know of any examples specifically.  RHEL6 had virtio support,
> > including virtio-rng more or less by accident (since it was only
> > supported under PowerVM).  SLES may not have made the same fortunate
> > error - I don't have a system handy to check.
> 
> Right, thanks David, I couldn't have explained it better.
> 
> >>> 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 ...
> >>>
> >>> to use the /dev/random backend, or "h-random=rng-egd" to use the
> >>> Entropy Gathering Daemon instead.
> >>
> >> I was going to suggest using -object here, but already I see you and
> >> David have reached an agreement for that.
> >>
> >> Out of curiosity: what does the host kernel use for its source when
> >> going the hypercall route?
> > 
> > I believe it draws from the same entropy pool as /dev/random.
> 
> The H_RANDOM handler in the kernel uses powernv_get_random_real_mode()
> in arch/powerpc/platforms/powernv/rng.c ... that seems to be a
> powernv-only pool (but it is also used to feed the normal kernel entropy
> pool, I think), but I am not an expert here so I might be wrong.

Thanks for the pointer, I'm going to take a look.

> >>> +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);
> >>> +        hrcrdp->received += size;
> >>> +    }
> >>> +    qemu_sem_post(&hrcrdp->sem);
> >>> +}
> >>> +
> >>> +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);
> >>> +    }
> >>
> >> Is it possible for a second hypercall to arrive while the first is
> >> waiting for the backend to provide data?
> > 
> > Yes it is.  The hypercall itself is synchronous, but you could get
> > concurrent calls from different guest CPUs.  Hence the need for
> > iothread unlocking.
> 
> BQL and semaphore handling should be ok, I think, but one remaining
> question is: Can the RngBackend deal with multiple requests in flight
> from different vCPUs? Or is it limited to one consumer only? Amit, do
> you know this?

It's not limited by one consumer, it should work fine for the way
you're using it.  For virtio-rng, though, I've had this feeling for a
while that it won't do the right thing (ie it will source more bytes
than asked for), which bothers me.  One of the things I want to look
at later..


		Amit

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

[Michael Ellerman]

On Wed, 2015-09-02 at 17:48 +1000, David Gibson wrote:
> On Wed, Sep 02, 2015 at 11:04:12AM +0530, Amit Shah wrote:
> > On (Mon) 31 Aug 2015 [20:46:02], 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.
> > 
> > virtio-rng, not rnd.
> > 
> > Can you elaborate what you mean by 'guests that do not support
> > virtio-rng yet'?  The Linux kernel has had the virtio-rng driver since
> > 2.6.26, so I'm assuming that's not the thing you're alluding to.
> > 
> > Not saying this hypercall isn't a good idea, just asking why.  I think
> > there's are valid reasons like the driver fails to load, or the driver
> > is compiled out, or simply is loaded too late in the boot cycle.
> 
> Yeah, I think we'd be talking about guests that just don't have it
> configured, although I suppose it's possible someone out there is
> using something earlier than 2.6.26 as well.  Note that H_RANDOM has
> been supported under PowerVM for a long time, and PowerVM doesn't have
> any virtio support.  So it is plausible that there are guests out
> there with with H_RANDOM support but no virtio-rng support, although I
> don't know of any examples specifically.  RHEL6 had virtio support,
> including virtio-rng more or less by accident (since it was only
> supported under PowerVM).  SLES may not have made the same fortunate
> error - I don't have a system handy to check.

There also could be folks who want to run non-Linux operating systems, which
don't have a virtio-rng driver, crazy I know :)

cheers

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2138 bytes --]

On Thu, Sep 03, 2015 at 11:21:24AM +1000, Michael Ellerman wrote:
> On Wed, 2015-09-02 at 17:48 +1000, David Gibson wrote:
> > On Wed, Sep 02, 2015 at 11:04:12AM +0530, Amit Shah wrote:
> > > On (Mon) 31 Aug 2015 [20:46:02], 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.
> > > 
> > > virtio-rng, not rnd.
> > > 
> > > Can you elaborate what you mean by 'guests that do not support
> > > virtio-rng yet'?  The Linux kernel has had the virtio-rng driver since
> > > 2.6.26, so I'm assuming that's not the thing you're alluding to.
> > > 
> > > Not saying this hypercall isn't a good idea, just asking why.  I think
> > > there's are valid reasons like the driver fails to load, or the driver
> > > is compiled out, or simply is loaded too late in the boot cycle.
> > 
> > Yeah, I think we'd be talking about guests that just don't have it
> > configured, although I suppose it's possible someone out there is
> > using something earlier than 2.6.26 as well.  Note that H_RANDOM has
> > been supported under PowerVM for a long time, and PowerVM doesn't have
> > any virtio support.  So it is plausible that there are guests out
> > there with with H_RANDOM support but no virtio-rng support, although I
> > don't know of any examples specifically.  RHEL6 had virtio support,
> > including virtio-rng more or less by accident (since it was only
> > supported under PowerVM).  SLES may not have made the same fortunate
> > error - I don't have a system handy to check.
> 
> There also could be folks who want to run non-Linux operating systems, which
> don't have a virtio-rng driver, crazy I know :)

Well, yes.  Although I don't have any concrete examples of those, either..

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 3645 bytes --]

On 01/09/15 02:47, David Gibson wrote:
> 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 ...
...
>> 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?

I tried that now, but calling rng_backend_request_entropy() from within
the callback function does not work (since entropy_available() in
rng-random.c clears the callback function variable after having called
the callback).

And since you normally seem get 8 bytes in the first shot already anyway
when using a good random number generator source, I think it's best to
simply keep the logic as I've currently got it - at least that's easiest
to understand when reading the source code.

>> +}
>> +
>> +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;
>> +}

I'll post a new version with the other changes soon.

 Thomas



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4352 bytes --]

On Mon, Sep 07, 2015 at 05:05:48PM +0200, Thomas Huth wrote:
> On 01/09/15 02:47, David Gibson wrote:
> > 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 ...
> ...
> >> 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?
> 
> I tried that now, but calling rng_backend_request_entropy() from within
> the callback function does not work (since entropy_available() in
> rng-random.c clears the callback function variable after having called
> the callback).

Ah, ok.  I'd missed the fact that rng_backend_request_entropy() was
being called again - I'd assumed that after the first call the backend
would just keep notifying until you had as much data as requested.

> And since you normally seem get 8 bytes in the first shot already anyway
> when using a good random number generator source, I think it's best to
> simply keep the logic as I've currently got it - at least that's easiest
> to understand when reading the source code.

Yes, I concur.

> 
> >> +}
> >> +
> >> +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;
> >> +}
> 
> I'll post a new version with the other changes soon.
> 
>  Thomas
> 
> 



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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[Sam Bobroff]

On Tue, Sep 01, 2015 at 12:53:26PM +0200, Thomas Huth wrote:
> On 01/09/15 02:38, David Gibson wrote:
> > On Mon, Aug 31, 2015 at 08:46:01PM +0200, Thomas Huth wrote:
> >> From: Michael Ellerman <michael@ellerman.id.au>
> >>
> >> Some powerpc systems have support for a hardware random number generator
> >> (hwrng). If such a hwrng is present the host kernel can provide access
> >> to it via the H_RANDOM hcall.
> >>
> >> The kernel advertises the presence of a hwrng with the KVM_CAP_PPC_HWRNG
> >> capability. If this is detected we add the appropriate device tree bits
> >> to advertise the presence of the hwrng to the guest kernel.
> >>
> >> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> >> [thuth: Refreshed patch so it applies to QEMU master branch]
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> > 
> > So, I'm confused by one thing.
> > 
> > I thought new kernel handled hcalls were supposed to be disabled by
> > default, but I don't see any calls to kvmppc_enable_hcall() to turn on
> > H_RANDOM.
> 
> Michael's patch was from 2013, the kvmppc_enable_hcall() stuff seems to
> be from 2014 ... so the enablement is likely missing in this patch,
> indeed. I didn't test the in-kernel hypercall yet, just my QEMU
> implementation so far, that's why I did not notice this yet.
> 
> Michael, do you want to rework your patch? Or shall I add an additional
> enablement patch to my queue?

If I recall correctly, it's specifically not enabled: there was quite a lot of
discussion about it when Michael posted the patches and I think the consensus
was that it should only be enabled by QEMU, and only if the user could decide
if it was used or not.

What if we set up another backend that just enables the hcall in KVM?

This would answer the question about what to do if both are available as well:
just do only what the user has asked for. (But I'm not sure what the default
behaviour should be if the user doesn't specify anything.)

(Oh also: I have tested the in-kernel hcall using a QEMU modified to enable it
and it seems to work :-)

Cheers,
Sam.

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2927 bytes --]

On Tue, Sep 08, 2015 at 03:03:16PM +1000, Sam Bobroff wrote:
> On Tue, Sep 01, 2015 at 12:53:26PM +0200, Thomas Huth wrote:
> > On 01/09/15 02:38, David Gibson wrote:
> > > On Mon, Aug 31, 2015 at 08:46:01PM +0200, Thomas Huth wrote:
> > >> From: Michael Ellerman <michael@ellerman.id.au>
> > >>
> > >> Some powerpc systems have support for a hardware random number generator
> > >> (hwrng). If such a hwrng is present the host kernel can provide access
> > >> to it via the H_RANDOM hcall.
> > >>
> > >> The kernel advertises the presence of a hwrng with the KVM_CAP_PPC_HWRNG
> > >> capability. If this is detected we add the appropriate device tree bits
> > >> to advertise the presence of the hwrng to the guest kernel.
> > >>
> > >> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> > >> [thuth: Refreshed patch so it applies to QEMU master branch]
> > >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > 
> > > So, I'm confused by one thing.
> > > 
> > > I thought new kernel handled hcalls were supposed to be disabled by
> > > default, but I don't see any calls to kvmppc_enable_hcall() to turn on
> > > H_RANDOM.
> > 
> > Michael's patch was from 2013, the kvmppc_enable_hcall() stuff seems to
> > be from 2014 ... so the enablement is likely missing in this patch,
> > indeed. I didn't test the in-kernel hypercall yet, just my QEMU
> > implementation so far, that's why I did not notice this yet.
> > 
> > Michael, do you want to rework your patch? Or shall I add an additional
> > enablement patch to my queue?
> 
> If I recall correctly, it's specifically not enabled: there was quite a lot of
> discussion about it when Michael posted the patches and I think the consensus
> was that it should only be enabled by QEMU, and only if the user could decide
> if it was used or not.
> 
> What if we set up another backend that just enables the hcall in KVM?

I think that's basically the right approach.

It can't quite be a "backend" as such, since the in-kernel hcall can
only supply H_RANDOM; it can't supply random for other purposes like
virtio-rng, which the general qemu rng backends can.

So I'd suggest two options controlling H_RANDOM:
	usekvm : boolean  [default true]
		Whether to enable the in-kernel implementation if
		available
	backend : ref to rng backend object [no default]
		Backend to use if in-kernel implementation is
		unavailable or disabled.

At this point rather than just implementing them as discrete machine
options, I suspect it will be more maintainable to split out the
h-random implementation as a pseudo-device with its own qdev and so
forth.  We already do similarly for the RTAS time of day functions
(spapr-rtc).

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[Thomas Huth]

On 08/09/15 07:03, Sam Bobroff wrote:
> On Tue, Sep 01, 2015 at 12:53:26PM +0200, Thomas Huth wrote:
>> On 01/09/15 02:38, David Gibson wrote:
>>> On Mon, Aug 31, 2015 at 08:46:01PM +0200, Thomas Huth wrote:
>>>> From: Michael Ellerman <michael@ellerman.id.au>
>>>>
>>>> Some powerpc systems have support for a hardware random number generator
>>>> (hwrng). If such a hwrng is present the host kernel can provide access
>>>> to it via the H_RANDOM hcall.
>>>>
>>>> The kernel advertises the presence of a hwrng with the KVM_CAP_PPC_HWRNG
>>>> capability. If this is detected we add the appropriate device tree bits
>>>> to advertise the presence of the hwrng to the guest kernel.
>>>>
>>>> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
>>>> [thuth: Refreshed patch so it applies to QEMU master branch]
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>
>>> So, I'm confused by one thing.
>>>
>>> I thought new kernel handled hcalls were supposed to be disabled by
>>> default, but I don't see any calls to kvmppc_enable_hcall() to turn on
>>> H_RANDOM.
>>
>> Michael's patch was from 2013, the kvmppc_enable_hcall() stuff seems to
>> be from 2014 ... so the enablement is likely missing in this patch,
>> indeed. I didn't test the in-kernel hypercall yet, just my QEMU
>> implementation so far, that's why I did not notice this yet.
>>
>> Michael, do you want to rework your patch? Or shall I add an additional
>> enablement patch to my queue?
> 
> If I recall correctly, it's specifically not enabled: there was quite a lot of
> discussion about it when Michael posted the patches and I think the consensus
> was that it should only be enabled by QEMU, and only if the user could decide
> if it was used or not.

Can you find this discussion in a mailing list archive somewhere? The
only discussions I've found are basically these:

http://lists.nongnu.org/archive/html/qemu-devel/2013-09/msg04233.html
https://lkml.org/lkml/fancy/2013/10/1/49

... and there it was only discussed that the call should be implemented
in QEMU, too. I did not spot any discussion about making it switchable
for the user?

 Thomas

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[Sam Bobroff]

On Tue, Sep 08, 2015 at 07:38:12AM +0200, Thomas Huth wrote:
> On 08/09/15 07:03, Sam Bobroff wrote:
> > On Tue, Sep 01, 2015 at 12:53:26PM +0200, Thomas Huth wrote:
> >> On 01/09/15 02:38, David Gibson wrote:
> >>> On Mon, Aug 31, 2015 at 08:46:01PM +0200, Thomas Huth wrote:
> >>>> From: Michael Ellerman <michael@ellerman.id.au>
> >>>>
> >>>> Some powerpc systems have support for a hardware random number generator
> >>>> (hwrng). If such a hwrng is present the host kernel can provide access
> >>>> to it via the H_RANDOM hcall.
> >>>>
> >>>> The kernel advertises the presence of a hwrng with the KVM_CAP_PPC_HWRNG
> >>>> capability. If this is detected we add the appropriate device tree bits
> >>>> to advertise the presence of the hwrng to the guest kernel.
> >>>>
> >>>> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> >>>> [thuth: Refreshed patch so it applies to QEMU master branch]
> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >>>
> >>> So, I'm confused by one thing.
> >>>
> >>> I thought new kernel handled hcalls were supposed to be disabled by
> >>> default, but I don't see any calls to kvmppc_enable_hcall() to turn on
> >>> H_RANDOM.
> >>
> >> Michael's patch was from 2013, the kvmppc_enable_hcall() stuff seems to
> >> be from 2014 ... so the enablement is likely missing in this patch,
> >> indeed. I didn't test the in-kernel hypercall yet, just my QEMU
> >> implementation so far, that's why I did not notice this yet.
> >>
> >> Michael, do you want to rework your patch? Or shall I add an additional
> >> enablement patch to my queue?
> > 
> > If I recall correctly, it's specifically not enabled: there was quite a lot of
> > discussion about it when Michael posted the patches and I think the consensus
> > was that it should only be enabled by QEMU, and only if the user could decide
> > if it was used or not.
> 
> Can you find this discussion in a mailing list archive somewhere? The
> only discussions I've found are basically these:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2013-09/msg04233.html
> https://lkml.org/lkml/fancy/2013/10/1/49
> 
> ... and there it was only discussed that the call should be implemented
> in QEMU, too. I did not spot any discussion about making it switchable
> for the user?

Sorry that part wasn't very well worded: I was trying to say that QEMU's
configuration should be able to choose how H_RANDOM handled, rather than having
it automatically choose based on what was available in KVM and that's what
we're considering now anyway. (See David's comment elsewhere in this thread.)

Sam.

Re: [Qemu-devel] [PATCH v2 1/2] spapr: Add support for hwrng when available

[Greg Kurz]

On Mon, 31 Aug 2015 20:46:01 +0200
Thomas Huth <thuth@redhat.com> wrote:

> From: Michael Ellerman <michael@ellerman.id.au>
> 
> Some powerpc systems have support for a hardware random number generator
> (hwrng). If such a hwrng is present the host kernel can provide access
> to it via the H_RANDOM hcall.
> 
> The kernel advertises the presence of a hwrng with the KVM_CAP_PPC_HWRNG
> capability. If this is detected we add the appropriate device tree bits
> to advertise the presence of the hwrng to the guest kernel.
> 
> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> [thuth: Refreshed patch so it applies to QEMU master branch]
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/ppc/spapr.c         | 16 ++++++++++++++++
>  include/hw/ppc/spapr.h |  1 +
>  target-ppc/kvm.c       |  5 +++++
>  target-ppc/kvm_ppc.h   |  5 +++++
>  4 files changed, 27 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index bf0c64f..bc3a112 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -466,6 +466,22 @@ static void *spapr_create_fdt_skel(hwaddr initrd_base,
> 
>      _FDT((fdt_end_node(fdt)));
> 
> +    if (kvmppc_hwrng_present()) {
> +        _FDT(fdt_begin_node(fdt, "ibm,platform-facilities"));
> +
> +        _FDT(fdt_property_string(fdt, "name", "ibm,platform-facilities"));
> +        _FDT(fdt_property_string(fdt, "device_type",
> +                                 "ibm,platform-facilities"));
> +        _FDT(fdt_property_cell(fdt, "#address-cells", 0x1));
> +        _FDT(fdt_property_cell(fdt, "#size-cells", 0x0));
> +        _FDT(fdt_begin_node(fdt, "ibm,random-v1"));
> +        _FDT(fdt_property_string(fdt, "name", "ibm,random-v1"));
> +        _FDT(fdt_property_string(fdt, "compatible", "ibm,random"));
> +        _FDT((fdt_end_node(fdt)));
> +    }
> +
> +    _FDT((fdt_end_node(fdt)));
> +
>      /* event-sources */
>      spapr_events_fdt_skel(fdt, epow_irq);
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 91a61ab..ab8906f 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
> 

H_RANDOM isn't used in this patch, I believe the above hunk belongs to
patch 2/2 actually.

Cheers.

--
Greg

> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 110436d..7317f8f 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -2484,3 +2484,8 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>  {
>      return data & 0xffff;
>  }
> +
> +bool kvmppc_hwrng_present(void)
> +{
> +    return kvm_enabled() && kvm_check_extension(kvm_state, KVM_CAP_PPC_HWRNG);
> +}
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index 4d30e27..62ff601 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);
> 
>  #else
> 
> @@ -248,6 +249,10 @@ static inline bool kvmppc_has_cap_fixup_hcalls(void)
>      abort();
>  }
> 
> +static inline bool kvmppc_hwrng_present(void)
> +{
> +    return false;
> +}
>  #endif
> 
>  #ifndef CONFIG_KVM

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 2294 bytes --]

On 08/09/15 07:15, David Gibson wrote:
> On Tue, Sep 08, 2015 at 03:03:16PM +1000, Sam Bobroff wrote:
>> On Tue, Sep 01, 2015 at 12:53:26PM +0200, Thomas Huth wrote:
>>> On 01/09/15 02:38, David Gibson wrote:
>>>> On Mon, Aug 31, 2015 at 08:46:01PM +0200, Thomas Huth wrote:
>>>>> From: Michael Ellerman <michael@ellerman.id.au>
>>>>>
>>>>> Some powerpc systems have support for a hardware random number generator
>>>>> (hwrng). If such a hwrng is present the host kernel can provide access
>>>>> to it via the H_RANDOM hcall.
...
>> What if we set up another backend that just enables the hcall in KVM?
> 
> I think that's basically the right approach.
> 
> It can't quite be a "backend" as such, since the in-kernel hcall can
> only supply H_RANDOM; it can't supply random for other purposes like
> virtio-rng, which the general qemu rng backends can.
> 
> So I'd suggest two options controlling H_RANDOM:
> 	usekvm : boolean  [default true]
> 		Whether to enable the in-kernel implementation if
> 		available
> 	backend : ref to rng backend object [no default]
> 		Backend to use if in-kernel implementation is
> 		unavailable or disabled.
> 
> At this point rather than just implementing them as discrete machine
> options, I suspect it will be more maintainable to split out the
> h-random implementation as a pseudo-device with its own qdev and so
> forth.  We already do similarly for the RTAS time of day functions
> (spapr-rtc).

I gave that I try, but it does not work as expected. To be able to
specify the options, I'd need to instantiate this device with the
"-device" option, right? Something like:

	-device spapr-rng,backend=rng0,usekvm=0

Now this does not work when I use TYPE_SYS_BUS_DEVICE as parent class
like it is done for spapr-rtc, since the user apparently can not plug
device to this bus on machine spapr (you can also not plug an spapr-rtc
device this way!).

The spapr-vlan, spapr-vty, etc. devices are TYPE_VIO_SPAPR_DEVICE, so I
also tried that instead, but then the rng device suddenly shows up under
/vdevice in the device tree - that's also not what we want, I guess.

So I am currently not sure whether this is the right approach. Any
recommendations? Or shall I stick with the machine option?

 Thomas



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 1929 bytes --]

On 09/09/15 23:10, Thomas Huth wrote:
> On 08/09/15 07:15, David Gibson wrote:
...
>> At this point rather than just implementing them as discrete machine
>> options, I suspect it will be more maintainable to split out the
>> h-random implementation as a pseudo-device with its own qdev and so
>> forth.  We already do similarly for the RTAS time of day functions
>> (spapr-rtc).
> 
> I gave that I try, but it does not work as expected. To be able to
> specify the options, I'd need to instantiate this device with the
> "-device" option, right? Something like:
> 
> 	-device spapr-rng,backend=rng0,usekvm=0
> 
> Now this does not work when I use TYPE_SYS_BUS_DEVICE as parent class
> like it is done for spapr-rtc, since the user apparently can not plug
> device to this bus on machine spapr (you can also not plug an spapr-rtc
> device this way!).
> 
> The spapr-vlan, spapr-vty, etc. devices are TYPE_VIO_SPAPR_DEVICE, so I
> also tried that instead, but then the rng device suddenly shows up under
> /vdevice in the device tree - that's also not what we want, I guess.

I did some more tests, and I think I can get this working with one small
modification to spapr_vio.c:

diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index c51eb8e..8e7f6b4 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -99,6 +99,14 @@ static int vio_make_devnode(VIOsPAPRDevice *dev,
     int vdevice_off, node_off, ret;
     char *dt_name;

+    if (!pc->dt_name) {
+        ret = 0;
+        if (pc->devnode) {
+            ret = (pc->devnode)(dev, fdt, -1);
+        }
+        return ret;
+    }
+
     vdevice_off = fdt_path_offset(fdt, "/vdevice");
     if (vdevice_off < 0) {
         return vdevice_off;

i.e. when the dt_name has not been set, the device won't be added to the
/vdevice device tree node. If that's acceptable, I'll continue with this
approach.

 Thomas



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2462 bytes --]

On Thu, Sep 10, 2015 at 09:33:21AM +0200, Thomas Huth wrote:
> On 09/09/15 23:10, Thomas Huth wrote:
> > On 08/09/15 07:15, David Gibson wrote:
> ...
> >> At this point rather than just implementing them as discrete machine
> >> options, I suspect it will be more maintainable to split out the
> >> h-random implementation as a pseudo-device with its own qdev and so
> >> forth.  We already do similarly for the RTAS time of day functions
> >> (spapr-rtc).
> > 
> > I gave that I try, but it does not work as expected. To be able to
> > specify the options, I'd need to instantiate this device with the
> > "-device" option, right? Something like:
> > 
> > 	-device spapr-rng,backend=rng0,usekvm=0
> > 
> > Now this does not work when I use TYPE_SYS_BUS_DEVICE as parent class
> > like it is done for spapr-rtc, since the user apparently can not plug
> > device to this bus on machine spapr (you can also not plug an spapr-rtc
> > device this way!).
> > 
> > The spapr-vlan, spapr-vty, etc. devices are TYPE_VIO_SPAPR_DEVICE, so I
> > also tried that instead, but then the rng device suddenly shows up under
> > /vdevice in the device tree - that's also not what we want, I guess.
> 
> I did some more tests, and I think I can get this working with one small
> modification to spapr_vio.c:
> 
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index c51eb8e..8e7f6b4 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -99,6 +99,14 @@ static int vio_make_devnode(VIOsPAPRDevice *dev,
>      int vdevice_off, node_off, ret;
>      char *dt_name;
> 
> +    if (!pc->dt_name) {
> +        ret = 0;
> +        if (pc->devnode) {
> +            ret = (pc->devnode)(dev, fdt, -1);
> +        }
> +        return ret;
> +    }
> +
>      vdevice_off = fdt_path_offset(fdt, "/vdevice");
>      if (vdevice_off < 0) {
>          return vdevice_off;
> 
> i.e. when the dt_name has not been set, the device won't be added to the
> /vdevice device tree node. If that's acceptable, I'll continue with this
> approach.

A bit hacky.

I think it would be preferable to build it under SysBus by default,
like spapr-rtc.  Properties can be set on the device using -global (or
-set, but -global is easier).

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 2421 bytes --]

On 10/09/15 12:40, David Gibson wrote:
> On Thu, Sep 10, 2015 at 09:33:21AM +0200, Thomas Huth wrote:
>> On 09/09/15 23:10, Thomas Huth wrote:
>>> On 08/09/15 07:15, David Gibson wrote:
>> ...
>>>> At this point rather than just implementing them as discrete machine
>>>> options, I suspect it will be more maintainable to split out the
>>>> h-random implementation as a pseudo-device with its own qdev and so
>>>> forth.  We already do similarly for the RTAS time of day functions
>>>> (spapr-rtc).
>>>
>>> I gave that I try, but it does not work as expected. To be able to
>>> specify the options, I'd need to instantiate this device with the
>>> "-device" option, right? Something like:
>>>
>>> 	-device spapr-rng,backend=rng0,usekvm=0
>>>
>>> Now this does not work when I use TYPE_SYS_BUS_DEVICE as parent class
>>> like it is done for spapr-rtc, since the user apparently can not plug
>>> device to this bus on machine spapr (you can also not plug an spapr-rtc
>>> device this way!).
>>>
>>> The spapr-vlan, spapr-vty, etc. devices are TYPE_VIO_SPAPR_DEVICE, so I
>>> also tried that instead, but then the rng device suddenly shows up under
>>> /vdevice in the device tree - that's also not what we want, I guess.
>>
>> I did some more tests, and I think I can get this working with one small
>> modification to spapr_vio.c
...
>> i.e. when the dt_name has not been set, the device won't be added to the
>> /vdevice device tree node. If that's acceptable, I'll continue with this
>> approach.
> 
> A bit hacky.
> 
> I think it would be preferable to build it under SysBus by default,
> like spapr-rtc.  Properties can be set on the device using -global (or
> -set, but -global is easier).

If anyhow possible, I'd prefere to use "-device" for this instead, because

a) it's easier to use for the user, for example you can simply use
   "-device spapr-rng,?" to get the list of properties - this
   does not seem to work with spapr-rtc (it has a "date" property
   which does not show up in the help text?)

b) unlike the rtc device which is always instantiated, the rng
   device is rather optional, so it is IMHO more intuitive if
   created via the -device option.

So I'd like to give it a try with the TYPE_VIO_SPAPR_DEVICE first ... if
you then still don't like the patches at all, I can still rework them to
use TYPE_SYS_BUS_DEVICE instead.

 Thomas



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[Greg Kurz]

On Wed, 9 Sep 2015 10:54:20 +1000
Sam Bobroff <sam.bobroff@au1.ibm.com> wrote:

> On Tue, Sep 08, 2015 at 07:38:12AM +0200, Thomas Huth wrote:
> > On 08/09/15 07:03, Sam Bobroff wrote:
> > > On Tue, Sep 01, 2015 at 12:53:26PM +0200, Thomas Huth wrote:
> > >> On 01/09/15 02:38, David Gibson wrote:
> > >>> On Mon, Aug 31, 2015 at 08:46:01PM +0200, Thomas Huth wrote:
> > >>>> From: Michael Ellerman <michael@ellerman.id.au>
> > >>>>
> > >>>> Some powerpc systems have support for a hardware random number generator
> > >>>> (hwrng). If such a hwrng is present the host kernel can provide access
> > >>>> to it via the H_RANDOM hcall.
> > >>>>
> > >>>> The kernel advertises the presence of a hwrng with the KVM_CAP_PPC_HWRNG
> > >>>> capability. If this is detected we add the appropriate device tree bits
> > >>>> to advertise the presence of the hwrng to the guest kernel.
> > >>>>
> > >>>> Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
> > >>>> [thuth: Refreshed patch so it applies to QEMU master branch]
> > >>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> > >>>
> > >>> So, I'm confused by one thing.
> > >>>
> > >>> I thought new kernel handled hcalls were supposed to be disabled by
> > >>> default, but I don't see any calls to kvmppc_enable_hcall() to turn on
> > >>> H_RANDOM.
> > >>
> > >> Michael's patch was from 2013, the kvmppc_enable_hcall() stuff seems to
> > >> be from 2014 ... so the enablement is likely missing in this patch,
> > >> indeed. I didn't test the in-kernel hypercall yet, just my QEMU
> > >> implementation so far, that's why I did not notice this yet.
> > >>
> > >> Michael, do you want to rework your patch? Or shall I add an additional
> > >> enablement patch to my queue?
> > > 
> > > If I recall correctly, it's specifically not enabled: there was quite a lot of
> > > discussion about it when Michael posted the patches and I think the consensus
> > > was that it should only be enabled by QEMU, and only if the user could decide
> > > if it was used or not.
> > 
> > Can you find this discussion in a mailing list archive somewhere? The
> > only discussions I've found are basically these:
> > 
> > http://lists.nongnu.org/archive/html/qemu-devel/2013-09/msg04233.html
> > https://lkml.org/lkml/fancy/2013/10/1/49
> > 
> > ... and there it was only discussed that the call should be implemented
> > in QEMU, too. I did not spot any discussion about making it switchable
> > for the user?
> 
> Sorry that part wasn't very well worded: I was trying to say that QEMU's
> configuration should be able to choose how H_RANDOM handled, rather than having
> it automatically choose based on what was available in KVM and that's what
> we're considering now anyway. (See David's comment elsewhere in this thread.)
> 
> Sam.
> 
> 

In this case, this patch isn't appropriate anymore: QEMU would advertise support
for "ibm,random-v1" but guests would be returned H_FUNCTION. The DT bits should
be in a later patch, after H_RANDOM is plugged in (either KVM or QEMU).

Cheers.

--
Greg

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[Alexander Graf]

> Am 10.09.2015 um 14:03 schrieb Thomas Huth <thuth@redhat.com>:
> 
>> On 10/09/15 12:40, David Gibson wrote:
>>> On Thu, Sep 10, 2015 at 09:33:21AM +0200, Thomas Huth wrote:
>>>> On 09/09/15 23:10, Thomas Huth wrote:
>>>> On 08/09/15 07:15, David Gibson wrote:
>>> ...
>>>>> At this point rather than just implementing them as discrete machine
>>>>> options, I suspect it will be more maintainable to split out the
>>>>> h-random implementation as a pseudo-device with its own qdev and so
>>>>> forth.  We already do similarly for the RTAS time of day functions
>>>>> (spapr-rtc).
>>>> 
>>>> I gave that I try, but it does not work as expected. To be able to
>>>> specify the options, I'd need to instantiate this device with the
>>>> "-device" option, right? Something like:
>>>> 
>>>>    -device spapr-rng,backend=rng0,usekvm=0
>>>> 
>>>> Now this does not work when I use TYPE_SYS_BUS_DEVICE as parent class
>>>> like it is done for spapr-rtc, since the user apparently can not plug
>>>> device to this bus on machine spapr (you can also not plug an spapr-rtc
>>>> device this way!).
>>>> 
>>>> The spapr-vlan, spapr-vty, etc. devices are TYPE_VIO_SPAPR_DEVICE, so I
>>>> also tried that instead, but then the rng device suddenly shows up under
>>>> /vdevice in the device tree - that's also not what we want, I guess.
>>> 
>>> I did some more tests, and I think I can get this working with one small
>>> modification to spapr_vio.c
> ...
>>> i.e. when the dt_name has not been set, the device won't be added to the
>>> /vdevice device tree node. If that's acceptable, I'll continue with this
>>> approach.
>> 
>> A bit hacky.
>> 
>> I think it would be preferable to build it under SysBus by default,
>> like spapr-rtc.  Properties can be set on the device using -global (or
>> -set, but -global is easier).
> 
> If anyhow possible, I'd prefere to use "-device" for this instead, because
> 
> a) it's easier to use for the user, for example you can simply use
>   "-device spapr-rng,?" to get the list of properties - this
>   does not seem to work with spapr-rtc (it has a "date" property
>   which does not show up in the help text?)
> 
> b) unlike the rtc device which is always instantiated, the rng
>   device is rather optional, so it is IMHO more intuitive if
>   created via the -device option.
> 
> So I'd like to give it a try with the TYPE_VIO_SPAPR_DEVICE first ... if
> you then still don't like the patches at all, I can still rework them to
> use TYPE_SYS_BUS_DEVICE instead.

Please don't use sysbus. If the vio device approach turns ugly, create a new spapr hcall bus instead. We should have had that from the beginning really.


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3521 bytes --]

On Thu, Sep 10, 2015 at 02:03:39PM +0200, Thomas Huth wrote:
> On 10/09/15 12:40, David Gibson wrote:
> > On Thu, Sep 10, 2015 at 09:33:21AM +0200, Thomas Huth wrote:
> >> On 09/09/15 23:10, Thomas Huth wrote:
> >>> On 08/09/15 07:15, David Gibson wrote:
> >> ...
> >>>> At this point rather than just implementing them as discrete machine
> >>>> options, I suspect it will be more maintainable to split out the
> >>>> h-random implementation as a pseudo-device with its own qdev and so
> >>>> forth.  We already do similarly for the RTAS time of day functions
> >>>> (spapr-rtc).
> >>>
> >>> I gave that I try, but it does not work as expected. To be able to
> >>> specify the options, I'd need to instantiate this device with the
> >>> "-device" option, right? Something like:
> >>>
> >>> 	-device spapr-rng,backend=rng0,usekvm=0
> >>>
> >>> Now this does not work when I use TYPE_SYS_BUS_DEVICE as parent class
> >>> like it is done for spapr-rtc, since the user apparently can not plug
> >>> device to this bus on machine spapr (you can also not plug an spapr-rtc
> >>> device this way!).
> >>>
> >>> The spapr-vlan, spapr-vty, etc. devices are TYPE_VIO_SPAPR_DEVICE, so I
> >>> also tried that instead, but then the rng device suddenly shows up under
> >>> /vdevice in the device tree - that's also not what we want, I guess.
> >>
> >> I did some more tests, and I think I can get this working with one small
> >> modification to spapr_vio.c
> ...
> >> i.e. when the dt_name has not been set, the device won't be added to the
> >> /vdevice device tree node. If that's acceptable, I'll continue with this
> >> approach.
> > 
> > A bit hacky.
> > 
> > I think it would be preferable to build it under SysBus by default,
> > like spapr-rtc.  Properties can be set on the device using -global (or
> > -set, but -global is easier).
> 
> If anyhow possible, I'd prefere to use "-device" for this instead, because
> 
> a) it's easier to use for the user, for example you can simply use
>    "-device spapr-rng,?" to get the list of properties - this
>    does not seem to work with spapr-rtc (it has a "date" property
>    which does not show up in the help text?)

Actually, I don't think that's got anything to do with -device versus
otherwise.  "date" doesn't appear because it's an "object" property
rather than a "qdev" property - that distinction is subtle and
confusing, yes.

> b) unlike the rtc device which is always instantiated, the rng
>    device is rather optional, so it is IMHO more intuitive if
>    created via the -device option.

Hrm, that's true though.  And.. we're back at the perrenial question
of what "standard" devices should be constructed by default.  And what
"default" means.

It seems to me that while the random device is optional, it should be
created by default.  But with -device there's not really a way to do
that.  But then again if it's constructed internally there's not
really a way to turn it off short of hacky machine options.  Ugh.

> So I'd like to give it a try with the TYPE_VIO_SPAPR_DEVICE first ... if
> you then still don't like the patches at all, I can still rework them to
> use TYPE_SYS_BUS_DEVICE instead.

I still dislike putting it on the VIO "bus", since PAPR doesn't
consider it a VIO device.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3243 bytes --]

On Thu, Sep 10, 2015 at 02:13:26PM +0200, Alexander Graf wrote:
> 
> 
> > Am 10.09.2015 um 14:03 schrieb Thomas Huth <thuth@redhat.com>:
> > 
> >> On 10/09/15 12:40, David Gibson wrote:
> >>> On Thu, Sep 10, 2015 at 09:33:21AM +0200, Thomas Huth wrote:
> >>>> On 09/09/15 23:10, Thomas Huth wrote:
> >>>> On 08/09/15 07:15, David Gibson wrote:
> >>> ...
> >>>>> At this point rather than just implementing them as discrete machine
> >>>>> options, I suspect it will be more maintainable to split out the
> >>>>> h-random implementation as a pseudo-device with its own qdev and so
> >>>>> forth.  We already do similarly for the RTAS time of day functions
> >>>>> (spapr-rtc).
> >>>> 
> >>>> I gave that I try, but it does not work as expected. To be able to
> >>>> specify the options, I'd need to instantiate this device with the
> >>>> "-device" option, right? Something like:
> >>>> 
> >>>>    -device spapr-rng,backend=rng0,usekvm=0
> >>>> 
> >>>> Now this does not work when I use TYPE_SYS_BUS_DEVICE as parent class
> >>>> like it is done for spapr-rtc, since the user apparently can not plug
> >>>> device to this bus on machine spapr (you can also not plug an spapr-rtc
> >>>> device this way!).
> >>>> 
> >>>> The spapr-vlan, spapr-vty, etc. devices are TYPE_VIO_SPAPR_DEVICE, so I
> >>>> also tried that instead, but then the rng device suddenly shows up under
> >>>> /vdevice in the device tree - that's also not what we want, I guess.
> >>> 
> >>> I did some more tests, and I think I can get this working with one small
> >>> modification to spapr_vio.c
> > ...
> >>> i.e. when the dt_name has not been set, the device won't be added to the
> >>> /vdevice device tree node. If that's acceptable, I'll continue with this
> >>> approach.
> >> 
> >> A bit hacky.
> >> 
> >> I think it would be preferable to build it under SysBus by default,
> >> like spapr-rtc.  Properties can be set on the device using -global (or
> >> -set, but -global is easier).
> > 
> > If anyhow possible, I'd prefere to use "-device" for this instead, because
> > 
> > a) it's easier to use for the user, for example you can simply use
> >   "-device spapr-rng,?" to get the list of properties - this
> >   does not seem to work with spapr-rtc (it has a "date" property
> >   which does not show up in the help text?)
> > 
> > b) unlike the rtc device which is always instantiated, the rng
> >   device is rather optional, so it is IMHO more intuitive if
> >   created via the -device option.
> > 
> > So I'd like to give it a try with the TYPE_VIO_SPAPR_DEVICE first ... if
> > you then still don't like the patches at all, I can still rework them to
> > use TYPE_SYS_BUS_DEVICE instead.
> 
> Please don't use sysbus. If the vio device approach turns ugly,
> create a new spapr hcall bus instead. We should have had that from
> the beginning really.

Ok.. why?

It's a system (pseudo-)device that doesn't have any common bus
infrastructure with anything else.  Isn't that what SysBus is for?

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 4070 bytes --]

On 11/09/15 02:45, David Gibson wrote:
> On Thu, Sep 10, 2015 at 02:03:39PM +0200, Thomas Huth wrote:
>> On 10/09/15 12:40, David Gibson wrote:
>>> On Thu, Sep 10, 2015 at 09:33:21AM +0200, Thomas Huth wrote:
>>>> On 09/09/15 23:10, Thomas Huth wrote:
>>>>> On 08/09/15 07:15, David Gibson wrote:
>>>> ...
>>>>>> At this point rather than just implementing them as discrete machine
>>>>>> options, I suspect it will be more maintainable to split out the
>>>>>> h-random implementation as a pseudo-device with its own qdev and so
>>>>>> forth.  We already do similarly for the RTAS time of day functions
>>>>>> (spapr-rtc).
>>>>>
>>>>> I gave that I try, but it does not work as expected. To be able to
>>>>> specify the options, I'd need to instantiate this device with the
>>>>> "-device" option, right? Something like:
>>>>>
>>>>> 	-device spapr-rng,backend=rng0,usekvm=0
>>>>>
>>>>> Now this does not work when I use TYPE_SYS_BUS_DEVICE as parent class
>>>>> like it is done for spapr-rtc, since the user apparently can not plug
>>>>> device to this bus on machine spapr (you can also not plug an spapr-rtc
>>>>> device this way!).
>>>>>
>>>>> The spapr-vlan, spapr-vty, etc. devices are TYPE_VIO_SPAPR_DEVICE, so I
>>>>> also tried that instead, but then the rng device suddenly shows up under
>>>>> /vdevice in the device tree - that's also not what we want, I guess.
>>>>
>>>> I did some more tests, and I think I can get this working with one small
>>>> modification to spapr_vio.c
>> ...
>>>> i.e. when the dt_name has not been set, the device won't be added to the
>>>> /vdevice device tree node. If that's acceptable, I'll continue with this
>>>> approach.
>>>
>>> A bit hacky.
>>>
>>> I think it would be preferable to build it under SysBus by default,
>>> like spapr-rtc.  Properties can be set on the device using -global (or
>>> -set, but -global is easier).
>>
>> If anyhow possible, I'd prefere to use "-device" for this instead, because
>>
>> a) it's easier to use for the user, for example you can simply use
>>    "-device spapr-rng,?" to get the list of properties - this
>>    does not seem to work with spapr-rtc (it has a "date" property
>>    which does not show up in the help text?)
> 
> Actually, I don't think that's got anything to do with -device versus
> otherwise.  "date" doesn't appear because it's an "object" property
> rather than a "qdev" property - that distinction is subtle and
> confusing, yes.

At least it is not very friendly for the user ... if a configuration
property does not show up in the help text, you've got to document it
somewhere else or nobody will be aware of it.

>> b) unlike the rtc device which is always instantiated, the rng
>>    device is rather optional, so it is IMHO more intuitive if
>>    created via the -device option.
> 
> Hrm, that's true though.  And.. we're back at the perrenial question
> of what "standard" devices should be constructed by default.  And what
> "default" means.
> 
> It seems to me that while the random device is optional, it should be
> created by default.  But with -device there's not really a way to do
> that.  But then again if it's constructed internally there's not
> really a way to turn it off short of hacky machine options.  Ugh.
> 
>> So I'd like to give it a try with the TYPE_VIO_SPAPR_DEVICE first ... if
>> you then still don't like the patches at all, I can still rework them to
>> use TYPE_SYS_BUS_DEVICE instead.
> 
> I still dislike putting it on the VIO "bus", since PAPR doesn't
> consider it a VIO device.

Hmm, that's also a valid point.

After doing some more research, I think I've found yet another
possibility (why isn't there a proper documentation/howto for all this
QOM stuff ... or did I just miss it?) :
Instead of using a bus, simply set parent = TYPE_DEVICE, so that it is a
"bus-less" device. Seems to work fine at a first glance, so unless
somebody tells me that this is a very bad idea, I'll try to rework my
patches accordingly...

 Thomas



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[Alexander Graf]

On 11.09.15 02:46, David Gibson wrote:
> On Thu, Sep 10, 2015 at 02:13:26PM +0200, Alexander Graf wrote:
>>
>>
>>> Am 10.09.2015 um 14:03 schrieb Thomas Huth <thuth@redhat.com>:
>>>
>>>> On 10/09/15 12:40, David Gibson wrote:
>>>>> On Thu, Sep 10, 2015 at 09:33:21AM +0200, Thomas Huth wrote:
>>>>>> On 09/09/15 23:10, Thomas Huth wrote:
>>>>>> On 08/09/15 07:15, David Gibson wrote:
>>>>> ...
>>>>>>> At this point rather than just implementing them as discrete machine
>>>>>>> options, I suspect it will be more maintainable to split out the
>>>>>>> h-random implementation as a pseudo-device with its own qdev and so
>>>>>>> forth.  We already do similarly for the RTAS time of day functions
>>>>>>> (spapr-rtc).
>>>>>>
>>>>>> I gave that I try, but it does not work as expected. To be able to
>>>>>> specify the options, I'd need to instantiate this device with the
>>>>>> "-device" option, right? Something like:
>>>>>>
>>>>>>    -device spapr-rng,backend=rng0,usekvm=0
>>>>>>
>>>>>> Now this does not work when I use TYPE_SYS_BUS_DEVICE as parent class
>>>>>> like it is done for spapr-rtc, since the user apparently can not plug
>>>>>> device to this bus on machine spapr (you can also not plug an spapr-rtc
>>>>>> device this way!).
>>>>>>
>>>>>> The spapr-vlan, spapr-vty, etc. devices are TYPE_VIO_SPAPR_DEVICE, so I
>>>>>> also tried that instead, but then the rng device suddenly shows up under
>>>>>> /vdevice in the device tree - that's also not what we want, I guess.
>>>>>
>>>>> I did some more tests, and I think I can get this working with one small
>>>>> modification to spapr_vio.c
>>> ...
>>>>> i.e. when the dt_name has not been set, the device won't be added to the
>>>>> /vdevice device tree node. If that's acceptable, I'll continue with this
>>>>> approach.
>>>>
>>>> A bit hacky.
>>>>
>>>> I think it would be preferable to build it under SysBus by default,
>>>> like spapr-rtc.  Properties can be set on the device using -global (or
>>>> -set, but -global is easier).
>>>
>>> If anyhow possible, I'd prefere to use "-device" for this instead, because
>>>
>>> a) it's easier to use for the user, for example you can simply use
>>>   "-device spapr-rng,?" to get the list of properties - this
>>>   does not seem to work with spapr-rtc (it has a "date" property
>>>   which does not show up in the help text?)
>>>
>>> b) unlike the rtc device which is always instantiated, the rng
>>>   device is rather optional, so it is IMHO more intuitive if
>>>   created via the -device option.
>>>
>>> So I'd like to give it a try with the TYPE_VIO_SPAPR_DEVICE first ... if
>>> you then still don't like the patches at all, I can still rework them to
>>> use TYPE_SYS_BUS_DEVICE instead.
>>
>> Please don't use sysbus. If the vio device approach turns ugly,
>> create a new spapr hcall bus instead. We should have had that from
>> the beginning really.
> 
> Ok.. why?
> 
> It's a system (pseudo-)device that doesn't have any common bus
> infrastructure with anything else.  Isn't that what SysBus is for?

No, sysbus means "A device that has MMIO and/or PIO connected via a bus
I'm too lazy to model" really. These devices have neither.

Back in the days before QOM, sysbus was our lowest common denominator,
but now that we have TYPE_DEVICE and can branch off of that, we really
shouldn't abuse sysbus devices for things they aren't.


Alex

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5050 bytes --]

On Fri, Sep 11, 2015 at 09:30:28AM +0200, Thomas Huth wrote:
> On 11/09/15 02:45, David Gibson wrote:
> > On Thu, Sep 10, 2015 at 02:03:39PM +0200, Thomas Huth wrote:
> >> On 10/09/15 12:40, David Gibson wrote:
> >>> On Thu, Sep 10, 2015 at 09:33:21AM +0200, Thomas Huth wrote:
> >>>> On 09/09/15 23:10, Thomas Huth wrote:
> >>>>> On 08/09/15 07:15, David Gibson wrote:
> >>>> ...
> >>>>>> At this point rather than just implementing them as discrete machine
> >>>>>> options, I suspect it will be more maintainable to split out the
> >>>>>> h-random implementation as a pseudo-device with its own qdev and so
> >>>>>> forth.  We already do similarly for the RTAS time of day functions
> >>>>>> (spapr-rtc).
> >>>>>
> >>>>> I gave that I try, but it does not work as expected. To be able to
> >>>>> specify the options, I'd need to instantiate this device with the
> >>>>> "-device" option, right? Something like:
> >>>>>
> >>>>> 	-device spapr-rng,backend=rng0,usekvm=0
> >>>>>
> >>>>> Now this does not work when I use TYPE_SYS_BUS_DEVICE as parent class
> >>>>> like it is done for spapr-rtc, since the user apparently can not plug
> >>>>> device to this bus on machine spapr (you can also not plug an spapr-rtc
> >>>>> device this way!).
> >>>>>
> >>>>> The spapr-vlan, spapr-vty, etc. devices are TYPE_VIO_SPAPR_DEVICE, so I
> >>>>> also tried that instead, but then the rng device suddenly shows up under
> >>>>> /vdevice in the device tree - that's also not what we want, I guess.
> >>>>
> >>>> I did some more tests, and I think I can get this working with one small
> >>>> modification to spapr_vio.c
> >> ...
> >>>> i.e. when the dt_name has not been set, the device won't be added to the
> >>>> /vdevice device tree node. If that's acceptable, I'll continue with this
> >>>> approach.
> >>>
> >>> A bit hacky.
> >>>
> >>> I think it would be preferable to build it under SysBus by default,
> >>> like spapr-rtc.  Properties can be set on the device using -global (or
> >>> -set, but -global is easier).
> >>
> >> If anyhow possible, I'd prefere to use "-device" for this instead, because
> >>
> >> a) it's easier to use for the user, for example you can simply use
> >>    "-device spapr-rng,?" to get the list of properties - this
> >>    does not seem to work with spapr-rtc (it has a "date" property
> >>    which does not show up in the help text?)
> > 
> > Actually, I don't think that's got anything to do with -device versus
> > otherwise.  "date" doesn't appear because it's an "object" property
> > rather than a "qdev" property - that distinction is subtle and
> > confusing, yes.
> 
> At least it is not very friendly for the user ... if a configuration
> property does not show up in the help text, you've got to document it
> somewhere else or nobody will be aware of it.

Not arguing with that.

In this case it happened because I just copied the setup code from
mc146818rtc which also doesn't set a description.

> >> b) unlike the rtc device which is always instantiated, the rng
> >>    device is rather optional, so it is IMHO more intuitive if
> >>    created via the -device option.
> > 
> > Hrm, that's true though.  And.. we're back at the perrenial question
> > of what "standard" devices should be constructed by default.  And what
> > "default" means.
> > 
> > It seems to me that while the random device is optional, it should be
> > created by default.  But with -device there's not really a way to do
> > that.  But then again if it's constructed internally there's not
> > really a way to turn it off short of hacky machine options.  Ugh.
> > 
> >> So I'd like to give it a try with the TYPE_VIO_SPAPR_DEVICE first ... if
> >> you then still don't like the patches at all, I can still rework them to
> >> use TYPE_SYS_BUS_DEVICE instead.
> > 
> > I still dislike putting it on the VIO "bus", since PAPR doesn't
> > consider it a VIO device.
> 
> Hmm, that's also a valid point.
> 
> After doing some more research, I think I've found yet another
> possibility (why isn't there a proper documentation/howto for all this
> QOM stuff ... or did I just miss it?) :

Tell me about it.  The fact that there are apparently a whole bunch of
conventions about how QOM things should be done that are neither
obvious nor document is starting to really irritate me.

> Instead of using a bus, simply set parent = TYPE_DEVICE, so that it is a
> "bus-less" device. Seems to work fine at a first glance, so unless
> somebody tells me that this is a very bad idea, I'll try to rework my
> patches accordingly...

From agraf's comment, this seems like the way to go.

I'm still pretty confused about where such a device sits in the
composition tree.  I had thought that SysBus was the root of the qdev
tree, but apparently not.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3954 bytes --]

On Fri, Sep 11, 2015 at 11:43:02AM +0200, Alexander Graf wrote:
> 
> 
> On 11.09.15 02:46, David Gibson wrote:
> > On Thu, Sep 10, 2015 at 02:13:26PM +0200, Alexander Graf wrote:
> >>
> >>
> >>> Am 10.09.2015 um 14:03 schrieb Thomas Huth <thuth@redhat.com>:
> >>>
> >>>> On 10/09/15 12:40, David Gibson wrote:
> >>>>> On Thu, Sep 10, 2015 at 09:33:21AM +0200, Thomas Huth wrote:
> >>>>>> On 09/09/15 23:10, Thomas Huth wrote:
> >>>>>> On 08/09/15 07:15, David Gibson wrote:
> >>>>> ...
> >>>>>>> At this point rather than just implementing them as discrete machine
> >>>>>>> options, I suspect it will be more maintainable to split out the
> >>>>>>> h-random implementation as a pseudo-device with its own qdev and so
> >>>>>>> forth.  We already do similarly for the RTAS time of day functions
> >>>>>>> (spapr-rtc).
> >>>>>>
> >>>>>> I gave that I try, but it does not work as expected. To be able to
> >>>>>> specify the options, I'd need to instantiate this device with the
> >>>>>> "-device" option, right? Something like:
> >>>>>>
> >>>>>>    -device spapr-rng,backend=rng0,usekvm=0
> >>>>>>
> >>>>>> Now this does not work when I use TYPE_SYS_BUS_DEVICE as parent class
> >>>>>> like it is done for spapr-rtc, since the user apparently can not plug
> >>>>>> device to this bus on machine spapr (you can also not plug an spapr-rtc
> >>>>>> device this way!).
> >>>>>>
> >>>>>> The spapr-vlan, spapr-vty, etc. devices are TYPE_VIO_SPAPR_DEVICE, so I
> >>>>>> also tried that instead, but then the rng device suddenly shows up under
> >>>>>> /vdevice in the device tree - that's also not what we want, I guess.
> >>>>>
> >>>>> I did some more tests, and I think I can get this working with one small
> >>>>> modification to spapr_vio.c
> >>> ...
> >>>>> i.e. when the dt_name has not been set, the device won't be added to the
> >>>>> /vdevice device tree node. If that's acceptable, I'll continue with this
> >>>>> approach.
> >>>>
> >>>> A bit hacky.
> >>>>
> >>>> I think it would be preferable to build it under SysBus by default,
> >>>> like spapr-rtc.  Properties can be set on the device using -global (or
> >>>> -set, but -global is easier).
> >>>
> >>> If anyhow possible, I'd prefere to use "-device" for this instead, because
> >>>
> >>> a) it's easier to use for the user, for example you can simply use
> >>>   "-device spapr-rng,?" to get the list of properties - this
> >>>   does not seem to work with spapr-rtc (it has a "date" property
> >>>   which does not show up in the help text?)
> >>>
> >>> b) unlike the rtc device which is always instantiated, the rng
> >>>   device is rather optional, so it is IMHO more intuitive if
> >>>   created via the -device option.
> >>>
> >>> So I'd like to give it a try with the TYPE_VIO_SPAPR_DEVICE first ... if
> >>> you then still don't like the patches at all, I can still rework them to
> >>> use TYPE_SYS_BUS_DEVICE instead.
> >>
> >> Please don't use sysbus. If the vio device approach turns ugly,
> >> create a new spapr hcall bus instead. We should have had that from
> >> the beginning really.
> > 
> > Ok.. why?
> > 
> > It's a system (pseudo-)device that doesn't have any common bus
> > infrastructure with anything else.  Isn't that what SysBus is for?
> 
> No, sysbus means "A device that has MMIO and/or PIO connected via a bus
> I'm too lazy to model" really. These devices have neither.

Oh.

So.. where is one supposed to find that out?

> Back in the days before QOM, sysbus was our lowest common denominator,
> but now that we have TYPE_DEVICE and can branch off of that, we really
> shouldn't abuse sysbus devices for things they aren't.

So what actually is the root of the qdev tree then?

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/2] spapr: Add support for hwrng when available

[Alexander Graf]

On 14.09.15 04:27, David Gibson wrote:
> On Fri, Sep 11, 2015 at 11:43:02AM +0200, Alexander Graf wrote:
>>
>>
>> On 11.09.15 02:46, David Gibson wrote:
>>> On Thu, Sep 10, 2015 at 02:13:26PM +0200, Alexander Graf wrote:
>>>>
>>>>
>>>>> Am 10.09.2015 um 14:03 schrieb Thomas Huth <thuth@redhat.com>:
>>>>>
>>>>>> On 10/09/15 12:40, David Gibson wrote:
>>>>>>> On Thu, Sep 10, 2015 at 09:33:21AM +0200, Thomas Huth wrote:
>>>>>>>> On 09/09/15 23:10, Thomas Huth wrote:
>>>>>>>> On 08/09/15 07:15, David Gibson wrote:
>>>>>>> ...
>>>>>>>>> At this point rather than just implementing them as discrete machine
>>>>>>>>> options, I suspect it will be more maintainable to split out the
>>>>>>>>> h-random implementation as a pseudo-device with its own qdev and so
>>>>>>>>> forth.  We already do similarly for the RTAS time of day functions
>>>>>>>>> (spapr-rtc).
>>>>>>>>
>>>>>>>> I gave that I try, but it does not work as expected. To be able to
>>>>>>>> specify the options, I'd need to instantiate this device with the
>>>>>>>> "-device" option, right? Something like:
>>>>>>>>
>>>>>>>>    -device spapr-rng,backend=rng0,usekvm=0
>>>>>>>>
>>>>>>>> Now this does not work when I use TYPE_SYS_BUS_DEVICE as parent class
>>>>>>>> like it is done for spapr-rtc, since the user apparently can not plug
>>>>>>>> device to this bus on machine spapr (you can also not plug an spapr-rtc
>>>>>>>> device this way!).
>>>>>>>>
>>>>>>>> The spapr-vlan, spapr-vty, etc. devices are TYPE_VIO_SPAPR_DEVICE, so I
>>>>>>>> also tried that instead, but then the rng device suddenly shows up under
>>>>>>>> /vdevice in the device tree - that's also not what we want, I guess.
>>>>>>>
>>>>>>> I did some more tests, and I think I can get this working with one small
>>>>>>> modification to spapr_vio.c
>>>>> ...
>>>>>>> i.e. when the dt_name has not been set, the device won't be added to the
>>>>>>> /vdevice device tree node. If that's acceptable, I'll continue with this
>>>>>>> approach.
>>>>>>
>>>>>> A bit hacky.
>>>>>>
>>>>>> I think it would be preferable to build it under SysBus by default,
>>>>>> like spapr-rtc.  Properties can be set on the device using -global (or
>>>>>> -set, but -global is easier).
>>>>>
>>>>> If anyhow possible, I'd prefere to use "-device" for this instead, because
>>>>>
>>>>> a) it's easier to use for the user, for example you can simply use
>>>>>   "-device spapr-rng,?" to get the list of properties - this
>>>>>   does not seem to work with spapr-rtc (it has a "date" property
>>>>>   which does not show up in the help text?)
>>>>>
>>>>> b) unlike the rtc device which is always instantiated, the rng
>>>>>   device is rather optional, so it is IMHO more intuitive if
>>>>>   created via the -device option.
>>>>>
>>>>> So I'd like to give it a try with the TYPE_VIO_SPAPR_DEVICE first ... if
>>>>> you then still don't like the patches at all, I can still rework them to
>>>>> use TYPE_SYS_BUS_DEVICE instead.
>>>>
>>>> Please don't use sysbus. If the vio device approach turns ugly,
>>>> create a new spapr hcall bus instead. We should have had that from
>>>> the beginning really.
>>>
>>> Ok.. why?
>>>
>>> It's a system (pseudo-)device that doesn't have any common bus
>>> infrastructure with anything else.  Isn't that what SysBus is for?
>>
>> No, sysbus means "A device that has MMIO and/or PIO connected via a bus
>> I'm too lazy to model" really. These devices have neither.
> 
> Oh.
> 
> So.. where is one supposed to find that out?

You could ask the same about any bus really. It's more or less common
sense / collective knowledge / call it what you want.

Just check out the sysbus code files and you'll see that 90% of them are
about handling mmio / pio and irqs. Do you need that logic? No? Then
sysbus is not for you :).

> 
>> Back in the days before QOM, sysbus was our lowest common denominator,
>> but now that we have TYPE_DEVICE and can branch off of that, we really
>> shouldn't abuse sysbus devices for things they aren't.
> 
> So what actually is the root of the qdev tree then?

qdev is legacy, qom is new :). In qdev sysbus was the root bus, in qom
it's not. For details on what exactly is the root for qom, please just
poke Andreas - I keep having a hard time to wrap my head around the qom
topology. I'm not even sure it has a root in the traditional sense.


Alex