Re: [PATCH 2/4] x86/svm: split svm_intercept_msr() into svm_{set,clear}_msr_intercept()

From: Jan Beulich <jbeulich@suse.com>
To: Xenia Ragiadakou <burzalodowa@gmail.com>
Cc: "Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH 2/4] x86/svm: split svm_intercept_msr() into svm_{set,clear}_msr_intercept()
Date: Tue, 28 Feb 2023 15:20:26 +0100	[thread overview]
Message-ID: <bf77733a-d0cc-8e31-5a05-f0709e7ef1fb@suse.com> (raw)
In-Reply-To: <20230227075652.3782973-3-burzalodowa@gmail.com>

On 27.02.2023 08:56, Xenia Ragiadakou wrote:
> This change aims to render the control interface of MSR intercepts identical
> between SVM and VMX code, so that the control of the MSR intercept in common
> code can be done through an hvm_funcs callback.
> 
> Create two new functions:
> - svm_set_msr_intercept(), enables interception of read/write accesses to the
>   corresponding MSR, by setting the corresponding read/write bits in the MSRPM
>   based on the flags
> - svm_clear_msr_intercept(), disables interception of read/write accesses to
>   the corresponding MSR, by clearing the corresponding read/write bits in the
>   MSRPM based on the flags

In how far is VMX'es present model better than SVM's? They both have pros and
cons, depending on the specific use. I'm not asking to do it the other way
around (at least not yet), I'd merely like to understand why we're going to
gain two new hooks (if I'm not mistaken) when we could also get away with
just one.

> --- a/xen/arch/x86/cpu/vpmu_amd.c
> +++ b/xen/arch/x86/cpu/vpmu_amd.c
> @@ -165,8 +165,9 @@ static void amd_vpmu_set_msr_bitmap(struct vcpu *v)
>  
>      for ( i = 0; i < num_counters; i++ )
>      {
> -        svm_intercept_msr(v, counters[i], MSR_INTERCEPT_NONE);
> -        svm_intercept_msr(v, ctrls[i], MSR_INTERCEPT_WRITE);
> +        svm_clear_msr_intercept(v, counters[i], MSR_RW);
> +        svm_set_msr_intercept(v, ctrls[i], MSR_W);
> +        svm_clear_msr_intercept(v, ctrls[i], MSR_R);
>      }
>  
>      msr_bitmap_on(vpmu);
> @@ -179,8 +180,8 @@ static void amd_vpmu_unset_msr_bitmap(struct vcpu *v)
>  
>      for ( i = 0; i < num_counters; i++ )
>      {
> -        svm_intercept_msr(v, counters[i], MSR_INTERCEPT_RW);
> -        svm_intercept_msr(v, ctrls[i], MSR_INTERCEPT_RW);
> +        svm_set_msr_intercept(v, counters[i], MSR_RW);
> +        svm_set_msr_intercept(v, ctrls[i], MSR_RW);
>      }

This, aiui, restores back original state (I question the condition that the
caller uses, though, but that's a separate issue). Therefore is the single
"set" in the earlier function actually needed?

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -288,23 +288,34 @@ svm_msrbit(unsigned long *msr_bitmap, uint32_t msr)
>      return msr_bit;
>  }
>  
> -void svm_intercept_msr(struct vcpu *v, uint32_t msr, int flags)
> +void svm_set_msr_intercept(struct vcpu *v, uint32_t msr, int flags)

Can the last parameter become "unsigned int", please?

>  {
> -    unsigned long *msr_bit;
> -    const struct domain *d = v->domain;
> +    unsigned long *msr_bit = svm_msrbit(v->arch.hvm.svm.msrpm, msr);
> +
> +    if ( msr_bit == NULL )
> +        return;
>  
> -    msr_bit = svm_msrbit(v->arch.hvm.svm.msrpm, msr);
> -    BUG_ON(msr_bit == NULL);

The conversion from BUG_ON() to "return" needs explanation; I don't see
why that's warranted here. From all I can tell the case is impossible
due to the way construct_vmcb() works, and hence BUG_ON() is appropriate
(and personally I would also be fine with no check at all, provided I'm
not overlooking anything).

> @@ -312,8 +323,10 @@ static void cf_check svm_enable_msr_interception(struct domain *d, uint32_t msr)
>  {
>      struct vcpu *v;
>  
> -    for_each_vcpu ( d, v )
> -        svm_intercept_msr(v, msr, MSR_INTERCEPT_WRITE);
> +    for_each_vcpu ( d, v ) {

Nit: Brace placement.

> @@ -595,22 +608,31 @@ static void cf_check svm_cpuid_policy_changed(struct vcpu *v)
>      vmcb_set_exception_intercepts(vmcb, bitmap);
>  
>      /* Give access to MSR_SPEC_CTRL if the guest has been told about it. */
> -    svm_intercept_msr(v, MSR_SPEC_CTRL,
> -                      cp->extd.ibrs ? MSR_INTERCEPT_NONE : MSR_INTERCEPT_RW);
> +    if ( cp->extd.ibrs )
> +        svm_clear_msr_intercept(v, MSR_SPEC_CTRL, MSR_RW);
> +    else
> +        svm_set_msr_intercept(v, MSR_SPEC_CTRL, MSR_RW);
>  
>      /*
>       * Always trap write accesses to VIRT_SPEC_CTRL in order to cache the guest
>       * setting and avoid having to perform a rdmsr on vmexit to get the guest
>       * setting even if VIRT_SSBD is offered to Xen itself.
>       */
> -    svm_intercept_msr(v, MSR_VIRT_SPEC_CTRL,
> -                      cp->extd.virt_ssbd && cpu_has_virt_ssbd &&
> -                      !cpu_has_amd_ssbd ?
> -                      MSR_INTERCEPT_WRITE : MSR_INTERCEPT_RW);
> +    if ( cp->extd.virt_ssbd && cpu_has_virt_ssbd && !cpu_has_amd_ssbd )
> +    {
> +        svm_set_msr_intercept(v, MSR_VIRT_SPEC_CTRL, MSR_W);
> +        svm_clear_msr_intercept(v, MSR_VIRT_SPEC_CTRL, MSR_R);
> +    }
> +    else
> +    {
> +        svm_set_msr_intercept(v, MSR_VIRT_SPEC_CTRL, MSR_RW);
> +    }

Preferably omit the braces for "else" here, just like you do above and ...

>      /* Give access to MSR_PRED_CMD if the guest has been told about it. */
> -    svm_intercept_msr(v, MSR_PRED_CMD,
> -                      cp->extd.ibpb ? MSR_INTERCEPT_NONE : MSR_INTERCEPT_RW);
> +    if ( cp->extd.ibpb )
> +        svm_clear_msr_intercept(v, MSR_VIRT_SPEC_CTRL, MSR_RW);
> +    else
> +        svm_set_msr_intercept(v, MSR_VIRT_SPEC_CTRL, MSR_RW);

... here.

> --- a/xen/arch/x86/include/asm/hvm/svm/vmcb.h
> +++ b/xen/arch/x86/include/asm/hvm/svm/vmcb.h
> @@ -585,13 +585,12 @@ void svm_destroy_vmcb(struct vcpu *v);
>  
>  void setup_vmcb_dump(void);
>  
> -#define MSR_INTERCEPT_NONE    0
> -#define MSR_INTERCEPT_READ    1
> -#define MSR_INTERCEPT_WRITE   2
> -#define MSR_INTERCEPT_RW      (MSR_INTERCEPT_WRITE | MSR_INTERCEPT_READ)
> -void svm_intercept_msr(struct vcpu *v, uint32_t msr, int enable);
> -#define svm_disable_intercept_for_msr(v, msr) svm_intercept_msr((v), (msr), MSR_INTERCEPT_NONE)
> -#define svm_enable_intercept_for_msr(v, msr) svm_intercept_msr((v), (msr), MSR_INTERCEPT_RW)
> +void svm_set_msr_intercept(struct vcpu *v, uint32_t msr, int flags);
> +void svm_clear_msr_intercept(struct vcpu *v, uint32_t msr, int flags);
> +#define svm_disable_intercept_for_msr(v, msr) \
> +    svm_clear_msr_intercept((v), (msr), MSR_RW)
> +#define svm_enable_intercept_for_msr(v, msr) \
> +    svm_set_intercept_msr((v), (msr), MSR_RW)

Please avoid excess parentheses. Also could you clarify why you retain
these shorthands when you don't use them in the conversion that you're
doing (e.g. amd_vpmu_unset_msr_bitmap())? Are you intending them to go
away down the road?

Jan