Re: [PATCH v7 2/9] ACPI/IORT: Add support for RMR node parsing
On Wed, Dec 8, 2021 at 3:37 PM Robin Murphy wrote: > > Jon, > > On 2021-12-08 13:26, Jon Nettleton wrote: > [...] > >>> Even marking them as IOMMU_READ/WRITE is as much of an assumption as > >>> using IOMMU_MMIO or IOMMU_CACHE. It just seems IOMMU_MMIO is the most > >>> popular since all the examples use it for MSI doorbells in the > >>> documentation. > >> > >> We don't merge code based on assumptions that can easily break because > >> the specifications don't contemplate the details that are required. > >> > >>> I am interested why this concern is only being brought up at this point > >>> on a patchset that has been on the mailing list for 8+ months? > >> > >> See above. We don't merge code that we know can break and is based on > >> assumptions, we need to update the IORT specifications to make them > >> cover all the use cases - in a predictable way - and that's what we are > >> working on. > > > > This is not really an answer to the question. The latest version of the > > IORT RMR spec was published in Feb 2021. Why was this issue not > > brought up with Rev 1 of this patchset? Instead you have wasted > > 10 months of developer and customer time. This could have easily been > > turned into a code first spec change request, which is a valid option > > for ACPI changes. > > It was only on v5 of the patchset - *six months* after the original RFC > posting - that anyone even first started to question the initial > assumptions made about attributes[1], and even then somebody familiar > countered that it didn't appear to matter[2]. Sorry, but you don't get > to U-turn and throw unjust shade at Arm for not being prescient. > > Yes, when those of us within Arm set out the initial RMR spec, an > assumption was made that it seemed reasonable for an OS to simply pick > some default strong memory type (Device or Normal-NC) and full > permissions if it did need to map RMRs at stage 1. That spec was > reviewed and published externally and no interested parties came forth > asking "hey, what about attributes?". Linux patches were written around > that assumption and proceeded through many rounds of review until we > eventually received sufficient feedback to demonstrate that the > assumption did not in fact hold well enough in general and there seemed > to be a genuine need for RMR attributes, and at that point we started > work on revising the spec. Was it documented anywhere that the RMR spec mandated Device or Normal-NC memory attributes? I have read through the spec pretty thoroughly and not seen this requirement documented anywhere. Also please feel free to point out where we can find information regarding how the spec is being revised. I am on causeway and in all the SC meetings and haven't seen this topic brought up at all. > > In the meantime, these patches have sat at v7 for four months - the > *other* outstanding review comments have not been addressed; I still > don't recall seeing an answer about whether LX2160 or anything else > currently deployed actually *needs* cacheable mappings or whether it > could muddle through with the IOMMU_MMIO assumption until proper "RMR > v2" support arrived later; even if so, an interim workaround specific to > LX2160 could have been proposed but hasn't. It is hardly reasonable to > pretend that Arm or the upstream maintainers are responsible for a lack > of development activity on the part of the submitters, no matter how > much blatant misinformation is repeated on Twitter. Oh the "other" comments where after 7 series of patches you decided that the approach that was agreed upon on the mailing list was no longer to your liking? Not to mention the month the patchset sat idle after initial comments from Ard that were cleared up, when I pinged the thread and it was ignored. If there was some sort of prompt response on the threads by the maintainers with accurate information about why the patches were being held up, or that they were working on a new spec maybe developers would have bothered to push the patchset forward. There is no misinformation on Twitter. After 7 series on a patchset after initial discussion that it would be designed so device-tree could leverage the backend work, you just changed your mind, and basically sent everything back to the start. Meanwhile only now in this thread are we finding out that the spec is getting re-worked again, which means that we will need to update our firmware, and wait for someone to write patches for the new spec, because guess what?... Arm didn't write the patches for any of the initial specs. Arm maintainers should be helping to find ways to get Arm "specifications", integrated fo
Re: [PATCH v7 2/9] ACPI/IORT: Add support for RMR node parsing
On Wed, Dec 8, 2021 at 1:19 PM Lorenzo Pieralisi
wrote:
>
> On Tue, Oct 12, 2021 at 10:00:24AM +0200, Jon Nettleton wrote:
> > On Mon, Oct 11, 2021 at 4:04 PM Robin Murphy wrote:
> > >
> > > On 2021-10-09 08:06, Jon Nettleton wrote:
> > > [...]
> > > >>> + if (rmr->flags & IOMMU_RMR_REMAP_PERMITTED) {
> > > >>> + type = IOMMU_RESV_DIRECT_RELAXABLE;
> > > >>> + /*
> > > >>> + * Set IOMMU_CACHE as
> > > >>> IOMMU_RESV_DIRECT_RELAXABLE is
> > > >>> + * normally used for allocated system memory
> > > >>> that is
> > > >>> + * then used for device specific reserved
> > > >>> regions.
> > > >>> + */
> > > >>> + prot |= IOMMU_CACHE;
> > > >>> + } else {
> > > >>> + type = IOMMU_RESV_DIRECT;
> > > >>> + /*
> > > >>> + * Set IOMMU_MMIO as IOMMU_RESV_DIRECT is
> > > >>> normally used
> > > >>> + * for device memory like MSI doorbell.
> > > >>> + */
> > > >>> + prot |= IOMMU_MMIO;
> > > >>> + }
> > > >>
> > > >> I'm not sure we ever got a definitive answer to this - does DPAA2
> > > >> actually go wrong if we use IOMMU_MMIO here? I'd still much prefer to
> > > >> make the fewest possible assumptions, since at this point it's
> > > >> basically
> > > >> just a stop-gap until we can fix the spec. It's become clear that we
> > > >> can't reliably rely on guessing attributes, so I'm not too fussed about
> > > >> theoretical cases that currently don't work (due to complete lack of
> > > >> RMR
> > > >> support) continuing to not work for the moment, as long as we can make
> > > >> the real-world cases we actually have work at all. Anything which only
> > > >> affects performance I'd rather leave until firmware can tell us what
> > > >> to do.
> > > >
> > > > Well it isn't DPAA2, it is FSL_MC_BUS that fails with IOMMU_MMIO
> > > > mappings. DPAA2 is just one connected device.
> > >
> > > Apologies if I'm being overly loose with terminology there - my point of
> > > reference for this hardware is documentation for the old LS2080A, where
> > > the "DPAA2 Reference Manual" gives a strong impression that the MC is a
> > > component belonging to the overall DPAA2 architecture. Either way it
> > > technically stands to reason that the other DPAA2 components would only
> > > be usable if the MC itself works (unless I've been holding a major
> > > misconception about that for years as well).
> > >
> > > In the context of this discussion, please consider any reference I may
> > > make to bits of NXP's hardware to be shorthand for "the thing for which
> > > NXP have a vested interest in IORT RMRs".
> >
> > Ultimately the spec doesn't mention what IOMMU properties the regions
> > should have.
>
> It will have to and that's what we are working on.
Where is this being worked on? I see no open tickets for this.
>
> > Even marking them as IOMMU_READ/WRITE is as much of an assumption as
> > using IOMMU_MMIO or IOMMU_CACHE. It just seems IOMMU_MMIO is the most
> > popular since all the examples use it for MSI doorbells in the
> > documentation.
>
> We don't merge code based on assumptions that can easily break because
> the specifications don't contemplate the details that are required.
>
> > I am interested why this concern is only being brought up at this point
> > on a patchset that has been on the mailing list for 8+ months?
>
> See above. We don't merge code that we know can break and is based on
> assumptions, we need to update the IORT specifications to make them
> cover all the use cases - in a predictable way - and that's what we are
> working on.
This is not really an answer to the question. The latest version of the
IORT RMR spec was published in Feb 2021. Why was this issue not
brought up with Rev 1 of this patchset? Instead you have wasted
10 months of developer and customer time. This could have easily been
turned into a code first spec change request, which is a val
Re: [PATCH v7 2/9] ACPI/IORT: Add support for RMR node parsing
On Mon, Oct 11, 2021 at 4:04 PM Robin Murphy wrote:
>
> On 2021-10-09 08:06, Jon Nettleton wrote:
> [...]
> >>> + if (rmr->flags & IOMMU_RMR_REMAP_PERMITTED) {
> >>> + type = IOMMU_RESV_DIRECT_RELAXABLE;
> >>> + /*
> >>> + * Set IOMMU_CACHE as IOMMU_RESV_DIRECT_RELAXABLE is
> >>> + * normally used for allocated system memory that is
> >>> + * then used for device specific reserved regions.
> >>> + */
> >>> + prot |= IOMMU_CACHE;
> >>> + } else {
> >>> + type = IOMMU_RESV_DIRECT;
> >>> + /*
> >>> + * Set IOMMU_MMIO as IOMMU_RESV_DIRECT is normally
> >>> used
> >>> + * for device memory like MSI doorbell.
> >>> + */
> >>> + prot |= IOMMU_MMIO;
> >>> + }
> >>
> >> I'm not sure we ever got a definitive answer to this - does DPAA2
> >> actually go wrong if we use IOMMU_MMIO here? I'd still much prefer to
> >> make the fewest possible assumptions, since at this point it's basically
> >> just a stop-gap until we can fix the spec. It's become clear that we
> >> can't reliably rely on guessing attributes, so I'm not too fussed about
> >> theoretical cases that currently don't work (due to complete lack of RMR
> >> support) continuing to not work for the moment, as long as we can make
> >> the real-world cases we actually have work at all. Anything which only
> >> affects performance I'd rather leave until firmware can tell us what to do.
> >
> > Well it isn't DPAA2, it is FSL_MC_BUS that fails with IOMMU_MMIO
> > mappings. DPAA2 is just one connected device.
>
> Apologies if I'm being overly loose with terminology there - my point of
> reference for this hardware is documentation for the old LS2080A, where
> the "DPAA2 Reference Manual" gives a strong impression that the MC is a
> component belonging to the overall DPAA2 architecture. Either way it
> technically stands to reason that the other DPAA2 components would only
> be usable if the MC itself works (unless I've been holding a major
> misconception about that for years as well).
>
> In the context of this discussion, please consider any reference I may
> make to bits of NXP's hardware to be shorthand for "the thing for which
> NXP have a vested interest in IORT RMRs".
Ultimately the spec doesn't mention what IOMMU properties the regions
should have. Even marking them as IOMMU_READ/WRITE is as much
of an assumption as using IOMMU_MMIO or IOMMU_CACHE. It just seems
IOMMU_MMIO is the most popular since all the examples use it for MSI
doorbells in the documentation.
I am interested why this concern is only being brought up at this point
in a patchset that has been on the mailing list for 8+ months? This is
based on a spec that has existed from Arm since 2020 with the most recent
revisions published in Feb 2021. The lack of RMR support in the kernel
is affecting real world products, and the ability for SystemReady ES
certified systems from just fully working with recent distributions. Even
worse, is that without this patchset customers are forced to jump through
hoops to purposefully re-enable smmu bypass making their systems less
secure.
How is this a good experience for customers of SystemReady hardware
when for any mainline distribution to work the first thing they have to do is
make their system less secure?
-Jon
>
> Thanks,
> Robin.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 9/9] iommu/dma: Reserve any RMR regions associated with a dev
On Fri, Oct 8, 2021 at 3:10 PM Robin Murphy 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
> > ---
> > 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(_list);
> > + if (iommu_dma_get_rmrs(fwspec->iommu_fwnode, _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, _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(>list);
> > + list_add_tail(>list, list);
> > + }
> > +
> > + iommu_dma_put_rmrs(fwspec->iommu_fwnode, _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);
> >
> >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 2/9] ACPI/IORT: Add support for RMR node parsing
On Fri, Oct 8, 2021 at 2:49 PM Robin Murphy wrote:
>
> On 2021-08-05 09:07, Shameer Kolothum wrote:
> > Add support for parsing RMR node information from ACPI.
> >
> > Find the associated streamid and smmu node info from the
> > RMR node and populate a linked list with RMR memory
> > descriptors.
> >
> > Signed-off-by: Shameer Kolothum
> > ---
> > drivers/acpi/arm64/iort.c | 134 +-
> > 1 file changed, 133 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > index 3b23fb775ac4..d76ba46ebe67 100644
> > --- a/drivers/acpi/arm64/iort.c
> > +++ b/drivers/acpi/arm64/iort.c
> > @@ -40,6 +40,8 @@ struct iort_fwnode {
> > static LIST_HEAD(iort_fwnode_list);
> > static DEFINE_SPINLOCK(iort_fwnode_lock);
> >
> > +static LIST_HEAD(iort_rmr_list); /* list of RMR regions from ACPI */
> > +
> > /**
> >* iort_set_fwnode() - Create iort_fwnode and use it to register
> >* iommu data in the iort_fwnode_list
> > @@ -393,7 +395,8 @@ static struct acpi_iort_node *iort_node_get_id(struct
> > acpi_iort_node *node,
> > if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT ||
> > node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX ||
> > node->type == ACPI_IORT_NODE_SMMU_V3 ||
> > - node->type == ACPI_IORT_NODE_PMCG) {
> > + node->type == ACPI_IORT_NODE_PMCG ||
> > + node->type == ACPI_IORT_NODE_RMR) {
> > *id_out = map->output_base;
> > return parent;
> > }
> > @@ -1566,6 +1569,134 @@ static void __init iort_enable_acs(struct
> > acpi_iort_node *iort_node)
> > #else
> > static inline void iort_enable_acs(struct acpi_iort_node *iort_node) { }
> > #endif
> > +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);
> > + }
> > + }
> > +}
> > +
> > +static void __init iort_node_get_rmr_info(struct acpi_iort_node *iort_node)
> > +{
> > + struct acpi_iort_node *smmu;
> > + struct acpi_iort_rmr *rmr;
> > + struct acpi_iort_rmr_desc *rmr_desc;
> > + u32 map_count = iort_node->mapping_count;
> > + u32 sid;
> > + int i;
> > +
> > + if (!iort_node->mapping_offset || map_count != 1) {
> > + pr_err(FW_BUG "Invalid ID mapping, skipping RMR node %p\n",
> > +iort_node);
> > + return;
> > + }
> > +
> > + /* Retrieve associated smmu and stream id */
> > + smmu = iort_node_get_id(iort_node, , 0);
> > + if (!smmu) {
> > + pr_err(FW_BUG "Invalid SMMU reference, skipping RMR node
> > %p\n",
> > +iort_node);
> > + return;
> > + }
> > +
> > + /* Retrieve RMR data */
> > + rmr = (struct acpi_iort_rmr *)iort_node->node_data;
> > + if (!rmr->rmr_offset || !rmr->rmr_count) {
> > + pr_err(FW_BUG "Invalid RMR descriptor array, skipping RMR
> > node %p\n",
> > +iort_node);
> > + return;
> > + }
> > +
> > + rmr_desc = ACPI_ADD_PTR(struct acpi_iort_rmr_desc, iort_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;
> > + 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 &
Re: [PATCH v7 1/9] iommu: Introduce a union to struct iommu_resv_region
On Fri, Oct 8, 2021 at 2:14 PM Robin Murphy wrote:
>
> On 2021-08-05 09:07, Shameer Kolothum wrote:
> > A union is introduced to struct iommu_resv_region to hold
> > any firmware specific data. This is in preparation to add
> > support for IORT RMR reserve regions and the union now holds
> > the RMR specific information.
> >
> > Signed-off-by: Shameer Kolothum
> > ---
> > include/linux/iommu.h | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index 32d448050bf7..bd0e4641c569 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -114,6 +114,13 @@ enum iommu_resv_type {
> > IOMMU_RESV_SW_MSI,
> > };
> >
> > +struct iommu_iort_rmr_data {
> > +#define IOMMU_RMR_REMAP_PERMITTED(1 << 0)
> > + u32 flags;
> > + u32 sid;/* Stream Id associated with RMR entry */
> > + void *smmu; /* Associated IORT SMMU node pointer */
> > +};
>
> Do we really need to duplicate all this data? AFAICS we could just save
> the acpi_iort_rmr pointer in the iommu_resv_region (with a forward
> declaration here if necessary) and defer parsing its actual mappings
> until the point where we can directly consume the results.
>From earlier discussions on this patchset, the original goal was also for
device-tree mechanisms to be able to hook into this code to support
similar RMR's and SMMU initialization, just not through the ACPI / IORT
path.
>
> Robin.
>
> > +
> > /**
> >* struct iommu_resv_region - descriptor for a reserved memory region
> >* @list: Linked list pointers
> > @@ -121,6 +128,7 @@ enum iommu_resv_type {
> >* @length: Length of the region in bytes
> >* @prot: IOMMU Protection flags (READ/WRITE/...)
> >* @type: Type of the reserved region
> > + * @rmr: ACPI IORT RMR specific data
> >*/
> > struct iommu_resv_region {
> > struct list_headlist;
> > @@ -128,6 +136,9 @@ struct iommu_resv_region {
> > size_t length;
> > int prot;
> > enum iommu_resv_typetype;
> > + union {
> > + struct iommu_iort_rmr_data rmr;
> > + } fw_data;
> > };
> >
> > /**
> >
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 2/9] ACPI/IORT: Add support for RMR node parsing
On Thu, Sep 16, 2021 at 10:26 AM Shameerali Kolothum Thodi wrote: > > > > > -Original Message----- > > From: Jon Nettleton [mailto:j...@solid-run.com] > > Sent: 16 September 2021 08:52 > > To: Shameerali Kolothum Thodi > > Cc: Robin Murphy ; Lorenzo Pieralisi > > ; Laurentiu Tudor ; > > linux-arm-kernel ; ACPI Devel Maling > > List ; Linux IOMMU > > ; Joerg Roedel ; Will > > Deacon ; wanghuiqiang ; > > Guohanjun (Hanjun Guo) ; Steven Price > > ; Sami Mujawar ; Eric > > Auger ; yangyicong > > Subject: Re: [PATCH v7 2/9] ACPI/IORT: Add support for RMR node parsing > > > > On Thu, Sep 16, 2021 at 9:26 AM Shameerali Kolothum Thodi > > wrote: > > > > > > > > > > > > > -Original Message- > > > > From: Jon Nettleton [mailto:j...@solid-run.com] > > > > Sent: 06 September 2021 20:51 > > > > To: Robin Murphy > > > > Cc: Lorenzo Pieralisi ; Shameerali > > > > Kolothum Thodi ; Laurentiu > > > > Tudor ; linux-arm-kernel > > > > ; ACPI Devel Maling List > > > > ; Linux IOMMU > > > > ; Linuxarm ; > > > > Joerg Roedel ; Will Deacon ; > > > > wanghuiqiang ; Guohanjun (Hanjun Guo) > > > > ; Steven Price ; Sami > > > > Mujawar ; Eric Auger > > ; > > > > yangyicong > > > > Subject: Re: [PATCH v7 2/9] ACPI/IORT: Add support for RMR node > > > > parsing > > > > > > > [...] > > > > > > > > > > > > > > > On the prot value assignment based on the remapping flag, I'd > > > > > > like to hear Robin/Joerg's opinion, I'd avoid being in a > > > > > > situation where "normally" this would work but then we have to quirk > > it. > > > > > > > > > > > > Is this a valid assumption _always_ ? > > > > > > > > > > No. Certainly applying IOMMU_CACHE without reference to the > > > > > device's _CCA attribute or how CPUs may be accessing a shared > > > > > buffer could lead to a loss of coherency. At worst, applying > > > > > IOMMU_MMIO to a device-private buffer *could* cause the device to > > > > > lose coherency with itself if the memory underlying the RMR may > > > > > have allocated into system caches. Note that the expected use for > > > > > non-remappable RMRs is the device holding some sort of long-lived > > > > > private data in system RAM - the MSI doorbell trick is far more of a > > > > > niche > > hack really. > > > > > > > > > > At the very least I think we need to refer to the device's memory > > > > > access properties here. > > > > > > > > > > Jon, Laurentiu - how do RMRs correspond to the EFI memory map on > > > > > your firmware? I'm starting to think that as long as the > > > > > underlying memory is described appropriately there then we should > > > > > be able to infer correct attributes from the EFI memory type and > > > > > flags. > > > > > > > > The devices are all cache coherent and marked as _CCA, 1. The > > > > Memory regions are in the virt table as > > ARM_MEMORY_REGION_ATTRIBUTE_DEVICE. > > > > > > > > The current chicken and egg problem we have is that during the > > > > fsl-mc-bus initialization we call > > > > > > > > error = acpi_dma_configure_id(>dev, DEV_DMA_COHERENT, > > > > _stream_id); > > > > > > > > which gets deferred because the SMMU has not been initialized yet. > > > > Then we initialize the RMR tables but there is no device reference > > > > there to be able to query device properties, only the stream id. > > > > After the IORT tables are parsed and the SMMU is setup, on the > > > > second device probe we associate everything based on the stream id > > > > and the fsl-mc-bus device is able to claim its 1-1 DMA mappings. > > > > > > Can we solve this order problem by delaying the > > > iommu_alloc_resv_region() to the iommu_dma_get_rmr_resv_regions(dev, > > > list) ? We could invoke > > > device_get_dma_attr() from there which I believe will return the _CCA > > attribute. > > > > > > Or is that still early to invoke that? > > > > That looks like it should work. Do we then also
Re: [PATCH v7 2/9] ACPI/IORT: Add support for RMR node parsing
On Thu, Sep 16, 2021 at 9:26 AM Shameerali Kolothum Thodi wrote: > > > > > -Original Message----- > > From: Jon Nettleton [mailto:j...@solid-run.com] > > Sent: 06 September 2021 20:51 > > To: Robin Murphy > > Cc: Lorenzo Pieralisi ; Shameerali Kolothum Thodi > > ; Laurentiu Tudor > > ; linux-arm-kernel > > ; ACPI Devel Maling List > > ; Linux IOMMU > > ; Linuxarm ; > > Joerg Roedel ; Will Deacon ; > > wanghuiqiang ; Guohanjun (Hanjun Guo) > > ; Steven Price ; Sami > > Mujawar ; Eric Auger ; > > yangyicong > > Subject: Re: [PATCH v7 2/9] ACPI/IORT: Add support for RMR node parsing > > > [...] > > > > > > > > > On the prot value assignment based on the remapping flag, I'd like > > > > to hear Robin/Joerg's opinion, I'd avoid being in a situation where > > > > "normally" this would work but then we have to quirk it. > > > > > > > > Is this a valid assumption _always_ ? > > > > > > No. Certainly applying IOMMU_CACHE without reference to the device's > > > _CCA attribute or how CPUs may be accessing a shared buffer could lead > > > to a loss of coherency. At worst, applying IOMMU_MMIO to a > > > device-private buffer *could* cause the device to lose coherency with > > > itself if the memory underlying the RMR may have allocated into system > > > caches. Note that the expected use for non-remappable RMRs is the > > > device holding some sort of long-lived private data in system RAM - > > > the MSI doorbell trick is far more of a niche hack really. > > > > > > At the very least I think we need to refer to the device's memory > > > access properties here. > > > > > > Jon, Laurentiu - how do RMRs correspond to the EFI memory map on your > > > firmware? I'm starting to think that as long as the underlying memory > > > is described appropriately there then we should be able to infer > > > correct attributes from the EFI memory type and flags. > > > > The devices are all cache coherent and marked as _CCA, 1. The Memory > > regions are in the virt table as ARM_MEMORY_REGION_ATTRIBUTE_DEVICE. > > > > The current chicken and egg problem we have is that during the fsl-mc-bus > > initialization we call > > > > error = acpi_dma_configure_id(>dev, DEV_DMA_COHERENT, > > _stream_id); > > > > which gets deferred because the SMMU has not been initialized yet. Then we > > initialize the RMR tables but there is no device reference there to be able > > to > > query device properties, only the stream id. After the IORT tables are > > parsed > > and the SMMU is setup, on the second device probe we associate everything > > based on the stream id and the fsl-mc-bus device is able to claim its 1-1 > > DMA > > mappings. > > Can we solve this order problem by delaying the iommu_alloc_resv_region() > to the iommu_dma_get_rmr_resv_regions(dev, list) ? We could invoke > device_get_dma_attr() from there which I believe will return the _CCA > attribute. > > Or is that still early to invoke that? That looks like it should work. Do we then also need to parse through the VirtualMemoryTable matching the start and end addresses to determine the other memory attributes like MMIO? -Jon > > Thanks, > Shameer > > > cat /sys/kernel/iommu_groups/0/reserved_regions > > 0x0100 0x10ff direct-relaxable > > 0x0800 0x080f msi > > 0x00080c00 0x00081bff direct-relaxable > > 0x001c 0x001c001f direct-relaxable > > 0x00208000 0x00209fff direct-relaxable > > > > -Jon > > > > > > > > Robin. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 2/9] ACPI/IORT: Add support for RMR node parsing
On Mon, Sep 6, 2021 at 7:44 PM Robin Murphy wrote:
>
> On 2021-08-05 17:03, Lorenzo Pieralisi wrote:
> > On Thu, Aug 05, 2021 at 09:07:17AM +0100, Shameer Kolothum wrote:
> >
> > [...]
> >
> >> +static void __init iort_node_get_rmr_info(struct acpi_iort_node
> >> *iort_node)
> >> +{
> >> +struct acpi_iort_node *smmu;
> >> +struct acpi_iort_rmr *rmr;
> >> +struct acpi_iort_rmr_desc *rmr_desc;
> >> +u32 map_count = iort_node->mapping_count;
> >> +u32 sid;
> >> +int i;
> >> +
> >> +if (!iort_node->mapping_offset || map_count != 1) {
> >> +pr_err(FW_BUG "Invalid ID mapping, skipping RMR node %p\n",
> >> + iort_node);
> >> +return;
> >> +}
> >> +
> >> +/* Retrieve associated smmu and stream id */
> >> +smmu = iort_node_get_id(iort_node, , 0);
> >> +if (!smmu) {
> >> +pr_err(FW_BUG "Invalid SMMU reference, skipping RMR node
> >> %p\n",
> >> + iort_node);
> >> +return;
> >> +}
> >> +
> >> +/* Retrieve RMR data */
> >> +rmr = (struct acpi_iort_rmr *)iort_node->node_data;
> >> +if (!rmr->rmr_offset || !rmr->rmr_count) {
> >> +pr_err(FW_BUG "Invalid RMR descriptor array, skipping RMR
> >> node %p\n",
> >> + iort_node);
> >> +return;
> >> +}
> >> +
> >> +rmr_desc = ACPI_ADD_PTR(struct acpi_iort_rmr_desc, iort_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;
> >> +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 & IOMMU_RMR_REMAP_PERMITTED) {
> >> +type = IOMMU_RESV_DIRECT_RELAXABLE;
> >> +/*
> >> + * Set IOMMU_CACHE as IOMMU_RESV_DIRECT_RELAXABLE is
> >> + * normally used for allocated system memory that is
> >> + * then used for device specific reserved regions.
> >> + */
> >> +prot |= IOMMU_CACHE;
> >> +} else {
> >> +type = IOMMU_RESV_DIRECT;
> >> +/*
> >> + * Set IOMMU_MMIO as IOMMU_RESV_DIRECT is normally
> >> used
> >> + * for device memory like MSI doorbell.
> >> + */
> >> +prot |= IOMMU_MMIO;
> >> +}
> >
> > On the prot value assignment based on the remapping flag, I'd like to
> > hear Robin/Joerg's opinion, I'd avoid being in a situation where
> > "normally" this would work but then we have to quirk it.
> >
> > Is this a valid assumption _always_ ?
>
> No. Certainly applying IOMMU_CACHE without reference to the device's
> _CCA attribute or how CPUs may be accessing a shared buffer could lead
> to a loss of coherency. At worst, applying IOMMU_MMIO to a
> device-private buffer *could* cause the device to lose coherency with
> itself if the memory underlying the RMR may have allocated into system
> caches. Note that the expected use for non-remappable RMRs is the device
> holding some sort of long-lived private data in system RAM - the MSI
> doorbell trick is far more of a niche hack really.
>
> At the very least I think we need to refer to the device's memory access
> properties here.
>
> Jon, Laurentiu - how do RMRs correspond to the EFI memory map on your
> firmware? I'm starting to think that as long as the underlying memory is
> described appropriately there then we should be able to infer correct
> attributes from the EFI memory type and flags.
The devices are all cache coherent and marked as _CCA, 1. The Memory
regions are in the virt table as ARM_MEMORY_REGION_ATTRIBUTE_DEVICE.
The current chicken and egg problem we have is that during the fsl-mc-bus
initialization we call
error = acpi_dma_configure_id(>dev, DEV_DMA_COHERENT,
_stream_id);
which gets deferred because the SMMU has not been initialized yet. Then we
initialize the RMR tables but there is no device reference there to be able to
Re: [PATCH v7 0/9] ACPI/IORT: Support for IORT RMR node
On Thu, Aug 5, 2021 at 4:09 PM Ard Biesheuvel wrote: > > On Thu, 5 Aug 2021 at 15:35, Shameerali Kolothum Thodi > wrote: > > > > > > > > > -Original Message- > > > From: Ard Biesheuvel [mailto:a...@kernel.org] > > > Sent: 05 August 2021 14:23 > > > To: Shameerali Kolothum Thodi > > > Cc: Linux ARM ; ACPI Devel Maling > > > List > > > ; Linux IOMMU > > > ; Linuxarm ; > > > Lorenzo Pieralisi ; Joerg Roedel > > > ; Robin Murphy ; Will Deacon > > > ; wanghuiqiang ; Guohanjun > > > (Hanjun Guo) ; Steven Price > > > ; Sami Mujawar ; Jon > > > Nettleton ; Eric Auger ; > > > yangyicong > > > Subject: Re: [PATCH v7 0/9] ACPI/IORT: Support for IORT RMR node > > > > > > On Thu, 5 Aug 2021 at 10:10, Shameer Kolothum > > > wrote: > > > > > > > > Hi, > > > > > > > > The series adds support to IORT RMR nodes specified in IORT > > > > Revision E.b -ARM DEN 0049E[0]. RMR nodes are used to describe > > > > memory ranges that are used by endpoints and require a unity > > > > mapping in SMMU. > > > > > > > > We have faced issues with 3408iMR RAID controller cards which > > > > fail to boot when SMMU is enabled. This is because these > > > > controllers make use of host memory for various caching related > > > > purposes and when SMMU is enabled the iMR firmware fails to > > > > access these memory regions as there is no mapping for them. > > > > IORT RMR provides a way for UEFI to describe and report these > > > > memory regions so that the kernel can make a unity mapping for > > > > these in SMMU. > > > > > > > > > > Does this mean we are ignoring the RMR memory ranges, and exposing the > > > entire physical address space to devices using the stream IDs in > > > question? > > > > Nope. RMR node is used to describe the memory ranges used by end points > > behind SMMU. And this information is used to create 1 : 1 mappings for those > > ranges in SMMU. Anything outside those ranges will result in translation > > fault(if there are no other dynamic DMA mappings). > > > > Excellent! It was not obvious to me from looking at the patches, so I > had to ask. > > Thanks, > Ard. > > > > > > > > > > Change History: > > > > > > > > v6 --> v7 > > > > > > > > The only change from v6 is the fix pointed out by Steve to > > > > the SMMUv2 SMR bypass install in patch #8. > > > > > > > > Thanks to the Tested-by tags by Laurentiu with SMMUv2 and > > > > Hanjun/Huiqiang with SMMUv3 for v6. I haven't added the tags > > > > yet as the series still needs more review[1]. > > > > > > > > Feedback and tests on this series is very much appreciated. > > > > > > > > v5 --> v6 > > > > - Addressed comments from Robin & Lorenzo. > > > > : Moved iort_parse_rmr() to acpi_iort_init() from > > > > iort_init_platform_devices(). > > > > : Removed use of struct iort_rmr_entry during the initial > > > > parse. Using struct iommu_resv_region instead. > > > > : Report RMR address alignment and overlap errors, but continue. > > > > : Reworked arm_smmu_init_bypass_stes() (patch # 6). > > > > - Updated SMMUv2 bypass SMR code. Thanks to Jon N (patch #8). > > > > - Set IOMMU protection flags(IOMMU_CACHE, IOMMU_MMIO) based > > > > on Type of RMR region. Suggested by Jon N. > > > > > > > > Thanks, > > > > Shameer > > > > [0] https://developer.arm.com/documentation/den0049/latest/ > > > > [1] > > > https://lore.kernel.org/linux-acpi/20210716083442.1708-1-shameerali.koloth > > > um.th...@huawei.com/T/#m043c95b869973a834b2fd57f3e1ed0325c84f3b7 > > > > -- > > > > v4 --> v5 > > > > -Added a fw_data union to struct iommu_resv_region and removed > > > > struct iommu_rmr (Based on comments from Joerg/Robin). > > > > -Added iommu_put_rmrs() to release mem. > > > > -Thanks to Steve for verifying on SMMUv2, but not added the Tested-by > > > > yet because of the above changes. > > > > > > > > v3 -->v4 > > > > -Included the SMMUv2 SMR bypass install changes suggested by > > > > Steve(patch #7) > > > > -As per Robin's comments
Re: [PATCH v7 2/9] ACPI/IORT: Add support for RMR node parsing
On Thu, Aug 5, 2021 at 6:03 PM Lorenzo Pieralisi
wrote:
>
> On Thu, Aug 05, 2021 at 09:07:17AM +0100, Shameer Kolothum wrote:
>
> [...]
>
> > +static void __init iort_node_get_rmr_info(struct acpi_iort_node *iort_node)
> > +{
> > + struct acpi_iort_node *smmu;
> > + struct acpi_iort_rmr *rmr;
> > + struct acpi_iort_rmr_desc *rmr_desc;
> > + u32 map_count = iort_node->mapping_count;
> > + u32 sid;
> > + int i;
> > +
> > + if (!iort_node->mapping_offset || map_count != 1) {
> > + pr_err(FW_BUG "Invalid ID mapping, skipping RMR node %p\n",
> > +iort_node);
> > + return;
> > + }
> > +
> > + /* Retrieve associated smmu and stream id */
> > + smmu = iort_node_get_id(iort_node, , 0);
> > + if (!smmu) {
> > + pr_err(FW_BUG "Invalid SMMU reference, skipping RMR node
> > %p\n",
> > +iort_node);
> > + return;
> > + }
> > +
> > + /* Retrieve RMR data */
> > + rmr = (struct acpi_iort_rmr *)iort_node->node_data;
> > + if (!rmr->rmr_offset || !rmr->rmr_count) {
> > + pr_err(FW_BUG "Invalid RMR descriptor array, skipping RMR
> > node %p\n",
> > +iort_node);
> > + return;
> > + }
> > +
> > + rmr_desc = ACPI_ADD_PTR(struct acpi_iort_rmr_desc, iort_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;
> > + 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 & IOMMU_RMR_REMAP_PERMITTED) {
> > + type = IOMMU_RESV_DIRECT_RELAXABLE;
> > + /*
> > + * Set IOMMU_CACHE as IOMMU_RESV_DIRECT_RELAXABLE is
> > + * normally used for allocated system memory that is
> > + * then used for device specific reserved regions.
> > + */
> > + prot |= IOMMU_CACHE;
> > + } else {
> > + type = IOMMU_RESV_DIRECT;
> > + /*
> > + * Set IOMMU_MMIO as IOMMU_RESV_DIRECT is normally
> > used
> > + * for device memory like MSI doorbell.
> > + */
> > + prot |= IOMMU_MMIO;
> > + }
>
> On the prot value assignment based on the remapping flag, I'd like to
> hear Robin/Joerg's opinion, I'd avoid being in a situation where
> "normally" this would work but then we have to quirk it.
>
> Is this a valid assumption _always_ ?
These assumptions were made based on the historic use cases I could
find reading the history. There aren't many known examples "in the wild"
because so far we haven't had a mechanism other than quirks based
around device-tree implementations.
Ultimately I believe the proper solution will need to be another flag
in the RMR table that specifies the type of memory an RMR Node
describes, not just the base and length.
-Jon
>
> Thanks,
> Lorenzo
>
> > +
> > + region = iommu_alloc_resv_region(addr, size, prot, type);
> > + if (region) {
> > + region->fw_data.rmr.flags = rmr->flags;
> > + region->fw_data.rmr.sid = sid;
> > + region->fw_data.rmr.smmu = smmu;
> > + list_add_tail(>list, _rmr_list);
> > + }
> > + }
> > +}
> > +
> > +static void __init iort_parse_rmr(void)
> > +{
> > + struct acpi_iort_node *iort_node, *iort_end;
> > + struct acpi_table_iort *iort;
> > + int i;
> > +
> > + if (iort_table->revision < 3)
> > + 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,
> >
Re: [PATCH v6 8/9] iommu/arm-smmu: Get associated RMR info and install bypass SMR
On Fri, Jul 16, 2021 at 3:52 PM Steven Price wrote:
>
> On 16/07/2021 09:34, Shameer Kolothum wrote:
> > From: Jon Nettleton
> >
> > Check if there is any RMR info associated with the devices behind
> > the SMMU and if any, install bypass SMRs for them. This is to
> > keep any ongoing traffic associated with these devices alive
> > when we enable/reset SMMU during probe().
> >
> > Signed-off-by: Jon Nettleton
> > Signed-off-by: Steven Price
> > Signed-off-by: Shameer Kolothum
> > ---
> > drivers/iommu/arm/arm-smmu/arm-smmu.c | 48 +++
> > 1 file changed, 48 insertions(+)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > index f22dbeb1e510..e9fb3d962a86 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > @@ -2063,6 +2063,50 @@ err_reset_platform_ops: __maybe_unused;
> > return err;
> > }
> >
> > +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
> > +{
> > + struct list_head rmr_list;
> > + struct iommu_resv_region *e;
> > + int i, cnt = 0;
> > + u32 reg;
> > +
> > + INIT_LIST_HEAD(_list);
> > + if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), _list))
> > + return;
> > +
> > + /*
> > + * Rather than trying to look at existing mappings that
> > + * are setup by the firmware and then invalidate the ones
> > + * that do no have matching RMR entries, just disable the
> > + * SMMU until it gets enabled again in the reset routine.
> > + */
> > + reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> > + reg &= ~ARM_SMMU_sCR0_CLIENTPD;
>
> This looks backwards, the spec states:
>
> Client Port Disable. The possible values of this bit are:
> 0 - Each SMMU client access is subject to SMMU translation, access
> control, and attribute generation.
> 1 - Each SMMU client access bypasses SMMU translation, access control,
> and attribute generation.
> This bit resets to 1.
>
> And indeed with the current code if sCR0_USFCFG was set when
> sCR0_CLIENTPD is cleared then I get a blank screen until the smmu is reset.
>
> So I believe this should be ORing in the value, i.e.
>
> reg |= ARM_SMMU_sCR0_CLIENTPD;
>
> And in my testing that works fine even if sCR0_USFCFG is set.
Sorry that is my bad. It was a hurried and sloppy copy paste on my part.
Thanks for catching it
-Jon
>
> Steve
>
> > + arm_smmu_gr0_write(smmu, ARM_SMMU_GR0_sCR0, reg);
> > +
> > + list_for_each_entry(e, _list, list) {
> > + u32 sid = e->fw_data.rmr.sid;
> > +
> > + i = arm_smmu_find_sme(smmu, sid, ~0);
> > + if (i < 0)
> > + continue;
> > + if (smmu->s2crs[i].count == 0) {
> > + smmu->smrs[i].id = sid;
> > + smmu->smrs[i].mask = 0;
> > + smmu->smrs[i].valid = true;
> > + }
> > + smmu->s2crs[i].count++;
> > + smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> > + smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> > +
> > + cnt++;
> > + }
> > +
> > + dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
> > +cnt == 1 ? "" : "s");
> > + iommu_dma_put_rmrs(dev_fwnode(smmu->dev), _list);
> > +}
> > +
> > static int arm_smmu_device_probe(struct platform_device *pdev)
> > {
> > struct resource *res;
> > @@ -2189,6 +2233,10 @@ static int arm_smmu_device_probe(struct
> > platform_device *pdev)
> > }
> >
> > platform_set_drvdata(pdev, smmu);
> > +
> > + /* Check for RMRs and install bypass SMRs if any */
> > + arm_smmu_rmr_install_bypass_smr(smmu);
> > +
> > arm_smmu_device_reset(smmu);
> > arm_smmu_test_smr_masks(smmu);
> >
> >
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
On Tue, Jun 29, 2021 at 7:34 PM Jon Nettleton wrote:
>
> On Mon, Jun 14, 2021 at 2:49 PM Shameerali Kolothum Thodi
> wrote:
> >
> >
> >
> > > -Original Message-
> > > From: Robin Murphy [mailto:robin.mur...@arm.com]
> > > Sent: 14 June 2021 12:23
> > > To: Shameerali Kolothum Thodi ;
> > > linux-arm-ker...@lists.infradead.org; linux-a...@vger.kernel.org;
> > > iommu@lists.linux-foundation.org
> > > Cc: j...@solid-run.com; Linuxarm ;
> > > steven.pr...@arm.com; Guohanjun (Hanjun Guo) ;
> > > yangyicong ; sami.muja...@arm.com;
> > > wanghuiqiang
> > > Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
> > > regions
> > >
> > > On 2021-05-24 12:02, Shameer Kolothum wrote:
> > > > Add a helper function that retrieves RMR memory descriptors
> > > > associated with a given IOMMU. This will be used by IOMMU
> > > > drivers to setup necessary mappings.
> > > >
> > > > Now that we have this, invoke it from the generic helper
> > > > interface.
> > > >
> > > > Signed-off-by: Shameer Kolothum
> > >
> > > > ---
> > > > drivers/acpi/arm64/iort.c | 50
> > > +++
> > > > drivers/iommu/dma-iommu.c | 4
> > > > include/linux/acpi_iort.h | 7 ++
> > > > 3 files changed, 61 insertions(+)
> > > >
> > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > > index fea1ffaedf3b..01917caf58de 100644
> > > > --- a/drivers/acpi/arm64/iort.c
> > > > +++ b/drivers/acpi/arm64/iort.c
> > > > @@ -12,6 +12,7 @@
> > > >
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct
> > > device *dev)
> > > > return err;
> > > > }
> > > >
> > > > +/**
> > > > + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with
> > > IOMMU
> > > > + * @iommu: fwnode for the IOMMU
> > > > + * @head: RMR list head to be populated
> > > > + *
> > > > + * Returns: 0 on success, <0 failure
> > > > + */
> > > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > > + struct list_head *head)
> > > > +{
> > > > + struct iort_rmr_entry *e;
> > > > + struct acpi_iort_node *iommu;
> > > > + int rmrs = 0;
> > > > +
> > > > + iommu = iort_get_iort_node(iommu_fwnode);
> > > > + if (!iommu || list_empty(_rmr_list))
> > > > + return -ENODEV;
> > > > +
> > > > + list_for_each_entry(e, _rmr_list, list) {
> > > > + int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC |
> > > IOMMU_MMIO;
> > > > + struct iommu_resv_region *region;
> > > > + enum iommu_resv_type type;
> > > > + struct acpi_iort_rmr_desc *rmr_desc;
> > > > +
> > > > + if (e->smmu != iommu)
> > > > + continue;
> > > > +
> > > > + rmr_desc = e->rmr_desc;
> > > > + if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> > > > + type = IOMMU_RESV_DIRECT_RELAXABLE;
> > > > + else
> > > > + type = IOMMU_RESV_DIRECT;
> > >
>
> I have been looking at this a bit more and looking at the history of
> RMR_REMAP_PERMITTED. Based on the history I have found it
> seems to me like this would be the better options for prot.
>
> @@ -840,7 +840,7 @@ int iort_iommu_get_rmrs(struct fwnode_handle
> *iommu_fwnode,
> return -ENODEV;
>
> list_for_each_entry(e, _rmr_list, list) {
> - int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC |
> IOMMU_MMIO;
> + int prot = IOMMU_READ | IOMMU_WRITE;
> struct iommu_resv_region *region;
> enum iommu_resv_type type;
> struct acpi_iort_rmr_desc *rmr_desc;
> @@ -849,10 +849,13 @@ int iort_iommu_get_rmrs(struct fwnode_handle
> *iommu_fwnode,
> continue;
>
> rmr_desc = e->rmr_desc;
> - i
Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
On Mon, Jun 14, 2021 at 2:49 PM Shameerali Kolothum Thodi
wrote:
>
>
>
> > -Original Message-
> > From: Robin Murphy [mailto:robin.mur...@arm.com]
> > Sent: 14 June 2021 12:23
> > To: Shameerali Kolothum Thodi ;
> > linux-arm-ker...@lists.infradead.org; linux-a...@vger.kernel.org;
> > iommu@lists.linux-foundation.org
> > Cc: j...@solid-run.com; Linuxarm ;
> > steven.pr...@arm.com; Guohanjun (Hanjun Guo) ;
> > yangyicong ; sami.muja...@arm.com;
> > wanghuiqiang
> > Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
> > regions
> >
> > On 2021-05-24 12:02, Shameer Kolothum wrote:
> > > Add a helper function that retrieves RMR memory descriptors
> > > associated with a given IOMMU. This will be used by IOMMU
> > > drivers to setup necessary mappings.
> > >
> > > Now that we have this, invoke it from the generic helper
> > > interface.
> > >
> > > Signed-off-by: Shameer Kolothum
> >
> > > ---
> > > drivers/acpi/arm64/iort.c | 50
> > +++
> > > drivers/iommu/dma-iommu.c | 4
> > > include/linux/acpi_iort.h | 7 ++
> > > 3 files changed, 61 insertions(+)
> > >
> > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > index fea1ffaedf3b..01917caf58de 100644
> > > --- a/drivers/acpi/arm64/iort.c
> > > +++ b/drivers/acpi/arm64/iort.c
> > > @@ -12,6 +12,7 @@
> > >
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct
> > device *dev)
> > > return err;
> > > }
> > >
> > > +/**
> > > + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with
> > IOMMU
> > > + * @iommu: fwnode for the IOMMU
> > > + * @head: RMR list head to be populated
> > > + *
> > > + * Returns: 0 on success, <0 failure
> > > + */
> > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > + struct list_head *head)
> > > +{
> > > + struct iort_rmr_entry *e;
> > > + struct acpi_iort_node *iommu;
> > > + int rmrs = 0;
> > > +
> > > + iommu = iort_get_iort_node(iommu_fwnode);
> > > + if (!iommu || list_empty(_rmr_list))
> > > + return -ENODEV;
> > > +
> > > + list_for_each_entry(e, _rmr_list, list) {
> > > + int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC |
> > IOMMU_MMIO;
> > > + struct iommu_resv_region *region;
> > > + enum iommu_resv_type type;
> > > + struct acpi_iort_rmr_desc *rmr_desc;
> > > +
> > > + if (e->smmu != iommu)
> > > + continue;
> > > +
> > > + rmr_desc = e->rmr_desc;
> > > + if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
> > > + type = IOMMU_RESV_DIRECT_RELAXABLE;
> > > + else
> > > + type = IOMMU_RESV_DIRECT;
> >
I have been looking at this a bit more and looking at the history of
RMR_REMAP_PERMITTED. Based on the history I have found it
seems to me like this would be the better options for prot.
@@ -840,7 +840,7 @@ int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
return -ENODEV;
list_for_each_entry(e, _rmr_list, list) {
- int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+ int prot = IOMMU_READ | IOMMU_WRITE;
struct iommu_resv_region *region;
enum iommu_resv_type type;
struct acpi_iort_rmr_desc *rmr_desc;
@@ -849,10 +849,13 @@ int iort_iommu_get_rmrs(struct fwnode_handle
*iommu_fwnode,
continue;
rmr_desc = e->rmr_desc;
- if (e->flags & IOMMU_RMR_REMAP_PERMITTED)
+ if (e->flags & IOMMU_RMR_REMAP_PERMITTED) {
type = IOMMU_RESV_DIRECT_RELAXABLE;
- else
+ prot |= IOMMU_CACHE;
+ } else {
type = IOMMU_RESV_DIRECT;
+ prot |= IOMMU_MMIO;
+ }
region = iommu_alloc_resv_region(rmr_desc->base_address,
rmr_desc->length,
any input on this? My reasoning is that IOMMU_RESV_DIRECT is specifically
referenced for things like MSI doorbell and in all the examples needs
IOMMU_MMIO, while REMAP_PERMITTED seems to be used for allocated
system memory that is then used for device specific reserved regions which
commits say would be like a GPU or USB device.
-Jon
> > Wasn't the idea that we can do all this during the initial parsing, i.e.
> > we don't even have iort_rmr_entry, we store them as iommu_resv_regions
> > and simply kmemdup() or whatever at this point?
>
>
> Hmm... Not yet. I removed struct iommu_rmr() from v4. But yes, it looks like
> we can get rid of iort_rmr_entry as well. Will give it a go in next.
>
> Thanks,
> Shameer
>
> > Robin.
> >
> > > +
> > > + region =
Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
On Tue, Jun 29, 2021 at 3:23 PM Robin Murphy wrote:
>
> On 2021-06-29 08:03, Jon Nettleton wrote:
> > On Mon, Jun 14, 2021 at 12:06 PM Robin Murphy wrote:
> >>
> >> On 2021-05-24 12:02, Shameer Kolothum wrote:
> >>> From: Jon Nettleton
> >>>
> >>> Check if there is any RMR info associated with the devices behind
> >>> the SMMU and if any, install bypass SMRs for them. This is to
> >>> keep any ongoing traffic associated with these devices alive
> >>> when we enable/reset SMMU during probe().
> >>>
> >>> Signed-off-by: Jon Nettleton
> >>> Signed-off-by: Steven Price
> >>> Signed-off-by: Shameer Kolothum
> >>> ---
> >>>drivers/iommu/arm/arm-smmu/arm-smmu.c | 65 +++
> >>>1 file changed, 65 insertions(+)
> >>>
> >>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >>> b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >>> index 6f72c4d208ca..56db3d3238fc 100644
> >>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >>> @@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
> >>>return err;
> >>>}
> >>>
> >>> +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
> >>> +{
> >>> + struct list_head rmr_list;
> >>> + struct iommu_resv_region *e;
> >>> + int i, cnt = 0;
> >>> + u32 smr;
> >>> + u32 reg;
> >>> +
> >>> + INIT_LIST_HEAD(_list);
> >>> + if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), _list))
> >>> + return;
> >>> +
> >>> + reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> >>> +
> >>> + if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg &
> >>> ARM_SMMU_sCR0_CLIENTPD)) {
> >>> + /*
> >>> + * SMMU is already enabled and disallowing bypass, so
> >>> preserve
> >>> + * the existing SMRs
> >>> + */
> >>> + for (i = 0; i < smmu->num_mapping_groups; i++) {
> >>> + smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
> >>
> >> To reiterate, just because a bootloader/crashed kernel/whatever may have
> >> left some configuration behind doesn't mean that it matters (e.g. what
> >> if these SMRs are pointing at translation contexts?). All we should be
> >> doing here is setting the relevant RMR accommodations in our "clean
> >> slate" software state before the reset routine applies it to the
> >> hardware, just like patch #5 does for SMMUv3.
> >>
> >> Trying to safely reset an SMMU when we discover it with CLIENTPD=0 is
> >> really another issue entirely, and I'd say is beyond the scope of this
> >> series
> >>
> >>> + if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> >>> + continue;
> >>
> >> Note that that's not even necessarily correct (thanks to EXIDS).
> >>
> >>> + smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> >>> + smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK,
> >>> smr);
> >>> + smmu->smrs[i].valid = true;
> >>> + }
> >>> + }
> >>> +
> >>> + list_for_each_entry(e, _list, list) {
> >>> + u32 sid = e->fw_data.rmr.sid;
> >>> +
> >>> + i = arm_smmu_find_sme(smmu, sid, ~0);
> >>> + if (i < 0)
> >>> + continue;
> >>> + if (smmu->s2crs[i].count == 0) {
> >>> + smmu->smrs[i].id = sid;
> >>> + smmu->smrs[i].mask = ~0;
> >>
> >> This is very wrong (as has now already been pointed out).
> >>
> >>> + smmu->smrs[i].valid = true;
> >>> + }
> >>> + smmu->s2crs[i].count++;
> >>> + smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> >>> + smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> >>> + smmu->s2crs[i].cbndx = 0xff;
> >>
> >> Nit: cbndx is left uninitialised for byp
Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
On Mon, Jun 14, 2021 at 12:06 PM Robin Murphy wrote:
>
> On 2021-05-24 12:02, Shameer Kolothum wrote:
> > From: Jon Nettleton
> >
> > Check if there is any RMR info associated with the devices behind
> > the SMMU and if any, install bypass SMRs for them. This is to
> > keep any ongoing traffic associated with these devices alive
> > when we enable/reset SMMU during probe().
> >
> > Signed-off-by: Jon Nettleton
> > Signed-off-by: Steven Price
> > Signed-off-by: Shameer Kolothum
> > ---
> > drivers/iommu/arm/arm-smmu/arm-smmu.c | 65 +++
> > 1 file changed, 65 insertions(+)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > index 6f72c4d208ca..56db3d3238fc 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > @@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
> > return err;
> > }
> >
> > +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
> > +{
> > + struct list_head rmr_list;
> > + struct iommu_resv_region *e;
> > + int i, cnt = 0;
> > + u32 smr;
> > + u32 reg;
> > +
> > + INIT_LIST_HEAD(_list);
> > + if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), _list))
> > + return;
> > +
> > + reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> > +
> > + if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> > + /*
> > + * SMMU is already enabled and disallowing bypass, so preserve
> > + * the existing SMRs
> > + */
> > + for (i = 0; i < smmu->num_mapping_groups; i++) {
> > + smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
>
> To reiterate, just because a bootloader/crashed kernel/whatever may have
> left some configuration behind doesn't mean that it matters (e.g. what
> if these SMRs are pointing at translation contexts?). All we should be
> doing here is setting the relevant RMR accommodations in our "clean
> slate" software state before the reset routine applies it to the
> hardware, just like patch #5 does for SMMUv3.
>
> Trying to safely reset an SMMU when we discover it with CLIENTPD=0 is
> really another issue entirely, and I'd say is beyond the scope of this
> series
>
> > + if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> > + continue;
>
> Note that that's not even necessarily correct (thanks to EXIDS).
>
> > + smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> > + smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK,
> > smr);
> > + smmu->smrs[i].valid = true;
> > + }
> > + }
> > +
> > + list_for_each_entry(e, _list, list) {
> > + u32 sid = e->fw_data.rmr.sid;
> > +
> > + i = arm_smmu_find_sme(smmu, sid, ~0);
> > + if (i < 0)
> > + continue;
> > + if (smmu->s2crs[i].count == 0) {
> > + smmu->smrs[i].id = sid;
> > + smmu->smrs[i].mask = ~0;
>
> This is very wrong (as has now already been pointed out).
>
> > + smmu->smrs[i].valid = true;
> > + }
> > + smmu->s2crs[i].count++;
> > + smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> > + smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> > + smmu->s2crs[i].cbndx = 0xff;
>
> Nit: cbndx is left uninitialised for bypass/fault entries elsewhere, so
> there's little point touching it here.
>
> > +
> > + cnt++;
> > + }
> > +
> > + if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> > + /* Remove the valid bit for unused SMRs */
> > + for (i = 0; i < smmu->num_mapping_groups; i++) {
> > + if (smmu->s2crs[i].count == 0)
> > + smmu->smrs[i].valid = false;
> > + }
>
> If this dance is only about avoiding stream match conflicts when trying
> to reprogram live SMRs, simply turning the SMMU off beforehand would be
> a lot simpler.
Robin,
I am not sure what you mean here, and maybe Steve wants to jump in and
help clarify.
My understanding is that "dance" is required for re
Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
On Mon, Jun 14, 2021 at 6:51 PM Shameerali Kolothum Thodi
wrote:
>
>
>
> > -Original Message-
> > From: Robin Murphy [mailto:robin.mur...@arm.com]
> > Sent: 14 June 2021 11:06
> > To: Shameerali Kolothum Thodi ;
> > linux-arm-ker...@lists.infradead.org; linux-a...@vger.kernel.org;
> > iommu@lists.linux-foundation.org
> > Cc: j...@solid-run.com; Linuxarm ;
> > steven.pr...@arm.com; Guohanjun (Hanjun Guo) ;
> > yangyicong ; sami.muja...@arm.com;
> > wanghuiqiang
> > Subject: Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and
> > install bypass SMR
> >
> > On 2021-05-24 12:02, Shameer Kolothum wrote:
> > > From: Jon Nettleton
> > >
> > > Check if there is any RMR info associated with the devices behind
> > > the SMMU and if any, install bypass SMRs for them. This is to
> > > keep any ongoing traffic associated with these devices alive
> > > when we enable/reset SMMU during probe().
> > >
> > > Signed-off-by: Jon Nettleton
> > > Signed-off-by: Steven Price
> > > Signed-off-by: Shameer Kolothum
> >
> > > ---
> > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 65
> > +++
> > > 1 file changed, 65 insertions(+)
> > >
> > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > index 6f72c4d208ca..56db3d3238fc 100644
> > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > @@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
> > > return err;
> > > }
> > >
> > > +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device
> > *smmu)
> > > +{
> > > + struct list_head rmr_list;
> > > + struct iommu_resv_region *e;
> > > + int i, cnt = 0;
> > > + u32 smr;
> > > + u32 reg;
> > > +
> > > + INIT_LIST_HEAD(_list);
> > > + if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), _list))
> > > + return;
> > > +
> > > + reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> > > +
> > > + if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg &
> > ARM_SMMU_sCR0_CLIENTPD)) {
> > > + /*
> > > +* SMMU is already enabled and disallowing bypass, so preserve
> > > +* the existing SMRs
> > > +*/
> > > + for (i = 0; i < smmu->num_mapping_groups; i++) {
> > > + smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
> >
> > To reiterate, just because a bootloader/crashed kernel/whatever may have
> > left some configuration behind doesn't mean that it matters (e.g. what
> > if these SMRs are pointing at translation contexts?). All we should be
> > doing here is setting the relevant RMR accommodations in our "clean
> > slate" software state before the reset routine applies it to the
> > hardware, just like patch #5 does for SMMUv3.
> >
> > Trying to safely reset an SMMU when we discover it with CLIENTPD=0 is
> > really another issue entirely, and I'd say is beyond the scope of this
> > series
> >
> > > + if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> > > + continue;
> >
> > Note that that's not even necessarily correct (thanks to EXIDS).
> >
> > > + smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> > > + smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK,
> > smr);
> > > + smmu->smrs[i].valid = true;
> > > + }
> > > + }
> > > +
> > > + list_for_each_entry(e, _list, list) {
> > > + u32 sid = e->fw_data.rmr.sid;
> > > +
> > > + i = arm_smmu_find_sme(smmu, sid, ~0);
> > > + if (i < 0)
> > > + continue;
> > > + if (smmu->s2crs[i].count == 0) {
> > > + smmu->smrs[i].id = sid;
> > > + smmu->smrs[i].mask = ~0;
> >
> > This is very wrong (as has now already been pointed out).
> >
> > > + smmu->smrs[i].valid = true;
> > > + }
> > > + smmu->s2crs[i].count++;
> > > + smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> > > + smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> >
Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
On Thu, Jun 3, 2021 at 1:51 PM Jon Nettleton wrote:
>
> On Thu, Jun 3, 2021 at 1:27 PM Steven Price wrote:
> >
> > On 03/06/2021 09:52, Jon Nettleton wrote:
> > > On Mon, May 24, 2021 at 1:04 PM Shameer Kolothum
> > > wrote:
> > >>
> > >> From: Jon Nettleton
> > >>
> > >> Check if there is any RMR info associated with the devices behind
> > >> the SMMU and if any, install bypass SMRs for them. This is to
> > >> keep any ongoing traffic associated with these devices alive
> > >> when we enable/reset SMMU during probe().
> > >>
> > >> Signed-off-by: Jon Nettleton
> > >> Signed-off-by: Steven Price
> > >> Signed-off-by: Shameer Kolothum
> > >> ---
> > >> drivers/iommu/arm/arm-smmu/arm-smmu.c | 65 +++
> > >> 1 file changed, 65 insertions(+)
> > >>
> > >> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > >> b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > >> index 6f72c4d208ca..56db3d3238fc 100644
> > >> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > >> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > >> @@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
> > >> return err;
> > >> }
> > >>
> > >> +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device
> > >> *smmu)
> > >> +{
> > >> + struct list_head rmr_list;
> > >> + struct iommu_resv_region *e;
> > >> + int i, cnt = 0;
> > >> + u32 smr;
> > >> + u32 reg;
> > >> +
> > >> + INIT_LIST_HEAD(_list);
> > >> + if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), _list))
> > >> + return;
> > >> +
> > >> + reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> > >> +
> > >> + if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg &
> > >> ARM_SMMU_sCR0_CLIENTPD)) {
> > >> + /*
> > >> +* SMMU is already enabled and disallowing bypass, so
> > >> preserve
> > >> +* the existing SMRs
> > >> +*/
> > >> + for (i = 0; i < smmu->num_mapping_groups; i++) {
> > >> + smr = arm_smmu_gr0_read(smmu,
> > >> ARM_SMMU_GR0_SMR(i));
> > >> + if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> > >> + continue;
> > >> + smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID,
> > >> smr);
> > >> + smmu->smrs[i].mask =
> > >> FIELD_GET(ARM_SMMU_SMR_MASK, smr);
> > >> + smmu->smrs[i].valid = true;
> > >> + }
> > >> + }
> > >> +
> > >> + list_for_each_entry(e, _list, list) {
> > >> + u32 sid = e->fw_data.rmr.sid;
> > >> +
> > >> + i = arm_smmu_find_sme(smmu, sid, ~0);
> > >> + if (i < 0)
> > >> + continue;
> > >> + if (smmu->s2crs[i].count == 0) {
> > >> + smmu->smrs[i].id = sid;
> > >> + smmu->smrs[i].mask = ~0;
> >
> > Looking at this code again, that mask looks wrong! According to the SMMU
> > spec MASK[i]==1 means ID[i] is ignored. I.e. this means completely
> > ignore the ID...
> >
> > I'm not at all sure why they designed the hardware that way round.
> >
> > >> + smmu->smrs[i].valid = true;
> > >> + }
> > >> + smmu->s2crs[i].count++;
> > >> + smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> > >> + smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> > >> + smmu->s2crs[i].cbndx = 0xff;
> > >> +
> > >> + cnt++;
> > >> + }
> > >> +
> > >> + if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg &
> > >> ARM_SMMU_sCR0_CLIENTPD)) {
> > >> + /* Remove the valid bit for unused SMRs */
> > >> + for (i = 0; i < smmu->num_mapping_groups; i++) {
> > >> + if
Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
On Wed, May 26, 2021 at 7:11 PM Laurentiu Tudor wrote:
>
>
>
> On 5/26/2021 7:36 PM, Shameerali Kolothum Thodi wrote:
> >
> >
> >> -Original Message-
> >> From: Laurentiu Tudor [mailto:laurentiu.tu...@nxp.com]
> >> Sent: 26 May 2021 08:53
> >> To: Shameerali Kolothum Thodi ;
> >> linux-arm-ker...@lists.infradead.org; linux-a...@vger.kernel.org;
> >> iommu@lists.linux-foundation.org
> >> Cc: j...@solid-run.com; Linuxarm ;
> >> steven.pr...@arm.com; Guohanjun (Hanjun Guo) ;
> >> yangyicong ; sami.muja...@arm.com;
> >> robin.mur...@arm.com; wanghuiqiang
> >> Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
> >> regions
> >>
> >> Hi Shameer,
> >>
> >> On 5/24/2021 2:02 PM, Shameer Kolothum wrote:
> >>> Add a helper function that retrieves RMR memory descriptors
> >>> associated with a given IOMMU. This will be used by IOMMU
> >>> drivers to setup necessary mappings.
> >>>
> >>> Now that we have this, invoke it from the generic helper
> >>> interface.
> >>>
> >>> Signed-off-by: Shameer Kolothum
> >>
> >>> ---
> >>> drivers/acpi/arm64/iort.c | 50
> >> +++
> >>> drivers/iommu/dma-iommu.c | 4
> >>> include/linux/acpi_iort.h | 7 ++
> >>> 3 files changed, 61 insertions(+)
> >>>
> >>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> >>> index fea1ffaedf3b..01917caf58de 100644
> >>> --- a/drivers/acpi/arm64/iort.c
> >>> +++ b/drivers/acpi/arm64/iort.c
> >>> @@ -12,6 +12,7 @@
> >>>
> >>> #include
> >>> #include
> >>> +#include
> >>> #include
> >>> #include
> >>> #include
> >>> @@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct
> >> device *dev)
> >>> return err;
> >>> }
> >>>
> >>> +/**
> >>> + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with
> >> IOMMU
> >>> + * @iommu: fwnode for the IOMMU
> >>> + * @head: RMR list head to be populated
> >>> + *
> >>> + * Returns: 0 on success, <0 failure
> >>> + */
> >>> +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> >>> + struct list_head *head)
> >>> +{
> >>> + struct iort_rmr_entry *e;
> >>> + struct acpi_iort_node *iommu;
> >>> + int rmrs = 0;
> >>> +
> >>> + iommu = iort_get_iort_node(iommu_fwnode);
> >>> + if (!iommu || list_empty(_rmr_list))
> >>> + return -ENODEV;
> >>> +
> >>> + list_for_each_entry(e, _rmr_list, list) {
> >>> + int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC |
> >> IOMMU_MMIO;
> >>
> >> We have a case with an IP block that needs EXEC rights on its reserved
> >> memory, so could you please drop the IOMMU_NOEXEC flag?
> >
> > Ok, I think I can drop that one if there are no other concerns. I was not
> > quite
> > sure what to include here in the first place as the IORT spec is not giving
> > any
> > further details about the RMR regions(May be the flags field can be
> > extended to
> > describe these details).
> >
>
> That would be great, given that some preliminary investigations on my
> side revealed that our IP block seems to be quite sensitive to memory
> attributes. I need to spend some more time on this but at first sight
> looks like it needs cacheable, normal memory (not mmio mapping).
>
> ---
> Thanks & Best Regards, Laurentiu
Laurentiu,
Is this regarding the mc-bin memory block or another IP? I am currently
running this patchset with IOMMU_NOEXEC under ACPI without any problems.
If so maybe we can touch base off list and align on the implementation.
-Jon
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
On Thu, Jun 3, 2021 at 1:27 PM Steven Price wrote:
>
> On 03/06/2021 09:52, Jon Nettleton wrote:
> > On Mon, May 24, 2021 at 1:04 PM Shameer Kolothum
> > wrote:
> >>
> >> From: Jon Nettleton
> >>
> >> Check if there is any RMR info associated with the devices behind
> >> the SMMU and if any, install bypass SMRs for them. This is to
> >> keep any ongoing traffic associated with these devices alive
> >> when we enable/reset SMMU during probe().
> >>
> >> Signed-off-by: Jon Nettleton
> >> Signed-off-by: Steven Price
> >> Signed-off-by: Shameer Kolothum
> >> ---
> >> drivers/iommu/arm/arm-smmu/arm-smmu.c | 65 +++
> >> 1 file changed, 65 insertions(+)
> >>
> >> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >> b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >> index 6f72c4d208ca..56db3d3238fc 100644
> >> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> >> @@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
> >> return err;
> >> }
> >>
> >> +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
> >> +{
> >> + struct list_head rmr_list;
> >> + struct iommu_resv_region *e;
> >> + int i, cnt = 0;
> >> + u32 smr;
> >> + u32 reg;
> >> +
> >> + INIT_LIST_HEAD(_list);
> >> + if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), _list))
> >> + return;
> >> +
> >> + reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> >> +
> >> + if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg &
> >> ARM_SMMU_sCR0_CLIENTPD)) {
> >> + /*
> >> +* SMMU is already enabled and disallowing bypass, so
> >> preserve
> >> +* the existing SMRs
> >> +*/
> >> + for (i = 0; i < smmu->num_mapping_groups; i++) {
> >> + smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
> >> + if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> >> + continue;
> >> + smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> >> + smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK,
> >> smr);
> >> + smmu->smrs[i].valid = true;
> >> + }
> >> + }
> >> +
> >> + list_for_each_entry(e, _list, list) {
> >> + u32 sid = e->fw_data.rmr.sid;
> >> +
> >> + i = arm_smmu_find_sme(smmu, sid, ~0);
> >> + if (i < 0)
> >> + continue;
> >> + if (smmu->s2crs[i].count == 0) {
> >> + smmu->smrs[i].id = sid;
> >> + smmu->smrs[i].mask = ~0;
>
> Looking at this code again, that mask looks wrong! According to the SMMU
> spec MASK[i]==1 means ID[i] is ignored. I.e. this means completely
> ignore the ID...
>
> I'm not at all sure why they designed the hardware that way round.
>
> >> + smmu->smrs[i].valid = true;
> >> + }
> >> + smmu->s2crs[i].count++;
> >> + smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> >> + smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> >> + smmu->s2crs[i].cbndx = 0xff;
> >> +
> >> + cnt++;
> >> + }
> >> +
> >> + if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg &
> >> ARM_SMMU_sCR0_CLIENTPD)) {
> >> + /* Remove the valid bit for unused SMRs */
> >> + for (i = 0; i < smmu->num_mapping_groups; i++) {
> >> + if (smmu->s2crs[i].count == 0)
> >> + smmu->smrs[i].valid = false;
> >> + }
> >> + }
> >> +
> >> + dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
> >> + cnt == 1 ? "" : "s");
> >> + iommu_dma_put_rmrs(dev_fwnode(smmu->dev), _list);
> >> +}
> >> +
> >> static int arm_smmu_device_probe(struct platform_device *pdev)
> >> {
> >> str
Re: [PATCH v5 7/8] iommu/arm-smmu: Get associated RMR info and install bypass SMR
On Mon, May 24, 2021 at 1:04 PM Shameer Kolothum
wrote:
>
> From: Jon Nettleton
>
> Check if there is any RMR info associated with the devices behind
> the SMMU and if any, install bypass SMRs for them. This is to
> keep any ongoing traffic associated with these devices alive
> when we enable/reset SMMU during probe().
>
> Signed-off-by: Jon Nettleton
> Signed-off-by: Steven Price
> Signed-off-by: Shameer Kolothum
> ---
> drivers/iommu/arm/arm-smmu/arm-smmu.c | 65 +++
> 1 file changed, 65 insertions(+)
>
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 6f72c4d208ca..56db3d3238fc 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -2042,6 +2042,67 @@ err_reset_platform_ops: __maybe_unused;
> return err;
> }
>
> +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
> +{
> + struct list_head rmr_list;
> + struct iommu_resv_region *e;
> + int i, cnt = 0;
> + u32 smr;
> + u32 reg;
> +
> + INIT_LIST_HEAD(_list);
> + if (iommu_dma_get_rmrs(dev_fwnode(smmu->dev), _list))
> + return;
> +
> + reg = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_sCR0);
> +
> + if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> + /*
> +* SMMU is already enabled and disallowing bypass, so preserve
> +* the existing SMRs
> +*/
> + for (i = 0; i < smmu->num_mapping_groups; i++) {
> + smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
> + if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> + continue;
> + smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> + smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK,
> smr);
> + smmu->smrs[i].valid = true;
> + }
> + }
> +
> + list_for_each_entry(e, _list, list) {
> + u32 sid = e->fw_data.rmr.sid;
> +
> + i = arm_smmu_find_sme(smmu, sid, ~0);
> + if (i < 0)
> + continue;
> + if (smmu->s2crs[i].count == 0) {
> + smmu->smrs[i].id = sid;
> + smmu->smrs[i].mask = ~0;
> + smmu->smrs[i].valid = true;
> + }
> + smmu->s2crs[i].count++;
> + smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> + smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> + smmu->s2crs[i].cbndx = 0xff;
> +
> + cnt++;
> + }
> +
> + if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> + /* Remove the valid bit for unused SMRs */
> + for (i = 0; i < smmu->num_mapping_groups; i++) {
> + if (smmu->s2crs[i].count == 0)
> + smmu->smrs[i].valid = false;
> + }
> + }
> +
> + dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
> + cnt == 1 ? "" : "s");
> + iommu_dma_put_rmrs(dev_fwnode(smmu->dev), _list);
> +}
> +
> static int arm_smmu_device_probe(struct platform_device *pdev)
> {
> struct resource *res;
> @@ -2168,6 +2229,10 @@ static int arm_smmu_device_probe(struct
> platform_device *pdev)
> }
>
> platform_set_drvdata(pdev, smmu);
> +
> + /* Check for RMRs and install bypass SMRs if any */
> + arm_smmu_rmr_install_bypass_smr(smmu);
> +
> arm_smmu_device_reset(smmu);
> arm_smmu_test_smr_masks(smmu);
>
> --
> 2.17.1
>
Shameer and Robin
I finally got around to updating edk2 and the HoneyComb IORT tables to
reflect the new standards.
Out of the box the new patchset was generating errors immediatly after
the smmu bringup.
arm-smmu arm-smmu.0.auto: Unhandled context fault: fsr=0x402, iova=0x208140,
fsynr=0x1d0040, cbfrsynra=0x4000, cb=0
These errors were generated even with disable_bypass=0
I tracked down the issue to
This code is skipped as Robin said would be correct
> + if ((reg & ARM_SMMU_sCR0_USFCFG) && !(reg & ARM_SMMU_sCR0_CLIENTPD)) {
> + /*
> +* SMMU is already enabled and disallowing bypass, so preserve
> +* the existing SMRs
> +*/
> + for (i = 0; i < smmu->num_map
Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory regions
On Wed, May 26, 2021 at 6:36 PM Shameerali Kolothum Thodi
wrote:
>
>
>
> > -Original Message-
> > From: Laurentiu Tudor [mailto:laurentiu.tu...@nxp.com]
> > Sent: 26 May 2021 08:53
> > To: Shameerali Kolothum Thodi ;
> > linux-arm-ker...@lists.infradead.org; linux-a...@vger.kernel.org;
> > iommu@lists.linux-foundation.org
> > Cc: j...@solid-run.com; Linuxarm ;
> > steven.pr...@arm.com; Guohanjun (Hanjun Guo) ;
> > yangyicong ; sami.muja...@arm.com;
> > robin.mur...@arm.com; wanghuiqiang
> > Subject: Re: [PATCH v5 3/8] ACPI/IORT: Add a helper to retrieve RMR memory
> > regions
> >
> > Hi Shameer,
> >
> > On 5/24/2021 2:02 PM, Shameer Kolothum wrote:
> > > Add a helper function that retrieves RMR memory descriptors
> > > associated with a given IOMMU. This will be used by IOMMU
> > > drivers to setup necessary mappings.
> > >
> > > Now that we have this, invoke it from the generic helper
> > > interface.
> > >
> > > Signed-off-by: Shameer Kolothum
> >
> > > ---
> > > drivers/acpi/arm64/iort.c | 50
> > +++
> > > drivers/iommu/dma-iommu.c | 4
> > > include/linux/acpi_iort.h | 7 ++
> > > 3 files changed, 61 insertions(+)
> > >
> > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > index fea1ffaedf3b..01917caf58de 100644
> > > --- a/drivers/acpi/arm64/iort.c
> > > +++ b/drivers/acpi/arm64/iort.c
> > > @@ -12,6 +12,7 @@
> > >
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -837,6 +838,53 @@ static inline int iort_add_device_replay(struct
> > device *dev)
> > > return err;
> > > }
> > >
> > > +/**
> > > + * iort_iommu_get_rmrs - Helper to retrieve RMR info associated with
> > IOMMU
> > > + * @iommu: fwnode for the IOMMU
> > > + * @head: RMR list head to be populated
> > > + *
> > > + * Returns: 0 on success, <0 failure
> > > + */
> > > +int iort_iommu_get_rmrs(struct fwnode_handle *iommu_fwnode,
> > > + struct list_head *head)
> > > +{
> > > + struct iort_rmr_entry *e;
> > > + struct acpi_iort_node *iommu;
> > > + int rmrs = 0;
> > > +
> > > + iommu = iort_get_iort_node(iommu_fwnode);
> > > + if (!iommu || list_empty(_rmr_list))
> > > + return -ENODEV;
> > > +
> > > + list_for_each_entry(e, _rmr_list, list) {
> > > + int prot = IOMMU_READ | IOMMU_WRITE | IOMMU_NOEXEC |
> > IOMMU_MMIO;
> >
> > We have a case with an IP block that needs EXEC rights on its reserved
> > memory, so could you please drop the IOMMU_NOEXEC flag?
>
> Ok, I think I can drop that one if there are no other concerns. I was not
> quite
> sure what to include here in the first place as the IORT spec is not giving
> any
> further details about the RMR regions(May be the flags field can be extended
> to
> describe these details).
We probably need to bring this back up to the ACPI forums. This is probably
something that should be attached to the memory range node which does have
4 extra reserved bytes.
-Jon
>
> Thanks,
> Shameer
>
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 09/10] iommu/arm-smmu: Get associated RMR info and install bypass SMR
On Mon, May 10, 2021 at 10:40 AM Shameerali Kolothum Thodi
wrote:
>
>
>
> > -Original Message-
> > From: Steven Price [mailto:steven.pr...@arm.com]
> > Sent: 06 May 2021 16:17
> > 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; wanghuiqiang
> > ; Guohanjun (Hanjun Guo)
> > ; sami.muja...@arm.com; j...@solid-run.com;
> > eric.au...@redhat.com
> > Subject: Re: [PATCH v3 09/10] iommu/arm-smmu: Get associated RMR info
> > and install bypass SMR
> >
> > On 20/04/2021 09:27, Shameer Kolothum wrote:
> > > From: Jon Nettleton
> > >
> > > Check if there is any RMR info associated with the devices behind
> > > the SMMU and if any, install bypass SMRs for them. This is to
> > > keep any ongoing traffic associated with these devices alive
> > > when we enable/reset SMMU during probe().
> > >
> > > Signed-off-by: Jon Nettleton
> > > Signed-off-by: Shameer Kolothum
> >
> > > ---
> > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 42
> > +++
> > > drivers/iommu/arm/arm-smmu/arm-smmu.h | 2 ++
> > > 2 files changed, 44 insertions(+)
> > >
> > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > index d8c6bfde6a61..4d2f91626d87 100644
> > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > @@ -2102,6 +2102,43 @@ err_reset_platform_ops: __maybe_unused;
> > > return err;
> > > }
> > >
> > > +static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device
> > *smmu)
> > > +{
> > > + struct iommu_rmr *e;
> > > + int i, cnt = 0;
> > > + u32 smr;
> > > +
> > > + for (i = 0; i < smmu->num_mapping_groups; i++) {
> > > + smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
> > > + if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
> > > + continue;
> > > +
> > > + list_for_each_entry(e, >rmr_list, list) {
> > > + if (FIELD_GET(ARM_SMMU_SMR_ID, smr) != e->sid)
> > > + continue;
> > > +
> > > + smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> > > + smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK,
> > smr);
> > > + smmu->smrs[i].valid = true;
> > > +
> > > + smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> > > + smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> > > + smmu->s2crs[i].cbndx = 0xff;
> > > +
> > > + cnt++;
> > > + }
> > > + }
> >
> > If I understand this correctly - this is looking at the current
> > (hardware) configuration of the SMMU and attempting to preserve any
> > bypass SMRs. However from what I can tell it suffers from the following
> > two problems:
> >
> > (a) Only the ID of the SMR is being checked, not the MASK. So if the
> > firmware has setup an SMR matching a number of streams this will break.
> >
> > (b) The SMMU might not be enabled at all (CLIENTPD==1) or bypass
> > enabled for unmatched streams (USFCFG==0).
> >
> > Certainly in my test setup case (b) applies and so this doesn't work.
> > Perhaps something like the below would work better? (It works in the
> > case of the SMMU not enabled - I've not tested case (a)).
>
> Thanks Steve for taking a look and testing this on SMMUv2. My knowledge
> on SMMUv2 is limited an don't have a setup to verify this. After reading
> the code, agree that the current implementation addresses the hardware
> configuration only and breaks all the scenarios explained above.
>
> I will include the below snippet in next respin if that works.
>
> Hi Jon,
>
> Could you please take a look and see the below changes works for
> you too?
My original code was derived from a solution that was proposed for
device-tree booted systems. I will review and test the changes and
then report back.
-Jon
>
> Thanks,
> Shameer
>
> > 8<
> > static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device
> > *smmu)
> > {
> > struct iommu_rmr *e;
> > int i, cnt = 0;
>
Re: [RFC PATCH v2 1/8] ACPICA: IORT: Update for revision E
On Mon, Mar 22, 2021 at 11:37 AM Shameerali Kolothum Thodi
wrote:
>
> [+]
>
> Hi Erik,
>
> As this is now just merged ino acpica-master and based on the discussion we
> had here,
>
> https://github.com/acpica/acpica/pull/638
>
> I had a discussion with ARM folks(Lorenzo) in the linaro-open-discussions
> call and
> can confirm that the IORT Revision E is not the final specification and has
> some issues
> which is now corrected in the latest E.b revision[1]. Also there are no
> current users
> for the Rev E and it may not be a good idea to push this version into the
> Linux kernel
> or elsewhere.
Well it was a released revision, although it was found to have issues.
Currently
HoneyComb Systems Ready certified firmware does include support for this table,
although incomplete. Without agreement on mainline support I am fine to update
to the latest spec bump.
>
> So could you please revert the merge and I am planning to work on the E.b
> soon.
> Please let me know if I need to explicitly send a revert pull request or not.
Can you please CC. me on your next release. I was planning on spending time
on this regardless. I already have a patchset for the fsl-mc-bus driver that
needs to change in order to function properly with RMR support.
Thanks.
>
> Thanks,
> Shameer
>
> 1. https://developer.arm.com/documentation/den0049/latest/
>
> > -Original Message-
> > From: iommu [mailto:iommu-boun...@lists.linux-foundation.org] On Behalf Of
> > Shameer Kolothum
> > Sent: 19 November 2020 12:12
> > To: linux-arm-ker...@lists.infradead.org; linux-a...@vger.kernel.org;
> > iommu@lists.linux-foundation.org; de...@acpica.org
> > Cc: Linuxarm ; steven.pr...@arm.com; Guohanjun
> > (Hanjun Guo) ; sami.muja...@arm.com;
> > robin.mur...@arm.com; wanghuiqiang
> > Subject: [RFC PATCH v2 1/8] ACPICA: IORT: Update for revision E
> >
> > IORT revision E contains a few additions like,
> > -Added an identifier field in the node descriptors to aid table
> > cross-referencing.
> > -Introduced the Reserved Memory Range(RMR) node. This is used
> > to describe memory ranges that are used by endpoints and requires
> > a unity mapping in SMMU.
> > -Introduced a flag in the RC node to express support for PRI.
> >
> > Signed-off-by: Shameer Kolothum
> > ---
> > include/acpi/actbl2.h | 25 +++--
> > 1 file changed, 19 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h index
> > ec66779cb193..274fce7b5c01 100644
> > --- a/include/acpi/actbl2.h
> > +++ b/include/acpi/actbl2.h
> > @@ -68,7 +68,7 @@
> > * IORT - IO Remapping Table
> > *
> > * Conforms to "IO Remapping Table System Software on ARM Platforms",
> > - * Document number: ARM DEN 0049D, March 2018
> > + * Document number: ARM DEN 0049E, June 2020
> > *
> >
> >
> > **/
> >
> > @@ -86,7 +86,8 @@ struct acpi_iort_node {
> > u8 type;
> > u16 length;
> > u8 revision;
> > - u32 reserved;
> > + u16 reserved;
> > + u16 identifier;
> > u32 mapping_count;
> > u32 mapping_offset;
> > char node_data[1];
> > @@ -100,7 +101,8 @@ enum acpi_iort_node_type {
> > ACPI_IORT_NODE_PCI_ROOT_COMPLEX = 0x02,
> > ACPI_IORT_NODE_SMMU = 0x03,
> > ACPI_IORT_NODE_SMMU_V3 = 0x04,
> > - ACPI_IORT_NODE_PMCG = 0x05
> > + ACPI_IORT_NODE_PMCG = 0x05,
> > + ACPI_IORT_NODE_RMR = 0x06,
> > };
> >
> > struct acpi_iort_id_mapping {
> > @@ -167,10 +169,10 @@ struct acpi_iort_root_complex {
> > u8 reserved[3]; /* Reserved, must be zero */
> > };
> >
> > -/* Values for ats_attribute field above */
> > +/* Masks for ats_attribute field above */
> >
> > -#define ACPI_IORT_ATS_SUPPORTED 0x0001 /* The root
> > complex supports ATS */
> > -#define ACPI_IORT_ATS_UNSUPPORTED 0x /* The root
> > complex doesn't support ATS */
> > +#define ACPI_IORT_ATS_SUPPORTED (1) /* The root complex
> > supports ATS */
> > +#define ACPI_IORT_PRI_SUPPORTED (1<<1) /* The root complex
> > supports PRI */
> >
> > struct acpi_iort_smmu {
> > u64 base_address; /* SMMU base address */
> > @@ -241,6 +243,17 @@ struct acpi_iort_pmcg {
> > u64 page1_base_address;
> > };
> >
> > +struct acpi_iort_rmr {
> > + u32 rmr_count;
> > + u32 rmr_offset;
> > +};
> > +
> > +struct acpi_iort_rmr_desc {
> > + u64 base_address;
> > + u64 length;
> > + u32 reserved;
> > +};
> > +
> >
> > /***
> >
> > *
> > * IVRS - I/O Virtualization Reporting Structure
> > --
> > 2.17.1
> >
> > ___
> > iommu mailing list
> > iommu@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/iommu
> ___
> linux-arm-kernel
Re: [RFC PATCH v2 0/8] ACPI/IORT: Support for IORT RMR node
On Mon, Jan 4, 2021 at 9:55 AM Shameerali Kolothum Thodi wrote: > > > > > -Original Message----- > > From: Jon Nettleton [mailto:j...@solid-run.com] > > Sent: 18 December 2020 10:53 > > To: Shameerali Kolothum Thodi > > Cc: Steven Price ; Robin Murphy > > ; linux-arm-ker...@lists.infradead.org; > > linux-a...@vger.kernel.org; iommu@lists.linux-foundation.org; > > de...@acpica.org; lorenzo.pieral...@arm.com; j...@8bytes.org; Guohanjun > > (Hanjun Guo) ; Linuxarm ; > > Jonathan Cameron ; > > sami.muja...@arm.com; wanghuiqiang > > Subject: Re: [RFC PATCH v2 0/8] ACPI/IORT: Support for IORT RMR node > > > > On Thu, Dec 17, 2020 at 4:53 PM Jon Nettleton wrote: > > > > > > On Thu, Dec 17, 2020 at 4:42 PM Shameerali Kolothum Thodi > > > wrote: > > > > > > > > > > > > > > > > > -Original Message- > > > > > From: Jon Nettleton [mailto:j...@solid-run.com] > > > > > Sent: 17 December 2020 14:48 > > > > > To: Shameerali Kolothum Thodi > > > > > > > Cc: Steven Price ; Robin Murphy > > > > > ; linux-arm-ker...@lists.infradead.org; > > > > > linux-a...@vger.kernel.org; iommu@lists.linux-foundation.org; > > > > > de...@acpica.org; lorenzo.pieral...@arm.com; j...@8bytes.org; > > Guohanjun > > > > > (Hanjun Guo) ; Linuxarm > > ; > > > > > Jonathan Cameron ; > > > > > sami.muja...@arm.com; wanghuiqiang > > > > > Subject: Re: [RFC PATCH v2 0/8] ACPI/IORT: Support for IORT RMR node > > > > > > > > > > On Mon, Dec 14, 2020 at 3:48 PM Shameerali Kolothum Thodi > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > -Original Message- > > > > > > > From: Steven Price [mailto:steven.pr...@arm.com] > > > > > > > Sent: 14 December 2020 13:43 > > > > > > > To: Robin Murphy ; Shameerali Kolothum > > Thodi > > > > > > > ; > > > > > > > linux-arm-ker...@lists.infradead.org; linux-a...@vger.kernel.org; > > > > > > > iommu@lists.linux-foundation.org; de...@acpica.org > > > > > > > Cc: Linuxarm ; lorenzo.pieral...@arm.com; > > > > > > > j...@8bytes.org; wanghuiqiang ; > > Guohanjun > > > > > > > (Hanjun Guo) ; Jonathan Cameron > > > > > > > ; sami.muja...@arm.com > > > > > > > Subject: Re: [RFC PATCH v2 0/8] ACPI/IORT: Support for IORT RMR > > node > > > > > > > > > > > > > > On 14/12/2020 12:33, Robin Murphy wrote: > > > > > > > > On 2020-12-14 10:55, Shameerali Kolothum Thodi wrote: > > > > > > > >> Hi Steve, > > > > > > > >> > > > > > > > >>> -Original Message- > > > > > > > >>> From: Steven Price [mailto:steven.pr...@arm.com] > > > > > > > >>> Sent: 10 December 2020 10:26 > > > > > > > >>> To: Shameerali Kolothum Thodi > > > > > > > ; > > > > > > > >>> linux-arm-ker...@lists.infradead.org; > > > > > > > >>> linux-a...@vger.kernel.org; > > > > > > > >>> iommu@lists.linux-foundation.org; de...@acpica.org > > > > > > > >>> Cc: Linuxarm ; > > lorenzo.pieral...@arm.com; > > > > > > > >>> j...@8bytes.org; robin.mur...@arm.com; wanghuiqiang > > > > > > > >>> ; Guohanjun (Hanjun Guo) > > > > > > > >>> ; Jonathan Cameron > > > > > > > >>> ; sami.muja...@arm.com > > > > > > > >>> Subject: Re: [RFC PATCH v2 0/8] ACPI/IORT: Support for IORT > > > > > > > >>> RMR > > node > > > > > > > >>> > > > > > > > >>> On 19/11/2020 12:11, Shameer Kolothum wrote: > > > > > > > >>>> RFC v1 --> v2: > > > > > > > >>>>- Added a generic interface for IOMMU drivers to retrieve > > > > > > > >>>> all > > the > > > > > > > >>>> RMR info associated with a given IOMMU. > > > > > > > >>>>- SMMUv3 driver
Re: [RFC PATCH v2 0/8] ACPI/IORT: Support for IORT RMR node
On Thu, Dec 17, 2020 at 4:53 PM Jon Nettleton wrote: > > On Thu, Dec 17, 2020 at 4:42 PM Shameerali Kolothum Thodi > wrote: > > > > > > > > > -----Original Message- > > > From: Jon Nettleton [mailto:j...@solid-run.com] > > > Sent: 17 December 2020 14:48 > > > To: Shameerali Kolothum Thodi > > > Cc: Steven Price ; Robin Murphy > > > ; linux-arm-ker...@lists.infradead.org; > > > linux-a...@vger.kernel.org; iommu@lists.linux-foundation.org; > > > de...@acpica.org; lorenzo.pieral...@arm.com; j...@8bytes.org; Guohanjun > > > (Hanjun Guo) ; Linuxarm ; > > > Jonathan Cameron ; > > > sami.muja...@arm.com; wanghuiqiang > > > Subject: Re: [RFC PATCH v2 0/8] ACPI/IORT: Support for IORT RMR node > > > > > > On Mon, Dec 14, 2020 at 3:48 PM Shameerali Kolothum Thodi > > > wrote: > > > > > > > > > > > > > > > > > -Original Message- > > > > > From: Steven Price [mailto:steven.pr...@arm.com] > > > > > Sent: 14 December 2020 13:43 > > > > > To: Robin Murphy ; Shameerali Kolothum Thodi > > > > > ; > > > > > linux-arm-ker...@lists.infradead.org; linux-a...@vger.kernel.org; > > > > > iommu@lists.linux-foundation.org; de...@acpica.org > > > > > Cc: Linuxarm ; lorenzo.pieral...@arm.com; > > > > > j...@8bytes.org; wanghuiqiang ; Guohanjun > > > > > (Hanjun Guo) ; Jonathan Cameron > > > > > ; sami.muja...@arm.com > > > > > Subject: Re: [RFC PATCH v2 0/8] ACPI/IORT: Support for IORT RMR node > > > > > > > > > > On 14/12/2020 12:33, Robin Murphy wrote: > > > > > > On 2020-12-14 10:55, Shameerali Kolothum Thodi wrote: > > > > > >> Hi Steve, > > > > > >> > > > > > >>> -Original Message- > > > > > >>> From: Steven Price [mailto:steven.pr...@arm.com] > > > > > >>> Sent: 10 December 2020 10:26 > > > > > >>> To: Shameerali Kolothum Thodi > > > > > ; > > > > > >>> linux-arm-ker...@lists.infradead.org; linux-a...@vger.kernel.org; > > > > > >>> iommu@lists.linux-foundation.org; de...@acpica.org > > > > > >>> Cc: Linuxarm ; lorenzo.pieral...@arm.com; > > > > > >>> j...@8bytes.org; robin.mur...@arm.com; wanghuiqiang > > > > > >>> ; Guohanjun (Hanjun Guo) > > > > > >>> ; Jonathan Cameron > > > > > >>> ; sami.muja...@arm.com > > > > > >>> Subject: Re: [RFC PATCH v2 0/8] ACPI/IORT: Support for IORT RMR > > > > > >>> node > > > > > >>> > > > > > >>> On 19/11/2020 12:11, Shameer Kolothum wrote: > > > > > >>>> RFC v1 --> v2: > > > > > >>>>- Added a generic interface for IOMMU drivers to retrieve all > > > > > >>>> the > > > > > >>>> RMR info associated with a given IOMMU. > > > > > >>>>- SMMUv3 driver gets the RMR list during probe() and installs > > > > > >>>> bypass STEs for all the SIDs in the RMR list. This is to > > > > > >>>> keep > > > > > >>>> the ongoing traffic alive(if any) during SMMUv3 reset. This > > > > > >>>> is > > > > > >>>> based on the suggestions received for v1 to take care of the > > > > > >>>> EFI framebuffer use case. Only sanity tested for now. > > > > > >>> > > > > > >>> Hi Shameer, > > > > > >>> > > > > > >>> Sorry for not looking at this before. > > > > > >>> > > > > > >>> Do you have any plans to implement support in the SMMUv2 driver? > > > The > > > > > >>> platform I've been testing the EFI framebuffer support on has the > > > > > >>> display controller behind SMMUv2, so as it stands this series > > > > > >>> doesn't > > > > > >>> work. I did hack something up for SMMUv2 so I was able to test the > > > first > > > > > >>> 4 patches. > > > > > >> > > > > > >> Thanks for taking a look. Sure, I can look into addi
Re: [RFC PATCH v2 0/8] ACPI/IORT: Support for IORT RMR node
On Thu, Dec 17, 2020 at 4:42 PM Shameerali Kolothum Thodi wrote: > > > > > -Original Message----- > > From: Jon Nettleton [mailto:j...@solid-run.com] > > Sent: 17 December 2020 14:48 > > To: Shameerali Kolothum Thodi > > Cc: Steven Price ; Robin Murphy > > ; linux-arm-ker...@lists.infradead.org; > > linux-a...@vger.kernel.org; iommu@lists.linux-foundation.org; > > de...@acpica.org; lorenzo.pieral...@arm.com; j...@8bytes.org; Guohanjun > > (Hanjun Guo) ; Linuxarm ; > > Jonathan Cameron ; > > sami.muja...@arm.com; wanghuiqiang > > Subject: Re: [RFC PATCH v2 0/8] ACPI/IORT: Support for IORT RMR node > > > > On Mon, Dec 14, 2020 at 3:48 PM Shameerali Kolothum Thodi > > wrote: > > > > > > > > > > > > > -Original Message- > > > > From: Steven Price [mailto:steven.pr...@arm.com] > > > > Sent: 14 December 2020 13:43 > > > > To: Robin Murphy ; Shameerali Kolothum Thodi > > > > ; > > > > linux-arm-ker...@lists.infradead.org; linux-a...@vger.kernel.org; > > > > iommu@lists.linux-foundation.org; de...@acpica.org > > > > Cc: Linuxarm ; lorenzo.pieral...@arm.com; > > > > j...@8bytes.org; wanghuiqiang ; Guohanjun > > > > (Hanjun Guo) ; Jonathan Cameron > > > > ; sami.muja...@arm.com > > > > Subject: Re: [RFC PATCH v2 0/8] ACPI/IORT: Support for IORT RMR node > > > > > > > > On 14/12/2020 12:33, Robin Murphy wrote: > > > > > On 2020-12-14 10:55, Shameerali Kolothum Thodi wrote: > > > > >> Hi Steve, > > > > >> > > > > >>> -Original Message- > > > > >>> From: Steven Price [mailto:steven.pr...@arm.com] > > > > >>> Sent: 10 December 2020 10:26 > > > > >>> To: Shameerali Kolothum Thodi > > > > ; > > > > >>> linux-arm-ker...@lists.infradead.org; linux-a...@vger.kernel.org; > > > > >>> iommu@lists.linux-foundation.org; de...@acpica.org > > > > >>> Cc: Linuxarm ; lorenzo.pieral...@arm.com; > > > > >>> j...@8bytes.org; robin.mur...@arm.com; wanghuiqiang > > > > >>> ; Guohanjun (Hanjun Guo) > > > > >>> ; Jonathan Cameron > > > > >>> ; sami.muja...@arm.com > > > > >>> Subject: Re: [RFC PATCH v2 0/8] ACPI/IORT: Support for IORT RMR node > > > > >>> > > > > >>> On 19/11/2020 12:11, Shameer Kolothum wrote: > > > > >>>> RFC v1 --> v2: > > > > >>>>- Added a generic interface for IOMMU drivers to retrieve all > > > > >>>> the > > > > >>>> RMR info associated with a given IOMMU. > > > > >>>>- SMMUv3 driver gets the RMR list during probe() and installs > > > > >>>> bypass STEs for all the SIDs in the RMR list. This is to keep > > > > >>>> the ongoing traffic alive(if any) during SMMUv3 reset. This is > > > > >>>> based on the suggestions received for v1 to take care of the > > > > >>>> EFI framebuffer use case. Only sanity tested for now. > > > > >>> > > > > >>> Hi Shameer, > > > > >>> > > > > >>> Sorry for not looking at this before. > > > > >>> > > > > >>> Do you have any plans to implement support in the SMMUv2 driver? > > The > > > > >>> platform I've been testing the EFI framebuffer support on has the > > > > >>> display controller behind SMMUv2, so as it stands this series > > > > >>> doesn't > > > > >>> work. I did hack something up for SMMUv2 so I was able to test the > > first > > > > >>> 4 patches. > > > > >> > > > > >> Thanks for taking a look. Sure, I can look into adding the support > > > > >> for > > > > >> SMMUv2. > > > > > > > > Great, thanks! > > > > > > > > >>> > > > > >>>>- During the probe/attach device, SMMUv3 driver reserves any > > > > >>>> RMR region associated with the device such that there is a > > unity > > > > >>>> mapping for them in SMMU. > > > > >>> > > > > >>> For the EFI framebuffer use case ther
Re: [RFC PATCH v2 0/8] ACPI/IORT: Support for IORT RMR node
On Mon, Dec 14, 2020 at 3:48 PM Shameerali Kolothum Thodi wrote: > > > > > -Original Message- > > From: Steven Price [mailto:steven.pr...@arm.com] > > Sent: 14 December 2020 13:43 > > To: Robin Murphy ; Shameerali Kolothum Thodi > > ; > > linux-arm-ker...@lists.infradead.org; linux-a...@vger.kernel.org; > > iommu@lists.linux-foundation.org; de...@acpica.org > > Cc: Linuxarm ; lorenzo.pieral...@arm.com; > > j...@8bytes.org; wanghuiqiang ; Guohanjun > > (Hanjun Guo) ; Jonathan Cameron > > ; sami.muja...@arm.com > > Subject: Re: [RFC PATCH v2 0/8] ACPI/IORT: Support for IORT RMR node > > > > On 14/12/2020 12:33, Robin Murphy wrote: > > > On 2020-12-14 10:55, Shameerali Kolothum Thodi wrote: > > >> Hi Steve, > > >> > > >>> -Original Message- > > >>> From: Steven Price [mailto:steven.pr...@arm.com] > > >>> Sent: 10 December 2020 10:26 > > >>> To: Shameerali Kolothum Thodi > > ; > > >>> linux-arm-ker...@lists.infradead.org; linux-a...@vger.kernel.org; > > >>> iommu@lists.linux-foundation.org; de...@acpica.org > > >>> Cc: Linuxarm ; lorenzo.pieral...@arm.com; > > >>> j...@8bytes.org; robin.mur...@arm.com; wanghuiqiang > > >>> ; Guohanjun (Hanjun Guo) > > >>> ; Jonathan Cameron > > >>> ; sami.muja...@arm.com > > >>> Subject: Re: [RFC PATCH v2 0/8] ACPI/IORT: Support for IORT RMR node > > >>> > > >>> On 19/11/2020 12:11, Shameer Kolothum wrote: > > RFC v1 --> v2: > > - Added a generic interface for IOMMU drivers to retrieve all the > > RMR info associated with a given IOMMU. > > - SMMUv3 driver gets the RMR list during probe() and installs > > bypass STEs for all the SIDs in the RMR list. This is to keep > > the ongoing traffic alive(if any) during SMMUv3 reset. This is > > based on the suggestions received for v1 to take care of the > > EFI framebuffer use case. Only sanity tested for now. > > >>> > > >>> Hi Shameer, > > >>> > > >>> Sorry for not looking at this before. > > >>> > > >>> Do you have any plans to implement support in the SMMUv2 driver? The > > >>> platform I've been testing the EFI framebuffer support on has the > > >>> display controller behind SMMUv2, so as it stands this series doesn't > > >>> work. I did hack something up for SMMUv2 so I was able to test the first > > >>> 4 patches. > > >> > > >> Thanks for taking a look. Sure, I can look into adding the support for > > >> SMMUv2. > > > > Great, thanks! > > > > >>> > > - During the probe/attach device, SMMUv3 driver reserves any > > RMR region associated with the device such that there is a unity > > mapping for them in SMMU. > > >>> > > >>> For the EFI framebuffer use case there is no device to attach so I > > >>> believe we are left with just the stream ID in bypass mode - which is > > >>> definitely an improvement (the display works!) > > >> > > >> Cool. That’s good to know. > > >> > > >> but not actually a unity > > >>> mapping of the RMR range. I'm not sure whether it's worth fixing this or > > >>> not, but I just wanted to point out there's still a need for a driver > > >>> for the device before the bypass mode is replaced with the unity > > >>> mapping. > > >> > > >> I am not sure either. My idea was we will have bypass STE setup for > > >> all devices > > >> with RMR initially and when the corresponding driver takes over(if > > >> that happens) > > >> we will have the unity mapping setup properly for the RMR regions. And > > >> for cases > > >> like the above, it will remain in the bypass mode. > > >> > > >> Do you see any problem(security?) if the dev streams remain in bypass > > >> mode for > > >> this dev? Or is it possible to have a stub driver for this dev, so > > >> that we will have > > >> the probe/attach invoked and everything will fall in place? > > > > > > The downside is that if a driver never binds to that device, it remains > > > bypassed. If some other externally-controlled malicious device could > > > manage to spoof that device's requester ID, that could potentially be > > > exploited. > > > > > >> TBH, I haven't looked into creating a temp domain for these types of > > >> the devices > > >> and also not sure how we benefit from that compared to the STE bypass > > >> mode. > > > > > > That said, setting up temporary translation contexts that ensure any > > > access can *only* be to RMR regions until a driver takes over is an > > > awful lot more work. I'm inclined to be pragmatic here and say we should > > > get things working at all with the simple bypass STE/S2CR method, then > > > look at adding the higher-security effort on top. > > > > > > Right now systems that need this are either broken (but effectively > > > secure) or using default bypass with SMMUv2. People who prefer to > > > maintain security over functionality in the interim can maintain that > > > status quo by simply continuing to not describe any RMRs. > > > > I agree with Robin, let's get this working with the bypass mode (until > > the
