From: eric.auger@linaro•org (Eric Auger)
To: linux-arm-kernel@lists•infradead.org
Subject: [RFC v4 09/14] msi: IOMMU map the doorbell address when needed
Date: Mon, 29 Feb 2016 16:27:13 +0100 [thread overview]
Message-ID: <56D46351.5060701@linaro.org> (raw)
In-Reply-To: <alpine.DEB.2.11.1602261907050.3638@nanos>
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
>
next prev parent reply other threads:[~2016-02-29 15:27 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-26 17:35 [RFC v4 00/14] KVM PCIe/MSI passthrough on ARM/ARM64 Eric Auger
2016-02-26 17:35 ` [RFC v4 01/14] iommu: Add DOMAIN_ATTR_MSI_MAPPING attribute Eric Auger
2016-02-26 17:35 ` [RFC v4 02/14] iommu: introduce a reserved iova cookie Eric Auger
2016-02-26 17:35 ` [RFC v4 03/14] dma-reserved-iommu: alloc/free_reserved_iova_domain Eric Auger
2016-02-26 17:35 ` [RFC v4 04/14] dma-reserved-iommu: reserved binding rb-tree and helpers Eric Auger
2016-02-26 17:35 ` [RFC v4 05/14] dma-reserved-iommu: iommu_get/put_single_reserved Eric Auger
2016-02-26 17:35 ` [RFC v4 06/14] dma-reserved-iommu: iommu_unmap_reserved Eric Auger
2016-02-26 17:35 ` [RFC v4 07/14] msi: Add a new MSI_FLAG_IRQ_REMAPPING flag Eric Auger
2016-02-26 18:06 ` Thomas Gleixner
2016-02-29 15:25 ` Eric Auger
2016-02-26 17:35 ` [RFC v4 08/14] msi: export msi_get_domain_info Eric Auger
2016-02-26 17:35 ` [RFC v4 09/14] msi: IOMMU map the doorbell address when needed Eric Auger
2016-02-26 18:19 ` Thomas Gleixner
2016-02-29 15:27 ` Eric Auger [this message]
2016-02-26 17:35 ` [RFC v4 10/14] vfio: introduce VFIO_IOVA_RESERVED vfio_dma type Eric Auger
2016-02-26 17:35 ` [RFC v4 11/14] vfio: allow the user to register reserved iova range for MSI mapping Eric Auger
2016-02-26 17:35 ` [RFC v4 12/14] vfio/type1: also check IRQ remapping capability at msi domain Eric Auger
2016-02-26 17:35 ` [RFC v4 13/14] iommu/arm-smmu: do not advertise IOMMU_CAP_INTR_REMAP Eric Auger
2016-02-26 17:35 ` [RFC v4 14/14] vfio/type1: return MSI mapping requirements with VFIO_IOMMU_GET_INFO Eric Auger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56D46351.5060701@linaro.org \
--to=eric.auger@linaro$(echo .)org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox