[PATCH] KVM: vmx: hyper-v: don't pass EPT configuration info to vmx_hv_remote_flush_tlb()

[PATCH] KVM: vmx: hyper-v: don't pass EPT configuration info to vmx_hv_remote_flush_tlb()

[Vitaly Kuznetsov]

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(-)

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

Re: [PATCH] KVM: vmx: hyper-v: don't pass EPT configuration info to vmx_hv_remote_flush_tlb()

[Tianyu Lan]

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
>


-- 
Best regards
Tianyu Lan

Re: [PATCH] KVM: vmx: hyper-v: don't pass EPT configuration info to vmx_hv_remote_flush_tlb()

[Vitaly Kuznetsov]

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]

-- 
Vitaly

Re: [PATCH] KVM: vmx: hyper-v: don't pass EPT configuration info to vmx_hv_remote_flush_tlb()

[Tianyu Lan]

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.
-- 
Best regards
Tianyu Lan

Re: [PATCH] KVM: vmx: hyper-v: don't pass EPT configuration info to vmx_hv_remote_flush_tlb()

[Vitaly Kuznetsov]

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.

-- 
Vitaly

Re: [PATCH] KVM: vmx: hyper-v: don't pass EPT configuration info to vmx_hv_remote_flush_tlb()

[Tianyu Lan]

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.
-- 
Best regards
Tianyu Lan

Re: [PATCH] KVM: vmx: hyper-v: don't pass EPT configuration info to vmx_hv_remote_flush_tlb()

[Paolo Bonzini]

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