Re: [PATCH v5 2/6] iommu/qcom: Use the asid read from device-tree if specified

[AngeloGioacchino Del Regno]

Il 01/08/23 15:49, Will Deacon ha scritto:
> On Thu, Jun 22, 2023 at 11:27:38AM +0200, AngeloGioacchino Del Regno wrote:
> > As specified in this driver, the context banks are 0x1000 apart but
> > on some SoCs the context number does not necessarily match this
> > logic, hence we end up using the wrong ASID: keeping in mind that
> > this IOMMU implementation relies heavily on SCM (TZ) calls, it is
> > mandatory that we communicate the right context number.
> >
> > Since this is all about how context banks are mapped in firmware,
> > which may be board dependent (as a different firmware version may
> > eventually change the expected context bank numbers), introduce a
> > new property "qcom,ctx-asid": when found, the ASID will be forced
> > as read from the devicetree.
> >
> > When "qcom,ctx-asid" is not found, this driver retains the previous
> > behavior as to avoid breaking older devicetrees or systems that do
> > not require forcing ASID numbers.
> >
> > Signed-off-by: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>
> > [Marijn: Rebased over next-20221111]
> > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > ---
> >   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 18 +++++++++++++++---
> >   1 file changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> > index a503ed758ec3..8face57c4180 100644
> > --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> > +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> > @@ -531,7 +531,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)) {
> >   		put_device(&iommu_pdev->dev);
> >   		return -EINVAL;
> >   	}
> > @@ -617,7 +618,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:
> > @@ -625,7 +627,17 @@ 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-asid", &val))
> > +		asid = val;
> > +	else
> > +		asid = reg / 0x1000;
> > +
> > +	return asid;
>
> Shouldn't we at least have some error checking here? For example, ensuring
> that the ASIDs are within range, aren't duplicates etc?

The only check that we can perform here for ASID-in-range is

if ((asid * 0x1000 > (mmio_start + mmio_size - 0x1000))
	return -EINVAL;

...as for duplicates, a check can *probably* (surely) be done... but I'm not
sure I have any more time to feed more code to this series from years ago...

> Also, can you elaborate a little more on what sort of ASID-to-Context
> mappings you actually see in practice?

I'm sorry, but not really. The first version of this (including the whole research
that I had to perform to write those patches) is from year 2019, so 4 years ago...

...I don't really remember the full details anymore - if not that all of this was
done because context banks are fixed (and setup by TZ), tz takes an asid number
when trying to perform any operation on the context bank, and there's no way to
reset mappings because everything is protected by the hypervisor (which will fault
and reboot the AP instantly if you try).

Cheers,
Angelo