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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/04/2025 2:15 am, dmkhn@xxxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
>
> Use `asm goto()` in vmread_safe() to simplify the error handling logic.
>
> Update __vmread() to return `unsigned long` as per suggestion in [1].
> Rename __vmread() to vmread_unsafe() to match the behavior.
> Update all call sites everywhere. Drop `UNLIKELY_*()`.
>
> Group all vmread*() calls close to each other in vmx.h
>
> Rename internal vmr*() to vmread*().
>
> [1] 
> https://lore.kernel.org/xen-devel/c452a1d7-4a57-4c5f-8a83-36a74ff228ec@xxxxxxxxxx/
>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Link to CI: 
> https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/1756781092
> ---
>  xen/arch/x86/cpu/vpmu_intel.c          |   3 +-
>  xen/arch/x86/hvm/vmx/intr.c            |  26 +--
>  xen/arch/x86/hvm/vmx/realmode.c        |   2 +-
>  xen/arch/x86/hvm/vmx/vmcs.c            | 160 ++++++++++---------
>  xen/arch/x86/hvm/vmx/vmx.c             | 209 +++++++++++--------------
>  xen/arch/x86/hvm/vmx/vvmx.c            |  43 +++--
>  xen/arch/x86/include/asm/domain.h      |   2 +-
>  xen/arch/x86/include/asm/hvm/vmx/vmx.h |  69 ++++----
>  8 files changed, 235 insertions(+), 279 deletions(-)

This is why I suggested not to convert everything in one go.  It's now a
patch doing multiple complicated things, and is proportionally harder to
review.

For everyone in public, it is especially daft that we have __vmread()
which is void and (if it doesn't BUG()) will pass it's return value by
pointer.  It leads to very unergonomic logic.

Start by just implementing vmread(), and updating __vmread() and
vmwrite_safe() to use it.  You cannot use asm goto() for vmread()
because of the no-outputs constraint that we still need to follow.

Then, in a separate patch, you can do simple conversion such as ...

>
> diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
> index 7ce98ee42e..9c93d1f28c 100644
> --- a/xen/arch/x86/cpu/vpmu_intel.c
> +++ b/xen/arch/x86/cpu/vpmu_intel.c
> @@ -796,8 +796,7 @@ static int cf_check core2_vpmu_do_interrupt(void)
>      else
>      {
>          /* No PMC overflow but perhaps a Trace Message interrupt. */
> -        __vmread(GUEST_IA32_DEBUGCTL, &msr_content);
> -        if ( !(msr_content & IA32_DEBUGCTLMSR_TR) )
> +        if ( !(vmread_unsafe(GUEST_IA32_DEBUGCTL) & IA32_DEBUGCTLMSR_TR) )
>              return 0;
>      }
>  

... this to vmread().  

Splitting the patch makes a substantial difference to review-ability,
because patch 1 is "is this new helper implemented correctly?", and
patch 2 is "is this boilerplate rearrangement no overall change?".

For vmr(), I'd start by just wrapping vmread().  It's debugging logic
where brevity is important.

> diff --git a/xen/arch/x86/include/asm/domain.h 
> b/xen/arch/x86/include/asm/domain.h
> index 6b877e33a1..ffe9acd75d 100644
> --- 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;

This comment was left as a hint, and you've just addressed the problem
forcing it to stay unsigned long.

Changing dr7 should be in a separate patch too.

~Andrew