Re: [PATCH 1/3] vm_event: svm: Don't BUG() when enabling intercept on hypervisor MSR

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.09.2026 13:56, Teddy Astie wrote:
> svm_msrbits() only cover MSRs in the ranges specified in APM and returns
> NULL for all the other ones (i.e hypervisor range), which triggers the
> BUG_ON() that follows.
> 
> APM specifies that MSR outside the architectural and AMD specific ranges
> are unconditionally intercepted, hence don't require any specific handling
> by SVM logic, fix this by ignoring cases when msr_bit is NULL.
> 
> This is only reachable with CONFIG_VM_EVENT using 
> XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR
> to configure a MSR intercept in the hypervisor range (0x40000000–0x40001fff).

Hence imo it is wrong to drop the sanity check altogether. See below.

Also, nit: Description lines want limiting to 75 chars.

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -237,7 +237,9 @@ void svm_intercept_msr(struct vcpu *v, uint32_t msr, int 
> flags)
>      const struct domain *d = v->domain;
>  
>      msr_bit = svm_msrbit(v->arch.hvm.svm.msrpm, msr);
> -    BUG_ON(msr_bit == NULL);
> +    if ( msr_bit == NULL )
> +        return;

Imo the function wants to return an error here, which all callers
except the one you mention check. I.e. the BUG_ON() would be moved
out to most callers (alternatively the one caller could range-check
the MSR itself). Right now for introspection intercepts can only be
enabled, hence why for that use "all other MSRs are intercepted
anyway" is fine. Suppose the inverse operation would be possible as
well: Then indicating "success" back to the caller when accesses to
the requested MSR are still intercepted would be wrong.

Also, nit: We generally prefer !msr_bit over msr_bit == NULL.

Jan