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 <[email protected]> >> --- >> 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
