[git pull] AMD-IOMMU (AMD-Vi) updates for 2.6.32

[git pull] AMD-IOMMU (AMD-Vi) updates for 2.6.32

[Joerg Roedel]

Hi Ingo,

The following changes since commit 37d0892c5a94e208cf863e3b7bac014edee4346d:
  Ian Kent (1):
        autofs4 - fix missed case when changing to use struct path

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/joro/linux-2.6-iommu.git amd-iommu/2.6.32

Joerg Roedel (37):
      x86/amd-iommu: Dump fault entry on DTE error
      x86/amd-iommu: Dump illegal command on ILLEGAL_COMMAND_ERROR
      x86/amd-iommu: Introduce function for iommu-local domain flush
      x86/amd-iommu: Remove some merge helper code
      x86/amd-iommu: replace "AMD IOMMU" by "AMD-Vi"
      x86/amd-iommu: Remove redundant 'IOMMU' string
      x86/amd-iommu: fix broken check in amd_iommu_flush_all_devices
      x86/amd-iommu: Add function to flush all DTEs on one IOMMU
      x86/amd-iommu: Add reset function for command buffers
      x86/amd-iommu: Reset command buffer on ILLEGAL_COMMAND_ERROR
      x86/amd-iommu: Panic if IOMMU command buffer reset fails
      x86/amd-iommu: Reset command buffer if wait loop fails
      x86/amd-iommu: Make fetch_pte aware of dynamic mapping levels
      x86/amd-iommu: Use fetch_pte in iommu_unmap_page
      x86/amd-iommu: Use fetch_pte in amd_iommu_iova_to_phys
      x86/amd-iommu: Add a gneric version of amd_iommu_flush_all_devices
      x86/amd-iommu: Introduce set_dte_entry function
      x86/amd-iommu: Flush domains if address space size was increased
      x86/amd-iommu: Introduce increase_address_space function
      x86/amd-iommu: Change alloc_pte to support 64 bit address space
      x86/amd-iommu: Remove last usages of IOMMU_PTE_L0_INDEX
      x86/amd-iommu: Remove bus_addr check in iommu_map_page
      x86/amd-iommu: Use 2-level page tables for dma_ops domains
      x86/amd-iommu: Remove old page table handling macros
      x86/amd-iommu: Support higher level PTEs in iommu_page_unmap
      x86/amd-iommu: Change iommu_map_page to support multiple page sizes
      x86/dma: Mark iommu_pass_through as __read_mostly
      x86/amd-iommu: Add core functions for pd allocation/freeing
      x86/amd-iommu: Add passthrough mode initialization functions
      x86/amd-iommu: Fix device table write order
      x86/amd-iommu: Align locking between attach_device and detach_device
      x86/amd-iommu: Make sure a device is assigned in passthrough mode
      x86/amd-iommu: Don't detach device from pt domain on driver unbind
      x86/amd-iommu: Initialize passthrough mode when requested
      Merge branches 'gart/fixes', 'amd-iommu/fixes+cleanups' and 'amd-iommu/fault-handling' into amd-iommu/2.6.32
      Merge branch 'amd-iommu/passthrough' into amd-iommu/2.6.32
      Merge branch 'amd-iommu/pagetable' into amd-iommu/2.6.32

Pavel Vasilyev (1):
      x86/gart: Do not select AGP for GART_IOMMU

 arch/x86/Kconfig                       |    1 -
 arch/x86/include/asm/amd_iommu.h       |    1 +
 arch/x86/include/asm/amd_iommu_types.h |   50 ++--
 arch/x86/kernel/amd_iommu.c            |  489 +++++++++++++++++++++++---------
 arch/x86/kernel/amd_iommu_init.c       |   42 ++-
 arch/x86/kernel/pci-dma.c              |    9 +-
 6 files changed, 429 insertions(+), 163 deletions(-)

These changes include updates and improvements in the page table code,
in fault handling and they implement iommu=pt mode for AMD-Vi too.
Last but not least there are also some minor cleanups and fixes for
AMD-Vi and GART included in this pull-request. All these changes are
compile-, boot- and load-tested.

I had to do three merge commits for this pull request because the
amd-iommu/passthrough and amd-iommu/pagetable branches touch the same
code in some parts and the octopus merge of these branches failed. I had
to merge these two branches seperatly.
Please consider to pull.

Joerg

Re: [git pull] AMD-IOMMU (AMD-Vi) updates for 2.6.32

[Ingo Molnar]

* Joerg Roedel <joerg.roedel@amd.com> wrote:

> Hi Ingo,
> 
> The following changes since commit 37d0892c5a94e208cf863e3b7bac014edee4346d:
>   Ian Kent (1):
>         autofs4 - fix missed case when changing to use struct path
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/joro/linux-2.6-iommu.git amd-iommu/2.6.32
> 
> Joerg Roedel (37):
>       x86/amd-iommu: Dump fault entry on DTE error
>       x86/amd-iommu: Dump illegal command on ILLEGAL_COMMAND_ERROR
>       x86/amd-iommu: Introduce function for iommu-local domain flush
>       x86/amd-iommu: Remove some merge helper code
>       x86/amd-iommu: replace "AMD IOMMU" by "AMD-Vi"
>       x86/amd-iommu: Remove redundant 'IOMMU' string
>       x86/amd-iommu: fix broken check in amd_iommu_flush_all_devices
>       x86/amd-iommu: Add function to flush all DTEs on one IOMMU
>       x86/amd-iommu: Add reset function for command buffers
>       x86/amd-iommu: Reset command buffer on ILLEGAL_COMMAND_ERROR
>       x86/amd-iommu: Panic if IOMMU command buffer reset fails
>       x86/amd-iommu: Reset command buffer if wait loop fails
>       x86/amd-iommu: Make fetch_pte aware of dynamic mapping levels
>       x86/amd-iommu: Use fetch_pte in iommu_unmap_page
>       x86/amd-iommu: Use fetch_pte in amd_iommu_iova_to_phys
>       x86/amd-iommu: Add a gneric version of amd_iommu_flush_all_devices
>       x86/amd-iommu: Introduce set_dte_entry function
>       x86/amd-iommu: Flush domains if address space size was increased
>       x86/amd-iommu: Introduce increase_address_space function
>       x86/amd-iommu: Change alloc_pte to support 64 bit address space
>       x86/amd-iommu: Remove last usages of IOMMU_PTE_L0_INDEX
>       x86/amd-iommu: Remove bus_addr check in iommu_map_page
>       x86/amd-iommu: Use 2-level page tables for dma_ops domains
>       x86/amd-iommu: Remove old page table handling macros
>       x86/amd-iommu: Support higher level PTEs in iommu_page_unmap
>       x86/amd-iommu: Change iommu_map_page to support multiple page sizes
>       x86/dma: Mark iommu_pass_through as __read_mostly
>       x86/amd-iommu: Add core functions for pd allocation/freeing
>       x86/amd-iommu: Add passthrough mode initialization functions
>       x86/amd-iommu: Fix device table write order
>       x86/amd-iommu: Align locking between attach_device and detach_device
>       x86/amd-iommu: Make sure a device is assigned in passthrough mode
>       x86/amd-iommu: Don't detach device from pt domain on driver unbind
>       x86/amd-iommu: Initialize passthrough mode when requested
>       Merge branches 'gart/fixes', 'amd-iommu/fixes+cleanups' and 'amd-iommu/fault-handling' into amd-iommu/2.6.32
>       Merge branch 'amd-iommu/passthrough' into amd-iommu/2.6.32
>       Merge branch 'amd-iommu/pagetable' into amd-iommu/2.6.32
> 
> Pavel Vasilyev (1):
>       x86/gart: Do not select AGP for GART_IOMMU
> 
>  arch/x86/Kconfig                       |    1 -
>  arch/x86/include/asm/amd_iommu.h       |    1 +
>  arch/x86/include/asm/amd_iommu_types.h |   50 ++--
>  arch/x86/kernel/amd_iommu.c            |  489 +++++++++++++++++++++++---------
>  arch/x86/kernel/amd_iommu_init.c       |   42 ++-
>  arch/x86/kernel/pci-dma.c              |    9 +-
>  6 files changed, 429 insertions(+), 163 deletions(-)
> 
> These changes include updates and improvements in the page table code,
> in fault handling and they implement iommu=pt mode for AMD-Vi too.
> Last but not least there are also some minor cleanups and fixes for
> AMD-Vi and GART included in this pull-request. All these changes are
> compile-, boot- and load-tested.
> 
> I had to do three merge commits for this pull request because the
> amd-iommu/passthrough and amd-iommu/pagetable branches touch the same
> code in some parts and the octopus merge of these branches failed. I had
> to merge these two branches seperatly.
> Please consider to pull.

Pulled, thanks a lot Joerg!

A few (small) comments:

+static void dump_dte_entry(u16 devid)
+{
+       int i;
+
+       for (i = 0; i < 8; ++i)
+               pr_err("AMD-Vi: DTE[%d]: %08x\n", i,
+                       amd_iommu_dev_table[devid].data[i]);

that 8 is not very obvious - it deserves a comment. (we allocate at 
least one page to amd_iommu_dev_table[] so it's safe - but where the 
8 comes from is not very clear.)

Also, we tend to write 'i++' not '++i' in loops. (there's other 
examples of this too in the iommu files)

This log printing pattern:

        printk(KERN_ERR "AMD-Vi: Event logged [");

        switch (type) {
        case EVENT_TYPE_ILL_DEV:
                printk("ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
                       "address=0x%016llx flags=0x%04x]\n",

is now broken (produces an unexpected newline) due to:

  5fd29d6: printk: clean up handling of log-levels and newlines

you probably want to convert all printk's to pr_*() calls, and use 
pr_cont() in the above place.

Similar comments hold for dump_command() as well.

+++ b/arch/x86/include/asm/amd_iommu_types.h
@@ -457,4 +457,7 @@ static inline void amd_iommu_stats_init(void) { 
}
 
 #endif /* CONFIG_AMD_IOMMU_STATS */
 
+/* some function prototypes */
+extern void amd_iommu_reset_cmd_buffer(struct amd_iommu *iommu);
+

function prototypes belong into amd_iommu.h, not amd_iommu_types.h.

there's also a lot of function prototypes declared in the .c file:

+++ b/arch/x86/kernel/amd_iommu.c
@@ -61,6 +61,7 @@ static u64* alloc_pte(struct protection_domain 
*dom,
 static void dma_ops_reserve_addresses(struct dma_ops_domain *dom,
                                      unsigned long start_page,
                                      unsigned int pages);
+static void reset_iommu_command_buffer(struct amd_iommu *iommu);

These can generally be avoided via cleaner ordering: first define 
simpler functions then more complex ones (which rely on simpler 
functions).

+       if (unlikely(i == EXIT_LOOP_COUNT)) {
+               spin_unlock(&iommu->lock);
+               reset_iommu_command_buffer(iommu);
+               spin_lock(&iommu->lock);
+       }

What did iommu->lock protect it here, why do we drop it, and why is 
it necessary to take it again? This pattern shows that either 
there's too much or too little locking.

+       /* increase reference counter */
+       domain->dev_cnt += 1;

use ++?

+ * If a device is not yet associated with a domain, this function does
+ * assigns it visible for the hardware

typo.

+static void update_domain(struct protection_domain *domain)
+{
+       if (!domain->updated)
+               return;
+
+       update_device_table(domain);
+       flush_devices_by_domain(domain);
+       iommu_flush_domain(domain->id);
+
+       domain->updated = false;
+}

the domain->updated is a bit weird naming. Perhaps 
domain->update_needed?

+               iommu_unmap_page(domain, iova, PM_MAP_4k);

small nit: mixed-case letters - PM_MAP_4K is better i guess.

+#define PM_ALIGNED(lvl, addr)  ((PM_MAP_MASK(lvl) & (addr)) == (addr))

macro with two uses of its parameter - this is side-effect-unsafe. 
Safer would be to turn this into an inline function. That would also 
do proper type checking.

+       /* allocate passthroug domain */

typo.

+/*****************************************************************************
+ *
+ * The next functions do a basic initialization of IOMMU for pass through
+ * mode
+ *
+ * In passthrough mode the IOMMU is initialized and enabled but not used for
+ * DMA-API translation.
+ *
+ 
*****************************************************************************/

please use the customary (multi-line) comment style:

  /*
   * Comment .....
   * ...... goes here.
   */

specified in Documentation/CodingStyle.

+#define PD_PASSTHROUGH_MASK    (1UL << 2) /* domain has no page
+                                             translation */

ditto.

+       domain->dev_cnt += 1;

Could use '++' here too.

Thanks,

	Ingo

Re: [git pull] AMD-IOMMU (AMD-Vi) updates for 2.6.32

[Joerg Roedel]

Hi Ingo,

thanks for your comments.

On Fri, Sep 04, 2009 at 03:06:45PM +0200, Ingo Molnar wrote:
> +static void dump_dte_entry(u16 devid)
> +{
> +       int i;
> +
> +       for (i = 0; i < 8; ++i)
> +               pr_err("AMD-Vi: DTE[%d]: %08x\n", i,
> +                       amd_iommu_dev_table[devid].data[i]);
> 
> that 8 is not very obvious - it deserves a comment. (we allocate at 
> least one page to amd_iommu_dev_table[] so it's safe - but where the 
> 8 comes from is not very clear.)

Right. I will add a comment.

> Also, we tend to write 'i++' not '++i' in loops. (there's other 
> examples of this too in the iommu files)

Ok, I use ++i for historic reasons from my old c++ times ;-) Is there a
specific reason i++ is prefered?

> This log printing pattern:
> 
>         printk(KERN_ERR "AMD-Vi: Event logged [");
> 
>         switch (type) {
>         case EVENT_TYPE_ILL_DEV:
>                 printk("ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
>                        "address=0x%016llx flags=0x%04x]\n",
> 
> is now broken (produces an unexpected newline) due to:
> 
>   5fd29d6: printk: clean up handling of log-levels and newlines
> 
> you probably want to convert all printk's to pr_*() calls, and use 
> pr_cont() in the above place.

Ok, I will chance this too.

> Similar comments hold for dump_command() as well.
> 
> +++ b/arch/x86/include/asm/amd_iommu_types.h
> @@ -457,4 +457,7 @@ static inline void amd_iommu_stats_init(void) { 
> }
>  
>  #endif /* CONFIG_AMD_IOMMU_STATS */
>  
> +/* some function prototypes */
> +extern void amd_iommu_reset_cmd_buffer(struct amd_iommu *iommu);
> +
> 
> function prototypes belong into amd_iommu.h, not amd_iommu_types.h.

The plan is to do it the other way around. Currently amd_iommu.h
contains the driver exported function prototypes and the prototypes of
functions only shared between amd_iommu_init.c and amd_iommu.c.
So my plan is to move the prototypes of the functions only shared
between the two driver source files to amd_iommu_types.h.
The prototypes I put into source files should all be forward
declarations of static functions only. Should these be in header files
too?

> there's also a lot of function prototypes declared in the .c file:
> 
> +++ b/arch/x86/kernel/amd_iommu.c
> @@ -61,6 +61,7 @@ static u64* alloc_pte(struct protection_domain 
> *dom,
>  static void dma_ops_reserve_addresses(struct dma_ops_domain *dom,
>                                       unsigned long start_page,
>                                       unsigned int pages);
> +static void reset_iommu_command_buffer(struct amd_iommu *iommu);
> 
> These can generally be avoided via cleaner ordering: first define 
> simpler functions then more complex ones (which rely on simpler 
> functions).
> 
> +       if (unlikely(i == EXIT_LOOP_COUNT)) {
> +               spin_unlock(&iommu->lock);
> +               reset_iommu_command_buffer(iommu);
> +               spin_lock(&iommu->lock);
> +       }
> 
> What did iommu->lock protect it here, why do we drop it, and why is 
> it necessary to take it again? This pattern shows that either 
> there's too much or too little locking.

This function runs with the iommu->lock held. But
reset_iommu_command_buffer calls functions which take it again. To not
let this dead-lock the lock must be released before calling
reset_iommu_command_buffer. I agree this is not very clean design. The
whole command sending and flushing infrastructure in the driver has
somewhat grown into a little mess and one of my next updates will be to
clean that up to make it easier to maintain.

> 
> +       /* increase reference counter */
> +       domain->dev_cnt += 1;
> 
> use ++?

Hmm, In this case I would prefer the += variant because its a single
instruction and not part of an expression. The compiler should optimize
it to the same instruction as a ++ variant.
I try to use the ++ variant when the result is part of another
expression.

> + * If a device is not yet associated with a domain, this function does
> + * assigns it visible for the hardware
> 
> typo.

Thanks, will fix.
> 
> +static void update_domain(struct protection_domain *domain)
> +{
> +       if (!domain->updated)
> +               return;
> +
> +       update_device_table(domain);
> +       flush_devices_by_domain(domain);
> +       iommu_flush_domain(domain->id);
> +
> +       domain->updated = false;
> +}
> 
> the domain->updated is a bit weird naming. Perhaps 
> domain->update_needed?

update_needed is a bit misleading. The domain _is_ already updated but
we need to inform the IOMMU hardware about it and flush TLBs and device
table entries for that domain. So 'updated' is the better choice here I
think. Or I make it more specific and call it pt_updated to make clear
that the page table changed (such a change means that the pt root
pointer changed).

> 
> +               iommu_unmap_page(domain, iova, PM_MAP_4k);
> 
> small nit: mixed-case letters - PM_MAP_4K is better i guess.

Thanks, will change that.

> +#define PM_ALIGNED(lvl, addr)  ((PM_MAP_MASK(lvl) & (addr)) == (addr))
> 
> macro with two uses of its parameter - this is side-effect-unsafe. 
> Safer would be to turn this into an inline function. That would also 
> do proper type checking.

Ok, I change that too.

> +       /* allocate passthroug domain */
> 
> typo.

Will fix, thanks.

> +/*****************************************************************************
> + *
> + * The next functions do a basic initialization of IOMMU for pass through
> + * mode
> + *
> + * In passthrough mode the IOMMU is initialized and enabled but not used for
> + * DMA-API translation.
> + *
> + 
> *****************************************************************************/
> 
> please use the customary (multi-line) comment style:
> 
>   /*
>    * Comment .....
>    * ...... goes here.
>    */
> 
> specified in Documentation/CodingStyle.
> 
> +#define PD_PASSTHROUGH_MASK    (1UL << 2) /* domain has no page
> +                                             translation */

Right, I will fix this too.

Thanks,
	
	Joerg

Re: [git pull] AMD-IOMMU (AMD-Vi) updates for 2.6.32

[Ingo Molnar]

* Joerg Roedel <joerg.roedel@amd.com> wrote:

> Hi Ingo,
> 
> thanks for your comments.
> 
> On Fri, Sep 04, 2009 at 03:06:45PM +0200, Ingo Molnar wrote:
> > +static void dump_dte_entry(u16 devid)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < 8; ++i)
> > +               pr_err("AMD-Vi: DTE[%d]: %08x\n", i,
> > +                       amd_iommu_dev_table[devid].data[i]);
> > 
> > that 8 is not very obvious - it deserves a comment. (we allocate at 
> > least one page to amd_iommu_dev_table[] so it's safe - but where the 
> > 8 comes from is not very clear.)
> 
> Right. I will add a comment.
> 
> > Also, we tend to write 'i++' not '++i' in loops. (there's other 
> > examples of this too in the iommu files)
> 
> Ok, I use ++i for historic reasons from my old c++ times ;-) Is there a
> specific reason i++ is prefered?
> 
> > This log printing pattern:
> > 
> >         printk(KERN_ERR "AMD-Vi: Event logged [");
> > 
> >         switch (type) {
> >         case EVENT_TYPE_ILL_DEV:
> >                 printk("ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
> >                        "address=0x%016llx flags=0x%04x]\n",
> > 
> > is now broken (produces an unexpected newline) due to:
> > 
> >   5fd29d6: printk: clean up handling of log-levels and newlines
> > 
> > you probably want to convert all printk's to pr_*() calls, and use 
> > pr_cont() in the above place.
> 
> Ok, I will chance this too.
> 
> > Similar comments hold for dump_command() as well.
> > 
> > +++ b/arch/x86/include/asm/amd_iommu_types.h
> > @@ -457,4 +457,7 @@ static inline void amd_iommu_stats_init(void) { 
> > }
> >  
> >  #endif /* CONFIG_AMD_IOMMU_STATS */
> >  
> > +/* some function prototypes */
> > +extern void amd_iommu_reset_cmd_buffer(struct amd_iommu *iommu);
> > +
> > 
> > function prototypes belong into amd_iommu.h, not amd_iommu_types.h.
> 
> The plan is to do it the other way around. Currently amd_iommu.h 
> contains the driver exported function prototypes and the 
> prototypes of functions only shared between amd_iommu_init.c and 
> amd_iommu.c. So my plan is to move the prototypes of the functions 
> only shared between the two driver source files to 
> amd_iommu_types.h. The prototypes I put into source files should 
> all be forward declarations of static functions only. Should these 
> be in header files too?

Well, generally we only put data type definitions and constants into 
*_types.h files. If you need multiple include files for methods i'd 
suggest to split amd_iomm.h into two or so.

	Ingo

Re: [git pull] AMD-IOMMU (AMD-Vi) updates for 2.6.32

[Joerg Roedel]

On Fri, Sep 04, 2009 at 04:11:13PM +0200, Ingo Molnar wrote:
> 
> * Joerg Roedel <joerg.roedel@amd.com> wrote:
> 
> > Hi Ingo,
> > 
> > thanks for your comments.
> > 
> > On Fri, Sep 04, 2009 at 03:06:45PM +0200, Ingo Molnar wrote:
> > > +static void dump_dte_entry(u16 devid)
> > > +{
> > > +       int i;
> > > +
> > > +       for (i = 0; i < 8; ++i)
> > > +               pr_err("AMD-Vi: DTE[%d]: %08x\n", i,
> > > +                       amd_iommu_dev_table[devid].data[i]);
> > > 
> > > that 8 is not very obvious - it deserves a comment. (we allocate at 
> > > least one page to amd_iommu_dev_table[] so it's safe - but where the 
> > > 8 comes from is not very clear.)
> > 
> > Right. I will add a comment.
> > 
> > > Also, we tend to write 'i++' not '++i' in loops. (there's other 
> > > examples of this too in the iommu files)
> > 
> > Ok, I use ++i for historic reasons from my old c++ times ;-) Is there a
> > specific reason i++ is prefered?
> > 
> > > This log printing pattern:
> > > 
> > >         printk(KERN_ERR "AMD-Vi: Event logged [");
> > > 
> > >         switch (type) {
> > >         case EVENT_TYPE_ILL_DEV:
> > >                 printk("ILLEGAL_DEV_TABLE_ENTRY device=%02x:%02x.%x "
> > >                        "address=0x%016llx flags=0x%04x]\n",
> > > 
> > > is now broken (produces an unexpected newline) due to:
> > > 
> > >   5fd29d6: printk: clean up handling of log-levels and newlines
> > > 
> > > you probably want to convert all printk's to pr_*() calls, and use 
> > > pr_cont() in the above place.
> > 
> > Ok, I will chance this too.
> > 
> > > Similar comments hold for dump_command() as well.
> > > 
> > > +++ b/arch/x86/include/asm/amd_iommu_types.h
> > > @@ -457,4 +457,7 @@ static inline void amd_iommu_stats_init(void) { 
> > > }
> > >  
> > >  #endif /* CONFIG_AMD_IOMMU_STATS */
> > >  
> > > +/* some function prototypes */
> > > +extern void amd_iommu_reset_cmd_buffer(struct amd_iommu *iommu);
> > > +
> > > 
> > > function prototypes belong into amd_iommu.h, not amd_iommu_types.h.
> > 
> > The plan is to do it the other way around. Currently amd_iommu.h 
> > contains the driver exported function prototypes and the 
> > prototypes of functions only shared between amd_iommu_init.c and 
> > amd_iommu.c. So my plan is to move the prototypes of the functions 
> > only shared between the two driver source files to 
> > amd_iommu_types.h. The prototypes I put into source files should 
> > all be forward declarations of static functions only. Should these 
> > be in header files too?
> 
> Well, generally we only put data type definitions and constants into 
> *_types.h files. If you need multiple include files for methods i'd 
> suggest to split amd_iomm.h into two or so.

Ok, right, its better to split amd_iommu.h here then.

	Joerg