<suravee.suthikulpanit@amd.com>
2013-Aug-07 02:40 UTC
[PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
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
Suravee Suthikulanit
2013-Aug-07 02:42 UTC
Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
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; >
Jan Beulich
2013-Aug-07 07:33 UTC
Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
>>> 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 0x0604Please 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
Stefan Bader
2013-Aug-07 08:33 UTC
Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Andrew Cooper
2013-Aug-07 09:34 UTC
Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
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; >
Jan Beulich
2013-Aug-07 09:57 UTC
Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
>>> 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
Suravee Suthikulanit
2013-Aug-07 15:31 UTC
Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
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 >
Jan Beulich
2013-Aug-07 15:42 UTC
Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
>>> 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
Suravee Suthikulanit
2013-Aug-07 19:20 UTC
Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
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.) > > JanJan, 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jan Beulich
2013-Aug-08 06:38 UTC
Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
>>> 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
Stefan Bader
2013-Aug-08 11:12 UTC
Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
On 07.08.2013 10:33, Stefan Bader wrote:>> > I will give it a spin, but might not b before tomorrow. > > -StefanSo 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; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jan Beulich
2013-Aug-08 11:49 UTC
Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
>>> 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
Stefan Bader
2013-Aug-08 12:07 UTC
Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
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- _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Jan Beulich
2013-Aug-08 12:29 UTC
Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
>>> 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-
Stefan Bader
2013-Aug-08 12:35 UTC
Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
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- > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Suravee Suthikulpanit
2013-Aug-30 01:25 UTC
Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
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... > > JanSorry 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
Jan Beulich
2013-Aug-30 08:09 UTC
Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
>>> 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
Suravee Suthikulpanit
2013-Aug-31 00:03 UTC
Re: [PATCH 1/1] x86/AMD: Fix setup ssss:bb:dd:f for d0 failed
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. > > JanOk, 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel