From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christoffer Dall Subject: Re: [PATCH v4 12/19] KVM: arm/arm64: Move ioremap calls to create_hyp_io_mappings Date: Mon, 15 Jan 2018 19:07:28 +0100 Message-ID: <20180115180728.GN21403@cbox> References: <20180104184334.16571-1-marc.zyngier@arm.com> <20180104184334.16571-13-marc.zyngier@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Catalin Marinas , Will Deacon , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org To: Marc Zyngier Return-path: Content-Disposition: inline In-Reply-To: <20180104184334.16571-13-marc.zyngier@arm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu List-Id: kvm.vger.kernel.org On Thu, Jan 04, 2018 at 06:43:27PM +0000, Marc Zyngier wrote: > Both HYP io mappings call ioremap, followed by create_hyp_io_mappings. > Let's move the ioremap call into create_hyp_io_mappings itself, which > simplifies the code a bit and allows for further refactoring. > > Signed-off-by: Marc Zyngier > --- > arch/arm/include/asm/kvm_mmu.h | 3 ++- > arch/arm64/include/asm/kvm_mmu.h | 3 ++- > virt/kvm/arm/mmu.c | 24 ++++++++++++++---------- > virt/kvm/arm/vgic/vgic-v2.c | 31 ++++++++----------------------- > 4 files changed, 26 insertions(+), 35 deletions(-) > > diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h > index fa6f2174276b..cb3bef71ec9b 100644 > --- a/arch/arm/include/asm/kvm_mmu.h > +++ b/arch/arm/include/asm/kvm_mmu.h > @@ -41,7 +41,8 @@ > #include > > int create_hyp_mappings(void *from, void *to, pgprot_t prot); > -int create_hyp_io_mappings(void *from, void *to, phys_addr_t); > +int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size, > + void __iomem **kaddr); > void free_hyp_pgds(void); > > void stage2_unmap_vm(struct kvm *kvm); > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h > index b0c3cbe9b513..09a208014457 100644 > --- a/arch/arm64/include/asm/kvm_mmu.h > +++ b/arch/arm64/include/asm/kvm_mmu.h > @@ -119,7 +119,8 @@ static inline unsigned long __kern_hyp_va(unsigned long v) > #include > > int create_hyp_mappings(void *from, void *to, pgprot_t prot); > -int create_hyp_io_mappings(void *from, void *to, phys_addr_t); > +int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size, > + void __iomem **kaddr); > void free_hyp_pgds(void); > > void stage2_unmap_vm(struct kvm *kvm); > diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c > index 84d09f1a44d4..38adbe0a016c 100644 > --- a/virt/kvm/arm/mmu.c > +++ b/virt/kvm/arm/mmu.c > @@ -709,26 +709,30 @@ int create_hyp_mappings(void *from, void *to, pgprot_t prot) > } > > /** > - * create_hyp_io_mappings - duplicate a kernel IO mapping into Hyp mode > - * @from: The kernel start VA of the range > - * @to: The kernel end VA of the range (exclusive) > + * create_hyp_io_mappings - Map IO into both kernel and HYP > * @phys_addr: The physical start address which gets mapped > + * @size: Size of the region being mapped > + * @kaddr: Kernel VA for this mapping > * > * The resulting HYP VA is the same as the kernel VA, modulo > * HYP_PAGE_OFFSET. > */ > -int create_hyp_io_mappings(void *from, void *to, phys_addr_t phys_addr) > +int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size, > + void __iomem **kaddr) nit: you could make this return a "void __iomem *" and use ERR_PTR etc. as well, but it'll probably look worse on the calling side, so either way is fine with me. > { > - unsigned long start = kern_hyp_va((unsigned long)from); > - unsigned long end = kern_hyp_va((unsigned long)to); > + unsigned long start, end; > > - if (is_kernel_in_hyp_mode()) > + *kaddr = ioremap(phys_addr, size); > + if (!*kaddr) > + return -ENOMEM; > + > + if (is_kernel_in_hyp_mode()) { > return 0; > + } nit: we don't need the curly braces > > - /* Check for a valid kernel IO mapping */ > - if (!is_vmalloc_addr(from) || !is_vmalloc_addr(to - 1)) > - return -EINVAL; > > + start = kern_hyp_va((unsigned long)*kaddr); > + end = kern_hyp_va((unsigned long)*kaddr + size); > return __create_hyp_mappings(hyp_pgd, start, end, > __phys_to_pfn(phys_addr), PAGE_HYP_DEVICE); > } > diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c > index 80897102da26..bc49d702f9f0 100644 > --- a/virt/kvm/arm/vgic/vgic-v2.c > +++ b/virt/kvm/arm/vgic/vgic-v2.c > @@ -332,16 +332,10 @@ int vgic_v2_probe(const struct gic_kvm_info *info) > if (!PAGE_ALIGNED(info->vcpu.start) || > !PAGE_ALIGNED(resource_size(&info->vcpu))) { > kvm_info("GICV region size/alignment is unsafe, using trapping (reduced performance)\n"); > - kvm_vgic_global_state.vcpu_base_va = ioremap(info->vcpu.start, > - resource_size(&info->vcpu)); > - if (!kvm_vgic_global_state.vcpu_base_va) { > - kvm_err("Cannot ioremap GICV\n"); > - return -ENOMEM; > - } > > - ret = create_hyp_io_mappings(kvm_vgic_global_state.vcpu_base_va, > - kvm_vgic_global_state.vcpu_base_va + resource_size(&info->vcpu), > - info->vcpu.start); > + ret = create_hyp_io_mappings(info->vcpu.start, > + resource_size(&info->vcpu), > + &kvm_vgic_global_state.vcpu_base_va); > if (ret) { > kvm_err("Cannot map GICV into hyp\n"); > goto out; > @@ -350,26 +344,17 @@ int vgic_v2_probe(const struct gic_kvm_info *info) > static_branch_enable(&vgic_v2_cpuif_trap); > } > > - kvm_vgic_global_state.vctrl_base = ioremap(info->vctrl.start, > - resource_size(&info->vctrl)); > - if (!kvm_vgic_global_state.vctrl_base) { > - kvm_err("Cannot ioremap GICH\n"); > - ret = -ENOMEM; > + ret = create_hyp_io_mappings(info->vctrl.start, > + resource_size(&info->vctrl), > + &kvm_vgic_global_state.vctrl_base); > + if (ret) { > + kvm_err("Cannot map VCTRL into hyp\n"); > goto out; > } > > vtr = readl_relaxed(kvm_vgic_global_state.vctrl_base + GICH_VTR); > kvm_vgic_global_state.nr_lr = (vtr & 0x3f) + 1; > > - ret = create_hyp_io_mappings(kvm_vgic_global_state.vctrl_base, > - kvm_vgic_global_state.vctrl_base + > - resource_size(&info->vctrl), > - info->vctrl.start); > - if (ret) { > - kvm_err("Cannot map VCTRL into hyp\n"); > - goto out; > - } > - > ret = kvm_register_vgic_device(KVM_DEV_TYPE_ARM_VGIC_V2); > if (ret) { > kvm_err("Cannot register GICv2 KVM device\n"); > -- > 2.14.2 > Otherwise looks good: Reviewed-by: Christoffer Dall From mboxrd@z Thu Jan 1 00:00:00 1970 From: christoffer.dall@linaro.org (Christoffer Dall) Date: Mon, 15 Jan 2018 19:07:28 +0100 Subject: [PATCH v4 12/19] KVM: arm/arm64: Move ioremap calls to create_hyp_io_mappings In-Reply-To: <20180104184334.16571-13-marc.zyngier@arm.com> References: <20180104184334.16571-1-marc.zyngier@arm.com> <20180104184334.16571-13-marc.zyngier@arm.com> Message-ID: <20180115180728.GN21403@cbox> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, Jan 04, 2018 at 06:43:27PM +0000, Marc Zyngier wrote: > Both HYP io mappings call ioremap, followed by create_hyp_io_mappings. > Let's move the ioremap call into create_hyp_io_mappings itself, which > simplifies the code a bit and allows for further refactoring. > > Signed-off-by: Marc Zyngier > --- > arch/arm/include/asm/kvm_mmu.h | 3 ++- > arch/arm64/include/asm/kvm_mmu.h | 3 ++- > virt/kvm/arm/mmu.c | 24 ++++++++++++++---------- > virt/kvm/arm/vgic/vgic-v2.c | 31 ++++++++----------------------- > 4 files changed, 26 insertions(+), 35 deletions(-) > > diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h > index fa6f2174276b..cb3bef71ec9b 100644 > --- a/arch/arm/include/asm/kvm_mmu.h > +++ b/arch/arm/include/asm/kvm_mmu.h > @@ -41,7 +41,8 @@ > #include > > int create_hyp_mappings(void *from, void *to, pgprot_t prot); > -int create_hyp_io_mappings(void *from, void *to, phys_addr_t); > +int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size, > + void __iomem **kaddr); > void free_hyp_pgds(void); > > void stage2_unmap_vm(struct kvm *kvm); > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h > index b0c3cbe9b513..09a208014457 100644 > --- a/arch/arm64/include/asm/kvm_mmu.h > +++ b/arch/arm64/include/asm/kvm_mmu.h > @@ -119,7 +119,8 @@ static inline unsigned long __kern_hyp_va(unsigned long v) > #include > > int create_hyp_mappings(void *from, void *to, pgprot_t prot); > -int create_hyp_io_mappings(void *from, void *to, phys_addr_t); > +int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size, > + void __iomem **kaddr); > void free_hyp_pgds(void); > > void stage2_unmap_vm(struct kvm *kvm); > diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c > index 84d09f1a44d4..38adbe0a016c 100644 > --- a/virt/kvm/arm/mmu.c > +++ b/virt/kvm/arm/mmu.c > @@ -709,26 +709,30 @@ int create_hyp_mappings(void *from, void *to, pgprot_t prot) > } > > /** > - * create_hyp_io_mappings - duplicate a kernel IO mapping into Hyp mode > - * @from: The kernel start VA of the range > - * @to: The kernel end VA of the range (exclusive) > + * create_hyp_io_mappings - Map IO into both kernel and HYP > * @phys_addr: The physical start address which gets mapped > + * @size: Size of the region being mapped > + * @kaddr: Kernel VA for this mapping > * > * The resulting HYP VA is the same as the kernel VA, modulo > * HYP_PAGE_OFFSET. > */ > -int create_hyp_io_mappings(void *from, void *to, phys_addr_t phys_addr) > +int create_hyp_io_mappings(phys_addr_t phys_addr, size_t size, > + void __iomem **kaddr) nit: you could make this return a "void __iomem *" and use ERR_PTR etc. as well, but it'll probably look worse on the calling side, so either way is fine with me. > { > - unsigned long start = kern_hyp_va((unsigned long)from); > - unsigned long end = kern_hyp_va((unsigned long)to); > + unsigned long start, end; > > - if (is_kernel_in_hyp_mode()) > + *kaddr = ioremap(phys_addr, size); > + if (!*kaddr) > + return -ENOMEM; > + > + if (is_kernel_in_hyp_mode()) { > return 0; > + } nit: we don't need the curly braces > > - /* Check for a valid kernel IO mapping */ > - if (!is_vmalloc_addr(from) || !is_vmalloc_addr(to - 1)) > - return -EINVAL; > > + start = kern_hyp_va((unsigned long)*kaddr); > + end = kern_hyp_va((unsigned long)*kaddr + size); > return __create_hyp_mappings(hyp_pgd, start, end, > __phys_to_pfn(phys_addr), PAGE_HYP_DEVICE); > } > diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c > index 80897102da26..bc49d702f9f0 100644 > --- a/virt/kvm/arm/vgic/vgic-v2.c > +++ b/virt/kvm/arm/vgic/vgic-v2.c > @@ -332,16 +332,10 @@ int vgic_v2_probe(const struct gic_kvm_info *info) > if (!PAGE_ALIGNED(info->vcpu.start) || > !PAGE_ALIGNED(resource_size(&info->vcpu))) { > kvm_info("GICV region size/alignment is unsafe, using trapping (reduced performance)\n"); > - kvm_vgic_global_state.vcpu_base_va = ioremap(info->vcpu.start, > - resource_size(&info->vcpu)); > - if (!kvm_vgic_global_state.vcpu_base_va) { > - kvm_err("Cannot ioremap GICV\n"); > - return -ENOMEM; > - } > > - ret = create_hyp_io_mappings(kvm_vgic_global_state.vcpu_base_va, > - kvm_vgic_global_state.vcpu_base_va + resource_size(&info->vcpu), > - info->vcpu.start); > + ret = create_hyp_io_mappings(info->vcpu.start, > + resource_size(&info->vcpu), > + &kvm_vgic_global_state.vcpu_base_va); > if (ret) { > kvm_err("Cannot map GICV into hyp\n"); > goto out; > @@ -350,26 +344,17 @@ int vgic_v2_probe(const struct gic_kvm_info *info) > static_branch_enable(&vgic_v2_cpuif_trap); > } > > - kvm_vgic_global_state.vctrl_base = ioremap(info->vctrl.start, > - resource_size(&info->vctrl)); > - if (!kvm_vgic_global_state.vctrl_base) { > - kvm_err("Cannot ioremap GICH\n"); > - ret = -ENOMEM; > + ret = create_hyp_io_mappings(info->vctrl.start, > + resource_size(&info->vctrl), > + &kvm_vgic_global_state.vctrl_base); > + if (ret) { > + kvm_err("Cannot map VCTRL into hyp\n"); > goto out; > } > > vtr = readl_relaxed(kvm_vgic_global_state.vctrl_base + GICH_VTR); > kvm_vgic_global_state.nr_lr = (vtr & 0x3f) + 1; > > - ret = create_hyp_io_mappings(kvm_vgic_global_state.vctrl_base, > - kvm_vgic_global_state.vctrl_base + > - resource_size(&info->vctrl), > - info->vctrl.start); > - if (ret) { > - kvm_err("Cannot map VCTRL into hyp\n"); > - goto out; > - } > - > ret = kvm_register_vgic_device(KVM_DEV_TYPE_ARM_VGIC_V2); > if (ret) { > kvm_err("Cannot register GICv2 KVM device\n"); > -- > 2.14.2 > Otherwise looks good: Reviewed-by: Christoffer Dall