George Dunlap
2011-Sep-01 14:20 UTC
[Xen-devel] [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges
On some systems, requests devices behind a PCIe-to-PCI bridge all
appear to the IOMMU as though they come from from slot 0, function
0 on that device; so the mapping code much punch a hole for X:0.0
in the IOMMU for such devices. When punching the hole, if that device
has already been mapped once, we simply need to check ownership to
make sure it''s legal. To do so, domain_context_mapping_one() will look
up the device for the mapping with pci_get_pdev() and look for the owner.
However, if there is no device in X:0.0, this look up will fail.
Rather than returning -ENODEV in this situation (causing a failure in
mapping the device), try to get the domain ownership from the iommu context
mapping itself.
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
diff -r 4a4882df5649 -r ede81b0552be xen/drivers/passthrough/vtd/iommu.c
--- a/xen/drivers/passthrough/vtd/iommu.c Wed Aug 31 15:23:49 2011 +0100
+++ b/xen/drivers/passthrough/vtd/iommu.c Thu Sep 01 15:18:18 2011 +0100
@@ -113,6 +113,27 @@ static int context_set_domain_id(struct
return 0;
}
+static int context_get_domain_id(struct context_entry *context,
+ struct iommu *iommu)
+{
+ unsigned long dom_index, nr_dom;
+ int domid = -1;
+
+ if (iommu && context)
+ {
+ nr_dom = cap_ndoms(iommu->cap);
+
+ dom_index = context_domain_id(*context);
+
+ if ( dom_index < nr_dom && iommu->domid_map)
+ domid = iommu->domid_map[dom_index];
+ else
+ dprintk(XENLOG_DEBUG VTDPREFIX, "%s: dom_index %lu exceeds
nr_dom %lu or iommu has no domid_map\n",
+ __func__, dom_index, nr_dom);
+ }
+ return domid;
+}
+
static struct intel_iommu *__init alloc_intel_iommu(void)
{
struct intel_iommu *intel;
@@ -1237,7 +1258,6 @@ int domain_context_mapping_one(
struct hvm_iommu *hd = domain_hvm_iommu(domain);
struct context_entry *context, *context_entries;
u64 maddr, pgd_maddr;
- struct pci_dev *pdev = NULL;
int agaw;
ASSERT(spin_is_locked(&pcidevs_lock));
@@ -1249,12 +1269,45 @@ int domain_context_mapping_one(
if ( context_present(*context) )
{
int res = 0;
+ struct pci_dev *pdev = NULL;
+ /* First try to get domain ownership from device structure. If
that''s
+ * not available, try to read it from the context itself. */
pdev = pci_get_pdev(bus, devfn);
- if (!pdev)
- res = -ENODEV;
- else if (pdev->domain != domain)
- res = -EINVAL;
+ if ( pdev )
+ {
+ if ( pdev->domain != domain )
+ {
+ dprintk(XENLOG_INFO VTDPREFIX, "d%d: bdf = %x:%x.%x owned
by d%d!",
+ domain->domain_id,
+ bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
+ (pdev->domain)
+ ? pdev->domain->domain_id : -1);
+ res = -EINVAL;
+ }
+ }
+ else
+ {
+ int cdomain;
+ cdomain = context_get_domain_id(context, iommu);
+
+ if ( cdomain < 0 )
+ {
+ dprintk(VTDPREFIX, "d%d: bdf = %x:%x.%x mapped, but
can''t find owner!\n",
+ domain->domain_id,
+ bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
+ res = -EINVAL;
+ }
+ else if ( cdomain != domain->domain_id )
+ {
+ dprintk(XENLOG_INFO VTDPREFIX, "d%d: bdf = %x:%x.%x
already mapped to d%d!",
+ domain->domain_id,
+ bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
+ cdomain);
+ res = -EINVAL;
+ }
+ }
+
unmap_vtd_domain_page(context_entries);
spin_unlock(&iommu->lock);
return res;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Jan Beulich
2011-Sep-05 12:15 UTC
Re: [Xen-devel] [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges
>>> On 01.09.11 at 16:20, George Dunlap <george.dunlap@eu.citrix.com> wrote: > On some systems, requests devices behind a PCIe-to-PCI bridge all > appear to the IOMMU as though they come from from slot 0, function > 0 on that device; so the mapping code much punch a hole for X:0.0 > in the IOMMU for such devices. When punching the hole, if that device > has already been mapped once, we simply need to check ownership to > make sure it''s legal. To do so, domain_context_mapping_one() will look > up the device for the mapping with pci_get_pdev() and look for the owner. > > However, if there is no device in X:0.0, this look up will fail.Was it really that there was no device at all at X:0.0, or rather that Xen just didn''t know about the device (because Dom0 failed to notify Xen, as could happen in the 2.6.18-derived trees up to pretty recently)? Also, didn''t we sort of agree that creating a phantom device would be more elegant (or at least much smaller a change)? Jan> Rather than returning -ENODEV in this situation (causing a failure in > mapping the device), try to get the domain ownership from the iommu context > mapping itself. > > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com> > > diff -r 4a4882df5649 -r ede81b0552be xen/drivers/passthrough/vtd/iommu.c > --- a/xen/drivers/passthrough/vtd/iommu.c Wed Aug 31 15:23:49 2011 +0100 > +++ b/xen/drivers/passthrough/vtd/iommu.c Thu Sep 01 15:18:18 2011 +0100 > @@ -113,6 +113,27 @@ static int context_set_domain_id(struct > return 0; > } > > +static int context_get_domain_id(struct context_entry *context, > + struct iommu *iommu) > +{ > + unsigned long dom_index, nr_dom; > + int domid = -1; > + > + if (iommu && context) > + { > + nr_dom = cap_ndoms(iommu->cap); > + > + dom_index = context_domain_id(*context); > + > + if ( dom_index < nr_dom && iommu->domid_map) > + domid = iommu->domid_map[dom_index]; > + else > + dprintk(XENLOG_DEBUG VTDPREFIX, "%s: dom_index %lu exceeds > nr_dom %lu or iommu has no domid_map\n", > + __func__, dom_index, nr_dom); > + } > + return domid; > +} > + > static struct intel_iommu *__init alloc_intel_iommu(void) > { > struct intel_iommu *intel; > @@ -1237,7 +1258,6 @@ int domain_context_mapping_one( > struct hvm_iommu *hd = domain_hvm_iommu(domain); > struct context_entry *context, *context_entries; > u64 maddr, pgd_maddr; > - struct pci_dev *pdev = NULL; > int agaw; > > ASSERT(spin_is_locked(&pcidevs_lock)); > @@ -1249,12 +1269,45 @@ int domain_context_mapping_one( > if ( context_present(*context) ) > { > int res = 0; > + struct pci_dev *pdev = NULL; > > + /* First try to get domain ownership from device structure. If > that''s > + * not available, try to read it from the context itself. */ > pdev = pci_get_pdev(bus, devfn); > - if (!pdev) > - res = -ENODEV; > - else if (pdev->domain != domain) > - res = -EINVAL; > + if ( pdev ) > + { > + if ( pdev->domain != domain ) > + { > + dprintk(XENLOG_INFO VTDPREFIX, "d%d: bdf = %x:%x.%x owned > by d%d!", > + domain->domain_id, > + bus, PCI_SLOT(devfn), PCI_FUNC(devfn), > + (pdev->domain) > + ? pdev->domain->domain_id : -1); > + res = -EINVAL; > + } > + } > + else > + { > + int cdomain; > + cdomain = context_get_domain_id(context, iommu); > + > + if ( cdomain < 0 ) > + { > + dprintk(VTDPREFIX, "d%d: bdf = %x:%x.%x mapped, but can''t > find owner!\n", > + domain->domain_id, > + bus, PCI_SLOT(devfn), PCI_FUNC(devfn)); > + res = -EINVAL; > + } > + else if ( cdomain != domain->domain_id ) > + { > + dprintk(XENLOG_INFO VTDPREFIX, "d%d: bdf = %x:%x.%x already > mapped to d%d!", > + domain->domain_id, > + bus, PCI_SLOT(devfn), PCI_FUNC(devfn), > + cdomain); > + res = -EINVAL; > + } > + } > + > unmap_vtd_domain_page(context_entries); > spin_unlock(&iommu->lock); > return res; > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George Dunlap
2011-Sep-05 14:56 UTC
Re: [Xen-devel] [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges
On Mon, Sep 5, 2011 at 1:15 PM, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 01.09.11 at 16:20, George Dunlap <george.dunlap@eu.citrix.com> wrote: >> On some systems, requests devices behind a PCIe-to-PCI bridge all >> appear to the IOMMU as though they come from from slot 0, function >> 0 on that device; so the mapping code much punch a hole for X:0.0 >> in the IOMMU for such devices. When punching the hole, if that device >> has already been mapped once, we simply need to check ownership to >> make sure it''s legal. To do so, domain_context_mapping_one() will look >> up the device for the mapping with pci_get_pdev() and look for the owner. >> >> However, if there is no device in X:0.0, this look up will fail. > > Was it really that there was no device at all at X:0.0, or rather that > Xen just didn''t know about the device (because Dom0 failed to notify > Xen, as could happen in the 2.6.18-derived trees up to pretty > recently)?Don''t know for sure; this was a partner that turned this up through our beta-test program. But IIRC, running "lspci" in dom0 reported nothing under X:0.0 (although I may well be remembering incorrectly). This was for XenServer 6.0 which is using Novell''s Xen-ified 2.6.32 kernel.> Also, didn''t we sort of agree that creating a phantom device would > be more elegant (or at least much smaller a change)?I don''t remember talking about that, but perhaps. :-) In reality, I don''t know the code well enough to whip up a patch (like, where / how would I make such a device), and this is not that much of a priority for me. If this patch isn''t accepted, it will probably fall to you or Keir (or some other sufficiently motivated party) to fix it. -George _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Sep-05 15:31 UTC
Re: [Xen-devel] [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges
>>> On 05.09.11 at 16:56, George Dunlap <George.Dunlap@eu.citrix.com> wrote: > On Mon, Sep 5, 2011 at 1:15 PM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 01.09.11 at 16:20, George Dunlap <george.dunlap@eu.citrix.com> wrote: >>> On some systems, requests devices behind a PCIe-to-PCI bridge all >>> appear to the IOMMU as though they come from from slot 0, function >>> 0 on that device; so the mapping code much punch a hole for X:0.0 >>> in the IOMMU for such devices. When punching the hole, if that device >>> has already been mapped once, we simply need to check ownership to >>> make sure it''s legal. To do so, domain_context_mapping_one() will look >>> up the device for the mapping with pci_get_pdev() and look for the owner. >>> >>> However, if there is no device in X:0.0, this look up will fail. >> >> Was it really that there was no device at all at X:0.0, or rather that >> Xen just didn''t know about the device (because Dom0 failed to notify >> Xen, as could happen in the 2.6.18-derived trees up to pretty >> recently)? > > Don''t know for sure; this was a partner that turned this up through > our beta-test program. But IIRC, running "lspci" in dom0 reported > nothing under X:0.0 (although I may well be remembering incorrectly). > > This was for XenServer 6.0 which is using Novell''s Xen-ified 2.6.32 kernel. > >> Also, didn''t we sort of agree that creating a phantom device would >> be more elegant (or at least much smaller a change)? > > I don''t remember talking about that, but perhaps. :-) > > In reality, I don''t know the code well enough to whip up a patch > (like, where / how would I make such a device), and this is not that > much of a priority for me. If this patch isn''t accepted, it will > probably fall to you or Keir (or some other sufficiently motivated > party) to fix it.Understood. I''d be curious of Allen''s thoughts here, as he would be the most knowledgeable one in this area. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kay, Allen M
2011-Sep-06 21:16 UTC
RE: [Xen-devel] [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges
I''m curious how does this context entry became valid in the first place
if pdev cannot be found?
In general, this patch seems harmless to other situations. It just does the
same new and old domain checking for the case where pdev is not found.
Allen
-----Original Message-----
From: xen-devel-bounces@lists.xensource.com
[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of George Dunlap
Sent: Thursday, September 01, 2011 7:20 AM
To: xen-devel@lists.xensource.com
Cc: george.dunlap@eu.citrix.com
Subject: [Xen-devel] [PATCH] xen, vtd: Fix device check for devices behind
PCIe-to-PCI bridges
On some systems, requests devices behind a PCIe-to-PCI bridge all
appear to the IOMMU as though they come from from slot 0, function
0 on that device; so the mapping code much punch a hole for X:0.0
in the IOMMU for such devices. When punching the hole, if that device
has already been mapped once, we simply need to check ownership to
make sure it''s legal. To do so, domain_context_mapping_one() will look
up the device for the mapping with pci_get_pdev() and look for the owner.
However, if there is no device in X:0.0, this look up will fail.
Rather than returning -ENODEV in this situation (causing a failure in
mapping the device), try to get the domain ownership from the iommu context
mapping itself.
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
diff -r 4a4882df5649 -r ede81b0552be xen/drivers/passthrough/vtd/iommu.c
--- a/xen/drivers/passthrough/vtd/iommu.c Wed Aug 31 15:23:49 2011 +0100
+++ b/xen/drivers/passthrough/vtd/iommu.c Thu Sep 01 15:18:18 2011 +0100
@@ -113,6 +113,27 @@ static int context_set_domain_id(struct
return 0;
}
+static int context_get_domain_id(struct context_entry *context,
+ struct iommu *iommu)
+{
+ unsigned long dom_index, nr_dom;
+ int domid = -1;
+
+ if (iommu && context)
+ {
+ nr_dom = cap_ndoms(iommu->cap);
+
+ dom_index = context_domain_id(*context);
+
+ if ( dom_index < nr_dom && iommu->domid_map)
+ domid = iommu->domid_map[dom_index];
+ else
+ dprintk(XENLOG_DEBUG VTDPREFIX, "%s: dom_index %lu exceeds
nr_dom %lu or iommu has no domid_map\n",
+ __func__, dom_index, nr_dom);
+ }
+ return domid;
+}
+
static struct intel_iommu *__init alloc_intel_iommu(void)
{
struct intel_iommu *intel;
@@ -1237,7 +1258,6 @@ int domain_context_mapping_one(
struct hvm_iommu *hd = domain_hvm_iommu(domain);
struct context_entry *context, *context_entries;
u64 maddr, pgd_maddr;
- struct pci_dev *pdev = NULL;
int agaw;
ASSERT(spin_is_locked(&pcidevs_lock));
@@ -1249,12 +1269,45 @@ int domain_context_mapping_one(
if ( context_present(*context) )
{
int res = 0;
+ struct pci_dev *pdev = NULL;
+ /* First try to get domain ownership from device structure. If
that''s
+ * not available, try to read it from the context itself. */
pdev = pci_get_pdev(bus, devfn);
- if (!pdev)
- res = -ENODEV;
- else if (pdev->domain != domain)
- res = -EINVAL;
+ if ( pdev )
+ {
+ if ( pdev->domain != domain )
+ {
+ dprintk(XENLOG_INFO VTDPREFIX, "d%d: bdf = %x:%x.%x owned
by d%d!",
+ domain->domain_id,
+ bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
+ (pdev->domain)
+ ? pdev->domain->domain_id : -1);
+ res = -EINVAL;
+ }
+ }
+ else
+ {
+ int cdomain;
+ cdomain = context_get_domain_id(context, iommu);
+
+ if ( cdomain < 0 )
+ {
+ dprintk(VTDPREFIX, "d%d: bdf = %x:%x.%x mapped, but
can''t find owner!\n",
+ domain->domain_id,
+ bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
+ res = -EINVAL;
+ }
+ else if ( cdomain != domain->domain_id )
+ {
+ dprintk(XENLOG_INFO VTDPREFIX, "d%d: bdf = %x:%x.%x
already mapped to d%d!",
+ domain->domain_id,
+ bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
+ cdomain);
+ res = -EINVAL;
+ }
+ }
+
unmap_vtd_domain_page(context_entries);
spin_unlock(&iommu->lock);
return res;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
George Dunlap
2011-Sep-07 09:52 UTC
Re: [Xen-devel] [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges
On Tue, Sep 6, 2011 at 10:16 PM, Kay, Allen M <allen.m.kay@intel.com> wrote:> I''m curious how does this context entry became valid in the first place if pdev cannot be found?Apparently the only code that needs the pdev is the part that does the ownership checking. :-)> In general, this patch seems harmless to other situations. It just does the same new and old domain checking for the case where pdev is not found.Great, thanks. -George> > Allen > > -----Original Message----- > From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of George Dunlap > Sent: Thursday, September 01, 2011 7:20 AM > To: xen-devel@lists.xensource.com > Cc: george.dunlap@eu.citrix.com > Subject: [Xen-devel] [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges > > On some systems, requests devices behind a PCIe-to-PCI bridge all > appear to the IOMMU as though they come from from slot 0, function > 0 on that device; so the mapping code much punch a hole for X:0.0 > in the IOMMU for such devices. When punching the hole, if that device > has already been mapped once, we simply need to check ownership to > make sure it''s legal. To do so, domain_context_mapping_one() will look > up the device for the mapping with pci_get_pdev() and look for the owner. > > However, if there is no device in X:0.0, this look up will fail. > > Rather than returning -ENODEV in this situation (causing a failure in > mapping the device), try to get the domain ownership from the iommu context > mapping itself. > > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com> > > diff -r 4a4882df5649 -r ede81b0552be xen/drivers/passthrough/vtd/iommu.c > --- a/xen/drivers/passthrough/vtd/iommu.c Wed Aug 31 15:23:49 2011 +0100 > +++ b/xen/drivers/passthrough/vtd/iommu.c Thu Sep 01 15:18:18 2011 +0100 > @@ -113,6 +113,27 @@ static int context_set_domain_id(struct > return 0; > } > > +static int context_get_domain_id(struct context_entry *context, > + struct iommu *iommu) > +{ > + unsigned long dom_index, nr_dom; > + int domid = -1; > + > + if (iommu && context) > + { > + nr_dom = cap_ndoms(iommu->cap); > + > + dom_index = context_domain_id(*context); > + > + if ( dom_index < nr_dom && iommu->domid_map) > + domid = iommu->domid_map[dom_index]; > + else > + dprintk(XENLOG_DEBUG VTDPREFIX, "%s: dom_index %lu exceeds nr_dom %lu or iommu has no domid_map\n", > + __func__, dom_index, nr_dom); > + } > + return domid; > +} > + > static struct intel_iommu *__init alloc_intel_iommu(void) > { > struct intel_iommu *intel; > @@ -1237,7 +1258,6 @@ int domain_context_mapping_one( > struct hvm_iommu *hd = domain_hvm_iommu(domain); > struct context_entry *context, *context_entries; > u64 maddr, pgd_maddr; > - struct pci_dev *pdev = NULL; > int agaw; > > ASSERT(spin_is_locked(&pcidevs_lock)); > @@ -1249,12 +1269,45 @@ int domain_context_mapping_one( > if ( context_present(*context) ) > { > int res = 0; > + struct pci_dev *pdev = NULL; > > + /* First try to get domain ownership from device structure. If that''s > + * not available, try to read it from the context itself. */ > pdev = pci_get_pdev(bus, devfn); > - if (!pdev) > - res = -ENODEV; > - else if (pdev->domain != domain) > - res = -EINVAL; > + if ( pdev ) > + { > + if ( pdev->domain != domain ) > + { > + dprintk(XENLOG_INFO VTDPREFIX, "d%d: bdf = %x:%x.%x owned by d%d!", > + domain->domain_id, > + bus, PCI_SLOT(devfn), PCI_FUNC(devfn), > + (pdev->domain) > + ? pdev->domain->domain_id : -1); > + res = -EINVAL; > + } > + } > + else > + { > + int cdomain; > + cdomain = context_get_domain_id(context, iommu); > + > + if ( cdomain < 0 ) > + { > + dprintk(VTDPREFIX, "d%d: bdf = %x:%x.%x mapped, but can''t find owner!\n", > + domain->domain_id, > + bus, PCI_SLOT(devfn), PCI_FUNC(devfn)); > + res = -EINVAL; > + } > + else if ( cdomain != domain->domain_id ) > + { > + dprintk(XENLOG_INFO VTDPREFIX, "d%d: bdf = %x:%x.%x already mapped to d%d!", > + domain->domain_id, > + bus, PCI_SLOT(devfn), PCI_FUNC(devfn), > + cdomain); > + res = -EINVAL; > + } > + } > + > unmap_vtd_domain_page(context_entries); > spin_unlock(&iommu->lock); > return res; > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George Dunlap
2011-Sep-13 09:12 UTC
Re: [Xen-devel] [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges
So what was the verdict on this one? Is someone going to commit to doing a "fake pdev" thing? If that''s not going to happen before the 4.2 release, I suggest we take this patch in the mean time. -George On Thu, Sep 1, 2011 at 3:20 PM, George Dunlap <george.dunlap@eu.citrix.com> wrote:> On some systems, requests devices behind a PCIe-to-PCI bridge all > appear to the IOMMU as though they come from from slot 0, function > 0 on that device; so the mapping code much punch a hole for X:0.0 > in the IOMMU for such devices. When punching the hole, if that device > has already been mapped once, we simply need to check ownership to > make sure it''s legal. To do so, domain_context_mapping_one() will look > up the device for the mapping with pci_get_pdev() and look for the owner. > > However, if there is no device in X:0.0, this look up will fail. > > Rather than returning -ENODEV in this situation (causing a failure in > mapping the device), try to get the domain ownership from the iommu context > mapping itself. > > Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com> > > diff -r 4a4882df5649 -r ede81b0552be xen/drivers/passthrough/vtd/iommu.c > --- a/xen/drivers/passthrough/vtd/iommu.c Wed Aug 31 15:23:49 2011 +0100 > +++ b/xen/drivers/passthrough/vtd/iommu.c Thu Sep 01 15:18:18 2011 +0100 > @@ -113,6 +113,27 @@ static int context_set_domain_id(struct > return 0; > } > > +static int context_get_domain_id(struct context_entry *context, > + struct iommu *iommu) > +{ > + unsigned long dom_index, nr_dom; > + int domid = -1; > + > + if (iommu && context) > + { > + nr_dom = cap_ndoms(iommu->cap); > + > + dom_index = context_domain_id(*context); > + > + if ( dom_index < nr_dom && iommu->domid_map) > + domid = iommu->domid_map[dom_index]; > + else > + dprintk(XENLOG_DEBUG VTDPREFIX, "%s: dom_index %lu exceeds nr_dom %lu or iommu has no domid_map\n", > + __func__, dom_index, nr_dom); > + } > + return domid; > +} > + > static struct intel_iommu *__init alloc_intel_iommu(void) > { > struct intel_iommu *intel; > @@ -1237,7 +1258,6 @@ int domain_context_mapping_one( > struct hvm_iommu *hd = domain_hvm_iommu(domain); > struct context_entry *context, *context_entries; > u64 maddr, pgd_maddr; > - struct pci_dev *pdev = NULL; > int agaw; > > ASSERT(spin_is_locked(&pcidevs_lock)); > @@ -1249,12 +1269,45 @@ int domain_context_mapping_one( > if ( context_present(*context) ) > { > int res = 0; > + struct pci_dev *pdev = NULL; > > + /* First try to get domain ownership from device structure. If that''s > + * not available, try to read it from the context itself. */ > pdev = pci_get_pdev(bus, devfn); > - if (!pdev) > - res = -ENODEV; > - else if (pdev->domain != domain) > - res = -EINVAL; > + if ( pdev ) > + { > + if ( pdev->domain != domain ) > + { > + dprintk(XENLOG_INFO VTDPREFIX, "d%d: bdf = %x:%x.%x owned by d%d!", > + domain->domain_id, > + bus, PCI_SLOT(devfn), PCI_FUNC(devfn), > + (pdev->domain) > + ? pdev->domain->domain_id : -1); > + res = -EINVAL; > + } > + } > + else > + { > + int cdomain; > + cdomain = context_get_domain_id(context, iommu); > + > + if ( cdomain < 0 ) > + { > + dprintk(VTDPREFIX, "d%d: bdf = %x:%x.%x mapped, but can''t find owner!\n", > + domain->domain_id, > + bus, PCI_SLOT(devfn), PCI_FUNC(devfn)); > + res = -EINVAL; > + } > + else if ( cdomain != domain->domain_id ) > + { > + dprintk(XENLOG_INFO VTDPREFIX, "d%d: bdf = %x:%x.%x already mapped to d%d!", > + domain->domain_id, > + bus, PCI_SLOT(devfn), PCI_FUNC(devfn), > + cdomain); > + res = -EINVAL; > + } > + } > + > unmap_vtd_domain_page(context_entries); > spin_unlock(&iommu->lock); > return res; > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2011-Sep-13 10:53 UTC
Re: [Xen-devel] [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges
>>> On 13.09.11 at 11:12, George Dunlap <George.Dunlap@eu.citrix.com> wrote: > So what was the verdict on this one? Is someone going to commit to > doing a "fake pdev" thing? If that''s not going to happen before the > 4.2 release, I suggest we take this patch in the mean time.Isn''t this -unstable c/s 23813:5535d7ce2673? Jan> On Thu, Sep 1, 2011 at 3:20 PM, George Dunlap > <george.dunlap@eu.citrix.com> wrote: >> On some systems, requests devices behind a PCIe-to-PCI bridge all >> appear to the IOMMU as though they come from from slot 0, function >> 0 on that device; so the mapping code much punch a hole for X:0.0 >> in the IOMMU for such devices. When punching the hole, if that device >> has already been mapped once, we simply need to check ownership to >> make sure it''s legal. To do so, domain_context_mapping_one() will look >> up the device for the mapping with pci_get_pdev() and look for the owner. >> >> However, if there is no device in X:0.0, this look up will fail. >> >> Rather than returning -ENODEV in this situation (causing a failure in >> mapping the device), try to get the domain ownership from the iommu context >> mapping itself. >> >> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com> >> >> diff -r 4a4882df5649 -r ede81b0552be xen/drivers/passthrough/vtd/iommu.c >> --- a/xen/drivers/passthrough/vtd/iommu.c Wed Aug 31 15:23:49 2011 +0100 >> +++ b/xen/drivers/passthrough/vtd/iommu.c Thu Sep 01 15:18:18 2011 > +0100 >> @@ -113,6 +113,27 @@ static int context_set_domain_id(struct >> return 0; >> } >> >> +static int context_get_domain_id(struct context_entry *context, >> + struct iommu *iommu) >> +{ >> + unsigned long dom_index, nr_dom; >> + int domid = -1; >> + >> + if (iommu && context) >> + { >> + nr_dom = cap_ndoms(iommu->cap); >> + >> + dom_index = context_domain_id(*context); >> + >> + if ( dom_index < nr_dom && iommu->domid_map) >> + domid = iommu->domid_map[dom_index]; >> + else >> + dprintk(XENLOG_DEBUG VTDPREFIX, "%s: dom_index %lu exceeds > nr_dom %lu or iommu has no domid_map\n", >> + __func__, dom_index, nr_dom); >> + } >> + return domid; >> +} >> + >> static struct intel_iommu *__init alloc_intel_iommu(void) >> { >> struct intel_iommu *intel; >> @@ -1237,7 +1258,6 @@ int domain_context_mapping_one( >> struct hvm_iommu *hd = domain_hvm_iommu(domain); >> struct context_entry *context, *context_entries; >> u64 maddr, pgd_maddr; >> - struct pci_dev *pdev = NULL; >> int agaw; >> >> ASSERT(spin_is_locked(&pcidevs_lock)); >> @@ -1249,12 +1269,45 @@ int domain_context_mapping_one( >> if ( context_present(*context) ) >> { >> int res = 0; >> + struct pci_dev *pdev = NULL; >> >> + /* First try to get domain ownership from device structure. If > that''s >> + * not available, try to read it from the context itself. */ >> pdev = pci_get_pdev(bus, devfn); >> - if (!pdev) >> - res = -ENODEV; >> - else if (pdev->domain != domain) >> - res = -EINVAL; >> + if ( pdev ) >> + { >> + if ( pdev->domain != domain ) >> + { >> + dprintk(XENLOG_INFO VTDPREFIX, "d%d: bdf = %x:%x.%x owned > by d%d!", >> + domain->domain_id, >> + bus, PCI_SLOT(devfn), PCI_FUNC(devfn), >> + (pdev->domain) >> + ? pdev->domain->domain_id : -1); >> + res = -EINVAL; >> + } >> + } >> + else >> + { >> + int cdomain; >> + cdomain = context_get_domain_id(context, iommu); >> + >> + if ( cdomain < 0 ) >> + { >> + dprintk(VTDPREFIX, "d%d: bdf = %x:%x.%x mapped, but can''t > find owner!\n", >> + domain->domain_id, >> + bus, PCI_SLOT(devfn), PCI_FUNC(devfn)); >> + res = -EINVAL; >> + } >> + else if ( cdomain != domain->domain_id ) >> + { >> + dprintk(XENLOG_INFO VTDPREFIX, "d%d: bdf = %x:%x.%x already > mapped to d%d!", >> + domain->domain_id, >> + bus, PCI_SLOT(devfn), PCI_FUNC(devfn), >> + cdomain); >> + res = -EINVAL; >> + } >> + } >> + >> unmap_vtd_domain_page(context_entries); >> spin_unlock(&iommu->lock); >> return res; >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xensource.com >> http://lists.xensource.com/xen-devel >>_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
George Dunlap
2011-Sep-14 08:48 UTC
Re: [Xen-devel] [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges
Ah yes, so it is. Sorry. -George On Tue, Sep 13, 2011 at 11:53 AM, Jan Beulich <JBeulich@suse.com> wrote:>>>> On 13.09.11 at 11:12, George Dunlap <George.Dunlap@eu.citrix.com> wrote: >> So what was the verdict on this one? Is someone going to commit to >> doing a "fake pdev" thing? If that''s not going to happen before the >> 4.2 release, I suggest we take this patch in the mean time. > > Isn''t this -unstable c/s 23813:5535d7ce2673? > > Jan > >> On Thu, Sep 1, 2011 at 3:20 PM, George Dunlap >> <george.dunlap@eu.citrix.com> wrote: >>> On some systems, requests devices behind a PCIe-to-PCI bridge all >>> appear to the IOMMU as though they come from from slot 0, function >>> 0 on that device; so the mapping code much punch a hole for X:0.0 >>> in the IOMMU for such devices. When punching the hole, if that device >>> has already been mapped once, we simply need to check ownership to >>> make sure it''s legal. To do so, domain_context_mapping_one() will look >>> up the device for the mapping with pci_get_pdev() and look for the owner. >>> >>> However, if there is no device in X:0.0, this look up will fail. >>> >>> Rather than returning -ENODEV in this situation (causing a failure in >>> mapping the device), try to get the domain ownership from the iommu context >>> mapping itself. >>> >>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com> >>> >>> diff -r 4a4882df5649 -r ede81b0552be xen/drivers/passthrough/vtd/iommu.c >>> --- a/xen/drivers/passthrough/vtd/iommu.c Wed Aug 31 15:23:49 2011 +0100 >>> +++ b/xen/drivers/passthrough/vtd/iommu.c Thu Sep 01 15:18:18 2011 >> +0100 >>> @@ -113,6 +113,27 @@ static int context_set_domain_id(struct >>> return 0; >>> } >>> >>> +static int context_get_domain_id(struct context_entry *context, >>> + struct iommu *iommu) >>> +{ >>> + unsigned long dom_index, nr_dom; >>> + int domid = -1; >>> + >>> + if (iommu && context) >>> + { >>> + nr_dom = cap_ndoms(iommu->cap); >>> + >>> + dom_index = context_domain_id(*context); >>> + >>> + if ( dom_index < nr_dom && iommu->domid_map) >>> + domid = iommu->domid_map[dom_index]; >>> + else >>> + dprintk(XENLOG_DEBUG VTDPREFIX, "%s: dom_index %lu exceeds >> nr_dom %lu or iommu has no domid_map\n", >>> + __func__, dom_index, nr_dom); >>> + } >>> + return domid; >>> +} >>> + >>> static struct intel_iommu *__init alloc_intel_iommu(void) >>> { >>> struct intel_iommu *intel; >>> @@ -1237,7 +1258,6 @@ int domain_context_mapping_one( >>> struct hvm_iommu *hd = domain_hvm_iommu(domain); >>> struct context_entry *context, *context_entries; >>> u64 maddr, pgd_maddr; >>> - struct pci_dev *pdev = NULL; >>> int agaw; >>> >>> ASSERT(spin_is_locked(&pcidevs_lock)); >>> @@ -1249,12 +1269,45 @@ int domain_context_mapping_one( >>> if ( context_present(*context) ) >>> { >>> int res = 0; >>> + struct pci_dev *pdev = NULL; >>> >>> + /* First try to get domain ownership from device structure. If >> that''s >>> + * not available, try to read it from the context itself. */ >>> pdev = pci_get_pdev(bus, devfn); >>> - if (!pdev) >>> - res = -ENODEV; >>> - else if (pdev->domain != domain) >>> - res = -EINVAL; >>> + if ( pdev ) >>> + { >>> + if ( pdev->domain != domain ) >>> + { >>> + dprintk(XENLOG_INFO VTDPREFIX, "d%d: bdf = %x:%x.%x owned >> by d%d!", >>> + domain->domain_id, >>> + bus, PCI_SLOT(devfn), PCI_FUNC(devfn), >>> + (pdev->domain) >>> + ? pdev->domain->domain_id : -1); >>> + res = -EINVAL; >>> + } >>> + } >>> + else >>> + { >>> + int cdomain; >>> + cdomain = context_get_domain_id(context, iommu); >>> + >>> + if ( cdomain < 0 ) >>> + { >>> + dprintk(VTDPREFIX, "d%d: bdf = %x:%x.%x mapped, but can''t >> find owner!\n", >>> + domain->domain_id, >>> + bus, PCI_SLOT(devfn), PCI_FUNC(devfn)); >>> + res = -EINVAL; >>> + } >>> + else if ( cdomain != domain->domain_id ) >>> + { >>> + dprintk(XENLOG_INFO VTDPREFIX, "d%d: bdf = %x:%x.%x already >> mapped to d%d!", >>> + domain->domain_id, >>> + bus, PCI_SLOT(devfn), PCI_FUNC(devfn), >>> + cdomain); >>> + res = -EINVAL; >>> + } >>> + } >>> + >>> unmap_vtd_domain_page(context_entries); >>> spin_unlock(&iommu->lock); >>> return res; >>> >>> _______________________________________________ >>> Xen-devel mailing list >>> Xen-devel@lists.xensource.com >>> http://lists.xensource.com/xen-devel >>> > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel