Re: [PATCH v1] x86/vmx: Rework __vmread()/vmread_safe()/vmr()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2025 03:15, dmkhn@xxxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
> 
> Use `asm goto()` in vmread_safe() to simplify the error handling logic.

This can't be quite right, considering we need to avoid outputs there.

> Update __vmread() to return `unsigned long` as per suggestion in [1].
> Rename __vmread() to vmread_unsafe() to match the behavior.

I disagree with this renaming: See e.g. rdmsr() and rdmsr_safe() that we have.
The common case function wants to not have unnecessary verbosity in its name.
And there's nothing unsafe about it in the general case. Plus if there was
anything unsafe, many of the call sites would require some form of error
handling.

> @@ -1957,38 +1955,44 @@ void cf_check vmx_do_resume(void)
>      hvm_do_resume(v);
>  
>      /* Sync host CR4 in case its value has changed. */
> -    __vmread(HOST_CR4, &host_cr4);
> -    if ( host_cr4 != read_cr4() )
> +    if ( vmread_unsafe(HOST_CR4) != read_cr4() )
>          __vmwrite(HOST_CR4, read_cr4());
>  
>      reset_stack_and_jump(vmx_asm_do_vmentry);
>  }
>  
> -static inline unsigned long vmr(unsigned long field)
> +static inline unsigned long vmread(unsigned long field)
>  {
> -    unsigned long val;
> +    unsigned long value = 0;
>  
> -    return vmread_safe(field, &val) ? 0 : val;
> +    asm goto ( "vmread %[field], %[value]\n\t"
> +               "jmp %l[out]"

Why's the JMP needed here? With it dropped, why's open-coding of vmread_unsafe()
necessary here? And why's the "safe" variant being replaced by the "unsafe" one?

> +               :
> +               : [field] "r" (field), [value] "m" (value)

"value" is an output and hence cannot be just "m" (and hence be an input").
The only option to make such work correctly would be to ...

> +               :

... add a "memory" clobber here. Which may have other unwanted side effects.

> +               : out );
> +out:

Nit (here and elsewhere): Labels indented by at least one blank please. See
./CODING_STYLE.

> +    return value;
>  }
>  
> -#define vmr16(fld) ({             \
> +#define vmread16(fld) ({          \
>      BUILD_BUG_ON((fld) & 0x6001); \
> -    (uint16_t)vmr(fld);           \
> +    (uint16_t)vmread(fld);        \
>  })
>  
> -#define vmr32(fld) ({                         \
> +#define vmread32(fld) ({                      \
>      BUILD_BUG_ON(((fld) & 0x6001) != 0x4000); \
> -    (uint32_t)vmr(fld);                       \
> +    (uint32_t)vmread(fld);                    \
>  })
>  
>  static void vmx_dump_sel(const char *name, uint32_t selector)
>  {
>      uint32_t sel, attr, limit;
>      uint64_t base;
> -    sel = vmr(selector);
> -    attr = vmr(selector + (GUEST_ES_AR_BYTES - GUEST_ES_SELECTOR));
> -    limit = vmr(selector + (GUEST_ES_LIMIT - GUEST_ES_SELECTOR));
> -    base = vmr(selector + (GUEST_ES_BASE - GUEST_ES_SELECTOR));
> +    sel = vmread(selector);
> +    attr = vmread(selector + (GUEST_ES_AR_BYTES - GUEST_ES_SELECTOR));
> +    limit = vmread(selector + (GUEST_ES_LIMIT - GUEST_ES_SELECTOR));
> +    base = vmread(selector + (GUEST_ES_BASE - GUEST_ES_SELECTOR));

The renaming causes entirely unnecessary extra churn here (and of course
elsewhere). The patch is already big enough without this.

> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -595,7 +595,7 @@ struct arch_vcpu
>  
>      /* Debug registers. */
>      unsigned long dr[4];
> -    unsigned long dr7; /* Ideally int, but __vmread() needs long. */
> +    unsigned long dr7; /* Ideally int, but vmread_unsafe() needs unsigned 
> long. */
>      unsigned int dr6;

If you left this comment alone, all would be (largely) fine - this particular
aspect could then be tidied in a follow-on path. But vmread_unsafe() 
specifically
does not need "unsigned long" anymore. The issue was with __vmread() taking a
pointer argument.

> --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h
> @@ -320,16 +320,40 @@ static always_inline void __vmpclear(u64 addr)
>      BUG();
>  }
>  
> -static always_inline void __vmread(unsigned long field, unsigned long *value)
> +static always_inline unsigned long vmread_unsafe(unsigned long field)
>  {
> -    asm volatile ( "vmread %1, %0\n\t"
> -                   /* CF==1 or ZF==1 --> BUG() */
> -                   UNLIKELY_START(be, vmread)
> -                   _ASM_BUGFRAME_TEXT(0)
> -                   UNLIKELY_END_SECTION
> -                   : "=rm" (*value)
> -                   : "r" (field),
> -                     _ASM_BUGFRAME_INFO(BUGFRAME_bug, __LINE__, __FILE__, 0) 
> );
> +    unsigned long value;
> +
> +    asm volatile ( "vmread %[field], %[value]\n\t"
> +                   "jc 1f\n\t"
> +                   "jz 1f\n\t"

Why not JBE as it was before?

> +                   "jmp 2f\n\t"
> +                   "1:\n\t"
> +                   "    ud2\n\t"
> +                   "2:"

This is specifically why we used UNLIKELY_*() before. There's no justification
whatsoever in the description for the dropping of its use here.

Plus - where did _ASM_BUGFRAME_TEXT(0) go? A bare UD2 isn't acceptable, as it
won't be possible to associate it back with the respective source line.

> +                   : [value] "=rm" (value)
> +                   : [field] "r" (field) );
> +
> +    return value;
> +}
> +
> +static inline enum vmx_insn_errno vmread_safe(unsigned long field,
> +                                              unsigned long *value)
> +{
> +    asm goto ( "vmread %[field], %[value]\n\t"
> +               "jc %l[vmfail_invalid]\n\t"
> +               "jz %l[vmfail_error]"
> +               :
> +               : [field] "r" (field), [value] "m" (*value)

See comments on the vmr() adjustments you're making.

Jan