On Mon, Feb 13, 2023 at 09:25:50PM +0800, Robert Hoo wrote:
>On Mon, 2023-02-13 at 10:01 +0800, Chao Gao wrote:
>> On Thu, Feb 09, 2023 at 10:40:19AM +0800, Robert Hoo wrote:
>> > Before apply to kvm_vcpu_is_illegal_gpa(), clear LAM bits if it's
>> > valid.
>>
>> I prefer to squash this patch into patch 2 because it is also related
>> to
>> CR3 LAM bits handling.
>>
>Though all surround CR3, I would prefer split into pieces, so that
>easier for review and accept. I can change their order to group
>together. Is is all right for you?
No. To me, it doesn't make review easier at all.
This patch itself is incomplete and lacks proper justification. Merging
related patches together can show the whole picture and it is easier
to write some justification.
There are two ways that make sense to me:
option 1:
patch 1: virtualize CR4.LAM_SUP
patch 2: virtualize CR3.LAM_U48/U57
patch 3: virtualize LAM masking on applicable data accesses
patch 4: expose LAM CPUID bit to user sapce VMM
option 2:
given the series has only 100 LoC, you can merge all patches into a
single patch.
>> > {
>> > bool skip_tlb_flush = false;
>> > @@ -1254,7 +1262,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu,
>> > unsigned long cr3)
>> > * stuff CR3, e.g. for RSM emulation, and there is no guarantee
>> > that
>> > * the current vCPU mode is accurate.
>> > */
>> > - if (kvm_vcpu_is_illegal_gpa(vcpu, cr3))
>> > + if (!kvm_is_valid_cr3(vcpu, cr3))
>>
>> There are other call sites of kvm_vcpu_is_illegal_gpa() to validate
>> cr3.
>> Do you need to modify them?
>
>I don't think so. Others are for gpa validation, no need to change.
>Here is for CR3.
how about the call in kvm_is_valid_sregs()? if you don't change it, when
user space VMM tries to set a CR3 with any LAM bits, KVM thinks the CR3
is illegal and returns an error. To me it means live migration probably
is broken.
And the call in nested_vmx_check_host_state()? L1 VMM should be allowed to
program a CR3 with LAM bits set to VMCS's HOST_CR3 field. Actually, it
is exactly what this patch 6 is doing.
Please inspect other call sites carefully.
>>
>> > return 1;
>> >
>> > if (is_pae_paging(vcpu) && !load_pdptrs(vcpu, cr3))
>> > --
>> > 2.31.1
>> >
>
