On 01.12.2025 17:36, Andrew Cooper wrote: > On 01/12/2025 9:02 am, Jan Beulich wrote: >> On 28.11.2025 18:47, Andrew Cooper wrote: >>> While we do this for unknown user mode exits, crashing for supervisor mode >>> exits is unhelpful. Intel in particular expect the unknown case to be #UD >>> because they do introduce new instructions with new VMEXIT_* codes without >>> other enablement controls. e.g. MSRLIST, USER_MSR, MSR_IMM, but AMD have >>> RDPRU and SKINIT as examples too. >> USER-MSR has MSR_USER_MSR_CTL, so doesn't fully fit here? (It's still not us >> to directly control exposure of the insns, but an OS would need to use the >> MSR to do so, and hence we need to properly handle writes to that MSR for >> the respective exits to become possible.) > > Oh yes, and the #GP from failing the MSR_USER_MSR_CTL check take > priority over the intercept. > >> MSRLIST has a dedicated exec control, so whether the exits can occur is >> under our control. > > Ah ok. > > >> RDPRU and SKINIT have dedicated intercepts, without use of which the >> respective exit can't occur aiui. > > Correct, but note how we intercept them unconditionally? > > MONITOR, MWAIT and SKINIT are for Xen's safety, because otherwise the > instructions would execute normally in guest context. > > RDPRU is to block access to the perf counters, because a guest has no > legitimate use for them. > > There are no enablement controls for these instructions. They're always > guest-available (on capable hardware) if not intercepted.
For our purposes, the intercept is the enable (i.e. we disable their use by injecting #UD if the intercept triggers). IOW I think those are slightly different in any event, in not really being "unknown". I don't mind their mentioning, but I think the distinction wants to at least be expressed somehow. >>> @@ -3083,8 +3067,13 @@ void asmlinkage svm_vmexit_handler(void) >>> gprintk(XENLOG_ERR, "Unexpected vmexit: reason %#"PRIx64", " >>> "exitinfo1 %#"PRIx64", exitinfo2 %#"PRIx64"\n", >>> exit_reason, vmcb->exitinfo1, vmcb->exitinfo2); >>> - crash_or_fault: >>> - svm_crash_or_fault(v); >>> + fallthrough; >>> + case VMEXIT_MONITOR: >>> + case VMEXIT_MWAIT: >>> + case VMEXIT_SKINIT: >>> + case VMEXIT_RDPRU: >>> + inject_ud: >>> + hvm_inject_hw_exception(X86_EXC_UD, X86_EVENT_NO_EC); >>> break; >>> } >>> >> Should this be brought more in line with respective VMX code (kept) below, >> in never bypassing the gprintk() by any of the case labels? Basically >> meaning that the case labels you move could simply be dropped for the time >> being (or else, like the INVCPID one visible in context below, would want >> re-inserting a few lines earlier). > > As said, they're all reachable by guests on capable hardware. > > I could add a /* Not implemented for guests */ if that would make it > clearer? Yes, ideally with "yet" also added - recall I've been sitting on an RDPRU emulator patch, awaiting you to fulfill your promise of sorting the CPUID side of things there. > But, we don't want the printk(). We know the labels are reachable, and > #UD is the right action. Hmm, yes, with what you have said further up I think I agree. Yet then my question goes the other way around: Do we want the log message for the (at least) two known exits in VMX, which are grouped with the default: label? IOW I'm still puzzled by the asymmetry between SVM and VMX in this regard. Jan
