KVM: dirty all pages in kvm_write_guest_cached()
Subsystems (2)
PATCH 1/1 KVM: dirty all pages in kvm_write_guest_cached()
We dirtied only one page because writes originally couldn't span more. Use improved syntax for '>> PAGE_SHIFT' while at it.
---
The function handles cross memslot writes in a different path.
I think we should dirty pages after partial writes too (r < len), but it probably won't happen and I already started refactoring :)
virt/kvm/kvm_main.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
virt/kvm/kvm_main.c +5 -1
@@ -1665,6 +1665,7 @@ int kvm_write_guest_cached(struct kvm *kvm, struct gfn_to_hva_cache *ghc, {struct kvm_memslots *slots =kvm_memslots (kvm);int r; +gfn_t gfn;BUG_ON (len > ghc->len);
@@ -1680,7 +1681,10 @@ int kvm_write_guest_cached(struct kvm *kvm, struct gfn_to_hva_cache *ghc, r =__copy_to_user ((void __user *)ghc->hva, data, len);if (r)return -EFAULT ; -mark_page_dirty_in_slot (kvm, ghc->memslot, ghc->gpa >>PAGE_SHIFT ); + +for (gfn =gpa_to_gfn (ghc->gpa); + gfn <=gpa_to_gfn (ghc->gpa + len -1 ); gfn++) +mark_page_dirty_in_slot (kvm, ghc->memslot, gfn);return 0 ; }
Signature · 1 line
2.3.4
Paolo Bonzini
maintainerQuoted 5 lines · Radim Krčmář
We dirtied only one page because writes originally couldn't span more. Use improved syntax for '>>
PAGE_SHIFT' while at it.Fixes 8f964525a121 ("KVM: Allow cross page reads and writes from cached translations.")Signed-off-by Radim Krčmář <[email protected]>
Cross-page reads and writes should never get here; they have ghc->memslot set to NULL and go through the slow path in kvm_write_guest.
What am I missing?
Paolo
Quoted 34 lines
---
The function handles cross memslot writes in a different path.I think we should dirty pages after partial writes too (r < len), but it probably won't happen and I already started refactoring :)
virt/kvm/kvm_main.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)virt/kvm/kvm_main.c +5
-1@@ -1665,6 +1665,7 @@ int kvm_write_guest_cached(struct kvm *kvm, struct gfn_to_hva_cache *ghc, {struct kvm_memslots *slots =kvm_memslots (kvm);int r; +gfn_t gfn;BUG_ON (len > ghc->len);@@ -1680,7 +1681,10 @@ int kvm_write_guest_cached(struct kvm *kvm, struct gfn_to_hva_cache *ghc, r =__copy_to_user ((void __user *)ghc->hva, data, len);if (r)return -EFAULT ; -mark_page_dirty_in_slot (kvm, ghc->memslot, ghc->gpa >>PAGE_SHIFT ); + +for (gfn =gpa_to_gfn (ghc->gpa); + gfn <=gpa_to_gfn (ghc->gpa + len -1 ); gfn++) +mark_page_dirty_in_slot (kvm, ghc->memslot, gfn);return 0 ; }
2015-04-08 10:49+0200, Paolo Bonzini:
Quoted 9 lines · Radim Krčmář
Quoted 5 lines
We dirtied only one page because writes originally couldn't span more. Use improved syntax for '>>
PAGE_SHIFT' while at it.Fixes 8f964525a121 ("KVM: Allow cross page reads and writes from cached translations.")Signed-off-by Radim Krčmář <[email protected]>Cross-page reads and writes should never get here; they have
ghc->memslotset to NULL and go through the slow path inkvm_write_guest.
Only cross-memslot writes have NULL memslot.
Quoted 1 line
What am I missing?
kvm_gfn_to_hva_cache_init() queries how many pages are remaining in the memslot and it compares it with the amount of needed pages. If the write will fit in memslot, it will be done without kvm_write_guest, regardless of the amount of written pages.
The relevant code path in kvm_gfn_to_hva_cache_init():
gfn_t nr_pages_needed = end_gfn - start_gfn + 1; ghc->memslot = gfn_to_memslot(kvm, start_gfn);
ghc->hva = gfn_to_hva_many(ghc->memslot, start_gfn, &nr_pages_avail); if (!kvm_is_error_hva(ghc->hva) && nr_pages_avail >= nr_pages_needed)
ghc->hva += offset;
return 0;
Paolo Bonzini
maintainerQuoted 5 lines · Radim Krčmář
2015-04-08 10:49+0200, Paolo Bonzini:
Quoted 4 lines · Radim Krčmář
Quoted 3 lines
We dirtied only one page because writes originally couldn't span more. Use improved syntax for '>>
PAGE_SHIFT' while at it.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Fixes 8f964525a121 ("KVM: Allow cross page reads and writes from cached translations.")Signed-off-by Radim Krčmář <[email protected]>Cross-page reads and writes should never get here; they have
ghc->memslotset to NULL and go through the slow path inkvm_write_guest.Only cross-memslot writes have NULL memslot.
The power of wrong comments...
Considering how kvm_gfn_to_hva_cache_init is used (one 1-byte field, two 4-byte fields, one 28-bytes struct that is 32-bytes aligned, one 32-bytes field that is in practice cacheline-aligned), I wonder if we should just use ghc->memslot = NULL for cross page writes. This would bypass the bug you are fixing here, and avoid worries about partial writes.
Paolo
PATCH 1/1 KVM: use slowpath for cross page cached accesses
2015-04-08 12:43+0200, Paolo Bonzini:
Quoted 14 lines · Radim Krčmář
Quoted 5 lines
2015-04-08 10:49+0200, Paolo Bonzini:
Quoted 4 lines · Radim Krčmář
Quoted 3 lines
We dirtied only one page because writes originally couldn't span more. Use improved syntax for '>>
PAGE_SHIFT' while at it.Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Fixes 8f964525a121 ("KVM: Allow cross page reads and writes from cached translations.")Signed-off-by Radim Krčmář <[email protected]>Cross-page reads and writes should never get here; they have
ghc->memslotset to NULL and go through the slow path inkvm_write_guest.Only cross-memslot writes have NULL memslot.
Quoted 7 lines
The power of wrong comments...
Considering how
kvm_gfn_to_hva_cache_initis used (one 1-byte field, two 4-byte fields, one 28-bytes struct that is 32-bytes aligned, one 32-bytes field that is in practice cacheline-aligned), I wonder if we should just useghc->memslot= NULL for cross page writes. This would bypass the bug you are fixing here, and avoid worries about partial writes.
Good idea, and it could make those comments right :) (Though in general, I prefer less constraints on APIs ...)
Partial writes would be a pain; copy_to_user API does not define which bytes were not written. I think the write can't fail mid-page, which makes our implementation ok, but I still worry a bit about it.
Anyway, here's the patch:
---8<---kvm_write_guest_cached() does not mark all written pages as dirty and code comments in kvm_gfn_to_hva_cache_init() talk about NULL memslot with cross page accesses. Fix all the easy way.
The check is '<= 1' to have the same result for 'len = 0' cache anywhere in the page. (nr_pages_needed is 0 on page boundary.)
--- virt/kvm/kvm_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
virt/kvm/kvm_main.c +2 -2
@@ -1637,8 +1637,8 @@ int kvm_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache *ghc, ghc->generation = slots->generation; ghc->len = len; ghc->memslot =gfn_to_memslot (kvm, start_gfn); - ghc->hva =gfn_to_hva_many (ghc->memslot, start_gfn, &nr_pages_avail); -if (!kvm_is_error_hva (ghc->hva) && nr_pages_avail >= nr_pages_needed) { + ghc->hva =gfn_to_hva_many (ghc->memslot, start_gfn,NULL ); +if (!kvm_is_error_hva (ghc->hva) && nr_pages_needed <=1 ) { ghc->hva += offset; }else {/*
Paolo Bonzini
maintainerRe: [PATCH] KVM: use slowpath for cross page cached accesses
Quoted 6 lines · Radim Krčmář
2015-04-08 12:43+0200, Paolo Bonzini:
Quoted 5 lines · Radim Krčmář
Quoted 1 line
Only cross-memslot writes have NULL memslot.
The power of wrong comments...
Quoted 8 lines
Quoted 5 lines
Considering how
kvm_gfn_to_hva_cache_initis used (one 1-byte field, two 4-byte fields, one 28-bytes struct that is 32-bytes aligned, one 32-bytes field that is in practice cacheline-aligned), I wonder if we should just useghc->memslot= NULL for cross page writes. This would bypass the bug you are fixing here, and avoid worries about partial writes.Good idea, and it could make those comments right :) (Though in general, I prefer less constraints on APIs ...)
It doesn't put constraints, it still handles cross page writes right (just slower). copy_to_user in some sense is the API that constrains us to do this.
Quoted 3 lines
Partial writes would be a pain;
copy_to_userAPI does not define which bytes were not written. I think the write can't fail mid-page, which makes our implementation ok
No, writes can't fail mid-page (I guess in atomic context it's theoretically possible, but we're equipped to handle the failure in that case).
Patch applied, thanks!
Paolo
Re: [PATCH] KVM: use slowpath for cross page cached accesses
In reply to Radim KrčmářQuoted 37 lines · Radim Krčmář
2015-04-08 12:43+0200, Paolo Bonzini:
Quoted 14 lines · Radim Krčmář
Quoted 5 lines
2015-04-08 10:49+0200, Paolo Bonzini:
Quoted 4 lines · Radim Krčmář
Quoted 3 lines
We dirtied only one page because writes originally couldn't span more. Use improved syntax for '>>
PAGE_SHIFT' while at it.Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Fixes 8f964525a121 ("KVM: Allow cross page reads and writes from cached translations.")Signed-off-by Radim Krčmář <[email protected]>Cross-page reads and writes should never get here; they have
ghc->memslotset to NULL and go through the slow path inkvm_write_guest.Only cross-memslot writes have NULL memslot.
Quoted 7 lines
The power of wrong comments...
Considering how
kvm_gfn_to_hva_cache_initis used (one 1-byte field, two 4-byte fields, one 28-bytes struct that is 32-bytes aligned, one 32-bytes field that is in practice cacheline-aligned), I wonder if we should just useghc->memslot= NULL for cross page writes. This would bypass the bug you are fixing here, and avoid worries about partial writes.Good idea, and it could make those comments right :) (Though in general, I prefer less constraints on APIs ...)
Partial writes would be a pain;
copy_to_userAPI does not define which bytes were not written. I think the write can't fail mid-page, which makes our implementation ok, but I still worry a bit about it.Anyway, here's the patch:
---8<---
kvm_write_guest_cached()does not mark all written pages as dirty and code comments inkvm_gfn_to_hva_cache_init()talk about NULL memslot with cross page accesses. Fix all the easy way.
Quoted 5 lines
The check is '<= 1' to have the same result for 'len = 0' cache anywhere in the page. (
nr_pages_neededis 0 on page boundary.)Fixes 8f964525a121 ("KVM: Allow cross page reads and writes from cached translations.")Signed-off-by Radim Krčmář <[email protected]>
Quoted 24 lines
---
virt/kvm/kvm_main.c| 4 ++--1 file changed, 2 insertions(+), 2 deletions(-)virt/kvm/kvm_main.c +2
-3@@ -1637,8 +1637,8 @@ int kvm_gfn_to_hva_cache_init(struct kvm *kvm, struct gfn_to_hva_cache *ghc, ghc->generation = slots->generation; ghc->len = len; ghc->memslot =gfn_to_memslot (kvm, start_gfn); - ghc->hva =gfn_to_hva_many (ghc->memslot, start_gfn, &nr_pages_avail); -if (!kvm_is_error_hva (ghc->hva) && nr_pages_avail >= nr_pages_needed) { + ghc->hva =gfn_to_hva_many (ghc->memslot, start_gfn,NULL ); +if (!kvm_is_error_hva (ghc->hva) && nr_pages_needed <=1 ) { ghc->hva += offset; }else {/* -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Patch "KVM: use slowpath for cross page cached accesses" has been added to the 4.0-stable tree
In reply to Radim KrčmářThis is a note to let you know that I've just added the patch titled
KVM: use slowpath for cross page cached accesses
to the 4.0-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
kvm-use-slowpath-for-cross-page-cached-accesses.patch
and it can be found in the queue-4.0 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.
Quoted 1 line
From
ca3f0874723fad81d0c701b63ae3a17a408d5f25Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Radim=20Kr=C4=8Dm=C3=A1=C5=99?= <[email protected]>
Date: Wed, 8 Apr 2015 14:16:48 +0200
Subject: KVM: use slowpath for cross page cached accesses
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
From: =?UTF-8?q?Radim=20Kr=C4=8Dm=C3=A1=C5=99?= <[email protected]>
commit ca3f0874723fad81d0c701b63ae3a17a408d5f25 upstream.
kvm_write_guest_cached() does not mark all written pages as dirty and code comments in kvm_gfn_to_hva_cache_init() talk about NULL memslot with cross page accesses. Fix all the easy way.
The check is '<= 1' to have the same result for 'len = 0' cache anywhere in the page. (nr_pages_needed is 0 on page boundary.)
Message-Id: <[email protected]>
--- virt/kvm/kvm_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
virt/kvm/kvm_main.c +2 -2
@@ -1653,8 +1653,8 @@ int kvm_gfn_to_hva_cache_init(struct kvm ghc->generation = slots->generation; ghc->len = len; ghc->memslot =gfn_to_memslot (kvm, start_gfn); - ghc->hva =gfn_to_hva_many (ghc->memslot, start_gfn, &nr_pages_avail); -if (!kvm_is_error_hva (ghc->hva) && nr_pages_avail >= nr_pages_needed) { + ghc->hva =gfn_to_hva_many (ghc->memslot, start_gfn,NULL ); +if (!kvm_is_error_hva (ghc->hva) && nr_pages_needed <=1 ) { ghc->hva += offset; }else {/*
Patches currently in stable-queue which might be from [email protected] are
queue-4.0/kvm-use-slowpath-for-cross-page-cached-accesses.patch queue-4.0/x86-vdso-fix-pvclock-races-with-task-migration.patch
Patch "KVM: use slowpath for cross page cached accesses" has been added to the 3.19-stable tree
In reply to Radim KrčmářThis is a note to let you know that I've just added the patch titled
KVM: use slowpath for cross page cached accesses
to the 3.19-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
kvm-use-slowpath-for-cross-page-cached-accesses.patch
and it can be found in the queue-3.19 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.
Quoted 1 line
From
ca3f0874723fad81d0c701b63ae3a17a408d5f25Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Radim=20Kr=C4=8Dm=C3=A1=C5=99?= <[email protected]>
Date: Wed, 8 Apr 2015 14:16:48 +0200
Subject: KVM: use slowpath for cross page cached accesses
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
From: =?UTF-8?q?Radim=20Kr=C4=8Dm=C3=A1=C5=99?= <[email protected]>
commit ca3f0874723fad81d0c701b63ae3a17a408d5f25 upstream.
kvm_write_guest_cached() does not mark all written pages as dirty and code comments in kvm_gfn_to_hva_cache_init() talk about NULL memslot with cross page accesses. Fix all the easy way.
The check is '<= 1' to have the same result for 'len = 0' cache anywhere in the page. (nr_pages_needed is 0 on page boundary.)
Message-Id: <[email protected]>
--- virt/kvm/kvm_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
virt/kvm/kvm_main.c +2 -2
@@ -1608,8 +1608,8 @@ int kvm_gfn_to_hva_cache_init(struct kvm ghc->generation = slots->generation; ghc->len = len; ghc->memslot =gfn_to_memslot (kvm, start_gfn); - ghc->hva =gfn_to_hva_many (ghc->memslot, start_gfn, &nr_pages_avail); -if (!kvm_is_error_hva (ghc->hva) && nr_pages_avail >= nr_pages_needed) { + ghc->hva =gfn_to_hva_many (ghc->memslot, start_gfn,NULL ); +if (!kvm_is_error_hva (ghc->hva) && nr_pages_needed <=1 ) { ghc->hva += offset; }else {/*
Patches currently in stable-queue which might be from [email protected] are
queue-3.19/kvm-use-slowpath-for-cross-page-cached-accesses.patch
Patch "KVM: use slowpath for cross page cached accesses" has been added to the 3.10-stable tree
In reply to Radim KrčmářThis is a note to let you know that I've just added the patch titled
KVM: use slowpath for cross page cached accesses
to the 3.10-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
kvm-use-slowpath-for-cross-page-cached-accesses.patch
and it can be found in the queue-3.10 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.
Quoted 1 line
From
ca3f0874723fad81d0c701b63ae3a17a408d5f25Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Radim=20Kr=C4=8Dm=C3=A1=C5=99?= <[email protected]>
Date: Wed, 8 Apr 2015 14:16:48 +0200
Subject: KVM: use slowpath for cross page cached accesses
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
From: =?UTF-8?q?Radim=20Kr=C4=8Dm=C3=A1=C5=99?= <[email protected]>
commit ca3f0874723fad81d0c701b63ae3a17a408d5f25 upstream.
kvm_write_guest_cached() does not mark all written pages as dirty and code comments in kvm_gfn_to_hva_cache_init() talk about NULL memslot with cross page accesses. Fix all the easy way.
The check is '<= 1' to have the same result for 'len = 0' cache anywhere in the page. (nr_pages_needed is 0 on page boundary.)
Message-Id: <[email protected]>
--- virt/kvm/kvm_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
virt/kvm/kvm_main.c +2 -2
@@ -1549,8 +1549,8 @@ int kvm_gfn_to_hva_cache_init(struct kvm ghc->generation = slots->generation; ghc->len = len; ghc->memslot =gfn_to_memslot (kvm, start_gfn); - ghc->hva =gfn_to_hva_many (ghc->memslot, start_gfn, &nr_pages_avail); -if (!kvm_is_error_hva (ghc->hva) && nr_pages_avail >= nr_pages_needed) { + ghc->hva =gfn_to_hva_many (ghc->memslot, start_gfn,NULL ); +if (!kvm_is_error_hva (ghc->hva) && nr_pages_needed <=1 ) { ghc->hva += offset; }else {/*
Patches currently in stable-queue which might be from [email protected] are
queue-3.10/kvm-use-slowpath-for-cross-page-cached-accesses.patch
Patch "KVM: use slowpath for cross page cached accesses" has been added to the 3.14-stable tree
In reply to Radim KrčmářThis is a note to let you know that I've just added the patch titled
KVM: use slowpath for cross page cached accesses
to the 3.14-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
kvm-use-slowpath-for-cross-page-cached-accesses.patch
and it can be found in the queue-3.14 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.
Quoted 1 line
From
ca3f0874723fad81d0c701b63ae3a17a408d5f25Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Radim=20Kr=C4=8Dm=C3=A1=C5=99?= <[email protected]>
Date: Wed, 8 Apr 2015 14:16:48 +0200
Subject: KVM: use slowpath for cross page cached accesses
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
From: =?UTF-8?q?Radim=20Kr=C4=8Dm=C3=A1=C5=99?= <[email protected]>
commit ca3f0874723fad81d0c701b63ae3a17a408d5f25 upstream.
kvm_write_guest_cached() does not mark all written pages as dirty and code comments in kvm_gfn_to_hva_cache_init() talk about NULL memslot with cross page accesses. Fix all the easy way.
The check is '<= 1' to have the same result for 'len = 0' cache anywhere in the page. (nr_pages_needed is 0 on page boundary.)
Message-Id: <[email protected]>
--- virt/kvm/kvm_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
virt/kvm/kvm_main.c +2 -2
@@ -1549,8 +1549,8 @@ int kvm_gfn_to_hva_cache_init(struct kvm ghc->generation = slots->generation; ghc->len = len; ghc->memslot =gfn_to_memslot (kvm, start_gfn); - ghc->hva =gfn_to_hva_many (ghc->memslot, start_gfn, &nr_pages_avail); -if (!kvm_is_error_hva (ghc->hva) && nr_pages_avail >= nr_pages_needed) { + ghc->hva =gfn_to_hva_many (ghc->memslot, start_gfn,NULL ); +if (!kvm_is_error_hva (ghc->hva) && nr_pages_needed <=1 ) { ghc->hva += offset; }else {/*
Patches currently in stable-queue which might be from [email protected] are
queue-3.14/kvm-use-slowpath-for-cross-page-cached-accesses.patch