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

[neel]

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.

[tychon]

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]

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

[neel]

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]

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]

Thanks, that makes sense. Looks good to me!