[PATCH] arch/mips/kvm/emulate.c: Don't waste /dev/random emulating TLBWR

[PATCH] arch/mips/kvm/emulate.c: Don't waste /dev/random emulating TLBWR

[George Spelvin]

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(-)

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

Re: [PATCH] arch/mips/kvm/emulate.c: Don't waste /dev/random emulating TLBWR

[Paul Burton]

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
> 

Re: [PATCH] arch/mips/kvm/emulate.c: Don't waste /dev/random emulating TLBWR

[George Spelvin]

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.