On 2022-02-15 13:42, Will Deacon wrote:
On Tue, Feb 15, 2022 at 01:30:26PM +0000, Robin Murphy wrote:
On 2022-02-15 13:00, Will Deacon wrote:
On Mon, Feb 14, 2022 at 08:55:20PM +0800, Yicong Yang wrote:
On 2022/1/24 21:11, Yicong Yang wrote:
The DMA of HiSilicon PTT device can only work with identical
mapping. So add a quirk for the device to force the domain
passthrough.
Signed-off-by: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 6dc6d8b6b368..6f67a2b1dd27 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2838,6 +2838,21 @@ static int arm_smmu_dev_disable_feature(struct device *dev,
}
}
+#define IS_HISI_PTT_DEVICE(pdev) ((pdev)->vendor == PCI_VENDOR_ID_HUAWEI && \
+ (pdev)->device == 0xa12e)
+
+static int arm_smmu_def_domain_type(struct device *dev)
+{
+ if (dev_is_pci(dev)) {
+ struct pci_dev *pdev = to_pci_dev(dev);
+
+ if (IS_HISI_PTT_DEVICE(pdev))
+ return IOMMU_DOMAIN_IDENTITY;
+ }
+
+ return 0;
+}
+
static struct iommu_ops arm_smmu_ops = {
.capable = arm_smmu_capable,
.domain_alloc = arm_smmu_domain_alloc,
@@ -2863,6 +2878,7 @@ static struct iommu_ops arm_smmu_ops = {
.sva_unbind = arm_smmu_sva_unbind,
.sva_get_pasid = arm_smmu_sva_get_pasid,
.page_response = arm_smmu_page_response,
+ .def_domain_type = arm_smmu_def_domain_type,
.pgsize_bitmap = -1UL, /* Restricted during device attach */
.owner = THIS_MODULE,
};
Is this quirk ok with the SMMU v3 driver? Just want to confirm that I'm on the
right way to dealing with the issue of our device.
I don't think the quirk should be in the SMMUv3 driver. Assumedly, you would
have the exact same problem if you stuck the PTT device behind a different
type of IOMMU, and so the quirk should be handled by a higher level of the
stack.
Conceptually, yes, but I'm inclined to be pragmatic here. Default domain
quirks could only move out as far as the other end of the call from
iommu_get_def_domain_type() - it's not like we could rely on some flag in a
driver which may not even be loaded yet, let alone matched to the device.
And even then there's an equal and opposite argument for why the core code
should have to maintain a list of platform-specific quirks rather than code
specific to the relevant platforms. The fact is that a HiSilicon RCiEP is
not going to end up behind anything other than a HiSilicon IOMMU, and if
those ever stop being SMMUv3 *and* such a quirk still exists we can worry
about it then.
Perhaps, but you know that by adding this hook it's only a matter of time
before we get random compatible string matches in there, so I'd rather keep
the flood gates closed as long as we can.
Given that this is a PCI device, why can't we have a PCI quirk for devices
which require an identity mapping and then handle that in the IOMMU core?
Oh, don't think I *like* having quirks in the driver, it just seems like
the least-worst choice from a bad bunch. All of the default domain
quirks so far (including this one) exist for integrated devices and/or
dodgy firmware setups such that they are platform-specific, so there is
no technical reason for trying to split *some* of them off into a
generic mechanism when the driver-based platform-specific mechanism
still needs to exist anyway (some of them do depend on driver state as
well).
Feel free to test the waters with a patch punting
qcom_smmu_def_domain_type() to core code, but I think you'll struggle to
find a reason to give in the commit message other than "I don't like it".
Ugly as it is, this is the status quo. I don't recall anyone ever arguing
that the equivalent quirks for Intel integrated graphics should be made
generic ;)
I don't know anything about Intel integrated graphics. Have they solved this
problem in a better way, or could they equally make use of a generic quirk?
See intel-iommu's device_def_domain_type() implementation. The shape of
it may seem quite familiar...
Robin.
