| Message ID | 201903210604.x2L64Ord018045@sdf.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arch/mips/kvm/emulate.c: Don't waste /dev/random emulating TLBWR | expand |
Hi George, On Thu, Mar 21, 2019 at 06:04:24AM +0000, George Spelvin wrote: > KVM_MIPS_GUEST_TLB_SIZE is 64, so we only need one random byte, > not 4. > > A more complex question is whether we need crypto-grade random > numbers at all. If safe, we could use prandom_u32(). If not, > we could seed a private PRNG and use prandom_u32_state(). > > Or could we just use asm("mfc0 %0, Random" : "=r" (index))? > > Signed-off-by: George Spelvin <lkml@sdf.org> > --- > I ran across this whie doing some other cleanups, and thought > I'd pass it on. > > get_random_bytes() is quite an expensive function call. > Is it needed at all? Thanks for the patch. I expect we should be fine with: index = prandom_u32_max(KVM_MIPS_GUEST_TLB_SIZE); We certainly don't need crypto-grade randomness here. Using the cp0 Random register would be an option for configurations prior to MIPSr6, where the Random register was deprecated & we shouldn't rely on its presence. So we could do: if (MIPS_ISA_REV < 6) index = read_c0_random() % KVM_MIPS_GUEST_TLB_SIZE; else index = prandom_u32_max(KVM_MIPS_GUEST_TLB_SIZE); Though whether that micro-optimization is worth the extra code is questionable. Thanks, Paul > arch/mips/kvm/emulate.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c > index ec9ed23bca7f..a689f3db3094 100644 > --- a/arch/mips/kvm/emulate.c > +++ b/arch/mips/kvm/emulate.c > @@ -1139,8 +1139,9 @@ enum emulation_result kvm_mips_emul_tlbwr(struct kvm_vcpu *vcpu) > struct mips_coproc *cop0 = vcpu->arch.cop0; > struct kvm_mips_tlb *tlb = NULL; > unsigned long pc = vcpu->arch.pc; > - int index; > + unsigned char index; > > + /* Do we need this quality of random numbers? Would prandom_u32 do? */ > get_random_bytes(&index, sizeof(index)); > index &= (KVM_MIPS_GUEST_TLB_SIZE - 1); > > -- > 2.20.1 >
On Fri, 22 Mar 2019 at 00:00:45 +0000, Paul Burton wrote: > On Thu, Mar 21, 2019 at 06:04:24AM +0000, George Spelvin wrote: >> KVM_MIPS_GUEST_TLB_SIZE is 64, so we only need one random byte, >> not 4. >> >> A more complex question is whether we need crypto-grade random >> numbers at all. If safe, we could use prandom_u32(). If not, >> we could seed a private PRNG and use prandom_u32_state(). >> >> Or could we just use asm("mfc0 %0, Random" : "=r" (index))? > Thanks for the patch. I expect we should be fine with: > > index = prandom_u32_max(KVM_MIPS_GUEST_TLB_SIZE); > > We certainly don't need crypto-grade randomness here. Using the cp0 > Random register would be an option for configurations prior to MIPSr6, > where the Random register was deprecated & we shouldn't rely on its > presence. So we could do: > > if (MIPS_ISA_REV < 6) > index = read_c0_random() % KVM_MIPS_GUEST_TLB_SIZE; > else > index = prandom_u32_max(KVM_MIPS_GUEST_TLB_SIZE); > > Though whether that micro-optimization is worth the extra code is > questionable. I'm also not sure if you *want* to use the random register, because that's a source of /dev/random entropy (arch/mips/include/asm/timex.h), and maybe exposing it to VM guests would be a bad thing. There's no need to get too fancy; prandom_u32_max is fine. (And gcc optimizes it to a shift if the argument is a compie-time power of 2.) Anyway, thank you for the response, and I'm assuming there's no need for revised patch from me; as it's less work for you to write your own.
diff --git a/arch/mips/kvm/emulate.c b/arch/mips/kvm/emulate.c index ec9ed23bca7f..a689f3db3094 100644 --- a/arch/mips/kvm/emulate.c +++ b/arch/mips/kvm/emulate.c @@ -1139,8 +1139,9 @@ enum emulation_result kvm_mips_emul_tlbwr(struct kvm_vcpu *vcpu) struct mips_coproc *cop0 = vcpu->arch.cop0; struct kvm_mips_tlb *tlb = NULL; unsigned long pc = vcpu->arch.pc; - int index; + unsigned char index; + /* Do we need this quality of random numbers? Would prandom_u32 do? */ get_random_bytes(&index, sizeof(index)); index &= (KVM_MIPS_GUEST_TLB_SIZE - 1);
KVM_MIPS_GUEST_TLB_SIZE is 64, so we only need one random byte, not 4. A more complex question is whether we need crypto-grade random numbers at all. If safe, we could use prandom_u32(). If not, we could seed a private PRNG and use prandom_u32_state(). Or could we just use asm("mfc0 %0, Random" : "=r" (index))? Signed-off-by: George Spelvin <lkml@sdf.org> --- I ran across this whie doing some other cleanups, and thought I'd pass it on. get_random_bytes() is quite an expensive function call. Is it needed at all? arch/mips/kvm/emulate.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)