diff for duplicates of <20171016204505.GN1845@lvm> diff --git a/a/1.txt b/N1/1.txt index 77bb264..dc6d6fa 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -6,10 +6,10 @@ On Tue, Sep 26, 2017 at 11:14:45PM +0200, Florent Revest wrote: > > > +++ b/virt/kvm/arm/mmu.c > > > @@ -772,6 +772,11 @@ static void stage2_unmap_memslot(struct kvm > > > *kvm, -> > > phys_addr_t size = PAGE_SIZE * memslot->npages; -> > > hva_t reg_end = hva + size; +> > > ????????phys_addr_t size = PAGE_SIZE * memslot->npages; +> > > ????????hva_t reg_end = hva + size; > > > -> > > + if (unlikely(!kvm->mm)) { +> > > +???????if (unlikely(!kvm->mm)) { > > I think you should consider using a predicate so that it's clear that > > this is for in-kernel VMs and not just some random situation where mm > > can be NULL. @@ -23,7 +23,7 @@ My point was then when I see (!kvm->mm) it looks like a bug, but if I saw is_in_kernel_vm(kvm) then it looks like a feature. > > So it's unclear to me why we don't need any special casing in -> > kvm_handle_guest_abort, related to MMIO exits etc. You probably +> > kvm_handle_guest_abort, related to MMIO exits etc.??You probably > > assume that we will never do emulation, but that should be described > > and addressed somewhere before I can critically review this patch. > @@ -38,19 +38,19 @@ in-kernel VM support' which are handled in such and such way. > > > +static int internal_vm_prep_mem(struct kvm *kvm, -> > > + const struct +> > > +???????????????????????????????const struct > > > kvm_userspace_memory_region *mem) > > > +{ -> > > + phys_addr_t addr, end; -> > > + unsigned long pfn; -> > > + int ret; -> > > + struct kvm_mmu_memory_cache cache = { 0 }; +> > > +???????phys_addr_t addr, end; +> > > +???????unsigned long pfn; +> > > +???????int ret; +> > > +???????struct kvm_mmu_memory_cache cache = { 0 }; > > > + -> > > + end = mem->guest_phys_addr + mem->memory_size; -> > > + pfn = __phys_to_pfn(mem->guest_phys_addr); -> > > + addr = mem->guest_phys_addr; +> > > +???????end = mem->guest_phys_addr + mem->memory_size; +> > > +???????pfn = __phys_to_pfn(mem->guest_phys_addr); +> > > +???????addr = mem->guest_phys_addr; > > My main concern here is that we don't do any checks on this region -> > and we could be mapping device memory here as well. Are we intending +> > and we could be mapping device memory here as well.??Are we intending > > that to be ok, and are we then relying on the guest to use proper > > memory attributes ? > @@ -68,28 +68,28 @@ This seems to break with that level of isolation, and that property of in-kernel VMs should be clearly pointed out somewhere. > > > + -> > > + for (; addr < end; addr += PAGE_SIZE) { -> > > + pte_t pte = pfn_pte(pfn, PAGE_S2); +> > > +???????for (; addr < end; addr += PAGE_SIZE) { +> > > +???????????????pte_t pte = pfn_pte(pfn, PAGE_S2); > > > + -> > > + pte = kvm_s2pte_mkwrite(pte); +> > > +???????????????pte = kvm_s2pte_mkwrite(pte); > > > + -> > > + ret = mmu_topup_memory_cache(&cache, -> > > + KVM_MMU_CACHE_MIN_PAGE +> > > +???????????????ret = mmu_topup_memory_cache(&cache, +> > > +????????????????????????????????????????????KVM_MMU_CACHE_MIN_PAGE > > > S, -> > > + KVM_NR_MEM_OBJS); +> > > +????????????????????????????????????????????KVM_NR_MEM_OBJS); > > You should be able to allocate all you need up front instead of doing > > it in sequences. > > Ok. > > > > -> > > + if (ret) { -> > > + mmu_free_memory_cache(&cache); -> > > + return ret; -> > > + } -> > > + spin_lock(&kvm->mmu_lock); -> > > + ret = stage2_set_pte(kvm, &cache, addr, &pte, 0); -> > > + spin_unlock(&kvm->mmu_lock); +> > > +???????????????if (ret) { +> > > +???????????????????????mmu_free_memory_cache(&cache); +> > > +???????????????????????return ret; +> > > +???????????????} +> > > +???????????????spin_lock(&kvm->mmu_lock); +> > > +???????????????ret = stage2_set_pte(kvm, &cache, addr, &pte, 0); +> > > +???????????????spin_unlock(&kvm->mmu_lock); > > Since you're likely to allocate some large contiguous chunks here, > > can you have a look at using section mappings? > diff --git a/a/content_digest b/N1/content_digest index 0f2e3da..29240e1 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -2,23 +2,10 @@ "ref\01503649901-5834-5-git-send-email-florent.revest@arm.com\0" "ref\020170831092305.GA13572@cbox\0" "ref\01506460485.5507.57.camel@gmail.com\0" - "From\0Christoffer Dall <cdall@linaro.org>\0" - "Subject\0Re: [RFC 04/11] KVM, arm, arm64: Offer PAs to IPAs idmapping to internal VMs\0" + "From\0cdall@linaro.org (Christoffer Dall)\0" + "Subject\0[RFC 04/11] KVM, arm, arm64: Offer PAs to IPAs idmapping to internal VMs\0" "Date\0Mon, 16 Oct 2017 22:45:05 +0200\0" - "To\0Florent Revest <revestflo@gmail.com>\0" - "Cc\0linux-efi@vger.kernel.org" - kvm@vger.kernel.org - matt@codeblueprint.co.uk - catalin.marinas@arm.com - ard.biesheuvel@linaro.org - will.deacon@arm.com - linux-kernel@vger.kernel.org - leif.lindholm@arm.com - marc.zyngier@arm.com - pbonzini@redhat.com - Florent Revest <florent.revest@arm.com> - kvmarm@lists.cs.columbia.edu - " linux-arm-kernel@lists.infradead.org\0" + "To\0linux-arm-kernel@lists.infradead.org\0" "\00:1\0" "b\0" "On Tue, Sep 26, 2017 at 11:14:45PM +0200, Florent Revest wrote:\n" @@ -29,10 +16,10 @@ "> > > +++ b/virt/kvm/arm/mmu.c\n" "> > > @@ -772,6 +772,11 @@ static void stage2_unmap_memslot(struct kvm\n" "> > > *kvm,\n" - "> > > \302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240phys_addr_t size = PAGE_SIZE * memslot->npages;\n" - "> > > \302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240hva_t reg_end = hva + size;\n" + "> > > ????????phys_addr_t size = PAGE_SIZE * memslot->npages;\n" + "> > > ????????hva_t reg_end = hva + size;\n" "> > > \n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240if (unlikely(!kvm->mm)) {\n" + "> > > +???????if (unlikely(!kvm->mm)) {\n" "> > I think you should consider using a predicate so that it's clear that\n" "> > this is for in-kernel VMs and not just some random situation where mm\n" "> > can be NULL.\n" @@ -46,7 +33,7 @@ "saw is_in_kernel_vm(kvm) then it looks like a feature.\n" "\n" "> > So it's unclear to me why we don't need any special casing in\n" - "> > kvm_handle_guest_abort, related to MMIO exits etc.\302\240\302\240You probably\n" + "> > kvm_handle_guest_abort, related to MMIO exits etc.??You probably\n" "> > assume that we will never do emulation, but that should be described\n" "> > and addressed somewhere before I can critically review this patch.\n" "> \n" @@ -61,19 +48,19 @@ "\n" "\n" "> > > +static int internal_vm_prep_mem(struct kvm *kvm,\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240const struct\n" + "> > > +???????????????????????????????const struct\n" "> > > kvm_userspace_memory_region *mem)\n" "> > > +{\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240phys_addr_t addr, end;\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240unsigned long pfn;\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240int ret;\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240struct kvm_mmu_memory_cache cache = { 0 };\n" + "> > > +???????phys_addr_t addr, end;\n" + "> > > +???????unsigned long pfn;\n" + "> > > +???????int ret;\n" + "> > > +???????struct kvm_mmu_memory_cache cache = { 0 };\n" "> > > +\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240end = mem->guest_phys_addr + mem->memory_size;\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240pfn = __phys_to_pfn(mem->guest_phys_addr);\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240addr = mem->guest_phys_addr;\n" + "> > > +???????end = mem->guest_phys_addr + mem->memory_size;\n" + "> > > +???????pfn = __phys_to_pfn(mem->guest_phys_addr);\n" + "> > > +???????addr = mem->guest_phys_addr;\n" "> > My main concern here is that we don't do any checks on this region\n" - "> > and we could be mapping device memory here as well.\302\240\302\240Are we intending\n" + "> > and we could be mapping device memory here as well.??Are we intending\n" "> > that to be ok, and are we then relying on the guest to use proper\n" "> > memory attributes ?\n" "> \n" @@ -91,28 +78,28 @@ "in-kernel VMs should be clearly pointed out somewhere.\n" "\n" "> > > +\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240for (; addr < end; addr += PAGE_SIZE) {\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240pte_t pte = pfn_pte(pfn, PAGE_S2);\n" + "> > > +???????for (; addr < end; addr += PAGE_SIZE) {\n" + "> > > +???????????????pte_t pte = pfn_pte(pfn, PAGE_S2);\n" "> > > +\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240pte = kvm_s2pte_mkwrite(pte);\n" + "> > > +???????????????pte = kvm_s2pte_mkwrite(pte);\n" "> > > +\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240ret = mmu_topup_memory_cache(&cache,\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240KVM_MMU_CACHE_MIN_PAGE\n" + "> > > +???????????????ret = mmu_topup_memory_cache(&cache,\n" + "> > > +????????????????????????????????????????????KVM_MMU_CACHE_MIN_PAGE\n" "> > > S,\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240KVM_NR_MEM_OBJS);\n" + "> > > +????????????????????????????????????????????KVM_NR_MEM_OBJS);\n" "> > You should be able to allocate all you need up front instead of doing\n" "> > it in sequences.\n" "> \n" "> Ok.\n" "> \n" "> > > \n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240if (ret) {\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240mmu_free_memory_cache(&cache);\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240return ret;\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240}\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240spin_lock(&kvm->mmu_lock);\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240ret = stage2_set_pte(kvm, &cache, addr, &pte, 0);\n" - "> > > +\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240\302\240spin_unlock(&kvm->mmu_lock);\n" + "> > > +???????????????if (ret) {\n" + "> > > +???????????????????????mmu_free_memory_cache(&cache);\n" + "> > > +???????????????????????return ret;\n" + "> > > +???????????????}\n" + "> > > +???????????????spin_lock(&kvm->mmu_lock);\n" + "> > > +???????????????ret = stage2_set_pte(kvm, &cache, addr, &pte, 0);\n" + "> > > +???????????????spin_unlock(&kvm->mmu_lock);\n" "> > Since you're likely to allocate some large contiguous chunks here,\n" "> > can you have a look at using section mappings?\n" "> \n" @@ -122,4 +109,4 @@ "Thanks!\n" -Christoffer -2eb3afa41125950a18e831d28da4c97f573f45dd237ee5790a89d2d17403d6b7 +1e67b3c3acabee68e30c03fc9c7c3a3b3fbdc8b8b7c830f716ee2b5e209ee694
diff --git a/a/content_digest b/N2/content_digest index 0f2e3da..b72a801 100644 --- a/a/content_digest +++ b/N2/content_digest @@ -6,19 +6,22 @@ "Subject\0Re: [RFC 04/11] KVM, arm, arm64: Offer PAs to IPAs idmapping to internal VMs\0" "Date\0Mon, 16 Oct 2017 22:45:05 +0200\0" "To\0Florent Revest <revestflo@gmail.com>\0" - "Cc\0linux-efi@vger.kernel.org" - kvm@vger.kernel.org + "Cc\0Florent Revest <florent.revest@arm.com>" + linux-arm-kernel@lists.infradead.org matt@codeblueprint.co.uk - catalin.marinas@arm.com ard.biesheuvel@linaro.org + pbonzini@redhat.com + rkrcmar@redhat.com + christoffer.dall@linaro.org + catalin.marinas@arm.com will.deacon@arm.com - linux-kernel@vger.kernel.org - leif.lindholm@arm.com + mark.rutland@arm.com marc.zyngier@arm.com - pbonzini@redhat.com - Florent Revest <florent.revest@arm.com> + linux-efi@vger.kernel.org + linux-kernel@vger.kernel.org + kvm@vger.kernel.org kvmarm@lists.cs.columbia.edu - " linux-arm-kernel@lists.infradead.org\0" + " leif.lindholm@arm.com\0" "\00:1\0" "b\0" "On Tue, Sep 26, 2017 at 11:14:45PM +0200, Florent Revest wrote:\n" @@ -122,4 +125,4 @@ "Thanks!\n" -Christoffer -2eb3afa41125950a18e831d28da4c97f573f45dd237ee5790a89d2d17403d6b7 +d236a5bc3a1de7fd38980ccb2454d34909dcd47e43430707ef78ff5d0db206f4
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.