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

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

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 ?

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

> Jan
Teddy

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature