Hi Robin,
> From: Robin Murphy, Sent: Friday, April 21, 2023 10:54 PM
> On 2023-04-21 13:25, Yoshihiro Shimoda wrote:
> > Add PCIe devices of R-Car Gen3/4 into devices_allowlist. For a PCI
> > device, ipmmu_attach_device() has to avoid enabling uTLB because
> > the uTLB has already been enabled by the parent device. Otherwise,
> > enable a wrong uTLB ID. Also ipmmu_device_is_allowed() has to
> > check whether the parent device is the PCIe host controller or not,
> > to use the IOMMU's dma_ops from a PCI device.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > ---
> > drivers/iommu/ipmmu-vmsa.c | 21 ++++++++++++++++++++-
> > 1 file changed, 20 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> > index 9f64c5c9f5b9..c635c9b192f4 100644
> > --- a/drivers/iommu/ipmmu-vmsa.c
> > +++ b/drivers/iommu/ipmmu-vmsa.c
> > @@ -19,6 +19,7 @@
> > #include <linux/of.h>
> > #include <linux/of_device.h>
> > #include <linux/of_platform.h>
> > +#include <linux/pci.h>
> > #include <linux/platform_device.h>
> > #include <linux/sizes.h>
> > #include <linux/slab.h>
> > @@ -624,6 +625,10 @@ static int ipmmu_attach_device(struct iommu_domain *io_domain,
> > if (ret < 0)
> > return ret;
> >
> > + /* Avoid to enable utlb if this is a PCI device */
> > + if (dev_is_pci(dev))
> > + return 0;
>
> Addressing that "TODO: Reference-count the microTLB..." comment instead
> would be even nicer :)
I see. However, I intended to avoid to enable utlb with unexpected ID from RID.
Also, as you mentioned about "iommus" property below, now I understood I should
not set "iommus" property to the PCIe root device. I realized that the RID is
always the same with "out id" if "iommu-map-mask" property is 0 like juno-base.dtsi.
So, I'll drop this condition on v2.
> > +
> > for (i = 0; i < fwspec->num_ids; ++i)
> > ipmmu_utlb_enable(domain, fwspec->ids[i]);
> >
> > @@ -702,10 +707,14 @@ static const struct soc_device_attribute soc_denylist[] = {
> > };
> >
> > static const char * const devices_allowlist[] = {
> > + "e65d0000.pcie", /* R-Car Gen4 */
> > + "e65d8000.pcie", /* R-Car Gen4 */
> > "ee100000.mmc",
> > "ee120000.mmc",
> > "ee140000.mmc",
> > - "ee160000.mmc"
> > + "ee160000.mmc",
> > + "ee800000.pcie", /* R-Car Gen3 */
> > + "fe000000.pcie", /* R-Car Gen3 */
>
> This would seem to imply that you have not only an "iommu-map" property
> representing the PCI devices, but also an "iommus" property representing
> that the SoC side of the root complex has its own DMA path to an IOMMU,
> distinct from the traffic coming over the PCI bridge. That can
> occasionally be true (e.g. if the root complex IP includes a standalone
> DMA engine), but more often it's just a mistake.
Thank you for the explanation. I understood it. So, I'll drop them on v2.
> > };
> >
> > static bool ipmmu_device_is_allowed(struct device *dev)
> > @@ -723,12 +732,22 @@ static bool ipmmu_device_is_allowed(struct device *dev)
> > if (soc_device_match(soc_denylist))
> > return false;
> >
> > +retry:
> > /* Check whether this device can work with the IPMMU */
> > for (i = 0; i < ARRAY_SIZE(devices_allowlist); i++) {
> > if (!strcmp(dev_name(dev), devices_allowlist[i]))
> > return true;
> > }
> >
> > + /*
> > + * Check whether this device has the parent device like a PCI device
> > + * except "soc".
> > + */
> > + if (dev->parent && strcmp(dev_name(dev->parent), "soc")) {
> > + dev = dev->parent;
> > + goto retry;
> > + }
>
> This is the place where a simple dev_is_pci() check would seem ideal.
I think so. So, I'll use dev_is_pci() and change the timing of the condition
from here to the "retry" location on v2.
Best regards,
Yoshihiro Shimoda
> Thanks,
> Robin.
>
> > +
> > /* Otherwise, do not allow use of IPMMU */
> > return false;
> > }
