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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
