Re: [SeaBIOS] [PATCH 2/3] AMD IOMMU support

[Michael S. Tsirkin]

On Fri, Feb 04, 2011 at 01:24:14AM +0200, Eduard - Gabriel Munteanu wrote:
> This initializes the AMD IOMMU and creates ACPI tables for it.
> Signed-off-by: Eduard - Gabriel Munteanu
>
> src/acpi.c     |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/config.h   |    3 ++
>  src/pci_ids.h  |    1 +
>  src/pci_regs.h |    1 +
>  src/pciinit.c  |   29 +++++++++++++++++++
>  5 files changed, 118 insertions(+), 0 deletions(-)
>
> diff --git a/src/acpi.c b/src/acpi.c
> index 18830dc..fca152c 100644
> --- a/src/acpi.c
> +++ b/src/acpi.c
> @@ -196,6 +196,36 @@ struct srat_memory_affinity
>      u32    reserved3[2];
>  } PACKED;
>
> +/*
> 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 ?
> #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?
> iommu_bdf = pci_find_class(PCI_CLASS_SYSTEM_IOMMU);
> if (iommu_bdf < 0)
>    return NULL;
> iommu_cap = pci_find_capability(iommu_bdf, PCI_CAP_ID_SEC);
> if (iommu_cap < 0)
>    return NULL;
> ivrs = malloc_high(sizeof(struct ivrs_table) + 4 * IVRS_MAX_DEVS);
> ivrs->iv_info = pci_config_readw(iommu_bdf, iommu_cap + 0x12) & ~0x000F;
> ivhd = &ivrs->ivhd;
> ivhd->type              = 0x10;
> ivhd->flags             = 0;
> ivhd->length            = sizeof(struct ivrs_ivhd);
> ivhd->devid             = iommu_bdf;
> ivhd->capab_off         = iommu_cap;
> ivhd->iommu_base_low    = pci_config_readl(iommu_bdf, iommu_cap + 0x04) &
>                          0xFFFFFFFE;
> ivhd->iommu_base_high   = pci_config_readl(iommu_bdf, iommu_cap + 0x08);
> ivhd->pci_seg_group     = 0;
> ivhd->iommu_info        = 0;
> ivhd->reserved          = 0;
> i = 0;
> foreachpci(bdf, max) {
>    if (bdf == ivhd->devid)
>        continue;
>    ivhd->entry[4 * i + 0] = 2;
>    ivhd->entry[4 * i + 1] = bdf & 0xFF;
>    ivhd->entry[4 * i + 2] = (bdf >> 8) & 0xFF;
>    ivhd->entry[4 * i + 3] = ~(1 << 3);
>    ivhd->length += 4;
>    if (++i >= IVRS_MAX_DEVS)
>        break;
> }
> build_header((void *) ivrs, IVRS_SIGNATURE,
>             sizeof(struct ivrs_table) + 4 * i, 1);
> return ivrs;
>
> +}
>
> static const struct pci_device_id acpi_find_tbl[] = {
>      /* PIIX4 Power Management device. */
>      PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3, NULL),
> @@ -625,6 +708,7 @@ acpi_bios_init(void)
>      ACPI_INIT_TABLE(build_madt());
>      ACPI_INIT_TABLE(build_hpet());
>      ACPI_INIT_TABLE(build_srat());
>
> ACPI_INIT_TABLE(build_ivrs());
>
> u16 i, external_tables = qemu_cfg_acpi_additional_tables();
>
> diff --git a/src/config.h b/src/config.h
> index 6356941..0ba5723 100644
> --- a/src/config.h
> +++ b/src/config.h
> @@ -172,6 +172,9 @@
>  #define BUILD_APIC_ADDR           0xfee00000
>  #define BUILD_IOAPIC_ADDR         0xfec00000
>
> +#define BUILD_AMD_IOMMU_START     0xfed00000
> +#define BUILD_AMD_IOMMU_END       0xfee00000    /* BUILD_APIC_ADDR */
>
> #define BUILD_SMM_INIT_ADDR       0x38000
>  #define BUILD_SMM_ADDR            0xa8000
>  #define BUILD_SMM_SIZE            0x8000
> diff --git a/src/pci_ids.h b/src/pci_ids.h
> index e1cded2..3cc3c6e 100644
> --- a/src/pci_ids.h
> +++ b/src/pci_ids.h
> @@ -72,6 +72,7 @@
>  #define PCI_CLASS_SYSTEM_RTC		0x0803
>  #define PCI_CLASS_SYSTEM_PCI_HOTPLUG	0x0804
>  #define PCI_CLASS_SYSTEM_SDHCI		0x0805
> +#define PCI_CLASS_SYSTEM_IOMMU		0x0806
>  #define PCI_CLASS_SYSTEM_OTHER		0x0880
>
> #define PCI_BASE_CLASS_INPUT		0x09
> diff --git a/src/pci_regs.h b/src/pci_regs.h
> index e5effd4..bfac824 100644
> --- a/src/pci_regs.h
> +++ b/src/pci_regs.h
> @@ -208,6 +208,7 @@
>  #define  PCI_CAP_ID_SHPC 	0x0C	/* PCI Standard Hot-Plug Controller */
>  #define  PCI_CAP_ID_SSVID	0x0D	/* Bridge subsystem vendor/device ID */
>  #define  PCI_CAP_ID_AGP3	0x0E	/* AGP Target PCI-PCI bridge */
> +#define  PCI_CAP_ID_SEC		0x0F	/* Secure Device (AMD IOMMU) */
>  #define  PCI_CAP_ID_EXP 	0x10	/* PCI Express */
>  #define  PCI_CAP_ID_MSIX	0x11	/* MSI-X */
>  #define PCI_CAP_LIST_NEXT	1	/* Next capability in the list */
> diff --git a/src/pciinit.c b/src/pciinit.c
> index ee2e72d..4ebcfbe 100644
> --- a/src/pciinit.c
> +++ b/src/pciinit.c
> @@ -21,6 +21,8 @@ static struct pci_region pci_bios_io_region;
>  static struct pci_region pci_bios_mem_region;
>  static struct pci_region pci_bios_prefmem_region;
>
> +static u32 amd_iommu_addr;
>
> /* host irqs corresponding to PCI irqs A-D */
>  const u8 pci_irqs[4] = {
>      10, 10, 11, 11
> @@ -256,6 +258,27 @@ static void apple_macio_init(u16 bdf, void *arg)
>      pci_set_io_region_addr(bdf, 0, 0x80800000);
>  }
>
> +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?
> 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?
> 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;

[PATCH 2/3] AMD IOMMU support

[Eduard - Gabriel Munteanu]

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;

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

Eduard

[PATCH 2/3] AMD IOMMU support

[Michael S. Tsirkin]

On Sun, Feb 06, 2011 at 03:41:45PM +0200, Eduard - Gabriel Munteanu wrote:
> 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.

Yes but the specific structure is amd specific, isn't it?
> > > #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.

Which values exactly?
> 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.

It's easy enough to enumerate capabilities and pci devices, isn't it?
> [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.

I expect assigning multiple domains will be useful.
I'm guessing multiple devices is what systems have
in this case? If so I'm not really sure why is there need
for multiple iommu capabilities per device.
> > > 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.

This hasn't been the case in the past, e.g. with
PCI_CLASS_NETWORK_ETHERNET, so I see no reason to assume
it here.
> > > 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;
>
> Thanks for your review, I read your other comments and will resubmit
> once I fix those issues.
>
> Eduard