In D21297#463778, @luporl wrote:

In D21297#463774, @imp wrote:

In D21297#463764, @luporl wrote:

I agree with @jhibbits: cas.c shouldn't be built for powerpc32, only powerpc64, but the change is OK.
If we go this way, then it is better to remove the (incorrect) conditional compilation of cas.c.
Else, powerpc/ofw/Makefile should be fixed.
This is the relevant part of it:

.if ${MACHINE_ARCH} == "powerpc64"
SRCS+=          cas.c
CFLAGS+=        -DCAS
.endif

It seems this is wrong when cross building to powerpc.
Perhaps it could be changed to something like this:

.if (!defined(TARGET_ARCH) && ${MACHINE_ARCH} == "powerpc64") || "${TARGET_ARCH}" == "powerpc64"
SRCS+=          cas.c
CFLAGS+=        -DCAS
.endif

But I'm also fine with just removing the if block.

TARGET_ARCH isn't a thing you can test anywhere outside of Makefile.inc1, so this suggestion can't be right. In the cross build case, we always set MACHINE_ARCH correctly.

Ok, but then the following use case is not supported, right?

cd /usr/src/stand && make TARGET=powerpc TARGET_ARCH=powerpc

Because the following command outputs "powerpc64":

In D21297#463764, @luporl wrote:

I agree with @jhibbits: cas.c shouldn't be built for powerpc32, only powerpc64, but the change is OK.
If we go this way, then it is better to remove the (incorrect) conditional compilation of cas.c.
Else, powerpc/ofw/Makefile should be fixed.
This is the relevant part of it:

.if ${MACHINE_ARCH} == "powerpc64"
SRCS+=          cas.c
CFLAGS+=        -DCAS
.endif

It seems this is wrong when cross building to powerpc.
Perhaps it could be changed to something like this:

.if (!defined(TARGET_ARCH) && ${MACHINE_ARCH} == "powerpc64") || "${TARGET_ARCH}" == "powerpc64"
SRCS+=          cas.c
CFLAGS+=        -DCAS
.endif

But I'm also fine with just removing the if block.

it looks to my eye that all prior comments had been addressed...

generally I like this. Couple of minor spacing nits, and a question about a possible improper use of KASSERT

This does what the comment says it does... I'm not qualified too know if that's actually the right thing to do or not.

This is good, and good commit message. V7 was released 40 years ago this past January. It flourished in the late 70s and early 80s as a platform people ported to (I'm aware of maybe a dozen such ports too 4 or 5 different architectures)... then all those ports migrated to System III or System V by maybe 1989 or so. The last time I saw this software pattern was in the mid 90s when I updated the program in question to not do that.... I doubt this code path has been execute in the last decade or two except by pedants testing V7 compatibility.

kill them with fire :)

Ah, I see where it's used now. There are several alternatives, but this is the least bad one that comes to mind.
We have a little bit of dead code vs having #ifdefs for FDT in usb_ethernet.c.
It's more modular to have it like this, so I'm cool with it.

looks fine as far as it goes, but what's including this? usb_ethernet.c certainly needs miibus too compile...

In D21060#462958, @kib wrote:

In D21060#462930, @imp wrote:

I'd personally rather see us bring back something from the past . We had WITH_CLANG_AS_CC which controlled creating cc->clang links. What if we had WITH_CLANG_LINKS or similar that would create clang->cc links and have it default to off? This isn't blocking if there's no support for it, though. I didn't click request changes for this :).

See below.

And have you done an EXP run too see what the fallout is? My 'request changes' is a hard no until that's run and any issues resolved.

I stated that exp run is required after the discussion is finished. I see no reason to ask portmgr to spend their time unless there is a clear route to commit the change.
WRT to optional symlinks, I am fine with it as far as defaults are to not have them. The reason for this change is to remove the surprise and the need to answer 'why' questions for people that start developing on FreeBSD. The same group of people are proficient in Linux and they do not have to manipulate $PATH or create symlinks in ~/bin because 'this is how FreeBSD happens to not-handle it' for all the time (it == conflict between base and ports binaries). If symlinks are on by default, this change does not worth the time.

looks great to me.

ah, enough searching and I see that it is.

well, it could be in the files, but it's unambiguous enough. I assume this is how it is upstream?

This detached license is basically required since bdf files don't have a good way to add it.

I'd personally rather see us bring back something from the past . We had WITH_CLANG_AS_CC which controlled creating cc->clang links. What if we had WITH_CLANG_LINKS or similar that would create clang->cc links and have it default to off? This isn't blocking if there's no support for it, though. I didn't click request changes for this :).

I think this is fine.... Not 100% sure, but I think we'll be good. The time may be here to always link in libsa.a, but if you don't *NEED* that, then let's not go there yet. the day will come though...

OK. Not really worth optimizing then. Thanks for the feedback.

Seems sane enough...

In D11115#462518, @emaste wrote:

side by side comparison with the .o generated

Looking at boot1.o differences there are lots of header, debug, etc. changes, and changes due to differing offsets. The real change is different instruction size for the test instruction at the beginning of read

read:           testb $FL_PACKET,%cs:MEM_REL+flags-start # LBA support enabled?

GNU as produces

0000:012e      2e f6 06 b0 08 80   test byte cs:[0x8b0], 0x80   ; [0x8b0:1]=255 ; 128

While Clang IAS produces:

0000:012e      2e 67 f6 05 b3 08 00 00 80  test byte cs:[0x8b3], 0x80   ; [0x8b3:1]=255 ; 128

This is fine for boot2, other than using three bytes more than necessary.

had the same thing in mind

P,S, I don't know the clicky button to say this is no longer blocked by core@, so don't let that stop you from committing it.

Alternatively, you could add a explicit reference to the location of the license file in FreeBSD's repo in the .c and .h. That would be fine enough.

In D18613#462036, @jpaetzel wrote:

@vbhakta_vmware.com is there anything I can do to facilitate the changes this driver needs to get it into the tree?

time to go... but need to remove from files and double check to make sure there's no lingering man page Xr's.

minor comment fixing.

In D21248#461858, @jhb wrote:

There were several other things that looked like they were also identical (e.g. several agp drivers, all the atkbdc stuff, acpi_wmi_if.m). Those can be followup commits though.

Combine and rebase

Per jhb suggestion, combining with https://reviews.freebsd.org/D21233

Update to use include more directly, per jhb

In D21210#461496, @jhb wrote:

Oh, I thought it seemed to be there from the code I read on Friday:

static void
read_file(char *fname)
{
...
	/*
	 * include "filename"
	 * filename    [ standard | optional ]
	 *	[ dev* [ | dev* ... ] | profiling-routine ] [ no-obj ]
	 *	[ compile-with "compile rule" [no-implicit-rule] ]
	 *      [ dependency "dependency-list"] [ before-depend ]
	 *	[ clean "file-list"] [ warning "text warning" ]
	 *	[ obj-prefix "file prefix"]
	 */
...
	if (eq(wd, "include")) {
		wd = get_quoted_word(fp);
		if (wd == (char *)EOF || wd == NULL)
			errout("%s: missing include filename.\n", fname);
		(void) snprintf(ifname, sizeof(ifname), "../../%s", wd);
		read_file(ifname);
		while (((wd = get_word(fp)) != (char *)EOF) && wd)
			;
		goto next;
	}

In D21232#461474, @emaste wrote:

In D21232#461469, @imp wrote:

These are fine but (a) you need to have mount privs to execute this ioctl; (b) nandfs is a panic trap due to bad locking and the system can't stay up once there's any vnode pressure at all; and (c) the set of nandfs users is the empty set due to (b). This is not exploitable in any meaningful way.

and you have to go out of your way to build and run it. But the fix is easier than explaining that we don't care about nandfs.

In D21210#461467, @jhb wrote:

Ah, I had thought we would use 'include' in files.i386 and files.amd64 rather than DEFAULTS, but either way is fine with me.

update per jhb's suggestions

In D21203#461464, @jhb wrote:

OTOH, I wonder if what we might not want instead is a 'NOTES_EXTRA'. You would modify makeLINT.mk to do something like this:

NOTES=	${.CURDIR}/../../conf/NOTES ${.CURDIR}/NOTES ${NOTES_EXTRA}

We might find that there are some other large "features" we might want to split out into an MI NOTES but only enable them for certain architectures.
An even simpler way perhaps might be to change makeLINT.mk to just use 'NOTES+=' instead of 'NOTES=', then you could change amd64/Makefile and i386/Makefile
to just have:

NOTES=${.CURDIR}/../../x86/conf/NOTES

And use '+=' in makeLINT.mk. Perhaps that is the simplest of all.

These are fine but (a) you need to have mount privs to execute this ioctl; (b) nandfs is a panic trap due to bad locking and the system can't stay up once there's any vnode pressure at all; and (c) the set of nandfs users is the empty set due to (b). This is not exploitable in any meaningful way.

This commit is purposely thin to get the framework up and going so we can focus on framework issues.

changes per jhb review, except the ifdef x86 in makeLINT.mk

And also https://reviews.freebsd.org/D21208

See follow on review https://reviews.freebsd.org/D21207

In D21200#460958, @jhb wrote:

I think a related question on the mail thread was if COMPAT_43 should be in any non-x86 config files at all. E.g. it shouldn't be in sys/conf/notes but should move to sys/x86/conf/NOTES once we start that.

I think these are good. There's comments in the Linux emulator that says they do different things because of this ifdef: