From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eduard - Gabriel Munteanu Subject: Re: [PATCH 2/3] AMD IOMMU support Date: Sun, 6 Feb 2011 15:41:45 +0200 Message-ID: <20110206134145.GA10279@localhost> References: <20110206114757.GF26242@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: seabios@seabios.org, kevin@koconnor.net, joro@8bytes.org, blauwirbel@gmail.com, paul@codesourcery.com, avi@redhat.com, anthony@codemonkey.ws, av1474@comtv.ru, yamahata@valinux.co.jp, kvm@vger.kernel.org, qemu-devel@nongnu.org To: "Michael S. Tsirkin" Return-path: Received: from mail-fx0-f46.google.com ([209.85.161.46]:55943 "EHLO mail-fx0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752452Ab1BFNlv (ORCPT ); Sun, 6 Feb 2011 08:41:51 -0500 Received: by fxm20 with SMTP id 20so4000829fxm.19 for ; Sun, 06 Feb 2011 05:41:50 -0800 (PST) Content-Disposition: inline In-Reply-To: <20110206114757.GF26242@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=41244 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Pm4sC-0005A5-UU for qemu-devel@nongnu.org; Sun, 06 Feb 2011 08:41:53 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Pm4sB-0000mW-Km for qemu-devel@nongnu.org; Sun, 06 Feb 2011 08:41:52 -0500 Received: from mail-fx0-f45.google.com ([209.85.161.45]:57040) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Pm4sB-0000mG-7W for qemu-devel@nongnu.org; Sun, 06 Feb 2011 08:41:51 -0500 Received: by fxm12 with SMTP id 12so4097641fxm.4 for ; Sun, 06 Feb 2011 05:41:50 -0800 (PST) Sender: Eduard - Gabriel Munteanu Date: Sun, 6 Feb 2011 15:41:45 +0200 From: Eduard - Gabriel Munteanu Message-ID: <20110206134145.GA10279@localhost> References: <20110206114757.GF26242@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20110206114757.GF26242@redhat.com> Subject: [Qemu-devel] Re: [PATCH 2/3] AMD IOMMU support List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: kvm@vger.kernel.org, joro@8bytes.org, seabios@seabios.org, qemu-devel@nongnu.org, blauwirbel@gmail.com, yamahata@valinux.co.jp, kevin@koconnor.net, avi@redhat.com, paul@codesourcery.com 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