| Message ID | 20181011100312.4083-1-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: vmx: hyper-v: don't pass EPT configuration info to vmx_hv_remote_flush_tlb() | expand |
On Thu, Oct 11, 2018 at 6:32 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > I'm observing random crashes in multi-vCPU L2 guests running on KVM on > Hyper-V. I bisected the issue to the commit 877ad952be3d ("KVM: vmx: Add > tlb_remote_flush callback support"). Hyper-V TLFS states: > > "AddressSpace specifies an address space ID (an EPT PML4 table pointer)" > > So apparently, Hyper-V doesn't expect us to pass naked EPTP, only PML4 > pointer should be used. Strip off EPT configuration information before > calling into vmx_hv_remote_flush_tlb(). Hi Vitaly: : Thanks to fix this. Sorry, I didn't meet the issue.. I think we may just store EPT PML4 table pointer without EPT configuration information in the ept_pointer field for this case. diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 619307b3e6bb..e316058b41a6 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -5379,7 +5379,7 @@ static void vmx_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) if (kvm_x86_ops->tlb_remote_flush) { spin_lock(&to_kvm_vmx(kvm)->ept_pointer_lock); - to_vmx(vcpu)->ept_pointer = eptp; + to_vmx(vcpu)->ept_pointer = cr3; to_kvm_vmx(kvm)->ept_pointers_match = EPT_POINTERS_CHECK; spin_unlock(&to_kvm_vmx(kvm)->ept_pointer_lock); > > Fixes: 877ad952be3d ("KVM: vmx: Add tlb_remote_flush callback support") > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/x86/kvm/vmx.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 612fd17be635..e665aa7167cf 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -1572,8 +1572,12 @@ static int vmx_hv_remote_flush_tlb(struct kvm *kvm) > goto out; > } > > + /* > + * FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE hypercall needs the address of the > + * base of EPT PML4 table, strip off EPT configuration information. > + */ > ret = hyperv_flush_guest_mapping( > - to_vmx(kvm_get_vcpu(kvm, 0))->ept_pointer); > + to_vmx(kvm_get_vcpu(kvm, 0))->ept_pointer & PAGE_MASK); > > out: > spin_unlock(&to_kvm_vmx(kvm)->ept_pointer_lock); > -- > 2.17.1 >
Tianyu Lan <lantianyu1986@gmail.com> writes: > On Thu, Oct 11, 2018 at 6:32 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> >> I'm observing random crashes in multi-vCPU L2 guests running on KVM on >> Hyper-V. I bisected the issue to the commit 877ad952be3d ("KVM: vmx: Add >> tlb_remote_flush callback support"). Hyper-V TLFS states: >> >> "AddressSpace specifies an address space ID (an EPT PML4 table pointer)" >> >> So apparently, Hyper-V doesn't expect us to pass naked EPTP, only PML4 >> pointer should be used. Strip off EPT configuration information before >> calling into vmx_hv_remote_flush_tlb(). > > Hi Vitaly: > : Thanks to fix this. Sorry, I didn't meet the issue.. > I think we may just store EPT PML4 table pointer without EPT > configuration information > in the ept_pointer field for this case. > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 619307b3e6bb..e316058b41a6 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -5379,7 +5379,7 @@ static void vmx_set_cr3(struct kvm_vcpu *vcpu, > unsigned long cr3) > > if (kvm_x86_ops->tlb_remote_flush) { > spin_lock(&to_kvm_vmx(kvm)->ept_pointer_lock); > - to_vmx(vcpu)->ept_pointer = eptp; > + to_vmx(vcpu)->ept_pointer = cr3; True, we can do that (and I even had a version of my patch doing so) but 'ept_pointer' will likely need to be renamed as it's not obvious why vcpu->ept_pointer != eptp. Alternatively, we can filter lower 12 bits out in hyperv_flush_guest_mapping() but I would rename 'as' parameter to eptp then. [snip]
On Thu, Oct 11, 2018 at 8:18 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > Tianyu Lan <lantianyu1986@gmail.com> writes: > > > On Thu, Oct 11, 2018 at 6:32 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > >> > >> I'm observing random crashes in multi-vCPU L2 guests running on KVM on > >> Hyper-V. I bisected the issue to the commit 877ad952be3d ("KVM: vmx: Add > >> tlb_remote_flush callback support"). Hyper-V TLFS states: > >> > >> "AddressSpace specifies an address space ID (an EPT PML4 table pointer)" > >> > >> So apparently, Hyper-V doesn't expect us to pass naked EPTP, only PML4 > >> pointer should be used. Strip off EPT configuration information before > >> calling into vmx_hv_remote_flush_tlb(). > > > > Hi Vitaly: > > : Thanks to fix this. Sorry, I didn't meet the issue.. > > I think we may just store EPT PML4 table pointer without EPT > > configuration information > > in the ept_pointer field for this case. > > > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > > index 619307b3e6bb..e316058b41a6 100644 > > --- a/arch/x86/kvm/vmx.c > > +++ b/arch/x86/kvm/vmx.c > > @@ -5379,7 +5379,7 @@ static void vmx_set_cr3(struct kvm_vcpu *vcpu, > > unsigned long cr3) > > > > if (kvm_x86_ops->tlb_remote_flush) { > > spin_lock(&to_kvm_vmx(kvm)->ept_pointer_lock); > > - to_vmx(vcpu)->ept_pointer = eptp; > > + to_vmx(vcpu)->ept_pointer = cr3; > > True, we can do that (and I even had a version of my patch doing so) > but 'ept_pointer' will likely need to be renamed as it's not obvious > why vcpu->ept_pointer != eptp. > Yes. that need to rename ept_pointer. > Alternatively, we can filter lower 12 bits out in > hyperv_flush_guest_mapping() but I would rename 'as' parameter to eptp > then. OK. I got it. Thanks.
Tianyu Lan <lantianyu1986@gmail.com> writes: > On Thu, Oct 11, 2018 at 8:18 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> >> Tianyu Lan <lantianyu1986@gmail.com> writes: >> >> > On Thu, Oct 11, 2018 at 6:32 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> >> >> >> I'm observing random crashes in multi-vCPU L2 guests running on KVM on >> >> Hyper-V. I bisected the issue to the commit 877ad952be3d ("KVM: vmx: Add >> >> tlb_remote_flush callback support"). Hyper-V TLFS states: >> >> >> >> "AddressSpace specifies an address space ID (an EPT PML4 table pointer)" >> >> >> >> So apparently, Hyper-V doesn't expect us to pass naked EPTP, only PML4 >> >> pointer should be used. Strip off EPT configuration information before >> >> calling into vmx_hv_remote_flush_tlb(). >> > >> > Hi Vitaly: >> > : Thanks to fix this. Sorry, I didn't meet the issue.. >> > I think we may just store EPT PML4 table pointer without EPT >> > configuration information >> > in the ept_pointer field for this case. >> > >> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> > index 619307b3e6bb..e316058b41a6 100644 >> > --- a/arch/x86/kvm/vmx.c >> > +++ b/arch/x86/kvm/vmx.c >> > @@ -5379,7 +5379,7 @@ static void vmx_set_cr3(struct kvm_vcpu *vcpu, >> > unsigned long cr3) >> > >> > if (kvm_x86_ops->tlb_remote_flush) { >> > spin_lock(&to_kvm_vmx(kvm)->ept_pointer_lock); >> > - to_vmx(vcpu)->ept_pointer = eptp; >> > + to_vmx(vcpu)->ept_pointer = cr3; >> >> True, we can do that (and I even had a version of my patch doing so) >> but 'ept_pointer' will likely need to be renamed as it's not obvious >> why vcpu->ept_pointer != eptp. >> > > Yes. that need to rename ept_pointer. > Honestly, I would prefer to keep more information cached, e.g. if someone needs EPT configuration data later he can easily get it from ept_pointer and by putting raw cr3 there we'll just keep less. But I don't have a strong opinion, I'll leave it up to the maintainers to tell us how to proceed) >> Alternatively, we can filter lower 12 bits out in >> hyperv_flush_guest_mapping() but I would rename 'as' parameter to eptp >> then. > > OK. I got it. Thanks.
On Thu, Oct 11, 2018 at 9:00 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > Tianyu Lan <lantianyu1986@gmail.com> writes: > > > On Thu, Oct 11, 2018 at 8:18 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > >> > >> Tianyu Lan <lantianyu1986@gmail.com> writes: > >> > >> > On Thu, Oct 11, 2018 at 6:32 PM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > >> >> > >> >> I'm observing random crashes in multi-vCPU L2 guests running on KVM on > >> >> Hyper-V. I bisected the issue to the commit 877ad952be3d ("KVM: vmx: Add > >> >> tlb_remote_flush callback support"). Hyper-V TLFS states: > >> >> > >> >> "AddressSpace specifies an address space ID (an EPT PML4 table pointer)" > >> >> > >> >> So apparently, Hyper-V doesn't expect us to pass naked EPTP, only PML4 > >> >> pointer should be used. Strip off EPT configuration information before > >> >> calling into vmx_hv_remote_flush_tlb(). > >> > > >> > Hi Vitaly: > >> > : Thanks to fix this. Sorry, I didn't meet the issue.. > >> > I think we may just store EPT PML4 table pointer without EPT > >> > configuration information > >> > in the ept_pointer field for this case. > >> > > >> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > >> > index 619307b3e6bb..e316058b41a6 100644 > >> > --- a/arch/x86/kvm/vmx.c > >> > +++ b/arch/x86/kvm/vmx.c > >> > @@ -5379,7 +5379,7 @@ static void vmx_set_cr3(struct kvm_vcpu *vcpu, > >> > unsigned long cr3) > >> > > >> > if (kvm_x86_ops->tlb_remote_flush) { > >> > spin_lock(&to_kvm_vmx(kvm)->ept_pointer_lock); > >> > - to_vmx(vcpu)->ept_pointer = eptp; > >> > + to_vmx(vcpu)->ept_pointer = cr3; > >> > >> True, we can do that (and I even had a version of my patch doing so) > >> but 'ept_pointer' will likely need to be renamed as it's not obvious > >> why vcpu->ept_pointer != eptp. > >> > > > > Yes. that need to rename ept_pointer. > > > > Honestly, I would prefer to keep more information cached, e.g. if > someone needs EPT configuration data later he can easily get it from > ept_pointer and by putting raw cr3 there we'll just keep less. > Yes, that makes sense.
On 11/10/2018 15:00, Vitaly Kuznetsov wrote: >> Yes. that need to rename ept_pointer. >> > Honestly, I would prefer to keep more information cached, e.g. if > someone needs EPT configuration data later he can easily get it from > ept_pointer and by putting raw cr3 there we'll just keep less. > > But I don't have a strong opinion, I'll leave it up to the maintainers > to tell us how to proceed) > I have (re)applied your patch (I had queued it and tested it earlier, now I've added it back). Paolo
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 612fd17be635..e665aa7167cf 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -1572,8 +1572,12 @@ static int vmx_hv_remote_flush_tlb(struct kvm *kvm) goto out; } + /* + * FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE hypercall needs the address of the + * base of EPT PML4 table, strip off EPT configuration information. + */ ret = hyperv_flush_guest_mapping( - to_vmx(kvm_get_vcpu(kvm, 0))->ept_pointer); + to_vmx(kvm_get_vcpu(kvm, 0))->ept_pointer & PAGE_MASK); out: spin_unlock(&to_kvm_vmx(kvm)->ept_pointer_lock);
I'm observing random crashes in multi-vCPU L2 guests running on KVM on Hyper-V. I bisected the issue to the commit 877ad952be3d ("KVM: vmx: Add tlb_remote_flush callback support"). Hyper-V TLFS states: "AddressSpace specifies an address space ID (an EPT PML4 table pointer)" So apparently, Hyper-V doesn't expect us to pass naked EPTP, only PML4 pointer should be used. Strip off EPT configuration information before calling into vmx_hv_remote_flush_tlb(). Fixes: 877ad952be3d ("KVM: vmx: Add tlb_remote_flush callback support") Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/kvm/vmx.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)