Re: [PATCH 2/3] AMD IOMMU support

[Eduard - Gabriel Munteanu <eduard.munteanu@linux360.ro>]

On Sun, Feb 06, 2011 at 01:47:57PM +0200, Michael S. Tsirkin wrote:
> On Fri, Feb 04, 2011 at 01:24:14AM +0200, Eduard - Gabriel Munteanu wrote:

Hi,

[snip]

> > +/*
> > + * IVRS (I/O Virtualization Reporting Structure) table.
> > + *
> > + * Describes the AMD IOMMU, as per:
> > + * "AMD I/O Virtualization Technology (IOMMU) Specification", rev 1.26
> > + */
> > +
> > +struct ivrs_ivhd
> > +{
> > +    u8    type;
> > +    u8    flags;
> > +    u16   length;
> > +    u16   devid;
> > +    u16   capab_off;
> > +    u32   iommu_base_low;
> > +    u32   iommu_base_high;
> > +    u16   pci_seg_group;
> > +    u16   iommu_info;
> > +    u32   reserved;
> > +    u8    entry[0];
> > +} PACKED;
> > +
> > +struct ivrs_table
> > +{
> > +    ACPI_TABLE_HEADER_DEF    /* ACPI common table header. */
> > +    u32                iv_info;
> > +    u32                reserved[2];
> > +    struct ivrs_ivhd   ivhd;
> > +} PACKED;
> > +
> 
> prefix with amd_iommu_ or amd_ then ?
>

This should be standard nomenclature already, even if IVRS is AMD
IOMMU-specific.

> >  #include "acpi-dsdt.hex"
> >  
> >  static void
> > @@ -579,6 +609,59 @@ build_srat(void)
> >      return srat;
> >  }
> >  
> > +#define IVRS_SIGNATURE 0x53525649 // IVRS
> > +#define IVRS_MAX_DEVS  32
> > +static void *
> > +build_ivrs(void)
> > +{
> > +    int iommu_bdf, iommu_cap;
> > +    int bdf, max, i;
> > +    struct ivrs_table *ivrs;
> > +    struct ivrs_ivhd *ivhd;
> > +
> > +    /* Note this currently works for a single IOMMU! */
> 
> Meant to be a FIXME?
> How hard is it to fix? Just stick this in a loop?
>

I suspect a real BIOS would have these values hardcoded anyway,
according to the topology of the PCI bus and which IOMMUs sit where. You
already mentioned the possibility of multiple IOMMU capabilities in the
same function/bus, in which case there's probably no easy way to guess
it from SeaBIOS.

[snip]

> > +static void amd_iommu_init(u16 bdf, void *arg)
> > +{
> > +    int cap;
> > +    u32 base_addr;
> > +
> > +    cap = pci_find_capability(bdf, PCI_CAP_ID_SEC);
> > +    if (cap < 0) {
> > +        return;
> > +    }
> 
> There actually can be multiple instances of this
> capability according to spec.
> Do we care?
> 

Hm, perhaps we should at least assign a base address there, that's easy.
As for QEMU/KVM usage we probably don't need it. 

> > +
> > +    if (amd_iommu_addr >= BUILD_AMD_IOMMU_END) {
> > +        return;
> > +    }
> > +    base_addr = amd_iommu_addr;
> > +    amd_iommu_addr += 0x4000;
> > +
> > +    pci_config_writel(bdf, cap + 0x0C, 0);
> > +    pci_config_writel(bdf, cap + 0x08, 0);
> > +    pci_config_writel(bdf, cap + 0x04, base_addr | 1);
> > +}
> > +
> >  static const struct pci_device_id pci_class_tbl[] = {
> >      /* STORAGE IDE */
> >      PCI_DEVICE_CLASS(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371SB_1,
> > @@ -279,6 +302,10 @@ static const struct pci_device_id pci_class_tbl[] = {
> >      PCI_DEVICE_CLASS(PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_BRIDGE_PCI,
> >                       pci_bios_init_device_bridge),
> >  
> > +    /* AMD IOMMU */
> 
> Makes sense to limit to AMD vendor id?
> 

I don't think so, I assume any PCI_CLASS_SYSTEM_IOMMU device would
implement the same specification, considering these ids have been
assigned by PCI-SIG.

> > +    PCI_DEVICE_CLASS(PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_SYSTEM_IOMMU,
> > +                     amd_iommu_init),
> > +
> >      /* default */
> >      PCI_DEVICE(PCI_ANY_ID, PCI_ANY_ID, pci_bios_allocate_regions),
> >  
> > @@ -408,6 +435,8 @@ pci_setup(void)
> >      pci_region_init(&pci_bios_prefmem_region,
> >                      BUILD_PCIPREFMEM_START, BUILD_PCIPREFMEM_END - 1);
> >  
> > +    amd_iommu_addr = BUILD_AMD_IOMMU_START;
> > +
> >      pci_bios_init_bus();
> >  
> >      int bdf, max;
> > -- 
> > 1.7.3.4

Thanks for your review, I read your other comments and will resubmit
once I fix those issues.


	Eduard