| Message ID | b52cdefb7a3f631340b98dcd38a274ae277bd561.1531239023.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Tue, Jul 10, 2018 at 05:13:45PM +0100, Robin Murphy wrote: > IORT revision D allows PCI root complex nodes to specify a memory > address size limit equivalently to named components, to help describe > straightforward integrations which don't really warrant a full-blown > _DMA method. Now that our headers are up-to-date, plumb it in. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/acpi/arm64/iort.c | 25 +++++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index 7a3a541046ed..4a66896e2aa3 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -947,6 +947,24 @@ static int nc_dma_get_range(struct device *dev, u64 *size) > return 0; > } > > +static int rc_dma_get_range(struct device *dev, u64 *size) > +{ > + struct acpi_iort_node *node; > + struct acpi_iort_root_complex *rc; > + > + node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX, > + iort_match_node_callback, dev); > + if (!node || node->revision < 1) > + return -ENODEV; > + > + rc = (struct acpi_iort_root_complex *)node->node_data; > + > + *size = rc->memory_address_limit >= 64 ? U64_MAX : > + 1ULL<<rc->memory_address_limit; > + > + return 0; > +} > + > /** > * iort_dma_setup() - Set-up device DMA parameters. > * > @@ -975,10 +993,13 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) > > size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1); > > - if (dev_is_pci(dev)) > + if (dev_is_pci(dev)) { > ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size); > - else > + if (ret == -ENODEV) > + ret = rc_dma_get_range(dev, &size); Thank you for putting together the patch. The question is whether it is OK to ignore the IORT address limits when _DMA is actually specified. It is a sort of grey area that has to be clarified, maybe we can add a check to detect a size mismatch, I do not know if something should be added at IORT spec level to clarify its relation to the _DMA object, if present. Thanks, Lorenzo > + } else { > ret = nc_dma_get_range(dev, &size); > + } > > if (!ret) { > msb = fls64(dmaaddr + size - 1); > -- > 2.17.1.dirty > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-07-16 4:10 PM, Lorenzo Pieralisi wrote: > On Tue, Jul 10, 2018 at 05:13:45PM +0100, Robin Murphy wrote: >> IORT revision D allows PCI root complex nodes to specify a memory >> address size limit equivalently to named components, to help describe >> straightforward integrations which don't really warrant a full-blown >> _DMA method. Now that our headers are up-to-date, plumb it in. >> >> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >> --- >> drivers/acpi/arm64/iort.c | 25 +++++++++++++++++++++++-- >> 1 file changed, 23 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c >> index 7a3a541046ed..4a66896e2aa3 100644 >> --- a/drivers/acpi/arm64/iort.c >> +++ b/drivers/acpi/arm64/iort.c >> @@ -947,6 +947,24 @@ static int nc_dma_get_range(struct device *dev, u64 *size) >> return 0; >> } >> >> +static int rc_dma_get_range(struct device *dev, u64 *size) >> +{ >> + struct acpi_iort_node *node; >> + struct acpi_iort_root_complex *rc; >> + >> + node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX, >> + iort_match_node_callback, dev); >> + if (!node || node->revision < 1) >> + return -ENODEV; >> + >> + rc = (struct acpi_iort_root_complex *)node->node_data; >> + >> + *size = rc->memory_address_limit >= 64 ? U64_MAX : >> + 1ULL<<rc->memory_address_limit; >> + >> + return 0; >> +} >> + >> /** >> * iort_dma_setup() - Set-up device DMA parameters. >> * >> @@ -975,10 +993,13 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) >> >> size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1); >> >> - if (dev_is_pci(dev)) >> + if (dev_is_pci(dev)) { >> ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size); >> - else >> + if (ret == -ENODEV) >> + ret = rc_dma_get_range(dev, &size); > > Thank you for putting together the patch. > > The question is whether it is OK to ignore the IORT address limits > when _DMA is actually specified. It is a sort of grey area that > has to be clarified, maybe we can add a check to detect a size > mismatch, I do not know if something should be added at IORT spec > level to clarify its relation to the _DMA object, if present. Yeah, I'm assuming that _DMA would be used to describe conditions more specific than the simple address size limit (i.e. bridge windows), so even if both are present, the range inferred from _DMA will always be less than or equal to that inferred from IORT, and thus rather than explicitly calculating the intersection of the two we can simply do this short-circuit. If IORT accurately reflects the total number of usable address bits, then I can't see that it would ever make sense for _DMA to specify an address range which exceeds that; I guess it comes down to how much effort we want to spend verifying firmware instead of trusting it. Robin. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[+Catalin, Will] On Mon, Jul 16, 2018 at 04:34:51PM +0100, Robin Murphy wrote: > On 2018-07-16 4:10 PM, Lorenzo Pieralisi wrote: > >On Tue, Jul 10, 2018 at 05:13:45PM +0100, Robin Murphy wrote: > >>IORT revision D allows PCI root complex nodes to specify a memory > >>address size limit equivalently to named components, to help describe > >>straightforward integrations which don't really warrant a full-blown > >>_DMA method. Now that our headers are up-to-date, plumb it in. > >> > >>Signed-off-by: Robin Murphy <robin.murphy@arm.com> > >>--- > >> drivers/acpi/arm64/iort.c | 25 +++++++++++++++++++++++-- > >> 1 file changed, 23 insertions(+), 2 deletions(-) > >> > >>diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > >>index 7a3a541046ed..4a66896e2aa3 100644 > >>--- a/drivers/acpi/arm64/iort.c > >>+++ b/drivers/acpi/arm64/iort.c > >>@@ -947,6 +947,24 @@ static int nc_dma_get_range(struct device *dev, u64 *size) > >> return 0; > >> } > >>+static int rc_dma_get_range(struct device *dev, u64 *size) > >>+{ > >>+ struct acpi_iort_node *node; > >>+ struct acpi_iort_root_complex *rc; > >>+ > >>+ node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX, > >>+ iort_match_node_callback, dev); > >>+ if (!node || node->revision < 1) > >>+ return -ENODEV; > >>+ > >>+ rc = (struct acpi_iort_root_complex *)node->node_data; > >>+ > >>+ *size = rc->memory_address_limit >= 64 ? U64_MAX : > >>+ 1ULL<<rc->memory_address_limit; > >>+ > >>+ return 0; > >>+} > >>+ > >> /** > >> * iort_dma_setup() - Set-up device DMA parameters. > >> * > >>@@ -975,10 +993,13 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) > >> size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1); > >>- if (dev_is_pci(dev)) > >>+ if (dev_is_pci(dev)) { > >> ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size); > >>- else > >>+ if (ret == -ENODEV) > >>+ ret = rc_dma_get_range(dev, &size); > > > >Thank you for putting together the patch. > > > >The question is whether it is OK to ignore the IORT address limits > >when _DMA is actually specified. It is a sort of grey area that > >has to be clarified, maybe we can add a check to detect a size > >mismatch, I do not know if something should be added at IORT spec > >level to clarify its relation to the _DMA object, if present. > > Yeah, I'm assuming that _DMA would be used to describe conditions > more specific than the simple address size limit (i.e. bridge > windows), so even if both are present, the range inferred from _DMA > will always be less than or equal to that inferred from IORT, and > thus rather than explicitly calculating the intersection of the two > we can simply do this short-circuit. > > If IORT accurately reflects the total number of usable address bits, > then I can't see that it would ever make sense for _DMA to specify > an address range which exceeds that; I guess it comes down to how > much effort we want to spend verifying firmware instead of trusting > it. I agree with this reasoning and the patch looks fine, I have not queued anything for this cycle for IORT so I would ask Will/Catalin to pick it up (if we still have time for v4.19): Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018/7/11 0:13, Robin Murphy wrote: > IORT revision D allows PCI root complex nodes to specify a memory > address size limit equivalently to named components, to help describe > straightforward integrations which don't really warrant a full-blown > _DMA method. Now that our headers are up-to-date, plumb it in. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/acpi/arm64/iort.c | 25 +++++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c > index 7a3a541046ed..4a66896e2aa3 100644 > --- a/drivers/acpi/arm64/iort.c > +++ b/drivers/acpi/arm64/iort.c > @@ -947,6 +947,24 @@ static int nc_dma_get_range(struct device *dev, u64 *size) > return 0; > } > > +static int rc_dma_get_range(struct device *dev, u64 *size) > +{ > + struct acpi_iort_node *node; > + struct acpi_iort_root_complex *rc; > + > + node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX, > + iort_match_node_callback, dev); > + if (!node || node->revision < 1) > + return -ENODEV; > + > + rc = (struct acpi_iort_root_complex *)node->node_data; > + > + *size = rc->memory_address_limit >= 64 ? U64_MAX : > + 1ULL<<rc->memory_address_limit; > + > + return 0; > +} > + > /** > * iort_dma_setup() - Set-up device DMA parameters. > * > @@ -975,10 +993,13 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) > > size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1); > > - if (dev_is_pci(dev)) > + if (dev_is_pci(dev)) { > ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size); > - else > + if (ret == -ENODEV) > + ret = rc_dma_get_range(dev, &size); > + } else { > ret = nc_dma_get_range(dev, &size); > + } > > if (!ret) { > msb = fls64(dmaaddr + size - 1); > Looks good to me, Reviewed-by: Hanjun Guo <hanjun.guo@linaro.org> Thanks Hanjun -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[+Christoph] On 18/07/18 17:36, Lorenzo Pieralisi wrote: > [+Catalin, Will] > > On Mon, Jul 16, 2018 at 04:34:51PM +0100, Robin Murphy wrote: >> On 2018-07-16 4:10 PM, Lorenzo Pieralisi wrote: >>> On Tue, Jul 10, 2018 at 05:13:45PM +0100, Robin Murphy wrote: >>>> IORT revision D allows PCI root complex nodes to specify a memory >>>> address size limit equivalently to named components, to help describe >>>> straightforward integrations which don't really warrant a full-blown >>>> _DMA method. Now that our headers are up-to-date, plumb it in. >>>> >>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >>>> --- >>>> drivers/acpi/arm64/iort.c | 25 +++++++++++++++++++++++-- >>>> 1 file changed, 23 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c >>>> index 7a3a541046ed..4a66896e2aa3 100644 >>>> --- a/drivers/acpi/arm64/iort.c >>>> +++ b/drivers/acpi/arm64/iort.c >>>> @@ -947,6 +947,24 @@ static int nc_dma_get_range(struct device *dev, u64 *size) >>>> return 0; >>>> } >>>> +static int rc_dma_get_range(struct device *dev, u64 *size) >>>> +{ >>>> + struct acpi_iort_node *node; >>>> + struct acpi_iort_root_complex *rc; >>>> + >>>> + node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX, >>>> + iort_match_node_callback, dev); >>>> + if (!node || node->revision < 1) >>>> + return -ENODEV; >>>> + >>>> + rc = (struct acpi_iort_root_complex *)node->node_data; >>>> + >>>> + *size = rc->memory_address_limit >= 64 ? U64_MAX : >>>> + 1ULL<<rc->memory_address_limit; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> /** >>>> * iort_dma_setup() - Set-up device DMA parameters. >>>> * >>>> @@ -975,10 +993,13 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) >>>> size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1); >>>> - if (dev_is_pci(dev)) >>>> + if (dev_is_pci(dev)) { >>>> ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size); >>>> - else >>>> + if (ret == -ENODEV) >>>> + ret = rc_dma_get_range(dev, &size); >>> >>> Thank you for putting together the patch. >>> >>> The question is whether it is OK to ignore the IORT address limits >>> when _DMA is actually specified. It is a sort of grey area that >>> has to be clarified, maybe we can add a check to detect a size >>> mismatch, I do not know if something should be added at IORT spec >>> level to clarify its relation to the _DMA object, if present. >> >> Yeah, I'm assuming that _DMA would be used to describe conditions >> more specific than the simple address size limit (i.e. bridge >> windows), so even if both are present, the range inferred from _DMA >> will always be less than or equal to that inferred from IORT, and >> thus rather than explicitly calculating the intersection of the two >> we can simply do this short-circuit. >> >> If IORT accurately reflects the total number of usable address bits, >> then I can't see that it would ever make sense for _DMA to specify >> an address range which exceeds that; I guess it comes down to how >> much effort we want to spend verifying firmware instead of trusting >> it. > > I agree with this reasoning and the patch looks fine, I have not > queued anything for this cycle for IORT so I would ask Will/Catalin > to pick it up (if we still have time for v4.19): > > Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cheers Lorenzo (and Hanjun). Given that my DMA mask series[1] is nominally based on top of this, it might make sense for Christoph to pick it up through the dma-mapping tree. Since I'm about to send a new version of that series I'll resend this one as part of that. Thanks, Robin. [1] https://www.mail-archive.com/iommu@lists.linux-foundation.org/msg24358.html -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c index 7a3a541046ed..4a66896e2aa3 100644 --- a/drivers/acpi/arm64/iort.c +++ b/drivers/acpi/arm64/iort.c @@ -947,6 +947,24 @@ static int nc_dma_get_range(struct device *dev, u64 *size) return 0; } +static int rc_dma_get_range(struct device *dev, u64 *size) +{ + struct acpi_iort_node *node; + struct acpi_iort_root_complex *rc; + + node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX, + iort_match_node_callback, dev); + if (!node || node->revision < 1) + return -ENODEV; + + rc = (struct acpi_iort_root_complex *)node->node_data; + + *size = rc->memory_address_limit >= 64 ? U64_MAX : + 1ULL<<rc->memory_address_limit; + + return 0; +} + /** * iort_dma_setup() - Set-up device DMA parameters. * @@ -975,10 +993,13 @@ void iort_dma_setup(struct device *dev, u64 *dma_addr, u64 *dma_size) size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1); - if (dev_is_pci(dev)) + if (dev_is_pci(dev)) { ret = acpi_dma_get_range(dev, &dmaaddr, &offset, &size); - else + if (ret == -ENODEV) + ret = rc_dma_get_range(dev, &size); + } else { ret = nc_dma_get_range(dev, &size); + } if (!ret) { msb = fls64(dmaaddr + size - 1);
IORT revision D allows PCI root complex nodes to specify a memory address size limit equivalently to named components, to help describe straightforward integrations which don't really warrant a full-blown _DMA method. Now that our headers are up-to-date, plumb it in. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- drivers/acpi/arm64/iort.c | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-)