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. ...
