From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755425AbcB2P1y (ORCPT ); Mon, 29 Feb 2016 10:27:54 -0500 Received: from mail-wm0-f52.google.com ([74.125.82.52]:35213 "EHLO mail-wm0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751261AbcB2P1v (ORCPT ); Mon, 29 Feb 2016 10:27:51 -0500 Subject: Re: [RFC v4 09/14] msi: IOMMU map the doorbell address when needed To: Thomas Gleixner References: <1456508154-2253-1-git-send-email-eric.auger@linaro.org> <1456508154-2253-10-git-send-email-eric.auger@linaro.org> Cc: eric.auger@st.com, robin.murphy@arm.com, alex.williamson@redhat.com, will.deacon@arm.com, joro@8bytes.org, jason@lakedaemon.net, marc.zyngier@arm.com, christoffer.dall@linaro.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, suravee.suthikulpanit@amd.com, patches@linaro.org, linux-kernel@vger.kernel.org, Manish.Jaggi@caviumnetworks.com, Bharat.Bhushan@freescale.com, pranav.sawargaonkar@gmail.com, p.fedin@samsung.com, iommu@lists.linux-foundation.org From: Eric Auger Message-ID: <56D46351.5060701@linaro.org> Date: Mon, 29 Feb 2016 16:27:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Thomas, On 02/26/2016 07:19 PM, Thomas Gleixner wrote: > On Fri, 26 Feb 2016, Eric Auger wrote: >> +static int msi_map_doorbell(struct iommu_domain *d, struct msi_msg *msg) >> +{ >> +#ifdef CONFIG_IOMMU_DMA_RESERVED >> + dma_addr_t iova; >> + phys_addr_t addr; >> + int ret; >> + >> +#ifdef CONFIG_PHYS_ADDR_T_64BIT >> + addr = ((phys_addr_t)(msg->address_hi) << 32) | msg->address_lo; >> +#else >> + addr = msg->address_lo; >> +#endif >> + >> + ret = iommu_get_single_reserved(d, addr, IOMMU_WRITE, &iova); >> + if (!ret) { >> + msg->address_lo = lower_32_bits(iova); >> + msg->address_hi = upper_32_bits(iova); >> + } >> + return ret; >> +#else >> + return -ENODEV; >> +#endif > > This is completely unreadable. Please make this in a way which is parseable. > A few small inline functions do the trick. OK I will rewrite this. > >> +static void msi_unmap_doorbell(struct iommu_domain *d, struct msi_msg *msg) >> +{ >> +#ifdef CONFIG_IOMMU_DMA_RESERVED >> + dma_addr_t iova; >> + >> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT >> + iova = ((dma_addr_t)(msg->address_hi) << 32) | msg->address_lo; >> +#else >> + iova = msg->address_lo; >> +#endif >> + >> + iommu_put_single_reserved(d, iova); >> +#endif > > Ditto. ok > >> +} >> + >> +#ifdef CONFIG_IOMMU_API >> +static struct iommu_domain * >> + irq_data_to_msi_mapping_domain(struct irq_data *irq_data) > > If you split lines, then the function name starts at the beginning of the line > and not at some randome place. ok > >> +{ >> + >> + struct msi_desc *desc; >> + struct device *dev; >> + struct iommu_domain *d; >> + int ret; > > Please order variables by descending length ok > >> + desc = irq_data_get_msi_desc(irq_data); >> + if (!desc) >> + return NULL; >> + >> + dev = msi_desc_to_dev(desc); >> + >> + d = iommu_get_domain_for_dev(dev); >> + if (!d) >> + return NULL; >> + >> + ret = iommu_domain_get_attr(d, DOMAIN_ATTR_MSI_MAPPING, NULL); >> + if (!ret) >> + return d; >> + else >> + return NULL; > > Does anyone except you understand the purpose of the function? Comments have > been invented for a reason. ok I will comment on the role of those functions. > >> +} >> +#else >> +static inline iommu_domain * >> + irq_data_to_msi_mapping_domain(struct irq_data *irq_data) >> +{ >> + return NULL; >> +} >> +#endif /* CONFIG_IOMMU_API */ >> + >> +static int msi_compose(struct irq_data *irq_data, >> + struct msi_msg *msg, bool erase) >> +{ >> + struct msi_msg old_msg; >> + struct iommu_domain *d; >> + int ret = 0; >> + >> + d = irq_data_to_msi_mapping_domain(irq_data); >> + if (unlikely(d)) >> + get_cached_msi_msg(irq_data->irq, &old_msg); >> + >> + if (erase) >> + memset(msg, 0, sizeof(*msg)); >> + else >> + ret = irq_chip_compose_msi_msg(irq_data, msg); >> + >> + if (!d) >> + goto out; >> + >> + /* handle (re-)mapping of MSI doorbell */ >> + if ((old_msg.address_lo != msg->address_lo) || >> + (old_msg.address_hi != msg->address_hi)) >> + msi_unmap_doorbell(d, &old_msg); >> + >> + if (!erase) >> + WARN_ON(msi_map_doorbell(d, msg)); >> + >> +out: >> + return ret; >> +} > > No, this is not the way we do this. You replace existing functionality by some > new fangled thing. which behaves differently. > > This wants to be seperate patches, which first create a wrapper for > irq_chip_compose_msi_msg() and then adds the new functionality to it including > a proper explanation. > > I have no idea how the above is supposed to be the same as the existing code > for the non iommu case. Sure I will decompose things and provide more explanation. Thank you for your time. Best Regards Eric > > Thanks, > > tglx >