* [PATCH 0/2] kvm: IOMMU read-only mapping support
@ 2013-01-24 22:03 Alex Williamson
2013-01-24 22:04 ` [PATCH 1/2] kvm: Force IOMMU remapping on memory slot read-only flag changes Alex Williamson
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Alex Williamson @ 2013-01-24 22:03 UTC (permalink / raw)
To: gleb, kvm; +Cc: yoshikawa_takuya_b1, linux-kernel, xiaoguangrong
A couple patches to make KVM IOMMU support honor read-only mappings.
This causes an un-map, re-map when the read-only flag changes and
makes use of it when setting IOMMU attributes. Thanks,
Alex
---
Alex Williamson (2):
kvm: Force IOMMU remapping on memory slot read-only flag changes
kvm: Obey read-only mappings in iommu
virt/kvm/iommu.c | 4 +++-
virt/kvm/kvm_main.c | 28 ++++++++++++++++++++++++----
2 files changed, 27 insertions(+), 5 deletions(-)
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 1/2] kvm: Force IOMMU remapping on memory slot read-only flag changes 2013-01-24 22:03 [PATCH 0/2] kvm: IOMMU read-only mapping support Alex Williamson @ 2013-01-24 22:04 ` Alex Williamson 2013-01-25 2:58 ` Xiao Guangrong 2013-01-24 22:04 ` [PATCH 2/2] kvm: Obey read-only mappings in iommu Alex Williamson ` (2 subsequent siblings) 3 siblings, 1 reply; 18+ messages in thread From: Alex Williamson @ 2013-01-24 22:04 UTC (permalink / raw) To: gleb, kvm; +Cc: yoshikawa_takuya_b1, linux-kernel, xiaoguangrong Memory slot flags can be altered without changing other parameters of the slot. The read-only attribute is the only one the IOMMU cares about, so generate an un-map, re-map when this occurs. This also avoid unnecessarily re-mapping the slot when no IOMMU visible changes are made. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- virt/kvm/kvm_main.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 5e709eb..3fec2cd 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -731,6 +731,7 @@ int __kvm_set_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot; struct kvm_memory_slot old, new; struct kvm_memslots *slots = NULL, *old_memslots; + bool old_iommu_mapped; r = check_memory_region_flags(mem); if (r) @@ -772,6 +773,8 @@ int __kvm_set_memory_region(struct kvm *kvm, new.npages = npages; new.flags = mem->flags; + old_iommu_mapped = old.npages; + /* * Disallow changing a memory slot's size or changing anything about * zero sized slots that doesn't involve making them non-zero. @@ -835,6 +838,7 @@ int __kvm_set_memory_region(struct kvm *kvm, /* slot was deleted or moved, clear iommu mapping */ kvm_iommu_unmap_pages(kvm, &old); + old_iommu_mapped = false; /* From this point no new shadow pages pointing to a deleted, * or moved, memslot will be created. * @@ -863,11 +867,27 @@ int __kvm_set_memory_region(struct kvm *kvm, goto out_free; } - /* map new memory slot into the iommu */ + /* + * IOMMU mapping: New slots need to be mapped. Old slots need to be + * un-mapped and re-mapped if their base changes or if flags that the + * iommu cares about change (read-only). Base change unmapping is + * handled above with slot deletion, so we only unmap incompatible + * flags here. Anything else the iommu might care about for existing + * slots (size changes, userspace addr changes) is disallowed above, + * so any other attribute changes getting here can be skipped. + */ if (npages) { - r = kvm_iommu_map_pages(kvm, &new); - if (r) - goto out_slots; + if (old_iommu_mapped && + ((new.flags ^ old.flags) & KVM_MEM_READONLY)) { + kvm_iommu_unmap_pages(kvm, &old); + old_iommu_mapped = false; + } + + if (!old_iommu_mapped) { + r = kvm_iommu_map_pages(kvm, &new); + if (r) + goto out_slots; + } } /* actual memory is freed via old in kvm_free_physmem_slot below */ ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] kvm: Force IOMMU remapping on memory slot read-only flag changes 2013-01-24 22:04 ` [PATCH 1/2] kvm: Force IOMMU remapping on memory slot read-only flag changes Alex Williamson @ 2013-01-25 2:58 ` Xiao Guangrong 0 siblings, 0 replies; 18+ messages in thread From: Xiao Guangrong @ 2013-01-25 2:58 UTC (permalink / raw) To: Alex Williamson; +Cc: gleb, kvm, yoshikawa_takuya_b1, linux-kernel On 01/25/2013 06:04 AM, Alex Williamson wrote: > Memory slot flags can be altered without changing other parameters of > the slot. The read-only attribute is the only one the IOMMU cares > about, so generate an un-map, re-map when this occurs. This also > avoid unnecessarily re-mapping the slot when no IOMMU visible changes > are made. > Reviewed-by: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com> ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] kvm: Obey read-only mappings in iommu 2013-01-24 22:03 [PATCH 0/2] kvm: IOMMU read-only mapping support Alex Williamson 2013-01-24 22:04 ` [PATCH 1/2] kvm: Force IOMMU remapping on memory slot read-only flag changes Alex Williamson @ 2013-01-24 22:04 ` Alex Williamson 2013-01-25 1:17 ` [PATCH 0/2] kvm: IOMMU read-only mapping support Takuya Yoshikawa 2013-01-27 15:29 ` Gleb Natapov 3 siblings, 0 replies; 18+ messages in thread From: Alex Williamson @ 2013-01-24 22:04 UTC (permalink / raw) To: gleb, kvm; +Cc: yoshikawa_takuya_b1, linux-kernel, xiaoguangrong We've been ignoring read-only mappings and programming everything into the iommu as read-write. Fix this to only include the write access flag when read-only is not set. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- virt/kvm/iommu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c index 4a340cb..72a130b 100644 --- a/virt/kvm/iommu.c +++ b/virt/kvm/iommu.c @@ -76,7 +76,9 @@ int kvm_iommu_map_pages(struct kvm *kvm, struct kvm_memory_slot *slot) gfn = slot->base_gfn; end_gfn = gfn + slot->npages; - flags = IOMMU_READ | IOMMU_WRITE; + flags = IOMMU_READ; + if (!(slot->flags & KVM_MEM_READONLY)) + flags |= IOMMU_WRITE; if (kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY) flags |= IOMMU_CACHE; ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kvm: IOMMU read-only mapping support 2013-01-24 22:03 [PATCH 0/2] kvm: IOMMU read-only mapping support Alex Williamson 2013-01-24 22:04 ` [PATCH 1/2] kvm: Force IOMMU remapping on memory slot read-only flag changes Alex Williamson 2013-01-24 22:04 ` [PATCH 2/2] kvm: Obey read-only mappings in iommu Alex Williamson @ 2013-01-25 1:17 ` Takuya Yoshikawa 2013-01-25 3:28 ` Xiao Guangrong 2013-01-27 15:29 ` Gleb Natapov 3 siblings, 1 reply; 18+ messages in thread From: Takuya Yoshikawa @ 2013-01-25 1:17 UTC (permalink / raw) To: Alex Williamson; +Cc: gleb, kvm, linux-kernel, xiaoguangrong On Thu, 24 Jan 2013 15:03:57 -0700 Alex Williamson <alex.williamson@redhat.com> wrote: > A couple patches to make KVM IOMMU support honor read-only mappings. > This causes an un-map, re-map when the read-only flag changes and > makes use of it when setting IOMMU attributes. Thanks, Looks good to me. I think I can naturally update my patch after this gets merged. Thanks, Takuya ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kvm: IOMMU read-only mapping support 2013-01-25 1:17 ` [PATCH 0/2] kvm: IOMMU read-only mapping support Takuya Yoshikawa @ 2013-01-25 3:28 ` Xiao Guangrong 2013-01-25 3:59 ` Takuya Yoshikawa 2013-01-28 10:59 ` Gleb Natapov 0 siblings, 2 replies; 18+ messages in thread From: Xiao Guangrong @ 2013-01-25 3:28 UTC (permalink / raw) To: Takuya Yoshikawa; +Cc: Alex Williamson, gleb, kvm, linux-kernel On 01/25/2013 09:17 AM, Takuya Yoshikawa wrote: > On Thu, 24 Jan 2013 15:03:57 -0700 > Alex Williamson <alex.williamson@redhat.com> wrote: > >> A couple patches to make KVM IOMMU support honor read-only mappings. >> This causes an un-map, re-map when the read-only flag changes and >> makes use of it when setting IOMMU attributes. Thanks, > > Looks good to me. > > I think I can naturally update my patch after this gets merged. > Please wait. The commit c972f3b1 changed the write-protect behaviour - it does wirte-protection only when dirty flag is set. [ I did not see this commit when we discussed the problem before. ] Further more, i notice that write-protect is not enough, when do sync shadow page: FNAME(sync_page): host_writable = sp->spt[i] & SPTE_HOST_WRITEABLE; set_spte(vcpu, &sp->spt[i], pte_access, PT_PAGE_TABLE_LEVEL, gfn, spte_to_pfn(sp->spt[i]), true, false, host_writable); It sets spte based on the old value that means the readonly flag check is missed. We need to call kvm_arch_flush_shadow_all under this case. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kvm: IOMMU read-only mapping support 2013-01-25 3:28 ` Xiao Guangrong @ 2013-01-25 3:59 ` Takuya Yoshikawa 2013-01-25 4:28 ` Takuya Yoshikawa 2013-01-28 10:59 ` Gleb Natapov 1 sibling, 1 reply; 18+ messages in thread From: Takuya Yoshikawa @ 2013-01-25 3:59 UTC (permalink / raw) To: Xiao Guangrong; +Cc: Alex Williamson, gleb, kvm, linux-kernel On Fri, 25 Jan 2013 11:28:40 +0800 Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com> wrote: > > I think I can naturally update my patch after this gets merged. > > > > Please wait. The patch I mentioned above won't change anything. Just cleans up set_memory_region(). The only possible change which we discussed before was whether we call iommu_map() on a flags change. > The commit c972f3b1 changed the write-protect behaviour - it does > wirte-protection only when dirty flag is set. > [ I did not see this commit when we discussed the problem before. ] I'll look at the commit later, after the lunch break. > Further more, i notice that write-protect is not enough, when do sync > shadow page: > > FNAME(sync_page): > > host_writable = sp->spt[i] & SPTE_HOST_WRITEABLE; > > set_spte(vcpu, &sp->spt[i], pte_access, > PT_PAGE_TABLE_LEVEL, gfn, > spte_to_pfn(sp->spt[i]), true, false, > host_writable); > > It sets spte based on the old value that means the readonly flag check > is missed. We need to call kvm_arch_flush_shadow_all under this case. So the change needed will be in arch/x86. in arch_commit_* one. Right? Note: I'm not touching arch_* memory slot APIs now because ARM KVM is coming now. So no problem, the flags will be passed as before. Takuya ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kvm: IOMMU read-only mapping support 2013-01-25 3:59 ` Takuya Yoshikawa @ 2013-01-25 4:28 ` Takuya Yoshikawa 0 siblings, 0 replies; 18+ messages in thread From: Takuya Yoshikawa @ 2013-01-25 4:28 UTC (permalink / raw) To: Takuya Yoshikawa; +Cc: Xiao Guangrong, Alex Williamson, gleb, kvm, linux-kernel On Fri, 25 Jan 2013 12:59:12 +0900 Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> wrote: > > The commit c972f3b1 changed the write-protect behaviour - it does > > wirte-protection only when dirty flag is set. > > [ I did not see this commit when we discussed the problem before. ] > > I'll look at the commit later, after the lunch break. I've done! So we need to do: if (npages && (GET_DIRTY || RDONLY)) write protect About sync_page, I'm not sure what is the best fix. Takuya > > > Further more, i notice that write-protect is not enough, when do sync > > shadow page: > > > > FNAME(sync_page): > > > > host_writable = sp->spt[i] & SPTE_HOST_WRITEABLE; > > > > set_spte(vcpu, &sp->spt[i], pte_access, > > PT_PAGE_TABLE_LEVEL, gfn, > > spte_to_pfn(sp->spt[i]), true, false, > > host_writable); > > > > It sets spte based on the old value that means the readonly flag check > > is missed. We need to call kvm_arch_flush_shadow_all under this case. > > So the change needed will be in arch/x86. > in arch_commit_* one. > > Right? > > Note: I'm not touching arch_* memory slot APIs now because ARM KVM > is coming now. So no problem, the flags will be passed as before. > > Takuya -- Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kvm: IOMMU read-only mapping support 2013-01-25 3:28 ` Xiao Guangrong 2013-01-25 3:59 ` Takuya Yoshikawa @ 2013-01-28 10:59 ` Gleb Natapov 2013-01-28 12:25 ` Takuya Yoshikawa 2013-01-29 3:06 ` Xiao Guangrong 1 sibling, 2 replies; 18+ messages in thread From: Gleb Natapov @ 2013-01-28 10:59 UTC (permalink / raw) To: Xiao Guangrong; +Cc: Takuya Yoshikawa, Alex Williamson, kvm, linux-kernel On Fri, Jan 25, 2013 at 11:28:40AM +0800, Xiao Guangrong wrote: > On 01/25/2013 09:17 AM, Takuya Yoshikawa wrote: > > On Thu, 24 Jan 2013 15:03:57 -0700 > > Alex Williamson <alex.williamson@redhat.com> wrote: > > > >> A couple patches to make KVM IOMMU support honor read-only mappings. > >> This causes an un-map, re-map when the read-only flag changes and > >> makes use of it when setting IOMMU attributes. Thanks, > > > > Looks good to me. > > > > I think I can naturally update my patch after this gets merged. > > > > Please wait. > > The commit c972f3b1 changed the write-protect behaviour - it does > wirte-protection only when dirty flag is set. > [ I did not see this commit when we discussed the problem before. ] > > Further more, i notice that write-protect is not enough, when do sync > shadow page: > > FNAME(sync_page): > > host_writable = sp->spt[i] & SPTE_HOST_WRITEABLE; > > set_spte(vcpu, &sp->spt[i], pte_access, > PT_PAGE_TABLE_LEVEL, gfn, > spte_to_pfn(sp->spt[i]), true, false, > host_writable); > > It sets spte based on the old value that means the readonly flag check > is missed. We need to call kvm_arch_flush_shadow_all under this case. Why not just disallow changing memory region KVM_MEM_READONLY flag without deleting the region? -- Gleb. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kvm: IOMMU read-only mapping support 2013-01-28 10:59 ` Gleb Natapov @ 2013-01-28 12:25 ` Takuya Yoshikawa 2013-01-28 15:36 ` Alex Williamson 2013-01-29 3:06 ` Xiao Guangrong 1 sibling, 1 reply; 18+ messages in thread From: Takuya Yoshikawa @ 2013-01-28 12:25 UTC (permalink / raw) To: Gleb Natapov Cc: Xiao Guangrong, Takuya Yoshikawa, Alex Williamson, kvm, linux-kernel On Mon, 28 Jan 2013 12:59:03 +0200 Gleb Natapov <gleb@redhat.com> wrote: > > It sets spte based on the old value that means the readonly flag check > > is missed. We need to call kvm_arch_flush_shadow_all under this case. > Why not just disallow changing memory region KVM_MEM_READONLY flag > without deleting the region? Sounds good. If you prefer, I can fold the required change into my patch. Takuya ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kvm: IOMMU read-only mapping support 2013-01-28 12:25 ` Takuya Yoshikawa @ 2013-01-28 15:36 ` Alex Williamson 2013-01-29 2:12 ` Takuya Yoshikawa 0 siblings, 1 reply; 18+ messages in thread From: Alex Williamson @ 2013-01-28 15:36 UTC (permalink / raw) To: Takuya Yoshikawa Cc: Gleb Natapov, Xiao Guangrong, Takuya Yoshikawa, kvm, linux-kernel On Mon, 2013-01-28 at 21:25 +0900, Takuya Yoshikawa wrote: > On Mon, 28 Jan 2013 12:59:03 +0200 > Gleb Natapov <gleb@redhat.com> wrote: > > > > It sets spte based on the old value that means the readonly flag check > > > is missed. We need to call kvm_arch_flush_shadow_all under this case. > > Why not just disallow changing memory region KVM_MEM_READONLY flag > > without deleting the region? > > Sounds good. > > If you prefer, I can fold the required change into my patch. That would seem to make my patch 1/2 unnecessary. Thanks, Alex ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kvm: IOMMU read-only mapping support 2013-01-28 15:36 ` Alex Williamson @ 2013-01-29 2:12 ` Takuya Yoshikawa 0 siblings, 0 replies; 18+ messages in thread From: Takuya Yoshikawa @ 2013-01-29 2:12 UTC (permalink / raw) To: Alex Williamson Cc: Takuya Yoshikawa, Gleb Natapov, Xiao Guangrong, kvm, linux-kernel On Mon, 28 Jan 2013 08:36:56 -0700 Alex Williamson <alex.williamson@redhat.com> wrote: > On Mon, 2013-01-28 at 21:25 +0900, Takuya Yoshikawa wrote: > > On Mon, 28 Jan 2013 12:59:03 +0200 > > Gleb Natapov <gleb@redhat.com> wrote: > > > > > > It sets spte based on the old value that means the readonly flag check > > > > is missed. We need to call kvm_arch_flush_shadow_all under this case. > > > Why not just disallow changing memory region KVM_MEM_READONLY flag > > > without deleting the region? > > > > Sounds good. > > > > If you prefer, I can fold the required change into my patch. > > That would seem to make my patch 1/2 unnecessary. Thanks, I've decided not to fold that change into my KVM: set_memory_region: Identify the requested change explicitly since this is a functional change, see v3 I posted today. Since QEMU is not the only hypervisor using KVM, we should make the change with a proper subject informing every user of the new restriction. If the maintainers prefer to remove Alex's patch 1/2 first and then rebase my v3 patch, I can do so. Takuya ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kvm: IOMMU read-only mapping support 2013-01-28 10:59 ` Gleb Natapov 2013-01-28 12:25 ` Takuya Yoshikawa @ 2013-01-29 3:06 ` Xiao Guangrong 2013-01-29 6:50 ` Gleb Natapov 1 sibling, 1 reply; 18+ messages in thread From: Xiao Guangrong @ 2013-01-29 3:06 UTC (permalink / raw) To: Gleb Natapov; +Cc: Takuya Yoshikawa, Alex Williamson, kvm, linux-kernel On 01/28/2013 06:59 PM, Gleb Natapov wrote: > On Fri, Jan 25, 2013 at 11:28:40AM +0800, Xiao Guangrong wrote: >> On 01/25/2013 09:17 AM, Takuya Yoshikawa wrote: >>> On Thu, 24 Jan 2013 15:03:57 -0700 >>> Alex Williamson <alex.williamson@redhat.com> wrote: >>> >>>> A couple patches to make KVM IOMMU support honor read-only mappings. >>>> This causes an un-map, re-map when the read-only flag changes and >>>> makes use of it when setting IOMMU attributes. Thanks, >>> >>> Looks good to me. >>> >>> I think I can naturally update my patch after this gets merged. >>> >> >> Please wait. >> >> The commit c972f3b1 changed the write-protect behaviour - it does >> wirte-protection only when dirty flag is set. >> [ I did not see this commit when we discussed the problem before. ] >> >> Further more, i notice that write-protect is not enough, when do sync >> shadow page: >> >> FNAME(sync_page): >> >> host_writable = sp->spt[i] & SPTE_HOST_WRITEABLE; >> >> set_spte(vcpu, &sp->spt[i], pte_access, >> PT_PAGE_TABLE_LEVEL, gfn, >> spte_to_pfn(sp->spt[i]), true, false, >> host_writable); >> >> It sets spte based on the old value that means the readonly flag check >> is missed. We need to call kvm_arch_flush_shadow_all under this case. > Why not just disallow changing memory region KVM_MEM_READONLY flag > without deleting the region? It will introduce some restriction when VM-sharing-mem is being implemented, but we need to do some optimization for it, at least, properly write-protect readonly pages (fix sync_page()) instead of zap_all_page. So, i guess we can do the simple fix first. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kvm: IOMMU read-only mapping support 2013-01-29 3:06 ` Xiao Guangrong @ 2013-01-29 6:50 ` Gleb Natapov 2013-01-29 7:37 ` Xiao Guangrong 0 siblings, 1 reply; 18+ messages in thread From: Gleb Natapov @ 2013-01-29 6:50 UTC (permalink / raw) To: Xiao Guangrong; +Cc: Takuya Yoshikawa, Alex Williamson, kvm, linux-kernel On Tue, Jan 29, 2013 at 11:06:43AM +0800, Xiao Guangrong wrote: > On 01/28/2013 06:59 PM, Gleb Natapov wrote: > > On Fri, Jan 25, 2013 at 11:28:40AM +0800, Xiao Guangrong wrote: > >> On 01/25/2013 09:17 AM, Takuya Yoshikawa wrote: > >>> On Thu, 24 Jan 2013 15:03:57 -0700 > >>> Alex Williamson <alex.williamson@redhat.com> wrote: > >>> > >>>> A couple patches to make KVM IOMMU support honor read-only mappings. > >>>> This causes an un-map, re-map when the read-only flag changes and > >>>> makes use of it when setting IOMMU attributes. Thanks, > >>> > >>> Looks good to me. > >>> > >>> I think I can naturally update my patch after this gets merged. > >>> > >> > >> Please wait. > >> > >> The commit c972f3b1 changed the write-protect behaviour - it does > >> wirte-protection only when dirty flag is set. > >> [ I did not see this commit when we discussed the problem before. ] > >> > >> Further more, i notice that write-protect is not enough, when do sync > >> shadow page: > >> > >> FNAME(sync_page): > >> > >> host_writable = sp->spt[i] & SPTE_HOST_WRITEABLE; > >> > >> set_spte(vcpu, &sp->spt[i], pte_access, > >> PT_PAGE_TABLE_LEVEL, gfn, > >> spte_to_pfn(sp->spt[i]), true, false, > >> host_writable); > >> > >> It sets spte based on the old value that means the readonly flag check > >> is missed. We need to call kvm_arch_flush_shadow_all under this case. > > Why not just disallow changing memory region KVM_MEM_READONLY flag > > without deleting the region? > > It will introduce some restriction when VM-sharing-mem is being implemented, > but we need to do some optimization for it, at least, properly write-protect > readonly pages (fix sync_page()) instead of zap_all_page. > What is VM-sharing-mem? > So, i guess we can do the simple fix first. > By simple fix you mean calling kvm_arch_flush_shadow_all() on READONLY flag change? -- Gleb. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kvm: IOMMU read-only mapping support 2013-01-29 6:50 ` Gleb Natapov @ 2013-01-29 7:37 ` Xiao Guangrong [not found] ` <20130129074852.GH22871@redhat.com> 0 siblings, 1 reply; 18+ messages in thread From: Xiao Guangrong @ 2013-01-29 7:37 UTC (permalink / raw) To: Gleb Natapov; +Cc: Takuya Yoshikawa, Alex Williamson, kvm, linux-kernel On 01/29/2013 02:50 PM, Gleb Natapov wrote: > On Tue, Jan 29, 2013 at 11:06:43AM +0800, Xiao Guangrong wrote: >> On 01/28/2013 06:59 PM, Gleb Natapov wrote: >>> On Fri, Jan 25, 2013 at 11:28:40AM +0800, Xiao Guangrong wrote: >>>> On 01/25/2013 09:17 AM, Takuya Yoshikawa wrote: >>>>> On Thu, 24 Jan 2013 15:03:57 -0700 >>>>> Alex Williamson <alex.williamson@redhat.com> wrote: >>>>> >>>>>> A couple patches to make KVM IOMMU support honor read-only mappings. >>>>>> This causes an un-map, re-map when the read-only flag changes and >>>>>> makes use of it when setting IOMMU attributes. Thanks, >>>>> >>>>> Looks good to me. >>>>> >>>>> I think I can naturally update my patch after this gets merged. >>>>> >>>> >>>> Please wait. >>>> >>>> The commit c972f3b1 changed the write-protect behaviour - it does >>>> wirte-protection only when dirty flag is set. >>>> [ I did not see this commit when we discussed the problem before. ] >>>> >>>> Further more, i notice that write-protect is not enough, when do sync >>>> shadow page: >>>> >>>> FNAME(sync_page): >>>> >>>> host_writable = sp->spt[i] & SPTE_HOST_WRITEABLE; >>>> >>>> set_spte(vcpu, &sp->spt[i], pte_access, >>>> PT_PAGE_TABLE_LEVEL, gfn, >>>> spte_to_pfn(sp->spt[i]), true, false, >>>> host_writable); >>>> >>>> It sets spte based on the old value that means the readonly flag check >>>> is missed. We need to call kvm_arch_flush_shadow_all under this case. >>> Why not just disallow changing memory region KVM_MEM_READONLY flag >>> without deleting the region? >> >> It will introduce some restriction when VM-sharing-mem is being implemented, >> but we need to do some optimization for it, at least, properly write-protect >> readonly pages (fix sync_page()) instead of zap_all_page. >> > What is VM-sharing-mem? Sharing memory between different guests. > >> So, i guess we can do the simple fix first. >> > By simple fix you mean calling kvm_arch_flush_shadow_all() on READONLY > flag change? Simply disallow READONLY flag changing. ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <20130129074852.GH22871@redhat.com>]
* Re: [PATCH 0/2] kvm: IOMMU read-only mapping support [not found] ` <20130129074852.GH22871@redhat.com> @ 2013-01-30 4:06 ` Xiao Guangrong 2013-01-30 4:26 ` Takuya Yoshikawa 0 siblings, 1 reply; 18+ messages in thread From: Xiao Guangrong @ 2013-01-30 4:06 UTC (permalink / raw) To: Gleb Natapov; +Cc: Takuya Yoshikawa, Alex Williamson, kvm, linux-kernel On 01/29/2013 03:48 PM, Gleb Natapov wrote: > On Tue, Jan 29, 2013 at 03:37:23PM +0800, Xiao Guangrong wrote: >> On 01/29/2013 02:50 PM, Gleb Natapov wrote: >>> On Tue, Jan 29, 2013 at 11:06:43AM +0800, Xiao Guangrong wrote: >>>> On 01/28/2013 06:59 PM, Gleb Natapov wrote: >>>>> On Fri, Jan 25, 2013 at 11:28:40AM +0800, Xiao Guangrong wrote: >>>>>> On 01/25/2013 09:17 AM, Takuya Yoshikawa wrote: >>>>>>> On Thu, 24 Jan 2013 15:03:57 -0700 >>>>>>> Alex Williamson <alex.williamson@redhat.com> wrote: >>>>>>> >>>>>>>> A couple patches to make KVM IOMMU support honor read-only mappings. >>>>>>>> This causes an un-map, re-map when the read-only flag changes and >>>>>>>> makes use of it when setting IOMMU attributes. Thanks, >>>>>>> >>>>>>> Looks good to me. >>>>>>> >>>>>>> I think I can naturally update my patch after this gets merged. >>>>>>> >>>>>> >>>>>> Please wait. >>>>>> >>>>>> The commit c972f3b1 changed the write-protect behaviour - it does >>>>>> wirte-protection only when dirty flag is set. >>>>>> [ I did not see this commit when we discussed the problem before. ] >>>>>> >>>>>> Further more, i notice that write-protect is not enough, when do sync >>>>>> shadow page: >>>>>> >>>>>> FNAME(sync_page): >>>>>> >>>>>> host_writable = sp->spt[i] & SPTE_HOST_WRITEABLE; >>>>>> >>>>>> set_spte(vcpu, &sp->spt[i], pte_access, >>>>>> PT_PAGE_TABLE_LEVEL, gfn, >>>>>> spte_to_pfn(sp->spt[i]), true, false, >>>>>> host_writable); >>>>>> >>>>>> It sets spte based on the old value that means the readonly flag check >>>>>> is missed. We need to call kvm_arch_flush_shadow_all under this case. >>>>> Why not just disallow changing memory region KVM_MEM_READONLY flag >>>>> without deleting the region? >>>> >>>> It will introduce some restriction when VM-sharing-mem is being implemented, >>>> but we need to do some optimization for it, at least, properly write-protect >>>> readonly pages (fix sync_page()) instead of zap_all_page. >>>> >>> What is VM-sharing-mem? >> >> Sharing memory between different guests. >> > That much I can figure out for the name itself. My question is how this > sharing will work? Why KVM_MEM_READONLY is needed for it? Why ability to > change KVM_MEM_READONLY flag without destroying memory region will be > important. What's wrong with nahanni, shared memory device we have today? I'm not clear now or maybe my memory is wrong, i need to find the origin discussion... > >>> >>>> So, i guess we can do the simple fix first. >>>> >>> By simple fix you mean calling kvm_arch_flush_shadow_all() on READONLY >>> flag change? >> >> Simply disallow READONLY flag changing. > Ok, can somebody craft a patch? Takuya, will you? ;) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kvm: IOMMU read-only mapping support 2013-01-30 4:06 ` Xiao Guangrong @ 2013-01-30 4:26 ` Takuya Yoshikawa 0 siblings, 0 replies; 18+ messages in thread From: Takuya Yoshikawa @ 2013-01-30 4:26 UTC (permalink / raw) To: Xiao Guangrong; +Cc: Gleb Natapov, Alex Williamson, kvm, linux-kernel On Wed, 30 Jan 2013 12:06:32 +0800 Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com> wrote: > >>>> So, i guess we can do the simple fix first. > >>>> > >>> By simple fix you mean calling kvm_arch_flush_shadow_all() on READONLY > >>> flag change? > >> > >> Simply disallow READONLY flag changing. > > Ok, can somebody craft a patch? > > Takuya, will you? ;) I will. Thank you for pointing out the problem. We can revisit the API later to determine what kind of changes should be allowed. Takuya ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] kvm: IOMMU read-only mapping support 2013-01-24 22:03 [PATCH 0/2] kvm: IOMMU read-only mapping support Alex Williamson ` (2 preceding siblings ...) 2013-01-25 1:17 ` [PATCH 0/2] kvm: IOMMU read-only mapping support Takuya Yoshikawa @ 2013-01-27 15:29 ` Gleb Natapov 3 siblings, 0 replies; 18+ messages in thread From: Gleb Natapov @ 2013-01-27 15:29 UTC (permalink / raw) To: Alex Williamson; +Cc: kvm, yoshikawa_takuya_b1, linux-kernel, xiaoguangrong On Thu, Jan 24, 2013 at 03:03:57PM -0700, Alex Williamson wrote: > A couple patches to make KVM IOMMU support honor read-only mappings. > This causes an un-map, re-map when the read-only flag changes and > makes use of it when setting IOMMU attributes. Thanks, > > Alex > Applied, thanks. > --- > > Alex Williamson (2): > kvm: Force IOMMU remapping on memory slot read-only flag changes > kvm: Obey read-only mappings in iommu > > > virt/kvm/iommu.c | 4 +++- > virt/kvm/kvm_main.c | 28 ++++++++++++++++++++++++---- > 2 files changed, 27 insertions(+), 5 deletions(-) -- Gleb. ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2013-01-30 4:26 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-24 22:03 [PATCH 0/2] kvm: IOMMU read-only mapping support Alex Williamson
2013-01-24 22:04 ` [PATCH 1/2] kvm: Force IOMMU remapping on memory slot read-only flag changes Alex Williamson
2013-01-25 2:58 ` Xiao Guangrong
2013-01-24 22:04 ` [PATCH 2/2] kvm: Obey read-only mappings in iommu Alex Williamson
2013-01-25 1:17 ` [PATCH 0/2] kvm: IOMMU read-only mapping support Takuya Yoshikawa
2013-01-25 3:28 ` Xiao Guangrong
2013-01-25 3:59 ` Takuya Yoshikawa
2013-01-25 4:28 ` Takuya Yoshikawa
2013-01-28 10:59 ` Gleb Natapov
2013-01-28 12:25 ` Takuya Yoshikawa
2013-01-28 15:36 ` Alex Williamson
2013-01-29 2:12 ` Takuya Yoshikawa
2013-01-29 3:06 ` Xiao Guangrong
2013-01-29 6:50 ` Gleb Natapov
2013-01-29 7:37 ` Xiao Guangrong
[not found] ` <20130129074852.GH22871@redhat.com>
2013-01-30 4:06 ` Xiao Guangrong
2013-01-30 4:26 ` Takuya Yoshikawa
2013-01-27 15:29 ` Gleb Natapov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).