| Message ID | 1470591415-3268-3-git-send-email-nikunj@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 2016-08-07 at 23:06 +0530, Nikunj A Dadhania wrote: > +target_ulong helper_darn(uint32_t l) > +{ > + target_ulong r = UINT64_MAX; > + > + if (l <= 2) { > + do { > + r = random() * random(); > + r &= l ? UINT64_MAX : UINT32_MAX; > + } while (r == UINT64_MAX); > + } > + > + return r; > +} > #endif Isn't this a bit week ? Look at the implementation of H_RANDOM... Cheers, Ben.
Benjamin Herrenschmidt <benh@kernel.crashing.org> writes: > On Sun, 2016-08-07 at 23:06 +0530, Nikunj A Dadhania wrote: >> +target_ulong helper_darn(uint32_t l) >> +{ >> + target_ulong r = UINT64_MAX; >> + >> + if (l <= 2) { >> + do { >> + r = random() * random(); >> + r &= l ? UINT64_MAX : UINT32_MAX; >> + } while (r == UINT64_MAX); >> + } >> + >> + return r; >> +} >> #endif > > Isn't this a bit week ? Look at the implementation of H_RANDOM... Sure, will have a look. Regards, Nikunj
On Mon, 2016-08-08 at 07:22 +0530, Nikunj A Dadhania wrote: > > > Isn't this a bit week ? Look at the implementation of H_RANDOM... > > Sure, will have a look. And if course I meant "weak" :-) That's what happens when I reply before breakfast ! Cheers, Ben.
Benjamin Herrenschmidt <benh@kernel.crashing.org> writes: > On Mon, 2016-08-08 at 07:22 +0530, Nikunj A Dadhania wrote: >> >> > Isn't this a bit week ? Look at the implementation of H_RANDOM... >> >> Sure, will have a look. > > And if course I meant "weak" :-) That's what happens when I reply > before breakfast ! I guessed that :-) Regards Nikunj
On 08/07/2016 11:06 PM, Nikunj A Dadhania wrote: > +target_ulong helper_darn(uint32_t l) > +{ > + target_ulong r = UINT64_MAX; > + > + if (l <= 2) { > + do { > + r = random() * random(); > + r &= l ? UINT64_MAX : UINT32_MAX; > + } while (r == UINT64_MAX); > + } > + > + return r; > +} Without considering the rng, I think you should split this into darn32 and darn64 based on L in translate.c. You can diagnose l==2 in translate.c as well. r~
Richard Henderson <rth@twiddle.net> writes: > On 08/07/2016 11:06 PM, Nikunj A Dadhania wrote: >> +target_ulong helper_darn(uint32_t l) >> +{ >> + target_ulong r = UINT64_MAX; >> + >> + if (l <= 2) { >> + do { >> + r = random() * random(); >> + r &= l ? UINT64_MAX : UINT32_MAX; >> + } while (r == UINT64_MAX); >> + } >> + >> + return r; >> +} > > Without considering the rng, I think you should split this into darn32 and > darn64 based on L in translate.c. You can diagnose l==2 in translate.c as well. Sure. Regards Nikunj
On Mon, Aug 08, 2016 at 07:33:37AM +1000, Benjamin Herrenschmidt wrote: > On Sun, 2016-08-07 at 23:06 +0530, Nikunj A Dadhania wrote: > > +target_ulong helper_darn(uint32_t l) > > +{ > > + target_ulong r = UINT64_MAX; > > + > > + if (l <= 2) { > > + do { > > + r = random() * random(); > > + r &= l ? UINT64_MAX : UINT32_MAX; > > + } while (r == UINT64_MAX); > > + } > > + > > + return r; > > +} > > #endif > > Isn't this a bit week ? Look at the implementation of H_RANDOM... Indeed, you should be using the rng backend that H_RANDOM, virtio-rng and the Intel random number instruction all use. But, worse than that: even if random() was a good RNG, I'm pretty sure than although random() * random() will give you a random number with twice as many bits as random() alone, it won't be uniformly distributed. That's probably not what you want.
David Gibson <david@gibson.dropbear.id.au> writes: > [ Unknown signature status ] > On Mon, Aug 08, 2016 at 07:33:37AM +1000, Benjamin Herrenschmidt wrote: >> On Sun, 2016-08-07 at 23:06 +0530, Nikunj A Dadhania wrote: >> > +target_ulong helper_darn(uint32_t l) >> > +{ >> > + target_ulong r = UINT64_MAX; >> > + >> > + if (l <= 2) { >> > + do { >> > + r = random() * random(); >> > + r &= l ? UINT64_MAX : UINT32_MAX; >> > + } while (r == UINT64_MAX); >> > + } >> > + >> > + return r; >> > +} >> > #endif >> >> Isn't this a bit week ? Look at the implementation of H_RANDOM... > > Indeed, you should be using the rng backend that H_RANDOM, virtio-rng > and the Intel random number instruction all use. I was looking at implementing this, AFAIU, I have to get a new RNG object in the initialization routine. We would need an instance of this per machine. So for pseries I can add in ppc_spapr_init(). I am not sure in case of linux-user where should this be initialized. One other place was init_proc_POWER9(), but that will be per cpu and member of CPUPPCState structure. Advantage is it will work for system emulation and linux-user both and we would not need a lock. > > But, worse than that: even if random() was a good RNG, I'm pretty sure > than although random() * random() will give you a random number with > twice as many bits as random() alone, it won't be uniformly > distributed. That's probably not what you want. Right, I had seen that issue. Regards, Nikunj
Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes: > David Gibson <david@gibson.dropbear.id.au> writes: > >> [ Unknown signature status ] >> On Mon, Aug 08, 2016 at 07:33:37AM +1000, Benjamin Herrenschmidt wrote: >>> On Sun, 2016-08-07 at 23:06 +0530, Nikunj A Dadhania wrote: >>> > +target_ulong helper_darn(uint32_t l) >>> > +{ >>> > + target_ulong r = UINT64_MAX; >>> > + >>> > + if (l <= 2) { >>> > + do { >>> > + r = random() * random(); >>> > + r &= l ? UINT64_MAX : UINT32_MAX; >>> > + } while (r == UINT64_MAX); >>> > + } >>> > + >>> > + return r; >>> > +} >>> > #endif >>> >>> Isn't this a bit week ? Look at the implementation of H_RANDOM... >> >> Indeed, you should be using the rng backend that H_RANDOM, virtio-rng >> and the Intel random number instruction all use. Can you point me to the intel instruction, I couldn't get rdrand implementation. > I was looking at implementing this, AFAIU, I have to get a new RNG > object in the initialization routine. We would need an instance of this > per machine. So for pseries I can add in ppc_spapr_init(). I am not sure > in case of linux-user where should this be initialized. > > One other place was init_proc_POWER9(), but that will be per cpu and > member of CPUPPCState structure. Advantage is it will work for system > emulation and linux-user both and we would not need a lock. More issues here. Random backend is not compiled for linux-user, adding that wasn't difficult, but then rng_backend_request_entropy() uses qemu_set_fd_handler() which is a stub for linux-user (stubs/set-fd-handler.c) Regards, Nikunj
On Tue, Aug 09, 2016 at 02:47:46PM +0530, Nikunj A Dadhania wrote: > Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes: > > > David Gibson <david@gibson.dropbear.id.au> writes: > > > >> [ Unknown signature status ] > >> On Mon, Aug 08, 2016 at 07:33:37AM +1000, Benjamin Herrenschmidt wrote: > >>> On Sun, 2016-08-07 at 23:06 +0530, Nikunj A Dadhania wrote: > >>> > +target_ulong helper_darn(uint32_t l) > >>> > +{ > >>> > + target_ulong r = UINT64_MAX; > >>> > + > >>> > + if (l <= 2) { > >>> > + do { > >>> > + r = random() * random(); > >>> > + r &= l ? UINT64_MAX : UINT32_MAX; > >>> > + } while (r == UINT64_MAX); > >>> > + } > >>> > + > >>> > + return r; > >>> > +} > >>> > #endif > >>> > >>> Isn't this a bit week ? Look at the implementation of H_RANDOM... > >> > >> Indeed, you should be using the rng backend that H_RANDOM, virtio-rng > >> and the Intel random number instruction all use. > > Can you point me to the intel instruction, I couldn't get rdrand > implementation. Ah.. turns out no. I'd assumed it was there and used the same backend as virtio-rng and H_RANDOM, but I hadn't actually looked at the code, and now that I'm trying I can't see it either. > > > I was looking at implementing this, AFAIU, I have to get a new RNG > > object in the initialization routine. We would need an instance of this > > per machine. So for pseries I can add in ppc_spapr_init(). I am not sure > > in case of linux-user where should this be initialized. > > > > One other place was init_proc_POWER9(), but that will be per cpu and > > member of CPUPPCState structure. Advantage is it will work for system > > emulation and linux-user both and we would not need a lock. > > More issues here. Random backend is not compiled for linux-user, adding > that wasn't difficult, but then rng_backend_request_entropy() uses > qemu_set_fd_handler() which is a stub for linux-user > (stubs/set-fd-handler.c)7 Ah.. yeah, not sure how we'll need to handle that.
David Gibson <david@gibson.dropbear.id.au> writes: > [ Unknown signature status ] > On Tue, Aug 09, 2016 at 02:47:46PM +0530, Nikunj A Dadhania wrote: >> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes: >> >> > David Gibson <david@gibson.dropbear.id.au> writes: >> > >> >> [ Unknown signature status ] >> >> On Mon, Aug 08, 2016 at 07:33:37AM +1000, Benjamin Herrenschmidt wrote: >> >>> On Sun, 2016-08-07 at 23:06 +0530, Nikunj A Dadhania wrote: >> >>> > +target_ulong helper_darn(uint32_t l) >> >>> > +{ >> >>> > + target_ulong r = UINT64_MAX; >> >>> > + >> >>> > + if (l <= 2) { >> >>> > + do { >> >>> > + r = random() * random(); >> >>> > + r &= l ? UINT64_MAX : UINT32_MAX; >> >>> > + } while (r == UINT64_MAX); >> >>> > + } >> >>> > + >> >>> > + return r; >> >>> > +} >> >>> > #endif >> >>> >> >>> Isn't this a bit week ? Look at the implementation of H_RANDOM... >> >> >> >> Indeed, you should be using the rng backend that H_RANDOM, virtio-rng >> >> and the Intel random number instruction all use. >> >> Can you point me to the intel instruction, I couldn't get rdrand >> implementation. > > Ah.. turns out no. I'd assumed it was there and used the same backend > as virtio-rng and H_RANDOM, but I hadn't actually looked at the code, > and now that I'm trying I can't see it either. > >> >> > I was looking at implementing this, AFAIU, I have to get a new RNG >> > object in the initialization routine. We would need an instance of this >> > per machine. So for pseries I can add in ppc_spapr_init(). I am not sure >> > in case of linux-user where should this be initialized. >> > >> > One other place was init_proc_POWER9(), but that will be per cpu and >> > member of CPUPPCState structure. Advantage is it will work for system >> > emulation and linux-user both and we would not need a lock. >> >> More issues here. Random backend is not compiled for linux-user, adding >> that wasn't difficult, but then rng_backend_request_entropy() uses >> qemu_set_fd_handler() which is a stub for linux-user >> (stubs/set-fd-handler.c)7 > > > Ah.. yeah, not sure how we'll need to handle that. I have sent updated patch, reading from /dev/random. Not sure if that is allowed in tcg. Works fine though. Regards Nikunj
On Fri, Aug 12, 2016 at 12:13:49PM +0530, Nikunj A Dadhania wrote: > David Gibson <david@gibson.dropbear.id.au> writes: > > > [ Unknown signature status ] > > On Tue, Aug 09, 2016 at 02:47:46PM +0530, Nikunj A Dadhania wrote: > >> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes: > >> > >> > David Gibson <david@gibson.dropbear.id.au> writes: > >> > > >> >> [ Unknown signature status ] > >> >> On Mon, Aug 08, 2016 at 07:33:37AM +1000, Benjamin Herrenschmidt wrote: > >> >>> On Sun, 2016-08-07 at 23:06 +0530, Nikunj A Dadhania wrote: > >> >>> > +target_ulong helper_darn(uint32_t l) > >> >>> > +{ > >> >>> > + target_ulong r = UINT64_MAX; > >> >>> > + > >> >>> > + if (l <= 2) { > >> >>> > + do { > >> >>> > + r = random() * random(); > >> >>> > + r &= l ? UINT64_MAX : UINT32_MAX; > >> >>> > + } while (r == UINT64_MAX); > >> >>> > + } > >> >>> > + > >> >>> > + return r; > >> >>> > +} > >> >>> > #endif > >> >>> > >> >>> Isn't this a bit week ? Look at the implementation of H_RANDOM... > >> >> > >> >> Indeed, you should be using the rng backend that H_RANDOM, virtio-rng > >> >> and the Intel random number instruction all use. > >> > >> Can you point me to the intel instruction, I couldn't get rdrand > >> implementation. > > > > Ah.. turns out no. I'd assumed it was there and used the same backend > > as virtio-rng and H_RANDOM, but I hadn't actually looked at the code, > > and now that I'm trying I can't see it either. > > > >> > >> > I was looking at implementing this, AFAIU, I have to get a new RNG > >> > object in the initialization routine. We would need an instance of this > >> > per machine. So for pseries I can add in ppc_spapr_init(). I am not sure > >> > in case of linux-user where should this be initialized. > >> > > >> > One other place was init_proc_POWER9(), but that will be per cpu and > >> > member of CPUPPCState structure. Advantage is it will work for system > >> > emulation and linux-user both and we would not need a lock. > >> > >> More issues here. Random backend is not compiled for linux-user, adding > >> that wasn't difficult, but then rng_backend_request_entropy() uses > >> qemu_set_fd_handler() which is a stub for linux-user > >> (stubs/set-fd-handler.c)7 > > > > > > Ah.. yeah, not sure how we'll need to handle that. > > I have sent updated patch, reading from /dev/random. Not sure if that is > allowed in tcg. Works fine though. Hrm. From a helper, there's nothing inherently wrong with reading from /dev/random, AFAIK. However, it may not work on non-Linux hosts, and doesn't let you connect it to urandom instead which probably make it unsuitable to be merged.
David Gibson <david@gibson.dropbear.id.au> writes: > [ Unknown signature status ] > On Fri, Aug 12, 2016 at 12:13:49PM +0530, Nikunj A Dadhania wrote: >> David Gibson <david@gibson.dropbear.id.au> writes: >> >> > [ Unknown signature status ] >> > On Tue, Aug 09, 2016 at 02:47:46PM +0530, Nikunj A Dadhania wrote: >> >> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes: >> >> >> >> > David Gibson <david@gibson.dropbear.id.au> writes: >> >> > >> >> >> [ Unknown signature status ] >> >> >> On Mon, Aug 08, 2016 at 07:33:37AM +1000, Benjamin Herrenschmidt wrote: >> >> >>> On Sun, 2016-08-07 at 23:06 +0530, Nikunj A Dadhania wrote: >> >> >>> > +target_ulong helper_darn(uint32_t l) >> >> >>> > +{ >> >> >>> > + target_ulong r = UINT64_MAX; >> >> >>> > + >> >> >>> > + if (l <= 2) { >> >> >>> > + do { >> >> >>> > + r = random() * random(); >> >> >>> > + r &= l ? UINT64_MAX : UINT32_MAX; >> >> >>> > + } while (r == UINT64_MAX); >> >> >>> > + } >> >> >>> > + >> >> >>> > + return r; >> >> >>> > +} >> >> >>> > #endif >> >> >>> >> >> >>> Isn't this a bit week ? Look at the implementation of H_RANDOM... >> >> >> >> >> >> Indeed, you should be using the rng backend that H_RANDOM, virtio-rng >> >> >> and the Intel random number instruction all use. >> >> >> >> Can you point me to the intel instruction, I couldn't get rdrand >> >> implementation. >> > >> > Ah.. turns out no. I'd assumed it was there and used the same backend >> > as virtio-rng and H_RANDOM, but I hadn't actually looked at the code, >> > and now that I'm trying I can't see it either. >> > >> >> >> >> > I was looking at implementing this, AFAIU, I have to get a new RNG >> >> > object in the initialization routine. We would need an instance of this >> >> > per machine. So for pseries I can add in ppc_spapr_init(). I am not sure >> >> > in case of linux-user where should this be initialized. >> >> > >> >> > One other place was init_proc_POWER9(), but that will be per cpu and >> >> > member of CPUPPCState structure. Advantage is it will work for system >> >> > emulation and linux-user both and we would not need a lock. >> >> >> >> More issues here. Random backend is not compiled for linux-user, adding >> >> that wasn't difficult, but then rng_backend_request_entropy() uses >> >> qemu_set_fd_handler() which is a stub for linux-user >> >> (stubs/set-fd-handler.c)7 >> > >> > >> > Ah.. yeah, not sure how we'll need to handle that. >> >> I have sent updated patch, reading from /dev/random. Not sure if that is >> allowed in tcg. Works fine though. > > Hrm. From a helper, there's nothing inherently wrong with reading > from /dev/random, AFAIK. However, it may not work on non-Linux hosts, > and doesn't let you connect it to urandom instead which probably make > it unsuitable to be merged. Maybe two implementation, one weaker one using random() for non-linux hosts, other using /dev/random ? Regards Nikunj
On 12.08.2016 08:43, Nikunj A Dadhania wrote: > David Gibson <david@gibson.dropbear.id.au> writes: > >> [ Unknown signature status ] >> On Tue, Aug 09, 2016 at 02:47:46PM +0530, Nikunj A Dadhania wrote: >>> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes: >>> >>>> David Gibson <david@gibson.dropbear.id.au> writes: >>>> >>>>> [ Unknown signature status ] >>>>> On Mon, Aug 08, 2016 at 07:33:37AM +1000, Benjamin Herrenschmidt wrote: >>>>>> On Sun, 2016-08-07 at 23:06 +0530, Nikunj A Dadhania wrote: >>>>>>> +target_ulong helper_darn(uint32_t l) >>>>>>> +{ >>>>>>> + target_ulong r = UINT64_MAX; >>>>>>> + >>>>>>> + if (l <= 2) { >>>>>>> + do { >>>>>>> + r = random() * random(); >>>>>>> + r &= l ? UINT64_MAX : UINT32_MAX; >>>>>>> + } while (r == UINT64_MAX); >>>>>>> + } >>>>>>> + >>>>>>> + return r; >>>>>>> +} >>>>>>> #endif >>>>>> >>>>>> Isn't this a bit week ? Look at the implementation of H_RANDOM... >>>>> >>>>> Indeed, you should be using the rng backend that H_RANDOM, virtio-rng >>>>> and the Intel random number instruction all use. >>> >>> Can you point me to the intel instruction, I couldn't get rdrand >>> implementation. >> >> Ah.. turns out no. I'd assumed it was there and used the same backend >> as virtio-rng and H_RANDOM, but I hadn't actually looked at the code, >> and now that I'm trying I can't see it either. >> >>> >>>> I was looking at implementing this, AFAIU, I have to get a new RNG >>>> object in the initialization routine. We would need an instance of this >>>> per machine. So for pseries I can add in ppc_spapr_init(). I am not sure >>>> in case of linux-user where should this be initialized. >>>> >>>> One other place was init_proc_POWER9(), but that will be per cpu and >>>> member of CPUPPCState structure. Advantage is it will work for system >>>> emulation and linux-user both and we would not need a lock. >>> >>> More issues here. Random backend is not compiled for linux-user, adding >>> that wasn't difficult, but then rng_backend_request_entropy() uses >>> qemu_set_fd_handler() which is a stub for linux-user >>> (stubs/set-fd-handler.c)7 >> >> >> Ah.. yeah, not sure how we'll need to handle that. > > I have sent updated patch, reading from /dev/random. Not sure if that is > allowed in tcg. Works fine though. You can not rely on /dev/random for this job, since it might block. So your guest would stop executing when there is not enough random data available in the host, and I think that's quite a bad behavior... Thomas
Thomas Huth <thuth@redhat.com> writes: > On 12.08.2016 08:43, Nikunj A Dadhania wrote: >> David Gibson <david@gibson.dropbear.id.au> writes: >> >>> [ Unknown signature status ] >>> On Tue, Aug 09, 2016 at 02:47:46PM +0530, Nikunj A Dadhania wrote: >>>> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes: >>>> >>>>> David Gibson <david@gibson.dropbear.id.au> writes: >>>>> >>>>>> [ Unknown signature status ] >>>>>> On Mon, Aug 08, 2016 at 07:33:37AM +1000, Benjamin Herrenschmidt wrote: >>>>>>> On Sun, 2016-08-07 at 23:06 +0530, Nikunj A Dadhania wrote: >>>>>>>> +target_ulong helper_darn(uint32_t l) >>>>>>>> +{ >>>>>>>> + target_ulong r = UINT64_MAX; >>>>>>>> + >>>>>>>> + if (l <= 2) { >>>>>>>> + do { >>>>>>>> + r = random() * random(); >>>>>>>> + r &= l ? UINT64_MAX : UINT32_MAX; >>>>>>>> + } while (r == UINT64_MAX); >>>>>>>> + } >>>>>>>> + >>>>>>>> + return r; >>>>>>>> +} >>>>>>>> #endif >>>>>>> >>>>>>> Isn't this a bit week ? Look at the implementation of H_RANDOM... >>>>>> >>>>>> Indeed, you should be using the rng backend that H_RANDOM, virtio-rng >>>>>> and the Intel random number instruction all use. >>>> >>>> Can you point me to the intel instruction, I couldn't get rdrand >>>> implementation. >>> >>> Ah.. turns out no. I'd assumed it was there and used the same backend >>> as virtio-rng and H_RANDOM, but I hadn't actually looked at the code, >>> and now that I'm trying I can't see it either. >>> >>>> >>>>> I was looking at implementing this, AFAIU, I have to get a new RNG >>>>> object in the initialization routine. We would need an instance of this >>>>> per machine. So for pseries I can add in ppc_spapr_init(). I am not sure >>>>> in case of linux-user where should this be initialized. >>>>> >>>>> One other place was init_proc_POWER9(), but that will be per cpu and >>>>> member of CPUPPCState structure. Advantage is it will work for system >>>>> emulation and linux-user both and we would not need a lock. >>>> >>>> More issues here. Random backend is not compiled for linux-user, adding >>>> that wasn't difficult, but then rng_backend_request_entropy() uses >>>> qemu_set_fd_handler() which is a stub for linux-user >>>> (stubs/set-fd-handler.c)7 >>> >>> >>> Ah.. yeah, not sure how we'll need to handle that. >> >> I have sent updated patch, reading from /dev/random. Not sure if that is >> allowed in tcg. Works fine though. > > You can not rely on /dev/random for this job, since it might block. So > your guest would stop executing when there is not enough random data > available in the host, and I think that's quite a bad behavior... Hmm.. rng-random does use this, but it might have way to time out probably: backends/rng-random.c: s->filename = g_strdup("/dev/random"); Regards Nikunj
On 12.08.2016 09:39, Nikunj A Dadhania wrote: > Thomas Huth <thuth@redhat.com> writes: > >> On 12.08.2016 08:43, Nikunj A Dadhania wrote: >>> David Gibson <david@gibson.dropbear.id.au> writes: >>> >>>> [ Unknown signature status ] >>>> On Tue, Aug 09, 2016 at 02:47:46PM +0530, Nikunj A Dadhania wrote: >>>>> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes: >>>>> >>>>>> David Gibson <david@gibson.dropbear.id.au> writes: >>>>>> >>>>>>> [ Unknown signature status ] >>>>>>> On Mon, Aug 08, 2016 at 07:33:37AM +1000, Benjamin Herrenschmidt wrote: >>>>>>>> On Sun, 2016-08-07 at 23:06 +0530, Nikunj A Dadhania wrote: >>>>>>>>> +target_ulong helper_darn(uint32_t l) >>>>>>>>> +{ >>>>>>>>> + target_ulong r = UINT64_MAX; >>>>>>>>> + >>>>>>>>> + if (l <= 2) { >>>>>>>>> + do { >>>>>>>>> + r = random() * random(); >>>>>>>>> + r &= l ? UINT64_MAX : UINT32_MAX; >>>>>>>>> + } while (r == UINT64_MAX); >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + return r; >>>>>>>>> +} >>>>>>>>> #endif >>>>>>>> >>>>>>>> Isn't this a bit week ? Look at the implementation of H_RANDOM... >>>>>>> >>>>>>> Indeed, you should be using the rng backend that H_RANDOM, virtio-rng >>>>>>> and the Intel random number instruction all use. >>>>> >>>>> Can you point me to the intel instruction, I couldn't get rdrand >>>>> implementation. >>>> >>>> Ah.. turns out no. I'd assumed it was there and used the same backend >>>> as virtio-rng and H_RANDOM, but I hadn't actually looked at the code, >>>> and now that I'm trying I can't see it either. >>>> >>>>> >>>>>> I was looking at implementing this, AFAIU, I have to get a new RNG >>>>>> object in the initialization routine. We would need an instance of this >>>>>> per machine. So for pseries I can add in ppc_spapr_init(). I am not sure >>>>>> in case of linux-user where should this be initialized. >>>>>> >>>>>> One other place was init_proc_POWER9(), but that will be per cpu and >>>>>> member of CPUPPCState structure. Advantage is it will work for system >>>>>> emulation and linux-user both and we would not need a lock. >>>>> >>>>> More issues here. Random backend is not compiled for linux-user, adding >>>>> that wasn't difficult, but then rng_backend_request_entropy() uses >>>>> qemu_set_fd_handler() which is a stub for linux-user >>>>> (stubs/set-fd-handler.c)7 >>>> >>>> >>>> Ah.. yeah, not sure how we'll need to handle that. >>> >>> I have sent updated patch, reading from /dev/random. Not sure if that is >>> allowed in tcg. Works fine though. >> >> You can not rely on /dev/random for this job, since it might block. So >> your guest would stop executing when there is not enough random data >> available in the host, and I think that's quite a bad behavior... > > Hmm.. rng-random does use this, but it might have way to time out probably: > > backends/rng-random.c: s->filename = g_strdup("/dev/random"); This is only the default value, which is likely suitable for virtio-rng, but not for something like this instruction (or the H_RANDOM hypercall). You can set the filename property to another file when instantiating it, like: qemu-system-xxx ... -object rng-random,filename=/dev/hwrng,id=rng0 ... That's the whole point these backends - you give the users the possibility to chose the right source of entropy. For example, on the POWER8 machine that I have here, /dev/random blocks after a couple of bytes, you can see that easily by typing something like "hexdump /dev/random". It only delivers more random data when I run "rngd -r /dev/hwrng" in the background. Thomas
Thomas Huth <thuth@redhat.com> writes: >>> You can not rely on /dev/random for this job, since it might block. So >>> your guest would stop executing when there is not enough random data >>> available in the host, and I think that's quite a bad behavior... >> >> Hmm.. rng-random does use this, but it might have way to time out probably: >> >> backends/rng-random.c: s->filename = g_strdup("/dev/random"); > > This is only the default value, which is likely suitable for > virtio-rng, Right, we will need to find maybe an algorithm that can give us sufficient distribution, or use random() with time-of-day and get some function. MIPS does have some thing in cpu_mips_get_random(). Its for 32-bit, probably can be extended to 64-bit. Mathematically, I am not sure. :-) > but not for something like this instruction (or the H_RANDOM hypercall). > You can set the filename property to another file when instantiating it, > like: > > qemu-system-xxx ... -object rng-random,filename=/dev/hwrng,id=rng0 ... > > That's the whole point these backends - you give the users the > possibility to chose the right source of entropy. > > For example, on the POWER8 machine that I have here, /dev/random blocks > after a couple of bytes, you can see that easily by typing something > like "hexdump /dev/random". It only delivers more random data when I run > "rngd -r /dev/hwrng" in the background. Regards, Nikunj
On 12.08.2016 10:41, Nikunj A Dadhania wrote: > Thomas Huth <thuth@redhat.com> writes: >>>> You can not rely on /dev/random for this job, since it might block. So >>>> your guest would stop executing when there is not enough random data >>>> available in the host, and I think that's quite a bad behavior... >>> >>> Hmm.. rng-random does use this, but it might have way to time out probably: >>> >>> backends/rng-random.c: s->filename = g_strdup("/dev/random"); >> >> This is only the default value, which is likely suitable for >> virtio-rng, > > Right, we will need to find maybe an algorithm that can give us > sufficient distribution, or use random() with time-of-day and get some > function. First, hands off rand() and random()! These are certainly not suited for implementing an opcode that is likely thought to deliver cryptographically suitable random data! This topic is really not that easy, I had the same problems also when thinking about the implementation of H_RANDOM. You need a source that gives you cryptographically suitable data, you've got to make sure that your algorithm does not block the guest, and you've got to do it in a somewhat system-independent manner (i.e. QEMU should still run on Windows and Mac OS X afterwards). That's why I came to the conclusion that it's best to use the rng backends for this job when implementing the H_RANDOM stuff, and let the users decide which source is best suited on their system. > MIPS does have some thing in cpu_mips_get_random(). Its for 32-bit, > probably can be extended to 64-bit. Mathematically, I am not sure. :-) That's certainly not an algorithm which is suitable for crypto data! >> but not for something like this instruction (or the H_RANDOM hypercall). >> You can set the filename property to another file when instantiating it, >> like: >> >> qemu-system-xxx ... -object rng-random,filename=/dev/hwrng,id=rng0 ... >> >> That's the whole point these backends - you give the users the >> possibility to chose the right source of entropy. Of course this is getting ugly here, since a CPU instruction is not as optional as the H_RANDOM hypercall for example. So I basically see to ways to follow here: 1) Implement it similar to the H_RANDOM hypercall, i.e. make it optional and let the user decide the right source of entropy with a "-object rng-random" backend. If such a backend has not been given on the command line, let the "darn" instruction simply always return 0xFFFFFFFFFFFFFFFF to signal an error condition - the guest is then forced to use another way to get random data instead. 2) Try to introduce a generic get_crypto_random_data() function to QEMU that can be called to get cryptographically suitable random data in any case. The function then should probe for /dev/random, /dev/hwrng or other suitable sources on its own when being called for the first time (might be some work to get this running on Windows and Mac OS X, too). You then still have to deal with a blocking /dev/random device, though, so maybe the data with the good entropy should not be returned directly but rather used to seed a good deterministic RNG instead, like the one from the glib (have a look at the g_rand_int() function and friends). Anyway, someone with a good knowledge of crypto stuff should review such an implementation first before we include that, I think. Thomas
Thomas Huth <thuth@redhat.com> writes: > On 12.08.2016 10:41, Nikunj A Dadhania wrote: >> Thomas Huth <thuth@redhat.com> writes: >>>>> You can not rely on /dev/random for this job, since it might block. So >>>>> your guest would stop executing when there is not enough random data >>>>> available in the host, and I think that's quite a bad behavior... >>>> >>>> Hmm.. rng-random does use this, but it might have way to time out probably: >>>> >>>> backends/rng-random.c: s->filename = g_strdup("/dev/random"); >>> >>> This is only the default value, which is likely suitable for >>> virtio-rng, >> >> Right, we will need to find maybe an algorithm that can give us >> sufficient distribution, or use random() with time-of-day and get some >> function. > > First, hands off rand() and random()! These are certainly not suited for > implementing an opcode that is likely thought to deliver > cryptographically suitable random data! Sure :-) > This topic is really not that easy, I had the same problems also when > thinking about the implementation of H_RANDOM. You need a source that > gives you cryptographically suitable data, you've got to make sure that > your algorithm does not block the guest, and you've got to do it in a > somewhat system-independent manner (i.e. QEMU should still run on > Windows and Mac OS X afterwards). That's why I came to the conclusion > that it's best to use the rng backends for this job when implementing > the H_RANDOM stuff, and let the users decide which source is best suited > on their system. Right, that was the first attempt, as we need to cater to linux-user as well, that does not have bells and whistles of system. Many functionality missing, right from the rng-backend isnt part of the linux-user emulator. >> MIPS does have some thing in cpu_mips_get_random(). Its for 32-bit, >> probably can be extended to 64-bit. Mathematically, I am not sure. :-) > > That's certainly not an algorithm which is suitable for crypto data! Thanks for confirming. >>> but not for something like this instruction (or the H_RANDOM hypercall). >>> You can set the filename property to another file when instantiating it, >>> like: >>> >>> qemu-system-xxx ... -object rng-random,filename=/dev/hwrng,id=rng0 ... >>> >>> That's the whole point these backends - you give the users the >>> possibility to chose the right source of entropy. > > Of course this is getting ugly here, since a CPU instruction is not as > optional as the H_RANDOM hypercall for example. > So I basically see to ways to follow here: > > 1) Implement it similar to the H_RANDOM hypercall, i.e. make it optional > and let the user decide the right source of entropy with a "-object > rng-random" backend. If such a backend has not been given on the command > line, let the "darn" instruction simply always return 0xFFFFFFFFFFFFFFFF > to signal an error condition - the guest is then forced to use another > way to get random data instead. We might need to differentiate between linux-user and system maybe to use the rng backend. > 2) Try to introduce a generic get_crypto_random_data() function to QEMU > that can be called to get cryptographically suitable random data in any > case. The function then should probe for /dev/random, /dev/hwrng or > other suitable sources on its own when being called for the first time Yes, I can try that. > (might be some work to get this running on Windows and Mac OS X, too). I am not to sure about this. Would need some help. > You then still have to deal with a blocking /dev/random device, though, > so maybe the data with the good entropy should not be returned directly > but rather used to seed a good deterministic RNG instead, like the one > from the glib (have a look at the g_rand_int() function and friends). > Anyway, someone with a good knowledge of crypto stuff should review such > an implementation first before we include that, I think. I can drop this from my patch series, until we have clarity here. Regards Nikunj
On 08/12/2016 10:51 AM, Nikunj A Dadhania wrote:
> I can drop this from my patch series, until we have clarity here.
Why don't you just drop the internals from helper_darn*, leaving both to always
return -1. That way we can keep the translation work that you did.
r~
Richard Henderson <rth@twiddle.net> writes: > On 08/12/2016 10:51 AM, Nikunj A Dadhania wrote: >> I can drop this from my patch series, until we have clarity here. > > Why don't you just drop the internals from helper_darn*, leaving both to always > return -1. That way we can keep the translation work that you did. Sure, will add it with some comments. Regards, Nikunj
On Fri, Aug 12, 2016 at 11:29:17AM +0200, Thomas Huth wrote: > On 12.08.2016 10:41, Nikunj A Dadhania wrote: > > Thomas Huth <thuth@redhat.com> writes: > >>>> You can not rely on /dev/random for this job, since it might block. So > >>>> your guest would stop executing when there is not enough random data > >>>> available in the host, and I think that's quite a bad behavior... > >>> > >>> Hmm.. rng-random does use this, but it might have way to time out probably: > >>> > >>> backends/rng-random.c: s->filename = g_strdup("/dev/random"); > >> > >> This is only the default value, which is likely suitable for > >> virtio-rng, > > > > Right, we will need to find maybe an algorithm that can give us > > sufficient distribution, or use random() with time-of-day and get some > > function. > > First, hands off rand() and random()! These are certainly not suited for > implementing an opcode that is likely thought to deliver > cryptographically suitable random data! > > This topic is really not that easy, I had the same problems also when > thinking about the implementation of H_RANDOM. You need a source that > gives you cryptographically suitable data, you've got to make sure that > your algorithm does not block the guest, and you've got to do it in a > somewhat system-independent manner (i.e. QEMU should still run on > Windows and Mac OS X afterwards). That's why I came to the conclusion > that it's best to use the rng backends for this job when implementing > the H_RANDOM stuff, and let the users decide which source is best suited > on their system. > > > MIPS does have some thing in cpu_mips_get_random(). Its for 32-bit, > > probably can be extended to 64-bit. Mathematically, I am not sure. :-) > > That's certainly not an algorithm which is suitable for crypto data! > > >> but not for something like this instruction (or the H_RANDOM hypercall). > >> You can set the filename property to another file when instantiating it, > >> like: > >> > >> qemu-system-xxx ... -object rng-random,filename=/dev/hwrng,id=rng0 ... > >> > >> That's the whole point these backends - you give the users the > >> possibility to chose the right source of entropy. > > Of course this is getting ugly here, since a CPU instruction is not as > optional as the H_RANDOM hypercall for example. > So I basically see to ways to follow here: > > 1) Implement it similar to the H_RANDOM hypercall, i.e. make it optional > and let the user decide the right source of entropy with a "-object > rng-random" backend. If such a backend has not been given on the command > line, let the "darn" instruction simply always return 0xFFFFFFFFFFFFFFFF > to signal an error condition - the guest is then forced to use another > way to get random data instead. > > 2) Try to introduce a generic get_crypto_random_data() function to QEMU > that can be called to get cryptographically suitable random data in any > case. The function then should probe for /dev/random, /dev/hwrng or > other suitable sources on its own when being called for the first time > (might be some work to get this running on Windows and Mac OS X, too). > You then still have to deal with a blocking /dev/random device, though, > so maybe the data with the good entropy should not be returned directly > but rather used to seed a good deterministic RNG instead, like the one > from the glib (have a look at the g_rand_int() function and friends). > Anyway, someone with a good knowledge of crypto stuff should review such > an implementation first before we include that, I think. So, a couple of options we have here. The error condition option for the instruction helps a fair bit. Option 1 is to to have helper_darn() wait for a short timeout to get random data, if it doesn't get something in time, return an error. To reduce the latency further, you could have a one word buffer with the next random number. A background thread would attempt to refill that from the rng backend. darn would return the number in the buffer, or an error if it hasn't been filled yet. This implementation could be reasonably done without a helper, I think, which might be an advantage. Unless there's some clever solution that x86 have already come up with for rdrand in linux-user, I think we want to find some way to construct the rng backend there as well. Falling back to a different rng for user only seems very risky to me.
diff --git a/target-ppc/helper.h b/target-ppc/helper.h index 8eada2f..257bfca 100644 --- a/target-ppc/helper.h +++ b/target-ppc/helper.h @@ -50,6 +50,7 @@ DEF_HELPER_FLAGS_1(cnttzd, TCG_CALL_NO_RWG_SE, tl, tl) DEF_HELPER_FLAGS_1(popcntd, TCG_CALL_NO_RWG_SE, tl, tl) DEF_HELPER_FLAGS_2(bpermd, TCG_CALL_NO_RWG_SE, i64, i64, i64) DEF_HELPER_3(srad, tl, env, tl, tl) +DEF_HELPER_1(darn, tl, i32) #endif DEF_HELPER_FLAGS_1(cntlsw32, TCG_CALL_NO_RWG_SE, i32, i32) diff --git a/target-ppc/int_helper.c b/target-ppc/int_helper.c index 552b2e0..2b9fe13 100644 --- a/target-ppc/int_helper.c +++ b/target-ppc/int_helper.c @@ -182,6 +182,20 @@ target_ulong helper_cnttzd(target_ulong t) { return ctz64(t); } + +target_ulong helper_darn(uint32_t l) +{ + target_ulong r = UINT64_MAX; + + if (l <= 2) { + do { + r = random() * random(); + r &= l ? UINT64_MAX : UINT32_MAX; + } while (r == UINT64_MAX); + } + + return r; +} #endif #if defined(TARGET_PPC64) diff --git a/target-ppc/translate.c b/target-ppc/translate.c index 2a87d1a..6a79fc1 100644 --- a/target-ppc/translate.c +++ b/target-ppc/translate.c @@ -526,6 +526,8 @@ EXTRACT_HELPER(FPW, 16, 1); /* addpcis */ EXTRACT_HELPER_DXFORM(DX, 10, 6, 6, 5, 16, 1, 1, 0, 0) +/* darn */ +EXTRACT_HELPER(L, 16, 2); /*** Jump target decoding ***/ /* Immediate address */ @@ -1893,6 +1895,14 @@ static void gen_cnttzd(DisasContext *ctx) gen_set_Rc0(ctx, cpu_gpr[rA(ctx->opcode)]); } } + +/* darn */ +static void gen_darn(DisasContext *ctx) +{ + TCGv_i32 l = tcg_const_i32(L(ctx->opcode)); + gen_helper_darn(cpu_gpr[rD(ctx->opcode)], l); + tcg_temp_free_i32(l); +} #endif /*** Integer rotate ***/ @@ -6238,6 +6248,7 @@ GEN_HANDLER_E(prtyw, 0x1F, 0x1A, 0x04, 0x0000F801, PPC_NONE, PPC2_ISA205), GEN_HANDLER(popcntd, 0x1F, 0x1A, 0x0F, 0x0000F801, PPC_POPCNTWD), GEN_HANDLER(cntlzd, 0x1F, 0x1A, 0x01, 0x00000000, PPC_64B), GEN_HANDLER_E(cnttzd, 0x1F, 0x1A, 0x11, 0x00000000, PPC_NONE, PPC2_ISA300), +GEN_HANDLER_E(darn, 0x1F, 0x13, 0x17, 0x001CF801, PPC_NONE, PPC2_ISA300), GEN_HANDLER_E(prtyd, 0x1F, 0x1A, 0x05, 0x0000F801, PPC_NONE, PPC2_ISA205), GEN_HANDLER_E(bpermd, 0x1F, 0x1C, 0x07, 0x00000001, PPC_NONE, PPC2_PERM_ISA206), #endif