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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan