Page MenuHomeFreeBSD

Deprecate the 3-way return values from vm_gla2gpa() and vm_copy_setup().
ClosedPublic

Authored by neel on May 2 2015, 3:54 AM.

Details

Summary

Prior to this change both functions returned 0 for success, -1 for failure
and +1 to indicate that an exception was injected into the guest.

The numerical value of ERESTART also happens to be -1 so when these functions
returned -1 it had to be translated to a positive errno value to prevent the
VM_RUN ioctl from being inadvertently restarted. This made it easy to introduce
bugs when writing emulation code.

Fix this by adding an 'int *guest_fault' parameter and setting it to '1' if
an exception was delivered to the guest. The return value is 0 or EFAULT so
no additional translation is needed.

Test Plan

Booted OpenBSD/i386 to test the PUSH/POP emulation.

Boot FreeBSD/i386 and overflow the kernel stack to trigger a double fault
and test the task switch emulation.
https://people.freebsd.org/~neel/bhyve/tests/kstackovf.tar.gz

Boot FreeBSD/i386 and install NMI handler that is invoked via task switching.
Verify that entry and exit from the NMI handler works as expected.
https://people.freebsd.org/~neel/bhyve/tests/task_switch_nmi.tar.gz

Boot FreeBSD/amd64 and execute the 'ins_test' to test the inout_str handling.
https://people.freebsd.org/~neel/bhyve/tests/inout_str.tar.gz

Boot FreeBSD/amd64 with the "dummy" device and use that to test MOVS:

  • 'dd' memory pattern to dummy device BAR1.
  • 'dd' from dummy device BAR1 and BAR2.
  • 'dd' from dummy device BAR2 to memory and verify it matches original pattern.

Boot Centos and Ubuntu guests.

Verify that this patch works on both AMD and Intel processors.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

neel retitled this revision from to Deprecate the 3-way return values from vm_gla2gpa() and vm_copy_setup()..May 2 2015, 3:54 AM
neel updated this object.
neel added reviewers: grehan, tychon.
neel edited the test plan for this revision. (Show Details)
neel updated this revision to Diff 5155.
tychon edited edge metadata.May 4 2015, 2:02 PM

Hi,

This looks very nice. My only feedback is that I got confused as to whether or not the "return fault (*fault)" provided by vmm_fetch_instruction was boolean or not. In most cases it was treated as such, but you've actually got the information to do better. Specifically around line 624 in the new vmm_instruction_emul.c you could set *fault to IDT_SS or IDT_GP. If you think no one will ever care about the specific fault, perhaps renaming fault to is_fault would further cement it's boolean nature.

Also as a mini-nit I'd suggest changing 'guest_fault' in the comment to 'fault' to match the function prototype vmm_instruction_emul.h

Tycho

neel edited edge metadata.May 4 2015, 4:40 PM
neel updated this revision to Diff 5179.

Update all extern function declarations to use 'int *is_fault' consistently.

neel added a comment.May 4 2015, 4:42 PM
In D2428#44714, @tychon wrote:

Hi,
This looks very nice. My only feedback is that I got confused as to whether or not the "return fault (*fault)" provided by vmm_fetch_instruction was boolean or not. In most cases it was treated as such, but you've actually got the information to do better. Specifically around line 624 in the new vmm_instruction_emul.c you could set *fault to IDT_SS or IDT_GP. If you think no one will ever care about the specific fault, perhaps renaming fault to is_fault would further cement it's boolean nature.
Also as a mini-nit I'd suggest changing 'guest_fault' in the comment to 'fault' to match the function prototype vmm_instruction_emul.h
Tycho

Ok, thanks. I have updated both vmm.h and vmm_instruction_emul.h to use 'int *is_fault' consistently so it is clear that its contents are boolean.

neel added a comment.May 4 2015, 5:57 PM
In D2428#44714, @tychon wrote:

Hi,
In most cases it was treated as such, but you've actually got the information to do better. Specifically around line 624 in the newvmm_instruction_emul.c you could set *fault to IDT_SS or IDT_GP. If you think no one will ever care about the specific fault, perhaps renaming fault to is_fault would further cement it's boolean nature.

Yup, it is definitely intended to be a boolean.

In all cases we have so far the caller doesn't need to care about the specific exception that was injected. If there arises a case where a caller does care then they could use 'vm_get_intinfo()' to inspect the specific exception that was injected.

In D2428#44714, @tychon wrote:

Hi,
This looks very nice. My only feedback is that I got confused as to whether or not the "return fault (*fault)" provided by vmm_fetch_instruction was boolean or not. In most cases it was treated as such, but you've actually got the information to do better. Specifically around line 624 in the new vmm_instruction_emul.c you could set *fault to IDT_SS or IDT_GP. If you think no one will ever care about the specific fault, perhaps renaming fault to is_fault would further cement it's boolean nature.
Also as a mini-nit I'd suggest changing 'guest_fault' in the comment to 'fault' to match the function prototype vmm_instruction_emul.h
Tycho

tychon edited edge metadata.May 4 2015, 6:01 PM
tychon accepted this revision.

Thanks, that makes sense. Looks good to me!

This revision is now accepted and ready to land.May 4 2015, 6:01 PM
This revision was automatically updated to reflect the committed changes.