Re: [PATCH v9 07/10] VMX: support MSR-IMM

[Jan Beulich <jbeulich@suse.com>]

On 26.11.2025 19:50, Andrew Cooper wrote:
> On 24/11/2025 3:00 pm, Jan Beulich wrote:
>> Hook up the new VM exit codes and handle guest uses of the insns.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v9: New.
>> ---
>> The lack of an enable bit is concerning; at least for the nested case
>> that's a security issue afaict (when L0 isn't aware of the insns, or more
>> specifically the exit codes).
> 
> This is why we need support statements of new CPUs.

I hope you don't mean the lack thereof to be a blocking factor for this
change?

> Intel say that unknown VMExits turning into #UD is the expected course
> of action.  That covers all of these cases which don't have an explicit
> enable.

Do you have a pointer?

> This is better than our current behaviour, which is non-architectural
> for supervisor code and practically the most unhelpful course of action
> going.
> 
> Obviously, logic turning on a new feature is expected to handle all the
> VMExit cases it can produce.

What you say here ...

> The corollary for nested virt is that L0 must never make a Virtual
> VMExit with case that isn't enabled.  Combined with #UD in the unknown
> case, that covers things reasonably well.
> 
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -453,7 +453,7 @@ void domain_cpu_policy_changed(struct do
>>      }
>>  
>>      /* Nested doesn't have the necessary processing, yet. */
>> -    if ( nestedhvm_enabled(d) && p->feat.user_msr )
>> +    if ( nestedhvm_enabled(d) && (p->feat.user_msr || p->feat.msr_imm) )
>>          return /* -EINVAL */;
> 
> What processing is missing?  (Aside from correcting the unknown case.)

... is what would need doing for this check to disappear. Going the #UD
route looks to make sense, but it still wouldn't feel quite right then
to drop this check. If we expose the feature, we shouldn't convert
respective exits to #UD. They aren't "unknown" (anymore) after all.

And then again the question is: Are you expecting me to deal with
switching to the #UD model as a prereq (if, as per above, that's
relevant at all here)? If so, I have to admit that it's not quite clear
to me what exactly this would be meant to look like: Alter the default
cases of the big switch()es in both the normal and nested exit handlers?
Or merely the latter?

>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -4762,6 +4762,7 @@ void asmlinkage vmx_vmexit_handler(struc
>>          break;
>>  
>>      case EXIT_REASON_URDMSR:
>> +    case EXIT_REASON_RDMSR_IMM:
> 
> Instructions which aren't enumerated in CPUID have reserved behaviour.
> 
> The exit handler needs to check cp->feat.msr_imm and inject #UD.
> 
> It's not perfect; un-intercepted MSRs will happen to execute correctly
> on capable hardware, but most MSRs are not intercepted and it's far
> closer to adequate behaviour than omitting the #UD check.

Hmm, okay, I can certainly do it this way.

Jan