| Message ID | 20230209024022.3371768-1-robert.hu@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Linear Address Masking (LAM) KVM Enabling | expand |
On Thu, Feb 09, 2023 at 10:40:13AM +0800, Robert Hoo wrote: >Intercept CR4.LAM_SUP by KVM, to avoid read VMCS field every time, with >expectation that guest won't toggle this bit frequently. > >Under EPT mode, CR3 is fully under guest control, guest LAM is thus transparent to >KVM. Nothing more need to do. I don't think it is correct. You have to strip LAM_U57/U48 from CR3 when walking guest page table and strip metadata from pointers when emulating instructions. > >For Shadow paging (EPT = off), KVM need to handle guest CR3.LAM_U48 and CR3.LAM_U57 >toggles. > >[1] ISE Chap10 https://cdrdv2.intel.com/v1/dl/getContent/671368 (Section 10.6 VMX interaction) >[2] Thus currently, Kernel enabling patch only enables LAM_U57. https://lore.kernel.org/lkml/20230123220500.21077-1-kirill.shutemov@linux.intel.com/ Please add a kvm-unit-test or kselftest for LAM, particularly for operations (e.g., canonical check for supervisor pointers, toggle CR4.LAM_SUP) which aren't covered by the test in Kirill's series.
On Thu, 2023-02-09 at 14:15 +0800, Chao Gao wrote: > On Thu, Feb 09, 2023 at 10:40:13AM +0800, Robert Hoo wrote: > > Intercept CR4.LAM_SUP by KVM, to avoid read VMCS field every time, > > with > > expectation that guest won't toggle this bit frequently. > > > > Under EPT mode, CR3 is fully under guest control, guest LAM is thus > > transparent to > > KVM. Nothing more need to do. > > I don't think it is correct. You have to strip LAM_U57/U48 from CR3 > when > walking guest page table and strip metadata from pointers when > emulating > instructions. > Yes, has added patch 8 for emulation case. Didn't explicitly note it in cover letter. > > > > For Shadow paging (EPT = off), KVM need to handle guest CR3.LAM_U48 > > and CR3.LAM_U57 > > toggles. > > > > [1] ISE Chap10 https://cdrdv2.intel.com/v1/dl/getContent/671368 > > (Section 10.6 VMX interaction) > > [2] Thus currently, Kernel enabling patch only enables LAM_U57. > > https://lore.kernel.org/lkml/20230123220500.21077-1-kirill.shutemov@linux.intel.com/ > > > > Please add a kvm-unit-test or kselftest for LAM, particularly for > operations (e.g., canonical check for supervisor pointers, toggle > CR4.LAM_SUP) which aren't covered by the test in Kirill's series. OK, I can explore for kvm-unit-test in separate patch set. BTW, this patch set has passed guest running Kirill's kselftests.
On Thu, Feb 09, 2023, Robert Hoo wrote: > On Thu, 2023-02-09 at 14:15 +0800, Chao Gao wrote: > > On Thu, Feb 09, 2023 at 10:40:13AM +0800, Robert Hoo wrote: > > Please add a kvm-unit-test or kselftest for LAM, particularly for > > operations (e.g., canonical check for supervisor pointers, toggle > > CR4.LAM_SUP) which aren't covered by the test in Kirill's series. > > OK, I can explore for kvm-unit-test in separate patch set. Please make tests your top priority. Without tests, I am not going to spend any time reviewing this series, or any other hardware enabling series[*]. I don't expect KVM specific tests for everything, i.e. it's ok to to rely things like running VMs that utilize LAM and/or running LAM selftests in the guest, but I do want a reasonably thorough explanation of how all the test pieces fit together to validate KVM's implementation. [*] https://lore.kernel.org/all/Y+Uq0JOEmmdI0YwA@google.com
On Thu, 2023-02-09 at 17:27 +0000, Sean Christopherson wrote: > On Thu, Feb 09, 2023, Robert Hoo wrote: > > On Thu, 2023-02-09 at 14:15 +0800, Chao Gao wrote: > > > On Thu, Feb 09, 2023 at 10:40:13AM +0800, Robert Hoo wrote: > > > Please add a kvm-unit-test or kselftest for LAM, particularly for > > > operations (e.g., canonical check for supervisor pointers, toggle > > > CR4.LAM_SUP) which aren't covered by the test in Kirill's series. > > > > OK, I can explore for kvm-unit-test in separate patch set. > > Please make tests your top priority. Without tests, I am not going > to spend any > time reviewing this series, or any other hardware enabling > series[*]. I don't > expect KVM specific tests for everything, i.e. it's ok to to rely > things like > running VMs that utilize LAM and/or running LAM selftests in the > guest, but I do > want a reasonably thorough explanation of how all the test pieces fit > together to > validate KVM's implementation. Sure, and ack on unit test is part of development work. This patch set had always been unit tested before sent out, i.e. "running LAM selftests in guest" on both ept=Y/N. CR4.LAM_SUP, as Chao pointed out, could not be covered by kselftest, I may explore it in kvm-unit-test. Or, would you mind that separate CR4.LAM_SUP enabling in another patch set? > > [*] https://lore.kernel.org/all/Y+Uq0JOEmmdI0YwA@google.com
On Fri, Feb 10, 2023 at 10:07:42AM +0800, Robert Hoo wrote: >On Thu, 2023-02-09 at 17:27 +0000, Sean Christopherson wrote: >> On Thu, Feb 09, 2023, Robert Hoo wrote: >> > On Thu, 2023-02-09 at 14:15 +0800, Chao Gao wrote: >> > > On Thu, Feb 09, 2023 at 10:40:13AM +0800, Robert Hoo wrote: >> > > Please add a kvm-unit-test or kselftest for LAM, particularly for >> > > operations (e.g., canonical check for supervisor pointers, toggle >> > > CR4.LAM_SUP) which aren't covered by the test in Kirill's series. >> > >> > OK, I can explore for kvm-unit-test in separate patch set. >> >> Please make tests your top priority. Without tests, I am not going >> to spend any >> time reviewing this series, or any other hardware enabling >> series[*]. I don't >> expect KVM specific tests for everything, i.e. it's ok to to rely >> things like >> running VMs that utilize LAM and/or running LAM selftests in the >> guest, but I do >> want a reasonably thorough explanation of how all the test pieces fit >> together to >> validate KVM's implementation. > >Sure, and ack on unit test is part of development work. > >This patch set had always been unit tested before sent out, i.e. >"running LAM selftests in guest" on both ept=Y/N. > >CR4.LAM_SUP, as Chao pointed out, could not be covered by kselftest, I >may explore it in kvm-unit-test. Alternatively, add another kselftest for LAM under kselftests/kvm. >Or, would you mind that separate CR4.LAM_SUP enabling in another patch >set? This isn't a good idea. KVM shouldn't advertise LAM to userspace VMM without CR4.LAM_SUP handling given LAM for supervisor pointers isn't enumerated by a separate CPUID bit. Then, without the "another patch set", this series just adds some dead code to KVM, which, IMO, is unacceptable. >> >> [*] https://lore.kernel.org/all/Y+Uq0JOEmmdI0YwA@google.com >
On Fri, 2023-02-10 at 10:07 +0800, Robert Hoo wrote: > On Thu, 2023-02-09 at 17:27 +0000, Sean Christopherson wrote: > > On Thu, Feb 09, 2023, Robert Hoo wrote: > > > On Thu, 2023-02-09 at 14:15 +0800, Chao Gao wrote: > > > > On Thu, Feb 09, 2023 at 10:40:13AM +0800, Robert Hoo wrote: > > > > Please add a kvm-unit-test or kselftest for LAM, particularly > > > > for > > > > operations (e.g., canonical check for supervisor pointers, > > > > toggle > > > > CR4.LAM_SUP) which aren't covered by the test in Kirill's > > > > series. > > > > > > OK, I can explore for kvm-unit-test in separate patch set. > > > > Please make tests your top priority. Without tests, I am not going > > to spend any > > time reviewing this series, or any other hardware enabling > > series[*]. I don't > > expect KVM specific tests for everything, i.e. it's ok to to rely > > things like > > running VMs that utilize LAM and/or running LAM selftests in the > > guest, but I do > > want a reasonably thorough explanation of how all the test pieces > > fit > > together to > > validate KVM's implementation. > > Sure, and ack on unit test is part of development work. > > This patch set had always been unit tested before sent out, i.e. > "running LAM selftests in guest" on both ept=Y/N. > > CR4.LAM_SUP, as Chao pointed out, could not be covered by kselftest, > I > may explore it in kvm-unit-test. > When I come to kvm-unit-test, just find that I had already developed some test case on CR4.LAM_SUP toggle and carried out on this patch set. Just forgot about it. Is it all right? if so, I will include it in next version. diff --git a/lib/x86/processor.h b/lib/x86/processor.h index 3d58ef7..c6b1db6 100644 --- a/lib/x86/processor.h +++ b/lib/x86/processor.h @@ -105,6 +105,8 @@ #define X86_CR4_CET BIT(X86_CR4_CET_BIT) #define X86_CR4_PKS_BIT (24) #define X86_CR4_PKS BIT(X86_CR4_PKS_BIT) +#define X86_CR4_LAM_SUP_BIT (28) +#define X86_CR4_LAM_SUP BIT(X86_CR4_LAM_SUP_BIT) #define X86_EFLAGS_CF_BIT (0) #define X86_EFLAGS_CF BIT(X86_EFLAGS_CF_BIT) @@ -248,6 +250,7 @@ static inline bool is_intel(void) #define X86_FEATURE_SPEC_CTRL (CPUID(0x7, 0, EDX, 26)) #define X86_FEATURE_ARCH_CAPABILITIES (CPUID(0x7, 0, EDX, 29)) #define X86_FEATURE_PKS (CPUID(0x7, 0, ECX, 31)) +#define X86_FEATURE_LAM (CPUID(0x7, 1, EAX, 26)) /* * Extended Leafs, a.k.a. AMD defined diff --git a/x86/Makefile.x86_64 b/x86/Makefile.x86_64 index f483dea..af626cc 100644 --- a/x86/Makefile.x86_64 +++ b/x86/Makefile.x86_64 @@ -34,6 +34,7 @@ tests += $(TEST_DIR)/rdpru.$(exe) tests += $(TEST_DIR)/pks.$(exe) tests += $(TEST_DIR)/pmu_lbr.$(exe) tests += $(TEST_DIR)/pmu_pebs.$(exe) +tests += $(TEST_DIR)/lam_sup.$(exe) ifeq ($(CONFIG_EFI),y) tests += $(TEST_DIR)/amd_sev.$(exe) diff --git a/x86/lam_sup.c b/x86/lam_sup.c new file mode 100644 index 0000000..3e05129 --- /dev/null +++ b/x86/lam_sup.c @@ -0,0 +1,37 @@ +#include "libcflat.h" +#include "processor.h" +#include "desc.h" + +int main(int ac, char **av) +{ + unsigned long cr4; + struct cpuid c = {0, 0, 0, 0}; + + c = cpuid_indexed(7, 1); + + if (!(c.a & (1 << 26))) { + report_skip("LAM is not supported. EAX = 0x%0x", c.a); + abort(); + } + + report_info("set CR4.LAM_SUP(bit 28)"); + + cr4 = read_cr4(); + write_cr4(cr4 | X86_CR4_LAM_SUP); + + report((cr4 | X86_CR4_LAM_SUP) == read_cr4(), "Set CR4.LAM_SUP succeeded."); + + report_info("clear CR4.LAM_SUP(bit 28)"); + + cr4 = read_cr4(); + write_cr4(cr4 & ~X86_CR4_LAM_SUP); + + report((cr4 & ~X86_CR4_LAM_SUP) == read_cr4(), "Clear CR4.LAM_SUP succeeded."); + + return report_summary(); +} + diff --git a/x86/unittests.cfg b/x86/unittests.cfg index f324e32..0c90dfe 100644 --- a/x86/unittests.cfg +++ b/x86/unittests.cfg @@ -478,3 +478,8 @@ file = cet.flat arch = x86_64 smp = 2 extra_params = -enable-kvm -m 2048 -cpu host + +[intel-lam] +file = lam_sup.flat +arch = x86_64 +extra_params = -enable-kvm -cpu host
On Fri, 2023-02-10 at 11:17 +0800, Chao Gao wrote: > > Alternatively, add another kselftest for LAM under kselftests/kvm. > Let me explore.
On Fri, Feb 10, 2023 at 04:39:49PM +0800, Robert Hoo wrote: >On Fri, 2023-02-10 at 10:07 +0800, Robert Hoo wrote: >> On Thu, 2023-02-09 at 17:27 +0000, Sean Christopherson wrote: >> > On Thu, Feb 09, 2023, Robert Hoo wrote: >> > > On Thu, 2023-02-09 at 14:15 +0800, Chao Gao wrote: >> > > > On Thu, Feb 09, 2023 at 10:40:13AM +0800, Robert Hoo wrote: >> > > > Please add a kvm-unit-test or kselftest for LAM, particularly >> > > > for >> > > > operations (e.g., canonical check for supervisor pointers, >> > > > toggle >> > > > CR4.LAM_SUP) which aren't covered by the test in Kirill's >> > > > series. >> > > >> > > OK, I can explore for kvm-unit-test in separate patch set. >> > >> > Please make tests your top priority. Without tests, I am not going >> > to spend any >> > time reviewing this series, or any other hardware enabling >> > series[*]. I don't >> > expect KVM specific tests for everything, i.e. it's ok to to rely >> > things like >> > running VMs that utilize LAM and/or running LAM selftests in the >> > guest, but I do >> > want a reasonably thorough explanation of how all the test pieces >> > fit >> > together to >> > validate KVM's implementation. >> >> Sure, and ack on unit test is part of development work. >> >> This patch set had always been unit tested before sent out, i.e. >> "running LAM selftests in guest" on both ept=Y/N. >> >> CR4.LAM_SUP, as Chao pointed out, could not be covered by kselftest, >> I >> may explore it in kvm-unit-test. >> >When I come to kvm-unit-test, just find that I had already developed >some test case on CR4.LAM_SUP toggle and carried out on this patch set. >Just forgot about it. > >Is it all right? if so, I will include it in next version. You can add more steps to the test, e.g., 1. check if CR4.LAM_SUP setting takes effort by storing metadata into a supervisor pointer and dereferencing the pointer. 2. turn 5-level paging on/off and check if LAM width complies with the spec. 3. add some negtive tests. e.g., if LAM isn't advertised to the guest, setting CR4.LAM_SUP isn't allowed. ...
On 2/9/2023 10:40 AM, Robert Hoo wrote: > ===Feature Introduction=== > > Linear-address masking (LAM) [1], modifies the checking that is applied to > *64-bit* linear addresses, allowing software to use of the untranslated > address (upper) bits for metadata. > As for which upper bits of linear address can be borrowed, LAM has 2 modes: > LAM_48 (bits 62:48, i.e. LAM width of 15) and LAM_57 (bits 62:57, i.e. LAM > width of 6), controlled by these new bits: CR3[62] (LAM_U48), CR3[61] > (LAM_U57), and CR4[28] (LAM_SUP). > > * LAM_U48 and LAM_U57 bits controls LAM for user mode address. I.e. if > CR3.LAM_U57 = 1, LAM57 is applied; if CR3.LAM_U48 = 1 and CR3.LAM_U57 = 0, > LAM48 is applied. > * LAM_SUP bit, combined with paging mode (4-level or 5-level), determines > LAM status for supervisor mode address. I.e. when CR4.LAM_SUP =1, 4-level > paging mode will have LAM48 for supervisor mode address while 5-level paging > will have LAM57. > > Note: > 1. LAM applies to only data address, not to instructions. > 2. LAM identification of an address as user or supervisor is based solely on the > value of pointer bit 63 and does not, for the purposes of LAM, depend on the CPL. > 3. For user mode address, it is possible that 5-level paging and LAM_U48 are both > set, in this case, the effective usable linear address width is 48, i.e. bit > 56:47 is reserved by LAM. [2] How to understand "reserved by LAM"? According to the spec, bits 56:48 of the pointer contained metadata. > 4. When VM exit, the problematic address saved in VMCS field is clean, i.e. > metadata cleared with canonical form. > > > ===LAM KVM Design=== > > Intercept CR4.LAM_SUP by KVM, to avoid read VMCS field every time, with > expectation that guest won't toggle this bit frequently. > > Under EPT mode, CR3 is fully under guest control, guest LAM is thus transparent to > KVM. Nothing more need to do. > > For Shadow paging (EPT = off), KVM need to handle guest CR3.LAM_U48 and CR3.LAM_U57 > toggles. > > [1] ISE Chap10 https://cdrdv2.intel.com/v1/dl/getContent/671368 (Section 10.6 VMX interaction) > [2] Thus currently, Kernel enabling patch only enables LAM_U57. https://lore.kernel.org/lkml/20230123220500.21077-1-kirill.shutemov@linux.intel.com/ > > --- > Changelog > v3 --> v4: > Drop unrelated Patch 1 in v3 (Binbin, Sean, Xiaoyao) > Intercept CR4.LAM_SUP instead of pass through to guest (Sean) > Just filter out CR3.LAM_{U48, U57}, instead of all reserved high bits > (Sean, Yuan) > Use existing __canonical_address() helper instead write a new one (Weijiang) > Add LAM handling in KVM emulation (Yu, Yuan) > Add Jingqi's reviwed-by on Patch 7 > Rebased to Kirill's latest code, which is 6.2-rc1 base. > > v2 --> v3: > As LAM Kernel patches are in tip tree now, rebase to it. > https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/ > > v1 --> v2: > 1. Fixes i386-allyesconfig build error on get_pgd(), where > CR3_HIGH_RSVD_MASK isn't applicable. > (Reported-by: kernel test robot <lkp@intel.com>) > 2. In kvm_set_cr3(), be conservative on skip tlb flush when only LAM bits > toggles. (Kirill) > > > Robert Hoo (9): > KVM: x86: Intercept CR4.LAM_SUP when LAM feature is enabled in guest > KVM: x86: MMU: Clear CR3 LAM bits when allocate shadow root > [Trivial] KVM: x86: MMU: Commets update > KVM: x86: MMU: Integrate LAM bits when build guest CR3 > KVM: x86: Untag LAM bits when applicable > KVM: x86: When judging setting CR3 valid or not, consider LAM bits > KVM: x86: When guest set CR3, handle LAM bits semantics > KVM: x86: LAM: Expose LAM CPUID to user space VMM > KVM: x86: emulation: Apply LAM when emulating data access > > arch/x86/include/asm/kvm_host.h | 3 ++- > arch/x86/kvm/cpuid.c | 2 +- > arch/x86/kvm/emulate.c | 6 +++++ > arch/x86/kvm/mmu.h | 5 ++++ > arch/x86/kvm/mmu/mmu.c | 11 ++++++-- > arch/x86/kvm/vmx/vmx.c | 6 ++++- > arch/x86/kvm/x86.c | 38 ++++++++++++++++++++++---- > arch/x86/kvm/x86.h | 47 +++++++++++++++++++++++++++++++++ > 8 files changed, 108 insertions(+), 10 deletions(-) > > base-commit: 03334443640f226f56f71b5dfa3b1be6d4a1a1bc > (https://git.kernel.org/pub/scm/linux/kernel/git/kas/linux.git lam)
On Mon, 2023-02-13 at 17:02 +0800, Binbin Wu wrote: > > 3. For user mode address, it is possible that 5-level paging and > > LAM_U48 are both > > set, in this case, the effective usable linear address width is > > 48, i.e. bit > > 56:47 is reserved by LAM. [2] > > How to understand "reserved by LAM"? > > According to the spec, bits 56:48 of the pointer contained metadata. > Emm, looks it's superfluous words, I'll remove it in next version. I meant the same as your understanding: bit 56:48 could be used by metadata, and cleared before translation, therefore not usable by 5- level paging, "reserved by LAM".
===Feature Introduction=== Linear-address masking (LAM) [1], modifies the checking that is applied to *64-bit* linear addresses, allowing software to use of the untranslated address (upper) bits for metadata. As for which upper bits of linear address can be borrowed, LAM has 2 modes: LAM_48 (bits 62:48, i.e. LAM width of 15) and LAM_57 (bits 62:57, i.e. LAM width of 6), controlled by these new bits: CR3[62] (LAM_U48), CR3[61] (LAM_U57), and CR4[28] (LAM_SUP). * LAM_U48 and LAM_U57 bits controls LAM for user mode address. I.e. if CR3.LAM_U57 = 1, LAM57 is applied; if CR3.LAM_U48 = 1 and CR3.LAM_U57 = 0, LAM48 is applied. * LAM_SUP bit, combined with paging mode (4-level or 5-level), determines LAM status for supervisor mode address. I.e. when CR4.LAM_SUP =1, 4-level paging mode will have LAM48 for supervisor mode address while 5-level paging will have LAM57. Note: 1. LAM applies to only data address, not to instructions. 2. LAM identification of an address as user or supervisor is based solely on the value of pointer bit 63 and does not, for the purposes of LAM, depend on the CPL. 3. For user mode address, it is possible that 5-level paging and LAM_U48 are both set, in this case, the effective usable linear address width is 48, i.e. bit 56:47 is reserved by LAM. [2] 4. When VM exit, the problematic address saved in VMCS field is clean, i.e. metadata cleared with canonical form. ===LAM KVM Design=== Intercept CR4.LAM_SUP by KVM, to avoid read VMCS field every time, with expectation that guest won't toggle this bit frequently. Under EPT mode, CR3 is fully under guest control, guest LAM is thus transparent to KVM. Nothing more need to do. For Shadow paging (EPT = off), KVM need to handle guest CR3.LAM_U48 and CR3.LAM_U57 toggles. [1] ISE Chap10 https://cdrdv2.intel.com/v1/dl/getContent/671368 (Section 10.6 VMX interaction) [2] Thus currently, Kernel enabling patch only enables LAM_U57. https://lore.kernel.org/lkml/20230123220500.21077-1-kirill.shutemov@linux.intel.com/ --- Changelog v3 --> v4: Drop unrelated Patch 1 in v3 (Binbin, Sean, Xiaoyao) Intercept CR4.LAM_SUP instead of pass through to guest (Sean) Just filter out CR3.LAM_{U48, U57}, instead of all reserved high bits (Sean, Yuan) Use existing __canonical_address() helper instead write a new one (Weijiang) Add LAM handling in KVM emulation (Yu, Yuan) Add Jingqi's reviwed-by on Patch 7 Rebased to Kirill's latest code, which is 6.2-rc1 base. v2 --> v3: As LAM Kernel patches are in tip tree now, rebase to it. https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/ v1 --> v2: 1. Fixes i386-allyesconfig build error on get_pgd(), where CR3_HIGH_RSVD_MASK isn't applicable. (Reported-by: kernel test robot <lkp@intel.com>) 2. In kvm_set_cr3(), be conservative on skip tlb flush when only LAM bits toggles. (Kirill) Robert Hoo (9): KVM: x86: Intercept CR4.LAM_SUP when LAM feature is enabled in guest KVM: x86: MMU: Clear CR3 LAM bits when allocate shadow root [Trivial] KVM: x86: MMU: Commets update KVM: x86: MMU: Integrate LAM bits when build guest CR3 KVM: x86: Untag LAM bits when applicable KVM: x86: When judging setting CR3 valid or not, consider LAM bits KVM: x86: When guest set CR3, handle LAM bits semantics KVM: x86: LAM: Expose LAM CPUID to user space VMM KVM: x86: emulation: Apply LAM when emulating data access arch/x86/include/asm/kvm_host.h | 3 ++- arch/x86/kvm/cpuid.c | 2 +- arch/x86/kvm/emulate.c | 6 +++++ arch/x86/kvm/mmu.h | 5 ++++ arch/x86/kvm/mmu/mmu.c | 11 ++++++-- arch/x86/kvm/vmx/vmx.c | 6 ++++- arch/x86/kvm/x86.c | 38 ++++++++++++++++++++++---- arch/x86/kvm/x86.h | 47 +++++++++++++++++++++++++++++++++ 8 files changed, 108 insertions(+), 10 deletions(-) base-commit: 03334443640f226f56f71b5dfa3b1be6d4a1a1bc (https://git.kernel.org/pub/scm/linux/kernel/git/kas/linux.git lam)