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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.09.2026 16:30, Teddy Astie wrote:
> Le 30/09/2026 à 16:04, Jan Beulich a écrit :
>> 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.
> 
> So we should do WARN_ON() then ?

Not sure, as would still hide an anomaly from the caller (which ultimately
is responsible; the log entry is only indicating the problem to the admin,
and only if they care to actually look). You didn't Cc Tamas on this
series, yet his input would be valuable here.

Jan