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

[dmkhn@xxxxxxxxx]

On Fri, May 16, 2025 at 01:42:23PM +0100, Andrew Cooper wrote:
> 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.

Sorry, I interpreted v4 feedback as "first, do straight reuse of vmread()
everywhere, then send follow on smaller patches cleaning up the code around
vmread()s".

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

Thanks for the feedback!

> 
> ~Andrew