@kp are you ok with this change? I think m_unshare can still be resolved orthogonally from the rest of this series.

In D47254#1077677, @kib wrote:

In D47254#1077565, @jhb wrote:

So the acpi bus will need some way to recognize special DMAR devices (e.g. a custom ivar) in acpi_get_domain(), and then for DMAR devices it needs to call some other helper routine instead of acpi_parse_pxm() that looks up the domain.

I do not want to push a knowledge about DMAR into acpi. Would something like this work: add a 'fake' bus dmarbus0 below acpi0, with the only non-default method BUS_GET_DOMAIN(), and make all dmarX children of dmarbus0, so the hierarchy would be

dmarX dmarbus0 acpi0 nexus0

Then dmarbus0 method is called and knows how to calculate the domain?

Hmmm, if the logic is the same for all IRQ resources ever allocated on intrng architectures, then doing it in the wrapper might indeed be simplest.

In D46786#1077536, @kp wrote:

In D46786#1077456, @jhb wrote:

In D46786#1074982, @kp wrote:

That seems reasonable, yes. Ideally I'd like to be able to assert that assumption (probably in pf_pull_hdr()) though.

It's also not the only case where we currently call m_unshare() to make sure we can access all the relevant data. I wound up adding one to if_ovpn here: https://cgit.freebsd.org/src/commit/sys/net/if_ovpn.c?id=5644e2c6d47c6113a61ab7fc0776b7227677656a
Short version, in kib's words: "ftpd uses sendfile(2) AFAIR, and the symptoms of the file corruption mean that M_RDONLY mbuf, of type M_EXTPG, modified by the network stack"

Hmmm, so for KTLS, we are careful to recognize when the pages of an unmapped mbuf are "borrowed" and allocate anonymous pages for encrypting them explicitly. I wonder if for DCO you want/need to be doing something similar to protect against what kib@ describes? In particular, check ktls_encrypt_record() in sys/kern/uipc_ktls.c and how it uses EPG_FLAG_ANON to detect "anonymous" writable pages vs borrowed pages. (Potentially once this series lands that makes M_RDONLY correct for unmapped mbufs we could retire EPG_FLAG_ANON and depend on M_RDONLY for that instead.)

Right, we'll have to detect M_RDONLY (and potentially EPG_FLAG_ANON too, unless it's retired first) and then convince opencrypto to write the output somewhere else.
Presumably we can 'just' allocate a new mbuf for the result and then call crypto_use_output_mbuf()?

I'm slightly reluctant to mess with what we have now, and not (just) because of laziness. I never actually managed to reproduce https://bugs.freebsd.org/280036 myself, so I don't have a setup to test that. Still, it looks like that's the direction we ought to go with this.

@mav does this version look ok? It still works for me with the basic 'newfs -E' test in a VM.

Presumably the gpiobus driver needs to call into subr_intr.c for those platforms then? (And that needs to happen in gpiobus.c, this is not ACPI specific)

So the acpi bus will need some way to recognize special DMAR devices (e.g. a custom ivar) in acpi_get_domain(), and then for DMAR devices it needs to call some other helper routine instead of acpi_parse_pxm() that looks up the domain.

Yes, the acpi_get_domain() would have to recognize dmar devices and do special handling. Today it just handles _PXM methods on Device objects, and if a device doesn't have a _PXM method it just punts. Presumably the DMAR devices are not Device nodes in the ACPI namespace and thus don't have a _PXM method?

Eh, this doesn't seem correct on the surface. How are the resources being allocated? If they are being allocated via gpiobus_alloc_resource(), then it is wrong to pass those resources up to some random other device driver in the tree and ask it to activate a resource it didn't create and doesn't know how to manage. Do you have more details on "that wasn't working"? Do you get a panic?

In D46786#1074982, @kp wrote:

That seems reasonable, yes. Ideally I'd like to be able to assert that assumption (probably in pf_pull_hdr()) though.

It's also not the only case where we currently call m_unshare() to make sure we can access all the relevant data. I wound up adding one to if_ovpn here: https://cgit.freebsd.org/src/commit/sys/net/if_ovpn.c?id=5644e2c6d47c6113a61ab7fc0776b7227677656a
Short version, in kib's words: "ftpd uses sendfile(2) AFAIR, and the symptoms of the file corruption mean that M_RDONLY mbuf, of type M_EXTPG, modified by the network stack"

Correct synchronous command completion handling

There are also various races (I think) with the CIFS being changed by the guest while a request is in flight. We really should be caching the CIFS for the duration of a command. The issue there though is that CIFS is variable-sized. :( We could at least cache the common header though I think which would probably handle all of the races I can see.

This fixes a regression from the previous fix. With current main if you just boot a VM with an AHCI attached disk backed by a zvol (so supports TRIM) and do newfs -E /dev/ada0 the guest FreeBSD kernel hangs in a loop of AHCI timeouts as mav@ worried in the previous review. I hadn't expected that the previous review was actually broken, but my guess is the added done >= sizeof(buf) - 8 check was wrong. It probably should have been '>' instead as if you get a full 512 byte block, it will break from the loop before the last valid entry and never send a reply leading to the hang.

Historically I had wanted the 'maxcpus' tunable to be one of the possible settings at creation time (and an nvlist is how I would have passed in such settings). However, I think I've mostly made that requirement OBE in practice.

This looks ok to me, though I'm not intimately familiar with all of iflib.

In D46948#1075055, @olce wrote:

In D46948#1075027, @jhb wrote:

OTOH, if we had any writable filesystems for which the sticky bit was relevant, I do think it would be simpler and more flexible if the masks applied to ALLPERMS instead of just ACCESSPERMS. msdosfs wouldn't change its defacto semantics if we made that change since it doesn't support those bits anyway.

I was also thinking along this line. I agree with both of you that in this case it doesn't seem to matter much, but consistency is also important and accommodating for future R/W filesystems (or the addition of masks to existing ones) tends to push towards just applying the mask to ALLPERMS. For the sticky bit, as you said, but perhaps also to be able to selectively allow only one the setuid and setgid bits (an effect which could be obtained alternatively through new mount options).

If keeping the current behavior, I would recommend ensuring that the mount fails if non-supported bits in the mask are specified. That way, an extension to more bits would still be possible in the future without breaking compatibility.

I'm not opposed to that necessarily. We discussed this a bit more in the review at D46948 if you'd like to chime in there.

I've found this useful as you can now see which drivers fail to set a description (among other things). :)

I don't think the parent can interfere here as DEVICE_ATTACH calls the child method directly without giving the parent driver a chance to interpose.

In D46948#1074925, @olce wrote:

In D46948#1074919, @jhb wrote:

@olce Rock Ridge extensions support extended modes like sticky bits on ISO images.

Ah thanks, missed that.

Shouldn't the masks allow to clear the non-ACCESSPERMS bits? Or are you considering that redundant with, e.g., the nosuid mount option?

Humm, I posit that pf(4) will never need to look into the content of unmapped mbufs. They will always be "plain" packet data (either data from a call to write(2) or sendfile(2)) and never headers inserted by any protocol layer. You can also get them for things like iSCSI and NVMe PDUs, but again only for the data portion, not even for PDU headers (which are in plain mbufs). The reason being that protocol headers have to be constructed, and the code that constructs the headers is going to want a mapped mbuf that it can write into. It could be that what we say here is that m_unshare() does not un-share unmapped mbufs but instead just bumps the refcount on the backing mbuf. We could certainly add a new helper of sorts that is something like m_mapped(m, off, len) that pf(4) could use before calls to m_pullup(). However, it is also true that nothing should be reading the unencrypted data in NIC TLS mbufs (where the NIC does the encryption so the data stays unencrypted all the way down the stack).

@olce Rock Ridge extensions support extended modes like sticky bits on ISO images. msdosfs has no way to store such bits and the permissions are all synthetic for msdosfs.

Adjust ()s to quiet warnings

So would ya'll rather me fix m_unshare()? It's clearly not being used today on unmapped mbufs today as it would be panicking when it used the pointers returned from mtod(), so I wouldn't be able to test it.

Ping

In D45950#1073921, @gallatin wrote:

In D45950#1073880, @jhb wrote:

I don't think we need the taskqueue. It's probably just a design copied from the Intel drivers, and I don't think it makes much sense for those either. The other thing that can be nice to do though when making this change is to instead build a temporary list of packets linked via m_nextpkt (mbufq works for this) and pass an entire batch to if_input. This lets you avoid dropping the lock as often.

What I dislike about m_nextpkt() is that you trade pointer chasing for lock frobbing. I tend to think an rx ring lock is a kludge that's almost never really needed. But if enough drivers are doing this, we might want to consider an mbuf array method for if_input.

I don't think we need the taskqueue. It's probably just a design copied from the Intel drivers, and I don't think it makes much sense for those either. The other thing that can be nice to do though when making this change is to instead build a temporary list of packets linked via m_nextpkt (mbufq works for this) and pass an entire batch to if_input. This lets you avoid dropping the lock as often.

Is the issue you mentioned previously that you are worried that some drivers might not support concurrent requests on a session? I can't recall if all drivers are ready for this. ccr(4) is at least, though it will perform better if you use separate sessions (Chelsio NICs have multiple crypto engines and each session is tied to a specific engine).

Ping @asomers

Ping

Ok, I'll drop it to 32 for commit then.

I can fix the type mismatch during commit. I have not looked to see if other stacks are affected.

Switch to old_len

You might try doing this again after my other series lands. In this case though I think you'd need M_WRITABLE_EXTPG.

Looks kind of similar. I have it as a downstream diff in CheriBSD as otherwise it don't compile at all. (CheriBSD doesn't let you mix and match user vs kernel pointers willy-nilly as they are different types.) I guess I'll have to deal with the merge conflict when merging your other change.

This is fine for now. I do think long term we want to figure out the minimum set of registers to preserve for each arch and zero the rest.