From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59710) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZbhqP-0007GS-Gi for qemu-devel@nongnu.org; Tue, 15 Sep 2015 00:27:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZbhqN-00085s-QC for qemu-devel@nongnu.org; Tue, 15 Sep 2015 00:27:49 -0400 Date: Tue, 15 Sep 2015 14:26:06 +1000 From: David Gibson Message-ID: <20150915042606.GV2547@voom.fritz.box> References: <1441963021-31423-1-git-send-email-thuth@redhat.com> <20150914021506.GD2547@voom.fritz.box> <55F66A04.4010304@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="SidV/bKy1CyabPX8" Content-Disposition: inline In-Reply-To: <55F66A04.4010304@redhat.com> 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: Thomas Huth 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 --SidV/bKy1CyabPX8 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Sep 14, 2015 at 08:32:36AM +0200, Thomas Huth wrote: > On 14/09/15 04:15, David Gibson wrote: > > On Fri, Sep 11, 2015 at 11:17:01AM +0200, Thomas Huth wrote: > >> The PAPR interface defines a hypercall to pass high-quality > >> hardware generated random numbers to guests. Recent kernels can > >> already provide this hypercall to the guest if the right hardware > >> random number generator is available. But in case the user wants > >> to use another source like EGD, or QEMU is running with an older > >> kernel, we should also have this call in QEMU, so that guests that > >> do not support virtio-rng yet can get good random numbers, too. > >> > >> This patch now adds a new pseude-device to QEMU that either > >> directly provides this hypercall to the guest or is able to > >> enable the in-kernel hypercall if available. The in-kernel > >> hypercall can be enabled with the use-kvm property, e.g.: > >> > >> qemu-system-ppc64 -device spapr-rng,use-kvm=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. > ... > >> + > >> +#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. >=20 > Not sure whether I fully got your point here... both callback function > and main thread are calling an extern C-function, so the compiler should > not assume that the memory stays the same in the main thread...? I'm not talking about a compiler barrier: the callback will likely be invoked on a different CPU from the vcpu thread that invoked H_RANDOM, so on a weakly ordered arch like Power we need a real CPU memory barrier. > Anyway, I've tested the hypercall by implementing it in SLOF and calling > it a couple of times there to see that all bits in the result behave > randomly, so for me this is working fine. Right, I'd be almost certain anyway that qemu_sem_post() (actually likely the pthreads functions it invokes) will include the necessary barriers to stop accesses leaking outside the locked region. >=20 > >> +} > >> + > >> +/* 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. >=20 > I did not spot a macro a la "DEFINE_PROP_LINK" that could be used for > this. Do you see a possibility to define a link that way? No, like I say since virtio-rng does it that way I assume there's a reason. > > More importantly, this should probably be called "rng" not "backend" > > to match virtio-rng. >=20 > Since the device is already called "spapr-rng", i.e. has "rng" in its > name, I'd rather like to keep this as "backend" to make it clear that > you specify the backend this way. Hm, personally I'd weigh consistency with virtio-rng higher than the slightly confusing name. >=20 > >> +} > >> + > >> +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". >=20 > Right, makes sense, I'll update this logic. Ok. --=20 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 --SidV/bKy1CyabPX8 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJV953eAAoJEGw4ysog2bOSDjwQAJYwAcpQX1tgE1hofU9HJcGf q0TVV7J3UojfV3EXQLs7kWi23YXn5J6aYHIr6L8rFTxpHQREdsCA8Gu2frz0CRbs nONKTZ7nRVO7T2IJtO+62F/oaQG9nleP+2CsG/V3l+tm7n5eyWULLzcWnKYRqLIl JKd2t9tIO4I7/OItyE3F3bpZXIXG4dvLomZueN4YAVkhW4PE7FvB1TFZ/sJFAu+1 uEC/TIxgw295Ur6f0g5fsorzxwALYtxpObsiOISuquf1Lmr0RcybrSbpmwzsVD+1 7qozwdECguJhuOWldcwrB8U/tIDn2gM8sEZrQ6urI2KKYBpGdvIfV+zdSY4JS8Gn zza8BpPNXIg87E8odz14VCTZgdSzwSHj1C1AoopjphitGp6RQJyA6VL6jm/gLxr7 6kIhVENWrqsU8CDTI4NKEPwYaan0p/73IVpWSdae5fCrYA9KJycvFkUjnkO1VqJE 903X1DgYqmHsyOhC8DUraCTs5hzvlCvx4uJVJJ+LAhmXx0qdlvgpuf46hhrb80wB DjW+2A9Zxdxm+jxt/xOxG68ELTroFcdkIMx0o6Dq/1biz49gCZUoReKEd0vgFET1 4ib7teDHL5HHQGrFgCobgNgfwiDm6heYjFS0TlxBsfKSWTjplVKNKFjpykj3SCoA z9Jd6j6iKTL8+GmSU0sg =SPRP -----END PGP SIGNATURE----- --SidV/bKy1CyabPX8--