Re: [PATCH] x86/vmx: Drop memory clobbers on VMX instruction wrappers

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/04/2025 1:00 pm, Jan Beulich wrote:
> On 07.04.2025 12:45, Andrew Cooper wrote:
>> The use, or not, of memory clobbers on the VMX instructions is complicated.
>>
>> There are two separate aspects to consider:
>>
>> 1. Originally, the VMX instructions used hardcoded bytes, including memory
>>    encodings.  Therefore, the compiler couldn't see the correct relationship
>>    between parameters.  The memory clobber for this purpose should have been
>>    dropped when switching to mnemonics.
>>
>>    This covers INVEPT and INVVPID, each of which has no change in memory, nor
>>    in fact the current address space in use.
> Yet then they need to come after respective table modifications.

They don't AFAICT, but the reasoning is complicated.  I'll expand on it
in v2.

>
>> 2. Most of these instructions operate on a VMCS region.  This is a (mostly)
>>    opaque data structure, operated on by physical address.  Again, this hides
>>    the relationship between the instructions and the VMCS from the compiler.
>>
>>    The processor might use internal state to cache the VMCS (writing it back
>>    to memory on VMCLEAR), or it might operate on memory directly.
>>
>>    Because the VMCS is opaque (so the compiler has nothing interesting to 
>> know
>>    about it), and because VMREAD/VMWRITE have chosen not to use a memory
>>    clobber (to tell the compiler that something changed), none of the other
>>    VMX instructions should use a memory clobber either.
> For this, there's actually a good example below, with everything needed in
> context.
>
>>    This covers VMXON, VMXOFF, VMPTRLD and VMPTCLEAR.
> Nit: The last insn is VMCLEAR.

Oh, so it is, and we've got an incorrectly named wrapper.

>
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -754,7 +754,7 @@ static int _vmx_cpu_up(bool bsp)
>>                 _ASM_EXTABLE(1b, %l[vmxon_fault])
>>                 :
>>                 : [addr] "m" (this_cpu(vmxon_region))
>> -               : "memory"
>> +               :
>>                 : vmxon_fail, vmxon_fault );
>>  
>>      this_cpu(vmxon) = 1;
>> @@ -811,7 +811,7 @@ void cf_check vmx_cpu_down(void)
>>  
>>      BUG_ON(!(read_cr4() & X86_CR4_VMXE));
>>      this_cpu(vmxon) = 0;
>> -    asm volatile ( "vmxoff" ::: "memory" );
>> +    asm volatile ( "vmxoff" );
> With the clobber dropped, the compiler is free to re-order the prior store
> with the asm(), despite the "volatile", isn't it? [1] This may then be
> applicable elsewhere as well.

Yeah, these might better stay as they are.

~Andrew