[PATCH v8 03/11] ACPI/IORT: Add helper functions to parse RMR nodes
Shameerali Kolothum Thodi
shameerali.kolothum.thodi at huawei.com
Fri Feb 25 09:31:49 PST 2022
> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:lorenzo.pieralisi at arm.com]
> Sent: 24 February 2022 10:14
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi at huawei.com>
> Cc: linux-arm-kernel at lists.infradead.org; linux-acpi at vger.kernel.org;
> iommu at lists.linux-foundation.org; Linuxarm <linuxarm at huawei.com>;
> joro at 8bytes.org; robin.murphy at arm.com; will at kernel.org; wanghuiqiang
> <wanghuiqiang at huawei.com>; Guohanjun (Hanjun Guo)
> <guohanjun at huawei.com>; steven.price at arm.com; Sami.Mujawar at arm.com;
> jon at solid-run.com; eric.auger at redhat.com; yangyicong
> <yangyicong at huawei.com>
> Subject: Re: [PATCH v8 03/11] ACPI/IORT: Add helper functions to parse RMR
> nodes
>
> On Mon, Feb 21, 2022 at 03:43:36PM +0000, 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
> <shameerali.kolothum.thodi at huawei.com>
> > ---
> > 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 - 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(®ion->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 ones */
>
> /* Update new ones */
>
> I'd remove the comment, or improve it, as it stands it does not
> help much - we can read the code.
Yeah. Will remove that.
> > + for (i = count; i < total_count; i++)
> > + new_sids[i] = id_start++;
> > +
> > + return new_sids;
> > +}
> > +
> > +static bool iort_rmr_has_dev(struct device *dev, u32 id_start,
> > + u32 id_count)
> > +{
> > + int i;
> > + struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > +
> > + if (dev_is_pci(dev)) {
> > + struct pci_dev *pdev = to_pci_dev(dev);
> > + struct pci_host_bridge *host = pci_find_host_bridge(pdev->bus);
> > +
> > + if (!host->preserve_config)
>
> Please add a comment here - it helps explain the logic behind it.
Ok. I will add a comment here and a reference to spec.
Thanks,
Shameer
>
> Thanks,
> Lorenzo
>
> > + return false;
> > + }
> > +
> > + for (i = 0; i < fwspec->num_ids; i++) {
> > + if (fwspec->ids[i] >= id_start &&
> > + fwspec->ids[i] <= id_start + id_count)
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +static void iort_node_get_rmr_info(struct acpi_iort_node *node,
> > + struct acpi_iort_node *iommu,
> > + struct device *dev, struct list_head *head)
> > +{
> > + struct acpi_iort_node *smmu = NULL;
> > + struct acpi_iort_rmr *rmr;
> > + struct acpi_iort_id_mapping *map;
> > + u32 *sids = NULL;
> > + u32 num_sids = 0;
> > + int i;
> > +
> > + if (!node->mapping_offset || !node->mapping_count) {
> > + pr_err(FW_BUG "Invalid ID mapping, skipping RMR node %p\n",
> > + node);
> > + return;
> > + }
> > +
> > + rmr = (struct acpi_iort_rmr *)node->node_data;
> > + if (!rmr->rmr_offset || !rmr->rmr_count)
> > + return;
> > +
> > + map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node,
> > + node->mapping_offset);
> > +
> > + /*
> > + * Go through the ID mappings and see if we have a match
> > + * for smmu and dev(if !NULL). If found, get the sids
> > + * for the Node.
> > + * Please note, id_count is equal to the number of IDs
> > + * in the range minus one.
> > + */
> > + for (i = 0; i < node->mapping_count; i++, map++) {
> > + struct acpi_iort_node *parent;
> > +
> > + if (!map->id_count)
> > + continue;
> > +
> > + parent = ACPI_ADD_PTR(struct acpi_iort_node, iort_table,
> > + map->output_reference);
> > + if (parent != iommu)
> > + continue;
> > +
> > + /* If dev is valid, check RMR node corresponds to the dev sid */
> > + if (dev && !iort_rmr_has_dev(dev, map->output_base,
> > + map->id_count))
> > + continue;
> > +
> > + /* Retrieve sids associated with the Node. */
> > + sids = iort_rmr_alloc_sids(sids, num_sids, map->output_base,
> > + map->id_count + 1);
> > + if (!sids)
> > + return;
> > +
> > + num_sids += map->id_count + 1;
> > + }
> > +
> > + if (!sids)
> > + return;
> > +
> > + iort_rmr_get_resv_regions(node, smmu, sids, num_sids, head);
> > + kfree(sids);
> > +}
> > +
> > +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)
> > + return;
> > +
> > + iort = (struct acpi_table_iort *)iort_table;
> > +
> > + iort_node = ACPI_ADD_PTR(struct acpi_iort_node, iort,
> > + iort->node_offset);
> > + iort_end = ACPI_ADD_PTR(struct acpi_iort_node, iort,
> > + iort_table->length);
> > +
> > + for (i = 0; i < iort->node_count; i++) {
> > + if (WARN_TAINT(iort_node >= iort_end,
> TAINT_FIRMWARE_WORKAROUND,
> > + "IORT node pointer overflows, bad table!\n"))
> > + return;
> > +
> > + if (iort_node->type == ACPI_IORT_NODE_RMR)
> > + iort_node_get_rmr_info(iort_node, iommu, dev, head);
> > +
> > + iort_node = ACPI_ADD_PTR(struct acpi_iort_node, iort_node,
> > + iort_node->length);
> > + }
> > +}
> > +
> > /**
> > * iort_iommu_msi_get_resv_regions - Reserved region driver helper
> > * @dev: Device from iommu_get_resv_regions()
> > --
> > 2.25.1
> >
More information about the linux-arm-kernel
mailing list