(+iommu list for visibility and confirmation of the intended constant externalization, see 2nd point below) Hi Mario, Thanks for your comments, see my answers inline below. On Thu, Jan 24, 2019 at 7:16 AM <Mario.Limonciello@xxxxxxxx> wrote: > > > -----Original Message----- > > From: platform-driver-x86-owner@xxxxxxxxxxxxxxx <platform-driver-x86- > > owner@xxxxxxxxxxxxxxx> On Behalf Of Szabolcs Fruhwald > > Sent: Wednesday, January 23, 2019 5:02 PM > > To: Stuart Hayes > > Cc: platform-driver-x86@xxxxxxxxxxxxxxx; Szabolcs Fruhwald > > Subject: [PATCH] dcdbas: Fix Intel-IOMMU domain allocation failure > > > > > > [EXTERNAL EMAIL] > > > > On Dell systems with intel_iommu enabled, dcdbas platform driver's DMA > > calls fail with ENOMEM / "DMAR: Allocating domain for dcdbas failed". > > As a curiosity was this from manually turning on IOMMU or the automatic IOMMU > enablement caused by 89a6079df791aeace2044ea93be1b397195824ec? > In our case it was manual turn-on. We need to deal with several hw platforms (preferably with a unified sw configuration) and on one particular hw platform (R730XD) we had problems even with intel_iommu=off (no USB). However, we need vt-d / iommu support by security reasons (preventing dma attacks), rather than by other aspects, eg virtualization. So we didn't try to completely turn it off. These issues came up in the past year as we are in the process of upgrading to v4 kernels and iommu support (and complexity) has been significantly improved since v3. > > > > This is because the intel-iommu driver only supports PCI bus / devices, > > therefore it is not (yet) possible to properly allocate and attach iommu > > group/domain and attach devices with no pci parent devices. > > > > This workaround is forcing the intel_iommu implementation to use identity > > mapping for this device, using the DUMMY_DEVICE_DOMAIN_INFO constant value > > defined and used in intel-iommu.c. > > I would think it makes sense to export this out to a common include that both files can > use if it's using the same constant value as drivers/iommu/intel-iommu.c > Absolutely, I thought so too. But, since the actual need to force id-mapping comes from the lack of support for non-pci buses particularly by the intel iommu implementation, it seemed a bit odd to move this constant into iommu.h. Other platforms' implementations are usually handling and hooking into other buses, eg platform bus. However, even these other hw platform iommu implementations are using ACPI or other (bios related) tables to generate source ids, which might still be an issue with drivers like dcdbas, with no real device info in these tables. Not to mention that forcing id-mapping might be a useful tool in driver developers' hands by other reasons too. Therefore, I just came to the conclusion that it is indeed useful to have this constant present in the common iommu header file (but with a more expressing name). Especially, as I just found that dcdbas is *not* the first one to implement this workaround: https://github.com/torvalds/linux/blob/71f3a82fab1b631ae9cb1feb677f498d4ca5007d/drivers/gpu/drm/i915/selftests/mock_gem_device.c#L154 Let me come up with a v2 patch-set, containing a separate patch externalizing this constant from intel_iommu.c to iommu.h and making the above code using it too first, followed by this change in dcdbas. > At least to me it seems likely that dcdbas is just one of the first drivers that will cause this. > > > > > Signed-off-by: Szabolcs Fruhwald <sfruhwald@xxxxxxxxxx> > > --- > > drivers/platform/x86/dcdbas.c | 5 +++++ > > drivers/platform/x86/dcdbas.h | 2 +- > > 2 files changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/platform/x86/dcdbas.c b/drivers/platform/x86/dcdbas.c > > index 88bd7efafe14..a5d6bb1bc590 100644 > > --- a/drivers/platform/x86/dcdbas.c > > +++ b/drivers/platform/x86/dcdbas.c > > @@ -652,6 +652,11 @@ static int dcdbas_probe(struct platform_device *dev) > > > > dcdbas_pdev = dev; > > > > + /* Intel-IOMMU workaround: platform-bus unsupported, force ID-mapping > > */ > > + #if IS_ENABLED(CONFIG_IOMMU_API) && > > defined(CONFIG_INTEL_IOMMU) > > + dev->dev.archdata.iommu = INTEL_IOMMU_DUMMY_DOMAIN; > > + #endif > > + > > /* Check if ACPI WSMT table specifies protected SMI buffer address */ > > error = dcdbas_check_wsmt(); > > if (error < 0) > > diff --git a/drivers/platform/x86/dcdbas.h b/drivers/platform/x86/dcdbas.h > > index 52729a494b00..373eb277933a 100644 > > --- a/drivers/platform/x86/dcdbas.h > > +++ b/drivers/platform/x86/dcdbas.h > > @@ -54,6 +54,7 @@ > > > > #define SMI_CMD_MAGIC (0x534D4931) > > #define SMM_EPS_SIG "$SCB" > > +#define INTEL_IOMMU_DUMMY_DOMAIN ((void *)-1) > > > > #define DCDBAS_DEV_ATTR_RW(_name) \ > > DEVICE_ATTR(_name,0600,_name##_show,_name##_store); > > @@ -114,4 +115,3 @@ struct smm_eps_table { > > } __packed; > > > > #endif /* _DCDBAS_H_ */ > > - > > -- > > 2.20.1.321.g9e740568ce-goog >
