From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46416) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWjMC-0000aD-7o for qemu-devel@nongnu.org; Tue, 01 Sep 2015 07:04:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZWjM9-0004W3-1j for qemu-devel@nongnu.org; Tue, 01 Sep 2015 07:04:04 -0400 References: <1441046762-5788-1-git-send-email-thuth@redhat.com> <1441046762-5788-3-git-send-email-thuth@redhat.com> <20150901004753.GJ11475@voom.redhat.com> From: Thomas Huth Message-ID: <55E58619.8050907@redhat.com> Date: Tue, 1 Sep 2015 13:03:53 +0200 MIME-Version: 1.0 In-Reply-To: <20150901004753.GJ11475@voom.redhat.com> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="EDJffucOKLoKMnOqsgvQaWnlauKcColbp" Subject: Re: [Qemu-devel] [PATCH v2 2/2] ppc/spapr_hcall: Implement H_RANDOM hypercall in QEMU List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-devel@nongnu.org, agraf@suse.de, armbru@redhat.com, michael@ellerman.id.au, qemu-ppc@nongnu.org, amit.shah@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --EDJffucOKLoKMnOqsgvQaWnlauKcColbp Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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=3Drng-random ... >=20 > 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=3Drng-egd" to use the >> Entropy Gathering Daemon instead. >> >> Signed-off-by: Thomas Huth >> --- >> 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 =2E.. >> @@ -1472,7 +1473,7 @@ static void ppc_spapr_init(MachineState *machine= ) >> uint32_t initrd_base =3D 0; >> long kernel_size =3D 0, initrd_size =3D 0; >> long load_limit, fw_size; >> - bool kernel_le =3D false; >> + bool kernel_le =3D false, enable_h_random; >=20 > 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. =2E.. >> 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; >> } >> =20 >> +typedef struct HRandomData { >> + QemuSemaphore sem; >> + union { >> + uint64_t v64; >> + uint8_t v8[8]; >> + } val; >> + int received; >> +} HRandomData; >> + >> +static RndRandom *hrandom_rng; >=20 > 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 =3D dest; >> + >> + if (src && size > 0) { >> + memcpy(&hrcrdp->val.v8[hrcrdp->received], src, size); >=20 > 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 =3D 0 or src = =3D NULL, so I had to add this here to avoid crashes. But I can also add an assert(hrcrdp->received + size <=3D 8) here if you like, just to be sure. >> + hrcrdp->received +=3D size; >> + } >> + qemu_sem_post(&hrcrdp->sem); >=20 > 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 --EDJffucOKLoKMnOqsgvQaWnlauKcColbp 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) iQIcBAEBAgAGBQJV5YYaAAoJEC7Z13T+cC21wVcQAKX762a5Xwfe2jMzf87UZb5x WlpAhkr9751uG/R4dDjQ6Jke7gAbe1g7UqQOdlwUEdpwy7/1KfMlKab4ikqJHuOr 65jNmknG4KjWDLQi7yqwl0FqB5Wm/+DRpq5axVx9ZMAUaPO5lOHmOxLlqtvfa8MD lNgXgbfXJGESCKfIuItpqLXegNN1Zoq+ho+EOc6HdN+d/mDu23ddwxM6mpdRHuwL 6hQTn2RQ6KTn5XvBjo6OFUdUEfQCs1we4He6OcpK+DFpzTxSVGdY6B7qLGSGF5PO SI2/8Us5QyEroeW1nFywHMc8XoloM7Uj8CLzNAaQOQgsjJJvCyO6gxxU1W5VpnoA FA/hzbLc3lT6QW/uCzU5vdTAaHXh7Fs/RFuQFMRTo0uheol8BPrlghq0lf/wCoiB Au4oYWVbzPLSAwVBr9y9jzQaUsV0eglI1zAGYK1pjYuK1gZE2FvW6SOvTEQ20fAK rby53pSVMNnw90ybxBCpqlRU/NtNWFeqwtVhAkgHaphEZT2JiwrypZQ3TRl7gNFM YLAR/SLkEyaehLGOrM7IGGXzXfNSlookxDwVnNoF4Ocv51mlUfptexcBKGOdAWFN rjJLJBmFZcwbHAbLGfyGx/CQ87AgiT2EyoVlWzQw02K5xNvB5E1mk6WwtQKp40x8 ifsCBW4ME2nPHdmRAr7A =kiKM -----END PGP SIGNATURE----- --EDJffucOKLoKMnOqsgvQaWnlauKcColbp--