From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1423013AbcBZSVV (ORCPT ); Fri, 26 Feb 2016 13:21:21 -0500 Received: from www.linutronix.de ([62.245.132.108]:33119 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1422835AbcBZSVR (ORCPT ); Fri, 26 Feb 2016 13:21:17 -0500 Date: Fri, 26 Feb 2016 19:19:45 +0100 (CET) From: Thomas Gleixner To: Eric Auger 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 Subject: Re: [RFC v4 09/14] msi: IOMMU map the doorbell address when needed In-Reply-To: <1456508154-2253-10-git-send-email-eric.auger@linaro.org> Message-ID: References: <1456508154-2253-1-git-send-email-eric.auger@linaro.org> <1456508154-2253-10-git-send-email-eric.auger@linaro.org> User-Agent: Alpine 2.11 (DEB 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > +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. > +} > + > +#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. > +{ > + > + struct msi_desc *desc; > + struct device *dev; > + struct iommu_domain *d; > + int ret; Please order variables by descending length > + 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. > +} > +#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. Thanks, tglx From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Gleixner Subject: Re: [RFC v4 09/14] msi: IOMMU map the doorbell address when needed Date: Fri, 26 Feb 2016 19:19:45 +0100 (CET) Message-ID: References: <1456508154-2253-1-git-send-email-eric.auger@linaro.org> <1456508154-2253-10-git-send-email-eric.auger@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: eric.auger@st.com, jason@lakedaemon.net, kvm@vger.kernel.org, patches@linaro.org, marc.zyngier@arm.com, joro@8bytes.org, will.deacon@arm.com, linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org, Manish.Jaggi@caviumnetworks.com, alex.williamson@redhat.com, linux-arm-kernel@lists.infradead.org, robin.murphy@arm.com, kvmarm@lists.cs.columbia.edu To: Eric Auger Return-path: In-Reply-To: <1456508154-2253-10-git-send-email-eric.auger@linaro.org> 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 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. > +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. > +} > + > +#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. > +{ > + > + struct msi_desc *desc; > + struct device *dev; > + struct iommu_domain *d; > + int ret; Please order variables by descending length > + 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. > +} > +#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. Thanks, tglx From mboxrd@z Thu Jan 1 00:00:00 1970 From: tglx@linutronix.de (Thomas Gleixner) Date: Fri, 26 Feb 2016 19:19:45 +0100 (CET) Subject: [RFC v4 09/14] msi: IOMMU map the doorbell address when needed In-Reply-To: <1456508154-2253-10-git-send-email-eric.auger@linaro.org> References: <1456508154-2253-1-git-send-email-eric.auger@linaro.org> <1456508154-2253-10-git-send-email-eric.auger@linaro.org> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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. > +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. > +} > + > +#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. > +{ > + > + struct msi_desc *desc; > + struct device *dev; > + struct iommu_domain *d; > + int ret; Please order variables by descending length > + 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. > +} > +#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. Thanks, tglx