Correct spelling of VM_MAXCPU

Change from exposing struct vm to using an accessor function, this just adds the accessor.

In D18755#400856, @araujo wrote:

In D18755#400855, @rgrimes wrote:

...

Hi @rgrimes,

I'm not sure the impact of the changes you are intend to do related with the ACPI MADT table, I have this ticket under my hands and as you are working on these changes now, maybe you should take a double look on this ticket. This ticket also has couple links to some discussions regarding the bump of VM_MAXCPU.

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

I have taken that bug, I was aware of those issues and plan a seperate review for fixing the MADT table issue, which does not even reference VM_MAXCPU when it should. I may quickly add a CTASSERT(VM_MAXCPU>20) which iirc is where that table starts to run into issues, though that would stop pmooney who is using efi only and hence does not have the issue this table creates. Fixing the MADT is a rather easy thing once all the other bits are in place.

Add bhyve group, I thought I had that in the list when I created the review. I am soliciting feedback on if I should abandon this diff and re-write the code using an accessor function (called vmm_vm_maxcpus()) to vmm.c that does the vmx->vm->maxcpus dereference and call that. The most common use of this is in vmm.c itself, and would probably end up inlined by the compiler anyway, though there are at least 4 other files that have the need for this, and iirc there is userland code that uses the VM_MAXCPU constant that needs to NOT do that!

In D18755#400526, @pmooney_pfmooney.com wrote:

Up until now, those structs have been kept opaque to the rest of the system, with accessor functions to perform necessary tasks/queries against them. Could you go into more detail about how your plans for dynamic VM_MAXCPU are impeded by similar constraints?

Age should not be a criteria for removal. BSD is old, very old, shall we remove it simply cause it is old? IMHO you all are getting way ahead of the process, having reviews to remove code before the cost/benefit and open discussions happens is backwards.

In D18574#395995, @araujo wrote:

Hi guys,

...

One possible solution would be to give the options for users to choose between tap and vmnet style to be used and also document it (we can add some info on our bhyve wiki page)!

In D18574#395968, @cem wrote:

In D18574#395967, @rgrimes wrote:

I do not like changing defaults without very good reasons, what reasons are there for making this change?

If you have not yet done so, please read the summary included in the review. If you have read it and still have this question, it could probably stand to be spelled out more clearly.

I do not like changing defaults without very good reasons, what reasons are there for making this change?

I like imp@'s revisions. I am accepting either version though.

In D17935#383667, @imp wrote:

I'd do a separate commit that just says 'CTM will move to ports in FreeBSD 13' in the ctm man page and get that merged before 12.0R (so ASAP).

There should be an UPDATING entry to tell people to go get the port.. This has a 0 length deprecation notice if we just go rip it out now. I would support first adding a gonein(13) and merging that to release 12 or 12.1. That should be done first, before this. Have the ports builders made a pkg avaliable for ^/head yet, I see that you can build it, but can pkg get one yet? I would not want to remove it until a binary can be fetched via pkg.

A M lib/libvmmapi/amd64/vmmapi_amd64.h (143 lines)
A M lib/libvmmapi/amd64/vmmapi_amd64.c (1035 lines)
These 2 files are showing up as "added" when clearly they are copied/moved from some place else. This may be an artifact of either git or phabricator and hopefully the svn commit shows the proper logic.

In D17848#381511, @araujo wrote:

In D17848#381508, @rgrimes wrote:

This is good, but it is not clear on *who* can approve as bhyve, in the past that was Peter, is it now jhb & tychon, or anyone who just happens to join the bhyve group, which is not controller.

If the bhyve group cannot be controlled, in this case would be better add the team members name explicitly.
I thought the bhyve group were controlled.

This is good, but it is not clear on *who* can approve as bhyve, in the past that was Peter, is it now jhb & tychon, or anyone who just happens to join the bhyve group, which is not controller.

Proposal: Rename the current bhyvectl code to bhyvedbg, and create a new bhyvectl that has --create, --destory and --list. This could also be the tool to do the bhyvectl --addcpu, --addmemory, --adddisk, etc, etc. This is just a random braincell firing of an idea, anyone and everyone can ignore as they see fit.

I shall re assert my earlier concern that we want to keep bhyvectl purely as a debugging tool, and not start to clutter it with stuff that is not part of that. Yes, there is the use of --destroy by others, but that is and should be the exception rather than the norm.

Sorry for the extra noise here, I did not mean to accept it as bhvye, only me personally, so did delete/add bhyve umbrella to clear that bit.

I am good with this too, other than it is not emulating it, it is just ignoring it by returning a 0, which may or may not have future implications.

In D16894#378335, @imp wrote:

This has been committed..

In D17594#377572, @bz wrote:

Anyone? I'd love to get this in ...

In D17654#377468, @brooks wrote:

@allanjude suggested we might do something like papers.freebsd.org where we run the git repo though go-hugo to produce a formatted web page. That might be worth doing, but is definitely out of scope for the time being.

In D17654#377395, @imp wrote:

I'll point out that https://github.com/freebsd is under FreeBSD Project control, for the most part. It's a risk, but this code likely has a limited enough time horizon to accept that limitation until someone can create a fcp.freebsd.org redirector.

Eg, it's not important enough to gate this commit for that.

One of the reasons for me asking to "macrotise" this is that we can commit it today with a poor URL in it, and very easily correct that URL some time soon(ish),
Brooks if you or Imp write the macro I'll do the leg work of converting the current diff to use that macro and remove all my objections from the current state of affairs.

Does it make any sense at all to #define a macro gone_by_fcp101?

The bhyve group hat was requested by Peter Grehan for "Maintainer" approval and awareness of bhyve bits, as far as I can see this review does not touch any bits that need bhyve maintainer approval, so you can commit with out an accept from them.

Though this may fix running 12.0 on 12.0 we well need EN's back to 11 and perhaps even 10 as the issue exists there as well.
How badly do we need that CLFLUSH instruction in 12?

Please break out the man page changes that are not related to the boot time tuneable into a seperate commit/differential.

Is there anyway to NOT have the 141,000 line xml version of the file stored in SVN?
Also we need to make absolutely sure that any services used by ports are not effected by anything this changes.

In D17040#364538, @emaste wrote:

Some more comments inline - as mentioned earlier we should try to address the root causes, as in D17074. That said, I'm fine with this approach pending root cause fixes (given that it doesn't build today), but want to make sure we have some way to track these outstanding issues such that they'll get fixed after the release.

It just hit me and I have to ask, why isnt libcasper avaliable in a static configuration? I certainly see a /usr/lib/libcasper.a on my systems.

In D16945#361418, @kevans wrote:

(Note to Rod: this is only partially what's going to end up in the review... this isn't a final product for review yet)

Allan, would you update the diff please

To add information, I have email from Paul Vixie and in fact he is wanting to break the reboot loop with -x so that it can be handled externally.

I do not believe this is a desirable change, at least it does not do what it claims it does.

Looks good, my minor nits can be ignored, up to you.
Oh, I see you changed the return code of the module load, I take it that is to lower its preference???

I am sorry for not getting to this today, I was side lined.

I respectfully request that a straight revert of the commits be done rather than this, for one this does not allow us to do the proper deprecation commits, nor to merge those commits back to stable/11. This is still a major violation of deprecation policy. Warner, would you new policy allow this type of thing to be accepted? I would hope not.

Looks ok, though I wonder about a shell script that writes a Makefile, it should be possible to do that in a Makefile by itself, there is the .for/.endfor operator to make which can duplicate what the shell script does.

Can I please have atleast 24 hours to review the text of this.

Im no perl fan so I wont comment on that aspect.

Can you please break the change to share/mk/bsd.confs.mk out into a separate review, though it is required by the moving of the profile files, it is actually not a part of that change directly. I would also like to ask that you tag bdrewery in on the review of the .mk change. I do not see how file owner/group/mode are set in the new Makefile.

Though I disagree with the relocation of this to libc as it is going to be installed with the absolute minimal system anyway so this delta just creates src tree churn, and user finger memory churn. You site your reason for moving it is to put it close to the sources, well, traditionally BSD sources are layed out to match the installed DESTDIR tree. I understand things like csh and sh conf files moving, that makes since in a world where csh or sh may or may not be installed by a pkgbase, however that makes no since in a world where libc and hence master.passwd shall always be installed.

A question has come up, isnt libc part of base, ie even in a pkgbase system libc must be installed? If that is the case then master.passwd must be installed, and I do not see a good compelling reason to move it from its current orthagonal to DESTDIR locatoin in the src tree.

In D16775#356743, @ian wrote:

If using option IPSEC (and I guess GELI and maybe other things) requires explicitly specifying device crypto, then shouldn't this change also cleanup and simplify sys/conf/files to remove "| ipsec | ipsec_support" from all the files that are optional on "crypto"?

And as long as I'm on about consistancy, if we're going to start specifying devices in std.arm* then shouldn't we comb the existing arm kernel config files to find all the common [pseudo-]devices and move them into std.arm* as well? I don't remember why we decided that the std.arm* files would contain only options, but I do vaguely remember discussing it; it wasn't by accident.

This looks okay to me for just the moving of master.passwd, though I am not found of its landing location, but can I ask for 24 hours to put together the net effect patch of what was commited, and partially reverted in combination with this patch to look for any of the pointy sticks that came up. Thanks, Rod

In D16705#355079, @rgrimes wrote:

In D16705#355071, @imp wrote:

In D16705#355069, @rgrimes wrote:

Would there be an objection to using SUBDIR names of loader.4th, loader.lua and loader.simp so that they sort next to each other vs the current names that scattter them about the directory? This also means that loader.foo/Makefile simply includes ../loader/Makefile all nice and orderly :-)

I would object.

We can't install them as loader.lua, loader.4th loader.simp because then we'd have loader.lua.efi, loader.4th.efi etc. Also the .4th suffix is what we use for actual 4th code. So that leaves us with a prefix, even if it is a bit less orderly.

Ack, ok simple enough reasoning.

In D16705#355071, @imp wrote:

In D16705#355069, @rgrimes wrote:

Would there be an objection to using SUBDIR names of loader.4th, loader.lua and loader.simp so that they sort next to each other vs the current names that scattter them about the directory? This also means that loader.foo/Makefile simply includes ../loader/Makefile all nice and orderly :-)

I would object.

We can't install them as loader.lua, loader.4th loader.simp because then we'd have loader.lua.efi, loader.4th.efi etc. Also the .4th suffix is what we use for actual 4th code. So that leaves us with a prefix, even if it is a bit less orderly.

Would there be an objection to using SUBDIR names of loader.4th, loader.lua and loader.simp so that they sort next to each other vs the current names that scattter them about the directory? This also means that loader.foo/Makefile simply includes ../loader/Makefile all nice and orderly :-)

Well, I am still on the fence with this one, seems someone has already done it to the route get -n command.

This is also going to require a man page update, as now there are more than the 4 exit codes listed in the man page.

In D16164#342949, @sbruno wrote:

In D16164#342938, @rgrimes wrote:

Does this not also make it so that these drivers require IFLIB, so they need to be marked as such in GENERIC/NOTES/etc and man pages?

Yeah, definitely. It also means that out of tree drivers (intel, bnxt) wouldn't work if a kernel is built without IFLIB. Is this ok to do?

Does this not also make it so that these drivers require IFLIB, so they need to be marked as such in GENERIC/NOTES/etc and man pages?

Looks good over all, just some nits on errx vs err when it appears that there is a proper errno avaliable.

I think it might be better to have 3 conv types, IDIRECT, ODIRECT, and DIRECT==IDIRECT|ODIRECT. You may or may not want vm effects on specific files.

I actually worry less about man page DEPRECATION, and more about having the binary spit something out so anyone using it for who knows what gets a heads up that it is/may be going away soon.