xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] VT-d: improve RMRR validity checking
@ 2010-01-21  2:46 Han, Weidong
  2010-01-21  8:25 ` Noboru Iwamatsu
  0 siblings, 1 reply; 77+ messages in thread
From: Han, Weidong @ 2010-01-21  2:46 UTC (permalink / raw)
  To: xen-devel@lists.xensource.com; +Cc: Keir Fraser

[-- Attachment #1: Type: text/plain, Size: 580 bytes --]

Currently, Xen checks RMRR range and disables VT-d if RMRR range is set incorrectly in BIOS rigorously. But, actually we can ignore the RMRR if the device under its scope are not pci discoverable, because the RMRR won't be used by non-existed or disabled devices. 

This patch ignores the RMRR if the device under its scope are not pci discoverable, and only checks the validity of RMRRs that are actually used. In order to avoid duplicate pci device detection code, this patch defines a function pci_device_detect for it.

Signed-off-by: Weidong Han <weidong.han@intel.com>

[-- Attachment #2: rmrr.patch --]
[-- Type: application/octet-stream, Size: 4672 bytes --]

diff -r 104110651ff6 xen/drivers/passthrough/pci.c
--- a/xen/drivers/passthrough/pci.c	Wed Jan 20 22:50:09 2010 +0800
+++ b/xen/drivers/passthrough/pci.c	Thu Jan 21 10:17:52 2010 +0800
@@ -362,6 +362,21 @@ out:
 }
 
 /*
+ * detect pci device, return 0 if it exists, or return 0
+ */
+int pci_device_detect(u8 bus, u8 dev, u8 func)
+{
+    u32 vendor;
+
+    vendor = pci_conf_read32(bus, dev, func, PCI_VENDOR_ID);
+    /* some broken boards return 0 or ~0 if a slot is empty: */
+    if ( (vendor == 0xffffffff) || (vendor == 0x00000000) ||
+         (vendor == 0x0000ffff) || (vendor == 0xffff0000) )
+        return 0;
+    return 1;
+}
+
+/*
  * scan pci devices to add all existed PCI devices to alldevs_list,
  * and setup pci hierarchy in array bus2bridge. This function is only
  * called in VT-d hardware setup
@@ -372,7 +387,6 @@ int __init scan_pci_devices(void)
     int bus, dev, func;
     u8 sec_bus, sub_bus;
     int type;
-    u32 l;
 
     spin_lock(&pcidevs_lock);
     for ( bus = 0; bus < 256; bus++ )
@@ -381,10 +395,7 @@ int __init scan_pci_devices(void)
         {
             for ( func = 0; func < 8; func++ )
             {
-                l = pci_conf_read32(bus, dev, func, PCI_VENDOR_ID);
-                /* some broken boards return 0 or ~0 if a slot is empty: */
-                if ( (l == 0xffffffff) || (l == 0x00000000) ||
-                     (l == 0x0000ffff) || (l == 0xffff0000) )
+                if ( pci_device_detect(bus, dev, func) == 0 )
                     continue;
 
                 pdev = alloc_pdev(bus, PCI_DEVFN(dev, func));
diff -r 104110651ff6 xen/drivers/passthrough/vtd/dmar.c
--- a/xen/drivers/passthrough/vtd/dmar.c	Wed Jan 20 22:50:09 2010 +0800
+++ b/xen/drivers/passthrough/vtd/dmar.c	Thu Jan 21 17:51:45 2010 +0800
@@ -410,14 +410,6 @@ acpi_parse_one_rmrr(struct acpi_dmar_ent
     u64 base_addr = rmrr->base_address, end_addr = rmrr->end_address;
     int ret = 0;
 
-    if ( base_addr >= end_addr )
-    {
-        dprintk(XENLOG_ERR VTDPREFIX,
-                "RMRR error: base_addr %"PRIx64" end_address %"PRIx64"\n",
-                base_addr, end_addr);
-        return -EFAULT;
-    }
-
 #ifdef CONFIG_X86
     /* This check is here simply to detect when RMRR values are
      * not properly represented in the system memory map and
@@ -441,9 +433,6 @@ acpi_parse_one_rmrr(struct acpi_dmar_ent
 
     rmrru->base_address = base_addr;
     rmrru->end_address = end_addr;
-    dprintk(XENLOG_INFO VTDPREFIX,
-            "  RMRR region: base_addr %"PRIx64" end_address %"PRIx64"\n",
-            rmrru->base_address, rmrru->end_address);
 
     dev_scope_start = (void *)(rmrr + 1);
     dev_scope_end   = ((void *)rmrr) + header->length;
@@ -453,7 +442,50 @@ acpi_parse_one_rmrr(struct acpi_dmar_ent
     if ( ret || (rmrru->scope.devices_cnt == 0) )
         xfree(rmrru);
     else
-        acpi_register_rmrr_unit(rmrru);
+    {
+        u8 b, d, f;
+        int i, ignore = 0;
+
+        for ( i = 0; i < rmrru->scope.devices_cnt; i++ )
+        {
+            b = PCI_BUS(rmrru->scope.devices[i]);
+            d = PCI_SLOT(rmrru->scope.devices[i]);
+            f = PCI_FUNC(rmrru->scope.devices[i]);
+
+            if ( pci_device_detect(b, d, f) == 0 )
+                ignore = 1;
+            else
+            {
+                ignore = 0;
+                break;
+            }
+        }
+
+        if ( ignore )
+        {
+            dprintk(XENLOG_WARNING VTDPREFIX,
+                "  Ignore the RMRR (%"PRIx64", %"PRIx64") due to "
+                "devices under its scope are not PCI discoverable!\n",
+                rmrru->base_address, rmrru->end_address);
+            xfree(rmrru);
+        }
+        else if ( base_addr > end_addr )
+        {
+            dprintk(XENLOG_WARNING VTDPREFIX,
+                "  The RMRR (%"PRIx64", %"PRIx64") is incorrect!\n",
+                rmrru->base_address, rmrru->end_address);
+            xfree(rmrru);
+            ret = -EFAULT;
+        }
+        else
+        {
+            dprintk(XENLOG_INFO VTDPREFIX,
+                "  RMRR region: base_addr %"PRIx64" end_address %"PRIx64"\n",
+                rmrru->base_address, rmrru->end_address);
+            acpi_register_rmrr_unit(rmrru);
+        }
+    }
+
     return ret;
 }
 
diff -r 104110651ff6 xen/include/xen/pci.h
--- a/xen/include/xen/pci.h	Wed Jan 20 22:50:09 2010 +0800
+++ b/xen/include/xen/pci.h	Thu Jan 21 17:39:27 2010 +0800
@@ -74,6 +74,7 @@ enum {
     DEV_TYPE_PCI,
 };
 
+int pci_device_detect(u8 bus, u8 dev, u8 func);
 int scan_pci_devices(void);
 int pdev_type(u8 bus, u8 devfn);
 int find_upstream_bridge(u8 *bus, u8 *devfn, u8 *secbus);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

^ permalink raw reply	[flat|nested] 77+ messages in thread
* Re: [PATCH] VT-d: improve RMRR validity checking, documenting boot options
@ 2010-01-23 16:20 Stephen Spector
  0 siblings, 0 replies; 77+ messages in thread
From: Stephen Spector @ 2010-01-23 16:20 UTC (permalink / raw)
  To: 'pasik@iki.fi', 'linux@eikelenboom.it'
  Cc: 'xen-devel@lists.xensource.com',
	'n_iwamatsu@jp.fujitsu.com',
	'weidong.han@intel.com',
	'joseph.cihula@intel.com',
	'allen.m.kay@intel.com', Keir Fraser

[-- Attachment #1: Type: text/plain, Size: 4539 bytes --]

Makes sense. Will move files on monday. Thanks

----- Original Message -----
From: Pasi Kärkkäinen <pasik@iki.fi>
To: Sander Eikelenboom <linux@eikelenboom.it>
Cc: Weidong Han <weidong.han@intel.com>; xen-devel@lists.xensource.com <xen-devel@lists.xensource.com>; Kay, Allen M <allen.m.kay@intel.com>; Cihula, Joseph <joseph.cihula@intel.com>; Noboru Iwamatsu <n_iwamatsu@jp.fujitsu.com>; Keir Fraser; Stephen Spector
Sent: Sat Jan 23 09:54:35 2010
Subject: Re: [Xen-devel] [PATCH] VT-d: improve RMRR validity checking,	documenting boot options

On Sat, Jan 23, 2010 at 03:33:50PM +0100, Sander Eikelenboom wrote:
> Hmm perhaps somewhat unrelated, but is there a comprehensive list with Xen specific boot options with explanation ?
> 
> Since some seem to be valid for 2.6.18.8 some for pvops as well, and the hypervisor has some of her own.
> If not I could perhaps try to make a Wiki with a table with options and explanation for it ?
> 
> This discussion seems to show sometimes you can interpret some option names in multiple ways and things have additional consequences.
> 

Have you checked this wiki page?:
http://wiki.xensource.com/xenwiki/VTdHowTo

But yeah, I think we should definitely add a wiki page
describing all the Xen + Dom0 kernel options.. a list that's up to date.

Stephen: Did you make some PDF document about Xen hypervisor boot options? 
I remember you doing PDF about the /etc/xen/<guest> cfgfile options earlier.

I think these documents should be put to a wiki page, it's much easier to update
and read them there.

-- Pasi


> --
> Sander
> 
> 
> Saturday, January 23, 2010, 2:08:50 PM, you wrote:
> 
> > On Sat, Jan 23, 2010 at 08:40:10PM +0800, Weidong Han wrote:
> >> Pasi Kärkkäinen wrote:
> >>> On Fri, Jan 22, 2010 at 08:15:11PM +0800, Weidong Han wrote:
> >>>   
> >>>> Sander Eikelenboom wrote:
> >>>>     
> >>>>> Hello Weidong,
> >>>>>
> >>>>> Wouldn't it be more clear to add an option to iommu= for this case ?
> >>>>>
> >>>>> if iommu=on,..,..,security
> >>>>>
> >>>>> With the security option specified:
> >>>>>      -it would be most strict in it's checks, since enforcing security with the iommu requires that as you have pointed out.
> >>>>>      -warn,fail or panic incase it can't enable all to enforce the security.
> >>>>>         
> >>>> iommu=force is for security. It does as you described above. So I 
> >>>> think  "security" option is not necessary.
> >>>>     
> >>>>> Without the security option specified (default)
> >>>>>      - it tries to work as with the security option specified
> >>>>>      - but incase of problems makes the assumption the iommu's main task is not security, but making as much of vt-d working to keep the passthrough functionality
> >>>>>      - it will only warn, that you will lose the security part, that it would be wise to let your bios be fixed, and not making it panic
> >>>>>      - and keep vt-d enabled
> >>>>>
> >>>>>         
> >>>> the default iommu=1 works like iommu=force if BIOS is correct. But in 
> >>>>  fact we encountered some buggy BIOS, and then we added some 
> >>>> workarounds  to make VT-d still be enabled,  or warn and disable VT-d 
> >>>> if the issue is  regarded as invalid and cannot be workarounded. 
> >>>> These workarounds make  Xen more defensive to VT-d BIOS issues. The 
> >>>> panic only occurs when  operating VT-d hardware fails, because it 
> >>>> means the hardware is possibly  malfunctional.
> >>>>
> >>>> In short, default iommu=1 can workaround known VT-d BIOS issues we   
> >>>> observed till now, while iommu=force ensures best security provided 
> >>>> by VT-d.
> >>>>
> >>>>     
> >>>
> >>> So the default iommu=1 might be insecure? And iommu=force is always 
> >>> secure? 
> >>>
> >>> To me "force" sounds like it makes it work always, no matter if it's secure or not..
> >>>   
> >> The "security" here means the protection provided VT-d. The main  
> >> difference between them is iommu=force tries to enable all VT-d units in  
> >> any case, if any VT-d unit cannot enabled, it will quit Xen booting  
> >> (panic), thus it guarantees security provided by VT-d. while when  
> >> iommu=1, in order to workaround some BIOS issues, it will ignore some  
> >> invalid DRHDs, or disable whole VT-d to keep Xen work without VT-d. 
> >>
> 
> > Ok.. Thanks for explaining it. 
> 
> > -- Pasi
> 
> 
> 
> 
> -- 
> Best regards,
>  Sander                            mailto:linux@eikelenboom.it
> 

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

^ permalink raw reply	[flat|nested] 77+ messages in thread
end of thread, other threads:[~2010-03-11  8:30 UTC | newest]

Thread overview: 77+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-01-21  2:46 [PATCH] VT-d: improve RMRR validity checking Han, Weidong
2010-01-21  8:25 ` Noboru Iwamatsu
2010-01-21  8:38   ` Han, Weidong
2010-01-21 10:03     ` Noboru Iwamatsu
2010-01-21 10:08       ` Noboru Iwamatsu
2010-01-21 10:19         ` Weidong Han
2010-01-21 10:27           ` Keir Fraser
2010-01-21 10:49             ` Weidong Han
2010-01-21 12:19               ` Noboru Iwamatsu
2010-01-21 12:46                 ` Weidong Han
2010-01-21 14:01                   ` Keir Fraser
2010-01-21 14:17                   ` Sander Eikelenboom
2010-01-21 14:33                     ` Keir Fraser
2010-01-22  2:12                       ` Weidong Han
2010-01-22  2:38                         ` Noboru Iwamatsu
2010-01-22  2:53                           ` Weidong Han
2010-01-22  3:16                             ` Noboru Iwamatsu
2010-01-22  8:47                               ` Weidong Han
2010-01-22  9:19                                 ` Sander Eikelenboom
2010-01-22 12:15                                   ` Weidong Han
2010-01-22 12:32                                     ` Pasi Kärkkäinen
2010-01-23 12:40                                       ` Weidong Han
2010-01-23 13:08                                         ` Pasi Kärkkäinen
2010-01-23 14:33                                           ` Sander Eikelenboom
2010-01-23 14:54                                             ` [PATCH] VT-d: improve RMRR validity checking, documenting boot options Pasi Kärkkäinen
2010-01-25 16:40                                               ` Stephen Spector
2010-01-25 16:58                                                 ` Documentation Xen-hypervisor and Dom0 xen-related boot options (was Re: [PATCH] VT-d: improve RMRR validity checking, documenting boot options) Sander Eikelenboom
2010-01-25 20:56                                                   ` Stephen Spector
2010-01-27 11:33                                                     ` Pasi Kärkkäinen
2010-01-25  7:06                                 ` [PATCH] VT-d: improve RMRR validity checking Noboru Iwamatsu
2010-01-25  7:56                                   ` Weidong Han
2010-01-25  9:02                                     ` Sander Eikelenboom
2010-01-25  9:11                                       ` Weidong Han
2010-01-25  9:22                                     ` Noboru Iwamatsu
2010-01-25 10:08                                       ` Weidong Han
2010-01-25 10:45                                         ` Sander Eikelenboom
2010-01-25 13:43                                           ` Keir Fraser
2010-01-25 13:57                                             ` Christian Tramnitz
2010-01-25 14:10                                             ` Weidong Han
2010-01-26  1:16                                               ` Noboru Iwamatsu
2010-01-26  5:51                                                 ` Weidong Han
2010-01-26  6:38                                                   ` Noboru Iwamatsu
2010-01-26  6:42                                                     ` Weidong Han
2010-01-25 14:12                                             ` Weidong Han
2010-01-25 14:13                                             ` Han, Weidong
2010-03-09 21:39                                 ` Alex Williamson
2010-03-09 21:30                                   ` Konrad Rzeszutek Wilk
2010-03-09 21:57                                     ` Alex Williamson
2010-03-09 22:22                                       ` Konrad Rzeszutek Wilk
2010-03-09 23:05                                         ` Alex Williamson
2010-03-09 23:25                                           ` Alex Williamson
2010-03-10  2:13                                             ` Alex Williamson
2010-03-10  2:40                                   ` Weidong Han
2010-03-10  3:18                                     ` Alex Williamson
2010-03-10  3:28                                       ` Weidong Han
2010-03-10  3:37                                         ` Alex Williamson
2010-03-10  4:25                                           ` Weidong Han
2010-03-10  4:47                                             ` Alex Williamson
2010-03-10  7:03                                               ` Weidong Han
2010-03-10 13:56                                                 ` Alex Williamson
2010-03-10 18:06                                                   ` Alex Williamson
2010-03-11  2:11                                                     ` Weidong Han
2010-03-11  2:32                                                       ` Alex Williamson
2010-03-11  3:44                                                         ` Weidong Han
2010-03-11  4:52                                                           ` Alex Williamson
2010-03-11  8:30                                                             ` Weidong Han
2010-01-21 15:28                     ` Andrew Lyon
2010-01-21 15:04                 ` Keir Fraser
2010-01-22  1:35                   ` Noboru Iwamatsu
2010-01-21 10:13       ` Weidong Han
2010-01-21 12:09         ` Noboru Iwamatsu
2010-01-21 12:38           ` Weidong Han
2010-01-22  0:23             ` Noboru Iwamatsu
2010-01-21  8:45   ` Andrew Lyon
2010-01-21 10:03     ` Weidong Han
2010-01-21  9:15   ` Keir Fraser
  -- strict thread matches above, loose matches on Subject: below --
2010-01-23 16:20 [PATCH] VT-d: improve RMRR validity checking, documenting boot options Stephen Spector

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).