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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> 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.

> --- 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.

Jan

[1] Quote: "Note that the compiler can move even volatile asm instructions
            relative to other code, including across jump instructions."