On Fri, Feb 14, 2025, Kim Phillips wrote: > On 2/13/25 6:55 PM, Sean Christopherson wrote: > > On Thu, Feb 13, 2025, Kim Phillips wrote: > > > > > Not sure if the cpu_feature_enabled() check is necessary, as init should > > > > > have failed if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES wasn't set in > > > > > sev_supported_vmsa_features. > > > > > > > > Two things missing from this series: > > > > > > > > 1: KVM enforcement. No way is KVM going to rely on userspace to opt-in to > > > > preventing the guest from enabling features. > > > > 2: Backwards compatilibity if KVM unconditionally enforces ALLOWED_SEV_FEATURES. > > > > Although maybe there's nothing to do here? I vaguely recall all of the gated > > > > features being unsupported, or something... > > > > > > This contradicts your review comment from the previous version of the series [1]. > > > > First off, my comment was anything but decisive. I don't see how anyone can read > > this and come away thinking "this is exactly what Sean wants". > > > > This may need additional uAPI so that userspace can opt-in. Dunno. I hope guests > > aren't abusing features, but IIUC, flipping this on has the potential to break > > existing VMs, correct? > > > > Second, there's a clear question there that went unanswered. Respond to questions > > and elaborate as needed until we're all on the same page. Don't just send patches. > > > > Third, letting userspace opt-in to something doesn't necessarily mean giving > > userspace full control. Which is the entire reason I asked the question about > > whether or not this can break userspace. E.g. we can likely get away with only > > making select features opt-in, and enforcing everything else by default. > > > > I don't think RESTRICTED_INJECTION or ALTERNATE_INJECTION can work without KVM > > cooperation, so enforcing those shouldn't break anything. > > > > It's still not clear to me that we don't have a bug with DEBUG_SWAP. AIUI, > > DEBUG_SWAP is allowed by default. I.e. if ALLOWED_FEATURES is unsupported, then > > the guest can use DEBUG_SWAP via SVM_VMGEXIT_AP_CREATE without KVM's knowledge. > > > > So _maybe_ we have to let userspace opt-in to enforcing DEBUG_SWAP, but I suspect > > we can get away with fully enabling ALLOWED_FEATURES without userspace's blessing. > > If I hardcode DEBUG_SWAP (bit 5) in the vmsa->sev_features assignment > in wakeup_cpu_via_vmgexit(), such guest boots successfully with the > kvm_amd module's debug_swap parameter set. > > The guest *doesn't* boot if I also turn on allowed_sev_features=1 with > qemu and this patchseries. > > So, the answer is yes, always enforcing ALLOWED_SEV_FEATURES does break > existing guests, thus the userspace opt-in for it. That is not an "existing" guest. That's a deliberately misconfigured guest that serves as testcase/reproducer. IIUC, the BSP can't enable DEBUG_SWAP through a backdoor, so I don't think it's at all sane/reasonable for the guest to expect that enabling DEBUG_SWAP only on APs will function. Ah, and KVM will still set the DR7 intercepts, i.e. the guest can't read/write DR7, so this is definitely a nonsensical/unsupported configuration. So unless I've missed something, KVM can unconditionally enforce ALLOWED_SEV_FEATURES.
