Page MenuHomeFreeBSD
Differential D5869

Make bsdinstall zfsboot+gpt+geli installs make use of GELIBOOT
ClosedPublic
Actions

Authored by allanjude on Apr 6 2016, 11:58 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 14, 2:41 PM
Unknown Object (File)
Fri, Mar 7, 3:55 AM
Unknown Object (File)
Sun, Mar 2, 7:39 AM
Unknown Object (File)
Feb 23 2025, 1:56 AM
Unknown Object (File)
Feb 22 2025, 3:40 PM
Unknown Object (File)
Feb 20 2025, 7:15 AM
Unknown Object (File)
Feb 18 2025, 4:35 PM
Unknown Object (File)
Feb 17 2025, 6:18 AM
View All Files
Subscribers
imp
lattera-gmail.com

Details

Reviewers
dteske
Commits
rS300436: bsdinstall/zfsboot GPT+BIOS+GELI installs now make use of GELIBOOT
Summary

Depends on D5867

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3878
Build 3921: arc lint + arc unit

Event Timeline

allanjude updated this revision to Diff 14957.Apr 6 2016, 11:58 PM
allanjude retitled this revision from to Make bsdinstall zfsboot+gpt+geli installs make use of GELIBOOT.
allanjude updated this object.
allanjude edited the test plan for this revision. (Show Details)
allanjude added a reviewer: dteske.
allanjude added a parent revision: D5867: Create the GELIBOOT GEOM_ELI flag.
Herald added a subscriber: imp. · View Herald TranscriptApr 6 2016, 11:58 PM
lattera-gmail.com added a subscriber: lattera-gmail.com.Apr 7 2016, 12:15 AM
allanjude updated this revision to Diff 15306.Apr 19 2016, 4:13 AM

Update after other changes to zfsboot

allanjude updated this revision to Diff 16673.May 21 2016, 8:58 PM

Make proper use of geliboot

Allow GELI w/ EFI+BIOS (use a bootpool for now, until EFI supports GELI)

Add the loader password prompt if a bootpool is used.
boot2 will prompt for the password is geliboot is used instead of a bootpool

allanjude updated this revision to Diff 16675.May 21 2016, 9:13 PM

Hardware accelerate encryption if possible

allanjude updated this revision to Diff 16676.May 21 2016, 9:26 PM

Move loading AESNI to before the GELI password prompt

allanjude updated this revision to Diff 16680.May 21 2016, 10:38 PM

Don't fail if AES-NI can't load (2nd try etc)

Closed by commit rS300436: bsdinstall/zfsboot GPT+BIOS+GELI installs now make use of GELIBOOT (authored by allanjude). · Explain WhyMay 22 2016, 8:32 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
usr.sbin/
bsdinstall/
scripts/
145 lines

Diff 16676

View Options

usr.sbin/bsdinstall/scripts/zfsboot

Show First 20 Lines • Show All 550 Lines • ▼ Show 20 Lineshardclock_sync(int cpu)
int *t; int *t;
KASSERT(!CPU_ABSENT(cpu), ("Absent CPU %d", cpu)); KASSERT(!CPU_ABSENT(cpu), ("Absent CPU %d", cpu));
t = DPCPU_ID_PTR(cpu, pcputicks); t = DPCPU_ID_PTR(cpu, pcputicks);
*t = ticks; *t = ticks;
} }
/* /*
* Compute number of ticks in the specified amount of time. * Compute number of ticks representing the specified amount of time.
*/ */
int int
kibUnsubmitted
Not Done
Inline Actions

There should be a space after comma, for all macro definitions.

kib: There should be a space after comma, for all macro definitions.
tvtohz(struct timeval *tv) tvtohz(struct timeval *tv)
{ {
unsigned long ticks; int64_t retval;
long sec, usec; int64_t usec;
/* /* convert time to microseconds */
kibUnsubmitted
Done
Inline Actions

This still assumes that int == int32_t. It can be larger.

kib: This still assumes that int == int32_t. It can be larger.
hselaskyAuthorUnsubmitted
Done
Inline Actions

Yes, it can be larger and smaller, but not for the platforms we support?

I can add a _Static_assert(sizeof(int) == sizeof(int32_t), "Assumption about integer types not met.");

hselasky: Yes, it can be larger and smaller, but not for the platforms we support? I can add a…
kibUnsubmitted
Done
Inline Actions

But then why do you need casts at all?
If you so rely on compiler I do not see how casts change anything. I compiled the following program both with clang 15.0.2 and gcc 12.2.0 on amd64, and both compiler generated the imull instruction.

#include <limits.h>

extern int hz;

int
tvtohz(long xx)
{
	if (xx > INT_MAX)
		return (INT_MAX);
	if (xx < 0)
		return (1);
	return (xx * hz);
}
kib: But then why do you need casts at all? If you so rely on compiler I do not see how casts change…
kibUnsubmitted
Done
Inline Actions

In fact, I think that uint64_t cast is not needed as well. The holistic test for tv_sec would be not tv_sec > INT_MAX, but tv_sec > INT_MAX / hz. The INT_MAX / hz value can be precomputed once in the place which sets hz, to avoid divide in each tvtohz() invocation.

kib: In fact, I think that uint64_t cast is not needed as well. The holistic test for tv_sec would…
hselaskyAuthorUnsubmitted
Done
Inline Actions

New logic does not make assumptions about size of "int".

hselasky: New logic does not make assumptions about size of "int".
* If the number of usecs in the whole seconds part of the time usec = (int64_t)tv->tv_sec * 1000000LL + (int64_t)tv->tv_usec;
kibUnsubmitted
Done
Inline Actions

LL is not needed due to the cast on the left side of mul
Second cast to int64 is indeed required

But, isn't it more wise to have long variants of your stime* functions, and use long there (basically you assume that int64 is same or higher rank than long, which is true for our arches right now, but who knows)

kib: LL is not needed due to the cast on the left side of mul Second cast to int64 is indeed…
hselaskyAuthorUnsubmitted
Done
Inline Actions

I don't want to use long, because the limit is then a variable, LONG_MAX / hz, which results in a division. I found it better to use comparison with INT_MAX .

hselasky: I don't want to use long, because the limit is then a variable, LONG_MAX / hz, which results in…
kibUnsubmitted
Done
Inline Actions

The double-cast (time_t->int32->int64) together with earlier checks for INT_MAX assumes that int32 == int, which is true now but not necessary so in the future. Why do you need it at all?

Why casting hz at all?

Also, the summary claims that you switched the code to use 32bit mul, while in fact you force 64bit multiplication (unless compiler is too smart, which I doubt).

kib: The double-cast (time_t->int32->int64) together with earlier checks for INT_MAX assumes that…
hselaskyAuthorUnsubmitted
Done
Inline Actions

You are right. I don't need to cast "hz". I'll update the patch.

Both clang and gcc know the (int64_t)(int32_t) thing:

volatile int a,b;

 a = 1 << 24;
 b = 1 << 24;

 printf("0x%llx\n", (int64_t)a * (int64_t)b);
 printf("0x%llx\n", (int64_t)a * b);

Results in a single imull instruction when using -m32 or native 32-bit compilation!

      volatile int a,b;

      a = 1 << 24;
401dc6:       c7 45 f8 00 00 00 01    movl   $0x1000000,-0x8(%ebp)
      b = 1 << 24;
401dcd:       c7 45 fc 00 00 00 01    movl   $0x1000000,-0x4(%ebp)

      printf("0x%llx\n", (int64_t)a * (int64_t)b);
401dd4:       8b 45 f8                mov    -0x8(%ebp),%eax
401dd7:       f7 6d fc                imull  -0x4(%ebp)
401dda:       52                      push   %edx
401ddb:       50                      push   %eax
401ddc:       68 fd 03 40 00          push   $0x4003fd
401de1:       e8 ba 00 00 00          call   401ea0 <printf@plt>
hselasky: You are right. I don't need to cast "hz". I'll update the patch. Both clang and gcc know the…
* difference fits in a long, then the total number of usecs will
kibUnsubmitted
Done
Inline Actions

This changes the visible behavior. Previous code allowed negative tv_usecs (although not quite correctly).

kib: This changes the visible behavior. Previous code allowed negative tv_usecs (although not quite…
* fit in an unsigned long. Compute the total and convert it to /* convert to ticks */
* ticks, rounding up and adding 1 to allow for the current tick retval = __stime64_scale32_floor(usec, hz, 1000000);
* to expire. Rounding also depends on unsigned long arithmetic
* to avoid overflow. /* range check */
* if (__predict_false(retval < 0))
* Otherwise, if the number of ticks in the whole seconds part of retval = 0;
* the time difference fits in a long, then convert the parts to else if (__predict_false(retval > INT_MAX - 1))
* ticks separately and add, using similar rounding methods and retval = INT_MAX - 1;
* overflow avoidance. This method would work in the previous
* case but it is slightly slower and assumes that hz is integral. /* add one additional tick */
* return ((int)retval + 1);
* Otherwise, round the time difference down to the maximum
* representable value.
*
* If ints have 32 bits, then the maximum value for any timeout in
* 10ms ticks is 248 days.
*/
sec = tv->tv_sec;
usec = tv->tv_usec;
if (usec < 0) {
sec--;
usec += 1000000;
}
if (sec < 0) {
#ifdef DIAGNOSTIC
if (usec > 0) {
sec++;
usec -= 1000000;
}
printf("tvotohz: negative time difference %ld sec %ld usec\n",
sec, usec);
#endif
ticks = 1;
} else if (sec <= LONG_MAX / 1000000)
ticks = howmany(sec * 1000000 + (unsigned long)usec, tick) + 1;
else if (sec <= LONG_MAX / hz)
ticks = sec * hz
+ howmany((unsigned long)usec, tick) + 1;
else
ticks = LONG_MAX;
if (ticks > INT_MAX)
ticks = INT_MAX;
return ((int)ticks);
} }
/* /*
* Start profiling on a process. * Start profiling on a process.
* *
* Kernel profiling passes proc0 which never exits and hence * Kernel profiling passes proc0 which never exits and hence
* keeps the profile clock running constantly. * keeps the profile clock running constantly.
kibUnsubmitted
Done
Inline Actions

This would break compilation on arches where one of the types is not long. Use explicit cast for tv_sec/tv_usec to long with %ld format.

kib: This would break compilation on arches where one of the types is not long. Use explicit cast…
*/ */
void void
startprofclock(struct proc *p) startprofclock(struct proc *p)
{ {
PROC_LOCK_ASSERT(p, MA_OWNED); PROC_LOCK_ASSERT(p, MA_OWNED);
if (p->p_flag & P_STOPPROF) if (p->p_flag & P_STOPPROF)
return; return;
if ((p->p_flag & P_PROFIL) == 0) { if ((p->p_flag & P_PROFIL) == 0) {
kibUnsubmitted
Done
Inline Actions

Move comment right above macro definition

kib: Move comment right above macro definition
p->p_flag |= P_PROFIL; p->p_flag |= P_PROFIL;
kibUnsubmitted
Done
Inline Actions

Outer int cast is not needed

kib: Outer int cast is not needed
mtx_lock(&time_lock); mtx_lock(&time_lock);
if (++profprocs == 1) if (++profprocs == 1)
cpu_startprofclock(); cpu_startprofclock();
mtx_unlock(&time_lock); mtx_unlock(&time_lock);
} }
kibUnsubmitted
Done
Inline Actions

How can this work, hz is runtime value, _Static_assert is compile-time construct.

kib: How can this work, hz is runtime value, _Static_assert is compile-time construct.
hselaskyAuthorUnsubmitted
Done
Inline Actions

The formula is linear. Consider it more a sanity check.

hselasky: The formula is linear. Consider it more a sanity check.
impUnsubmitted
Done
Inline Actions

I was confused until I realized hz here != extern int hz. It's the arg to the macro, so this just makes sure we have a good range... Maybe change it? Or have a better name than TIME_ASSERT for the macro.

IMHO, it should be out the function to reinforce that it's not code that executes.

imp: I was confused until I realized hz here != extern int hz. It's the arg to the macro, so this…
} }
/* /*
* Stop profiling on a process. * Stop profiling on a process.
*/ */
void void
stopprofclock(struct proc *p) stopprofclock(struct proc *p)
{ {
▲ Show 20 Lines • Show All 221 Lines • Show Last 20 Lines