BUGZILLA [112941] - Cannot reenable SRIOV after disabling SRIOV on AMD GPU

BUGZILLA [112941] - Cannot reenable SRIOV after disabling SRIOV on AMD GPU

[Zytaruk, Kelly]

Bjorn,

As per our offline discussions I have created Bugzilla #112941 for the SRIOV issue.

When trying to enable SRIOV on AMD GPU after doing a previous enable / disable sequence the following warning is shown in dmesg.  I suspect that there might be something missing from the cleanup on the disable.  

I had a quick look at the code and it is checking for something in the iommu, something to do with being attached to a domain.  I am not familiar with this code yet (what does it mean to be attached to a domain?) so it might take a little while before I can get the time to check it out and understand it.

>From a quick glance I notice that during SRIOV enable the function do_attach()  in amd_iommu.c is called but during disable I don't see a corresponding call to do_detach (...).  
do_detach(...) is called in the second enable SRIOV  sequence as a cleanup because it thinks that the iommu is still attached which it shouldn't be (as far as I understand).

If the iommu reports that the device is being removed why isn't it also detached??? Is this by design or an omission?
I see the following in dmesg when I do a disable, note the device is removed.

[  131.674066] pci 0000:02:00.0: PME# disabled
[  131.682191] iommu: Removing device 0000:02:00.0 from group 2

Stack trace of warn is shown below.

[  368.510742] pci 0000:02:00.2: calling pci_fixup_video+0x0/0xb1
[  368.510847] pci 0000:02:00.3: [1002:692f] type 00 class 0x030000
[  368.510888] pci 0000:02:00.3: Max Payload Size set to 256 (was 128, max 256)
[  368.510907] pci 0000:02:00.3: calling quirk_no_pm_reset+0x0/0x1a
[  368.511005] vgaarb: device added: PCI:0000:02:00.3,decodes=io+mem,owns=none,locks=none
[  368.511421] ------------[ cut here ]------------
[  368.511426] WARNING: CPU: 1 PID: 3390 at drivers/pci/ats.c:85 pci_disable_ats+0x26/0xa4()
[  368.511428] Modules linked in: sriov(O) parport_pc ppdev bnep lp parport rfcomm bluetooth rfkill binfmt_misc nfsd auth_rpcgss oid_registry nfs_acl nfs lockd grace fscache sunrpc bridge stp llc loop hid_generic usbhid hid kvm_amd snd_hda_codec_hdmi snd_hda_codec_generic snd_hda_intel snd_hda_codec kvm snd_hda_core ohci_pci xhci_pci xhci_hcd snd_hwdep ohci_hcd acpi_cpufreq ehci_pci irqbypass ehci_hcd snd_pcm usbcore ghash_clmulni_intel tpm_tis drbg ansi_cprng sp5100_tco i2c_piix4 tpm aesni_intel i2c_core fam15h_power edac_mce_amd snd_seq snd_timer snd_seq_device snd soundcore k10temp edac_core aes_x86_64 usb_common ablk_helper wmi evdev cryptd pcspkr processor video lrw gf128mul glue_helper button ext4 crc16 mbcache jbd2 sg sd_mod ata_generic ahci libahci pata_atiixp sdhci_pci sdhci tg3 ptp libata crc32c_intel pps_core mmc_core libphy scsi_mod
[  368.511483] CPU: 1 PID: 3390 Comm: bash Tainted: G        W  O    4.5.0-rc3+ #2
[  368.511484] Hardware name: AMD BANTRY/Bantry, BIOS TBT4521N_03 05/21/2014
[  368.511486]  0000000000000000 ffff880840e8b948 ffffffff8124558c 0000000000000000
[  368.511490]  0000000000000009 ffff880840e8b988 ffffffff8105d643 ffff880840e8b998
[  368.511492]  ffffffff8128dd0a ffff88084034f000 ffff88084034f098 0000000000000292
[  368.511496] Call Trace:
[  368.511500]  [<ffffffff8124558c>] dump_stack+0x63/0x7f
[  368.511504]  [<ffffffff8105d643>] warn_slowpath_common+0x9c/0xb6
[  368.511507]  [<ffffffff8128dd0a>] ? pci_disable_ats+0x26/0xa4
[  368.511510]  [<ffffffff8105d672>] warn_slowpath_null+0x15/0x17
[  368.511513]  [<ffffffff8128dd0a>] pci_disable_ats+0x26/0xa4
[  368.511516]  [<ffffffff8147fed3>] ? _raw_write_unlock_irqrestore+0x20/0x34
[  368.511518]  [<ffffffff81328f9f>] detach_device+0x83/0x90
[  368.511520]  [<ffffffff81329067>] amd_iommu_attach_device+0x62/0x2eb
[  368.511523]  [<ffffffff81322e21>] __iommu_attach_device+0x1c/0x71
[  368.511525]  [<ffffffff8132418a>] iommu_group_add_device+0x260/0x300
[  368.511528]  [<ffffffff81323e6d>] ? pci_device_group+0xa6/0x10e
[  368.511530]  [<ffffffff813242ac>] iommu_group_get_for_dev+0x82/0xa0
[  368.511532]  [<ffffffff81326bb0>] amd_iommu_add_device+0x110/0x2c8
[  368.511534]  [<ffffffff81323149>] iommu_bus_notifier+0x30/0xa5
[  368.511537]  [<ffffffff81076134>] notifier_call_chain+0x32/0x5c
[  368.511541]  [<ffffffff8107626b>] __blocking_notifier_call_chain+0x41/0x5a
[  368.511544]  [<ffffffff81076293>] blocking_notifier_call_chain+0xf/0x11
[  368.511547]  [<ffffffff8133b01a>] device_add+0x38b/0x52a
[  368.511550]  [<ffffffff81271d32>] pci_device_add+0x25c/0x27c
[  368.511553]  [<ffffffff8128e69d>] pci_enable_sriov+0x44c/0x642
[  368.511557]  [<ffffffffa051471f>] sriov_enable+0x94/0xde [sriov]
[  368.511560]  [<ffffffffa05147bd>] cmd_sriov+0x54/0x8d [sriov]
[  368.511563]  [<ffffffffa0514352>] dev_write+0x95/0xb8 [sriov]
[  368.511566]  [<ffffffff81165577>] __vfs_write+0x23/0xa2
[  368.511570]  [<ffffffff811deeda>] ? security_file_permission+0x37/0x40
[  368.511573]  [<ffffffff81165fbe>] ? rw_verify_area+0x67/0xcc
[  368.511575]  [<ffffffff811668fe>] vfs_write+0x86/0xdc
[  368.511578]  [<ffffffff81166af0>] SyS_write+0x50/0x85
[  368.511632]  [<ffffffff814804ae>] entry_SYSCALL_64_fastpath+0x12/0x71
[  368.511634] ---[ end trace 69e2140f488cb003 ]---

Thanks,
Kelly

Re: BUGZILLA [112941] - Cannot reenable SRIOV after disabling SRIOV on AMD GPU

[Bjorn Helgaas]

[+cc Joerg, Alex]

Hi Kelly,

On Tue, Feb 23, 2016 at 03:52:13PM +0000, Zytaruk, Kelly wrote:
> As per our offline discussions I have created Bugzilla #112941 for
> the SRIOV issue.

https://bugzilla.kernel.org/show_bug.cgi?id=112941

> When trying to enable SRIOV on AMD GPU after doing a previous enable
> / disable sequence the following warning is shown in dmesg.  I
> suspect that there might be something missing from the cleanup on
> the disable.  
> 
> I had a quick look at the code and it is checking for something in
> the iommu, something to do with being attached to a domain.  I am
> not familiar with this code yet (what does it mean to be attached to
> a domain?) so it might take a little while before I can get the time
> to check it out and understand it.
> 
> From a quick glance I notice that during SRIOV enable the function
> do_attach()  in amd_iommu.c is called but during disable I don't see
> a corresponding call to do_detach (...).  do_detach(...) is called
> in the second enable SRIOV  sequence as a cleanup because it thinks
> that the iommu is still attached which it shouldn't be (as far as I
> understand).
> 
> If the iommu reports that the device is being removed why isn't it
> also detached??? Is this by design or an omission?

I don't know enough about the IOMMU code to understand this, but maybe
the IOMMU experts I copied do.

> I see the following in dmesg when I do a disable, note the device is removed.
> 
> [  131.674066] pci 0000:02:00.0: PME# disabled
> [  131.682191] iommu: Removing device 0000:02:00.0 from group 2
> 
> Stack trace of warn is shown below.
> 
> [  368.510742] pci 0000:02:00.2: calling pci_fixup_video+0x0/0xb1
> [  368.510847] pci 0000:02:00.3: [1002:692f] type 00 class 0x030000
> [  368.510888] pci 0000:02:00.3: Max Payload Size set to 256 (was 128, max 256)
> [  368.510907] pci 0000:02:00.3: calling quirk_no_pm_reset+0x0/0x1a
> [  368.511005] vgaarb: device added: PCI:0000:02:00.3,decodes=io+mem,owns=none,locks=none
> [  368.511421] ------------[ cut here ]------------
> [  368.511426] WARNING: CPU: 1 PID: 3390 at drivers/pci/ats.c:85 pci_disable_ats+0x26/0xa4()

This warning is because dev->ats_enabled doesn't have the value we
expect.  I think we only modify ats_enabled in two places.  Can you
stick a dump_stack() at those two places?  Maybe a little more context
will make this obvious.

Bjorn

RE: BUGZILLA [112941] - Cannot reenable SRIOV after disabling SRIOV on AMD GPU

[Zytaruk, Kelly]

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> Sent: Tuesday, February 23, 2016 12:02 PM
> To: Zytaruk, Kelly
> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org;
> bhelgaas@google.com; Marsan, Luugi; Joerg Roedel; Alex Williamson
> Subject: Re: BUGZILLA [112941] - Cannot reenable SRIOV after disabling SRIOV
> on AMD GPU
> 
> [+cc Joerg, Alex]
> 
> Hi Kelly,
> 
> On Tue, Feb 23, 2016 at 03:52:13PM +0000, Zytaruk, Kelly wrote:
> > As per our offline discussions I have created Bugzilla #112941 for the
> > SRIOV issue.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=112941
> 
> > When trying to enable SRIOV on AMD GPU after doing a previous enable /
> > disable sequence the following warning is shown in dmesg.  I suspect
> > that there might be something missing from the cleanup on the disable.
> >
> > I had a quick look at the code and it is checking for something in the
> > iommu, something to do with being attached to a domain.  I am not
> > familiar with this code yet (what does it mean to be attached to a
> > domain?) so it might take a little while before I can get the time to
> > check it out and understand it.
> >
> > From a quick glance I notice that during SRIOV enable the function
> > do_attach()  in amd_iommu.c is called but during disable I don't see a
> > corresponding call to do_detach (...).  do_detach(...) is called in
> > the second enable SRIOV  sequence as a cleanup because it thinks that
> > the iommu is still attached which it shouldn't be (as far as I
> > understand).
> >
> > If the iommu reports that the device is being removed why isn't it
> > also detached??? Is this by design or an omission?
> 
> I don't know enough about the IOMMU code to understand this, but maybe the
> IOMMU experts I copied do.
> 
> > I see the following in dmesg when I do a disable, note the device is removed.
> >
> > [  131.674066] pci 0000:02:00.0: PME# disabled [  131.682191] iommu:
> > Removing device 0000:02:00.0 from group 2
> >
> > Stack trace of warn is shown below.
> >
> > [  368.510742] pci 0000:02:00.2: calling pci_fixup_video+0x0/0xb1 [
> > 368.510847] pci 0000:02:00.3: [1002:692f] type 00 class 0x030000 [
> > 368.510888] pci 0000:02:00.3: Max Payload Size set to 256 (was 128,
> > max 256) [  368.510907] pci 0000:02:00.3: calling
> > quirk_no_pm_reset+0x0/0x1a [  368.511005] vgaarb: device added:
> > PCI:0000:02:00.3,decodes=io+mem,owns=none,locks=none
> > [  368.511421] ------------[ cut here ]------------ [  368.511426]
> > WARNING: CPU: 1 PID: 3390 at drivers/pci/ats.c:85
> > pci_disable_ats+0x26/0xa4()
> 
> This warning is because dev->ats_enabled doesn't have the value we expect.  I
> think we only modify ats_enabled in two places.  Can you stick a dump_stack() at
> those two places?  Maybe a little more context will make this obvious.
> 

Yes, I only see the two places.
The dump_stack() doesn't help much other than tell me that dev->ats_enabled is never set to 0.  The code path never gets hit.

dev->ats_enabled is set to 1 when the VF is created but it is not set to 0 when the VF is destroyed.

The code path looks like detach_device (from amd_iommu.c) calls pci_disable_ats() which sets ats_enabled = 0.
>From the log trace detach_device() is not called when SRIOV is disabled, so when SRIOV is enabled again ats_enabled is still == 1. 

I am not sure where detach_device() should be called but my guess is that detach_device() should be somewhere in the disable SRIOV path.  I don't yet know enough about the iommu code.

> Bjorn

RE: BUGZILLA [112941] - Cannot reenable SRIOV after disabling SRIOV on AMD GPU

[Zytaruk, Kelly]

> -----Original Message-----
> From: linux-pci-owner@vger.kernel.org [mailto:linux-pci-
> owner@vger.kernel.org] On Behalf Of Zytaruk, Kelly
> Sent: Tuesday, February 23, 2016 12:47 PM
> To: Bjorn Helgaas
> Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org;
> bhelgaas@google.com; Marsan, Luugi; Joerg Roedel; Alex Williamson
> Subject: RE: BUGZILLA [112941] - Cannot reenable SRIOV after disabling SRIOV
> on AMD GPU
> 
> 
> 
> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:helgaas@kernel.org]
> > Sent: Tuesday, February 23, 2016 12:02 PM
> > To: Zytaruk, Kelly
> > Cc: linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org;
> > bhelgaas@google.com; Marsan, Luugi; Joerg Roedel; Alex Williamson
> > Subject: Re: BUGZILLA [112941] - Cannot reenable SRIOV after disabling
> > SRIOV on AMD GPU
> >
> > [+cc Joerg, Alex]
> >
> > Hi Kelly,
> >
> > On Tue, Feb 23, 2016 at 03:52:13PM +0000, Zytaruk, Kelly wrote:
> > > As per our offline discussions I have created Bugzilla #112941 for
> > > the SRIOV issue.
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=112941
> >
> > > When trying to enable SRIOV on AMD GPU after doing a previous enable
> > > / disable sequence the following warning is shown in dmesg.  I
> > > suspect that there might be something missing from the cleanup on the
> disable.
> > >
> > > I had a quick look at the code and it is checking for something in
> > > the iommu, something to do with being attached to a domain.  I am
> > > not familiar with this code yet (what does it mean to be attached to
> > > a
> > > domain?) so it might take a little while before I can get the time
> > > to check it out and understand it.
> > >
> > > From a quick glance I notice that during SRIOV enable the function
> > > do_attach()  in amd_iommu.c is called but during disable I don't see
> > > a corresponding call to do_detach (...).  do_detach(...) is called
> > > in the second enable SRIOV  sequence as a cleanup because it thinks
> > > that the iommu is still attached which it shouldn't be (as far as I
> > > understand).
> > >
> > > If the iommu reports that the device is being removed why isn't it
> > > also detached??? Is this by design or an omission?
> >
> > I don't know enough about the IOMMU code to understand this, but maybe
> > the IOMMU experts I copied do.
> >
> > > I see the following in dmesg when I do a disable, note the device is removed.
> > >
> > > [  131.674066] pci 0000:02:00.0: PME# disabled [  131.682191] iommu:
> > > Removing device 0000:02:00.0 from group 2
> > >
> > > Stack trace of warn is shown below.
> > >
> > > [  368.510742] pci 0000:02:00.2: calling pci_fixup_video+0x0/0xb1 [
> > > 368.510847] pci 0000:02:00.3: [1002:692f] type 00 class 0x030000 [
> > > 368.510888] pci 0000:02:00.3: Max Payload Size set to 256 (was 128,
> > > max 256) [  368.510907] pci 0000:02:00.3: calling
> > > quirk_no_pm_reset+0x0/0x1a [  368.511005] vgaarb: device added:
> > > PCI:0000:02:00.3,decodes=io+mem,owns=none,locks=none
> > > [  368.511421] ------------[ cut here ]------------ [  368.511426]
> > > WARNING: CPU: 1 PID: 3390 at drivers/pci/ats.c:85
> > > pci_disable_ats+0x26/0xa4()
> >
> > This warning is because dev->ats_enabled doesn't have the value we
> > expect.  I think we only modify ats_enabled in two places.  Can you
> > stick a dump_stack() at those two places?  Maybe a little more context will
> make this obvious.
> >
> 
> Yes, I only see the two places.
> The dump_stack() doesn't help much other than tell me that dev->ats_enabled is
> never set to 0.  The code path never gets hit.
> 
> dev->ats_enabled is set to 1 when the VF is created but it is not set to 0 when
> the VF is destroyed.
> 
> The code path looks like detach_device (from amd_iommu.c) calls
> pci_disable_ats() which sets ats_enabled = 0.
> From the log trace detach_device() is not called when SRIOV is disabled, so when
> SRIOV is enabled again ats_enabled is still == 1.
> 
> I am not sure where detach_device() should be called but my guess is that
> detach_device() should be somewhere in the disable SRIOV path.  I don't yet
> know enough about the iommu code.

I have made some progress on this and it is related in part to the asymmetrical nature of the iommu attach/detach calls.
There are three code flows that need to be examined and I have summarized them below.

The iommu attach code flows as follows;
.attach_dev = amd_iommu_attach_device
    --> amd_iommu_attach_device
        --> get dev_data from dev.archdata.iommu
               If dev_data->domain != NULL  --> detach_device()   <-- the WARNING is coming from this code path.
               attach_device ()

The iommu detach code path flows as follows;
.detach_dev = amd_iommu_detach_device
    --> amd_iommu_detach_device 
        --> detach_device()
            -->  __detach_device()
                --> do_detach()
                    --> set dev_data->domain = NULL;
            --> pci_disable_ats()    <-- expects ats_enabled == 1 on entry
                --> Set ats_enabled = 0

And finally when pci_enable_sriov() is called the following flow is important;
--> virtfn_add()    <-- allocates a new virtual function device
    --> pci_device_add()
        --> iommu_init_device()
            --> find_dev_data()

Now here is the problem.  
When pci_enable_sriov is called for the first time, amd_iommu_attach_device() gets called and it adds dev->archdata.iommu to its dev_data list (stored by device_id which looks like it is related to BDF) . Remember that the value of dev_data->domain is saved here.

When pci_disable_sriov is called, amd_iommu_detach_device is NOT called.  The dev_data for the device is still on the iommu dev_data list even though the device has been destroyed.

When pci_enable_sriov() is called for the second time it creates new dev's for the virtual functions BUT the dev_data for the dev (identified by device_id) still remains in the iommu dev_data list. 

Iommu_init_device() searches the dev_data list to see if a dev_data already exists for this dev. It erroneously finds one.  When amd_iommu_attach_device() is ultimately called it uses the dev_data from the previous dev and sees that dev_data->domain != NULL.  This causes the call to detach_device() which eventually calls pci_disable_ats().  BUT this is a new dev and ats is not enabled yet and the ats_enabled flag == 0.  Hence the WARNING and bug.

I have done the triage but I am not sure where the fix should be.  I quite accidentally found the following somewhat related thread at http://www.gossamer-threads.com/lists/linux/kernel/2225731.  It seems that he is having a similar problem but on a different platform.

I don't know if the asynchronous nature of the iommu attach/detach is by design or if it is broken somewhere up the tree and just not working in my case.  Maybe one of the iommu owners could answer this.


> 
> > Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of
> a message to majordomo@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

Re: BUGZILLA [112941] - Cannot reenable SRIOV after disabling SRIOV on AMD GPU

[Joerg Roedel]

Hi Kelly,

On Wed, Feb 24, 2016 at 06:29:56PM +0000, Zytaruk, Kelly wrote:
> I don't know if the asynchronous nature of the iommu attach/detach is
> by design or if it is broken somewhere up the tree and just not
> working in my case.  Maybe one of the iommu owners could answer this.

Thanks a lot for your bug report and the detailed analysis of the
issue. I attached a possible fix for you to try out. Can you please test
it and report whether it changes anything?


Thanks,

	Joerg

>From 027b6489833aef8ce5ec46cef3e0f9e6a61dbdcd Mon Sep 17 00:00:00 2001
From: Joerg Roedel <jroedel@suse.de>
Date: Fri, 26 Feb 2016 16:48:59 +0100
Subject: [PATCH] iommu/amd: Detach device from domain before removal

Detach the device that is about to be removed from its
domain (if it has one) to clear any related state like DTE
entry and device's ATS state.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/amd_iommu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 539b0de..60cc89f 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -114,6 +114,7 @@ struct kmem_cache *amd_iommu_irq_cache;
 
 static void update_domain(struct protection_domain *domain);
 static int protection_domain_init(struct protection_domain *domain);
+static void detach_device(struct device *dev);
 
 /*
  * For dynamic growth the aperture size is split into ranges of 128MB of
@@ -384,6 +385,9 @@ static void iommu_uninit_device(struct device *dev)
 	if (!dev_data)
 		return;
 
+	if (dev_data->domain)
+		detach_device(dev);
+
 	iommu_device_unlink(amd_iommu_rlookup_table[dev_data->devid]->iommu_dev,
 			    dev);
 
-- 
1.8.4.5

RE: BUGZILLA [112941] - Cannot reenable SRIOV after disabling SRIOV on AMD GPU

[Zytaruk, Kelly]

> -----Original Message-----
> From: Joerg Roedel [mailto:joro@8bytes.org]
> Sent: Friday, February 26, 2016 10:56 AM
> To: Zytaruk, Kelly
> Cc: Bjorn Helgaas; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org;
> bhelgaas@google.com; Marsan, Luugi; Alex Williamson
> Subject: Re: BUGZILLA [112941] - Cannot reenable SRIOV after disabling SRIOV
> on AMD GPU
> 
> Hi Kelly,
> 
> On Wed, Feb 24, 2016 at 06:29:56PM +0000, Zytaruk, Kelly wrote:
> > I don't know if the asynchronous nature of the iommu attach/detach is
> > by design or if it is broken somewhere up the tree and just not
> > working in my case.  Maybe one of the iommu owners could answer this.
> 
> Thanks a lot for your bug report and the detailed analysis of the issue. I attached
> a possible fix for you to try out. Can you please test it and report whether it
> changes anything?
> 
Joerg,

I applied the fix and the WARN on ats_enabled flag goes away.  The detach_device() gets called against the correct dev when pci_sriov_disable is called.  This looks like it is fixed.

I have a couple questions;

1)     find_dev_data()
I put some printk statements into the enable and disable path for iommu.  On the first enable in find_dev_data() I see the following.  Note that the archdata.iommu data area does not exist and must be initialized;

[ 2237.701423] pci_device_add - call device_add for base device 0000:02:00.0 dev->ats_enabled = 0
[ 2237.701555] vgaarb: device added: PCI:0000:02:00.0,decodes=io+mem,owns=none,locks=none
[ 2237.701560] iommu_init_device - find archdata.iommu for dev 0000:02:00.0, device id = 512
[ 2237.701565] find_dev_data - no archdata.iommu for devid 512, allocate a new one
[ 2237.701568] find_dev_data - devid 512 not attached to domain

One the second enable (after a disable) find_dev_data() finds and reuses the previous archdata.iommu as shown below.

[ 2316.549788] pci_device_add - call device_add for base device 0000:02:00.0 dev->ats_enabled = 0
[ 2316.549931] vgaarb: device added: PCI:0000:02:00.0,decodes=io+mem,owns=none,locks=none
[ 2316.549936] iommu_init_device - find archdata.iommu for dev 0000:02:00.0, device id = 512
[ 2316.549942] find_dev_data - found an existing archdata.iommu for devid 512
[ 2316.549944] find_dev_data - devid 512 not attached to domain

Since the second enable is reusing the archdata.iommu from the first enable is there any further cleanup that would need to be done to the archdata.iommu data area? 
What is this area used for?  I understand that archdata is platform specific but what does iommu use it for, is there a good document that describes its use or do I have to read through the source code?
How can I test to ensure that it is properly reused and has proper data integrity?

2) What is "dev_data->domain" and "group" in relation to iommu.  I tried google and came up with meaningless references.  Is it documented anywhere?

Thanks,
Kelly

> 
> Thanks,
> 
> 	Joerg
> 
> From 027b6489833aef8ce5ec46cef3e0f9e6a61dbdcd Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <jroedel@suse.de>
> Date: Fri, 26 Feb 2016 16:48:59 +0100
> Subject: [PATCH] iommu/amd: Detach device from domain before removal
> 
> Detach the device that is about to be removed from its domain (if it has one) to
> clear any related state like DTE entry and device's ATS state.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/amd_iommu.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c index
> 539b0de..60cc89f 100644
> --- a/drivers/iommu/amd_iommu.c
> +++ b/drivers/iommu/amd_iommu.c
> @@ -114,6 +114,7 @@ struct kmem_cache *amd_iommu_irq_cache;
> 
>  static void update_domain(struct protection_domain *domain);  static int
> protection_domain_init(struct protection_domain *domain);
> +static void detach_device(struct device *dev);
> 
>  /*
>   * For dynamic growth the aperture size is split into ranges of 128MB of @@ -
> 384,6 +385,9 @@ static void iommu_uninit_device(struct device *dev)
>  	if (!dev_data)
>  		return;
> 
> +	if (dev_data->domain)
> +		detach_device(dev);
> +
>  	iommu_device_unlink(amd_iommu_rlookup_table[dev_data->devid]-
> >iommu_dev,
>  			    dev);
> 
> --
> 1.8.4.5

Re: BUGZILLA [112941] - Cannot reenable SRIOV after disabling SRIOV on AMD GPU

[Joerg Roedel]

Hi Kelly,

On Fri, Feb 26, 2016 at 07:16:29PM +0000, Zytaruk, Kelly wrote:
> I applied the fix and the WARN on ats_enabled flag goes away.  The
> detach_device() gets called against the correct dev when
> pci_sriov_disable is called.  This looks like it is fixed.

Great, thanks for testing. I'll send the patch upstream so that it gets
included into v4.5

> I have a couple questions;
> 
> 1)     find_dev_data()
> I put some printk statements into the enable and disable path for
> iommu.  On the first enable in find_dev_data() I see the following.
> Note that the archdata.iommu data area does not exist and must be
> initialized;
> 
> [ 2237.701423] pci_device_add - call device_add for base device 0000:02:00.0 dev->ats_enabled = 0
> [ 2237.701555] vgaarb: device added: PCI:0000:02:00.0,decodes=io+mem,owns=none,locks=none
> [ 2237.701560] iommu_init_device - find archdata.iommu for dev 0000:02:00.0, device id = 512
> [ 2237.701565] find_dev_data - no archdata.iommu for devid 512, allocate a new one
> [ 2237.701568] find_dev_data - devid 512 not attached to domain
> 
> One the second enable (after a disable) find_dev_data() finds and reuses the previous archdata.iommu as shown below.
> 
> [ 2316.549788] pci_device_add - call device_add for base device 0000:02:00.0 dev->ats_enabled = 0
> [ 2316.549931] vgaarb: device added: PCI:0000:02:00.0,decodes=io+mem,owns=none,locks=none
> [ 2316.549936] iommu_init_device - find archdata.iommu for dev 0000:02:00.0, device id = 512
> [ 2316.549942] find_dev_data - found an existing archdata.iommu for devid 512
> [ 2316.549944] find_dev_data - devid 512 not attached to domain
> 
> Since the second enable is reusing the archdata.iommu from the first
> enable is there any further cleanup that would need to be done to the
> archdata.iommu data area?

Possibly yes, I need to have a closer look there. That caching of
dev_data structures is done for historical reasons. I'll check first if
this is still necessary.

> What is this area used for?  I understand that archdata is platform
> specific but what does iommu use it for, is there a good document that
> describes its use or do I have to read through the source code?
> How can I test to ensure that it is properly reused and has proper
> data integrity?

There are no documents about the inner structure of the AMD IOMMU driver
besides the source code. The dev_data area is used to attach
iommu-driver specific data (like the domain it is attached to) to a
struct device.

> 
> 2) What is "dev_data->domain" and "group" in relation to iommu.  I
> tried google and came up with meaningless references.  Is it
> documented anywhere?

The dev_data->domain member points to the domain this device is
currently attached to, while group points to the iommu-group the device
is in.


	Joerg