Re: [PATCH 5/6] vVMX: operand size in decode_vmx_inst()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.06.2025 13:32, Andrew Cooper wrote:
> On 11/06/2025 11:44 am, Jan Beulich wrote:
>> Address size is entirely irrelevant to operand size determination; For
>> VMREAD and VMWRITE outside of 64-bit mode operand size is 32 bits, while
>> in 64-bit mode it's (naturally) 64 bits. For all other insns it's 64
>> bits (a physical address) or 128 bits (INVEPT, INVVPID). To limit the
>> amount of change here, keep the latter at reading only 64 bits from
>> guest space.
>>
>> Fixes: 09fce8016596 ("Nested VMX: Emulation of guest VMXON/OFF instruction")
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> Beyond the wrong operand handling for INVEPT and INVVPID, the latter
>> also doesn't even have the part read checked to have bits 16 and above
>> all clear.
> 
> There are more bugs than these.
> 
> https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/x86-next-euan
> 
> Judging by the choice of branch name, I think I'd collected those
> pending the re-opening of a dev window, and apparently forgotten.
> 
> At this point I don't think we want to take the branch as is (especially
> as I've forgotten why it wasn't taken before), but I think there is some
> salvageable work in there, beyond the changes in this patch.

Interesting. Apparently it has been too long since this was posted, as I
don't recall it at all. However, I'm not convinced everything's right
there. E.g. in "x86/vvmx: Use correct sizes when reading operands" there
is this hunk:

@@ -427,6 +427,7 @@ static int decode_vmx_inst(struct cpu_user_regs *regs,
     struct segment_register seg;
     unsigned long base, index, seg_base, disp, offset;
     int scale, size;
+    unsigned int bytes = vmx_guest_x86_mode(v);
 
     __vmread(VMX_INSTRUCTION_INFO, &offset);
     info.word = offset;

Which is hardly correct for 16-bit protected mode. INVEPT and INVVPID
handling also isn't moving to 128-bit operands, despite what the title
suggests. The commit message wrongly claims the upper halves don't need
to be read.

Jan