Re: [PATCH v2 5/4] x86/hvm: Drop hvm_{get,set}_guest_bndcfgs() and use {get,set}_regs() instead

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Jun Nakajima" <jun.nakajima@intel.com>,
	"Kevin Tian" <kevin.tian@intel.com>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v2 5/4] x86/hvm: Drop hvm_{get,set}_guest_bndcfgs() and use {get,set}_regs() instead
Date: Wed, 19 Jan 2022 14:50:58 +0100	[thread overview]
Message-ID: <93ec9bab-0a21-365b-3bcd-0fbf70bdbbbb@suse.com> (raw)
In-Reply-To: <20220117192533.6048-1-andrew.cooper3@citrix.com>

On 17.01.2022 20:25, Andrew Cooper wrote:
> hvm_{get,set}_guest_bndcfgs() are thin wrappers around accessing MSR_BNDCFGS.
> 
> MPX was implemented on Skylake uarch CPUs and dropped in subsequent CPUs, and
> is disabled by default in Xen VMs.
> 
> It would be nice to move all the logic into vmx_msr_{read,write}_intercept(),
> but the common HVM migration code uses guest_{rd,wr}msr().  Therefore, use
> {get,set}_regs() to reduce the quantity of "common" HVM code.
> 
> In lieu of having hvm_set_guest_bndcfgs() split out, use some #ifdef
> CONFIG_HVM in guest_wrmsr().  In vmx_{get,set}_regs(), split the switch
> statements into two depending on whether the require remote VMCS acquisition
> or not.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

One remark:

> @@ -323,10 +324,9 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>          break;
>  
>      case MSR_IA32_BNDCFGS:
> -        if ( !cp->feat.mpx || !is_hvm_domain(d) ||
> -             !hvm_get_guest_bndcfgs(v, val) )
> +        if ( !cp->feat.mpx ) /* Implies Intel HVM only */

Wouldn't it make sense to accompany this comment by ...

>              goto gp_fault;
> -        break;

    ASSERT(is_hvm_domain(d));

(and then the same on the "set" path)?

Jan