xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
@ 2013-08-07  2:40 suravee.suthikulpanit
  2013-08-07  2:42 ` Suravee Suthikulanit
                   ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: suravee.suthikulpanit @ 2013-08-07  2:40 UTC (permalink / raw)
  To: xen-devel, JBeulich, xiantao.zhang; +Cc: Suravee Suthikulpanit

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>

The host bridge device (i.e. 0x18 for AMD) does not
require IOMMU, and therefore is not included in the IVRS.
The current logic tries to map all PCI devices to an IOMMU. 
In this case, "xl dmesg" shows the following message on AMD sytem.

(XEN) setup 0000:00:18.0 for d0 failed (-19)
(XEN) setup 0000:00:18.1 for d0 failed (-19)
(XEN) setup 0000:00:18.2 for d0 failed (-19)
(XEN) setup 0000:00:18.3 for d0 failed (-19)
(XEN) setup 0000:00:18.4 for d0 failed (-19)
(XEN) setup 0000:00:18.5 for d0 failed (-19)

This patch add new device type (i.e. DEV_TYPE_PCI_HOST_BRIDGE) which
is corresponded to PCI class code 0x06 and sub-class 0x00. Then, it
use this new type to filter when trying to map device to IOMMU.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/drivers/passthrough/amd/pci_amd_iommu.c |   20 +++++++++++++----
 xen/drivers/passthrough/pci.c               |   31 ++++++++++++++++-----------
 xen/drivers/passthrough/vtd/iommu.c         |    2 ++
 xen/include/xen/pci.h                       |    1 +
 4 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 9684ae8..0bb954a 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -175,10 +175,22 @@ static int __init amd_iommu_setup_dom0_device(u8 devfn, struct pci_dev *pdev)
 
     if ( unlikely(!iommu) )
     {
-        AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n",
-                        pdev->seg, pdev->bus,
-                        PCI_SLOT(devfn), PCI_FUNC(devfn));
-        return -ENODEV;
+        /* Filter the bridge devices */
+        if ( (pdev->type == DEV_TYPE_PCIe_BRIDGE)
+        ||   (pdev->type == DEV_TYPE_PCIe2PCI_BRIDGE)
+        ||   (pdev->type == DEV_TYPE_LEGACY_PCI_BRIDGE)
+        ||   (pdev->type == DEV_TYPE_PCI_HOST_BRIDGE) )
+        {
+            AMD_IOMMU_DEBUG("Skipping device %04x:%02x:%02x.%u (type %x)\n",
+                        pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
+                        pdev->type);
+            return 0;
+        } else {
+            AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n",
+                    pdev->seg, pdev->bus,
+                    PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+            return -ENODEV;
+        }
     }
 
     amd_iommu_setup_domain_device(pdev->domain, iommu, devfn, pdev);
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index f2756c9..0b94fc4 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -189,9 +189,6 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
         u16 cap;
         u8 sec_bus, sub_bus;
 
-        case DEV_TYPE_PCIe_BRIDGE:
-            break;
-
         case DEV_TYPE_PCIe2PCI_BRIDGE:
         case DEV_TYPE_LEGACY_PCI_BRIDGE:
             sec_bus = pci_conf_read8(pseg->nr, bus, PCI_SLOT(devfn),
@@ -239,6 +236,8 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
             break;
 
         case DEV_TYPE_PCI:
+        case DEV_TYPE_PCI_HOST_BRIDGE:
+        case DEV_TYPE_PCIe_BRIDGE:
             break;
 
         default:
@@ -691,35 +690,43 @@ void pci_release_devices(struct domain *d)
     spin_unlock(&pcidevs_lock);
 }
 
-#define PCI_CLASS_BRIDGE_PCI     0x0604
+#define PCI_CLASS_HOST_PCI_BRIDGE    0x0600
+#define PCI_CLASS_PCI_PCI_BRIDGE     0x0604
 
 enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn)
 {
-    u16 class_device, creg;
+    u16 creg;
     u8 d = PCI_SLOT(devfn), f = PCI_FUNC(devfn);
-    int pos = pci_find_cap_offset(seg, bus, d, f, PCI_CAP_ID_EXP);
+    u16 class_device = pci_conf_read16(seg, bus, d, f, PCI_CLASS_DEVICE);
+    int cap_offset = pci_find_cap_offset(seg, bus, d, f, PCI_CAP_ID_EXP);
 
-    class_device = pci_conf_read16(seg, bus, d, f, PCI_CLASS_DEVICE);
     switch ( class_device )
     {
-    case PCI_CLASS_BRIDGE_PCI:
-        if ( !pos )
+    case PCI_CLASS_PCI_PCI_BRIDGE:
+        if ( !cap_offset )
             return DEV_TYPE_LEGACY_PCI_BRIDGE;
-        creg = pci_conf_read16(seg, bus, d, f, pos + PCI_EXP_FLAGS);
+
+        creg = pci_conf_read16(seg, bus, d, f, cap_offset + PCI_EXP_FLAGS);
+
         switch ( (creg & PCI_EXP_FLAGS_TYPE) >> 4 )
         {
         case PCI_EXP_TYPE_PCI_BRIDGE:
             return DEV_TYPE_PCIe2PCI_BRIDGE;
         case PCI_EXP_TYPE_PCIE_BRIDGE:
             return DEV_TYPE_PCI2PCIe_BRIDGE;
+        default: 
+            return DEV_TYPE_PCIe_BRIDGE;
         }
-        return DEV_TYPE_PCIe_BRIDGE;
+        break;
+
+    case PCI_CLASS_HOST_PCI_BRIDGE:
+        return DEV_TYPE_PCI_HOST_BRIDGE;
 
     case 0x0000: case 0xffff:
         return DEV_TYPE_PCI_UNKNOWN;
     }
 
-    return pos ? DEV_TYPE_PCIe_ENDPOINT : DEV_TYPE_PCI;
+    return cap_offset ? DEV_TYPE_PCIe_ENDPOINT : DEV_TYPE_PCI;
 }
 
 /*
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 76f7b8e..046262c 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1448,6 +1448,7 @@ static int domain_context_mapping(
         break;
 
     case DEV_TYPE_PCI:
+    case DEV_TYPE_PCI_HOST_BRIDGE:
         if ( iommu_verbose )
             dprintk(VTDPREFIX, "d%d:PCI: map %04x:%02x:%02x.%u\n",
                     domain->domain_id, seg, bus,
@@ -1577,6 +1578,7 @@ static int domain_context_unmap(
         break;
 
     case DEV_TYPE_PCI:
+    case DEV_TYPE_PCI_HOST_BRIDGE:
         if ( iommu_verbose )
             dprintk(VTDPREFIX, "d%d:PCI: unmap %04x:%02x:%02x.%u\n",
                     domain->domain_id, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index ca72a99..2530491 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -73,6 +73,7 @@ struct pci_dev {
         DEV_TYPE_PCIe2PCI_BRIDGE,   // PCIe-to-PCI/PCIx bridge
         DEV_TYPE_PCI2PCIe_BRIDGE,   // PCI/PCIx-to-PCIe bridge
         DEV_TYPE_LEGACY_PCI_BRIDGE, // Legacy PCI bridge
+        DEV_TYPE_PCI_HOST_BRIDGE,   // PCI Host bridge
         DEV_TYPE_PCI,
     } type;
 
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
  2013-08-07  2:40 [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed suravee.suthikulpanit
@ 2013-08-07  2:42 ` Suravee Suthikulanit
  2013-08-07  8:33   ` Stefan Bader
  2013-08-07  7:33 ` Jan Beulich
  2013-08-07  9:34 ` Andrew Cooper
  2 siblings, 1 reply; 18+ messages in thread
From: Suravee Suthikulanit @ 2013-08-07  2:42 UTC (permalink / raw)
  To: suravee.suthikulpanit; +Cc: Stefan Bader, xiantao.zhang, JBeulich, xen-devel

Also CC Stefan.

Suravee

On 8/6/2013 9:40 PM, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> The host bridge device (i.e. 0x18 for AMD) does not
> require IOMMU, and therefore is not included in the IVRS.
> The current logic tries to map all PCI devices to an IOMMU.
> In this case, "xl dmesg" shows the following message on AMD sytem.
>
> (XEN) setup 0000:00:18.0 for d0 failed (-19)
> (XEN) setup 0000:00:18.1 for d0 failed (-19)
> (XEN) setup 0000:00:18.2 for d0 failed (-19)
> (XEN) setup 0000:00:18.3 for d0 failed (-19)
> (XEN) setup 0000:00:18.4 for d0 failed (-19)
> (XEN) setup 0000:00:18.5 for d0 failed (-19)
>
> This patch add new device type (i.e. DEV_TYPE_PCI_HOST_BRIDGE) which
> is corresponded to PCI class code 0x06 and sub-class 0x00. Then, it
> use this new type to filter when trying to map device to IOMMU.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>   xen/drivers/passthrough/amd/pci_amd_iommu.c |   20 +++++++++++++----
>   xen/drivers/passthrough/pci.c               |   31 ++++++++++++++++-----------
>   xen/drivers/passthrough/vtd/iommu.c         |    2 ++
>   xen/include/xen/pci.h                       |    1 +
>   4 files changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index 9684ae8..0bb954a 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -175,10 +175,22 @@ static int __init amd_iommu_setup_dom0_device(u8 devfn, struct pci_dev *pdev)
>   
>       if ( unlikely(!iommu) )
>       {
> -        AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n",
> -                        pdev->seg, pdev->bus,
> -                        PCI_SLOT(devfn), PCI_FUNC(devfn));
> -        return -ENODEV;
> +        /* Filter the bridge devices */
> +        if ( (pdev->type == DEV_TYPE_PCIe_BRIDGE)
> +        ||   (pdev->type == DEV_TYPE_PCIe2PCI_BRIDGE)
> +        ||   (pdev->type == DEV_TYPE_LEGACY_PCI_BRIDGE)
> +        ||   (pdev->type == DEV_TYPE_PCI_HOST_BRIDGE) )
> +        {
> +            AMD_IOMMU_DEBUG("Skipping device %04x:%02x:%02x.%u (type %x)\n",
> +                        pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
> +                        pdev->type);
> +            return 0;
> +        } else {
> +            AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n",
> +                    pdev->seg, pdev->bus,
> +                    PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> +            return -ENODEV;
> +        }
>       }
>   
>       amd_iommu_setup_domain_device(pdev->domain, iommu, devfn, pdev);
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index f2756c9..0b94fc4 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -189,9 +189,6 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>           u16 cap;
>           u8 sec_bus, sub_bus;
>   
> -        case DEV_TYPE_PCIe_BRIDGE:
> -            break;
> -
>           case DEV_TYPE_PCIe2PCI_BRIDGE:
>           case DEV_TYPE_LEGACY_PCI_BRIDGE:
>               sec_bus = pci_conf_read8(pseg->nr, bus, PCI_SLOT(devfn),
> @@ -239,6 +236,8 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>               break;
>   
>           case DEV_TYPE_PCI:
> +        case DEV_TYPE_PCI_HOST_BRIDGE:
> +        case DEV_TYPE_PCIe_BRIDGE:
>               break;
>   
>           default:
> @@ -691,35 +690,43 @@ void pci_release_devices(struct domain *d)
>       spin_unlock(&pcidevs_lock);
>   }
>   
> -#define PCI_CLASS_BRIDGE_PCI     0x0604
> +#define PCI_CLASS_HOST_PCI_BRIDGE    0x0600
> +#define PCI_CLASS_PCI_PCI_BRIDGE     0x0604
>   
>   enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn)
>   {
> -    u16 class_device, creg;
> +    u16 creg;
>       u8 d = PCI_SLOT(devfn), f = PCI_FUNC(devfn);
> -    int pos = pci_find_cap_offset(seg, bus, d, f, PCI_CAP_ID_EXP);
> +    u16 class_device = pci_conf_read16(seg, bus, d, f, PCI_CLASS_DEVICE);
> +    int cap_offset = pci_find_cap_offset(seg, bus, d, f, PCI_CAP_ID_EXP);
>   
> -    class_device = pci_conf_read16(seg, bus, d, f, PCI_CLASS_DEVICE);
>       switch ( class_device )
>       {
> -    case PCI_CLASS_BRIDGE_PCI:
> -        if ( !pos )
> +    case PCI_CLASS_PCI_PCI_BRIDGE:
> +        if ( !cap_offset )
>               return DEV_TYPE_LEGACY_PCI_BRIDGE;
> -        creg = pci_conf_read16(seg, bus, d, f, pos + PCI_EXP_FLAGS);
> +
> +        creg = pci_conf_read16(seg, bus, d, f, cap_offset + PCI_EXP_FLAGS);
> +
>           switch ( (creg & PCI_EXP_FLAGS_TYPE) >> 4 )
>           {
>           case PCI_EXP_TYPE_PCI_BRIDGE:
>               return DEV_TYPE_PCIe2PCI_BRIDGE;
>           case PCI_EXP_TYPE_PCIE_BRIDGE:
>               return DEV_TYPE_PCI2PCIe_BRIDGE;
> +        default:
> +            return DEV_TYPE_PCIe_BRIDGE;
>           }
> -        return DEV_TYPE_PCIe_BRIDGE;
> +        break;
> +
> +    case PCI_CLASS_HOST_PCI_BRIDGE:
> +        return DEV_TYPE_PCI_HOST_BRIDGE;
>   
>       case 0x0000: case 0xffff:
>           return DEV_TYPE_PCI_UNKNOWN;
>       }
>   
> -    return pos ? DEV_TYPE_PCIe_ENDPOINT : DEV_TYPE_PCI;
> +    return cap_offset ? DEV_TYPE_PCIe_ENDPOINT : DEV_TYPE_PCI;
>   }
>   
>   /*
> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> index 76f7b8e..046262c 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1448,6 +1448,7 @@ static int domain_context_mapping(
>           break;
>   
>       case DEV_TYPE_PCI:
> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>           if ( iommu_verbose )
>               dprintk(VTDPREFIX, "d%d:PCI: map %04x:%02x:%02x.%u\n",
>                       domain->domain_id, seg, bus,
> @@ -1577,6 +1578,7 @@ static int domain_context_unmap(
>           break;
>   
>       case DEV_TYPE_PCI:
> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>           if ( iommu_verbose )
>               dprintk(VTDPREFIX, "d%d:PCI: unmap %04x:%02x:%02x.%u\n",
>                       domain->domain_id, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index ca72a99..2530491 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -73,6 +73,7 @@ struct pci_dev {
>           DEV_TYPE_PCIe2PCI_BRIDGE,   // PCIe-to-PCI/PCIx bridge
>           DEV_TYPE_PCI2PCIe_BRIDGE,   // PCI/PCIx-to-PCIe bridge
>           DEV_TYPE_LEGACY_PCI_BRIDGE, // Legacy PCI bridge
> +        DEV_TYPE_PCI_HOST_BRIDGE,   // PCI Host bridge
>           DEV_TYPE_PCI,
>       } type;
>   

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
  2013-08-07  2:40 [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed suravee.suthikulpanit
  2013-08-07  2:42 ` Suravee Suthikulanit
@ 2013-08-07  7:33 ` Jan Beulich
  2013-08-07 15:31   ` Suravee Suthikulanit
  2013-08-07  9:34 ` Andrew Cooper
  2 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2013-08-07  7:33 UTC (permalink / raw)
  To: suravee.suthikulpanit; +Cc: xen-devel, xiantao.zhang

>>> On 07.08.13 at 04:40, <suravee.suthikulpanit@amd.com> wrote:
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -175,10 +175,22 @@ static int __init amd_iommu_setup_dom0_device(u8 devfn, 
> struct pci_dev *pdev)
>  
>      if ( unlikely(!iommu) )
>      {
> -        AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n",
> -                        pdev->seg, pdev->bus,
> -                        PCI_SLOT(devfn), PCI_FUNC(devfn));
> -        return -ENODEV;
> +        /* Filter the bridge devices */
> +        if ( (pdev->type == DEV_TYPE_PCIe_BRIDGE)
> +        ||   (pdev->type == DEV_TYPE_PCIe2PCI_BRIDGE)
> +        ||   (pdev->type == DEV_TYPE_LEGACY_PCI_BRIDGE)
> +        ||   (pdev->type == DEV_TYPE_PCI_HOST_BRIDGE) )

Indentation.

> +        {
> +            AMD_IOMMU_DEBUG("Skipping device %04x:%02x:%02x.%u (type %x)\n",
> +                        pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
> +                        pdev->type);

I can see why host bridges can be skipped, but is this really also true
for other bridge types, most importantly legacy ones?

Also, please say at least "bridge" here instead of "device", to make
matters as clear as possible to people inspecting logs.

> +            return 0;
> +        } else {

No need for the "else" here; dropping it will reduce the churn the
patch causes, ...

> +            AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n",
> +                    pdev->seg, pdev->bus,
> +                    PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> +            return -ENODEV;
> +        }

... as the indentation of this then won't need to change.

> @@ -691,35 +690,43 @@ void pci_release_devices(struct domain *d)
>      spin_unlock(&pcidevs_lock);
>  }
>  
> -#define PCI_CLASS_BRIDGE_PCI     0x0604
> +#define PCI_CLASS_HOST_PCI_BRIDGE    0x0600
> +#define PCI_CLASS_PCI_PCI_BRIDGE     0x0604

Please don't needlessly introduce names different from their Linux
counterparts.

>  enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn)
>  {
> -    u16 class_device, creg;
> +    u16 creg;
>      u8 d = PCI_SLOT(devfn), f = PCI_FUNC(devfn);
> -    int pos = pci_find_cap_offset(seg, bus, d, f, PCI_CAP_ID_EXP);
> +    u16 class_device = pci_conf_read16(seg, bus, d, f, PCI_CLASS_DEVICE);
> +    int cap_offset = pci_find_cap_offset(seg, bus, d, f, PCI_CAP_ID_EXP);
>  
> -    class_device = pci_conf_read16(seg, bus, d, f, PCI_CLASS_DEVICE);
>      switch ( class_device )
>      {
> -    case PCI_CLASS_BRIDGE_PCI:
> -        if ( !pos )
> +    case PCI_CLASS_PCI_PCI_BRIDGE:
> +        if ( !cap_offset )
>              return DEV_TYPE_LEGACY_PCI_BRIDGE;
> -        creg = pci_conf_read16(seg, bus, d, f, pos + PCI_EXP_FLAGS);
> +
> +        creg = pci_conf_read16(seg, bus, d, f, cap_offset + PCI_EXP_FLAGS);
> +
>          switch ( (creg & PCI_EXP_FLAGS_TYPE) >> 4 )
>          {
>          case PCI_EXP_TYPE_PCI_BRIDGE:
>              return DEV_TYPE_PCIe2PCI_BRIDGE;
>          case PCI_EXP_TYPE_PCIE_BRIDGE:
>              return DEV_TYPE_PCI2PCIe_BRIDGE;
> +        default: 
> +            return DEV_TYPE_PCIe_BRIDGE;
>          }
> -        return DEV_TYPE_PCIe_BRIDGE;
> +        break;
> +
> +    case PCI_CLASS_HOST_PCI_BRIDGE:
> +        return DEV_TYPE_PCI_HOST_BRIDGE;

All the changes to this function apart from the above two lines
look entirely unrelated to the intentions of the patch. Please
drop them, or move them to a follow-up cleanup patch.

> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1448,6 +1448,7 @@ static int domain_context_mapping(
>          break;
>  
>      case DEV_TYPE_PCI:
> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>          if ( iommu_verbose )
>              dprintk(VTDPREFIX, "d%d:PCI: map %04x:%02x:%02x.%u\n",
>                      domain->domain_id, seg, bus,
> @@ -1577,6 +1578,7 @@ static int domain_context_unmap(
>          break;
>  
>      case DEV_TYPE_PCI:
> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>          if ( iommu_verbose )
>              dprintk(VTDPREFIX, "d%d:PCI: unmap %04x:%02x:%02x.%u\n",
>                      domain->domain_id, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));

Did you really grep for the uses of DEV_TYPE_PCI? Is see another
similar use in xen/drivers/passthrough/vtd/intremap.c which would
- afaict - also need similar adjustment.

And in any case I'm expecting Xiantao to comment on whether host
bridges should be treated any different from normal PCI devices.

Jan

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
  2013-08-07  2:42 ` Suravee Suthikulanit
@ 2013-08-07  8:33   ` Stefan Bader
  2013-08-08 11:12     ` Stefan Bader
  0 siblings, 1 reply; 18+ messages in thread
From: Stefan Bader @ 2013-08-07  8:33 UTC (permalink / raw)
  To: Suravee Suthikulanit; +Cc: xiantao.zhang, JBeulich, xen-devel


[-- Attachment #1.1: Type: text/plain, Size: 7180 bytes --]

On 07.08.2013 03:42, Suravee Suthikulanit wrote:
> Also CC Stefan.
> 
> Suravee
> 
> On 8/6/2013 9:40 PM, suravee.suthikulpanit@amd.com wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> The host bridge device (i.e. 0x18 for AMD) does not
>> require IOMMU, and therefore is not included in the IVRS.
>> The current logic tries to map all PCI devices to an IOMMU.
>> In this case, "xl dmesg" shows the following message on AMD sytem.
>>
>> (XEN) setup 0000:00:18.0 for d0 failed (-19)
>> (XEN) setup 0000:00:18.1 for d0 failed (-19)
>> (XEN) setup 0000:00:18.2 for d0 failed (-19)
>> (XEN) setup 0000:00:18.3 for d0 failed (-19)
>> (XEN) setup 0000:00:18.4 for d0 failed (-19)
>> (XEN) setup 0000:00:18.5 for d0 failed (-19)
>>
>> This patch add new device type (i.e. DEV_TYPE_PCI_HOST_BRIDGE) which
>> is corresponded to PCI class code 0x06 and sub-class 0x00. Then, it
>> use this new type to filter when trying to map device to IOMMU.
>>
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> ---
>>   xen/drivers/passthrough/amd/pci_amd_iommu.c |   20 +++++++++++++----
>>   xen/drivers/passthrough/pci.c               |   31 ++++++++++++++++-----------
>>   xen/drivers/passthrough/vtd/iommu.c         |    2 ++
>>   xen/include/xen/pci.h                       |    1 +
>>   4 files changed, 38 insertions(+), 16 deletions(-)
>>
>> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> index 9684ae8..0bb954a 100644
>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> @@ -175,10 +175,22 @@ static int __init amd_iommu_setup_dom0_device(u8 devfn,
>> struct pci_dev *pdev)
>>         if ( unlikely(!iommu) )
>>       {
>> -        AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n",
>> -                        pdev->seg, pdev->bus,
>> -                        PCI_SLOT(devfn), PCI_FUNC(devfn));
>> -        return -ENODEV;
>> +        /* Filter the bridge devices */
>> +        if ( (pdev->type == DEV_TYPE_PCIe_BRIDGE)
>> +        ||   (pdev->type == DEV_TYPE_PCIe2PCI_BRIDGE)
>> +        ||   (pdev->type == DEV_TYPE_LEGACY_PCI_BRIDGE)
>> +        ||   (pdev->type == DEV_TYPE_PCI_HOST_BRIDGE) )
>> +        {
>> +            AMD_IOMMU_DEBUG("Skipping device %04x:%02x:%02x.%u (type %x)\n",
>> +                        pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
>> +                        pdev->type);
>> +            return 0;
>> +        } else {
>> +            AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n",
>> +                    pdev->seg, pdev->bus,
>> +                    PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>> +            return -ENODEV;
>> +        }
>>       }
>>         amd_iommu_setup_domain_device(pdev->domain, iommu, devfn, pdev);
>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>> index f2756c9..0b94fc4 100644
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -189,9 +189,6 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8
>> bus, u8 devfn)
>>           u16 cap;
>>           u8 sec_bus, sub_bus;
>>   -        case DEV_TYPE_PCIe_BRIDGE:
>> -            break;
>> -
>>           case DEV_TYPE_PCIe2PCI_BRIDGE:
>>           case DEV_TYPE_LEGACY_PCI_BRIDGE:
>>               sec_bus = pci_conf_read8(pseg->nr, bus, PCI_SLOT(devfn),
>> @@ -239,6 +236,8 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8
>> bus, u8 devfn)
>>               break;
>>             case DEV_TYPE_PCI:
>> +        case DEV_TYPE_PCI_HOST_BRIDGE:
>> +        case DEV_TYPE_PCIe_BRIDGE:
>>               break;
>>             default:
>> @@ -691,35 +690,43 @@ void pci_release_devices(struct domain *d)
>>       spin_unlock(&pcidevs_lock);
>>   }
>>   -#define PCI_CLASS_BRIDGE_PCI     0x0604
>> +#define PCI_CLASS_HOST_PCI_BRIDGE    0x0600
>> +#define PCI_CLASS_PCI_PCI_BRIDGE     0x0604
>>     enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn)
>>   {
>> -    u16 class_device, creg;
>> +    u16 creg;
>>       u8 d = PCI_SLOT(devfn), f = PCI_FUNC(devfn);
>> -    int pos = pci_find_cap_offset(seg, bus, d, f, PCI_CAP_ID_EXP);
>> +    u16 class_device = pci_conf_read16(seg, bus, d, f, PCI_CLASS_DEVICE);
>> +    int cap_offset = pci_find_cap_offset(seg, bus, d, f, PCI_CAP_ID_EXP);
>>   -    class_device = pci_conf_read16(seg, bus, d, f, PCI_CLASS_DEVICE);
>>       switch ( class_device )
>>       {
>> -    case PCI_CLASS_BRIDGE_PCI:
>> -        if ( !pos )
>> +    case PCI_CLASS_PCI_PCI_BRIDGE:
>> +        if ( !cap_offset )
>>               return DEV_TYPE_LEGACY_PCI_BRIDGE;
>> -        creg = pci_conf_read16(seg, bus, d, f, pos + PCI_EXP_FLAGS);
>> +
>> +        creg = pci_conf_read16(seg, bus, d, f, cap_offset + PCI_EXP_FLAGS);
>> +
>>           switch ( (creg & PCI_EXP_FLAGS_TYPE) >> 4 )
>>           {
>>           case PCI_EXP_TYPE_PCI_BRIDGE:
>>               return DEV_TYPE_PCIe2PCI_BRIDGE;
>>           case PCI_EXP_TYPE_PCIE_BRIDGE:
>>               return DEV_TYPE_PCI2PCIe_BRIDGE;
>> +        default:
>> +            return DEV_TYPE_PCIe_BRIDGE;
>>           }
>> -        return DEV_TYPE_PCIe_BRIDGE;
>> +        break;
>> +
>> +    case PCI_CLASS_HOST_PCI_BRIDGE:
>> +        return DEV_TYPE_PCI_HOST_BRIDGE;
>>         case 0x0000: case 0xffff:
>>           return DEV_TYPE_PCI_UNKNOWN;
>>       }
>>   -    return pos ? DEV_TYPE_PCIe_ENDPOINT : DEV_TYPE_PCI;
>> +    return cap_offset ? DEV_TYPE_PCIe_ENDPOINT : DEV_TYPE_PCI;
>>   }
>>     /*
>> diff --git a/xen/drivers/passthrough/vtd/iommu.c
>> b/xen/drivers/passthrough/vtd/iommu.c
>> index 76f7b8e..046262c 100644
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -1448,6 +1448,7 @@ static int domain_context_mapping(
>>           break;
>>         case DEV_TYPE_PCI:
>> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>>           if ( iommu_verbose )
>>               dprintk(VTDPREFIX, "d%d:PCI: map %04x:%02x:%02x.%u\n",
>>                       domain->domain_id, seg, bus,
>> @@ -1577,6 +1578,7 @@ static int domain_context_unmap(
>>           break;
>>         case DEV_TYPE_PCI:
>> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>>           if ( iommu_verbose )
>>               dprintk(VTDPREFIX, "d%d:PCI: unmap %04x:%02x:%02x.%u\n",
>>                       domain->domain_id, seg, bus, PCI_SLOT(devfn),
>> PCI_FUNC(devfn));
>> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
>> index ca72a99..2530491 100644
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -73,6 +73,7 @@ struct pci_dev {
>>           DEV_TYPE_PCIe2PCI_BRIDGE,   // PCIe-to-PCI/PCIx bridge
>>           DEV_TYPE_PCI2PCIe_BRIDGE,   // PCI/PCIx-to-PCIe bridge
>>           DEV_TYPE_LEGACY_PCI_BRIDGE, // Legacy PCI bridge
>> +        DEV_TYPE_PCI_HOST_BRIDGE,   // PCI Host bridge
>>           DEV_TYPE_PCI,
>>       } type;
>>   
> 
> 
I will give it a spin, but might not b before tomorrow.

-Stefan


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

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

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
  2013-08-07  2:40 [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed suravee.suthikulpanit
  2013-08-07  2:42 ` Suravee Suthikulanit
  2013-08-07  7:33 ` Jan Beulich
@ 2013-08-07  9:34 ` Andrew Cooper
  2013-08-07  9:57   ` Jan Beulich
  2 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2013-08-07  9:34 UTC (permalink / raw)
  To: suravee.suthikulpanit; +Cc: xiantao.zhang, JBeulich, xen-devel

On 07/08/13 03:40, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>
> The host bridge device (i.e. 0x18 for AMD) does not
> require IOMMU, and therefore is not included in the IVRS.
> The current logic tries to map all PCI devices to an IOMMU. 
> In this case, "xl dmesg" shows the following message on AMD sytem.

Can you elaborate on what you mean by "does not require IOMMU" ? How
does anything behind the host bridges get translated if they are in the
path of the IOMMU?

~Andrew

>
> (XEN) setup 0000:00:18.0 for d0 failed (-19)
> (XEN) setup 0000:00:18.1 for d0 failed (-19)
> (XEN) setup 0000:00:18.2 for d0 failed (-19)
> (XEN) setup 0000:00:18.3 for d0 failed (-19)
> (XEN) setup 0000:00:18.4 for d0 failed (-19)
> (XEN) setup 0000:00:18.5 for d0 failed (-19)
>
> This patch add new device type (i.e. DEV_TYPE_PCI_HOST_BRIDGE) which
> is corresponded to PCI class code 0x06 and sub-class 0x00. Then, it
> use this new type to filter when trying to map device to IOMMU.
>
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  xen/drivers/passthrough/amd/pci_amd_iommu.c |   20 +++++++++++++----
>  xen/drivers/passthrough/pci.c               |   31 ++++++++++++++++-----------
>  xen/drivers/passthrough/vtd/iommu.c         |    2 ++
>  xen/include/xen/pci.h                       |    1 +
>  4 files changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index 9684ae8..0bb954a 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -175,10 +175,22 @@ static int __init amd_iommu_setup_dom0_device(u8 devfn, struct pci_dev *pdev)
>  
>      if ( unlikely(!iommu) )
>      {
> -        AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n",
> -                        pdev->seg, pdev->bus,
> -                        PCI_SLOT(devfn), PCI_FUNC(devfn));
> -        return -ENODEV;
> +        /* Filter the bridge devices */
> +        if ( (pdev->type == DEV_TYPE_PCIe_BRIDGE)
> +        ||   (pdev->type == DEV_TYPE_PCIe2PCI_BRIDGE)
> +        ||   (pdev->type == DEV_TYPE_LEGACY_PCI_BRIDGE)
> +        ||   (pdev->type == DEV_TYPE_PCI_HOST_BRIDGE) )
> +        {
> +            AMD_IOMMU_DEBUG("Skipping device %04x:%02x:%02x.%u (type %x)\n",
> +                        pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
> +                        pdev->type);
> +            return 0;
> +        } else {
> +            AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n",
> +                    pdev->seg, pdev->bus,
> +                    PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> +            return -ENODEV;
> +        }
>      }
>  
>      amd_iommu_setup_domain_device(pdev->domain, iommu, devfn, pdev);
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index f2756c9..0b94fc4 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -189,9 +189,6 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>          u16 cap;
>          u8 sec_bus, sub_bus;
>  
> -        case DEV_TYPE_PCIe_BRIDGE:
> -            break;
> -
>          case DEV_TYPE_PCIe2PCI_BRIDGE:
>          case DEV_TYPE_LEGACY_PCI_BRIDGE:
>              sec_bus = pci_conf_read8(pseg->nr, bus, PCI_SLOT(devfn),
> @@ -239,6 +236,8 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
>              break;
>  
>          case DEV_TYPE_PCI:
> +        case DEV_TYPE_PCI_HOST_BRIDGE:
> +        case DEV_TYPE_PCIe_BRIDGE:
>              break;
>  
>          default:
> @@ -691,35 +690,43 @@ void pci_release_devices(struct domain *d)
>      spin_unlock(&pcidevs_lock);
>  }
>  
> -#define PCI_CLASS_BRIDGE_PCI     0x0604
> +#define PCI_CLASS_HOST_PCI_BRIDGE    0x0600
> +#define PCI_CLASS_PCI_PCI_BRIDGE     0x0604
>  
>  enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn)
>  {
> -    u16 class_device, creg;
> +    u16 creg;
>      u8 d = PCI_SLOT(devfn), f = PCI_FUNC(devfn);
> -    int pos = pci_find_cap_offset(seg, bus, d, f, PCI_CAP_ID_EXP);
> +    u16 class_device = pci_conf_read16(seg, bus, d, f, PCI_CLASS_DEVICE);
> +    int cap_offset = pci_find_cap_offset(seg, bus, d, f, PCI_CAP_ID_EXP);
>  
> -    class_device = pci_conf_read16(seg, bus, d, f, PCI_CLASS_DEVICE);
>      switch ( class_device )
>      {
> -    case PCI_CLASS_BRIDGE_PCI:
> -        if ( !pos )
> +    case PCI_CLASS_PCI_PCI_BRIDGE:
> +        if ( !cap_offset )
>              return DEV_TYPE_LEGACY_PCI_BRIDGE;
> -        creg = pci_conf_read16(seg, bus, d, f, pos + PCI_EXP_FLAGS);
> +
> +        creg = pci_conf_read16(seg, bus, d, f, cap_offset + PCI_EXP_FLAGS);
> +
>          switch ( (creg & PCI_EXP_FLAGS_TYPE) >> 4 )
>          {
>          case PCI_EXP_TYPE_PCI_BRIDGE:
>              return DEV_TYPE_PCIe2PCI_BRIDGE;
>          case PCI_EXP_TYPE_PCIE_BRIDGE:
>              return DEV_TYPE_PCI2PCIe_BRIDGE;
> +        default: 
> +            return DEV_TYPE_PCIe_BRIDGE;
>          }
> -        return DEV_TYPE_PCIe_BRIDGE;
> +        break;
> +
> +    case PCI_CLASS_HOST_PCI_BRIDGE:
> +        return DEV_TYPE_PCI_HOST_BRIDGE;
>  
>      case 0x0000: case 0xffff:
>          return DEV_TYPE_PCI_UNKNOWN;
>      }
>  
> -    return pos ? DEV_TYPE_PCIe_ENDPOINT : DEV_TYPE_PCI;
> +    return cap_offset ? DEV_TYPE_PCIe_ENDPOINT : DEV_TYPE_PCI;
>  }
>  
>  /*
> diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
> index 76f7b8e..046262c 100644
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1448,6 +1448,7 @@ static int domain_context_mapping(
>          break;
>  
>      case DEV_TYPE_PCI:
> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>          if ( iommu_verbose )
>              dprintk(VTDPREFIX, "d%d:PCI: map %04x:%02x:%02x.%u\n",
>                      domain->domain_id, seg, bus,
> @@ -1577,6 +1578,7 @@ static int domain_context_unmap(
>          break;
>  
>      case DEV_TYPE_PCI:
> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>          if ( iommu_verbose )
>              dprintk(VTDPREFIX, "d%d:PCI: unmap %04x:%02x:%02x.%u\n",
>                      domain->domain_id, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index ca72a99..2530491 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -73,6 +73,7 @@ struct pci_dev {
>          DEV_TYPE_PCIe2PCI_BRIDGE,   // PCIe-to-PCI/PCIx bridge
>          DEV_TYPE_PCI2PCIe_BRIDGE,   // PCI/PCIx-to-PCIe bridge
>          DEV_TYPE_LEGACY_PCI_BRIDGE, // Legacy PCI bridge
> +        DEV_TYPE_PCI_HOST_BRIDGE,   // PCI Host bridge
>          DEV_TYPE_PCI,
>      } type;
>  

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
  2013-08-07  9:34 ` Andrew Cooper
@ 2013-08-07  9:57   ` Jan Beulich
  0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2013-08-07  9:57 UTC (permalink / raw)
  To: suravee.suthikulpanit, Andrew Cooper; +Cc: xiantao.zhang, xen-devel

>>> On 07.08.13 at 11:34, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 07/08/13 03:40, suravee.suthikulpanit@amd.com wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> The host bridge device (i.e. 0x18 for AMD) does not
>> require IOMMU, and therefore is not included in the IVRS.
>> The current logic tries to map all PCI devices to an IOMMU. 
>> In this case, "xl dmesg" shows the following message on AMD sytem.
> 
> Can you elaborate on what you mean by "does not require IOMMU" ? How
> does anything behind the host bridges get translated if they are in the
> path of the IOMMU?

Iiuc it's the nature of a host bridge that what's behind it is the
host (and in particular no PCI devices), which doesn't require
IOMMU translation.

Jan

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
  2013-08-07  7:33 ` Jan Beulich
@ 2013-08-07 15:31   ` Suravee Suthikulanit
  2013-08-07 15:42     ` Jan Beulich
  0 siblings, 1 reply; 18+ messages in thread
From: Suravee Suthikulanit @ 2013-08-07 15:31 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, xiantao.zhang

On 8/7/2013 2:33 AM, Jan Beulich wrote:
>>>> On 07.08.13 at 04:40, <suravee.suthikulpanit@amd.com> wrote:
>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> @@ -175,10 +175,22 @@ static int __init amd_iommu_setup_dom0_device(u8 devfn,
>> struct pci_dev *pdev)
>>   
>>       if ( unlikely(!iommu) )
>>       {
>> -        AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n",
>> -                        pdev->seg, pdev->bus,
>> -                        PCI_SLOT(devfn), PCI_FUNC(devfn));
>> -        return -ENODEV;
>> +        /* Filter the bridge devices */
>> +        if ( (pdev->type == DEV_TYPE_PCIe_BRIDGE)
>> +        ||   (pdev->type == DEV_TYPE_PCIe2PCI_BRIDGE)
>> +        ||   (pdev->type == DEV_TYPE_LEGACY_PCI_BRIDGE)
>> +        ||   (pdev->type == DEV_TYPE_PCI_HOST_BRIDGE) )
> Indentation.
Ok
>
>> +        {
>> +            AMD_IOMMU_DEBUG("Skipping device %04x:%02x:%02x.%u (type %x)\n",
>> +                        pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
>> +                        pdev->type);
> I can see why host bridges can be skipped, but is this really also true
> for other bridge types, most importantly legacy ones?

I am using the same logic here as in Intel's version in the driver/passthrough/vtd/iommu/domain_context_map.

> Also, please say at least "bridge" here instead of "device", to make matters as clear as possible to people inspecting logs.

yep.

>
>> +            return 0;
>> +        } else {
> No need for the "else" here; dropping it will reduce the churn the
> patch causes, ...
>
>> +            AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n",
>> +                    pdev->seg, pdev->bus,
>> +                    PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>> +            return -ENODEV;
>> +        }
> ... as the indentation of this then won't need to change.

Yep

>
>> @@ -691,35 +690,43 @@ void pci_release_devices(struct domain *d)
>>       spin_unlock(&pcidevs_lock);
>>   }
>>   
>> -#define PCI_CLASS_BRIDGE_PCI     0x0604
>> +#define PCI_CLASS_HOST_PCI_BRIDGE    0x0600
>> +#define PCI_CLASS_PCI_PCI_BRIDGE     0x0604
> Please don't needlessly introduce names different from their Linux
> counterparts.

Oh, sorry.  I actually didn't notice the Linux ones. Thanks for pointing out.

>
>>   enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn)
>>   {
>> -    u16 class_device, creg;
>> +    u16 creg;
>>       u8 d = PCI_SLOT(devfn), f = PCI_FUNC(devfn);
>> -    int pos = pci_find_cap_offset(seg, bus, d, f, PCI_CAP_ID_EXP);
>> +    u16 class_device = pci_conf_read16(seg, bus, d, f, PCI_CLASS_DEVICE);
>> +    int cap_offset = pci_find_cap_offset(seg, bus, d, f, PCI_CAP_ID_EXP);
>>   
>> -    class_device = pci_conf_read16(seg, bus, d, f, PCI_CLASS_DEVICE);
>>       switch ( class_device )
>>       {
>> -    case PCI_CLASS_BRIDGE_PCI:
>> -        if ( !pos )
>> +    case PCI_CLASS_PCI_PCI_BRIDGE:
>> +        if ( !cap_offset )
>>               return DEV_TYPE_LEGACY_PCI_BRIDGE;
>> -        creg = pci_conf_read16(seg, bus, d, f, pos + PCI_EXP_FLAGS);
>> +
>> +        creg = pci_conf_read16(seg, bus, d, f, cap_offset + PCI_EXP_FLAGS);
>> +
>>           switch ( (creg & PCI_EXP_FLAGS_TYPE) >> 4 )
>>           {
>>           case PCI_EXP_TYPE_PCI_BRIDGE:
>>               return DEV_TYPE_PCIe2PCI_BRIDGE;
>>           case PCI_EXP_TYPE_PCIE_BRIDGE:
>>               return DEV_TYPE_PCI2PCIe_BRIDGE;
>> +        default:
>> +            return DEV_TYPE_PCIe_BRIDGE;
>>           }
>> -        return DEV_TYPE_PCIe_BRIDGE;
>> +        break;
>> +
>> +    case PCI_CLASS_HOST_PCI_BRIDGE:
>> +        return DEV_TYPE_PCI_HOST_BRIDGE;
> All the changes to this function apart from the above two lines
> look entirely unrelated to the intentions of the patch. Please
> drop them, or move them to a follow-up cleanup patch.

Ok, I'm dropping it.

>
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -1448,6 +1448,7 @@ static int domain_context_mapping(
>>           break;
>>   
>>       case DEV_TYPE_PCI:
>> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>>           if ( iommu_verbose )
>>               dprintk(VTDPREFIX, "d%d:PCI: map %04x:%02x:%02x.%u\n",
>>                       domain->domain_id, seg, bus,
>> @@ -1577,6 +1578,7 @@ static int domain_context_unmap(
>>           break;
>>   
>>       case DEV_TYPE_PCI:
>> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>>           if ( iommu_verbose )
>>               dprintk(VTDPREFIX, "d%d:PCI: unmap %04x:%02x:%02x.%u\n",
>>                       domain->domain_id, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
> Did you really grep for the uses of DEV_TYPE_PCI? Is see another
> similar use in xen/drivers/passthrough/vtd/intremap.c which would
> - afaict - also need similar adjustment.

Ah, I missed that one.  Thank you.

> And in any case I'm expecting Xiantao to comment on whether host
> bridges should be treated any different from normal PCI devices.

I was able to try on an Intel box to try to make sure that I am not breaking anything.
But it would be nice if Xiantao could also help testing this.

Thank you,

Suravee
>
> Jan
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
  2013-08-07 15:31   ` Suravee Suthikulanit
@ 2013-08-07 15:42     ` Jan Beulich
  2013-08-07 19:20       ` Suravee Suthikulanit
  0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2013-08-07 15:42 UTC (permalink / raw)
  To: Suravee Suthikulanit; +Cc: xen-devel, xiantao.zhang

>>> On 07.08.13 at 17:31, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote:
> On 8/7/2013 2:33 AM, Jan Beulich wrote:
>>>>> On 07.08.13 at 04:40, <suravee.suthikulpanit@amd.com> wrote:
>>> +        /* Filter the bridge devices */
>>> +        if ( (pdev->type == DEV_TYPE_PCIe_BRIDGE)
>>> +        ||   (pdev->type == DEV_TYPE_PCIe2PCI_BRIDGE)
>>> +        ||   (pdev->type == DEV_TYPE_LEGACY_PCI_BRIDGE)
>>> +        ||   (pdev->type == DEV_TYPE_PCI_HOST_BRIDGE) )
>>> +        {
>>> +            AMD_IOMMU_DEBUG("Skipping device %04x:%02x:%02x.%u (type %x)\n",
>>> +                        pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
>>> +                        pdev->type);
>> I can see why host bridges can be skipped, but is this really also true
>> for other bridge types, most importantly legacy ones?
> 
> I am using the same logic here as in Intel's version in the 
> driver/passthrough/vtd/iommu/domain_context_map.

No, not really: That code establishes a mapping for the upstream
bridge of a legacy PCI device when handling that device. Your
proposed code doesn't afaics. (This I understand is because
bridges aren't expected to get assigned to guests, and hence
otherwise the mapping for the bridge would never get established
for a device behind it for other than Dom0.)

Jan

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
  2013-08-07 15:42     ` Jan Beulich
@ 2013-08-07 19:20       ` Suravee Suthikulanit
  2013-08-08  6:38         ` Jan Beulich
  0 siblings, 1 reply; 18+ messages in thread
From: Suravee Suthikulanit @ 2013-08-07 19:20 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, xiantao.zhang


[-- Attachment #1.1: Type: text/plain, Size: 2132 bytes --]

On 8/7/2013 10:42 AM, Jan Beulich wrote:
>>>> On 07.08.13 at 17:31, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote:
>> On 8/7/2013 2:33 AM, Jan Beulich wrote:
>>>>>> On 07.08.13 at 04:40, <suravee.suthikulpanit@amd.com> wrote:
>>>> +        /* Filter the bridge devices */
>>>> +        if ( (pdev->type == DEV_TYPE_PCIe_BRIDGE)
>>>> +        ||   (pdev->type == DEV_TYPE_PCIe2PCI_BRIDGE)
>>>> +        ||   (pdev->type == DEV_TYPE_LEGACY_PCI_BRIDGE)
>>>> +        ||   (pdev->type == DEV_TYPE_PCI_HOST_BRIDGE) )
>>>> +        {
>>>> +            AMD_IOMMU_DEBUG("Skipping device %04x:%02x:%02x.%u (type %x)\n",
>>>> +                        pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
>>>> +                        pdev->type);
>>> I can see why host bridges can be skipped, but is this really also true
>>> for other bridge types, most importantly legacy ones?
>> I am using the same logic here as in Intel's version in the
>> driver/passthrough/vtd/iommu/domain_context_map.
> No, not really: That code establishes a mapping for the upstream
> bridge of a legacy PCI device when handling that device. Your
> proposed code doesn't afaics. (This I understand is because
> bridges aren't expected to get assigned to guests, and hence
> otherwise the mapping for the bridge would never get established
> for a device behind it for other than Dom0.)
>
> Jan
Jan,

AFAICT from the domain_context_mapping() below, which get called when 
the devices are assigned to domains

static int domain_context_mapping(
     struct domain *domain, u8 devfn, const struct pci_dev *pdev)
{
     struct acpi_drhd_unit *drhd;
     int ret = 0;
     u8 seg = pdev->seg, bus = pdev->bus, secbus;

     drhd = acpi_find_matched_drhd_unit(pdev);
     if ( !drhd )
         return -ENODEV;

     ASSERT(spin_is_locked(&pcidevs_lock));

     switch ( pdev->type )
     {
     case DEV_TYPE_PCIe_BRIDGE:
     case DEV_TYPE_PCIe2PCI_BRIDGE:
     case DEV_TYPE_LEGACY_PCI_BRIDGE:
         break;

These bridges are actually not mapped(i.e. skipped).  That is why I 
think the logic above is supposed to be doing the same thing.

Suravee

[-- Attachment #1.2: Type: text/html, Size: 4479 bytes --]

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

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
  2013-08-07 19:20       ` Suravee Suthikulanit
@ 2013-08-08  6:38         ` Jan Beulich
  2013-08-30  1:25           ` Suravee Suthikulpanit
  0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2013-08-08  6:38 UTC (permalink / raw)
  To: Suravee Suthikulanit; +Cc: xen-devel, xiantao.zhang

>>> On 07.08.13 at 21:20, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote:
> On 8/7/2013 10:42 AM, Jan Beulich wrote:
>>>>> On 07.08.13 at 17:31, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote:
>>> I am using the same logic here as in Intel's version in the
>>> driver/passthrough/vtd/iommu/domain_context_map.
>> No, not really: That code establishes a mapping for the upstream
>> bridge of a legacy PCI device when handling that device. Your
>> proposed code doesn't afaics. (This I understand is because
>> bridges aren't expected to get assigned to guests, and hence
>> otherwise the mapping for the bridge would never get established
>> for a device behind it for other than Dom0.)
> 
> AFAICT from the domain_context_mapping() below, which get called when 
> the devices are assigned to domains
> 
> static int domain_context_mapping(
>      struct domain *domain, u8 devfn, const struct pci_dev *pdev)
> {
>      struct acpi_drhd_unit *drhd;
>      int ret = 0;
>      u8 seg = pdev->seg, bus = pdev->bus, secbus;
> 
>      drhd = acpi_find_matched_drhd_unit(pdev);
>      if ( !drhd )
>          return -ENODEV;
> 
>      ASSERT(spin_is_locked(&pcidevs_lock));
> 
>      switch ( pdev->type )
>      {
>      case DEV_TYPE_PCIe_BRIDGE:
>      case DEV_TYPE_PCIe2PCI_BRIDGE:
>      case DEV_TYPE_LEGACY_PCI_BRIDGE:
>          break;
> 
> These bridges are actually not mapped(i.e. skipped).  That is why I 
> think the logic above is supposed to be doing the same thing.

Just look a few lines down: There the bridges will get mappings
established when a device behind them gets mapped.

But since devices can't be behind host bridges, excluding those
_may_ still be appropriate/desirable. Still waiting for Xiantao to
voice his opinion...

Jan

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
  2013-08-07  8:33   ` Stefan Bader
@ 2013-08-08 11:12     ` Stefan Bader
  2013-08-08 11:49       ` Jan Beulich
  0 siblings, 1 reply; 18+ messages in thread
From: Stefan Bader @ 2013-08-08 11:12 UTC (permalink / raw)
  To: Suravee Suthikulanit; +Cc: JBeulich, xiantao.zhang, xen-devel


[-- Attachment #1.1.1: Type: text/plain, Size: 5795 bytes --]

On 07.08.2013 10:33, Stefan Bader wrote:

>>
> I will give it a spin, but might not b before tomorrow.
> 
> -Stefan

So I went ahead and modified Suravee's patch according to what I think Jan was
suggesting. This seems to be working and preventing the messages without showing
any immediately visible problems.

-Stefan

---

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Date: Tue, 6 Aug 2013 21:40:08 -0500
Subject: [Xen-devel] [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed

The host bridge device (i.e. 0x18 for AMD) does not
require IOMMU, and therefore is not included in the IVRS.
The current logic tries to map all PCI devices to an IOMMU.
In this case, "xl dmesg" shows the following message on AMD sytem.

(XEN) setup 0000:00:18.0 for d0 failed (-19)
(XEN) setup 0000:00:18.1 for d0 failed (-19)
(XEN) setup 0000:00:18.2 for d0 failed (-19)
(XEN) setup 0000:00:18.3 for d0 failed (-19)
(XEN) setup 0000:00:18.4 for d0 failed (-19)
(XEN) setup 0000:00:18.5 for d0 failed (-19)

This patch add new device type (i.e. DEV_TYPE_PCI_HOST_BRIDGE) which
is corresponded to PCI class code 0x06 and sub-class 0x00. Then, it
use this new type to filter when trying to map device to IOMMU.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/drivers/passthrough/amd/pci_amd_iommu.c |   20 +++++++++++++----
 xen/drivers/passthrough/pci.c               |   31 ++++++++++++++++-----------
 xen/drivers/passthrough/vtd/iommu.c         |    2 ++
 xen/include/xen/pci.h                       |    1 +
 4 files changed, 38 insertions(+), 16 deletions(-)

Index: xen-4.3.0/xen/drivers/passthrough/amd/pci_amd_iommu.c
===================================================================
--- xen-4.3.0.orig/xen/drivers/passthrough/amd/pci_amd_iommu.c	2013-08-08
09:34:30.572474632 +0200
+++ xen-4.3.0/xen/drivers/passthrough/amd/pci_amd_iommu.c	2013-08-08
10:04:55.056397984 +0200
@@ -175,9 +175,20 @@ static int __init amd_iommu_setup_dom0_d

     if ( unlikely(!iommu) )
     {
+        /* Filter the bridge devices */
+        if ( (pdev->type == DEV_TYPE_PCIe_BRIDGE)
+             || (pdev->type == DEV_TYPE_PCIe2PCI_BRIDGE)
+             || (pdev->type == DEV_TYPE_LEGACY_PCI_BRIDGE)
+             || (pdev->type == DEV_TYPE_PCI_HOST_BRIDGE) )
+        {
+            AMD_IOMMU_DEBUG("Skipping bridge %04x:%02x:%02x.%u (type %x)\n",
+                        pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
+                        pdev->type);
+            return 0;
+        }
         AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n",
                         pdev->seg, pdev->bus,
-                        PCI_SLOT(devfn), PCI_FUNC(devfn));
+                        PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
         return -ENODEV;
     }

Index: xen-4.3.0/xen/drivers/passthrough/pci.c
===================================================================
--- xen-4.3.0.orig/xen/drivers/passthrough/pci.c	2013-08-08 09:34:30.572474632 +0200
+++ xen-4.3.0/xen/drivers/passthrough/pci.c	2013-08-08 10:02:34.472403890 +0200
@@ -189,9 +189,6 @@ static struct pci_dev *alloc_pdev(struct
         u16 cap;
         u8 sec_bus, sub_bus;

-        case DEV_TYPE_PCIe_BRIDGE:
-            break;
-
         case DEV_TYPE_PCIe2PCI_BRIDGE:
         case DEV_TYPE_LEGACY_PCI_BRIDGE:
             sec_bus = pci_conf_read8(pseg->nr, bus, PCI_SLOT(devfn),
@@ -239,6 +236,8 @@ static struct pci_dev *alloc_pdev(struct
             break;

         case DEV_TYPE_PCI:
+        case DEV_TYPE_PCI_HOST_BRIDGE:
+        case DEV_TYPE_PCIe_BRIDGE:
             break;

         default:
@@ -691,7 +690,8 @@ void pci_release_devices(struct domain *
     spin_unlock(&pcidevs_lock);
 }

-#define PCI_CLASS_BRIDGE_PCI     0x0604
+#define PCI_CLASS_PCI_HOST_BRIDGE	0x0600
+#define PCI_CLASS_BRIDGE_PCI		0x0604

 enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn)
 {
@@ -715,6 +715,9 @@ enum pdev_type pdev_type(u16 seg, u8 bus
         }
         return DEV_TYPE_PCIe_BRIDGE;

+    case PCI_CLASS_PCI_HOST_BRIDGE:
+        return DEV_TYPE_PCI_HOST_BRIDGE;
+
     case 0x0000: case 0xffff:
         return DEV_TYPE_PCI_UNKNOWN;
     }
Index: xen-4.3.0/xen/drivers/passthrough/vtd/iommu.c
===================================================================
--- xen-4.3.0.orig/xen/drivers/passthrough/vtd/iommu.c	2013-08-08
09:34:30.572474632 +0200
+++ xen-4.3.0/xen/drivers/passthrough/vtd/iommu.c	2013-08-08 09:34:30.564474633
+0200
@@ -1447,6 +1447,7 @@ static int domain_context_mapping(
         break;

     case DEV_TYPE_PCI:
+    case DEV_TYPE_PCI_HOST_BRIDGE:
         if ( iommu_verbose )
             dprintk(VTDPREFIX, "d%d:PCI: map %04x:%02x:%02x.%u\n",
                     domain->domain_id, seg, bus,
@@ -1576,6 +1577,7 @@ static int domain_context_unmap(
         break;

     case DEV_TYPE_PCI:
+    case DEV_TYPE_PCI_HOST_BRIDGE:
         if ( iommu_verbose )
             dprintk(VTDPREFIX, "d%d:PCI: unmap %04x:%02x:%02x.%u\n",
                     domain->domain_id, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
Index: xen-4.3.0/xen/include/xen/pci.h
===================================================================
--- xen-4.3.0.orig/xen/include/xen/pci.h	2013-08-08 09:34:30.572474632 +0200
+++ xen-4.3.0/xen/include/xen/pci.h	2013-08-08 09:34:30.564474633 +0200
@@ -73,6 +73,7 @@ struct pci_dev {
         DEV_TYPE_PCIe2PCI_BRIDGE,   // PCIe-to-PCI/PCIx bridge
         DEV_TYPE_PCI2PCIe_BRIDGE,   // PCI/PCIx-to-PCIe bridge
         DEV_TYPE_LEGACY_PCI_BRIDGE, // Legacy PCI bridge
+        DEV_TYPE_PCI_HOST_BRIDGE,   // PCI Host bridge
         DEV_TYPE_PCI,
     } type;


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.1.2: xen-amd-bridge.patch --]
[-- Type: text/x-diff; name="xen-amd-bridge.patch", Size: 5462 bytes --]

From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Date: Tue, 6 Aug 2013 21:40:08 -0500
Subject: [Xen-devel] [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed

The host bridge device (i.e. 0x18 for AMD) does not
require IOMMU, and therefore is not included in the IVRS.
The current logic tries to map all PCI devices to an IOMMU. 
In this case, "xl dmesg" shows the following message on AMD sytem.

(XEN) setup 0000:00:18.0 for d0 failed (-19)
(XEN) setup 0000:00:18.1 for d0 failed (-19)
(XEN) setup 0000:00:18.2 for d0 failed (-19)
(XEN) setup 0000:00:18.3 for d0 failed (-19)
(XEN) setup 0000:00:18.4 for d0 failed (-19)
(XEN) setup 0000:00:18.5 for d0 failed (-19)

This patch add new device type (i.e. DEV_TYPE_PCI_HOST_BRIDGE) which
is corresponded to PCI class code 0x06 and sub-class 0x00. Then, it
use this new type to filter when trying to map device to IOMMU.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 xen/drivers/passthrough/amd/pci_amd_iommu.c |   20 +++++++++++++----
 xen/drivers/passthrough/pci.c               |   31 ++++++++++++++++-----------
 xen/drivers/passthrough/vtd/iommu.c         |    2 ++
 xen/include/xen/pci.h                       |    1 +
 4 files changed, 38 insertions(+), 16 deletions(-)

Index: xen-4.3.0/xen/drivers/passthrough/amd/pci_amd_iommu.c
===================================================================
--- xen-4.3.0.orig/xen/drivers/passthrough/amd/pci_amd_iommu.c	2013-08-08 09:34:30.572474632 +0200
+++ xen-4.3.0/xen/drivers/passthrough/amd/pci_amd_iommu.c	2013-08-08 10:04:55.056397984 +0200
@@ -175,9 +175,20 @@ static int __init amd_iommu_setup_dom0_d
 
     if ( unlikely(!iommu) )
     {
+        /* Filter the bridge devices */
+        if ( (pdev->type == DEV_TYPE_PCIe_BRIDGE)
+             || (pdev->type == DEV_TYPE_PCIe2PCI_BRIDGE)
+             || (pdev->type == DEV_TYPE_LEGACY_PCI_BRIDGE)
+             || (pdev->type == DEV_TYPE_PCI_HOST_BRIDGE) )
+        {
+            AMD_IOMMU_DEBUG("Skipping bridge %04x:%02x:%02x.%u (type %x)\n",
+                        pdev->seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
+                        pdev->type);
+            return 0;
+        }
         AMD_IOMMU_DEBUG("No iommu for device %04x:%02x:%02x.%u\n",
                         pdev->seg, pdev->bus,
-                        PCI_SLOT(devfn), PCI_FUNC(devfn));
+                        PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
         return -ENODEV;
     }
 
Index: xen-4.3.0/xen/drivers/passthrough/pci.c
===================================================================
--- xen-4.3.0.orig/xen/drivers/passthrough/pci.c	2013-08-08 09:34:30.572474632 +0200
+++ xen-4.3.0/xen/drivers/passthrough/pci.c	2013-08-08 10:02:34.472403890 +0200
@@ -189,9 +189,6 @@ static struct pci_dev *alloc_pdev(struct
         u16 cap;
         u8 sec_bus, sub_bus;
 
-        case DEV_TYPE_PCIe_BRIDGE:
-            break;
-
         case DEV_TYPE_PCIe2PCI_BRIDGE:
         case DEV_TYPE_LEGACY_PCI_BRIDGE:
             sec_bus = pci_conf_read8(pseg->nr, bus, PCI_SLOT(devfn),
@@ -239,6 +236,8 @@ static struct pci_dev *alloc_pdev(struct
             break;
 
         case DEV_TYPE_PCI:
+        case DEV_TYPE_PCI_HOST_BRIDGE:
+        case DEV_TYPE_PCIe_BRIDGE:
             break;
 
         default:
@@ -691,7 +690,8 @@ void pci_release_devices(struct domain *
     spin_unlock(&pcidevs_lock);
 }
 
-#define PCI_CLASS_BRIDGE_PCI     0x0604
+#define PCI_CLASS_PCI_HOST_BRIDGE	0x0600
+#define PCI_CLASS_BRIDGE_PCI		0x0604
 
 enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn)
 {
@@ -715,6 +715,9 @@ enum pdev_type pdev_type(u16 seg, u8 bus
         }
         return DEV_TYPE_PCIe_BRIDGE;
 
+    case PCI_CLASS_PCI_HOST_BRIDGE:
+        return DEV_TYPE_PCI_HOST_BRIDGE;
+
     case 0x0000: case 0xffff:
         return DEV_TYPE_PCI_UNKNOWN;
     }
Index: xen-4.3.0/xen/drivers/passthrough/vtd/iommu.c
===================================================================
--- xen-4.3.0.orig/xen/drivers/passthrough/vtd/iommu.c	2013-08-08 09:34:30.572474632 +0200
+++ xen-4.3.0/xen/drivers/passthrough/vtd/iommu.c	2013-08-08 09:34:30.564474633 +0200
@@ -1447,6 +1447,7 @@ static int domain_context_mapping(
         break;
 
     case DEV_TYPE_PCI:
+    case DEV_TYPE_PCI_HOST_BRIDGE:
         if ( iommu_verbose )
             dprintk(VTDPREFIX, "d%d:PCI: map %04x:%02x:%02x.%u\n",
                     domain->domain_id, seg, bus,
@@ -1576,6 +1577,7 @@ static int domain_context_unmap(
         break;
 
     case DEV_TYPE_PCI:
+    case DEV_TYPE_PCI_HOST_BRIDGE:
         if ( iommu_verbose )
             dprintk(VTDPREFIX, "d%d:PCI: unmap %04x:%02x:%02x.%u\n",
                     domain->domain_id, seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
Index: xen-4.3.0/xen/include/xen/pci.h
===================================================================
--- xen-4.3.0.orig/xen/include/xen/pci.h	2013-08-08 09:34:30.572474632 +0200
+++ xen-4.3.0/xen/include/xen/pci.h	2013-08-08 09:34:30.564474633 +0200
@@ -73,6 +73,7 @@ struct pci_dev {
         DEV_TYPE_PCIe2PCI_BRIDGE,   // PCIe-to-PCI/PCIx bridge
         DEV_TYPE_PCI2PCIe_BRIDGE,   // PCI/PCIx-to-PCIe bridge
         DEV_TYPE_LEGACY_PCI_BRIDGE, // Legacy PCI bridge
+        DEV_TYPE_PCI_HOST_BRIDGE,   // PCI Host bridge
         DEV_TYPE_PCI,
     } type;
 

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

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

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
  2013-08-08 11:12     ` Stefan Bader
@ 2013-08-08 11:49       ` Jan Beulich
  2013-08-08 12:07         ` Stefan Bader
  0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2013-08-08 11:49 UTC (permalink / raw)
  To: Stefan Bader; +Cc: xiantao.zhang, Suravee Suthikulanit, xen-devel

>>> On 08.08.13 at 13:12, Stefan Bader <stefan.bader@canonical.com> wrote:
> On 07.08.2013 10:33, Stefan Bader wrote:
> So I went ahead and modified Suravee's patch according to what I think Jan 
> was suggesting.

Mostly.

> This seems to be working and preventing the messages without 
> showing any immediately visible problems.

Question is - did you in particular try with a legacy PCI device
behind a legacy PCI bridge? That's the main scenario where the
bridge not having got mapped could result in problems (see my
other reply to Suravee on this thread).

Jan

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
  2013-08-08 11:49       ` Jan Beulich
@ 2013-08-08 12:07         ` Stefan Bader
  2013-08-08 12:29           ` Jan Beulich
  0 siblings, 1 reply; 18+ messages in thread
From: Stefan Bader @ 2013-08-08 12:07 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xiantao.zhang, Suravee Suthikulanit, xen-devel


[-- Attachment #1.1: Type: text/plain, Size: 2719 bytes --]

On 08.08.2013 13:49, Jan Beulich wrote:
>>>> On 08.08.13 at 13:12, Stefan Bader <stefan.bader@canonical.com> wrote:
>> On 07.08.2013 10:33, Stefan Bader wrote:
>> So I went ahead and modified Suravee's patch according to what I think Jan 
>> was suggesting.
> 
> Mostly.
> 
>> This seems to be working and preventing the messages without 
>> showing any immediately visible problems.
> 
> Question is - did you in particular try with a legacy PCI device
> behind a legacy PCI bridge? That's the main scenario where the
> bridge not having got mapped could result in problems (see my
> other reply to Suravee on this thread).
> 
> Jan
> 

Not sure whether I confuse things here. Would the VGA card be what you meant?

-Stefan

-[0000:00]-+-00.0  Advanced Micro Devices, Inc. [AMD/ATI] RD890 PCI to PCI bridg
e (external gfx0 port A) [1002:5a13]
           ...
           +-14.4-[04]----04.0  Matrox Electronics Systems Ltd. MGA G200eW
WPCM450 [102b:0532]

00:14.4 PCI bridge [0604]: Advanced Micro Devices, Inc. [AMD/ATI] SBx00 PCI to
PCI Bridge [1002:4384] (prog-if 01 [Subtractive decode])
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping-
SERR+ FastB2B- DisINTx-
	Status: Cap- 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort-
<MAbort- >SERR- <PERR- INTx-
	Latency: 64
	Bus: primary=00, secondary=04, subordinate=04, sec-latency=64
	I/O behind bridge: 0000f000-00000fff
	Memory behind bridge: fdf00000-fe7fffff
	Prefetchable memory behind bridge: fc000000-fcffffff
	Secondary status: 66MHz- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort-
<MAbort+ <SERR- <PERR-
	BridgeCtl: Parity- SERR+ NoISA- VGA+ MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-

04:04.0 VGA compatible controller [0300]: Matrox Electronics Systems Ltd. MGA
G200eW WPCM450 [102b:0532] (rev 0a) (prog-if 00 [VGA controller])
	Subsystem: Super Micro Computer Inc Device [15d9:a711]
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping-
SERR- FastB2B- DisINTx-
	Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort-
<MAbort- >SERR- <PERR- INTx-
	Latency: 64 (4000ns min, 8000ns max), Cache Line Size: 64 bytes
	Interrupt: pin A routed to IRQ 10
	Region 0: Memory at fc000000 (32-bit, prefetchable) [size=16M]
	Region 1: Memory at fdffc000 (32-bit, non-prefetchable) [size=16K]
	Region 2: Memory at fe000000 (32-bit, non-prefetchable) [size=8M]
	Expansion ROM at <unassigned> [disabled]
	Capabilities: [dc] Power Management version 1
		Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-




[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

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

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
  2013-08-08 12:07         ` Stefan Bader
@ 2013-08-08 12:29           ` Jan Beulich
  2013-08-08 12:35             ` Stefan Bader
  0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2013-08-08 12:29 UTC (permalink / raw)
  To: Stefan Bader; +Cc: xiantao.zhang, Suravee Suthikulanit, xen-devel

>>> On 08.08.13 at 14:07, Stefan Bader <stefan.bader@canonical.com> wrote:
> On 08.08.2013 13:49, Jan Beulich wrote:
>> Question is - did you in particular try with a legacy PCI device
>> behind a legacy PCI bridge? That's the main scenario where the
>> bridge not having got mapped could result in problems (see my
>> other reply to Suravee on this thread).
> 
> Not sure whether I confuse things here. Would the VGA card be what you 
> meant?

If the output you provided was complete, then this really seems to
be a legacy PCI device, so yes. But please realize that this working
for you is not a guarantee that it's working in general - whether
requests get forwarded by legacy bridges with the original requester
ID or the bridge's is subject to the bridge implementation afaik. So a
theoretical answer is going to be needed here anyway... But thanks
for clarifying!

Jan

> -[0000:00]-+-00.0  Advanced Micro Devices, Inc. [AMD/ATI] RD890 PCI to PCI 
> bridg
> e (external gfx0 port A) [1002:5a13]
>            ...
>            +-14.4-[04]----04.0  Matrox Electronics Systems Ltd. MGA G200eW
> WPCM450 [102b:0532]
> 
> 00:14.4 PCI bridge [0604]: Advanced Micro Devices, Inc. [AMD/ATI] SBx00 PCI 
> to
> PCI Bridge [1002:4384] (prog-if 01 [Subtractive decode])
> 	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping-
> SERR+ FastB2B- DisINTx-
> 	Status: Cap- 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort-
> <MAbort- >SERR- <PERR- INTx-
> 	Latency: 64
> 	Bus: primary=00, secondary=04, subordinate=04, sec-latency=64
> 	I/O behind bridge: 0000f000-00000fff
> 	Memory behind bridge: fdf00000-fe7fffff
> 	Prefetchable memory behind bridge: fc000000-fcffffff
> 	Secondary status: 66MHz- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort-
> <MAbort+ <SERR- <PERR-
> 	BridgeCtl: Parity- SERR+ NoISA- VGA+ MAbort- >Reset- FastB2B-
> 		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
> 
> 04:04.0 VGA compatible controller [0300]: Matrox Electronics Systems Ltd. 
> MGA
> G200eW WPCM450 [102b:0532] (rev 0a) (prog-if 00 [VGA controller])
> 	Subsystem: Super Micro Computer Inc Device [15d9:a711]
> 	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping-
> SERR- FastB2B- DisINTx-
> 	Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort-
> <MAbort- >SERR- <PERR- INTx-
> 	Latency: 64 (4000ns min, 8000ns max), Cache Line Size: 64 bytes
> 	Interrupt: pin A routed to IRQ 10
> 	Region 0: Memory at fc000000 (32-bit, prefetchable) [size=16M]
> 	Region 1: Memory at fdffc000 (32-bit, non-prefetchable) [size=16K]
> 	Region 2: Memory at fe000000 (32-bit, non-prefetchable) [size=8M]
> 	Expansion ROM at <unassigned> [disabled]
> 	Capabilities: [dc] Power Management version 1
> 		Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
> 		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
  2013-08-08 12:29           ` Jan Beulich
@ 2013-08-08 12:35             ` Stefan Bader
  0 siblings, 0 replies; 18+ messages in thread
From: Stefan Bader @ 2013-08-08 12:35 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xiantao.zhang, Suravee Suthikulanit, xen-devel


[-- Attachment #1.1: Type: text/plain, Size: 3227 bytes --]

On 08.08.2013 14:29, Jan Beulich wrote:
>>>> On 08.08.13 at 14:07, Stefan Bader <stefan.bader@canonical.com> wrote:
>> On 08.08.2013 13:49, Jan Beulich wrote:
>>> Question is - did you in particular try with a legacy PCI device
>>> behind a legacy PCI bridge? That's the main scenario where the
>>> bridge not having got mapped could result in problems (see my
>>> other reply to Suravee on this thread).
>>
>> Not sure whether I confuse things here. Would the VGA card be what you 
>> meant?
> 
> If the output you provided was complete, then this really seems to
> be a legacy PCI device, so yes. But please realize that this working
> for you is not a guarantee that it's working in general - whether
> requests get forwarded by legacy bridges with the original requester
> ID or the bridge's is subject to the bridge implementation afaik. So a
> theoretical answer is going to be needed here anyway... But thanks
> for clarifying!

No, sure it is only one piece of the puzzle and a completing theoretical answer
is needed. Would not want to rely on any single testing.

-Stefan
> 
> Jan
> 
>> -[0000:00]-+-00.0  Advanced Micro Devices, Inc. [AMD/ATI] RD890 PCI to PCI 
>> bridg
>> e (external gfx0 port A) [1002:5a13]
>>            ...
>>            +-14.4-[04]----04.0  Matrox Electronics Systems Ltd. MGA G200eW
>> WPCM450 [102b:0532]
>>
>> 00:14.4 PCI bridge [0604]: Advanced Micro Devices, Inc. [AMD/ATI] SBx00 PCI 
>> to
>> PCI Bridge [1002:4384] (prog-if 01 [Subtractive decode])
>> 	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping-
>> SERR+ FastB2B- DisINTx-
>> 	Status: Cap- 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort-
>> <MAbort- >SERR- <PERR- INTx-
>> 	Latency: 64
>> 	Bus: primary=00, secondary=04, subordinate=04, sec-latency=64
>> 	I/O behind bridge: 0000f000-00000fff
>> 	Memory behind bridge: fdf00000-fe7fffff
>> 	Prefetchable memory behind bridge: fc000000-fcffffff
>> 	Secondary status: 66MHz- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort-
>> <MAbort+ <SERR- <PERR-
>> 	BridgeCtl: Parity- SERR+ NoISA- VGA+ MAbort- >Reset- FastB2B-
>> 		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
>>
>> 04:04.0 VGA compatible controller [0300]: Matrox Electronics Systems Ltd. 
>> MGA
>> G200eW WPCM450 [102b:0532] (rev 0a) (prog-if 00 [VGA controller])
>> 	Subsystem: Super Micro Computer Inc Device [15d9:a711]
>> 	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping-
>> SERR- FastB2B- DisINTx-
>> 	Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort-
>> <MAbort- >SERR- <PERR- INTx-
>> 	Latency: 64 (4000ns min, 8000ns max), Cache Line Size: 64 bytes
>> 	Interrupt: pin A routed to IRQ 10
>> 	Region 0: Memory at fc000000 (32-bit, prefetchable) [size=16M]
>> 	Region 1: Memory at fdffc000 (32-bit, non-prefetchable) [size=16K]
>> 	Region 2: Memory at fe000000 (32-bit, non-prefetchable) [size=8M]
>> 	Expansion ROM at <unassigned> [disabled]
>> 	Capabilities: [dc] Power Management version 1
>> 		Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-)
>> 		Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
> 
> 
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

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

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
  2013-08-08  6:38         ` Jan Beulich
@ 2013-08-30  1:25           ` Suravee Suthikulpanit
  2013-08-30  8:09             ` Jan Beulich
  0 siblings, 1 reply; 18+ messages in thread
From: Suravee Suthikulpanit @ 2013-08-30  1:25 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, xiantao.zhang

On 8/8/2013 1:38 AM, Jan Beulich wrote:
>>>> On 07.08.13 at 21:20, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote:
>> On 8/7/2013 10:42 AM, Jan Beulich wrote:
>>>>>> On 07.08.13 at 17:31, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote:
>>>> I am using the same logic here as in Intel's version in the
>>>> driver/passthrough/vtd/iommu/domain_context_map.
>>> No, not really: That code establishes a mapping for the upstream
>>> bridge of a legacy PCI device when handling that device. Your
>>> proposed code doesn't afaics. (This I understand is because
>>> bridges aren't expected to get assigned to guests, and hence
>>> otherwise the mapping for the bridge would never get established
>>> for a device behind it for other than Dom0.)
>> AFAICT from the domain_context_mapping() below, which get called when
>> the devices are assigned to domains
>>
>> static int domain_context_mapping(
>>       struct domain *domain, u8 devfn, const struct pci_dev *pdev)
>> {
>>       struct acpi_drhd_unit *drhd;
>>       int ret = 0;
>>       u8 seg = pdev->seg, bus = pdev->bus, secbus;
>>
>>       drhd = acpi_find_matched_drhd_unit(pdev);
>>       if ( !drhd )
>>           return -ENODEV;
>>
>>       ASSERT(spin_is_locked(&pcidevs_lock));
>>
>>       switch ( pdev->type )
>>       {
>>       case DEV_TYPE_PCIe_BRIDGE:
>>       case DEV_TYPE_PCIe2PCI_BRIDGE:
>>       case DEV_TYPE_LEGACY_PCI_BRIDGE:
>>           break;
>>
>> These bridges are actually not mapped(i.e. skipped).  That is why I
>> think the logic above is supposed to be doing the same thing.
> Just look a few lines down: There the bridges will get mappings
> established when a device behind them gets mapped.
>
> But since devices can't be behind host bridges, excluding those
> _may_ still be appropriate/desirable. Still waiting for Xiantao to
> voice his opinion...
>
> Jan
Sorry for didn't get a chance to follow up with this sooner.  Ok, I see 
the code you mentioned.
However, if I understand this correctly, it is also mapping the bridge 
to the domU
along with the end-device.  This is not the same as what you mentioned 
that the bridge should
be mapped to only Dom0.

Suravee

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
  2013-08-30  1:25           ` Suravee Suthikulpanit
@ 2013-08-30  8:09             ` Jan Beulich
  2013-08-31  0:03               ` Suravee Suthikulpanit
  0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2013-08-30  8:09 UTC (permalink / raw)
  To: Suravee Suthikulpanit; +Cc: xen-devel, xiantao.zhang

>>> On 30.08.13 at 03:25, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
wrote:
> On 8/8/2013 1:38 AM, Jan Beulich wrote:
>>>>> On 07.08.13 at 21:20, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> 
> wrote:
>>> On 8/7/2013 10:42 AM, Jan Beulich wrote:
>>>>>>> On 07.08.13 at 17:31, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> 
> wrote:
>>>>> I am using the same logic here as in Intel's version in the
>>>>> driver/passthrough/vtd/iommu/domain_context_map.
>>>> No, not really: That code establishes a mapping for the upstream
>>>> bridge of a legacy PCI device when handling that device. Your
>>>> proposed code doesn't afaics. (This I understand is because
>>>> bridges aren't expected to get assigned to guests, and hence
>>>> otherwise the mapping for the bridge would never get established
>>>> for a device behind it for other than Dom0.)
>>> AFAICT from the domain_context_mapping() below, which get called when
>>> the devices are assigned to domains
>>>
>>> static int domain_context_mapping(
>>>       struct domain *domain, u8 devfn, const struct pci_dev *pdev)
>>> {
>>>       struct acpi_drhd_unit *drhd;
>>>       int ret = 0;
>>>       u8 seg = pdev->seg, bus = pdev->bus, secbus;
>>>
>>>       drhd = acpi_find_matched_drhd_unit(pdev);
>>>       if ( !drhd )
>>>           return -ENODEV;
>>>
>>>       ASSERT(spin_is_locked(&pcidevs_lock));
>>>
>>>       switch ( pdev->type )
>>>       {
>>>       case DEV_TYPE_PCIe_BRIDGE:
>>>       case DEV_TYPE_PCIe2PCI_BRIDGE:
>>>       case DEV_TYPE_LEGACY_PCI_BRIDGE:
>>>           break;
>>>
>>> These bridges are actually not mapped(i.e. skipped).  That is why I
>>> think the logic above is supposed to be doing the same thing.
>> Just look a few lines down: There the bridges will get mappings
>> established when a device behind them gets mapped.
>>
>> But since devices can't be behind host bridges, excluding those
>> _may_ still be appropriate/desirable. Still waiting for Xiantao to
>> voice his opinion...
>>
> Sorry for didn't get a chance to follow up with this sooner.  Ok, I see 
> the code you mentioned.
> However, if I understand this correctly, it is also mapping the bridge 
> to the domU
> along with the end-device.  This is not the same as what you mentioned 
> that the bridge should
> be mapped to only Dom0.

I don't think I ever said this. Whether we're talking about DomU or
Dom0 here is irrelevant. I'm just pointing out that bridges do get
mappings established for them whenever a device behind them gets
mapped.

Jan

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
  2013-08-30  8:09             ` Jan Beulich
@ 2013-08-31  0:03               ` Suravee Suthikulpanit
  0 siblings, 0 replies; 18+ messages in thread
From: Suravee Suthikulpanit @ 2013-08-31  0:03 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, xiantao.zhang


[-- Attachment #1.1: Type: text/plain, Size: 1203 bytes --]

On 8/30/2013 3:09 AM, Jan Beulich wrote:
>> Sorry for didn't get a chance to follow up with this sooner.  Ok, I see
>> >the code you mentioned.
>> >However, if I understand this correctly, it is also mapping the bridge
>> >to the domU
>> >along with the end-device.  This is not the same as what you mentioned
>> >that the bridge should
>> >be mapped to only Dom0.
> I don't think I ever said this. Whether we're talking about DomU or
> Dom0 here is irrelevant. I'm just pointing out that bridges do get
> mappings established for them whenever a device behind them gets
> mapped.
>
> Jan
Ok, I think Iunderstand it a bit more now. I'll modify the code to skip 
only the hostbridge when they are not
specifiedin the IVRS as it is not needed to be mapped to adomain.  I'll 
send out the code shortly.

However, I also found out that the current AMD IOMMU driver maps other 
type of bridges
(i.e. DEV_TYPE_PCIe_BRIDGE, DEV_TYPE_PCIe2PCI_BRIDGE, and 
DEV_TYPE_LEGACY_PCI_BRIDGE) _only_ to Dom0,
and they won't get reassignedwhen the end point devices downstreamare 
assignedto other domain.
I don't think this is correct.I'll work on this and and send out a 
separate patch for this.

Thank you,

Suravee


[-- Attachment #1.2: Type: text/html, Size: 5039 bytes --]

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

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2013-08-31  0:04 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-07  2:40 [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed suravee.suthikulpanit
2013-08-07  2:42 ` Suravee Suthikulanit
2013-08-07  8:33   ` Stefan Bader
2013-08-08 11:12     ` Stefan Bader
2013-08-08 11:49       ` Jan Beulich
2013-08-08 12:07         ` Stefan Bader
2013-08-08 12:29           ` Jan Beulich
2013-08-08 12:35             ` Stefan Bader
2013-08-07  7:33 ` Jan Beulich
2013-08-07 15:31   ` Suravee Suthikulanit
2013-08-07 15:42     ` Jan Beulich
2013-08-07 19:20       ` Suravee Suthikulanit
2013-08-08  6:38         ` Jan Beulich
2013-08-30  1:25           ` Suravee Suthikulpanit
2013-08-30  8:09             ` Jan Beulich
2013-08-31  0:03               ` Suravee Suthikulpanit
2013-08-07  9:34 ` Andrew Cooper
2013-08-07  9:57   ` Jan Beulich

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).