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@xxxxxxxxxx >
