[PATCH 1/1 V2] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed

[PATCH 1/1 V2] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed

[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>
Reported-by: Stefan Bader <stefan.bader@canonical.com>
---
Changes for V2:
	- Various clean up (per Jan comments).
	- Add device type in the debug message
	- Only skipping host bridge

 xen/drivers/passthrough/amd/pci_amd_iommu.c | 14 ++++++++++++--
 xen/drivers/passthrough/pci.c               |  9 +++++----
 xen/drivers/passthrough/vtd/intremap.c      |  1 +
 xen/drivers/passthrough/vtd/iommu.c         |  2 ++
 xen/include/xen/pci.h                       |  1 +
 5 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 9684ae8..e352a02 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -147,9 +147,10 @@ static void amd_iommu_setup_domain_device(
 
         amd_iommu_flush_device(iommu, req_id);
 
-        AMD_IOMMU_DEBUG("Setup I/O page table: device id = %#x, "
+        AMD_IOMMU_DEBUG("Setup I/O page table: device id = %#x, type = %x, "
                         "root table = %#"PRIx64", "
-                        "domain = %d, paging mode = %d\n", req_id,
+                        "domain = %d, paging mode = %d\n",
+                        req_id, pdev->type,
                         page_to_maddr(hd->root_table),
                         hd->domain_id, hd->paging_mode);
     }
@@ -175,6 +176,15 @@ static int __init amd_iommu_setup_dom0_device(u8 devfn, struct pci_dev *pdev)
 
     if ( unlikely(!iommu) )
     {
+        /* Filter the bridge devices */
+        if ( (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));
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index b488e2a..c151bc8 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -203,9 +203,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),
@@ -253,6 +250,8 @@ static struct pci_dev *alloc_pdev(struct pci_seg *pseg, u8 bus, u8 devfn)
             break;
 
         case DEV_TYPE_PCI:
+        case DEV_TYPE_PCIe_BRIDGE:
+        case DEV_TYPE_PCI_HOST_BRIDGE:
             break;
 
         default:
@@ -706,6 +705,7 @@ void pci_release_devices(struct domain *d)
     spin_unlock(&pcidevs_lock);
 }
 
+#define PCI_CLASS_BRIDGE_HOST    0x0600
 #define PCI_CLASS_BRIDGE_PCI     0x0604
 
 enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn)
@@ -729,7 +729,8 @@ enum pdev_type pdev_type(u16 seg, u8 bus, u8 devfn)
             return DEV_TYPE_PCI2PCIe_BRIDGE;
         }
         return DEV_TYPE_PCIe_BRIDGE;
-
+    case PCI_CLASS_BRIDGE_HOST:
+        return DEV_TYPE_PCI_HOST_BRIDGE;
     case 0x0000: case 0xffff:
         return DEV_TYPE_PCI_UNKNOWN;
     }
diff --git a/xen/drivers/passthrough/vtd/intremap.c b/xen/drivers/passthrough/vtd/intremap.c
index f3bb31b..e4ed7a4 100644
--- a/xen/drivers/passthrough/vtd/intremap.c
+++ b/xen/drivers/passthrough/vtd/intremap.c
@@ -453,6 +453,7 @@ static void set_msi_source_id(struct pci_dev *pdev, struct iremap_entry *ire)
         break;
 
     case DEV_TYPE_PCI:
+    case DEV_TYPE_PCI_HOST_BRIDGE:
     case DEV_TYPE_LEGACY_PCI_BRIDGE:
     case DEV_TYPE_PCI2PCIe_BRIDGE:
         ret = find_upstream_bridge(seg, &bus, &devfn, &secbus);
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index fd3abcb..817a352 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1451,6 +1451,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,
@@ -1580,6 +1581,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 c367736..d33a15d 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -63,6 +63,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.8.1.2

Re: [PATCH 1/1 V2] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed

[Andrew Cooper]

On 31/08/2013 01:41, suravee.suthikulpanit@amd.com wrote:
> @@ -175,6 +176,15 @@ static int __init amd_iommu_setup_dom0_device(u8 devfn, struct pci_dev *pdev)
>  
>      if ( unlikely(!iommu) )
>      {
> +        /* Filter the bridge devices */
> +        if ( (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);

I know this is being picky, but I would prefer "Skipping host bridge"
here, to avoid any possible confusion that non-host bridges might be
considered for being skipped.

~Andrew

Re: [PATCH 1/1 V2] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed

[Suravee Suthikulpanit]

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

On 9/1/2013 3:40 PM, Andrew Cooper wrote:
> On 31/08/2013 01:41, suravee.suthikulpanit@amd.com wrote:
>> @@ -175,6 +176,15 @@ static int __init amd_iommu_setup_dom0_device(u8 devfn, struct pci_dev *pdev)
>>   
>>       if ( unlikely(!iommu) )
>>       {
>> +        /* Filter the bridge devices */
>> +        if ( (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);
> I know this is being picky, but I would prefer "Skipping host bridge"
> here, to avoid any possible confusion that non-host bridges might be
> considered for being skipped.
>
> ~Andrew
>
I'm fine with that.  Jan, would you mind modifying the message when you 
commit the code, or you want me to send out a new patch?

Thanks.

Suravee

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

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

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

Re: [PATCH 1/1 V2] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed

[Jan Beulich]

>>> On 31.08.13 at 02:41, <suravee.suthikulpanit@amd.com> wrote:

While the non-VT-d changes all look fine, I'm all but certain the VT-d
ones are correct:

> --- a/xen/drivers/passthrough/vtd/intremap.c
> +++ b/xen/drivers/passthrough/vtd/intremap.c
> @@ -453,6 +453,7 @@ static void set_msi_source_id(struct pci_dev *pdev, 
> struct iremap_entry *ire)
>          break;
>  
>      case DEV_TYPE_PCI:
> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>      case DEV_TYPE_LEGACY_PCI_BRIDGE:
>      case DEV_TYPE_PCI2PCIe_BRIDGE:
>          ret = find_upstream_bridge(seg, &bus, &devfn, &secbus);

There shouldn't be an upstream bridge to a host bridge, and
hence adding the case here (rather than above) is at least
pointlessly running more complex code (all under the unlikely but
not impossible assumption that a host bridge would have MSI set
up for it in the first place).

Adding it together with DEV_TYPE_PCIe_ENDPOINT et al would
thus seem better.

> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1451,6 +1451,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,
> @@ -1580,6 +1581,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));

Host bridges would so far have gone through the respective default
cases, not setting up any mapping, but logging a message. I think
the change ought to be that host bridges result in both functions to
fail, but without the log message saying "unknown" (and perhaps
with -EPERM or -EACCES rather than -EINVAL).

But in any event - you forgot to Cc the VT-d maintainer, who will
need to ack the change anyway.

Jan

Re: [PATCH 1/1 V2] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed

[Jan Beulich]

>>> On 03.09.13 at 04:36, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
wrote:
> On 9/1/2013 3:40 PM, Andrew Cooper wrote:
>> On 31/08/2013 01:41, suravee.suthikulpanit@amd.com wrote:
>>> @@ -175,6 +176,15 @@ static int __init amd_iommu_setup_dom0_device(u8 devfn, 
> struct pci_dev *pdev)
>>>   
>>>       if ( unlikely(!iommu) )
>>>       {
>>> +        /* Filter the bridge devices */
>>> +        if ( (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);
>> I know this is being picky, but I would prefer "Skipping host bridge"
>> here, to avoid any possible confusion that non-host bridges might be
>> considered for being skipped.
>>
>> ~Andrew
>>
> I'm fine with that.  Jan, would you mind modifying the message when you 
> commit the code, or you want me to send out a new patch?

I'll try to remember that, but considering the reply I just sent to
the patch itself I don't think you're going to get around doing a
v3, in which case you should do the change yourself.

Jan

Re: [PATCH 1/1 V2] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed

[Suravee Suthikulanit]

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

On 9/4/2013 3:53 AM, Jan Beulich wrote:
>>>> On 03.09.13 at 04:36, Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> wrote:
>> On 9/1/2013 3:40 PM, Andrew Cooper wrote:
>>> On 31/08/2013 01:41, suravee.suthikulpanit@amd.com wrote:
>>>> @@ -175,6 +176,15 @@ static int __init amd_iommu_setup_dom0_device(u8 devfn,
>> struct pci_dev *pdev)
>>>>    
>>>>        if ( unlikely(!iommu) )
>>>>        {
>>>> +        /* Filter the bridge devices */
>>>> +        if ( (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);
>>> I know this is being picky, but I would prefer "Skipping host bridge"
>>> here, to avoid any possible confusion that non-host bridges might be
>>> considered for being skipped.
>>>
>>> ~Andrew
>>>
>> I'm fine with that.  Jan, would you mind modifying the message when you
>> commit the code, or you want me to send out a new patch?
> I'll try to remember that, but considering the reply I just sent to
> the patch itself I don't think you're going to get around doing a
> v3, in which case you should do the change yourself.
>
> Jan
>
I'll take care of itin V3.

Suravee

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

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

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

Re: [PATCH 1/1 V2] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed

[Suravee Suthikulanit]

On 9/4/2013 3:51 AM, Jan Beulich wrote:
>>>> On 31.08.13 at 02:41, <suravee.suthikulpanit@amd.com> wrote:
>>>>
>> --- a/xen/drivers/passthrough/vtd/intremap.c
>> +++ b/xen/drivers/passthrough/vtd/intremap.c
>> @@ -453,6 +453,7 @@ static void set_msi_source_id(struct pci_dev *pdev,
>> struct iremap_entry *ire)
>>           break;
>>   
>>       case DEV_TYPE_PCI:
>> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>>       case DEV_TYPE_LEGACY_PCI_BRIDGE:
>>       case DEV_TYPE_PCI2PCIe_BRIDGE:
>>           ret = find_upstream_bridge(seg, &bus, &devfn, &secbus);
> There shouldn't be an upstream bridge to a host bridge, and
> hence adding the case here (rather than above) is at least
> pointlessly running more complex code (all under the unlikely but
> not impossible assumption that a host bridge would have MSI set
> up for it in the first place).
I put it here because the original code (before introducing the 
DEV_TYPE_PCI_HOST_BRIDGE) would have classified the host bridge device 
as "DEV_TYPE_PCI".  Therefore, I was trying to keep the logic the same 
for Intel.   However, I agree that there should not be upstream bridge 
from host bridge.  But I am not sure that the case below would be 
appropriate.

     case DEV_TYPE_PCIe_ENDPOINT:
     case DEV_TYPE_PCIe_BRIDGE:
     case DEV_TYPE_PCIe2PCI_BRIDGE:
         switch ( pdev->phantom_stride )
         {
         case 1: sq = SQ_13_IGNORE_3; break;
         case 2: sq = SQ_13_IGNORE_2; break;
         case 4: sq = SQ_13_IGNORE_1; break;
         default: sq = SQ_ALL_16; break;
         }
         set_ire_sid(ire, SVT_VERIFY_SID_SQ, sq, PCI_BDF2(bus, devfn));
         break;

Do we need to call set_ire_sid() for host bridge device?  Or should we 
just have it's own case which does nothing.
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -1451,6 +1451,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,
>> @@ -1580,6 +1581,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));
> Host bridges would so far have gone through the respective default
> cases, not setting up any mapping, but logging a message. I think
> the change ought to be that host bridges result in both functions to
> fail, but without the log message saying "unknown" (and perhaps
> with -EPERM or -EACCES rather than -EINVAL).
I could do that.
> But in any event - you forgot to Cc the VT-d maintainer, who will
> need to ack the change anyway.
Sorry and thanks :)
>
> Jan
>
>

Re: [PATCH 1/1 V2] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed

[Jan Beulich]

>>> On 04.09.13 at 17:54, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote:
> On 9/4/2013 3:51 AM, Jan Beulich wrote:
>>>>> On 31.08.13 at 02:41, <suravee.suthikulpanit@amd.com> wrote:
>>> --- a/xen/drivers/passthrough/vtd/intremap.c
>>> +++ b/xen/drivers/passthrough/vtd/intremap.c
>>> @@ -453,6 +453,7 @@ static void set_msi_source_id(struct pci_dev *pdev,
>>> struct iremap_entry *ire)
>>>           break;
>>>   
>>>       case DEV_TYPE_PCI:
>>> +    case DEV_TYPE_PCI_HOST_BRIDGE:
>>>       case DEV_TYPE_LEGACY_PCI_BRIDGE:
>>>       case DEV_TYPE_PCI2PCIe_BRIDGE:
>>>           ret = find_upstream_bridge(seg, &bus, &devfn, &secbus);
>> There shouldn't be an upstream bridge to a host bridge, and
>> hence adding the case here (rather than above) is at least
>> pointlessly running more complex code (all under the unlikely but
>> not impossible assumption that a host bridge would have MSI set
>> up for it in the first place).
> I put it here because the original code (before introducing the 
> DEV_TYPE_PCI_HOST_BRIDGE) would have classified the host bridge device 
> as "DEV_TYPE_PCI".  Therefore, I was trying to keep the logic the same 
> for Intel.   However, I agree that there should not be upstream bridge 
> from host bridge.

Let's wait for Xiantao's input (in the hope that he'll respond soon).

Jan