diff for duplicates of <1506460485.5507.57.camel@gmail.com> diff --git a/a/1.txt b/N1/1.txt index fa5b7ef..c81b8f1 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -5,10 +5,10 @@ On Thu, 2017-08-31 at 11:23 +0200, Christoffer Dall 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. @@ -18,7 +18,7 @@ However if you'd prefer it otherwise, I'll make sure this condition will be made clearer. > 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. @@ -27,19 +27,19 @@ internal VMs. I can not think of a usage when this would be useful. I'd make sure this would be documented in an eventual later RFC. > > +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 ? @@ -48,32 +48,32 @@ Runtime Services sandboxing. It also relies on the guest being correctly configured. > > + -> > + 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? Will do. Thank you very much, - Florent +? ? Florent diff --git a/a/content_digest b/N1/content_digest index 75504df..2f7fa8e 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -1,26 +1,10 @@ "ref\01503649901-5834-1-git-send-email-florent.revest@arm.com\0" "ref\01503649901-5834-5-git-send-email-florent.revest@arm.com\0" "ref\020170831092305.GA13572@cbox\0" - "From\0Florent Revest <revestflo@gmail.com>\0" - "Subject\0Re: [RFC 04/11] KVM, arm, arm64: Offer PAs to IPAs idmapping to internal VMs\0" + "From\0revestflo@gmail.com (Florent Revest)\0" + "Subject\0[RFC 04/11] KVM, arm, arm64: Offer PAs to IPAs idmapping to internal VMs\0" "Date\0Tue, 26 Sep 2017 23:14:45 +0200\0" - "To\0Christoffer Dall <cdall@linaro.org>" - " Florent Revest <florent.revest@arm.com>\0" - "Cc\0linux-arm-kernel@lists.infradead.org" - matt@codeblueprint.co.uk - ard.biesheuvel@linaro.org - pbonzini@redhat.com - rkrcmar@redhat.com - christoffer.dall@linaro.org - catalin.marinas@arm.com - will.deacon@arm.com - mark.rutland@arm.com - marc.zyngier@arm.com - linux-efi@vger.kernel.org - linux-kernel@vger.kernel.org - kvm@vger.kernel.org - kvmarm@lists.cs.columbia.edu - " leif.lindholm@arm.com\0" + "To\0linux-arm-kernel@lists.infradead.org\0" "\00:1\0" "b\0" "On Thu, 2017-08-31 at 11:23 +0200, Christoffer Dall wrote:\n" @@ -30,10 +14,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" @@ -43,7 +27,7 @@ "will be made clearer.\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" @@ -52,19 +36,19 @@ "make sure this would be documented in an eventual later RFC.\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" @@ -73,34 +57,34 @@ "correctly configured.\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" "Will do.\n" "\n" "Thank you very much,\n" - "\302\240 \302\240 Florent" + ? ? Florent -ad53821e9c6640f5ba87d71b746d8141e310fe86a0b353f04a69ec6c16010064 +458db6d7d7039e0e193f1ba0a4daa4e3167f22c2604d29a7dae428a3ef9dd0e8
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.