From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44372) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAEz1-0004sQ-2x for qemu-devel@nongnu.org; Wed, 01 Jul 2015 06:11:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZAEyz-0004Ky-Os for qemu-devel@nongnu.org; Wed, 01 Jul 2015 06:11:11 -0400 Received: from hall.aurel32.net ([2001:bc8:30d7:100::1]:45968) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAEyz-0004Kl-IW for qemu-devel@nongnu.org; Wed, 01 Jul 2015 06:11:09 -0400 Date: Wed, 1 Jul 2015 12:11:07 +0200 From: Aurelien Jarno Message-ID: <20150701101107.GB11361@aurel32.net> References: <478545ddab5b50a804bed4eea1c8f38e155335fa.1435723168.git.serge.vakulenko@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <478545ddab5b50a804bed4eea1c8f38e155335fa.1435723168.git.serge.vakulenko@gmail.com> Subject: Re: [Qemu-devel] [PATCH pic32 v2 2/5] Fixed random index generation for TLBWR instruction. It was not quite random and did not skip Wired entries. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Serge Vakulenko Cc: Leon Alrae , qemu-devel@nongnu.org On 2015-06-30 21:12, Serge Vakulenko wrote: > Signed-off-by: Serge Vakulenko > --- > hw/mips/cputimer.c | 18 +++++------------- > 1 file changed, 5 insertions(+), 13 deletions(-) > > diff --git a/hw/mips/cputimer.c b/hw/mips/cputimer.c > index 4f02a9f..94a29df 100644 > --- a/hw/mips/cputimer.c > +++ b/hw/mips/cputimer.c > @@ -25,21 +25,13 @@ > #include "qemu/timer.h" > #include "sysemu/kvm.h" > > -#define TIMER_FREQ 100 * 1000 * 1000 > - > -/* XXX: do not use a global */ > +/* Generate a random TLB index. > + * Skip wired entries. */ > uint32_t cpu_mips_get_random (CPUMIPSState *env) > { > - static uint32_t lfsr = 1; > - static uint32_t prev_idx = 0; > - uint32_t idx; > - /* Don't return same value twice, so get another value */ > - do { > - lfsr = (lfsr >> 1) ^ (-(lfsr & 1u) & 0xd0000001u); > - idx = lfsr % (env->tlb->nb_tlb - env->CP0_Wired) + env->CP0_Wired; > - } while (idx == prev_idx); > - prev_idx = idx; > - return idx; > + env->CP0_Random = env->CP0_Wired + > + random() % (env->tlb->nb_tlb - env->CP0_Wired); > + return env->CP0_Random; > } > > /* MIPS R4K timer */ Can you please give us more details about what issue you are trying to fix there? Especially I don't understand about the "skip wired entries" part. It seems the original code handles the wired entries correctly, and at least your patch doesn't seem to change anything regarded that part. Secondly, I don't think calling random() is the correct thing to do. It's an expensive function that is not thread safe. Quoting the specification: "Within the required constraints of the upper and lower bounds, the manner in which the processor selects values for the Random register is implementation-dependent." So it's fine if we use a PRNG like the current code, but I agree we might want to improve it if it has some issues. We want to keep its value reproducible though so that the icount mode works as expected. -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurelien@aurel32.net http://www.aurel32.net