2020-08-24 09:21:40

by Lai Jiangshan

[permalink] [raw]
Subject: [PATCH] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed

From: Lai Jiangshan <[email protected]>

8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes)
changed it without giving any reason in the changelog.

In theory, the syncing is needed, and need to be fixed by reverting
this part of change.

Signed-off-by: Lai Jiangshan <[email protected]>
---
arch/x86/kvm/mmu/mmu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 4e03841f053d..9a93de921f2b 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
}

if (sp->unsync_children)
- kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
+ kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);

__clear_sp_write_flooding_count(sp);

--
2.19.1.6.gb485710b


2020-08-28 01:49:28

by Lai Jiangshan

[permalink] [raw]
Subject: Re: [PATCH] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed

Ping @Sean Christopherson

On Mon, Aug 24, 2020 at 5:18 PM Lai Jiangshan <[email protected]> wrote:
>
> From: Lai Jiangshan <[email protected]>
>
> 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes)
> changed it without giving any reason in the changelog.
>
> In theory, the syncing is needed, and need to be fixed by reverting
> this part of change.
>
> Signed-off-by: Lai Jiangshan <[email protected]>
> ---
> arch/x86/kvm/mmu/mmu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 4e03841f053d..9a93de921f2b 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> }
>
> if (sp->unsync_children)
> - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
> + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
>
> __clear_sp_write_flooding_count(sp);
>
> --
> 2.19.1.6.gb485710b
>

2020-08-28 01:50:27

by Lai Jiangshan

[permalink] [raw]
Subject: Re: [PATCH] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed

Ping @Sean Christopherson

On Mon, Aug 24, 2020 at 5:18 PM Lai Jiangshan <[email protected]> wrote:
>
> From: Lai Jiangshan <[email protected]>
>
> 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes)
> changed it without giving any reason in the changelog.
>
> In theory, the syncing is needed, and need to be fixed by reverting
> this part of change.
>
> Signed-off-by: Lai Jiangshan <[email protected]>
> ---
> arch/x86/kvm/mmu/mmu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 4e03841f053d..9a93de921f2b 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> }
>
> if (sp->unsync_children)
> - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
> + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
>
> __clear_sp_write_flooding_count(sp);
>
> --
> 2.19.1.6.gb485710b
>

2020-08-31 13:11:32

by Vitaly Kuznetsov

[permalink] [raw]
Subject: Re: [PATCH] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed

Lai Jiangshan <[email protected]> writes:

> Ping @Sean Christopherson
>

Let's try 'Beetlejuice' instead :-)

> On Mon, Aug 24, 2020 at 5:18 PM Lai Jiangshan <[email protected]> wrote:
>>
>> From: Lai Jiangshan <[email protected]>
>>
>> 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes)
>> changed it without giving any reason in the changelog.
>>
>> In theory, the syncing is needed, and need to be fixed by reverting
>> this part of change.

Even if the original commit is not wordy enough this is hardly
better. Are you seeing a particular scenario when a change in current
vCPU's MMU requires flushing TLB entries for *other* contexts, ... (see
below)

>>
>> Signed-off-by: Lai Jiangshan <[email protected]>
>> ---
>> arch/x86/kvm/mmu/mmu.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
>> index 4e03841f053d..9a93de921f2b 100644
>> --- a/arch/x86/kvm/mmu/mmu.c
>> +++ b/arch/x86/kvm/mmu/mmu.c
>> @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
>> }
>>
>> if (sp->unsync_children)
>> - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
>> + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);

... in particular, why are you reverting only this hunk? Please elaborate.

>>
>> __clear_sp_write_flooding_count(sp);
>>
>> --
>> 2.19.1.6.gb485710b
>>
>

--
Vitaly

2020-09-01 01:30:52

by Lai Jiangshan

[permalink] [raw]
Subject: Re: [PATCH] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed

On Mon, Aug 31, 2020 at 9:09 PM Vitaly Kuznetsov <[email protected]> wrote:
>
> Lai Jiangshan <[email protected]> writes:
>
> > Ping @Sean Christopherson
> >
>
> Let's try 'Beetlejuice' instead :-)
>
> > On Mon, Aug 24, 2020 at 5:18 PM Lai Jiangshan <[email protected]> wrote:
> >>
> >> From: Lai Jiangshan <[email protected]>
> >>
> >> 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes)
> >> changed it without giving any reason in the changelog.
> >>
> >> In theory, the syncing is needed, and need to be fixed by reverting
> >> this part of change.
>
> Even if the original commit is not wordy enough this is hardly
> better.

Hello,
Thank you for reviewing it.

I'm sorry that when I said "reverting this part of change",
I meant "reverting this line of code". This line of code itself
is quite clear that it is not related to the original commit
according to its changelog.

> Are you seeing a particular scenario when a change in current
> vCPU's MMU requires flushing TLB entries for *other* contexts, ... (see
> below)

So I don't think the patch needs to explain this because the patch
does not change/revert anything about it.

Anyway, using a "revert" in the changelog is misleading, when it
is not really reverting the whole targeted commit. I would
remove this wording.

For the change in my patch, when kvm_mmu_get_page() gets a
page with unsync children, the host side pagetable is
unsynchronized with the guest side pagedtable, and the
guest might not issue a "flush" operation on it. It is
all about the host's emulation of the pagetable. So the host
has the responsibility to synchronize the pagetables.

Thanks
Lai

> >>
> >> Signed-off-by: Lai Jiangshan <[email protected]>
> >> ---
> >> arch/x86/kvm/mmu/mmu.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> >> index 4e03841f053d..9a93de921f2b 100644
> >> --- a/arch/x86/kvm/mmu/mmu.c
> >> +++ b/arch/x86/kvm/mmu/mmu.c
> >> @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> >> }
> >>
> >> if (sp->unsync_children)
> >> - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
> >> + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
>
> ... in particular, why are you reverting only this hunk? Please elaborate.
>
> >>
> >> __clear_sp_write_flooding_count(sp);
> >>
> >> --
> >> 2.19.1.6.gb485710b
> >>
> >
>
> --
> Vitaly
>

2020-09-01 08:12:45

by Vitaly Kuznetsov

[permalink] [raw]
Subject: Re: [PATCH] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed

Lai Jiangshan <[email protected]> writes:

> On Mon, Aug 31, 2020 at 9:09 PM Vitaly Kuznetsov <[email protected]> wrote:
>>
>> Lai Jiangshan <[email protected]> writes:
>>
>> > Ping @Sean Christopherson
>> >
>>
>> Let's try 'Beetlejuice' instead :-)
>>
>> > On Mon, Aug 24, 2020 at 5:18 PM Lai Jiangshan <[email protected]> wrote:
>> >>
>> >> From: Lai Jiangshan <[email protected]>
>> >>
>> >> 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes)
>> >> changed it without giving any reason in the changelog.
>> >>
>> >> In theory, the syncing is needed, and need to be fixed by reverting
>> >> this part of change.
>>
>> Even if the original commit is not wordy enough this is hardly
>> better.
>
> Hello,
> Thank you for reviewing it.
>
> I'm sorry that when I said "reverting this part of change",
> I meant "reverting this line of code". This line of code itself
> is quite clear that it is not related to the original commit
> according to its changelog.
>
>> Are you seeing a particular scenario when a change in current
>> vCPU's MMU requires flushing TLB entries for *other* contexts, ... (see
>> below)
>
> So I don't think the patch needs to explain this because the patch
> does not change/revert anything about it.
>
> Anyway, using a "revert" in the changelog is misleading, when it
> is not really reverting the whole targeted commit. I would
> remove this wording.
>
> For the change in my patch, when kvm_mmu_get_page() gets a
> page with unsync children, the host side pagetable is
> unsynchronized with the guest side pagedtable, and the
> guest might not issue a "flush" operation on it. It is
> all about the host's emulation of the pagetable. So the host
> has the responsibility to synchronize the pagetables.
>

Ah, I see now, so it seems Sean's commit has a stray change in it: the
intention was to change KVM_REQ_TLB_FLUSH -> KVM_REQ_TLB_FLUSH_CURRENT
so we don't unneedlesly flush other contexts but one of the hunks
changed KVM_REQ_MMU_SYNC instead. Syncronizing MMU roots can't be
replaced with a TLB flush, we need to revert back the change. This
sounds reasonable to me, please send out v2 with the updated
description. Thanks!

--
Vitaly

2020-09-02 12:56:57

by Lai Jiangshan

[permalink] [raw]
Subject: [PATCH V2] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed

From: Lai Jiangshan <[email protected]>

When kvm_mmu_get_page() gets a page with unsynced children, the spt
pagetable is unsynchronized with the guest pagetable. But the
guest might not issue a "flush" operation on it when the pagetable
entry is changed from zero or other cases. The hypervisor has the
responsibility to synchronize the pagetables.

The linux kernel behaves as above for many years, But
8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes)
inadvertently included a line of code to change it without giving any
reason in the changelog. It is clear that the commit's intention was to
change KVM_REQ_TLB_FLUSH -> KVM_REQ_TLB_FLUSH_CURRENT, so we don't
unneedlesly flush other contexts but one of the hunks changed
nearby KVM_REQ_MMU_SYNC instead.

The this patch changes it back.

Link: https://lore.kernel.org/lkml/[email protected]/
Cc: Sean Christopherson <[email protected]>
Cc: Vitaly Kuznetsov <[email protected]>
Signed-off-by: Lai Jiangshan <[email protected]>
---
Changed from v1:
update patch description

arch/x86/kvm/mmu/mmu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 4e03841f053d..9a93de921f2b 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
}

if (sp->unsync_children)
- kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
+ kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);

__clear_sp_write_flooding_count(sp);

--
2.19.1.6.gb485710b

2020-09-02 14:50:14

by Vitaly Kuznetsov

[permalink] [raw]
Subject: Re: [PATCH V2] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed

Lai Jiangshan <[email protected]> writes:

> From: Lai Jiangshan <[email protected]>
>
> When kvm_mmu_get_page() gets a page with unsynced children, the spt
> pagetable is unsynchronized with the guest pagetable. But the
> guest might not issue a "flush" operation on it when the pagetable
> entry is changed from zero or other cases. The hypervisor has the
> responsibility to synchronize the pagetables.
>
> The linux kernel behaves as above for many years, But
> 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes)

Nit: checkpatch.pl complains here with

ERROR: Please use git commit description style 'commit <12+ chars of
sha1> ("<title line>")' - ie: 'commit 8c8560b83390 ("KVM: x86/mmu: Use
KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes")'
#118:
8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes)

> inadvertently included a line of code to change it without giving any
> reason in the changelog. It is clear that the commit's intention was to
> change KVM_REQ_TLB_FLUSH -> KVM_REQ_TLB_FLUSH_CURRENT, so we don't
> unneedlesly flush other contexts but one of the hunks changed
> nearby KVM_REQ_MMU_SYNC instead.
>
> The this patch changes it back.
>
> Link: https://lore.kernel.org/lkml/[email protected]/
> Cc: Sean Christopherson <[email protected]>
> Cc: Vitaly Kuznetsov <[email protected]>
> Signed-off-by: Lai Jiangshan <[email protected]>
> ---
> Changed from v1:
> update patch description
>
> arch/x86/kvm/mmu/mmu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 4e03841f053d..9a93de921f2b 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> }
>
> if (sp->unsync_children)
> - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
> + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
>
> __clear_sp_write_flooding_count(sp);

FWIW,

Reviewed-by: Vitaly Kuznetsov <[email protected]>

but it'd be great to hear from Sean).

--
Vitaly

2020-09-03 01:26:24

by Sean Christopherson

[permalink] [raw]
Subject: Re: [PATCH V2] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed

On Wed, Sep 02, 2020 at 04:12:55PM +0200, Vitaly Kuznetsov wrote:
> Lai Jiangshan <[email protected]> writes:
>
> > From: Lai Jiangshan <[email protected]>
> >
> > When kvm_mmu_get_page() gets a page with unsynced children, the spt
> > pagetable is unsynchronized with the guest pagetable. But the
> > guest might not issue a "flush" operation on it when the pagetable
> > entry is changed from zero or other cases. The hypervisor has the
> > responsibility to synchronize the pagetables.
> >
> > The linux kernel behaves as above for many years, But
> > 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes)
>
> Nit: checkpatch.pl complains here with
>
> ERROR: Please use git commit description style 'commit <12+ chars of
> sha1> ("<title line>")' - ie: 'commit 8c8560b83390 ("KVM: x86/mmu: Use
> KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes")'
> #118:
> 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes)

Definitely needs a

Fixes: 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes)

At that point I'd just have the changelog say "a recent commit".

> > inadvertently included a line of code to change it without giving any
> > reason in the changelog. It is clear that the commit's intention was to
> > change KVM_REQ_TLB_FLUSH -> KVM_REQ_TLB_FLUSH_CURRENT, so we don't
> > unneedlesly flush other contexts but one of the hunks changed
> > nearby KVM_REQ_MMU_SYNC instead.
> >
> > The this patch changes it back.
> >
> > Link: https://lore.kernel.org/lkml/[email protected]/
> > Cc: Sean Christopherson <[email protected]>
> > Cc: Vitaly Kuznetsov <[email protected]>
> > Signed-off-by: Lai Jiangshan <[email protected]>
> > ---
> > Changed from v1:
> > update patch description
> >
> > arch/x86/kvm/mmu/mmu.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 4e03841f053d..9a93de921f2b 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> > }
> >
> > if (sp->unsync_children)
> > - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
> > + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
> >
> > __clear_sp_write_flooding_count(sp);
>
> FWIW,
>
> Reviewed-by: Vitaly Kuznetsov <[email protected]>
>
> but it'd be great to hear from Sean).

I got nothing, AFAICT I was simply overzealous.

Reviewed-by: Sean Christopherson <[email protected]>

2020-09-03 15:25:21

by Lai Jiangshan

[permalink] [raw]
Subject: [PATCH V3] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed

From: Lai Jiangshan <[email protected]>

When kvm_mmu_get_page() gets a page with unsynced children, the spt
pagetable is unsynchronized with the guest pagetable. But the
guest might not issue a "flush" operation on it when the pagetable
entry is changed from zero or other cases. The hypervisor has the
responsibility to synchronize the pagetables.

The linux kernel behaves correctly as above for many years, but a recent
commit 8c8560b83390 ("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for
MMU specific flushes") inadvertently included a line of code to change it
without giving any reason in the changelog. It is clear that the commit's
intention was to change KVM_REQ_TLB_FLUSH -> KVM_REQ_TLB_FLUSH_CURRENT,
so we don't unneedlesly flush other contexts but one of the hunks changed
nearby KVM_REQ_MMU_SYNC instead.

This patch changes it back.

Fixes: 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes)
Link: https://lore.kernel.org/lkml/[email protected]/
Reviewed-by: Sean Christopherson <[email protected]>
Reviewed-by: Vitaly Kuznetsov <[email protected]>
Signed-off-by: Lai Jiangshan <[email protected]>
---
Changed from v1:
update patch description

Changed form v2:
update patch description

arch/x86/kvm/mmu/mmu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 4e03841f053d..9a93de921f2b 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
}

if (sp->unsync_children)
- kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
+ kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);

__clear_sp_write_flooding_count(sp);

--
2.19.1.6.gb485710b

2020-09-10 10:22:52

by Lai Jiangshan

[permalink] [raw]
Subject: Re: [PATCH V3] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed

Hi, Paolo

Could you pick the patch please?

I think it'd be better to be merged before v5.9 is released
since it fixes a flaw.

Thanks
Lai

On Thu, Sep 3, 2020 at 11:22 PM Lai Jiangshan <[email protected]> wrote:
>
> From: Lai Jiangshan <[email protected]>
>
> When kvm_mmu_get_page() gets a page with unsynced children, the spt
> pagetable is unsynchronized with the guest pagetable. But the
> guest might not issue a "flush" operation on it when the pagetable
> entry is changed from zero or other cases. The hypervisor has the
> responsibility to synchronize the pagetables.
>
> The linux kernel behaves correctly as above for many years, but a recent
> commit 8c8560b83390 ("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for
> MMU specific flushes") inadvertently included a line of code to change it
> without giving any reason in the changelog. It is clear that the commit's
> intention was to change KVM_REQ_TLB_FLUSH -> KVM_REQ_TLB_FLUSH_CURRENT,
> so we don't unneedlesly flush other contexts but one of the hunks changed
> nearby KVM_REQ_MMU_SYNC instead.
>
> This patch changes it back.
>
> Fixes: 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes)
> Link: https://lore.kernel.org/lkml/[email protected]/
> Reviewed-by: Sean Christopherson <[email protected]>
> Reviewed-by: Vitaly Kuznetsov <[email protected]>
> Signed-off-by: Lai Jiangshan <[email protected]>
> ---
> Changed from v1:
> update patch description
>
> Changed form v2:
> update patch description
>
> arch/x86/kvm/mmu/mmu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 4e03841f053d..9a93de921f2b 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> }
>
> if (sp->unsync_children)
> - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
> + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
>
> __clear_sp_write_flooding_count(sp);
>
> --
> 2.19.1.6.gb485710b
>

2020-09-11 17:20:06

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH V2] kvm x86/mmu: use KVM_REQ_MMU_SYNC to sync when needed

On 02/09/20 15:54, Lai Jiangshan wrote:
> From: Lai Jiangshan <[email protected]>
>
> When kvm_mmu_get_page() gets a page with unsynced children, the spt
> pagetable is unsynchronized with the guest pagetable. But the
> guest might not issue a "flush" operation on it when the pagetable
> entry is changed from zero or other cases. The hypervisor has the
> responsibility to synchronize the pagetables.
>
> The linux kernel behaves as above for many years, But
> 8c8560b83390("KVM: x86/mmu: Use KVM_REQ_TLB_FLUSH_CURRENT for MMU specific flushes)
> inadvertently included a line of code to change it without giving any
> reason in the changelog. It is clear that the commit's intention was to
> change KVM_REQ_TLB_FLUSH -> KVM_REQ_TLB_FLUSH_CURRENT, so we don't
> unneedlesly flush other contexts but one of the hunks changed
> nearby KVM_REQ_MMU_SYNC instead.
>
> The this patch changes it back.
>
> Link: https://lore.kernel.org/lkml/[email protected]/
> Cc: Sean Christopherson <[email protected]>
> Cc: Vitaly Kuznetsov <[email protected]>
> Signed-off-by: Lai Jiangshan <[email protected]>
> ---
> Changed from v1:
> update patch description
>
> arch/x86/kvm/mmu/mmu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 4e03841f053d..9a93de921f2b 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -2468,7 +2468,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu,
> }
>
> if (sp->unsync_children)
> - kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
> + kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
>
> __clear_sp_write_flooding_count(sp);
>
>

Queued, thanks.

Paolo