Re: [PATCH v7 02/10] KVM: MMU: abstract spte write-protect
On 06/21/2012 04:48 AM, Xiao Guangrong wrote: On 06/20/2012 10:11 PM, Takuya Yoshikawa wrote: We can change the debug message later if needed. Actually, i am going to use tracepoint instead of these debug code. Yes, these should be in the kvmmmu namespace. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 02/10] KVM: MMU: abstract spte write-protect
On Wed, 20 Jun 2012 15:57:15 +0800
Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com wrote:
Introduce a common function to abstract spte write-protect to
cleanup the code
Signed-off-by: Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com
...
+/* Return true if the spte is dropped. */
+static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool *flush)
+{
+ u64 spte = *sptep;
+
+ if (!is_writable_pte(spte))
+ return false;
+
+ rmap_printk(rmap_write_protect: spte %p %llx\n, sptep, *sptep);
...
@@ -3902,16 +3915,7 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm,
int slot)
!is_last_spte(pt[i], sp-role.level))
continue;
- if (is_large_pte(pt[i])) {
- drop_spte(kvm, pt[i]);
- --kvm-stat.lpages;
- continue;
- }
-
- /* avoid RMW */
- if (is_writable_pte(pt[i]))
- mmu_spte_update(pt[i],
- pt[i] ~PT_WRITABLE_MASK);
+ spte_write_protect(kvm, pt[i], flush);
Adding rmap_printk() here seems wrong.
If you think it is not a problem, please explain why you think so in
the changelog.
Thanks,
Takuya
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 02/10] KVM: MMU: abstract spte write-protect
On 06/20/2012 05:02 PM, Takuya Yoshikawa wrote:
On Wed, 20 Jun 2012 15:57:15 +0800
Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com wrote:
Introduce a common function to abstract spte write-protect to
cleanup the code
Signed-off-by: Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com
...
+/* Return true if the spte is dropped. */
+static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool *flush)
+{
+u64 spte = *sptep;
+
+if (!is_writable_pte(spte))
+return false;
+
+rmap_printk(rmap_write_protect: spte %p %llx\n, sptep, *sptep);
...
@@ -3902,16 +3915,7 @@ void kvm_mmu_slot_remove_write_access(struct kvm
*kvm, int slot)
!is_last_spte(pt[i], sp-role.level))
continue;
-if (is_large_pte(pt[i])) {
-drop_spte(kvm, pt[i]);
---kvm-stat.lpages;
-continue;
-}
-
-/* avoid RMW */
-if (is_writable_pte(pt[i]))
-mmu_spte_update(pt[i],
-pt[i] ~PT_WRITABLE_MASK);
+spte_write_protect(kvm, pt[i], flush);
Adding rmap_printk() here seems wrong.
Strange! Why do you think it is wrong? It is just debug code.
If you think it is not a problem, please explain why you think so in
the changelog.
It is a from the first place and it is used to debug and not compiled at all.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 02/10] KVM: MMU: abstract spte write-protect
On Wed, 20 Jun 2012 17:11:06 +0800 Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com wrote: Strange! Why do you think it is wrong? It is just debug code. kvm_mmu_slot_remove_write_access() does not use rmap but the debug code says: rmap_printk(rmap_write_protect: spte %p %llx\n, sptep, *sptep); If you think it is not a problem, please explain why you think so in the changelog. It is a from the first place and it is used to debug and not compiled at all. It was not in kvm_mmu_slot_remove_write_access() before, no? This patch says that the write protection code becomes commonly usable function, but it still has rmap_write_protect specific debug code in it; using it in kvm_mmu_slot_remove_write_access(), which is not at all related to rmap_write_protect, is strange. As you say, this is just debug code and does not have any practical problem. But randomly putting debug code is not a good thing. Thanks, Takuya -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 02/10] KVM: MMU: abstract spte write-protect
On 06/20/2012 08:56 PM, Takuya Yoshikawa wrote: On Wed, 20 Jun 2012 17:11:06 +0800 Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com wrote: Strange! Why do you think it is wrong? It is just debug code. kvm_mmu_slot_remove_write_access() does not use rmap but the debug code says: rmap_printk(rmap_write_protect: spte %p %llx\n, sptep, *sptep); It is not a problem since all sptes which are pointing to gfn is existed in rmap. If you think it is not a problem, please explain why you think so in the changelog. It is a from the first place and it is used to debug and not compiled at all. It was not in kvm_mmu_slot_remove_write_access() before, no? This patch says that the write protection code becomes commonly usable function, but it still has rmap_write_protect specific debug code in it; using it in kvm_mmu_slot_remove_write_access(), which is not at all related to rmap_write_protect, is strange. As you say, this is just debug code and does not have any practical problem. But randomly putting debug code is not a good thing. Again, rmap does not break the logic, the spte we handle in this function must be in rmap. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 02/10] KVM: MMU: abstract spte write-protect
On Wed, 20 Jun 2012 21:21:07 +0800 Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com wrote: Again, rmap does not break the logic, the spte we handle in this function must be in rmap. I'm not saying whether this breaks some logic or not. rmap_printk(rmap_write_protect: spte %p %llx\n, sptep, *sptep); helps developers by saying rmap_write_protect() protected For another function, the message should be another_func_name: spte ... But OK, I won't argue about this anymore. We can change the debug message later if needed. Thanks, Takuya -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 02/10] KVM: MMU: abstract spte write-protect
On 06/20/2012 10:11 PM, Takuya Yoshikawa wrote: We can change the debug message later if needed. Actually, i am going to use tracepoint instead of these debug code. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 02/10] KVM: MMU: abstract spte write-protect
On Thu, 21 Jun 2012 09:48:05 +0800 Xiao Guangrong xiaoguangr...@linux.vnet.ibm.com wrote: We can change the debug message later if needed. Actually, i am going to use tracepoint instead of these debug code. That's very nice! Then, please change the trace log to correspond to the new spte_write_protect name at that time. Thank you, Xiao. Takuya -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
