In D20762#449673, @markj wrote:

Reformat to 80 columns.

Finally review please

Fix 7F to F7 in 4 places

I was kinda waiting for jhb@ on this, as it is his patch, but given the urgency I am commandering this, and I'll push a new patch up for review. I would like to ask markj and pmooney to final review that and accept it before I commit to ^/head and push for an immediate MFC, and merge to releng. I'll also need to merge r347065 which is 6 weeks past MFC date.

Ping, since 11.3RC3 is back on the table I would like to see this pushed through along with r347065 which should of already been pushed through as it is 3 weeks past original MFC.

In D19495#449173, @darius.mihaim_gmail.com wrote:

I have applied the feedback @jhb gave, except for the #ifdef guards. Will work on it after rebasing with a newer master.

I helped Jason Tubnor test this patch on 11.3RC2, we had to first merge r347065 to get this patch to apply, and then had to make the 0x7F to 0xF7 Markj points out to get it to work. OpenBSD now runs as a guest in 11.3RC2

I am working with the original poster to get him a binary so he can test this fix.

In D12419#447945, @scottl wrote:

Just tested with TruOS/Trident from a May 2019 kernel, and it's getting ECAPMODE. Maybe the problems with Capsicum aren't worked out yet. I'll look some more at it.

This appears to be hung up on Capsicum additions

In agreement on it probably needs cleaned up, but first finding out why it was done this way as a safe position to take.

In D20626#445823, @kevans wrote:

In D20626#445734, @rgrimes wrote:

I am pulling Kyle Evans in on this as he just did a bunch of cleanup on mac generation code and there may already be existing code to reused rather than have yet another mac generator.

...

One could theoretically take advantage of this to have a stable MAC across hosts for cases like, e.g., migration, no? Although it's not clear that bhyve folk would want to maintain this kind of promise, I could see it being somewhat handy.

For the purpose of migration you take the mac with you, that is part of the guest state.

I am pulling Kyle Evans in on this as he just did a bunch of cleanup on mac generation code and there may already be existing code to reused rather than have yet another mac generator.

Good find.

In D20581#445096, @v.maffione_gmail.com wrote:

Moved include <machine/atomic.h> to virtio.h.

Please document why the -2 is needed per the earlier discussion. Does this need urgent attention to get in 11.3?

Looks like diff has gotten really confused by something making this very hard to see what it is that really changed since we are seeing function
pci_emul_cmdsts_write
compared to a new function
pci_emul_cmd_changed and are not actually seeing what changed in
pci_emul_cmdsts_write

Just questions really, no changes requested.

In D20520#443376, @imp wrote:

bootools is a terrible name, so bad I'm ticking 'request changes'.,

I agree here, but did not have a better name so opted to say nothing.

I am not really keen on adding yet another top level .mk file that sets command names, much of this is caused by scatering etc/Makefile contents and rules about the tree when it was and should of been maintained as the one place for all of this and simply called as a submake. Perhaps it makes since anymore, perhaps not. But certainly having INSTALL and INSTALL_CMD default to and 99.999% of the time be "install" is not a good idea, same for MTREE and MTREE_CMD, they are being used in mixed manners, this needs to be sorted out asap and just use
MTREE_CMD and untwist some of this MTREE overload that should really overlaod MTREE_CMD.

This is a ping. The pr https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=229852 has had another success report. This really should get in tree and in release, what is holding it up?

In D20448#441525, @emaste wrote:

The very first version had:

f3()
{
					/* Empty line if no variables. */
	return (1);
}

Quote above As Ed beat me to the comment, I had be saying: Actually look closer at that 1.1 version, at line 119 it existed, and then in 1.3 was modified at line 159 became 194.
We can ask Kirk for clarity?

I would even be fine with "If the function has no local variables", I do not think the length should come into play at all. Can someone explain why we have this special case at all? I know we want a blank line between variable declarations and the start of the function, is this just an attempt to preserve that empty line when there are no declarations?

Getting way above my pay grade

@allanjude I know your work load is huge as well, if you need me to push a new diff I can do that, but then you have to agree to commit it or I have to pester bde/phk for an approval.

Concur, but not sure how to force it closed.

@jhb, reopening a commited phab review messes with things, as now when you make a second commit the default top patch in this review well be the one you commit, and one has to go digging in the review history to find the prior patch that was in the first commit. It is fine to reopen a review for a revert, but I think one should start a new review for a new change, which is what this is.

Minor nits, and a major win, good work!

Any more little gold mines like this sitting around?

Ok to commit once blank lines are added per jhb in person

I thought this had already been committed?

I would like to see this in next weeks 11.3-PRERELEASE build, this looks to be a low risk barrier addition, @jhb does that seem to be a reasonable target, or would you like to see a longer ^head test cycle? Does anyone see any risk in this change?

This may be a fix for https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=231117, the person is testing.

Thanks for the quick spin on this Patrick

Ping

@jhb it would be good to not have this issue in 11.3

You can mark all my comments as done, I am fine with your answers, I added bz and requested his review on the one change.

In D20065#433092, @kib wrote:

In D20065#433088, @markj wrote:

In D20065#432936, @jhb wrote:

In D20065#432918, @markj wrote:

In D20065#432917, @jhb wrote:

To be clear, the limit isn't being applied today? From the existing code it seems like it should be already in force if I'm reading the diff to vm_map_wire correctly?

The diff is being applied on top of D19908. Without that change, the limit does not get applied to bhyve.

Ok.

Also, if bhyve is allowed to ignore the limit, the amount of wired memory it uses does mean that small requests by, say, gpg will then fail on the host, yes?

Yes, that's right (and true in head today). In the original version of this diff, where bhyve was doing system wiring, that was not the case since system-wired pages are not counted towards the limit.

Ok. I still don't really know how I feel. I think I probably want all long-term wirings to honor the limit as kib@ says and that users will just have to up the limit when using wired VMs with bhyve. If we can get a suitably unique errno value back in whatever creates the wired memory for bhyve and emit a useful error message to the user, that is probably sufficient.

Ok. I guess we can just try and see if there's any fallout. I do think it means that D19908 is not MFCable.

If you add a knob to disable accounting for bhyve requests, it can be merged.

In D19908#433193, @rgrimes wrote:

Do we have to add _user_ to the variables, I only see a rename here, and not a second value used in place of where _users_ went in, or am I totally missing something?

Do we have to add _user_ to the variables, I only see a rename here, and not a second value used in place of where _users_ went in, or am I totally missing something?

I have made one quick pass over this, now that I have read all the bits I would like to make a second pass, my comments now are nits mostly, and can be safely ignored. However I do, as others, have some pretty big concerns about sharing the rcvif with tags, is there some great cost in not doing that?

Well this in any way help with the fact we do not apply any pressure to the zfs arc_max and end up in a memory shortfall that causes high paging? I suspect not as most people do not wire there VM's in memory unless they know that is what they need to do to keep zfs from killing your box with OOM

In D20065#431805, @markj wrote:

In D20065#431692, @rgrimes wrote:

I can see both sides of this, can I ask that the bhyve developers group which meets every 2 weeks be given a chance to discuss this and provide feedback on it? Thanks. Many of us well be at BSDCan a day early for bhyvecon if you wish to discuss it in person.

Why can't the discussion happen here?

Some of it is, but we meet on a cyclic basis to have high bandwidth low delay discussions and I was just asking for the opportunity to allow that discussion to occur. Or if you referring to the in person option that was offered up "if you wish".

I can see both sides of this, can I ask that the bhyve developers group which meets every 2 weeks be given a chance to discuss this and provide feedback on it? Thanks. Many of us well be at BSDCan a day early for bhyvecon if you wish to discuss it in person.

In D19993#431685, @asomers wrote:

The symlink is a new addition for 13.0 (and hopefully 12.1). I would be ok with deleting it for 13.0. But how about I commit this change to head first, and then delete the symlink? That will make MFCs cleaner.

This makes it much clearer to me, thanks jhb

We only need to maintain "backwards" compatibility in the stable/ branches (12 and 11) as far as I am concerned head can just nuke the compatibilty link, and have a 13.0 release notes item that is "fuse has been renamed to fusefs".

A question to @jhb, should the order of the enum and the code updated to be in alphabetic order, in a seperate review, it would only obfuscate the change to do it here.

Looks good, other than a lot of extra blank lines.