But LGTM aside from that

Thanks

Looks like phab mangled my tabs so the updated lines are all using spaces :(

I just used their email address for Reported by because given (a) the lack of history with the project (b) no trace of them existing anywhere else online (c) the low-quality suspicious nature of the patches (d) the name being one of *the* quintessential placeholders, I am suspicious as to whether it's their real name.

There's also lib/libc/gen/tls.c for PT_TLS

Fixed properly in 1e39a314d870e312f623199e146eda6bdbc293a3.

I don't think you tested this.

D58301 seems a better alternative to this?

In D58301#1337950, @aokblast wrote:

In D58301#1337946, @jrtc27 wrote:

In D58301#1337933, @aokblast wrote:

In D58301#1337930, @jrtc27 wrote:

In D58301#1337928, @aokblast wrote:

In D58301#1337926, @jrtc27 wrote:

bus_read_8 has been replace to bus_space_read series in sys/bus.h.

Not sure what "bus_space_read series" is though, should that not be bus_space_read_8?

Sorry, it should be bus_space_{read,write} series.

But why series? I don't understand. Why not just use the _8 suffix everywhere?

The _8 suffix remains unchanged before and after my patch. The purpose of this change is to replace the original bus_ series with the bus_space_ series.

Ok, I think I now can decipher what you're trying to get at. What you're trying to say, I think, is something like the following?

bus_{read,write}_8 are macro wrappers around the corresponding bus_space functions in sys/bus.h, so implementing bus_{read,write}_8 won't work. Implement the underlying bus_space functions instead.

Yes, sorry about my bad english expression:)

In D58301#1337933, @aokblast wrote:

In D58301#1337930, @jrtc27 wrote:

In D58301#1337928, @aokblast wrote:

In D58301#1337926, @jrtc27 wrote:

bus_read_8 has been replace to bus_space_read series in sys/bus.h.

Not sure what "bus_space_read series" is though, should that not be bus_space_read_8?

Sorry, it should be bus_space_{read,write} series.

But why series? I don't understand. Why not just use the _8 suffix everywhere?

The _8 suffix remains unchanged before and after my patch. The purpose of this change is to replace the original bus_ series with the bus_space_ series.

In D58301#1337928, @aokblast wrote:

In D58301#1337926, @jrtc27 wrote:

bus_read_8 has been replace to bus_space_read series in sys/bus.h.

Not sure what "bus_space_read series" is though, should that not be bus_space_read_8?

Sorry, it should be bus_space_{read,write} series.

bus_read_8 has been replace to bus_space_read series in sys/bus.h.

Huh, I hadn't thought of doing it this way. This does seem like a better approach than D58318 and, assuming there's no concern about other code accidentally using these (which is no different to the intent of the existing bus_read_8/bus_write_8 stubbing), reintroducing the READ8/WRITE8 macros (whether called that or something else).

(But also happy with the current diff if you'd rather go with that)

Ok, so long as everyone's in agreement that this ultimately needs to land in FreeBSD as a vendor import of a new upstream release, I'm happy

I don't see any of this upstream at https://gitlab.com/alfix/bsddialog, nor even as a merge request there?

Maybe for now it should just be "if ${_tool} is ld and a relative symlink to the same directory then wrap that too"?

You can't unconditionally add it though. For XLD it's probably ok to just check whether it's "ld" or not, but for LD I don't know what the right thing to do is.

Hm. If the link is absolute, we definitely don't need to do anything to it, and if the link is relative, well, who knows? We might already be linking it, copying it, adding our own wrapper, etc.

In D58050#1331122, @markj wrote:

In D58050#1331120, @jrtc27 wrote:

In D58050#1331116, @markj wrote:

@jrtc27 note that IN_IGNORED and IN_UNMOUNT aren't events that you explicitly ask for when you call inotify_add_watch(), they're generated automatically.

Uh, I think you do? I don't think you're meant to get them if not in the mask.

No, you're supposed to get them no matter what you asked for. The Linux man page describes them as flags set by the kernel in returned events. Note also that these flags don't belong to IN_ALL_EVENTS.

The untested https://termbin.com/tmhd is what my mental model is from reading the manpage and thinking about what sensible semantics would be. Maybe it should follow the existing locking discipline as in this patch, though I don't obviously see why that complexity is important for slightly shrinking the lock duration.

In D58050#1331116, @markj wrote:

@jrtc27 note that IN_IGNORED and IN_UNMOUNT aren't events that you explicitly ask for when you call inotify_add_watch(), they're generated automatically.

We may still end up with local diffs to include it in places that upstream doesn't, though

Linux lets you mprotect(PROT_MTE) to enable it. I don't know about disabling.

Would inverting it to PV_MTE_DIRTY not make more sense, as then the default state (and pages that aren't even mapped with tags enabled) is 0?

Implementation-wise it seems reasonable, but every time bsdinstall hardening gains an option the security world cries out in pain over having sensible defaults and good UX :')

Fixes: 7a58744fd0f1 ("Split out the attachment from the generic-ehci driver")

The change itself looks fine but I think the commit message needs rewriting. It is quite verbose on irrelevant details (like how bus_(space_)* foo work under the hood), and talks about data on the PCI bus (which is implementation, not architecture). Also the bug reference should follow the proper format (see tools/tools/git/hooks/prepare-commit-msg). Honestly the commit message would be better if it was entirely replaced with something like:

In D57422#1315673, @cperciva wrote:

In D57422#1315672, @jrtc27 wrote:

Hm, this'll break if building on Linux... don't know if GCE is one of the ones for which that's now supported, probably not, but a bit sad to have this

Oh, I didn't know our VM image building code supported cross-building. That would explain why the original patch (which was submitted from Google) was wrong though.

Hm, this'll break if building on Linux... don't know if GCE is one of the ones for which that's now supported, probably not, but a bit sad to have this

Hm, seems the current version is the same as the arm file, and now it's the same as the aarch64 one? I'm guessing copying from arm is how this came to be...

In D57379#1314791, @minsoochoo0122_proton.me wrote:

In D57379#1314786, @jrtc27 wrote:

In D57379#1314785, @minsoochoo0122_proton.me wrote:

In D57379#1314781, @jrtc27 wrote:

Having said that, what is the use case you have for these? Generally speaking you shouldn't be using sub-word atomics unless absolutely needed, since performance will be worse.

Just curious: do we have trampolines for sub-atomic operations at instruction level? Having them for Zabha or LDADD* would be great optimization opportunity.

I don't understand what your question means

The code has todo for Zabha support. I guess this means there will be implementation in future

if (has_zabha) {
    AMOADD.{B,H};
} else {
    load and fcmpset;
}

Oh, I think I used the term "trampoline" incorrectly here. I meant branching based on detection of Zabha (or equivalent on other ISA).

Back to my original question: How can we achieve this "special path" so processors with Zabha don't have severe performance impact?

In D57379#1314785, @minsoochoo0122_proton.me wrote:

In D57379#1314781, @jrtc27 wrote:

Having said that, what is the use case you have for these? Generally speaking you shouldn't be using sub-word atomics unless absolutely needed, since performance will be worse.

Just curious: do we have trampolines for sub-atomic operations at instruction level? Having them for Zabha or LDADD* would be great optimization opportunity.

In D57379#1314783, @zishun.yi.dev_gmail.com wrote:

In D57379#1314781, @jrtc27 wrote:

Having said that, what is the use case you have for these? Generally speaking you shouldn't be using sub-word atomics unless absolutely needed, since performance will be worse.

Ah, I didn't actually consider the performance before.
I added these to simplify the KASAN porting. KASAN needs to intercept these defines, but riscv doesn't have these definitions, so adding them ensures the KASAN common code won't be polluted by arch-specific code.

Having said that, what is the use case you have for these? Generally speaking you shouldn't be using sub-word atomics unless absolutely needed, since performance will be worse.

Surely these belong in atomic_subword.h or similar? They're not at all specific to RISC-V, and other architectures lack various of these.

In D57274#1312947, @emaste wrote:

I mean, sure, I guess, but what's the threat model here? I sure hope you trust the tarballs you're unpacking to install a system not to be malicious, otherwise what's the point?

I could come up with a contrived exploit scenario, but yes if someone controls the tarballs being unpacked during install it's likely much easier to just provide a trojaned binary.

This came to secteam and I thought "sure, why not."

I mean, sure, I guess, but what's the threat model here? I sure hope you trust the tarballs you're unpacking to install a system not to be malicious, otherwise what's the point?

In D56865#1312084, @arichardson wrote:

In D56865#1302643, @arichardson wrote:

In D56865#1302516, @jrtc27 wrote:

But won’t we have built elftoolchain strip in that case?

Seems like we didn't in my case, let me check if that is an explicit disable or the default. Or maybe we don't add the links anymore?

I get this error even with an explicit WITH_ELFTOOLCHAIN_BOOTSTRAP. Because I am also building with MK_LLVM_BINUTILS=no. So it sounds like either usr.bin/elfcopy needs another check for BOOTSTRAPPING, or we use this change (or possibly both fixes)? @emaste what do you think?

In D53910#1303910, @ziaee wrote:

In D53910#1303904, @jrtc27 wrote:

In D53910#1303893, @ziaee wrote:

In D53910#1303832, @jrtc27 wrote:

In D53910#1303533, @ziaee wrote:

Please understand that I am personally committed to seeing this website refresh land, however I am formally requesting changes proposed over email. I would like to propose the line objected to is replaced with "For over 30 years, this has been a grass roots project carried by passionate volunteers."

Firstly, for those of us not privy to this private email chain, what specifically is being objected to, and why?

The line "Check out the engagement driven by the FreeBSD Foundation on YouTube" is being objected to on the grounds that it is inappropriate for the "Community" box.

A social media presence dedicated to FreeBSD, by people who are involved in FreeBSD, that is an official FreeBSD-related organisation, and is engaging with people on the internet, is inappropriate for the "Community" section of the FreeBSD website how?..

The FreeBSD foundation is it's own entity, with it's own website. The Project is consists of a democratic system, which the Foundation exists completely separate and outside of.

In D53910#1303893, @ziaee wrote:

In D53910#1303832, @jrtc27 wrote:

In D53910#1303533, @ziaee wrote:

Please understand that I am personally committed to seeing this website refresh land, however I am formally requesting changes proposed over email. I would like to propose the line objected to is replaced with "For over 30 years, this has been a grass roots project carried by passionate volunteers."

Firstly, for those of us not privy to this private email chain, what specifically is being objected to, and why?

The line "Check out the engagement driven by the FreeBSD Foundation on YouTube" is being objected to on the grounds that it is inappropriate for the "Community" box.