Re: [PATCH] dcdbas: Fix Intel-IOMMU domain allocation failure

Re: [PATCH] dcdbas: Fix Intel-IOMMU domain allocation failure

[Szabolcs Fruhwald via iommu]

(+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-8PEkshWhKlo@public.gmane.org> wrote:
>
> > -----Original Message-----
> > From: platform-driver-x86-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org <platform-driver-x86-
> > owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> On Behalf Of Szabolcs Fruhwald
> > Sent: Wednesday, January 23, 2019 5:02 PM
> > To: Stuart Hayes
> > Cc: platform-driver-x86-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; 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-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > ---
> >  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
>

Re: [PATCH] dcdbas: Fix Intel-IOMMU domain allocation failure

[Andy Shevchenko]

On Thu, Jan 24, 2019 at 10:31 PM Szabolcs Fruhwald <sfruhwald-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>
> (+iommu list for visibility and confirmation of the intended constant
> externalization, see 2nd point below)

> 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.

Wait... It sounds to me like a part of arch code where we define
arch_setup_pdev_archdata() and use this dummy domain.
Though dummy domain definition should come from IOMMU framework.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] dcdbas: Fix Intel-IOMMU domain allocation failure

[Szabolcs Fruhwald via iommu]

Hi Andy,

I took a quick look at arch_setup_pdev_archdata(), overridden it in
dcdbas.c and it works well (despite it's being called twice, since
it's called from platform_device_alloc and platform_device_register).

However, as I am not super familiar with ELF weak method references,
especially with its scope resolution / versioning part, so as I see
this weak method was introduced mainly for arch/** specific hooks.
Is it safe to override this method from driver code, when let's say
there's another implementation in the x86 arch code (currently there
isn't)?

So while I agree that archdata should be ideally written from within
this hook to make sure the value is there when the bus notifiers call
the iommu code (currently not registered for platform_bus), I am just
a bit worried that this might mask a future generic arch
implementation and could cause issues. What do you think?

Best Regards,
Szabolcs

On Thu, Jan 24, 2019 at 1:44 PM Andy Shevchenko
<andy.shevchenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
> On Thu, Jan 24, 2019 at 10:31 PM Szabolcs Fruhwald <sfruhwald-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> >
> > (+iommu list for visibility and confirmation of the intended constant
> > externalization, see 2nd point below)
>
> > 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.
>
> Wait... It sounds to me like a part of arch code where we define
> arch_setup_pdev_archdata() and use this dummy domain.
> Though dummy domain definition should come from IOMMU framework.
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH] dcdbas: Fix Intel-IOMMU domain allocation failure

[Andy Shevchenko]

On Fri, Jan 25, 2019 at 3:50 AM Szabolcs Fruhwald <sfruhwald-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:

First of all, please do not top post!

> I took a quick look at arch_setup_pdev_archdata(), overridden it in
> dcdbas.c and it works well (despite it's being called twice, since
> it's called from platform_device_alloc and platform_device_register).
>
> However, as I am not super familiar with ELF weak method references,
> especially with its scope resolution / versioning part, so as I see
> this weak method was introduced mainly for arch/** specific hooks.
> Is it safe to override this method from driver code, when let's say
> there's another implementation in the x86 arch code (currently there
> isn't)?

No, it should be done somewhere in arch/x86.

OTOH, Intel iommu driver can do it based on the check dev_is_pci().
For now I think it's better to solve this inside Intel iommu driver.

> So while I agree that archdata should be ideally written from within
> this hook to make sure the value is there when the bus notifiers call
> the iommu code (currently not registered for platform_bus), I am just
> a bit worried that this might mask a future generic arch
> implementation and could cause issues. What do you think?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] dcdbas: Fix Intel-IOMMU domain allocation failure

[Robin Murphy]

On 25/01/2019 11:58, Andy Shevchenko wrote:
> On Fri, Jan 25, 2019 at 3:50 AM Szabolcs Fruhwald <sfruhwald-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> 
> First of all, please do not top post!
> 
>> I took a quick look at arch_setup_pdev_archdata(), overridden it in
>> dcdbas.c and it works well (despite it's being called twice, since
>> it's called from platform_device_alloc and platform_device_register).
>>
>> However, as I am not super familiar with ELF weak method references,
>> especially with its scope resolution / versioning part, so as I see
>> this weak method was introduced mainly for arch/** specific hooks.
>> Is it safe to override this method from driver code, when let's say
>> there's another implementation in the x86 arch code (currently there
>> isn't)?
> 
> No, it should be done somewhere in arch/x86.
> 
> OTOH, Intel iommu driver can do it based on the check dev_is_pci().
> For now I think it's better to solve this inside Intel iommu driver.

Indeed - hacking code into individual device drivers which is entirely 
specific to the current implementation of the intel-iommu driver sounds 
like a recipe for a maintenance disaster (not to mention the extra fun 
if any of those devices also end up in AMD-based systems).

AFAICS, the VT-d spec accommodates non-PCI devices via DRHD and 
corresponding ANDD entries, and the driver already has at least some 
degree of support for those - see dmar_acpi_insert_dev_scope() - so it 
may not need much more than just hooking up to the platform bus. If 
these platform devices do actually master through the IOMMU but don't 
have an appropriate device scope described, then frankly that's broken 
firmware, but the place to work around that would still be in the DMAR code.

Robin.

Re: [PATCH] dcdbas: Fix Intel-IOMMU domain allocation failure

[Szabolcs Fruhwald via iommu]

On Fri, Jan 25, 2019 at 5:12 AM Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
>
> On 25/01/2019 11:58, Andy Shevchenko wrote:
> > On Fri, Jan 25, 2019 at 3:50 AM Szabolcs Fruhwald <sfruhwald-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> >
> > First of all, please do not top post!
> >
> >> I took a quick look at arch_setup_pdev_archdata(), overridden it in
> >> dcdbas.c and it works well (despite it's being called twice, since
> >> it's called from platform_device_alloc and platform_device_register).
> >>
> >> However, as I am not super familiar with ELF weak method references,
> >> especially with its scope resolution / versioning part, so as I see
> >> this weak method was introduced mainly for arch/** specific hooks.
> >> Is it safe to override this method from driver code, when let's say
> >> there's another implementation in the x86 arch code (currently there
> >> isn't)?
> >
> > No, it should be done somewhere in arch/x86.
> >
> > OTOH, Intel iommu driver can do it based on the check dev_is_pci().
> > For now I think it's better to solve this inside Intel iommu driver.
>
> Indeed - hacking code into individual device drivers which is entirely
> specific to the current implementation of the intel-iommu driver sounds
> like a recipe for a maintenance disaster (not to mention the extra fun
> if any of those devices also end up in AMD-based systems).
>
> AFAICS, the VT-d spec accommodates non-PCI devices via DRHD and
> corresponding ANDD entries, and the driver already has at least some
> degree of support for those - see dmar_acpi_insert_dev_scope() - so it
> may not need much more than just hooking up to the platform bus. If
> these platform devices do actually master through the IOMMU but don't
> have an appropriate device scope described, then frankly that's broken
> firmware, but the place to work around that would still be in the DMAR code.
>
> Robin.

It does, however, unfortunately, AFAK this 'device' is not listed in
any of those
ACPI tables. This is not even a real device, it's a CMOS chip with a fixed mem
segment and it needs dma only to let it read out the cmd data from a buffer.
So it's not really possible to support proper iommu for this driver.
Same is true for the previously mentioned drm915 mock test device.

Either way, there are device drivers which for various reasons may need to
opt-out / force 1:1 iommu mapping to work properly when iommu is turned on.

I agree that the current way of handling this (archdata.iommu) in intel_iommu
driver is not nice, a better solution might be to create a generalized way
through iommu.h (eg a method or constant value stored in main dev /
pdev struct),
which is well documented to be used only for legacy drivers / special
cases where
forced 1:1 mapping is truly justified / last resort. And other arch
iommu drivers can
support this, if and until necessary (legacy drivers don't need anymore).