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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.09.2026 15:39, Teddy Astie wrote:
> Le 30/09/2026 à 15:08, Jan Beulich a écrit :
>> On 30.09.2026 14:57, Teddy Astie wrote:
>>> Le 30/09/2026 à 11:16, Jan Beulich a écrit :
>>>> 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.
>>>>
>>>
>>> IIUC, you're suggesting to report to the caller that the hardware
>>> doesn't cover it, such as we can detect cases where we accidentally pass
>>> a bogus MSR index that is outside of hardware ranges.
>>>
>>> In the VM introspection case, we could ignore it since we're only
>>> interested on intercepts, where outside hardware ranges would always be
>>> intercepted.
>>>
>>> Did I got the idea correctly ?
>>
>> Yes. Just that the more I think about it, the more I prefer the alternative
>> that I also mentioned. Not the least because it's less churn.
>>
> 
> One of the idea would be to check monitored_msr() (somewhat like what 
> you proposed for VMX) so that the VM event case is covered, but we 
> ASSERT (or BUG_ON ?) in the other cases if we pass a unexpected MSR to 
> this function.
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 71e48351f2..be866b9055 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -236,7 +236,12 @@ 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 )
> +    {
> +        ASSERT(monitored_msr(d, msr));
> +        return;
> +    }

Just that ASSERT() only covers debug builds and BUG_ON() is too heavy on
release ones: There's no reason to kill an entire host because of this.

Jan