RE: [PATCH v8 03/11] ACPI/IORT: Add helper functions to parse RMR nodes
Hi Eric,
> -Original Message-
> From: Eric Auger [mailto:eric.au...@redhat.com]
> Sent: 10 March 2022 10:32
> To: Shameerali Kolothum Thodi ;
> linux-arm-ker...@lists.infradead.org; linux-a...@vger.kernel.org;
> iommu@lists.linux-foundation.org
> Cc: Linuxarm ; lorenzo.pieral...@arm.com;
> j...@8bytes.org; robin.mur...@arm.com; w...@kernel.org; wanghuiqiang
> ; Guohanjun (Hanjun Guo)
> ; steven.pr...@arm.com; sami.muja...@arm.com;
> j...@solid-run.com; yangyicong
> Subject: Re: [PATCH v8 03/11] ACPI/IORT: Add helper functions to parse RMR
> nodes
>
> Hi Shameer,
>
> On 2/21/22 4:43 PM, Shameer Kolothum wrote:
> > The helper functions here parse through the IORT RMR nodes and
> > populate a reserved region list corresponding to a given iommu
> > and device(optional). These also go through the ID mappings of
> > the RMR node and retrieves all the SIDs associated with a RMR
> > descriptor.
> >
> > Signed-off-by: Shameer Kolothum
>
> > ---
> > drivers/acpi/arm64/iort.c | 225
> ++
> > 1 file changed, 225 insertions(+)
> >
> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > index 0730c4dbb700..05da9ebff50a 100644
> > --- a/drivers/acpi/arm64/iort.c
> > +++ b/drivers/acpi/arm64/iort.c
> > @@ -830,6 +830,231 @@ static struct acpi_iort_node
> *iort_get_msi_resv_iommu(struct device *dev)
> > return NULL;
> > }
> >
[...]
> > +static void iort_find_rmrs(struct acpi_iort_node *iommu, struct device
> > *dev,
> > + struct list_head *head)
> > +{
> > + struct acpi_table_iort *iort;
> > + struct acpi_iort_node *iort_node, *iort_end;
> > + int i;
> > +
> > + if (iort_table->revision < 5)
> This means E.b and E.c revs are not supported. Is it what we want?
Yes. E.b lacks memory attributes info associated with RMR node. Though E.c
added those, it broke backward compatibility with ACPICA E.b support and
is now deprecated.
Thanks,
Shameer
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v8 03/11] ACPI/IORT: Add helper functions to parse RMR nodes
Hi Shameer,
On 2/21/22 4:43 PM, Shameer Kolothum wrote:
> The helper functions here parse through the IORT RMR nodes and
> populate a reserved region list corresponding to a given iommu
> and device(optional). These also go through the ID mappings of
> the RMR node and retrieves all the SIDs associated with a RMR
> descriptor.
>
> Signed-off-by: Shameer Kolothum
> ---
> drivers/acpi/arm64/iort.c | 225 ++
> 1 file changed, 225 insertions(+)
>
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 0730c4dbb700..05da9ebff50a 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -830,6 +830,231 @@ static struct acpi_iort_node
> *iort_get_msi_resv_iommu(struct device *dev)
> return NULL;
> }
>
> +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;
> +
> + 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_rmr_get_resv_regions(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)
> + 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,
> + sizeof(*new_sids), GFP_KERNEL);
> + if (!new_sids)
> + return NULL;
> +
> + /*Update new
RE: [PATCH v8 03/11] ACPI/IORT: Add helper functions to parse RMR nodes
> -Original Message-
> From: Lorenzo Pieralisi [mailto:lorenzo.pieral...@arm.com]
> Sent: 24 February 2022 10:14
> To: Shameerali Kolothum Thodi
> Cc: linux-arm-ker...@lists.infradead.org; linux-a...@vger.kernel.org;
> iommu@lists.linux-foundation.org; Linuxarm ;
> j...@8bytes.org; robin.mur...@arm.com; w...@kernel.org; wanghuiqiang
> ; Guohanjun (Hanjun Guo)
> ; steven.pr...@arm.com; sami.muja...@arm.com;
> j...@solid-run.com; eric.au...@redhat.com; yangyicong
>
> Subject: Re: [PATCH v8 03/11] ACPI/IORT: Add helper functions to parse RMR
> nodes
>
> On Mon, Feb 21, 2022 at 03:43:36PM +, Shameer Kolothum wrote:
> > The helper functions here parse through the IORT RMR nodes and
> > populate a reserved region list corresponding to a given iommu
> > and device(optional). These also go through the ID mappings of
> > the RMR node and retrieves all the SIDs associated with a RMR
> > descriptor.
> >
> > Signed-off-by: Shameer Kolothum
>
> > ---
> > drivers/acpi/arm64/iort.c | 225
> ++
> > 1 file changed, 225 insertions(+)
>
> I have very minor comments - I would ask Robin to ack the updated
> flags management.
>
> Functions should be introduced where they are used, this patch
> is not bisectable:
Right. I missed the below warning. Will do.
> drivers/acpi/arm64/iort.c:1028:13: warning: ‘iort_find_rmrs’ defined but not
> used [-Wunused-function]
> 1028 | static void iort_find_rmrs(struct acpi_iort_node *iommu, struct device
> *dev,
>
> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > index 0730c4dbb700..05da9ebff50a 100644
> > --- a/drivers/acpi/arm64/iort.c
> > +++ b/drivers/acpi/arm64/iort.c
> > @@ -830,6 +830,231 @@ static struct acpi_iort_node
> *iort_get_msi_resv_iommu(struct device *dev)
> > return NULL;
> > }
> >
> > +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;
> > +
> > + end = start + length - 1;
>
> We could probably check for length != 0, the kernel is not there to
> validate firmware but this would not hurt either.
Ok.
> > +
> > + /* 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_rmr_get_resv_regions(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
Re: [PATCH v8 03/11] ACPI/IORT: Add helper functions to parse RMR nodes
On Mon, Feb 21, 2022 at 03:43:36PM +, Shameer Kolothum wrote:
> The helper functions here parse through the IORT RMR nodes and
> populate a reserved region list corresponding to a given iommu
> and device(optional). These also go through the ID mappings of
> the RMR node and retrieves all the SIDs associated with a RMR
> descriptor.
>
> Signed-off-by: Shameer Kolothum
> ---
> drivers/acpi/arm64/iort.c | 225 ++
> 1 file changed, 225 insertions(+)
I have very minor comments - I would ask Robin to ack the updated
flags management.
Functions should be introduced where they are used, this patch
is not bisectable:
drivers/acpi/arm64/iort.c:1028:13: warning: ‘iort_find_rmrs’ defined but not
used [-Wunused-function]
1028 | static void iort_find_rmrs(struct acpi_iort_node *iommu, struct device
*dev,
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 0730c4dbb700..05da9ebff50a 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -830,6 +830,231 @@ static struct acpi_iort_node
> *iort_get_msi_resv_iommu(struct device *dev)
> return NULL;
> }
>
> +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;
> +
> + end = start + length - 1;
We could probably check for length != 0, the kernel is not there to
validate firmware but this would not hurt either.
> +
> + /* 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_rmr_get_resv_regions(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)
> + 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 =
