From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Huth Date: Mon, 14 Sep 2015 06:32:36 +0000 Subject: Re: [PATCH v3] ppc/spapr: Implement H_RANDOM hypercall in QEMU Message-Id: <55F66A04.4010304@redhat.com> MIME-Version: 1 Content-Type: multipart/mixed; boundary="qRQMiwplpdu8ROfvJvds9xAN6Vm9jCCIs" List-Id: References: <1441963021-31423-1-git-send-email-thuth@redhat.com> <20150914021506.GD2547@voom.fritz.box> In-Reply-To: <20150914021506.GD2547@voom.fritz.box> To: David Gibson Cc: agraf@suse.de, kvm-ppc@vger.kernel.org, qemu-devel@nongnu.org, michael@ellerman.id.au, qemu-ppc@nongnu.org, amit.shah@redhat.com, sam.bobroff@au1.ibm.com, gkurz@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --qRQMiwplpdu8ROfvJvds9xAN6Vm9jCCIs Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 14/09/15 04:15, David Gibson wrote: > On Fri, Sep 11, 2015 at 11:17:01AM +0200, Thomas Huth wrote: >> The PAPR interface defines a hypercall to pass high-quality >> hardware generated random numbers to guests. Recent kernels can >> already provide this hypercall to the guest if the right hardware >> random number generator is available. But in case the user wants >> to use another source like EGD, or QEMU is running with an older >> kernel, we should also have this call in QEMU, so that guests that >> do not support virtio-rng yet can get good random numbers, too. >> >> This patch now adds a new pseude-device to QEMU that either >> directly provides this hypercall to the guest or is able to >> enable the in-kernel hypercall if available. The in-kernel >> hypercall can be enabled with the use-kvm property, e.g.: >> >> qemu-system-ppc64 -device spapr-rng,use-kvm=3Dtrue >> >> For handling the hypercall in QEMU instead, a RngBackend is required >> since the hypercall should provide "good" random data instead of >> pseudo-random (like from a "simple" library function like rand() >> or g_random_int()). Since there are multiple RngBackends available, >> the user must select an appropriate backend via the "backend" >> property of the device, e.g.: >> >> qemu-system-ppc64 -object rng-random,filename=3D/dev/hwrng,id=3Drng0 = \ >> -device spapr-rng,backend=3Drng0 ... >> >> See http://wiki.qemu-project.org/Features-Done/VirtIORNG for >> other example of specifying RngBackends. =2E.. >> + >> +#include "qemu/error-report.h" >> +#include "sysemu/sysemu.h" >> +#include "sysemu/device_tree.h" >> +#include "sysemu/rng.h" >> +#include "hw/ppc/spapr.h" >> +#include "kvm_ppc.h" >> + >> +#define SPAPR_RNG(obj) \ >> + OBJECT_CHECK(sPAPRRngState, (obj), TYPE_SPAPR_RNG) >> + >> +typedef struct sPAPRRngState { >> + /*< private >*/ >> + DeviceState ds; >> + RngBackend *backend; >> + bool use_kvm; >> +} sPAPRRngState; >> + >> +typedef struct HRandomData { >> + QemuSemaphore sem; >> + union { >> + uint64_t v64; >> + uint8_t v8[8]; >> + } val; >> + int received; >> +} HRandomData; >> + >> +/* Callback function for the RngBackend */ >> +static void random_recv(void *dest, const void *src, size_t size) >> +{ >> + HRandomData *hrdp =3D dest; >> + >> + if (src && size > 0) { >> + assert(size + hrdp->received <=3D sizeof(hrdp->val.v8)); >> + memcpy(&hrdp->val.v8[hrdp->received], src, size); >> + hrdp->received +=3D size; >> + } >> + >> + qemu_sem_post(&hrdp->sem); >=20 > I'm assuming qemu_sem_post() includes the necessary memory barrier to > make sure the requesting thread actually sees the data. Not sure whether I fully got your point here... both callback function and main thread are calling an extern C-function, so the compiler should not assume that the memory stays the same in the main thread...? Anyway, I've tested the hypercall by implementing it in SLOF and calling it a couple of times there to see that all bits in the result behave randomly, so for me this is working fine. >> +} >> + >> +/* Handler for the H_RANDOM hypercall */ >> +static target_ulong h_random(PowerPCCPU *cpu, sPAPRMachineState *spap= r, >> + target_ulong opcode, target_ulong *args)= >> +{ >> + sPAPRRngState *rngstate; >> + HRandomData hrdata; >> + >> + rngstate =3D SPAPR_RNG(object_resolve_path_type("", TYPE_SPAPR_RN= G, NULL)); >> + >> + if (!rngstate || !rngstate->backend) { >> + return H_HARDWARE; >> + } >> + >> + qemu_sem_init(&hrdata.sem, 0); >> + hrdata.val.v64 =3D 0; >> + hrdata.received =3D 0; >> + >> + qemu_mutex_unlock_iothread(); >> + while (hrdata.received < 8) { >> + rng_backend_request_entropy(rngstate->backend, 8 - hrdata.rec= eived, >> + random_recv, &hrdata); >> + qemu_sem_wait(&hrdata.sem); >> + } >> + qemu_mutex_lock_iothread(); >> + >> + qemu_sem_destroy(&hrdata.sem); >> + args[0] =3D hrdata.val.v64; >> + >> + return H_SUCCESS; >> +} >> + >> +static void spapr_rng_instance_init(Object *obj) >> +{ >> + sPAPRRngState *rngstate =3D SPAPR_RNG(obj); >> + >> + if (object_resolve_path_type("", TYPE_SPAPR_RNG, NULL) !=3D NULL)= { >> + error_report("spapr-rng can not be instantiated twice!"); >> + return; >> + } >> + >> + object_property_add_link(obj, "backend", TYPE_RNG_BACKEND, >> + (Object **)&rngstate->backend, >> + object_property_allow_set_link, >> + OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL); >> + object_property_set_description(obj, "backend", >> + "ID of the random number generato= r backend", >> + NULL); >=20 > Since virtio-rng does it the same way, I'm assuming there's a reason > this is constructed with object_propery_add() rather than listing it > in spapr_rng_properties, but it's not obvious what the reason is. I did not spot a macro a la "DEFINE_PROP_LINK" that could be used for this. Do you see a possibility to define a link that way? > More importantly, this should probably be called "rng" not "backend" > to match virtio-rng. Since the device is already called "spapr-rng", i.e. has "rng" in its name, I'd rather like to keep this as "backend" to make it clear that you specify the backend this way. >> +} >> + >> +static void spapr_rng_realize(DeviceState *dev, Error **errp) >> +{ >> + >> + sPAPRRngState *rngstate =3D SPAPR_RNG(dev); >> + >> + if (rngstate->use_kvm) { >> + if (kvmppc_enable_hwrng() !=3D 0) { >> + error_setg(errp, "Could not initialize in-kernel H_RANDOM= call!"); >> + } >> + return; >> + } >> + >> + if (!rngstate->backend) { >> + error_setg(errp, "spapr-rng needs a RNG backend!"); >> + return; >> + } >=20 > So, the logic here means you have to explicitly choose whether to use > the kernel implementation or the qemu imeplementation. >=20 > It seems to me it might be useful to be able to specify "use the > kernel implementation if available, otherwise fall back to qemu". Right, makes sense, I'll update this logic. Thanks for the review, Thomas --qRQMiwplpdu8ROfvJvds9xAN6Vm9jCCIs Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJV9moEAAoJEC7Z13T+cC21eCgP/3HiwlBHxeaaYA+nQCkfsDu7 /jQp+feeXHLvE23GeJA9IpLxtFMW9mcf1KfTk/Z/ve7sDVbb7QdliaAMxmYGAQOC MjW4sRzJZzRFbSxCetwt3ajeZRLw1ZlkjF39Kdm4lK6UraA0VSpbOCVfCLYr6Z8o 2Z3gLFjWWp3JUgOl7pxFeN81+b93KdyCXIWmGVusTUxSviuOxlX7JoAyLfbr1xRs P8MD7HQRiKmy3UudU7UpMdUTK3R5ki5igtZYGkpvd+3mEORjbXBlJdz3p132b1Fm /wz8NM71Kp+zbr6Zke53ZgvGQbrPO1yygsQVh0sPNVt/EZUsFhTbChx4xxQ7Dr+B zU1IkB4v8hssLMVpJwJgEizy6Xe1W0SeELw3JnVJ3zq+Zy5DiJyQxQapmb/boY1R ctmpJC72jhQpsMCISbOyGVLx7o4YWveDSUt67kR7Xf9e0wKBY7aLSHT5Zw/yqcDi pZII6UwTVEG7DgkpsXl0GhMofkkHrXH5WoXxAPzGdg9VDobl0HsDmpeUyCqYjMpE 9AqOIYMRDt3vkQvnkeTNlYkNygvh0cZeDJPxK7AC4KS9g5ecVeL4inm/foqeorN6 mF1SMRskhw9sSOuSkQJnbfItWFgIDKzQL6wDm4Co6zQckkS7ewF2aX6GWaJJNPOG mU2I/l0ilEFSGfW7TL3w =XaCy -----END PGP SIGNATURE----- --qRQMiwplpdu8ROfvJvds9xAN6Vm9jCCIs-- From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48645) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZbNJm-0005fp-Bg for qemu-devel@nongnu.org; Mon, 14 Sep 2015 02:32:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZbNJj-000805-3H for qemu-devel@nongnu.org; Mon, 14 Sep 2015 02:32:46 -0400 References: <1441963021-31423-1-git-send-email-thuth@redhat.com> <20150914021506.GD2547@voom.fritz.box> From: Thomas Huth Message-ID: <55F66A04.4010304@redhat.com> Date: Mon, 14 Sep 2015 08:32:36 +0200 MIME-Version: 1.0 In-Reply-To: <20150914021506.GD2547@voom.fritz.box> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="qRQMiwplpdu8ROfvJvds9xAN6Vm9jCCIs" Subject: Re: [Qemu-devel] [PATCH v3] ppc/spapr: Implement H_RANDOM hypercall in QEMU List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: agraf@suse.de, kvm-ppc@vger.kernel.org, qemu-devel@nongnu.org, michael@ellerman.id.au, qemu-ppc@nongnu.org, amit.shah@redhat.com, sam.bobroff@au1.ibm.com, gkurz@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --qRQMiwplpdu8ROfvJvds9xAN6Vm9jCCIs Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 14/09/15 04:15, David Gibson wrote: > On Fri, Sep 11, 2015 at 11:17:01AM +0200, Thomas Huth wrote: >> The PAPR interface defines a hypercall to pass high-quality >> hardware generated random numbers to guests. Recent kernels can >> already provide this hypercall to the guest if the right hardware >> random number generator is available. But in case the user wants >> to use another source like EGD, or QEMU is running with an older >> kernel, we should also have this call in QEMU, so that guests that >> do not support virtio-rng yet can get good random numbers, too. >> >> This patch now adds a new pseude-device to QEMU that either >> directly provides this hypercall to the guest or is able to >> enable the in-kernel hypercall if available. The in-kernel >> hypercall can be enabled with the use-kvm property, e.g.: >> >> qemu-system-ppc64 -device spapr-rng,use-kvm=3Dtrue >> >> For handling the hypercall in QEMU instead, a RngBackend is required >> since the hypercall should provide "good" random data instead of >> pseudo-random (like from a "simple" library function like rand() >> or g_random_int()). Since there are multiple RngBackends available, >> the user must select an appropriate backend via the "backend" >> property of the device, e.g.: >> >> qemu-system-ppc64 -object rng-random,filename=3D/dev/hwrng,id=3Drng0 = \ >> -device spapr-rng,backend=3Drng0 ... >> >> See http://wiki.qemu-project.org/Features-Done/VirtIORNG for >> other example of specifying RngBackends. =2E.. >> + >> +#include "qemu/error-report.h" >> +#include "sysemu/sysemu.h" >> +#include "sysemu/device_tree.h" >> +#include "sysemu/rng.h" >> +#include "hw/ppc/spapr.h" >> +#include "kvm_ppc.h" >> + >> +#define SPAPR_RNG(obj) \ >> + OBJECT_CHECK(sPAPRRngState, (obj), TYPE_SPAPR_RNG) >> + >> +typedef struct sPAPRRngState { >> + /*< private >*/ >> + DeviceState ds; >> + RngBackend *backend; >> + bool use_kvm; >> +} sPAPRRngState; >> + >> +typedef struct HRandomData { >> + QemuSemaphore sem; >> + union { >> + uint64_t v64; >> + uint8_t v8[8]; >> + } val; >> + int received; >> +} HRandomData; >> + >> +/* Callback function for the RngBackend */ >> +static void random_recv(void *dest, const void *src, size_t size) >> +{ >> + HRandomData *hrdp =3D dest; >> + >> + if (src && size > 0) { >> + assert(size + hrdp->received <=3D sizeof(hrdp->val.v8)); >> + memcpy(&hrdp->val.v8[hrdp->received], src, size); >> + hrdp->received +=3D size; >> + } >> + >> + qemu_sem_post(&hrdp->sem); >=20 > I'm assuming qemu_sem_post() includes the necessary memory barrier to > make sure the requesting thread actually sees the data. Not sure whether I fully got your point here... both callback function and main thread are calling an extern C-function, so the compiler should not assume that the memory stays the same in the main thread...? Anyway, I've tested the hypercall by implementing it in SLOF and calling it a couple of times there to see that all bits in the result behave randomly, so for me this is working fine. >> +} >> + >> +/* Handler for the H_RANDOM hypercall */ >> +static target_ulong h_random(PowerPCCPU *cpu, sPAPRMachineState *spap= r, >> + target_ulong opcode, target_ulong *args)= >> +{ >> + sPAPRRngState *rngstate; >> + HRandomData hrdata; >> + >> + rngstate =3D SPAPR_RNG(object_resolve_path_type("", TYPE_SPAPR_RN= G, NULL)); >> + >> + if (!rngstate || !rngstate->backend) { >> + return H_HARDWARE; >> + } >> + >> + qemu_sem_init(&hrdata.sem, 0); >> + hrdata.val.v64 =3D 0; >> + hrdata.received =3D 0; >> + >> + qemu_mutex_unlock_iothread(); >> + while (hrdata.received < 8) { >> + rng_backend_request_entropy(rngstate->backend, 8 - hrdata.rec= eived, >> + random_recv, &hrdata); >> + qemu_sem_wait(&hrdata.sem); >> + } >> + qemu_mutex_lock_iothread(); >> + >> + qemu_sem_destroy(&hrdata.sem); >> + args[0] =3D hrdata.val.v64; >> + >> + return H_SUCCESS; >> +} >> + >> +static void spapr_rng_instance_init(Object *obj) >> +{ >> + sPAPRRngState *rngstate =3D SPAPR_RNG(obj); >> + >> + if (object_resolve_path_type("", TYPE_SPAPR_RNG, NULL) !=3D NULL)= { >> + error_report("spapr-rng can not be instantiated twice!"); >> + return; >> + } >> + >> + object_property_add_link(obj, "backend", TYPE_RNG_BACKEND, >> + (Object **)&rngstate->backend, >> + object_property_allow_set_link, >> + OBJ_PROP_LINK_UNREF_ON_RELEASE, NULL); >> + object_property_set_description(obj, "backend", >> + "ID of the random number generato= r backend", >> + NULL); >=20 > Since virtio-rng does it the same way, I'm assuming there's a reason > this is constructed with object_propery_add() rather than listing it > in spapr_rng_properties, but it's not obvious what the reason is. I did not spot a macro a la "DEFINE_PROP_LINK" that could be used for this. Do you see a possibility to define a link that way? > More importantly, this should probably be called "rng" not "backend" > to match virtio-rng. Since the device is already called "spapr-rng", i.e. has "rng" in its name, I'd rather like to keep this as "backend" to make it clear that you specify the backend this way. >> +} >> + >> +static void spapr_rng_realize(DeviceState *dev, Error **errp) >> +{ >> + >> + sPAPRRngState *rngstate =3D SPAPR_RNG(dev); >> + >> + if (rngstate->use_kvm) { >> + if (kvmppc_enable_hwrng() !=3D 0) { >> + error_setg(errp, "Could not initialize in-kernel H_RANDOM= call!"); >> + } >> + return; >> + } >> + >> + if (!rngstate->backend) { >> + error_setg(errp, "spapr-rng needs a RNG backend!"); >> + return; >> + } >=20 > So, the logic here means you have to explicitly choose whether to use > the kernel implementation or the qemu imeplementation. >=20 > It seems to me it might be useful to be able to specify "use the > kernel implementation if available, otherwise fall back to qemu". Right, makes sense, I'll update this logic. Thanks for the review, Thomas --qRQMiwplpdu8ROfvJvds9xAN6Vm9jCCIs Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJV9moEAAoJEC7Z13T+cC21eCgP/3HiwlBHxeaaYA+nQCkfsDu7 /jQp+feeXHLvE23GeJA9IpLxtFMW9mcf1KfTk/Z/ve7sDVbb7QdliaAMxmYGAQOC MjW4sRzJZzRFbSxCetwt3ajeZRLw1ZlkjF39Kdm4lK6UraA0VSpbOCVfCLYr6Z8o 2Z3gLFjWWp3JUgOl7pxFeN81+b93KdyCXIWmGVusTUxSviuOxlX7JoAyLfbr1xRs P8MD7HQRiKmy3UudU7UpMdUTK3R5ki5igtZYGkpvd+3mEORjbXBlJdz3p132b1Fm /wz8NM71Kp+zbr6Zke53ZgvGQbrPO1yygsQVh0sPNVt/EZUsFhTbChx4xxQ7Dr+B zU1IkB4v8hssLMVpJwJgEizy6Xe1W0SeELw3JnVJ3zq+Zy5DiJyQxQapmb/boY1R ctmpJC72jhQpsMCISbOyGVLx7o4YWveDSUt67kR7Xf9e0wKBY7aLSHT5Zw/yqcDi pZII6UwTVEG7DgkpsXl0GhMofkkHrXH5WoXxAPzGdg9VDobl0HsDmpeUyCqYjMpE 9AqOIYMRDt3vkQvnkeTNlYkNygvh0cZeDJPxK7AC4KS9g5ecVeL4inm/foqeorN6 mF1SMRskhw9sSOuSkQJnbfItWFgIDKzQL6wDm4Co6zQckkS7ewF2aX6GWaJJNPOG mU2I/l0ilEFSGfW7TL3w =XaCy -----END PGP SIGNATURE----- --qRQMiwplpdu8ROfvJvds9xAN6Vm9jCCIs--