Page MenuHomeFreeBSD

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

Authored by rgrimes on Sat, Jan 5, 5:18 PM.

Details

Reviewers
jhb
tychon
pmooney_pfmooney.com
Group Reviewers
bhyve
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.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

rgrimes created this revision.Sat, Jan 5, 5:18 PM

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?

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.

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 added a subscriber: araujo.Tue, Jan 8, 4:46 PM

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

...

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.

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 updated this revision to Diff 52745.Thu, Jan 10, 4:46 PM
rgrimes retitled this revision from Relocate struct vm; and associated sub structures to a new header file to Add accessor for vm->maxcpus in preperation for run time maxcpu setting.
rgrimes edited the summary of this revision. (Show Details)

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

rgrimes updated this revision to Diff 52899.Wed, Jan 16, 9:39 PM

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

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.

rgrimes edited the test plan for this revision. (Show Details)Wed, Jan 16, 9:59 PM