| Message ID | 20230606091842.13123-3-binbin.wu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Linear Address Masking (LAM) KVM Enabling | expand |
On Tue, 2023-06-06 at 17:18 +0800, Binbin Wu wrote: > Move CR4.LAM_SUP out of CR4_RESERVED_BITS and its reservation depends on vcpu > supporting LAM feature or not. Leave the bit intercepted to avoid vmread every > time when KVM fetches its value, with the expectation that guest won't toggle > the bit frequently. KVM only needs to do vmread once to cache guest's CR4, and presumable vmread is a lot cheaper than a VMEXIT. So I don't see the value of intercepting it if there's no need to do. But presumably I think we cannot allow guest to own this bit because KVM wants to return a valid CR4 if LAM isn't exposed to guest? Otherwise guest can still set this bit even LAM isn't exposed to guest. Am I missing something? If not, your justification of intercepting this bit isn't correct and needs update.
On 6/7/2023 11:40 AM, Huang, Kai wrote: > On Tue, 2023-06-06 at 17:18 +0800, Binbin Wu wrote: >> Move CR4.LAM_SUP out of CR4_RESERVED_BITS and its reservation depends on vcpu >> supporting LAM feature or not. Leave the bit intercepted to avoid vmread every >> time when KVM fetches its value, with the expectation that guest won't toggle >> the bit frequently. > KVM only needs to do vmread once to cache guest's CR4, and presumable vmread is > a lot cheaper than a VMEXIT. So I don't see the value of intercepting it if > there's no need to do. Here is the discussion about the general rule of interception of CR4 bit. Sean mentioned: "As a base rule, KVM intercepts CR4 bits unless there's a reason not to, e.g. if the CR4 bit in question is written frequently by real guests and/or never consumed by KVM." https://lore.kernel.org/all/Y7xA53sLxCwzfvgD@google.com/ And CR4.LAM_SUP value will be used to determin the LAM mode when apply LAM masking in instruction emulations / VMExit handlers, and if the bit is passed-through, it will be a vmread in these pathes. > > But presumably I think we cannot allow guest to own this bit because KVM wants > to return a valid CR4 if LAM isn't exposed to guest? Otherwise guest can still > set this bit even LAM isn't exposed to guest. > > Am I missing something? Right, this is also a reason why the CR4.LAM_SUP bit should be intercepted. Will update the justification. I suppose this reason is enough for justification, will remove the performance part in changelog. Thanks. > > If not, your justification of intercepting this bit isn't correct and needs > update. >
On Wed, 2023-06-07 at 12:55 +0800, Binbin Wu wrote: > > On 6/7/2023 11:40 AM, Huang, Kai wrote: > > On Tue, 2023-06-06 at 17:18 +0800, Binbin Wu wrote: > > > Move CR4.LAM_SUP out of CR4_RESERVED_BITS and its reservation depends on vcpu > > > supporting LAM feature or not. Leave the bit intercepted to avoid vmread every > > > time when KVM fetches its value, with the expectation that guest won't toggle > > > the bit frequently. > > KVM only needs to do vmread once to cache guest's CR4, and presumable vmread is > > a lot cheaper than a VMEXIT. So I don't see the value of intercepting it if > > there's no need to do. > Here is the discussion about the general rule of interception of CR4 bit. > Sean mentioned: "As a base > rule, KVM intercepts CR4 bits unless there's a reason not to, e.g. if > the CR4 bit > in question is written frequently by real guests and/or never consumed > by KVM." > https://lore.kernel.org/all/Y7xA53sLxCwzfvgD@google.com/ > > And CR4.LAM_SUP value will be used to determin the LAM mode when apply > LAM masking in instruction emulations / VMExit handlers, > and if the bit is passed-through, it will be a vmread in these pathes. Yeah agreed. > > > > > But presumably I think we cannot allow guest to own this bit because KVM wants > > to return a valid CR4 if LAM isn't exposed to guest? Otherwise guest can still > > set this bit even LAM isn't exposed to guest. > > > > Am I missing something? > Right, this is also a reason why the CR4.LAM_SUP bit should be intercepted. > Will update the justification. > I suppose this reason is enough for justification, will remove the > performance part in changelog. Anyway, Reviewed-by: Kai Huang <kai.huang@intel.com>
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index fb9d1f2d6136..c6f03d151c31 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -125,7 +125,8 @@ | X86_CR4_PGE | X86_CR4_PCE | X86_CR4_OSFXSR | X86_CR4_PCIDE \ | X86_CR4_OSXSAVE | X86_CR4_SMEP | X86_CR4_FSGSBASE \ | X86_CR4_OSXMMEXCPT | X86_CR4_LA57 | X86_CR4_VMXE \ - | X86_CR4_SMAP | X86_CR4_PKE | X86_CR4_UMIP)) + | X86_CR4_SMAP | X86_CR4_PKE | X86_CR4_UMIP \ + | X86_CR4_LAM_SUP)) #define CR8_RESERVED_BITS (~(unsigned long)X86_CR8_TPR) diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 2d9d155691a7..0dd2970ba5c8 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -7600,6 +7600,9 @@ static void nested_vmx_cr_fixed1_bits_update(struct kvm_vcpu *vcpu) cr4_fixed1_update(X86_CR4_UMIP, ecx, feature_bit(UMIP)); cr4_fixed1_update(X86_CR4_LA57, ecx, feature_bit(LA57)); + entry = kvm_find_cpuid_entry_index(vcpu, 0x7, 1); + cr4_fixed1_update(X86_CR4_LAM_SUP, eax, feature_bit(LAM)); + #undef cr4_fixed1_update } diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h index 82e3dafc5453..24e2b56356b8 100644 --- a/arch/x86/kvm/x86.h +++ b/arch/x86/kvm/x86.h @@ -528,6 +528,8 @@ bool kvm_msr_allowed(struct kvm_vcpu *vcpu, u32 index, u32 type); __reserved_bits |= X86_CR4_VMXE; \ if (!__cpu_has(__c, X86_FEATURE_PCID)) \ __reserved_bits |= X86_CR4_PCIDE; \ + if (!__cpu_has(__c, X86_FEATURE_LAM)) \ + __reserved_bits |= X86_CR4_LAM_SUP; \ __reserved_bits; \ })