Re: [PATCH v4 3/3] x86/vmx: Replace __vmread() with vmread()

[dmkhn@xxxxxxxxx]

On Mon, Apr 28, 2025 at 01:58:56PM +0200, Jan Beulich wrote:
> On 26.04.2025 09:28, dmkhn@xxxxxxxxx wrote:
> > @@ -1957,8 +1955,7 @@ 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(HOST_CR4) != read_cr4() )
> 
> It's unclear to me whether this constitutes a violation of Misra rule 13.2.
> There are two function calls in this expression. Both funcs are inline, and
> vmread() is even always_inline, but they're still function calls, which can
> be carried out in either order. And neither can plausibly be pure, let
> alone const.

Thanks, I will revert that hunk.

> 
> > @@ -1573,12 +1565,12 @@ static void cf_check vmx_get_nonreg_state(struct 
> > vcpu *v,
> >  {
> >      vmx_vmcs_enter(v);
> >
> > -    __vmread(GUEST_ACTIVITY_STATE, &nrs->vmx.activity_state);
> > -    __vmread(GUEST_INTERRUPTIBILITY_INFO, &nrs->vmx.interruptibility_info);
> > -    __vmread(GUEST_PENDING_DBG_EXCEPTIONS, &nrs->vmx.pending_dbg);
> > +    nrs->vmx.activity_state = vmread(GUEST_ACTIVITY_STATE);
> 
> This struct field can now also shrink down to unsigned int (or uit32_t if need
> be). Likely there are others (and maybe also plain variables). Which in turn
> is an indication that, as was suggested before, this patch wants further
> splitting up. That'll also help with overall progress, as then what's
> uncontroversial and not in need of further adjustment can go in right away.

I'll will minimize the churn more in next iteration.

> 
> Jan