Re: [PATCH 2/2] KVM: arm64: Skip the cache flush when coalescing tables into a block
On 2021/3/9 16:43, Marc Zyngier wrote:
On Tue, 09 Mar 2021 08:34:43 +,
"wangyanan (Y)" wrote:
On 2021/3/9 0:34, Will Deacon wrote:
On Mon, Jan 25, 2021 at 10:10:44PM +0800, Yanan Wang wrote:
After dirty-logging is stopped for a VM configured with huge mappings,
KVM will recover the table mappings back to block mappings. As we only
replace the existing page tables with a block entry and the cacheability
has not been changed, the cache maintenance opreations can be skipped.
Signed-off-by: Yanan Wang
---
arch/arm64/kvm/mmu.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 8e8549ea1d70..37b427dcbc4f 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -744,7 +744,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
phys_addr_t fault_ipa,
{
int ret = 0;
bool write_fault, writable, force_pte = false;
- bool exec_fault;
+ bool exec_fault, adjust_hugepage;
bool device = false;
unsigned long mmu_seq;
struct kvm *kvm = vcpu->kvm;
@@ -872,12 +872,18 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
phys_addr_t fault_ipa,
mark_page_dirty(kvm, gfn);
}
-if (fault_status != FSC_PERM && !device)
+ /*
+* There is no necessity to perform cache maintenance operations if we
+* will only replace the existing table mappings with a block mapping.
+*/
+ adjust_hugepage = fault_granule < vma_pagesize ? true : false;
nit: you don't need the '? true : false' part
That said, your previous patch checks for 'fault_granule > vma_pagesize',
so I'm not sure the local variable helps all that much here because it
obscures the size checks in my opinion. It would be more straight-forward
if we could structure the logic as:
if (fault_granule < vma_pagesize) {
} else if (fault_granule > vma_page_size) {
} else {
}
With some comments describing what we can infer about the memcache and cache
maintenance requirements for each case.
Thanks for your suggestion here, Will.
But I have resent another newer series [1] (KVM: arm64: Improve
efficiency of stage2 page table)
recently, which has the same theme but different solutions that I
think are better.
[1]
https://lore.kernel.org/lkml/20210208112250.163568-1-wangyana...@huawei.com/
Could you please comment on that series ? I think it can be found in
your inbox :).
There were already a bunch of comments on that series, and I stopped
at the point where the cache maintenance was broken. Please respin
that series if you want further feedback on it.
Ok, I will send a new version later.
In the future, if you deprecate a series (which is completely
understandable), please leave a note on the list with a pointer to the
new series so that people don't waste time reviewing an obsolete
series. Or post the new series with a new version number so that it is
obvious that the original series has been superseded.
I apologize for this, I will be more careful in the future.
Thanks,
Yanan
Thanks,
M.
Re: [PATCH 2/2] KVM: arm64: Skip the cache flush when coalescing tables into a block
On Tue, 09 Mar 2021 08:34:43 +,
"wangyanan (Y)" wrote:
>
>
> On 2021/3/9 0:34, Will Deacon wrote:
> > On Mon, Jan 25, 2021 at 10:10:44PM +0800, Yanan Wang wrote:
> >> After dirty-logging is stopped for a VM configured with huge mappings,
> >> KVM will recover the table mappings back to block mappings. As we only
> >> replace the existing page tables with a block entry and the cacheability
> >> has not been changed, the cache maintenance opreations can be skipped.
> >>
> >> Signed-off-by: Yanan Wang
> >> ---
> >> arch/arm64/kvm/mmu.c | 12 +---
> >> 1 file changed, 9 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> >> index 8e8549ea1d70..37b427dcbc4f 100644
> >> --- a/arch/arm64/kvm/mmu.c
> >> +++ b/arch/arm64/kvm/mmu.c
> >> @@ -744,7 +744,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
> >> phys_addr_t fault_ipa,
> >> {
> >>int ret = 0;
> >>bool write_fault, writable, force_pte = false;
> >> - bool exec_fault;
> >> + bool exec_fault, adjust_hugepage;
> >>bool device = false;
> >>unsigned long mmu_seq;
> >>struct kvm *kvm = vcpu->kvm;
> >> @@ -872,12 +872,18 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
> >> phys_addr_t fault_ipa,
> >>mark_page_dirty(kvm, gfn);
> >>}
> >> -if (fault_status != FSC_PERM && !device)
> >> + /*
> >> + * There is no necessity to perform cache maintenance operations if we
> >> + * will only replace the existing table mappings with a block mapping.
> >> + */
> >> + adjust_hugepage = fault_granule < vma_pagesize ? true : false;
> > nit: you don't need the '? true : false' part
> >
> > That said, your previous patch checks for 'fault_granule > vma_pagesize',
> > so I'm not sure the local variable helps all that much here because it
> > obscures the size checks in my opinion. It would be more straight-forward
> > if we could structure the logic as:
> >
> >
> > if (fault_granule < vma_pagesize) {
> >
> > } else if (fault_granule > vma_page_size) {
> >
> > } else {
> >
> > }
> >
> > With some comments describing what we can infer about the memcache and cache
> > maintenance requirements for each case.
> Thanks for your suggestion here, Will.
> But I have resent another newer series [1] (KVM: arm64: Improve
> efficiency of stage2 page table)
> recently, which has the same theme but different solutions that I
> think are better.
> [1]
> https://lore.kernel.org/lkml/20210208112250.163568-1-wangyana...@huawei.com/
>
> Could you please comment on that series ? I think it can be found in
> your inbox :).
There were already a bunch of comments on that series, and I stopped
at the point where the cache maintenance was broken. Please respin
that series if you want further feedback on it.
In the future, if you deprecate a series (which is completely
understandable), please leave a note on the list with a pointer to the
new series so that people don't waste time reviewing an obsolete
series. Or post the new series with a new version number so that it is
obvious that the original series has been superseded.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
Re: [PATCH 2/2] KVM: arm64: Skip the cache flush when coalescing tables into a block
On 2021/3/9 0:34, Will Deacon wrote:
On Mon, Jan 25, 2021 at 10:10:44PM +0800, Yanan Wang wrote:
After dirty-logging is stopped for a VM configured with huge mappings,
KVM will recover the table mappings back to block mappings. As we only
replace the existing page tables with a block entry and the cacheability
has not been changed, the cache maintenance opreations can be skipped.
Signed-off-by: Yanan Wang
---
arch/arm64/kvm/mmu.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 8e8549ea1d70..37b427dcbc4f 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -744,7 +744,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
phys_addr_t fault_ipa,
{
int ret = 0;
bool write_fault, writable, force_pte = false;
- bool exec_fault;
+ bool exec_fault, adjust_hugepage;
bool device = false;
unsigned long mmu_seq;
struct kvm *kvm = vcpu->kvm;
@@ -872,12 +872,18 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
phys_addr_t fault_ipa,
mark_page_dirty(kvm, gfn);
}
- if (fault_status != FSC_PERM && !device)
+ /*
+* There is no necessity to perform cache maintenance operations if we
+* will only replace the existing table mappings with a block mapping.
+*/
+ adjust_hugepage = fault_granule < vma_pagesize ? true : false;
nit: you don't need the '? true : false' part
That said, your previous patch checks for 'fault_granule > vma_pagesize',
so I'm not sure the local variable helps all that much here because it
obscures the size checks in my opinion. It would be more straight-forward
if we could structure the logic as:
if (fault_granule < vma_pagesize) {
} else if (fault_granule > vma_page_size) {
} else {
}
With some comments describing what we can infer about the memcache and cache
maintenance requirements for each case.
Thanks for your suggestion here, Will.
But I have resent another newer series [1] (KVM: arm64: Improve
efficiency of stage2 page table)
recently, which has the same theme but different solutions that I think
are better.
[1]
https://lore.kernel.org/lkml/20210208112250.163568-1-wangyana...@huawei.com/
Could you please comment on that series ? I think it can be found in
your inbox :).
Thanks,
Yanan
Will
.
Re: [PATCH 2/2] KVM: arm64: Skip the cache flush when coalescing tables into a block
On Mon, Jan 25, 2021 at 10:10:44PM +0800, Yanan Wang wrote:
> After dirty-logging is stopped for a VM configured with huge mappings,
> KVM will recover the table mappings back to block mappings. As we only
> replace the existing page tables with a block entry and the cacheability
> has not been changed, the cache maintenance opreations can be skipped.
>
> Signed-off-by: Yanan Wang
> ---
> arch/arm64/kvm/mmu.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 8e8549ea1d70..37b427dcbc4f 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -744,7 +744,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
> phys_addr_t fault_ipa,
> {
> int ret = 0;
> bool write_fault, writable, force_pte = false;
> - bool exec_fault;
> + bool exec_fault, adjust_hugepage;
> bool device = false;
> unsigned long mmu_seq;
> struct kvm *kvm = vcpu->kvm;
> @@ -872,12 +872,18 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
> phys_addr_t fault_ipa,
> mark_page_dirty(kvm, gfn);
> }
>
> - if (fault_status != FSC_PERM && !device)
> + /*
> + * There is no necessity to perform cache maintenance operations if we
> + * will only replace the existing table mappings with a block mapping.
> + */
> + adjust_hugepage = fault_granule < vma_pagesize ? true : false;
nit: you don't need the '? true : false' part
That said, your previous patch checks for 'fault_granule > vma_pagesize',
so I'm not sure the local variable helps all that much here because it
obscures the size checks in my opinion. It would be more straight-forward
if we could structure the logic as:
if (fault_granule < vma_pagesize) {
} else if (fault_granule > vma_page_size) {
} else {
}
With some comments describing what we can infer about the memcache and cache
maintenance requirements for each case.
Will
[PATCH 2/2] KVM: arm64: Skip the cache flush when coalescing tables into a block
After dirty-logging is stopped for a VM configured with huge mappings,
KVM will recover the table mappings back to block mappings. As we only
replace the existing page tables with a block entry and the cacheability
has not been changed, the cache maintenance opreations can be skipped.
Signed-off-by: Yanan Wang
---
arch/arm64/kvm/mmu.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 8e8549ea1d70..37b427dcbc4f 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -744,7 +744,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
phys_addr_t fault_ipa,
{
int ret = 0;
bool write_fault, writable, force_pte = false;
- bool exec_fault;
+ bool exec_fault, adjust_hugepage;
bool device = false;
unsigned long mmu_seq;
struct kvm *kvm = vcpu->kvm;
@@ -872,12 +872,18 @@ static int user_mem_abort(struct kvm_vcpu *vcpu,
phys_addr_t fault_ipa,
mark_page_dirty(kvm, gfn);
}
- if (fault_status != FSC_PERM && !device)
+ /*
+* There is no necessity to perform cache maintenance operations if we
+* will only replace the existing table mappings with a block mapping.
+*/
+ adjust_hugepage = fault_granule < vma_pagesize ? true : false;
+ if (fault_status != FSC_PERM && !device && !adjust_hugepage)
clean_dcache_guest_page(pfn, vma_pagesize);
if (exec_fault) {
prot |= KVM_PGTABLE_PROT_X;
- invalidate_icache_guest_page(pfn, vma_pagesize);
+ if (!adjust_hugepage)
+ invalidate_icache_guest_page(pfn, vma_pagesize);
}
if (device)
--
2.19.1
