Note that vmm_fetch_instruction() can return EFAULT, so the caller's test s >= 0 might ignore errors.

It would be better to return the segment number using a separate parameter, and use the return value to signal an error or success (0).

I'd prefer this to be an explicit panic().

Why not use vmm_decode_instruction()? It will find the segment index for you, and also handles the rep prefix. From reading the OUTS instruction description, I think you have to handle the default prefix case too.

Should we initialize fault to 0? I believe (albeit with invalid inputs) vm_copy_setup can return non-zero but not set *fault and it seems generally reasonable to ensure it's initialized.

I thought

if (err || fault)

have already addressed that?

Right, this code follows an existing pattern. vm_fetch_instruction() will initialize fault when it returns 0, so we have to check for errors first.

You need to provide a default value if there was no segment prefix.

I don't think the value vme->u.inst_emul.gla is valid. That gets set in svm_inout_str_seginfo(), but that's for a VMCB_EXIT_NPF exit.

Actually, does this use of vmm_decode_instruction() work at all? outs is not listed in one_byte_opcodes[], so won't decode_opcode() fail?

I think we want to also verify that the decoded instruction is an outs, since it's possible that the guest modified the instruction, but see my other comment above.

I do check on the Opteron machine and the guest starts without problem. I also check the kernel without this patch and it shows the expected vmexit fault and the bhyve stop immediately. But you are right, it is strange that decode_opcode works since the instruction is not listed.

vm exit[0]
reason SVM
rip 0x00000000bfa9eac0
inst_length 2
exitcode 0x7b
exitinfo1 0x511021d
exitinfo2 0xbfa9eac2

Ah, I think you are missing a vie_init() call?

Emm, I don't think vie_init is necessary since it just copy the instruction buffer and set the length. In the previous codes do fetch + decode inside the
vmm.c, it call decode after fetch immediately without calling vie_init.

I try to do some printf in vmm.ko and it shows the binary only execute INS without OUTS. That is the reason why it works since the original check in

	if (inout_string && !decode_assist())
		return (UNHANDLED);

is regardless of you are in or out. Also, the in direction cannot be overrides. A quick workaround now is that we can move the UNHANDLED check to svm_inout_str_seginfo and make it check the out direction only.

However, what we need actually is the prefix only. Maybe we can make decode_prefix global then call decode_prefix only?

Emm, I don't think vie_init is necessary since it just copy the instruction buffer and set the length.

Well, it also zeroes the structure. In particular, vie.op is zeroed, so vie->op.op_type == VIE_OP_TYPE_NONE. I think the test in decode_opcode() is returning early because it's testing uninitialized memory.

In the previous codes do fetch + decode inside the
vmm.c, it call decode after fetch immediately without calling vie_init.

There, the vie_init() happens in vmexit_inst_emul(), before we exit the inner loop. After exiting the loop, vm_handle_inst_emul() uses it (possibly more than once).

I try to do some printf in vmm.ko and it shows the binary only execute INS without OUTS.

So, let's try handling only INS then? Assuming that that's sufficient, anyway. Then we can hard-code ES.

BTW, did you verify that other info1 fields are correct, even without DecodeAssist?

However, what we need actually is the prefix only. Maybe we can make decode_prefix global then call decode_prefix only?

I prefer not to take such shortcuts. If we want to handle OUTS as well, then we should 1) fetch and decode the full instruction, 2) verify that it is indeed an OUTS, 3) extract the segment register.

This definitely needs a comment explaining what we're doing here.

This is too magical. I prefer something like the following:

1. Observe that we in fact can add fields to the end of struct vm_inout_str without breaking the userspace ABI. This is because it is only used in a union and some other fields, especially inst_emul, are larger. Please double-check this.
2. Add a cs_d field, like inst_emul has.
3. In the SVM layer, factor out the code in svm_handle_inst_emul() which fetches the code segment descriptor and default operand size bit, so it can be shared with the OUTS handler.
4. Save the code segment descriptor in inout_str.seg_desc, save the default operand size bit in inout_str.cs_d.
5. Use those fields here.

This should still verify that the decoded instruction is an outs.

Observe that we in fact can add fields to the end of struct vm_inout_str without breaking the userspace ABI. This is because it is only used in a union and some other fields, especially inst_emul, are larger. Please double-check this.

Checked by _Static_assert

Now it is much easier to see that error != 0 is impossible here. So, I think the explicit panic() is no longer necessary. Let's make svm_get_cs_info() return void, and internally it can assert error == 0, similar to VMX inout_str_seginfo().

Please initialize these fields in VMX inout_str_seginfo() too.

It is ok to move the enum to vmm_instruction_emul.h (just be careful to verify that bhyve still compiles, since vmm_instruction_emul.* is compiled in userspace as well). Or, you can add some predicate function vie_instruction_is_outs() to vmm_instruction_emul.c. Either option is fine IMO.