Re: [PATCH v9 06/11] ACPI/IORT: Add support to retrieve IORT RMR reserved regions
On Tue, Apr 19, 2022 at 08:31:55AM +, Shameerali Kolothum Thodi via iommu wrote: > Sorry for the delayed response(was on holidays). So if we move the > iommu_dma_put_resv_region() call to generic_iommu_put_resv_regions() , > will that address the concerns here? > > I think it will resolve the issue in 05/11 as well pointed out by Christoph > where we end up not releasing reserved regions when > CONFIG_IOMMU_DMA is not set. As Robin pointed out we might not really deduct that ACPI means RMR. I suspect the best would be to just attach a free callback to the regions. Either by adding it directly to struct iommu_resv_region if we thing there are few enough regions to not be worried about the memory use, or other by adding a container struct for the list_head that contains the free callback. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v9 06/11] ACPI/IORT: Add support to retrieve IORT RMR reserved regions
> -Original Message-
> From: Christoph Hellwig [mailto:h...@infradead.org]
> Sent: 07 April 2022 15:00
> To: Robin Murphy
> Cc: Christoph Hellwig ; Shameerali Kolothum Thodi
> ; j...@solid-run.com; Linuxarm
> ; steven.pr...@arm.com; linux-a...@vger.kernel.org;
> iommu@lists.linux-foundation.org; wanghuiqiang
> ; Guohanjun (Hanjun Guo)
> ; yangyicong ;
> sami.muja...@arm.com; w...@kernel.org;
> linux-arm-ker...@lists.infradead.org
> Subject: Re: [PATCH v9 06/11] ACPI/IORT: Add support to retrieve IORT RMR
> reserved regions
>
> On Thu, Apr 07, 2022 at 02:53:38PM +0100, Robin Murphy wrote:
> > > Why can't this just go into generic_iommu_put_resv_regions? The idea
> > > that the iommu low-level drivers need to call into dma-iommu which is
> > > a consumer of the IOMMU API is odd. Especially if that just calls out
> > > to ACPI code and generic IOMMU code only anyway.
> >
> > Because assuming ACPI means IORT is not generic. Part of the aim in adding
> > the union to iommu_resv_region is that stuff like AMD's unity_map_entry
> and
> > Intel's dmar_rmrr_unit can be folded into it as well, and their reserved
> > region handling correspondingly simplified too.
> >
> > The iommu_dma_{get,put}_resv_region() helpers are kind of intended to be
> > specific to the fwnode mechanism which deals with IORT and devicetree
> (once
> > the reserved region bindings are fully worked out).
>
> But IORT is not driverâ‚‹specific code. So we'll need a USE_IORT flag
> somewhere in core IOMMU code instead of trying to stuff this into
> driver operations. and dma-iommu mostly certainly implies IORT even
> less than ACPI.
Sorry for the delayed response(was on holidays). So if we move the
iommu_dma_put_resv_region() call to generic_iommu_put_resv_regions() ,
will that address the concerns here?
I think it will resolve the issue in 05/11 as well pointed out by Christoph
where we end up not releasing reserved regions when
CONFIG_IOMMU_DMA is not set.
Something like below,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f2c45b85b9fc..d08204a25ba8 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2605,6 +2605,8 @@ void generic_iommu_put_resv_regions(struct
device *dev, struct list_head *list)
{
struct iommu_resv_region *entry, *next;
+ iommu_dma_put_resv_regions(dev, list);
+
list_for_each_entry_safe(entry, next, list, list)
kfree(entry);
}
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 5811233dc9fb..8cb1e419db49 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -393,8 +393,6 @@ void iommu_dma_put_resv_regions(struct device
*dev, struct list_head *list)
{
if (!is_of_node(dev_iommu_fwspec_get(dev)->iommu_fwnode))
iort_iommu_put_resv_regions(dev, list);
-
- generic_iommu_put_resv_regions(dev, list);
}
Please let me know if this is not good enough.
Thanks,
Shameer
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 06/11] ACPI/IORT: Add support to retrieve IORT RMR reserved regions
On Thu, Apr 07, 2022 at 02:53:38PM +0100, Robin Murphy wrote:
> > Why can't this just go into generic_iommu_put_resv_regions? The idea
> > that the iommu low-level drivers need to call into dma-iommu which is
> > a consumer of the IOMMU API is odd. Especially if that just calls out
> > to ACPI code and generic IOMMU code only anyway.
>
> Because assuming ACPI means IORT is not generic. Part of the aim in adding
> the union to iommu_resv_region is that stuff like AMD's unity_map_entry and
> Intel's dmar_rmrr_unit can be folded into it as well, and their reserved
> region handling correspondingly simplified too.
>
> The iommu_dma_{get,put}_resv_region() helpers are kind of intended to be
> specific to the fwnode mechanism which deals with IORT and devicetree (once
> the reserved region bindings are fully worked out).
But IORT is not driverâ‚‹specific code. So we'll need a USE_IORT flag
somewhere in core IOMMU code instead of trying to stuff this into
driver operations. and dma-iommu mostly certainly implies IORT even
less than ACPI.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 06/11] ACPI/IORT: Add support to retrieve IORT RMR reserved regions
On 2022-04-07 14:28, Christoph Hellwig wrote:
+static void iort_rmr_desc_check_overlap(struct acpi_iort_rmr_desc *desc, u32
count)
Overly long line.
void iommu_dma_put_resv_regions(struct device *dev, struct list_head *list)
{
+ if (!is_of_node(dev_iommu_fwspec_get(dev)->iommu_fwnode))
+ iort_iommu_put_resv_regions(dev, list);
+
generic_iommu_put_resv_regions(dev, list);
}
Why can't this just go into generic_iommu_put_resv_regions? The idea
that the iommu low-level drivers need to call into dma-iommu which is
a consumer of the IOMMU API is odd. Especially if that just calls out
to ACPI code and generic IOMMU code only anyway.
Because assuming ACPI means IORT is not generic. Part of the aim in
adding the union to iommu_resv_region is that stuff like AMD's
unity_map_entry and Intel's dmar_rmrr_unit can be folded into it as
well, and their reserved region handling correspondingly simplified too.
The iommu_dma_{get,put}_resv_region() helpers are kind of intended to be
specific to the fwnode mechanism which deals with IORT and devicetree
(once the reserved region bindings are fully worked out).
Thanks,
Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 06/11] ACPI/IORT: Add support to retrieve IORT RMR reserved regions
> +static void iort_rmr_desc_check_overlap(struct acpi_iort_rmr_desc *desc, u32
> count)
Overly long line.
> void iommu_dma_put_resv_regions(struct device *dev, struct list_head *list)
> {
> + if (!is_of_node(dev_iommu_fwspec_get(dev)->iommu_fwnode))
> + iort_iommu_put_resv_regions(dev, list);
> +
> generic_iommu_put_resv_regions(dev, list);
> }
Why can't this just go into generic_iommu_put_resv_regions? The idea
that the iommu low-level drivers need to call into dma-iommu which is
a consumer of the IOMMU API is odd. Especially if that just calls out
to ACPI code and generic IOMMU code only anyway.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 06/11] ACPI/IORT: Add support to retrieve IORT RMR reserved regions
On Mon, Apr 04, 2022 at 01:42:04PM +0100, Shameer Kolothum wrote:
> Parse through the IORT RMR nodes and populate the reserve region list
> corresponding to a given IOMMU and device(optional). Also, go through
> the ID mappings of the RMR node and retrieve all the SIDs associated
> with it.
>
> Now that we have this support, update iommu_dma_get/_put_resv_regions()
> paths to include the RMR reserve regions.
>
> Signed-off-by: Shameer Kolothum
> ---
> drivers/acpi/arm64/iort.c | 275 ++
> drivers/iommu/dma-iommu.c | 3 +
> include/linux/acpi_iort.h | 4 +
> 3 files changed, 282 insertions(+)
Very minor style comment below, regardless:
Reviewed-by: Lorenzo Pieralisi # for ACPI
IORT
>
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 63acc3c5b275..1147387cfddb 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -812,6 +812,259 @@ void acpi_configure_pmsi_domain(struct device *dev)
> }
>
> #ifdef CONFIG_IOMMU_API
> +static void iort_rmr_desc_check_overlap(struct acpi_iort_rmr_desc *desc, u32
> count)
> +{
> + int i, j;
> +
> + for (i = 0; i < count; i++) {
> + u64 end, start = desc[i].base_address, length = desc[i].length;
> +
> + if (!length) {
> + pr_err(FW_BUG "RMR descriptor[0x%llx] with zero length,
> continue anyway\n",
> +start);
> + continue;
> + }
> +
> + end = start + length - 1;
> +
> + /* Check for address overlap */
> + for (j = i + 1; j < count; j++) {
> + u64 e_start = desc[j].base_address;
> + u64 e_end = e_start + desc[j].length - 1;
> +
> + if (start <= e_end && end >= e_start)
> + pr_err(FW_BUG "RMR descriptor[0x%llx - 0x%llx]
> overlaps, continue anyway\n",
> +start, end);
> + }
> + }
> +}
> +
> +/*
> + * Please note, we will keep the already allocated RMR reserve
> + * regions in case of a memory allocation failure.
> + */
> +static void iort_get_rmrs(struct acpi_iort_node *node,
> + struct acpi_iort_node *smmu,
> + u32 *sids, u32 num_sids,
> + struct list_head *head)
> +{
> + struct acpi_iort_rmr *rmr = (struct acpi_iort_rmr *)node->node_data;
> + struct acpi_iort_rmr_desc *rmr_desc;
> + int i;
> +
> + rmr_desc = ACPI_ADD_PTR(struct acpi_iort_rmr_desc, node,
> + rmr->rmr_offset);
> +
> + iort_rmr_desc_check_overlap(rmr_desc, rmr->rmr_count);
> +
> + for (i = 0; i < rmr->rmr_count; i++, rmr_desc++) {
> + struct iommu_resv_region *region;
> + enum iommu_resv_type type;
> + u32 *sids_copy;
> + int prot = IOMMU_READ | IOMMU_WRITE;
> + u64 addr = rmr_desc->base_address, size = rmr_desc->length;
> +
> + if (!IS_ALIGNED(addr, SZ_64K) || !IS_ALIGNED(size, SZ_64K)) {
> + /* PAGE align base addr and size */
> + addr &= PAGE_MASK;
> + size = PAGE_ALIGN(size +
> offset_in_page(rmr_desc->base_address));
> +
> + pr_err(FW_BUG "RMR descriptor[0x%llx - 0x%llx] not
> aligned to 64K, continue with [0x%llx - 0x%llx]\n",
> +rmr_desc->base_address,
> +rmr_desc->base_address + rmr_desc->length - 1,
> +addr, addr + size - 1);
> + }
> +
> + if (rmr->flags & ACPI_IORT_RMR_REMAP_PERMITTED)
> + type = IOMMU_RESV_DIRECT_RELAXABLE;
> + else
> + type = IOMMU_RESV_DIRECT;
> +
> + if (rmr->flags & ACPI_IORT_RMR_ACCESS_PRIVILEGE)
> + prot |= IOMMU_PRIV;
> +
> + /* Attributes 0x00 - 0x03 represents device memory */
> + if (ACPI_IORT_RMR_ACCESS_ATTRIBUTES(rmr->flags) <=
> + ACPI_IORT_RMR_ATTR_DEVICE_GRE)
> + prot |= IOMMU_MMIO;
> + else if (ACPI_IORT_RMR_ACCESS_ATTRIBUTES(rmr->flags) ==
> + ACPI_IORT_RMR_ATTR_NORMAL_IWB_OWB)
> + prot |= IOMMU_CACHE;
> +
> + /* Create a copy of SIDs array to associate with this resv
> region */
> + sids_copy = kmemdup(sids, num_sids * sizeof(*sids), GFP_KERNEL);
> + if (!sids_copy)
> + return;
> +
> + region = iommu_alloc_resv_region(addr, size, prot, type);
> + if (!region) {
> + kfree(sids_copy);
> + return;
> + }
> +
> + region->fw_data.rmr.sids = sids_copy;
> + region->fw_data.rmr.num_sids = num_sids;
> + list_add_tail(>list, head);
> + }
[PATCH v9 06/11] ACPI/IORT: Add support to retrieve IORT RMR reserved regions
Parse through the IORT RMR nodes and populate the reserve region list
corresponding to a given IOMMU and device(optional). Also, go through
the ID mappings of the RMR node and retrieve all the SIDs associated
with it.
Now that we have this support, update iommu_dma_get/_put_resv_regions()
paths to include the RMR reserve regions.
Signed-off-by: Shameer Kolothum
---
drivers/acpi/arm64/iort.c | 275 ++
drivers/iommu/dma-iommu.c | 3 +
include/linux/acpi_iort.h | 4 +
3 files changed, 282 insertions(+)
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 63acc3c5b275..1147387cfddb 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -812,6 +812,259 @@ void acpi_configure_pmsi_domain(struct device *dev)
}
#ifdef CONFIG_IOMMU_API
+static void iort_rmr_desc_check_overlap(struct acpi_iort_rmr_desc *desc, u32
count)
+{
+ int i, j;
+
+ for (i = 0; i < count; i++) {
+ u64 end, start = desc[i].base_address, length = desc[i].length;
+
+ if (!length) {
+ pr_err(FW_BUG "RMR descriptor[0x%llx] with zero length,
continue anyway\n",
+ start);
+ continue;
+ }
+
+ end = start + length - 1;
+
+ /* Check for address overlap */
+ for (j = i + 1; j < count; j++) {
+ u64 e_start = desc[j].base_address;
+ u64 e_end = e_start + desc[j].length - 1;
+
+ if (start <= e_end && end >= e_start)
+ pr_err(FW_BUG "RMR descriptor[0x%llx - 0x%llx]
overlaps, continue anyway\n",
+ start, end);
+ }
+ }
+}
+
+/*
+ * Please note, we will keep the already allocated RMR reserve
+ * regions in case of a memory allocation failure.
+ */
+static void iort_get_rmrs(struct acpi_iort_node *node,
+ struct acpi_iort_node *smmu,
+ u32 *sids, u32 num_sids,
+ struct list_head *head)
+{
+ struct acpi_iort_rmr *rmr = (struct acpi_iort_rmr *)node->node_data;
+ struct acpi_iort_rmr_desc *rmr_desc;
+ int i;
+
+ rmr_desc = ACPI_ADD_PTR(struct acpi_iort_rmr_desc, node,
+ rmr->rmr_offset);
+
+ iort_rmr_desc_check_overlap(rmr_desc, rmr->rmr_count);
+
+ for (i = 0; i < rmr->rmr_count; i++, rmr_desc++) {
+ struct iommu_resv_region *region;
+ enum iommu_resv_type type;
+ u32 *sids_copy;
+ int prot = IOMMU_READ | IOMMU_WRITE;
+ u64 addr = rmr_desc->base_address, size = rmr_desc->length;
+
+ if (!IS_ALIGNED(addr, SZ_64K) || !IS_ALIGNED(size, SZ_64K)) {
+ /* PAGE align base addr and size */
+ addr &= PAGE_MASK;
+ size = PAGE_ALIGN(size +
offset_in_page(rmr_desc->base_address));
+
+ pr_err(FW_BUG "RMR descriptor[0x%llx - 0x%llx] not
aligned to 64K, continue with [0x%llx - 0x%llx]\n",
+ rmr_desc->base_address,
+ rmr_desc->base_address + rmr_desc->length - 1,
+ addr, addr + size - 1);
+ }
+
+ if (rmr->flags & ACPI_IORT_RMR_REMAP_PERMITTED)
+ type = IOMMU_RESV_DIRECT_RELAXABLE;
+ else
+ type = IOMMU_RESV_DIRECT;
+
+ if (rmr->flags & ACPI_IORT_RMR_ACCESS_PRIVILEGE)
+ prot |= IOMMU_PRIV;
+
+ /* Attributes 0x00 - 0x03 represents device memory */
+ if (ACPI_IORT_RMR_ACCESS_ATTRIBUTES(rmr->flags) <=
+ ACPI_IORT_RMR_ATTR_DEVICE_GRE)
+ prot |= IOMMU_MMIO;
+ else if (ACPI_IORT_RMR_ACCESS_ATTRIBUTES(rmr->flags) ==
+ ACPI_IORT_RMR_ATTR_NORMAL_IWB_OWB)
+ prot |= IOMMU_CACHE;
+
+ /* Create a copy of SIDs array to associate with this resv
region */
+ sids_copy = kmemdup(sids, num_sids * sizeof(*sids), GFP_KERNEL);
+ if (!sids_copy)
+ return;
+
+ region = iommu_alloc_resv_region(addr, size, prot, type);
+ if (!region) {
+ kfree(sids_copy);
+ return;
+ }
+
+ region->fw_data.rmr.sids = sids_copy;
+ region->fw_data.rmr.num_sids = num_sids;
+ list_add_tail(>list, head);
+ }
+}
+
+static u32 *iort_rmr_alloc_sids(u32 *sids, u32 count, u32 id_start,
+ u32 new_count)
+{
+ u32 *new_sids;
+ u32 total_count = count + new_count;
+ int i;
+
+ new_sids = krealloc_array(sids, count + new_count,
+
