Re: [PATCH v6 06/20] KVM: VMX: Set FRED MSR intercepts

[Xin Li <xin@zytor.com>]

>> Hi Sean,
>>
>> I'd like to bring up an issue concerning MSR_IA32_PL0_SSP.
>>
>> The FRED spec claims:
>>
>> The FRED SSP MSRs are supported by any processor that enumerates
>> CPUID.(EAX=7,ECX=1):EAX.FRED[bit 17] as 1. If such a processor does not
>> support CET, FRED transitions will not use the MSRs (because shadow stacks
>> are not enabled), but the MSRs would still be accessible using MSR-access
>> instructions (e.g., RDMSR, WRMSR).
>>
>> It means KVM needs to handle MSR_IA32_PL0_SSP even when FRED is supported
>> but CET is not.  And this can be broken down into two subtasks:
>>
>> 1) Allow such a guest to access MSR_IA32_PL0_SSP w/o triggering #GP.  And
>> this behavior is already implemented in patch 8 of this series.
>>
>> 2) Save and restore MSR_IA32_PL0_SSP in both KVM and Qemu for such a guest.
> 
> What novel work needs to be done in KVM?  For QEMU, I assume it's just adding an
> "or FRED" somewhere.  For KVM, I'm missing what additional work would be required
> that wouldn't be naturally covered by patch 8 (assuming patch 8 is bug-free).

Extra patches:

1) A patch to save/restore guest MSR_IA32_PL0_SSP (i.e., FRED SSP0), as
what we have done for RSP0, following is the patch on top of the patch 
saving/restoring RSP0:

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 449a5e02c7de..0bf684342a71 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1294,8 +1294,13 @@ void vmx_prepare_switch_to_guest(struct kvm_vcpu *vcpu)

  	wrmsrq(MSR_KERNEL_GS_BASE, vmx->msr_guest_kernel_gs_base);

-	if (guest_cpu_cap_has(vcpu, X86_FEATURE_FRED))
+	if (guest_cpu_cap_has(vcpu, X86_FEATURE_FRED)) {
  		wrmsrns(MSR_IA32_FRED_RSP0, vmx->msr_guest_fred_rsp0);
+
+		/* XSAVES/XRSTORS do not cover SSP MSRs */
+		if (!kvm_cpu_cap_has(X86_FEATURE_SHSTK))
+			wrmsrns(MSR_IA32_FRED_SSP0, vmx->msr_guest_fred_ssp0);
+	}
  #else
  	savesegment(fs, fs_sel);
  	savesegment(gs, gs_sel);
@@ -1349,6 +1354,10 @@ static void vmx_prepare_switch_to_host(struct 
vcpu_vmx *vmx)
  		 * CPU exits to userspace (RSP0 is a per-task value).
  		 */
  		fred_sync_rsp0(vmx->msr_guest_fred_rsp0);
+
+		/* XSAVES/XRSTORS do not cover SSP MSRs */
+		if (!kvm_cpu_cap_has(X86_FEATURE_SHSTK))
+			vmx->msr_guest_fred_ssp0 = read_msr(MSR_IA32_FRED_SSP0);
  	}
  #endif
  	load_fixmap_gdt(raw_smp_processor_id());
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 733fa2ef4bea..12c1cf827cb7 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -228,6 +228,7 @@ struct vcpu_vmx {
  #ifdef CONFIG_X86_64
  	u64		      msr_guest_kernel_gs_base;
  	u64		      msr_guest_fred_rsp0;
+	u64		      msr_guest_fred_ssp0;
  #endif

  	u64		      spec_ctrl;

And We might want to zero host MSR_IA32_PL0_SSP when switching to host.


2) Add vmx_read_guest_fred_ssp0()/vmx_write_guest_fred_ssp0(), and use
them to read/write MSR_IA32_PL0_SSP in patch 8:

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 99106750b1e3..cbdc67682d27 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1400,9 +1408,23 @@ static void vmx_write_guest_fred_rsp0(struct 
vcpu_vmx *vmx, u64 data)
  	vmx_write_guest_host_msr(vmx, MSR_IA32_FRED_RSP0, data,
  				 &vmx->msr_guest_fred_rsp0);
  }
+
+static u64 vmx_read_guest_fred_ssp0(struct vcpu_vmx *vmx)
+{
+	return vmx_read_guest_host_msr(vmx, MSR_IA32_FRED_SSP0,
+				       &vmx->msr_guest_fred_ssp0);
+}
+
+static void vmx_write_guest_fred_ssp0(struct vcpu_vmx *vmx, u64 data)
+{
+	vmx_write_guest_host_msr(vmx, MSR_IA32_FRED_SSP0, data,
+				 &vmx->msr_guest_fred_ssp0);
+}
  #endif

  static void grow_ple_window(struct kvm_vcpu *vcpu)
@@ -2189,6 +2211,18 @@ int vmx_get_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
  	case MSR_IA32_DEBUGCTLMSR:
  		msr_info->data = vmx_guest_debugctl_read();
  		break;
+	case MSR_IA32_PL0_SSP:
+		/*
+		 * If kvm_cpu_cap_has(X86_FEATURE_SHSTK) but
+		 * !guest_cpu_cap_has(vcpu, X86_FEATURE_SHSTK), XSAVES/XRSTORS
+		 * cover SSP MSRs.
+		 */
+		if (!kvm_cpu_cap_has(X86_FEATURE_SHSTK) &&
+		    guest_cpu_cap_has(vcpu, X86_FEATURE_FRED)) {
+			msr_info->data = vmx_read_guest_fred_ssp0(vmx);
+			break;
+		}
+		fallthrough;
  	default:
  	find_uret_msr:
  		msr = vmx_find_uret_msr(vmx, msr_info->index);
@@ -2540,7 +2574,18 @@ int vmx_set_msr(struct kvm_vcpu *vcpu, struct 
msr_data *msr_info)
  		}
  		ret = kvm_set_msr_common(vcpu, msr_info);
  		break;
-
+	case MSR_IA32_PL0_SSP:
+		/*
+		 * If kvm_cpu_cap_has(X86_FEATURE_SHSTK) but
+		 * !guest_cpu_cap_has(vcpu, X86_FEATURE_SHSTK), XSAVES/XRSTORS
+		 * cover SSP MSRs.
+		 */
+		if (!kvm_cpu_cap_has(X86_FEATURE_SHSTK) &&
+		    guest_cpu_cap_has(vcpu, X86_FEATURE_FRED)) {
+			vmx_write_guest_fred_ssp0(vmx, data);
+			break;
+		}
+		fallthrough;
  	default:
  	find_uret_msr:
  		msr = vmx_find_uret_msr(vmx, msr_index);


3) Another change I was discussing with Chao:
https://lore.kernel.org/lkml/2ed04dff-e778-46c6-bd5f-51295763af06@zytor.com/

> 
>> I have the patches for 2) but they are not included in this series, because
>>
>> 1) how much do we care the value in MSR_IA32_PL0_SSP in such a guest?
>>
>> Yes, Chao told me that you are the one saying that MSRs can be used as
>> clobber registers and KVM should preserve the value.  Does MSR_IA32_PL0_SSP
>> in such a guest count?
> 
> If the architecture says that MSR_IA32_PL0_SSP exists and is accessible, then
> KVM needs to honor that.
> 
>> 2) Saving/restoring MSR_IA32_PL0_SSP adds complexity, though it's seldom
>> used.  Is it worth it?
> 
> Honoring the architecture is generally not optional.  There are extreme cases
> where KVM violates that rule and takes (often undocumented) erratum, e.g. APIC
> base relocation would require an absurd amount of complexity for no real world
> benefit.  But I would be very surprised if the complexity in KVM or QEMU to support
> this scenario is at all meaningful, let alone enough to justify diverging from
> the architectural spec.

Let me post v7 which includes all the required changes.

Thanks!
     Xin