Changeset View
Changeset View
Standalone View
Standalone View
sys/dev/random/ivy.c
Show First 20 Lines • Show All 91 Lines • ▼ Show 20 Lines | __asm __volatile( | ||||
"dec %0\n\t" /* otherwise, retry-- */ | "dec %0\n\t" /* otherwise, retry-- */ | ||||
"jne 1b\n\t" /* and loop if retries are not exhausted */ | "jne 1b\n\t" /* and loop if retries are not exhausted */ | ||||
"2:" | "2:" | ||||
: "+r" (retry), "=r" (rndval) : : "cc"); | : "+r" (retry), "=r" (rndval) : : "cc"); | ||||
*buf = rndval; | *buf = rndval; | ||||
return (retry); | return (retry); | ||||
} | } | ||||
static int | |||||
x86_unimpl_store(u_long *buf __unused) | |||||
{ | |||||
KASSERT(0, ("%s called", __func__)); | |||||
cem: it's more like we shouldn't be invoking x86_rng_store at all if neither rdrand nor rdseed was… | |||||
cemUnsubmitted Done Inline ActionsHm, I preferred panic -- KASSERT is compiled out on !INVARIANTS and this is always wrong. If you prefer KASSERT, we should at least return failure (0) instead of no return value, to match function signature. cem: Hm, I preferred panic -- KASSERT is compiled out on !INVARIANTS and this is always wrong.
If… | |||||
markjAuthorUnsubmitted Done Inline ActionsOk. KASSERT seemed reasonable since the function should be unreachable unless there's a bug somewhere. markj: Ok. KASSERT seemed reasonable since the function should be unreachable unless there's a bug… | |||||
} | |||||
kibUnsubmitted Not Done Inline ActionsI am surprised that this function compiles. You should return something from it. I vote to panic() always. kib: I am surprised that this function compiles. You should return something from it.
I vote to… | |||||
markjAuthorUnsubmitted Done Inline ActionsIt compiles with INVARIANTS on because the branch in the KASSERT() is always taken. markj: It compiles with INVARIANTS on because the branch in the KASSERT() is always taken. | |||||
DEFINE_IFUNC(static, int, x86_rng_store, (u_long *buf), static) | DEFINE_IFUNC(static, int, x86_rng_store, (u_long *buf), static) | ||||
{ | { | ||||
has_rdrand = (cpu_feature2 & CPUID2_RDRAND); | has_rdrand = (cpu_feature2 & CPUID2_RDRAND); | ||||
has_rdseed = (cpu_stdext_feature & CPUID_STDEXT_RDSEED); | has_rdseed = (cpu_stdext_feature & CPUID_STDEXT_RDSEED); | ||||
if (has_rdseed) | if (has_rdseed) | ||||
return (x86_rdseed_store); | return (x86_rdseed_store); | ||||
else if (has_rdrand) | else if (has_rdrand) | ||||
return (x86_rdrand_store); | return (x86_rdrand_store); | ||||
else | else | ||||
return (NULL); | return (x86_unimpl_store); | ||||
Not Done Inline ActionsIn fact you could return x86_rdseed_store if !has_rdrand, hardware then would trap if RDSEED is not implemented. kib: In fact you could return x86_rdseed_store if !has_rdrand, hardware then would trap if RDSEED is… | |||||
Done Inline ActionsOr even always return x86_rdrand() when !has_rdseed. I think I prefer having an explicit panic() though. markj: Or even always return x86_rdrand() when !has_rdseed. I think I prefer having an explicit panic… | |||||
} | } | ||||
/* It is required that buf length is a multiple of sizeof(u_long). */ | /* It is required that buf length is a multiple of sizeof(u_long). */ | ||||
static u_int | static u_int | ||||
random_ivy_read(void *buf, u_int c) | random_ivy_read(void *buf, u_int c) | ||||
{ | { | ||||
u_long *b, rndval; | u_long *b, rndval; | ||||
u_int count; | u_int count; | ||||
▲ Show 20 Lines • Show All 44 Lines • Show Last 20 Lines |
it's more like we shouldn't be invoking x86_rng_store at all if neither rdrand nor rdseed was detected -- MOD_LOAD() shouldn't register our callback unless HW support is present.
So I guess I'm saying, this should panic because of an invariants violation, not "missing hardware support".