| Message ID | 1468933367-23159-10-git-send-email-eric.auger@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 19 Jul 2016, Eric Auger wrote: > /** > + * msi_handle_doorbell_mappings: in case the irq data corresponds to an > + * MSI that requires iommu mapping, traverse the irq domain hierarchy > + * to retrieve the doorbells to handle and iommu_map/unmap them according > + * to @map boolean. > + * > + * @data: irq data handle > + * @map: mapping if true, unmapping if false > + */ Please run that through the kernel doc generator. It does not work that way. The format is: /** * function_name - Short function description * @arg1: Description of arg1 * @argument2: Description of argument2 * * Long explanation including documentation of the return values. */ > +static int msi_handle_doorbell_mappings(struct irq_data *data, bool map) > +{ > + const struct irq_chip_msi_doorbell_info *dbinfo; > + struct iommu_domain *domain; > + struct irq_chip *chip; > + struct device *dev; > + dma_addr_t iova; > + int ret = 0, cpu; > + > + while (data) { > + dev = msi_desc_to_dev(irq_data_get_msi_desc(data)); > + domain = iommu_msi_domain(dev); > + if (domain) { > + chip = irq_data_get_irq_chip(data); > + if (chip->msi_doorbell_info) > + break; > + } > + data = data->parent_data; > + } Please split that out into a seperate function struct irq_data *msi_get_doorbell_info(data) { ..... if (chip->msi_doorbell_info) return chip->msi_get_doorbell_info(data); } return NULL; } info = msi_get_doorbell_info(data); ..... > + if (!data) > + return 0; > + > + dbinfo = chip->msi_doorbell_info(data); > + if (!dbinfo) > + return -EINVAL; > + > + if (!dbinfo->doorbell_is_percpu) { > + if (!map) { > + iommu_msi_put_doorbell_iova(domain, > + dbinfo->global_doorbell); > + return 0; > + } > + return iommu_msi_get_doorbell_iova(domain, > + dbinfo->global_doorbell, > + dbinfo->size, dbinfo->prot, > + &iova); > + } You can spare an indentation level with a helper function if (!dbinfo->doorbell_is_percpu) return msi_map_global_doorbell(domain, dbinfo); > + > + /* percpu doorbells */ > + for_each_possible_cpu(cpu) { > + phys_addr_t __percpu *db_addr = > + per_cpu_ptr(dbinfo->percpu_doorbells, cpu); > + > + if (!map) { > + iommu_msi_put_doorbell_iova(domain, *db_addr); > + } else { > + > + ret = iommu_msi_get_doorbell_iova(domain, *db_addr, > + dbinfo->size, > + dbinfo->prot, &iova); > + if (ret) > + return ret; > + } > + } Same here: for_each_possible_cpu(cpu) { ret = msi_map_percpu_doorbell(domain, cpu); if (ret) return ret; } return 0; Hmm? > + > + return 0; > +} > + > +/** > * msi_domain_alloc_irqs - Allocate interrupts from a MSI interrupt domain > * @domain: The domain to allocate from > * @dev: Pointer to device struct of the device for which the interrupts > @@ -352,17 +423,29 @@ int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev, > > virq = __irq_domain_alloc_irqs(domain, virq, desc->nvec_used, > dev_to_node(dev), &arg, false); > - if (virq < 0) { > - ret = -ENOSPC; > - if (ops->handle_error) > - ret = ops->handle_error(domain, desc, ret); > - if (ops->msi_finish) > - ops->msi_finish(&arg, ret); > - return ret; > - } > + if (virq < 0) > + goto error; > > for (i = 0; i < desc->nvec_used; i++) > irq_set_msi_desc_off(virq, i, desc); > + > + for (i = 0; i < desc->nvec_used; i++) { > + struct irq_data *d = irq_get_irq_data(virq + i); > + > + ret = msi_handle_doorbell_mappings(d, true); > + if (ret) > + break; > + } > + if (ret) { > + for (; i >= 0; i--) { > + struct irq_data *d = irq_get_irq_data(virq + i); > + > + msi_handle_doorbell_mappings(d, false); > + } > + irq_domain_free_irqs(virq, desc->nvec_used); > + desc->irq = 0; > + goto error; How is that supposed to work? You clear desc->irq and then you call ops->handle_error. Why are you adding this extra stuff here? Look at the call sites of msi_domain_alloc_irqs(). All of them use msi_domain_free_irqs() in case of error. There is no reason why you can't do the same.... > /** > @@ -396,6 +486,9 @@ void msi_domain_free_irqs(struct irq_domain *domain, struct device *dev) > * entry. If that's the case, don't do anything. > */ > if (desc->irq) { > + msi_handle_doorbell_mappings( > + irq_get_irq_data(desc->irq), > + false); > irq_domain_free_irqs(desc->irq, desc->nvec_used); > desc->irq = 0; Can you please restructure the code so it reads if (desc->irq) continue; msi_handle_doorbell_mappings(irq_get_irq_data(desc->irq), false); irq_domain_free_irqs(desc->irq, desc->nvec_used); desc->irq = 0; Just blindly whacking stuff into the 80 char limit is not helping readability. Thanks, tglx
Hi Thomas, On 20/07/2016 11:04, Thomas Gleixner wrote: > On Tue, 19 Jul 2016, Eric Auger wrote: >> /** >> + * msi_handle_doorbell_mappings: in case the irq data corresponds to an >> + * MSI that requires iommu mapping, traverse the irq domain hierarchy >> + * to retrieve the doorbells to handle and iommu_map/unmap them according >> + * to @map boolean. >> + * >> + * @data: irq data handle >> + * @map: mapping if true, unmapping if false >> + */ > > > Please run that through the kernel doc generator. It does not work that way. > > The format is: > > /** > * function_name - Short function description > * @arg1: Description of arg1 > * @argument2: Description of argument2 > * > * Long explanation including documentation of the return values. > */ > >> +static int msi_handle_doorbell_mappings(struct irq_data *data, bool map) >> +{ >> + const struct irq_chip_msi_doorbell_info *dbinfo; >> + struct iommu_domain *domain; >> + struct irq_chip *chip; >> + struct device *dev; >> + dma_addr_t iova; >> + int ret = 0, cpu; >> + >> + while (data) { >> + dev = msi_desc_to_dev(irq_data_get_msi_desc(data)); >> + domain = iommu_msi_domain(dev); >> + if (domain) { >> + chip = irq_data_get_irq_chip(data); >> + if (chip->msi_doorbell_info) >> + break; >> + } >> + data = data->parent_data; >> + } > > Please split that out into a seperate function > > struct irq_data *msi_get_doorbell_info(data) > { > ..... > if (chip->msi_doorbell_info) > return chip->msi_get_doorbell_info(data); > } > return NULL; > } > > info = msi_get_doorbell_info(data); > ..... > >> + if (!data) >> + return 0; >> + >> + dbinfo = chip->msi_doorbell_info(data); >> + if (!dbinfo) >> + return -EINVAL; >> + >> + if (!dbinfo->doorbell_is_percpu) { >> + if (!map) { >> + iommu_msi_put_doorbell_iova(domain, >> + dbinfo->global_doorbell); >> + return 0; >> + } >> + return iommu_msi_get_doorbell_iova(domain, >> + dbinfo->global_doorbell, >> + dbinfo->size, dbinfo->prot, >> + &iova); >> + } > > You can spare an indentation level with a helper function > > if (!dbinfo->doorbell_is_percpu) > return msi_map_global_doorbell(domain, dbinfo); > >> + >> + /* percpu doorbells */ >> + for_each_possible_cpu(cpu) { >> + phys_addr_t __percpu *db_addr = >> + per_cpu_ptr(dbinfo->percpu_doorbells, cpu); >> + >> + if (!map) { >> + iommu_msi_put_doorbell_iova(domain, *db_addr); >> + } else { >> + >> + ret = iommu_msi_get_doorbell_iova(domain, *db_addr, >> + dbinfo->size, >> + dbinfo->prot, &iova); >> + if (ret) >> + return ret; >> + } >> + } > > Same here: > > for_each_possible_cpu(cpu) { > ret = msi_map_percpu_doorbell(domain, cpu); > if (ret) > return ret; > } > return 0; > > Hmm? > >> + >> + return 0; >> +} >> + >> +/** >> * msi_domain_alloc_irqs - Allocate interrupts from a MSI interrupt domain >> * @domain: The domain to allocate from >> * @dev: Pointer to device struct of the device for which the interrupts >> @@ -352,17 +423,29 @@ int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev, >> >> virq = __irq_domain_alloc_irqs(domain, virq, desc->nvec_used, >> dev_to_node(dev), &arg, false); >> - if (virq < 0) { >> - ret = -ENOSPC; >> - if (ops->handle_error) >> - ret = ops->handle_error(domain, desc, ret); >> - if (ops->msi_finish) >> - ops->msi_finish(&arg, ret); >> - return ret; >> - } >> + if (virq < 0) >> + goto error; >> >> for (i = 0; i < desc->nvec_used; i++) >> irq_set_msi_desc_off(virq, i, desc); >> + >> + for (i = 0; i < desc->nvec_used; i++) { >> + struct irq_data *d = irq_get_irq_data(virq + i); >> + >> + ret = msi_handle_doorbell_mappings(d, true); >> + if (ret) >> + break; >> + } >> + if (ret) { >> + for (; i >= 0; i--) { >> + struct irq_data *d = irq_get_irq_data(virq + i); >> + >> + msi_handle_doorbell_mappings(d, false); >> + } >> + irq_domain_free_irqs(virq, desc->nvec_used); >> + desc->irq = 0; >> + goto error; > > How is that supposed to work? You clear desc->irq and then you call > ops->handle_error. if I don't clear the desc->irq I enter an infinite loop in pci_enable_msix_range. This happens because msix_capability_init and pcie_enable_msix returns 1. In msix_capability_init, at out_avail: we enumerate the msi_desc which have a non zero irq, hence the returned value equal to 1. Currently the only handle_error ops I found, pci_msi_domain_handle_error does not use irq field so works although questionable. As for the irq_domain_free_irqs I think I can remove it since handled later. How do you advise to handle the above situation? Thanks Eric > > Why are you adding this extra stuff here? Look at the call sites of > msi_domain_alloc_irqs(). All of them use msi_domain_free_irqs() in case of > error. There is no reason why you can't do the same.... > >> /** >> @@ -396,6 +486,9 @@ void msi_domain_free_irqs(struct irq_domain *domain, struct device *dev) >> * entry. If that's the case, don't do anything. >> */ >> if (desc->irq) { >> + msi_handle_doorbell_mappings( >> + irq_get_irq_data(desc->irq), >> + false); >> irq_domain_free_irqs(desc->irq, desc->nvec_used); >> desc->irq = 0; > > Can you please restructure the code so it reads > > if (desc->irq) > continue; > > msi_handle_doorbell_mappings(irq_get_irq_data(desc->irq), > false); > irq_domain_free_irqs(desc->irq, desc->nvec_used); > desc->irq = 0; > > Just blindly whacking stuff into the 80 char limit is not helping readability. > > Thanks, > > tglx >
B1;2802;0cEric, On Mon, 25 Jul 2016, Auger Eric wrote: > On 20/07/2016 11:04, Thomas Gleixner wrote: > > On Tue, 19 Jul 2016, Eric Auger wrote: > >> + if (ret) { > >> + for (; i >= 0; i--) { > >> + struct irq_data *d = irq_get_irq_data(virq + i); > >> + > >> + msi_handle_doorbell_mappings(d, false); > >> + } > >> + irq_domain_free_irqs(virq, desc->nvec_used); > >> + desc->irq = 0; > >> + goto error; > > > > How is that supposed to work? You clear desc->irq and then you call > > ops->handle_error. > > if I don't clear the desc->irq I enter an infinite loop in > pci_enable_msix_range. > > This happens because msix_capability_init and pcie_enable_msix returns 1. > In msix_capability_init, at out_avail: we enumerate the msi_desc which have > a non zero irq, hence the returned value equal to 1. > > Currently the only handle_error ops I found, pci_msi_domain_handle_error > does not use irq field so works although questionable. The logic here is: If the allocation does not succeed for the requested number of interrupts, we tell the caller how many interrupts we were able to set up. So the caller can decide what to do. In your case you don't want to have a partial allocation, so instead of playing silly games with desc->irq you should add a flag which tells the PCI code that you are not interested in a partial allocation and that it should return an error code instead. Something like PCI_DEV_FLAGS_MSI_NO_PARTIAL_ALLOC should do the trick. > As for the irq_domain_free_irqs I think I can remove it since handled later. Not only the free_irqs(). You should let the teardown function handle everything including your doorbell mapping teardown. It's nothing special and free_msi_irqs() at the end of msix_capability_init() will take care of it. Thanks, tglx
Hi Thomas, On 26/07/2016 11:00, Thomas Gleixner wrote: > B1;2802;0cEric, > > On Mon, 25 Jul 2016, Auger Eric wrote: >> On 20/07/2016 11:04, Thomas Gleixner wrote: >>> On Tue, 19 Jul 2016, Eric Auger wrote: >>>> + if (ret) { >>>> + for (; i >= 0; i--) { >>>> + struct irq_data *d = irq_get_irq_data(virq + i); >>>> + >>>> + msi_handle_doorbell_mappings(d, false); >>>> + } >>>> + irq_domain_free_irqs(virq, desc->nvec_used); >>>> + desc->irq = 0; >>>> + goto error; >>> >>> How is that supposed to work? You clear desc->irq and then you call >>> ops->handle_error. >> >> if I don't clear the desc->irq I enter an infinite loop in >> pci_enable_msix_range. >> >> This happens because msix_capability_init and pcie_enable_msix returns 1. >> In msix_capability_init, at out_avail: we enumerate the msi_desc which have >> a non zero irq, hence the returned value equal to 1. >> >> Currently the only handle_error ops I found, pci_msi_domain_handle_error >> does not use irq field so works although questionable. > > The logic here is: If the allocation does not succeed for the requested number > of interrupts, we tell the caller how many interrupts we were able to set up. > So the caller can decide what to do. > > In your case you don't want to have a partial allocation, so instead of > playing silly games with desc->irq you should add a flag which tells the PCI > code that you are not interested in a partial allocation and that it should > return an error code instead. In that case can we consider we even succeeded in allocating 1 MSI? In case the IOMMU mapping fails, the MSI transaction will never reach the target MSI frame so it is not usable. So when you mean "partial" I understand we did not succeed in allocating maxvec IRQs, correct? Here we succeeded in allocating 0 IRQ and still msi_capability_init returns 1. msi_capability_init doc-comment says "a positive return value indicates the number of interrupts which could have been allocated." I understand allocation success currently only depends on the fact virq was allocated and set to desc->irq. But with that IOMMU stuff doesn't the criteria changes? > Something like PCI_DEV_FLAGS_MSI_NO_PARTIAL_ALLOC should do the trick. > >> As for the irq_domain_free_irqs I think I can remove it since handled later. > > Not only the free_irqs(). You should let the teardown function handle > everything including your doorbell mapping teardown. It's nothing special and > free_msi_irqs() at the end of msix_capability_init() will take care of it. Yep I was forced to call free_irqs myself since free_msi_irqs was doing nothing due the fact I resetted the irq field. Wrong thing loop ;-) Thanks Eric > > Thanks, > > tglx >
On Tue, 26 Jul 2016, Auger Eric wrote: > On 26/07/2016 11:00, Thomas Gleixner wrote: > > In your case you don't want to have a partial allocation, so instead of > > playing silly games with desc->irq you should add a flag which tells the PCI > > code that you are not interested in a partial allocation and that it should > > return an error code instead. > > In that case can we consider we even succeeded in allocating 1 MSI? In case > the IOMMU mapping fails, the MSI transaction will never reach the target MSI > frame so it is not usable. So when you mean "partial" I understand we did > not succeed in allocating maxvec IRQs, correct? Here we succeeded in > allocating 0 IRQ and still msi_capability_init returns 1. > > msi_capability_init doc-comment says "a positive return value indicates the > number of interrupts which could have been allocated." > > I understand allocation success currently only depends on the fact virq was > allocated and set to desc->irq. But with that IOMMU stuff doesn't the > criteria changes? Right. But then you need to express it differently in a consistent way. Not by hacking around it by setting desc->irq to 0. Something like a flag field in msi_desc which denotes various properties would be a possible solution. MSI_IRQ_ALLOCATED and MSI_IRQ_REMAPPED would be sufficient for now. And the deallocation/cleanup would rely on those flags rather than checking desc->irq. Thanks, tglx
diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c index 72bf4d6..69b5b19 100644 --- a/kernel/irq/msi.c +++ b/kernel/irq/msi.c @@ -14,6 +14,9 @@ #include <linux/irq.h> #include <linux/irqdomain.h> #include <linux/msi.h> +#include <linux/msi-iommu.h> +#include <linux/iommu.h> +#include <linux/msi-doorbell.h> /* Temparory solution for building, will be removed later */ #include <linux/pci.h> @@ -322,6 +325,74 @@ int msi_domain_populate_irqs(struct irq_domain *domain, struct device *dev, } /** + * msi_handle_doorbell_mappings: in case the irq data corresponds to an + * MSI that requires iommu mapping, traverse the irq domain hierarchy + * to retrieve the doorbells to handle and iommu_map/unmap them according + * to @map boolean. + * + * @data: irq data handle + * @map: mapping if true, unmapping if false + */ +static int msi_handle_doorbell_mappings(struct irq_data *data, bool map) +{ + const struct irq_chip_msi_doorbell_info *dbinfo; + struct iommu_domain *domain; + struct irq_chip *chip; + struct device *dev; + dma_addr_t iova; + int ret = 0, cpu; + + while (data) { + dev = msi_desc_to_dev(irq_data_get_msi_desc(data)); + domain = iommu_msi_domain(dev); + if (domain) { + chip = irq_data_get_irq_chip(data); + if (chip->msi_doorbell_info) + break; + } + data = data->parent_data; + } + + if (!data) + return 0; + + dbinfo = chip->msi_doorbell_info(data); + if (!dbinfo) + return -EINVAL; + + if (!dbinfo->doorbell_is_percpu) { + if (!map) { + iommu_msi_put_doorbell_iova(domain, + dbinfo->global_doorbell); + return 0; + } + return iommu_msi_get_doorbell_iova(domain, + dbinfo->global_doorbell, + dbinfo->size, dbinfo->prot, + &iova); + } + + /* percpu doorbells */ + for_each_possible_cpu(cpu) { + phys_addr_t __percpu *db_addr = + per_cpu_ptr(dbinfo->percpu_doorbells, cpu); + + if (!map) { + iommu_msi_put_doorbell_iova(domain, *db_addr); + } else { + + ret = iommu_msi_get_doorbell_iova(domain, *db_addr, + dbinfo->size, + dbinfo->prot, &iova); + if (ret) + return ret; + } + } + + return 0; +} + +/** * msi_domain_alloc_irqs - Allocate interrupts from a MSI interrupt domain * @domain: The domain to allocate from * @dev: Pointer to device struct of the device for which the interrupts @@ -352,17 +423,29 @@ int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev, virq = __irq_domain_alloc_irqs(domain, virq, desc->nvec_used, dev_to_node(dev), &arg, false); - if (virq < 0) { - ret = -ENOSPC; - if (ops->handle_error) - ret = ops->handle_error(domain, desc, ret); - if (ops->msi_finish) - ops->msi_finish(&arg, ret); - return ret; - } + if (virq < 0) + goto error; for (i = 0; i < desc->nvec_used; i++) irq_set_msi_desc_off(virq, i, desc); + + for (i = 0; i < desc->nvec_used; i++) { + struct irq_data *d = irq_get_irq_data(virq + i); + + ret = msi_handle_doorbell_mappings(d, true); + if (ret) + break; + } + if (ret) { + for (; i >= 0; i--) { + struct irq_data *d = irq_get_irq_data(virq + i); + + msi_handle_doorbell_mappings(d, false); + } + irq_domain_free_irqs(virq, desc->nvec_used); + desc->irq = 0; + goto error; + } } if (ops->msi_finish) @@ -377,6 +460,13 @@ int msi_domain_alloc_irqs(struct irq_domain *domain, struct device *dev, } return 0; +error: + ret = -ENOSPC; + if (ops->handle_error) + ret = ops->handle_error(domain, desc, ret); + if (ops->msi_finish) + ops->msi_finish(&arg, ret); + return ret; } /** @@ -396,6 +486,9 @@ void msi_domain_free_irqs(struct irq_domain *domain, struct device *dev) * entry. If that's the case, don't do anything. */ if (desc->irq) { + msi_handle_doorbell_mappings( + irq_get_irq_data(desc->irq), + false); irq_domain_free_irqs(desc->irq, desc->nvec_used); desc->irq = 0; }
This patch handles the iommu mapping of MSI doorbells that require to be mapped in an iommu domain. This happens on msi_domain_alloc/free_irqs since this is called in code that can sleep (pci_enable/disable_msi): iommu_map/unmap is not stated as atomic. On msi_domain_(de)activate and msi_domain_set_affinity, which must be atomic, we just lookup for this pre-allocated/mapped IOVA. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- v10 -> v11: - restore v9 version based on irq_chip msi_doorbell_info v9 -> v10: - use irqchip API to lookup for the chip_data's doorbell v8 -> v9: - decouple irq_data parsing from the actual mapping/unmapping v7 -> v8: - new percpu pointer type - exit from the irq domain hierarchy parsing on first map/unmap success - reset desc->irq to 0 on mapping failure v7: creation --- kernel/irq/msi.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 101 insertions(+), 8 deletions(-)