Re: [PATCH] x86/hvm: Unilaterally inject #UD for unknown VMExits

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

>
> IOW it looks to be really only MSR-IMM which is special.
>
>> @@ -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?

But, we don't want the printk().  We know the labels are reachable, and
#UD is the right action.

~Andrew