| Message ID | 1250755318.8974.26.camel@macbook.infradead.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, 20 Aug 2009 09:01:58 +0100 David Woodhouse <dwmw2@infradead.org> wrote: > + if (iommu->cap == (uint64_t)-1 && iommu->ecap == (uint64_t)-1) { > + /* Promote an attitude of violence to a BIOS engineer today */ > + WARN(1, "Your BIOS is broken; DMAR reported at address %llx returns all ones!\n" > + "BIOS vendor: %s; Ver: %s; Product Version: %s\n", > + drhd->reg_base_addr, Printing a u64 with %ll will (still) generate a warning on four architectures. > + dmi_get_system_info(DMI_BIOS_VENDOR), > + dmi_get_system_info(DMI_BIOS_VERSION), > + dmi_get_system_info(DMI_PRODUCT_VERSION)); > + goto err_unmap; > + } -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2009-08-20 at 02:44 -0700, Andrew Morton wrote: > Printing a u64 with %ll will (still) generate a warning on four > architectures. Still that many? I thought we'd fixed them all already. Certainly we fixed IA64, which is the last one I cared about in the context of this particular code.
On Thu, Aug 20, 2009 at 02:44:53AM -0700, Andrew Morton wrote: > On Thu, 20 Aug 2009 09:01:58 +0100 David Woodhouse <dwmw2@infradead.org> wrote: > > > + if (iommu->cap == (uint64_t)-1 && iommu->ecap == (uint64_t)-1) { > > + /* Promote an attitude of violence to a BIOS engineer today */ > > + WARN(1, "Your BIOS is broken; DMAR reported at address %llx returns all ones!\n" > > + "BIOS vendor: %s; Ver: %s; Product Version: %s\n", > > + drhd->reg_base_addr, > > Printing a u64 with %ll will (still) generate a warning on four architectures. We've got them all now. $ grep -l int-l64 arch/*/include/asm/types.h arch/alpha/include/asm/types.h arch/ia64/include/asm/types.h arch/mips/include/asm/types.h arch/powerpc/include/asm/types.h $ grep -l int-ll64 $(grep -l int-l64 arch/*/include/asm/types.h) arch/alpha/include/asm/types.h arch/ia64/include/asm/types.h arch/mips/include/asm/types.h arch/powerpc/include/asm/types.h ie all architectures which use int-l64 only do so for the benefit of userspace, and use int-ll64 within the kernel. I did check this by hand too ;-)
On Thu, 20 Aug 2009 12:14:37 +0100 David Woodhouse <dwmw2@infradead.org> wrote: > On Thu, 2009-08-20 at 02:44 -0700, Andrew Morton wrote: > > Printing a u64 with %ll will (still) generate a warning on four > > architectures. > > Still that many? I thought we'd fixed them all already. You're right, they all seem to be fixed. Nobody tells me nuttin. Whee. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 20, 2009 at 1:01 AM, David Woodhouse <dwmw2@infradead.org> wrote: > + Â Â Â if (iommu->cap == (uint64_t)-1 && iommu->ecap == (uint64_t)-1) { > + Â Â Â Â Â Â Â /* Promote an attitude of violence to a BIOS engineer today */ > + Â Â Â Â Â Â Â WARN(1, "Your BIOS is broken; DMAR reported at address %llx returns all ones!\n" Think about changing this to a warning that "Your IOMMU appears to be disabled." All ones is, after all, the traditional hint that the device is turned off. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2009-08-20 at 12:29 -0700, Ray Lee wrote: > Think about changing this to a warning that "Your IOMMU appears to be > disabled." All ones is, after all, the traditional hint that the > device is turned off. Hints are all very well, but the BIOS provided an ACPI table explicitly telling us that there was an active IOMMU at this location.
On Thu, Aug 20, 2009 at 08:32:20PM +0100, David Woodhouse wrote: > On Thu, 2009-08-20 at 12:29 -0700, Ray Lee wrote: > > Think about changing this to a warning that "Your IOMMU appears to be > > disabled." All ones is, after all, the traditional hint that the > > device is turned off. > > Hints are all very well, but the BIOS provided an ACPI table explicitly > telling us that there was an active IOMMU at this location. Could that be to reserve address space that the "disabled" IOMMU might still be responding to? Ie the BIOS hides the control registers so the OS won't talk to the device but the IOMMU might still attempt to lookup certain address ranges. I'm more inclined to believe it's sloppiness on the part of the BIOS writers but thought this might be an alternative explanation. thanks, grant -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2009-08-20 at 15:47 -0600, Grant Grundler wrote: > > Could that be to reserve address space that the "disabled" IOMMU > might still be responding to? > > Ie the BIOS hides the control registers so the OS won't talk to the > device but the IOMMU might still attempt to lookup certain address > ranges. > > I'm more inclined to believe it's sloppiness on the part of the BIOS > writers but thought this might be an alternative explanation. Nah, this is just the normal story: "We smoked too much crack and fried our brains, and we don't do any QA on the crap we write because that would leave fewer hours in the day for us to service our crack habit". We _really_ need open source firmware. Or at _least_ firmware written by competent engineers -- but I think we've all fairly much given up on that happening by now?
On 20.08.2009 23:54, David Woodhouse wrote: > On Thu, 2009-08-20 at 15:47 -0600, Grant Grundler wrote: > >> I'm more inclined to believe it's sloppiness on the part of the BIOS >> writers but thought this might be an alternative explanation. >> > > Nah, this is just the normal story: "We smoked too much crack and fried > our brains, and we don't do any QA on the crap we write because that > would leave fewer hours in the day for us to service our crack habit". > > We _really_ need open source firmware. > > Or at _least_ firmware written by competent engineers -- but I think > we've all fairly much given up on that happening by now? > coreboot is an open source (GPLv2) x86 lightweight firmware written in C (with optional BIOS and EFI compatibility modules if anyone cares). While it does not support all current x86 chipsets, it still does support quite a few mainboards. The developers are friendly and try to fix any reported bugs. There are two big obstacles until x86 world domination, though: A dozen core developers are simply not enough for the huge task (there are more open specs than developers can keep up with), and some chipset vendors don't open their specs. Anyone interested is invited to contribute and make the world a better place. Oh, and it supports packing a Linux kernel in the mainboard flash ROM for diskless/networkless booting (that coreboot+Linux combination was called LinuxBIOS some time ago). More information at http://www.coreboot.org/ Regards, Carl-Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/dmar.c b/drivers/pci/dmar.c index 7b287cb..380b60e 100644 --- a/drivers/pci/dmar.c +++ b/drivers/pci/dmar.c @@ -632,20 +632,31 @@ int alloc_iommu(struct dmar_drhd_unit *drhd) iommu->cap = dmar_readq(iommu->reg + DMAR_CAP_REG); iommu->ecap = dmar_readq(iommu->reg + DMAR_ECAP_REG); + if (iommu->cap == (uint64_t)-1 && iommu->ecap == (uint64_t)-1) { + /* Promote an attitude of violence to a BIOS engineer today */ + WARN(1, "Your BIOS is broken; DMAR reported at address %llx returns all ones!\n" + "BIOS vendor: %s; Ver: %s; Product Version: %s\n", + drhd->reg_base_addr, + dmi_get_system_info(DMI_BIOS_VENDOR), + dmi_get_system_info(DMI_BIOS_VERSION), + dmi_get_system_info(DMI_PRODUCT_VERSION)); + goto err_unmap; + } + #ifdef CONFIG_DMAR agaw = iommu_calculate_agaw(iommu); if (agaw < 0) { printk(KERN_ERR "Cannot get a valid agaw for iommu (seq_id = %d)\n", iommu->seq_id); - goto error; + goto err_unmap; } msagaw = iommu_calculate_max_sagaw(iommu); if (msagaw < 0) { printk(KERN_ERR "Cannot get a valid max agaw for iommu (seq_id = %d)\n", iommu->seq_id); - goto error; + goto err_unmap; } #endif iommu->agaw = agaw; @@ -665,7 +676,7 @@ int alloc_iommu(struct dmar_drhd_unit *drhd) } ver = readl(iommu->reg + DMAR_VER_REG); - pr_debug("IOMMU %llx: ver %d:%d cap %llx ecap %llx\n", + pr_info("IOMMU %llx: ver %d:%d cap %llx ecap %llx\n", (unsigned long long)drhd->reg_base_addr, DMAR_VER_MAJOR(ver), DMAR_VER_MINOR(ver), (unsigned long long)iommu->cap, @@ -675,7 +686,10 @@ int alloc_iommu(struct dmar_drhd_unit *drhd) drhd->iommu = iommu; return 0; -error: + + err_unmap: + iounmap(iommu->reg); + error: kfree(iommu); return -1; }
Yet another reason why trusting this stuff to the BIOS was a bad idea. There now seem to be a bunch of BIOSes which report an IOMMU at a physical address which just returns all ones. (Perhaps only when VT-d is actually _disabled_ in the BIOS?) Well done, Dell and HP -- although I didn't think it was possible, you have _further_ lowered my already-unprintable opinion of closed source BIOSes and BIOS engineers. This patch makes the kernel detect this particularly brokenness and abort early -- and fixes up the missing iounmap in the error paths which I noticed while I was poking at it. This should fix kernel.org bug #14003, which was being called a 'regression' -- I think because the IOMMU code used to trip over _another_ BIOS bug earlier than this one, and that one _did_ cause it to abort. Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> --- drivers/pci/dmar.c | 22 ++++++++++++++++++---- 1 files changed, 18 insertions(+), 4 deletions(-)