* Re: [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges
2011-09-01 14:20 [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges George Dunlap
@ 2011-09-05 12:15 ` Jan Beulich
2011-09-05 14:56 ` George Dunlap
2011-09-06 21:16 ` Kay, Allen M
2011-09-13 9:12 ` George Dunlap
2 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2011-09-05 12:15 UTC (permalink / raw)
To: george.dunlap; +Cc: xen-devel
>>> 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges
2011-09-05 12:15 ` Jan Beulich
@ 2011-09-05 14:56 ` George Dunlap
2011-09-05 15:31 ` Jan Beulich
0 siblings, 1 reply; 9+ messages in thread
From: George Dunlap @ 2011-09-05 14:56 UTC (permalink / raw)
To: Jan Beulich, Keir Fraser; +Cc: xen-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges
2011-09-05 14:56 ` George Dunlap
@ 2011-09-05 15:31 ` Jan Beulich
0 siblings, 0 replies; 9+ messages in thread
From: Jan Beulich @ 2011-09-05 15:31 UTC (permalink / raw)
To: George Dunlap; +Cc: Keir Fraser, xen-devel, Allen M Kay
>>> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges
2011-09-01 14:20 [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges George Dunlap
2011-09-05 12:15 ` Jan Beulich
@ 2011-09-06 21:16 ` Kay, Allen M
2011-09-07 9:52 ` George Dunlap
2011-09-13 9:12 ` George Dunlap
2 siblings, 1 reply; 9+ messages in thread
From: Kay, Allen M @ 2011-09-06 21:16 UTC (permalink / raw)
To: George Dunlap, Jan Beulich; +Cc: xen-devel@lists.xensource.com
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges
2011-09-06 21:16 ` Kay, Allen M
@ 2011-09-07 9:52 ` George Dunlap
0 siblings, 0 replies; 9+ messages in thread
From: George Dunlap @ 2011-09-07 9:52 UTC (permalink / raw)
To: Kay, Allen M; +Cc: xen-devel@lists.xensource.com, Jan Beulich
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges
2011-09-01 14:20 [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges George Dunlap
2011-09-05 12:15 ` Jan Beulich
2011-09-06 21:16 ` Kay, Allen M
@ 2011-09-13 9:12 ` George Dunlap
2011-09-13 10:53 ` Jan Beulich
2 siblings, 1 reply; 9+ messages in thread
From: George Dunlap @ 2011-09-13 9:12 UTC (permalink / raw)
To: xen-devel; +Cc: Keir Fraser
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges
2011-09-13 9:12 ` George Dunlap
@ 2011-09-13 10:53 ` Jan Beulich
2011-09-14 8:48 ` George Dunlap
0 siblings, 1 reply; 9+ messages in thread
From: Jan Beulich @ 2011-09-13 10:53 UTC (permalink / raw)
To: George Dunlap; +Cc: xen-devel, Keir Fraser
>>> 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
>>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] xen, vtd: Fix device check for devices behind PCIe-to-PCI bridges
2011-09-13 10:53 ` Jan Beulich
@ 2011-09-14 8:48 ` George Dunlap
0 siblings, 0 replies; 9+ messages in thread
From: George Dunlap @ 2011-09-14 8:48 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread