Re: [Intel IOMMU][patch 3/8] Generic hardware support for Intel IOMMU.

[Ashok Raj <ashok.raj@intel.com>]

On Tue, Apr 24, 2007 at 09:27:08PM +0200, Andi Kleen wrote:
> On Tuesday 24 April 2007 08:03:02 Ashok Raj wrote:
> >
> > +#ifdef CONFIG_DMAR
> > +#ifdef CONFIG_SMP
> > +static void dmar_msi_set_affinity(unsigned int irq, cpumask_t mask)
> 
> 
> Why does it need an own interrupt type?

Problem is its MSI type interrupt, but we cannot use pci_dev since its not
a PCI device, hence requires its own way of setup etc.

> 
> > +
> > +config IOVA_GUARD_PAGE
> > +	bool "Enables gaurd page when allocating IO Virtual Address for IOMMU"
> > +	depends on DMAR
> > +
> > +config IOVA_NEXT_CONTIG
> > +	bool "Keeps IOVA allocations consequent between allocations"
> > +	depends on DMAR && EXPERIMENTAL
> 
> Needs reference to Intel and better description
> 
> The file should have a high level description what it is good for etc.
> 
> Need high level overview over what locks protects what and if there
> is a locking order.
> 
> It doesn't seem to enable sg merging? Since you have enough space 
> that should work.

Most of the IOVA stuff is really generic, and could be used outside
of the Intel code with probably some rework. Since today only DMAR 
requires it we have depends on DMAR, but we could make it more generic
and let the IOMMU driver just turn it on as required.

> 
> > +static char *fault_reason_strings[] =
> > +{
> > +	"Software",
> > +	"Present bit in root entry is clear",
> > +	"Present bit in context entry is clear",
> > +	"Invalid context entry",
> > +	"Access beyond MGAW",
> > +	"PTE Write access is not set",
> > +	"PTE Read access is not set",
> > +	"Next page table ptr is invalid",
> > +	"Root table address invalid",
> > +	"Context table ptr is invalid",
> > +	"non-zero reserved fields in RTP",
> > +	"non-zero reserved fields in CTP",
> > +	"non-zero reserved fields in PTE",
> > +	"Unknown"
> > +};
> > +
> > +#define MAX_FAULT_REASON_IDX 	(12)
> 
> 
> You got 14 of them. better use ARRAY_SIZE

Its the last index(zero based) of the useful entry returned by the fault record.

Only used to find out if index from fault record is out of bounds.

We will work on the remaining comments and repost.