Re: [PATCH v5 1/2] x86/vmx: replace __vmread() with vmread()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/05/2025 6:28 am, dmkhn@xxxxxxxxx wrote:
> diff --git a/xen/arch/x86/hvm/vmx/intr.c b/xen/arch/x86/hvm/vmx/intr.c
> index 91b407e6bc..b622ae1e60 100644
> --- a/xen/arch/x86/hvm/vmx/intr.c
> +++ b/xen/arch/x86/hvm/vmx/intr.c
> @@ -65,7 +65,7 @@ static void vmx_enable_intr_window(struct vcpu *v, struct 
> hvm_intack intack)
>      {
>          unsigned long intr;
>  
> -        __vmread(VM_ENTRY_INTR_INFO, &intr);
> +        intr = vmread(VM_ENTRY_INTR_INFO);
>          TRACE(TRC_HVM_INTR_WINDOW, intack.vector, intack.source,
>                (intr & INTR_INFO_VALID_MASK) ? intr & 0xff : -1);
>      }

As Jan said in v4, lots of these should now change away from being
unsigned long.

For example, this delta alone:

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 203ca83c16e7..c540ea5bd850 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -4154,9 +4154,8 @@ static void undo_nmis_unblocked_by_iret(void)
 
 void asmlinkage vmx_vmexit_handler(struct cpu_user_regs *regs)
 {
-    unsigned long exit_qualification, exit_reason, idtv_info, intr_info
= 0;
-    unsigned long cs_ar_bytes = 0;
-    unsigned int vector = 0;
+    unsigned long exit_qualification;
+    unsigned int exit_reason, idtv_info, intr_info = 0, cs_ar_bytes =
0, vector = 0;
     struct vcpu *v = current;
     struct domain *currd = v->domain;
 
@@ -4830,7 +4829,7 @@ void asmlinkage vmx_vmexit_handler(struct
cpu_user_regs *regs)
     /* fall through */
     default:
     exit_and_crash:
-        gprintk(XENLOG_ERR, "Unexpected vmexit: reason %lu\n",
exit_reason);
+        gprintk(XENLOG_ERR, "Unexpected vmexit: reason %u\n", exit_reason);
 
         if ( vmx_get_cpl() )
             hvm_inject_hw_exception(X86_EXC_UD,

results in:

    add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-331 (-331)
    Function                                     old     new   delta
    vmx_vmexit_handler.cold                      929     839     -90
    vmx_vmexit_handler                          5490    5249    -241

worth of saving in the fastpath.  (Yes, I chose this example carefully
because it's surely the largest win to be had.)

I've just sent out a minor docs patch annotating the sizes of the fields.

This patch wants splitting into at least 3:

 * One for the 64bit and natural fields which are a straight transform
and no type-change away from unsigned long.
 * One for the 16bit fields (there are few enough that this can easily
be a single patch).
 * One or more for the 32bit fields, doing a type change to unsigned int
too.  (Might get quite large.  Hard to judge whether it wants to be one
or more without seeing it.)

~Andrew