IOMMU Archive on lore.kernel.org
 help / color / Atom feed
From: AngeloGioacchino Del Regno <kholk11@gmail.com>
To: Robin Murphy <robin.murphy@arm.com>
Cc: MSM <linux-arm-msm@vger.kernel.org>,
	marijns95@gmail.com, Andy Gross <agross@kernel.org>,
	iommu@lists.linux-foundation.org
Subject: Re: [PATCH v4 2/7] iommu/qcom: Use the asid read from device-tree if specified
Date: Tue, 15 Oct 2019 15:06:36 +0200
Message-ID: <CAK7fi1biOEtjJFqiiPRPe5LxfzNEeQ2WUVNh5trmAj_xQ23bJA@mail.gmail.com> (raw)
In-Reply-To: <25f3e314-3fcc-fc4f-e7b5-4f28418ea02f@arm.com>

Il giorno mar 15 ott 2019 alle ore 14:10 Robin Murphy
<robin.murphy@arm.com> ha scritto:
>
> On 01/10/2019 23:02, kholk11@gmail.com wrote:
> > From: AngeloGioacchino Del Regno <kholk11@gmail.com>
> >
> > As specified in this driver, the context banks are 0x1000 apart.
> > Problem is that sometimes the context number (our asid) does not
> > match this logic and we end up using the wrong one: this starts
> > being a problem in the case that we need to send TZ commands
> > to do anything on a specific context.
> >
> > For this reason, read the ASID from the DT if the property
> > "qcom,ctx-num" is present on the IOMMU context node.
>
> Can you clarify exactly what the situation is here? Is the hypervisor
> broken in that it presents context banks at made-up offsets and fails to
> correctly associate those offsets with the real underlying hardware
> context in some cases? Or is the hypervisor actually intercepting
> TTBR/CONTEXTIDR/TLBI* writes such that we don't actually have control
> over the ASID used by a given context bank (which I would also consider
> broken)? Or is "context number" a specific thing in the SCM interface
> which represents neither of the above?
>
> At the moment, it seems like a bit too much is being conflated here just
> because of some implicit behaviour in the driver.
>
> Robin.
>

From what I've understood, it's a mix of everything you just said!

This IOMMU implementation is considered "secure IOMMU" from the
Qualcomm's point of view (at least that's what they wrote in their original
downstream driver), having - secured and unsecured - pre-set (by one of the
bootloader stages) contexts.

On some SoCs (or some firmwares, if you prefer) the context number does
not correspond to that "ctx_no = addr / 0x1000" calculation for some reason
which I'm 99.9% sure is that the bootloader just configures them like this...
so, let's make a true and practical example of what happens on MSM8956/76
(and not only on this SoC):

- GFX3D IOMMU
Offset 0 -- CTX number 0
Offset 0x1000 -- CTX number 2
Offset 0x2000 -- CTX number 1

- APPS IOMMU
Offset 0x1000 -- CTX number 1
Offset blahblah -- CTX number sequential until it starts doing...
Offset 0x10000 -- CTX number 14 (!!!)
Offset blahblah -- CTX number sequential again until the end

See how the context numbers are jumping around? That's only part of
the story why we want this kind of handling: of course, with the hypervisor
intercepting writes, we would be doing ops on the wrong context and we'd
see (personal experience, tested that) for example image corruption while
trying to use adreno, or worse "random" reboots because of the hypervisor
faulting on us because it doesn't like us writing on secured context regs like
FSR or trying to reset it.

Another part of the story is on another commit of this patch series, which is
the one that's introducing the switch to the AArch64 pagetable format: when
we want to do this operation, we have to send a request to TZ containing,
like the restore_sec_cfg, the sec_id and the ASID... then the hypervisor will
agree to switch our IOMMU context's pt format.

So, at the end of the day:
1. Hypervisor seems to be broken as you seem to think
2. We don't have control on the ASID. It's preallocated and fixed.
3. The SCM interface also uses the ASID number, for which - again - we
    have no control.

I hope that was a good enough and understandable explaination of
what we have here :))

Sorry for the wall of text!
Angelo


> > Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
> > ---
> >   .../devicetree/bindings/iommu/qcom,iommu.txt    |  1 +
> >   drivers/iommu/qcom_iommu.c                      | 17 ++++++++++++++---
> >   2 files changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
> > index 059139abce35..ba0b77889f02 100644
> > --- a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
> > +++ b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
> > @@ -46,6 +46,7 @@ to non-secure vs secure interrupt line.
> >                        for routing of context bank irq's to secure vs non-
> >                        secure lines.  (Ie. if the iommu contains secure
> >                        context banks)
> > +- qcom,ctx-num     : The number associated to the context bank
> >
> >
> >   ** Examples:
> > diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
> > index c31e7bc4ccbe..504ca69adc5a 100644
> > --- a/drivers/iommu/qcom_iommu.c
> > +++ b/drivers/iommu/qcom_iommu.c
> > @@ -585,7 +585,8 @@ static int qcom_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
> >        * index into qcom_iommu->ctxs:
> >        */
> >       if (WARN_ON(asid < 1) ||
> > -         WARN_ON(asid > qcom_iommu->num_ctxs))
> > +         WARN_ON(asid > qcom_iommu->num_ctxs) ||
> > +         WARN_ON(qcom_iommu->ctxs[asid - 1] == NULL))
> >               return -EINVAL;
> >
> >       if (!fwspec->iommu_priv) {
> > @@ -693,7 +694,8 @@ static int qcom_iommu_sec_ptbl_init(struct device *dev)
> >
> >   static int get_asid(const struct device_node *np)
> >   {
> > -     u32 reg;
> > +     u32 reg, val;
> > +     int asid;
> >
> >       /* read the "reg" property directly to get the relative address
> >        * of the context bank, and calculate the asid from that:
> > @@ -701,7 +703,16 @@ static int get_asid(const struct device_node *np)
> >       if (of_property_read_u32_index(np, "reg", 0, &reg))
> >               return -ENODEV;
> >
> > -     return reg / 0x1000;      /* context banks are 0x1000 apart */
> > +     /* Context banks are 0x1000 apart but, in some cases, the ASID
> > +      * number doesn't match to this logic and needs to be passed
> > +      * from the DT configuration explicitly.
> > +      */
> > +     if (of_property_read_u32(np, "qcom,ctx-num", &val))
> > +             asid = reg / 0x1000;
> > +     else
> > +             asid = val;
> > +
> > +     return asid;
> >   }
> >
> >   static int qcom_iommu_ctx_probe(struct platform_device *pdev)
> >
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-01 22:01 [PATCH v4 0/7] Add support for QCOM IOMMU v2 and 500 kholk11
2019-10-01 22:01 ` [PATCH v4 1/7] firmware: qcom: scm: Add function to set IOMMU pagetable addressing kholk11
2019-10-15 11:14   ` Joerg Roedel
2019-10-15 12:33     ` AngeloGioacchino Del Regno
2019-10-15 12:40       ` Joerg Roedel
2019-10-15 13:09         ` AngeloGioacchino Del Regno
2019-10-01 22:02 ` [PATCH v4 2/7] iommu/qcom: Use the asid read from device-tree if specified kholk11
2019-10-15 12:09   ` Robin Murphy
2019-10-15 13:06     ` AngeloGioacchino Del Regno [this message]
2019-10-01 22:02 ` [PATCH v4 3/7] iommu/qcom: Write TCR before TTBRs to fix ASID access behavior kholk11
2019-10-01 22:02 ` [PATCH v4 4/7] iommu/qcom: Properly reset the IOMMU context kholk11
2019-10-02 11:29   ` Robin Murphy
2019-10-01 22:02 ` [PATCH v4 5/7] iommu/qcom: Add support for AArch64 IOMMU pagetables kholk11
2019-10-01 22:02 ` [PATCH v4 6/7] iommu/qcom: Index contexts by asid number to allow asid 0 kholk11
2019-10-01 22:02 ` [PATCH v4 7/7] iommu/qcom: Add support for QCIOMMUv2 and QCIOMMU-500 secured contexts kholk11
2019-10-05  4:56 ` [PATCH v4 0/7] Add support for QCOM IOMMU v2 and 500 Bjorn Andersson
2019-10-05  9:32   ` AngeloGioacchino Del Regno

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAK7fi1biOEtjJFqiiPRPe5LxfzNEeQ2WUVNh5trmAj_xQ23bJA@mail.gmail.com \
    --to=kholk11@gmail.com \
    --cc=agross@kernel.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=marijns95@gmail.com \
    --cc=robin.murphy@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

IOMMU Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iommu/0 linux-iommu/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iommu linux-iommu/ https://lore.kernel.org/linux-iommu \
		iommu@lists.linux-foundation.org
	public-inbox-index linux-iommu

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linux-foundation.lists.iommu


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git