| Message ID | 20230421122538.3389336-1-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | iommu/ipmmu-vmsa: Allow PCIe devices | expand |
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@renesas.com> > --- > 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 :) > + > 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. > }; > > 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. Thanks, Robin. > + > /* Otherwise, do not allow use of IPMMU */ > return false; > }
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@renesas.com> > > --- > > 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; > > }
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; + 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 */ }; 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; + } + /* Otherwise, do not allow use of IPMMU */ return false; }
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@renesas.com> --- drivers/iommu/ipmmu-vmsa.c | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-)