Re: CFT EFI Boot Refactoring

[Ben Woods]

On 3 December 2016 at 14:40, Ben Woods  wrote:

> I just applied your diff to my subversion repository, and tried to
> buildworld, but the build failed with the following error:
>
> make[6]: make[6]: don't know how to make efipart.c. Stop
>
> make[6]: stopped in /usr/src/sys/boot/efi/drivers
> *** [all_subdir_sys/boot/efi/drivers] Error code 2
>
>
> Does it build ok for you?
>
> Because I use subversion, and I wanted to build it from my main tree, I
> had to regenerate your patch using "git diff --no-prefix
> master..origin/efize_new > /tmp/efize_new.diff".
> I could then apply this cleanly with "svn patch /tmp/efize_new.diff".
>
>
Never mind, I have realised that efipart.c was moved from boot/efi/libefi/
to boot/efi/drivers/ in your git patch, but subversion did not interpret
this change and simply patched the file in place.
I have fixed this with:
$ svn mv boot/efi/libefi/efipart.c boot/efi/drivers/efipart.c

Sorry for the noise (and the previous top post).

Regards,
Ben

--
From: Benjamin Woods
woods...@gmail.com
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"

Re: copyinstr and ENAMETOOLONG

[Jilles Tjoelker]

On Fri, Dec 02, 2016 at 10:20:32AM -0600, Eric van Gyzen wrote:
> On 11/02/2016 15:33, Jilles Tjoelker wrote:
> > On Wed, Nov 02, 2016 at 02:24:43PM -0500, Eric van Gyzen wrote:
> >> Does copyinstr guarantee that it has filled the output buffer when it
> >> returns ENAMETOOLONG?  I usually try to answer my own questions, but I
> >> don't speak many dialects of assembly.  :)
> > 
> > The few I checked appear to work by checking the length while copying,
> > but the man page copy(9) does not guarantee that, and similar code in
> > sys/compat/linux/linux_misc.c linux_prctl() LINUX_PR_SET_NAME performs a
> > copyin() if copyinstr() fails with [ENAMETOOLONG].

> Thanks.

> > In implementing copyinstr(), checking the length first may make sense
> > for economy of code: a user-strnlen() using an algorithm like
> > lib/libc/string/strlen.c and a copyin() connected together with a C
> > copyinstr(). This would probably be faster than the current amd64 code
> > (which uses lods and stos, meh). With that implementation, filling the
> > buffer in the [ENAMETOOLONG] case requires a small piece of additional
> > code.

> >> I ask because I'd like to make the following change, and I'd like to
> >> know whether I should zero the buffer before calling copyinstr to ensure
> >> that I don't set the thread's name to the garbage that was on the stack.

> [snip]
> > For API design, it makes more sense to set error = 0 if a truncated name
> > is being set anyway. This preserves the property that the name remains
> > unchanged if the call fails.

> That makes sense.

> > A change to the man page thr_set_name(2) is needed in any case.

> Of course.

> Would you like to review the following?

> Index: lib/libc/sys/thr_set_name.2
> ===
> --- lib/libc/sys/thr_set_name.2   (revision 309300)
> +++ lib/libc/sys/thr_set_name.2   (working copy)
> @@ -28,7 +28,7 @@
>  .\"
>  .\" $FreeBSD$
>  .\"
> -.Dd June 1, 2016
> +.Dd December 2, 2016
>  .Dt THR_SET_NAME 2
>  .Os
>  .Sh NAME
> @@ -43,37 +43,34 @@
>  .Sh DESCRIPTION
>  The
>  .Fn thr_set_name
> -sets the user-visible name for the kernel thread with the identifier
> +system call sets the user-visible name for the thread with the identifier
>  .Va id
> -in the current process, to the NUL-terminated string
> +in the current process to the NUL-terminated string
>  .Va name .
> +The name will be silently truncated to fit into a buffer of
> +.Dv MAXCOMLEN + 1
> +bytes.
>  The thread name can be seen in the output of the
>  .Xr ps 1
>  and
>  .Xr top 1
>  commands, in the kernel debuggers and kernel tracing facility outputs,
> -also in userland debuggers and program core files, as notes.
> +and in userland debuggers and program core files, as notes.
>  .Sh RETURN VALUES
>  If successful,
>  .Fn thr_set_name
> -will return zero, otherwise \-1 is returned, and
> +returns zero; otherwise, \-1 is returned, and
>  .Va errno
>  is set to indicate the error.
>  .Sh ERRORS
>  The
>  .Fn thr_set_name
> -operation may return the following errors:
> +system call may return the following errors:
>  .Bl -tag -width Er
>  .It Bq Er EFAULT
>  The memory pointed to by the
>  .Fa name
>  argument is not valid.
> -.It Bq Er ENAMETOOLONG
> -The string pointed to by the
> -.Fa name
> -argument exceeds
> -.Dv MAXCOMLEN + 1
> -bytes in length.
>  .It Bq Er ESRCH
>  The thread with the identifier
>  .Fa id
> @@ -92,6 +89,6 @@ does not exist in the current process.
>  .Xr ktr 9
>  .Sh STANDARDS
>  The
> -.Fn thr_new
> -system call is non-standard and is used by
> +.Fn thr_set_name
> +system call is non-standard and is used by the
>  .Lb libthr .
> Index: share/man/man3/pthread_set_name_np.3
> ===
> --- share/man/man3/pthread_set_name_np.3  (revision 309300)
> +++ share/man/man3/pthread_set_name_np.3  (working copy)
> @@ -24,7 +24,7 @@
>  .\"
>  .\" $FreeBSD$
>  .\"
> -.Dd February 13, 2003
> +.Dd December 2, 2016
>  .Dt PTHREAD_SET_NAME_NP 3
>  .Os
>  .Sh NAME
> @@ -39,14 +39,15 @@
>  .Sh DESCRIPTION
>  The
>  .Fn pthread_set_name_np
> -function sets internal name for thread specified by
> -.Fa tid
> -argument to string value specified by
> +function applies a copy of the given
>  .Fa name
> -argument.
> +to the thread specified by the given
> +.Fa tid .
>  .Sh ERRORS
>  Because of the debugging nature of this function, all errors that may
>  appear inside are silently ignored.
> +.Sh SEE ALSO
> +.Xr thr_set_name 2
>  .Sh AUTHORS
>  This manual page was written by
>  .An Alexey Zelkin Aq Mt phan...@freebsd.org .
> Index: sys/kern/kern_thr.c
> ===
> --- sys/kern/kern_thr.c   (revision 309300)
> +++ sys/kern/kern_thr.c   (working copy)
> @@ -578,8 +578,11 @@ sys_thr_set_name(struct thread *td, struct thr_set
>   error = 0;
>   name[0] = '\0';
>   if (uap->name != NULL) {
> - error = 

QLogic FastLinQ support

[Slawa Olhovchenkov]

Is support for QLogic FastLinQ QL45000 planed in FreeBSD?
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"

CFT EFI Boot Refactoring

[Eric McCorkle]

Hello everyone,

My work to refactor the EFI boot loader has been in review for some time
now.  This work is a behavior-neutral refactoring which eliminates
duplicated code in boot1, provides better integration of boot1 and
loader with the EFI API, and moves towards better compliance with the
recommendations of the UEFI driver writer's guide.  This work also
serves as a precursor to more work, such as GELI, hot-plugging, and
other things.

One of the reviewers was able to trigger a hang on his setup; however,
it's not clear whether this is a problem in the refactoring, or whether
it's due to a related bug:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=214423

Therefore, I would like to issue a CFT for this changeset.  We need
people using the boot1/loader EFI boot setup to test their setup using
boot1 and loader as built with this patch applied.

You can also get the source tree directly from my github
(https://github.com/emc2/freebsd.git).  Use the efize_new branch to get
this changeset.  Note that I am maintaining the state of this branch in
a single change at this point using rebase -i, so there *will* be forced
pushes to this branch.


Here are some notes on testing the changeset:

* To test it, just do a buildworld, then copy loader.efi in place and
copy boot1.efi to /efi/BOOT/BOOTX64.EFI on your ESP.  If your system
boots, then the test was successful (there are no new features in this
changeset).

* The output of boot1 is slightly different, so you'll be able to tell
if you installed it correctly.

* I recommend keeping a copy of the basic boot1 around on your ESP, just
in case something goes wrong.  On my setup, I have a backup at
/efi/BOOT/BOOTX64.BAK (with the main program at /efi/BOOT/BOOTX64.EFI,
of course)

* I have been using this on a machine with two disks, a ZFS pool
spanning both disks, and a dummy UFS filesystem for months now, so it
can be considered relatively safe.

* This has also been tested on basic setups without incident, so
priority is on complex or odd setups.

* If something goes wrong, you will most likely get a boot-hang.  If
this happens, please contact me directly with the details, and I'll
coordinate on diagnosis.
diff --git a/lib/libstand/Makefile b/lib/libstand/Makefile
index 0ebcaf1..3b608c5 100644
--- a/lib/libstand/Makefile
+++ b/lib/libstand/Makefile
@@ -41,7 +41,7 @@ SRCS+= ntoh.c
 .PATH: ${LIBC_SRC}/string
 SRCS+=	bcmp.c bcopy.c bzero.c ffs.c fls.c \
 	memccpy.c memchr.c memcmp.c memcpy.c memmove.c memset.c \
-	qdivrem.c strcat.c strchr.c strcmp.c strcpy.c \
+	qdivrem.c strcat.c strchr.c strcmp.c strcpy.c stpcpy.c stpncpy.c \
 	strcspn.c strlcat.c strlcpy.c strlen.c strncat.c strncmp.c strncpy.c \
 	strpbrk.c strrchr.c strsep.c strspn.c strstr.c strtok.c swab.c
 .if ${MACHINE_CPUARCH} == "arm"
diff --git a/lib/libstand/stand.h b/lib/libstand/stand.h
index f77a586..066aff0 100644
--- a/lib/libstand/stand.h
+++ b/lib/libstand/stand.h
@@ -24,7 +24,7 @@
  * SUCH DAMAGE.
  *
  * $FreeBSD$
- * From	$NetBSD: stand.h,v 1.22 1997/06/26 19:17:40 drochner Exp $	
+ * From	$NetBSD: stand.h,v 1.22 1997/06/26 19:17:40 drochner Exp $
  */
 
 /*-
@@ -131,7 +131,7 @@ extern struct fs_ops pkgfs_fsops;
 #define	SEEK_CUR	1	/* set file offset to current plus offset */
 #define	SEEK_END	2	/* set file offset to EOF plus offset */
 
-/* 
+/*
  * Device switch
  */
 struct devsw {
@@ -166,8 +166,9 @@ struct devdesc
 #define DEVT_NONE	0
 #define DEVT_DISK	1
 #define DEVT_NET	2
-#define DEVT_CD		3
+#define DEVT_CD	3
 #define DEVT_ZFS	4
+#define DEVT_EFI	5
 int			d_unit;
 void		*d_opendata;
 };
@@ -279,7 +280,7 @@ extern struct	dirent *readdirfd(int);
 
 extern void	srandom(u_long seed);
 extern u_long	random(void);
-
+
 /* imports from stdlib, locally modified */
 extern long	strtol(const char *, char **, int);
 extern unsigned long	strtoul(const char *, char **, int);
@@ -368,9 +369,9 @@ extern int	null_stat(struct open_file *f, struct stat *sb);
 extern int	null_readdir(struct open_file *f, struct dirent *d);
 
 
-/* 
- * Machine dependent functions and data, must be provided or stubbed by 
- * the consumer 
+/*
+ * Machine dependent functions and data, must be provided or stubbed by
+ * the consumer
  */
 extern int		getchar(void);
 extern int		ischar(void);
diff --git a/sys/boot/efi/Makefile b/sys/boot/efi/Makefile
index 66481f8..00490d0 100644
--- a/sys/boot/efi/Makefile
+++ b/sys/boot/efi/Makefile
@@ -15,7 +15,7 @@ SUBDIR+=	fdt
 .if ${MACHINE_CPUARCH} == "aarch64" || \
 ${MACHINE_CPUARCH} == "amd64" || \
 ${MACHINE_CPUARCH} == "arm"
-SUBDIR+=	libefi loader boot1
+SUBDIR+=	libefi drivers loader boot1
 .endif
 
 .endif # ${COMPILER_TYPE} != "gcc" || ${COMPILER_VERSION} >= 40500
diff --git a/sys/boot/efi/boot1/Makefile b/sys/boot/efi/boot1/Makefile
index 110a857..7480c9c 100644
--- a/sys/boot/efi/boot1/Makefile
+++ b/sys/boot/efi/boot1/Makefile
@@ -8,34 +8,50 @@ MK_SSP=		no
 
 PROG=		boot1.sym
 INTERNALPROG=
-WARNS?=		6
+WARNS?=		3
+
+# Include bcache code.

Re: copyinstr and ENAMETOOLONG

[Eric van Gyzen]

On 11/02/2016 15:33, Jilles Tjoelker wrote:
> On Wed, Nov 02, 2016 at 02:24:43PM -0500, Eric van Gyzen wrote:
>> Does copyinstr guarantee that it has filled the output buffer when it
>> returns ENAMETOOLONG?  I usually try to answer my own questions, but I
>> don't speak many dialects of assembly.  :)
> 
> The few I checked appear to work by checking the length while copying,
> but the man page copy(9) does not guarantee that, and similar code in
> sys/compat/linux/linux_misc.c linux_prctl() LINUX_PR_SET_NAME performs a
> copyin() if copyinstr() fails with [ENAMETOOLONG].

Thanks.

> In implementing copyinstr(), checking the length first may make sense
> for economy of code: a user-strnlen() using an algorithm like
> lib/libc/string/strlen.c and a copyin() connected together with a C
> copyinstr(). This would probably be faster than the current amd64 code
> (which uses lods and stos, meh). With that implementation, filling the
> buffer in the [ENAMETOOLONG] case requires a small piece of additional
> code.
> 
>> I ask because I'd like to make the following change, and I'd like to
>> know whether I should zero the buffer before calling copyinstr to ensure
>> that I don't set the thread's name to the garbage that was on the stack.
> 
>> Index: kern_thr.c
>> ===
>> --- kern_thr.c   (revision 308217)
>> +++ kern_thr.c   (working copy)
>> @@ -580,8 +580,13 @@ sys_thr_set_name(struct thread *td, struct thr_set
>>  if (uap->name != NULL) {
>>  error = copyinstr(uap->name, name, sizeof(name),
>>  NULL);
>> -if (error)
>> -return (error);
>> +if (error) {
>> +if (error == ENAMETOOLONG) {
>> +name[sizeof(name) - 1] = '\0';
>> +} else {
>> +return (error);
>> +}
>> +}
>>  }
>>  p = td->td_proc;
>>  ttd = tdfind((lwpid_t)uap->id, p->p_pid);
> 
> For API design, it makes more sense to set error = 0 if a truncated name
> is being set anyway. This preserves the property that the name remains
> unchanged if the call fails.

That makes sense.

> A change to the man page thr_set_name(2) is needed in any case.

Of course.

Would you like to review the following?

Index: lib/libc/sys/thr_set_name.2
===
--- lib/libc/sys/thr_set_name.2 (revision 309300)
+++ lib/libc/sys/thr_set_name.2 (working copy)
@@ -28,7 +28,7 @@
 .\"
 .\" $FreeBSD$
 .\"
-.Dd June 1, 2016
+.Dd December 2, 2016
 .Dt THR_SET_NAME 2
 .Os
 .Sh NAME
@@ -43,37 +43,34 @@
 .Sh DESCRIPTION
 The
 .Fn thr_set_name
-sets the user-visible name for the kernel thread with the identifier
+system call sets the user-visible name for the thread with the identifier
 .Va id
-in the current process, to the NUL-terminated string
+in the current process to the NUL-terminated string
 .Va name .
+The name will be silently truncated to fit into a buffer of
+.Dv MAXCOMLEN + 1
+bytes.
 The thread name can be seen in the output of the
 .Xr ps 1
 and
 .Xr top 1
 commands, in the kernel debuggers and kernel tracing facility outputs,
-also in userland debuggers and program core files, as notes.
+and in userland debuggers and program core files, as notes.
 .Sh RETURN VALUES
 If successful,
 .Fn thr_set_name
-will return zero, otherwise \-1 is returned, and
+returns zero; otherwise, \-1 is returned, and
 .Va errno
 is set to indicate the error.
 .Sh ERRORS
 The
 .Fn thr_set_name
-operation may return the following errors:
+system call may return the following errors:
 .Bl -tag -width Er
 .It Bq Er EFAULT
 The memory pointed to by the
 .Fa name
 argument is not valid.
-.It Bq Er ENAMETOOLONG
-The string pointed to by the
-.Fa name
-argument exceeds
-.Dv MAXCOMLEN + 1
-bytes in length.
 .It Bq Er ESRCH
 The thread with the identifier
 .Fa id
@@ -92,6 +89,6 @@ does not exist in the current process.
 .Xr ktr 9
 .Sh STANDARDS
 The
-.Fn thr_new
-system call is non-standard and is used by
+.Fn thr_set_name
+system call is non-standard and is used by the
 .Lb libthr .
Index: share/man/man3/pthread_set_name_np.3
===
--- share/man/man3/pthread_set_name_np.3(revision 309300)
+++ share/man/man3/pthread_set_name_np.3(working copy)
@@ -24,7 +24,7 @@
 .\"
 .\" $FreeBSD$
 .\"
-.Dd February 13, 2003
+.Dd December 2, 2016
 .Dt PTHREAD_SET_NAME_NP 3
 .Os
 .Sh NAME
@@ -39,14 +39,15 @@
 .Sh DESCRIPTION
 The
 .Fn pthread_set_name_np
-function sets internal name for thread specified by
-.Fa tid
-argument to string value specified by
+function applies a copy of the given
 .Fa name
-argument.
+to the thread specified by the given
+.Fa tid .
 .Sh ERRORS
 Because of the debugging nature of this function, all errors that may
 appear inside are silently ignored.
+.Sh SEE ALSO
+.Xr