Re: [PATCH v4 0/9] Linear Address Masking (LAM) KVM Enabling

[Chao Gao <chao.gao@intel.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
>