Add accessor for vm->maxcpus in preperation for run time maxcpu setting

[rgrimes]

Summary

In working on the dynamic VM_MAXCPU code it has become necessary to access information inside struct vm; from several source files. The original idea of exposing the struct outside vmm.c has been changed to add an accessor function vm_get_maxcpus()

Test Plan

Compile vmm.ko, bhyveload, and bhyve. Load a 16 vCPU guest FreeBSD 12.0-RELEASE test vm on both Intel and Amd systems.

[pmooney_pfmooney.com]

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?

[rgrimes]

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?

I thought about that, I could use the topology accessors to rewrite all the

if (vcpuid < 0 ||vcpuid >VM_MAXCPU)

or I could add a vmm_topology_rangecheck(vcpuid) function either of these add a function call. If we are really hung up on the idea of accessors and such I can do that, but 90% of these accesses to this value are in the vmm.c file, with 4 other filex wanting access to the values.

[rgrimes]

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!

[araujo]

In D18755#400855, @rgrimes wrote:

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!

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

[rgrimes]

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.

[pmooney_pfmooney.com]

In D18755#400855, @rgrimes wrote:

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!

I think there's precedent for consumers inside vmm.c to simply access the struct member, rather than needing to pass through the accessor. Adding the accessor would obviate the need to make the struct vm/struct vcpu definitions visible to the rest of the vmm module.

[rgrimes]

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

[rgrimes]

Add convertion of most VM_MAXCPU references to either vm->maxcpus (vmm.c) or to use accessor function vm_get_maxcpus().

[rgrimes]

In D18755#400877, @pmooney_pfmooney.com wrote:

In D18755#400855, @rgrimes wrote:

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!

I think there's precedent for consumers inside vmm.c to simply access the struct member, rather than needing to pass through the accessor. Adding the accessor would obviate the need to make the struct vm/struct vcpu definitions visible to the rest of the vmm module.

I have rewritten my changes to use either the accessor if from external to vmm.c or using direct struct member reference if inside vmm.c. There are still a few left to fix that did not lend themselves to direct mechanical change that I am working on.

[jhb]

I think using an accessor is a good path forward.

sys/amd64/vmm/vmm.c

469 ↗

(On Diff #52899)

I think you can drop this line.

506 ↗

(On Diff #52899)

I think you can drop this line, especially given the XXX on line 499 after the maxcpus != 0 check.

[rgrimes]

sys/amd64/vmm/vmm.c

469 ↗

(On Diff #52899)

This line is needed for other work that checks this 0 value to mean "we have not allocated" data structures. Also I didnt touch this in this review, so that would be a change beyond the purpose of this code review.

506 ↗

(On Diff #52899)

This and line 499 are handled in other code work, for now there is no need to change this here to make this patch do what it needs to do.

[jhb]

sys/amd64/vmm/vmm.c

469 ↗

(On Diff #52899)

Usually I would wait to include that until the change that actually uses it. As it is, it looks odd in this change.

[rgrimes]

sys/amd64/vmm/vmm.c

469 ↗

(On Diff #52899)

John, if you do NOT set this value here all the accessors used to replace VM_MAXCPU by the vm->maxcpus would get a value of 0 seriously breaking things.

[jhb]

sys/amd64/vmm/vmm.c

469 ↗

(On Diff #52899)

Huh? The comment is about the 'maxcpus = 0' line that is pointless since the next line sets it to VM_MAXCPU.

[rgrimes]

Update to incorporate feedback from bde@ (mentor), wrap long lines, sort prototypes within group vm_get* in sys/amd64/include/vmm.h.

[pmooney_pfmooney.com]

sys/amd64/vmm/vmm.c

469 ↗

(On Diff #54175)

Nuke the 0 assignment so that maxcpus is never in what would now be an illegal state?

506 ↗

(On Diff #54175)

Nuke the = maxcpus assignment so that a smaller provided value won't temporarily put the vm in an illegal state?

sys/amd64/vmm/vmm_dev.c

140 ↗

(On Diff #54175)

Cache the max here?

159 ↗

(On Diff #54175)

Cache the max?

206 ↗

(On Diff #54175)

Cache the max - 1 once, instead of recalculating it all over the function?

[rgrimes]

sys/amd64/vmm/vmm.c

469 ↗

(On Diff #54175)

Agreed, I need to update the diff and upload again.

506 ↗

(On Diff #54175)

Agreed, I should of done this yesterday right after the call when it was fresh in my mind, I had meant to nuke this as per the call discussion.

469 ↗

(On Diff #52899)

Let me try again. The original line of code that sets this to 0 and has the comment not implemented is still correct, in the since I have no reason to change it at this time. BUT if I do not change the value stored in this (the following line) the code would break bhyve in a very bad way, so I choose to leave the line that shall become valid later, and have its comment removed when it "is implemented", ie initing this to 0 is the correct long term value, and added a temporary line that shall be later deleted when the code can handle the fact that vm->maxcpus might be 0 and some mallocs need to be done. Since you have accepted the code anyway we should probably just drop this issue and move forward as this all cleans up when the final version is committed.

sys/amd64/vmm/vmm_dev.c

159 ↗

(On Diff #54175)

I would hope the compiler would do that already without me having to explicitly code code this as
foo = vm_get_max()
for (; vcpu < foo); vcpu++)
Ah, bleh, it can not do that, these are not inlined so it does not know that there are no side effects from that function call. Ok, jhb@ do you agree I should probably cache these values locally?

Also doesn't this go away when we upstream the better locking implementation you have already done?

206 ↗

(On Diff #54175)

Will do

[pmooney_pfmooney.com]

sys/amd64/vmm/vmm_dev.c

159 ↗

(On Diff #54175)

There would be some caching needed in vcpu_lock_all(), which would become part of the "write lock acquisition" path. It would be safe for the unlock path, since maxcpu would be effectively fixed while you held the write lock.

[rgrimes]

Address review comments from Patrick Mooney. Found some additional places that caching would be helpful so update them too.

[jhb]

sys/amd64/vmm/vmm_dev.c

159 ↗

(On Diff #54175)

You can tell the compiler that the return value is always the same for a given input by marking the function prototype with '__pure2' or some such in which case the compiler would cache the return value and only call it once. What you have is fine though.