diff mbox series

[v7,9/9] iommu/dma: Reserve any RMR regions associated with a dev

Message ID 20210805080724.480-10-shameerali.kolothum.thodi@huawei.com
State New
Headers show
Series ACPI/IORT: Support for IORT RMR node | expand

Commit Message

Shameerali Kolothum Thodi Aug. 5, 2021, 8:07 a.m. UTC 
Get ACPI IORT RMR regions associated with a dev reserved
so that there is a unity mapping for them in SMMU.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

---
 drivers/iommu/dma-iommu.c | 56 +++++++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 5 deletions(-)

-- 
2.17.1

Comments

Robin Murphy Oct. 8, 2021, 1:09 p.m. UTC | #1 
On 2021-08-05 09:07, Shameer Kolothum wrote:
> Get ACPI IORT RMR regions associated with a dev reserved

> so that there is a unity mapping for them in SMMU.


This feels like most of it belongs in the IORT code rather than 
iommu-dma (which should save the temporary list copy as well).

Robin.

> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

> ---

>   drivers/iommu/dma-iommu.c | 56 +++++++++++++++++++++++++++++++++++----

>   1 file changed, 51 insertions(+), 5 deletions(-)

> 

> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c

> index 1b6e27475279..c1ae0c3d4b33 100644

> --- a/drivers/iommu/dma-iommu.c

> +++ b/drivers/iommu/dma-iommu.c

> @@ -207,22 +207,68 @@ void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,

>   }

>   EXPORT_SYMBOL(iommu_dma_put_rmrs);

>   

> +static bool iommu_dma_dev_has_rmr(struct iommu_fwspec *fwspec,

> +				  struct iommu_resv_region *e)

> +{

> +	int i;

> +

> +	for (i = 0; i < fwspec->num_ids; i++) {

> +		if (e->fw_data.rmr.sid == fwspec->ids[i])

> +			return true;

> +	}

> +

> +	return false;

> +}

> +

> +static void iommu_dma_get_rmr_resv_regions(struct device *dev,

> +					   struct list_head *list)

> +{

> +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);

> +	struct list_head rmr_list;

> +	struct iommu_resv_region *rmr, *tmp;

> +

> +	INIT_LIST_HEAD(&rmr_list);

> +	if (iommu_dma_get_rmrs(fwspec->iommu_fwnode, &rmr_list))

> +		return;

> +

> +	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)

> +			return;

> +	}

> +

> +	list_for_each_entry_safe(rmr, tmp, &rmr_list, list) {

> +		if (!iommu_dma_dev_has_rmr(fwspec, rmr))

> +			continue;

> +

> +		/* Remove from iommu RMR list and add to dev resv_regions */

> +		list_del_init(&rmr->list);

> +		list_add_tail(&rmr->list, list);

> +	}

> +

> +	iommu_dma_put_rmrs(fwspec->iommu_fwnode, &rmr_list);

> +}

> +

>   /**

>    * iommu_dma_get_resv_regions - Reserved region driver helper

>    * @dev: Device from iommu_get_resv_regions()

>    * @list: Reserved region list from iommu_get_resv_regions()

>    *

>    * IOMMU drivers can use this to implement their .get_resv_regions callback

> - * for general non-IOMMU-specific reservations. Currently, this covers GICv3

> - * ITS region reservation on ACPI based ARM platforms that may require HW MSI

> - * reservation.

> + * for general non-IOMMU-specific reservations. Currently this covers,

> + *  -GICv3 ITS region reservation on ACPI based ARM platforms that may

> + *   require HW MSI reservation.

> + *  -Any ACPI IORT RMR memory range reservations (IORT spec rev E.b)

>    */

>   void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list)

>   {

>   

> -	if (!is_of_node(dev_iommu_fwspec_get(dev)->iommu_fwnode))

> +	if (!is_of_node(dev_iommu_fwspec_get(dev)->iommu_fwnode)) {

>   		iort_iommu_msi_get_resv_regions(dev, list);

> -

> +		iommu_dma_get_rmr_resv_regions(dev, list);

> +	}

>   }

>   EXPORT_SYMBOL(iommu_dma_get_resv_regions);

>   

>
Jon Nettleton Oct. 9, 2021, 7:07 a.m. UTC | #2 
On Fri, Oct 8, 2021 at 3:10 PM Robin Murphy <robin.murphy@arm.com> wrote:
>

> On 2021-08-05 09:07, Shameer Kolothum wrote:

> > Get ACPI IORT RMR regions associated with a dev reserved

> > so that there is a unity mapping for them in SMMU.

>

> This feels like most of it belongs in the IORT code rather than

> iommu-dma (which should save the temporary list copy as well).


See previous comment.  The original intent was for device-tree to also
be able to use these mechanisms to create RMR's and support them
in the SMMU.

-Jon

>

> Robin.

>

> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>

> > ---

> >   drivers/iommu/dma-iommu.c | 56 +++++++++++++++++++++++++++++++++++----

> >   1 file changed, 51 insertions(+), 5 deletions(-)

> >

> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c

> > index 1b6e27475279..c1ae0c3d4b33 100644

> > --- a/drivers/iommu/dma-iommu.c

> > +++ b/drivers/iommu/dma-iommu.c

> > @@ -207,22 +207,68 @@ void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,

> >   }

> >   EXPORT_SYMBOL(iommu_dma_put_rmrs);

> >

> > +static bool iommu_dma_dev_has_rmr(struct iommu_fwspec *fwspec,

> > +                               struct iommu_resv_region *e)

> > +{

> > +     int i;

> > +

> > +     for (i = 0; i < fwspec->num_ids; i++) {

> > +             if (e->fw_data.rmr.sid == fwspec->ids[i])

> > +                     return true;

> > +     }

> > +

> > +     return false;

> > +}

> > +

> > +static void iommu_dma_get_rmr_resv_regions(struct device *dev,

> > +                                        struct list_head *list)

> > +{

> > +     struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);

> > +     struct list_head rmr_list;

> > +     struct iommu_resv_region *rmr, *tmp;

> > +

> > +     INIT_LIST_HEAD(&rmr_list);

> > +     if (iommu_dma_get_rmrs(fwspec->iommu_fwnode, &rmr_list))

> > +             return;

> > +

> > +     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)

> > +                     return;

> > +     }

> > +

> > +     list_for_each_entry_safe(rmr, tmp, &rmr_list, list) {

> > +             if (!iommu_dma_dev_has_rmr(fwspec, rmr))

> > +                     continue;

> > +

> > +             /* Remove from iommu RMR list and add to dev resv_regions */

> > +             list_del_init(&rmr->list);

> > +             list_add_tail(&rmr->list, list);

> > +     }

> > +

> > +     iommu_dma_put_rmrs(fwspec->iommu_fwnode, &rmr_list);

> > +}

> > +

> >   /**

> >    * iommu_dma_get_resv_regions - Reserved region driver helper

> >    * @dev: Device from iommu_get_resv_regions()

> >    * @list: Reserved region list from iommu_get_resv_regions()

> >    *

> >    * IOMMU drivers can use this to implement their .get_resv_regions callback

> > - * for general non-IOMMU-specific reservations. Currently, this covers GICv3

> > - * ITS region reservation on ACPI based ARM platforms that may require HW MSI

> > - * reservation.

> > + * for general non-IOMMU-specific reservations. Currently this covers,

> > + *  -GICv3 ITS region reservation on ACPI based ARM platforms that may

> > + *   require HW MSI reservation.

> > + *  -Any ACPI IORT RMR memory range reservations (IORT spec rev E.b)

> >    */

> >   void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list)

> >   {

> >

> > -     if (!is_of_node(dev_iommu_fwspec_get(dev)->iommu_fwnode))

> > +     if (!is_of_node(dev_iommu_fwspec_get(dev)->iommu_fwnode)) {

> >               iort_iommu_msi_get_resv_regions(dev, list);

> > -

> > +             iommu_dma_get_rmr_resv_regions(dev, list);

> > +     }

> >   }

> >   EXPORT_SYMBOL(iommu_dma_get_resv_regions);

> >

> >
Robin Murphy Oct. 11, 2021, 3 p.m. UTC | #3 
On 2021-10-09 08:07, Jon Nettleton wrote:
> On Fri, Oct 8, 2021 at 3:10 PM Robin Murphy <robin.murphy@arm.com> wrote:

>>

>> On 2021-08-05 09:07, Shameer Kolothum wrote:

>>> Get ACPI IORT RMR regions associated with a dev reserved

>>> so that there is a unity mapping for them in SMMU.

>>

>> This feels like most of it belongs in the IORT code rather than

>> iommu-dma (which should save the temporary list copy as well).

> 

> See previous comment.  The original intent was for device-tree to also

> be able to use these mechanisms to create RMR's and support them

> in the SMMU.


Can you clarify how code behind an "if (!is_of_node(...))" check 
alongside other IORT-specific code is expected to be useful for DT?

Yes, iommu_dma_get_resv_regions() itself wants to end up serving as an 
abstraction layer, but that still doesn't mean it has to do much more 
than dispatch into firmware-specific backends as appropriate.

Robin.
Shameerali Kolothum Thodi Oct. 11, 2021, 3:42 p.m. UTC | #4 
> -----Original Message-----

> From: Robin Murphy [mailto:robin.murphy@arm.com]

> Sent: 11 October 2021 16:01

> To: Jon Nettleton <jon@solid-run.com>

> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;

> linux-arm-kernel <linux-arm-kernel@lists.infradead.org>; ACPI Devel Maling

> List <linux-acpi@vger.kernel.org>; Linux IOMMU

> <iommu@lists.linux-foundation.org>; Linuxarm <linuxarm@huawei.com>;

> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Joerg Roedel

> <joro@8bytes.org>; Will Deacon <will@kernel.org>; wanghuiqiang

> <wanghuiqiang@huawei.com>; Guohanjun (Hanjun Guo)

> <guohanjun@huawei.com>; Steven Price <steven.price@arm.com>; Sami

> Mujawar <Sami.Mujawar@arm.com>; Eric Auger <eric.auger@redhat.com>;

> yangyicong <yangyicong@huawei.com>

> Subject: Re: [PATCH v7 9/9] iommu/dma: Reserve any RMR regions associated

> with a dev

> 

> On 2021-10-09 08:07, Jon Nettleton wrote:

> > On Fri, Oct 8, 2021 at 3:10 PM Robin Murphy <robin.murphy@arm.com>

> wrote:

> >>

> >> On 2021-08-05 09:07, Shameer Kolothum wrote:

> >>> Get ACPI IORT RMR regions associated with a dev reserved

> >>> so that there is a unity mapping for them in SMMU.

> >>

> >> This feels like most of it belongs in the IORT code rather than

> >> iommu-dma (which should save the temporary list copy as well).

> >

> > See previous comment.  The original intent was for device-tree to also

> > be able to use these mechanisms to create RMR's and support them

> > in the SMMU.

> 

> Can you clarify how code behind an "if (!is_of_node(...))" check

> alongside other IORT-specific code is expected to be useful for DT?

> 

> Yes, iommu_dma_get_resv_regions() itself wants to end up serving as an

> abstraction layer, but that still doesn't mean it has to do much more

> than dispatch into firmware-specific backends as appropriate.


(Resending as I accidently replied earlier from our internal ML id. Sorry)

The way I thought about is as below,

1.  iommu_dma_get_resv_regions() will invoke the common iommu_dma_get_rmr_resv_regions().
    Yes, the if (!is_of_node(...)) is not required here.
2.  iommu_dma_get_rmr_resv_regions() calls iommu_dma_get_rmrs().
    iommu_dma_get_rmrs() has the  (!is_of_node(...)) check to call into IORT or DT specific functions
    to retrieve the RMR reserve regions associated with a given iommu_fwnode.
3.  The common iommu_dma_get_rmr_resv_regions() further checks for PCI host preserve_config
    and whether the returned RMR list actually has any dev specific region to reserve or not.

So the only firmware specific backend is handled inside the iommu_dma_get_rmrs() and that is also called
from the SMMU driver probe to install bypass SIDs.

Anyway, if the eventual DT implementation or further IORT spec changes makes this abstraction
irrelevant I am Ok to move this into the IORT code.

Thanks,
Shameer
diff mbox series

Patch

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 1b6e27475279..c1ae0c3d4b33 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -207,22 +207,68 @@  void iommu_dma_put_rmrs(struct fwnode_handle *iommu_fwnode,
 }
 EXPORT_SYMBOL(iommu_dma_put_rmrs);
 
+static bool iommu_dma_dev_has_rmr(struct iommu_fwspec *fwspec,
+				  struct iommu_resv_region *e)
+{
+	int i;
+
+	for (i = 0; i < fwspec->num_ids; i++) {
+		if (e->fw_data.rmr.sid == fwspec->ids[i])
+			return true;
+	}
+
+	return false;
+}
+
+static void iommu_dma_get_rmr_resv_regions(struct device *dev,
+					   struct list_head *list)
+{
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct list_head rmr_list;
+	struct iommu_resv_region *rmr, *tmp;
+
+	INIT_LIST_HEAD(&rmr_list);
+	if (iommu_dma_get_rmrs(fwspec->iommu_fwnode, &rmr_list))
+		return;
+
+	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)
+			return;
+	}
+
+	list_for_each_entry_safe(rmr, tmp, &rmr_list, list) {
+		if (!iommu_dma_dev_has_rmr(fwspec, rmr))
+			continue;
+
+		/* Remove from iommu RMR list and add to dev resv_regions */
+		list_del_init(&rmr->list);
+		list_add_tail(&rmr->list, list);
+	}
+
+	iommu_dma_put_rmrs(fwspec->iommu_fwnode, &rmr_list);
+}
+
 /**
  * iommu_dma_get_resv_regions - Reserved region driver helper
  * @dev: Device from iommu_get_resv_regions()
  * @list: Reserved region list from iommu_get_resv_regions()
  *
  * IOMMU drivers can use this to implement their .get_resv_regions callback
- * for general non-IOMMU-specific reservations. Currently, this covers GICv3
- * ITS region reservation on ACPI based ARM platforms that may require HW MSI
- * reservation.
+ * for general non-IOMMU-specific reservations. Currently this covers,
+ *  -GICv3 ITS region reservation on ACPI based ARM platforms that may
+ *   require HW MSI reservation.
+ *  -Any ACPI IORT RMR memory range reservations (IORT spec rev E.b)
  */
 void iommu_dma_get_resv_regions(struct device *dev, struct list_head *list)
 {
 
-	if (!is_of_node(dev_iommu_fwspec_get(dev)->iommu_fwnode))
+	if (!is_of_node(dev_iommu_fwspec_get(dev)->iommu_fwnode)) {
 		iort_iommu_msi_get_resv_regions(dev, list);
-
+		iommu_dma_get_rmr_resv_regions(dev, list);
+	}
 }
 EXPORT_SYMBOL(iommu_dma_get_resv_regions);