* Re: [PATCH] IOMMU: Prevent VT-d device IOTLB operations on wrong IOMMU
2014-06-12 11:40 [PATCH] IOMMU: Prevent VT-d device IOTLB operations on wrong IOMMU Malcolm Crossley
@ 2014-06-12 12:00 ` Paul Durrant
2014-06-12 12:05 ` Andrew Cooper
2014-06-12 19:10 ` Tian, Kevin
2 siblings, 0 replies; 5+ messages in thread
From: Paul Durrant @ 2014-06-12 12:00 UTC (permalink / raw)
To: Malcolm Crossley, xen-devel@lists.xen.org
Cc: yang.z.zhang@intel.com, Kevin Tian,
Aravind.Gopalakrishnan@amd.com, jbeulich@suse.com,
suravee.suthikulpanit@amd.com
> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Malcolm Crossley
> Sent: 12 June 2014 12:40
> To: xen-devel@lists.xen.org
> Cc: yang.z.zhang@intel.com; Kevin Tian; Aravind.Gopalakrishnan@amd.com;
> suravee.suthikulpanit@amd.com; jbeulich@suse.com
> Subject: [Xen-devel] [PATCH] IOMMU: Prevent VT-d device IOTLB
> operations on wrong IOMMU
>
> PCIE ATS allows for device's to contain IOTLB's, the VT-d code was iterating
> around all ATS capable devices and issuing IOTLB operations for all IOMMU's,
I think you'll be getting a visit from the apostrophe police ;-) There are others below you need to fix too.
Paul
> even though each ATS device's is only accessible via one particular IOMMU.
>
> Issuing an IOMMU operation to a device not accessible via that IOMMU
> results
> in an IOMMU timeout because the device does not reply. VT-d IOMMU
> timeouts
> result in a Xen panic.
>
> Therefore this bug prevents any Intel system with 2 or more ATS enabled
> IOMMU's,
> each with an ATS device connected to them, from booting Xen.
>
> The patch add's a IOMMU pointer to the ATS device struct so the VT-d code
> can
> ensure it does not issue IOMMU ATS operations on the wrong IOMMU. A
> void
> pointer has to be used because AMD and Intel IOMMU implementations do
> not have
> a common IOMMU structure or indexing mechanism.
>
> Optimised AMD's IOMMU code by leveraging the new IOMMU field in the
> ATS
> structure.
>
> Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
>
> diff -r 4708591d8aa8 -r 90352a46dbcb
> xen/drivers/passthrough/amd/iommu_cmd.c
> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
> @@ -303,8 +303,7 @@ void amd_iommu_flush_iotlb(u8 devfn, con
> if ( !pci_ats_enabled(ats_pdev->seg, ats_pdev->bus, ats_pdev->devfn) )
> return;
>
> - iommu = find_iommu_for_device(ats_pdev->seg,
> - PCI_BDF2(ats_pdev->bus, ats_pdev->devfn));
> + iommu = ats_pdev->iommu;
>
> if ( !iommu )
> {
> diff -r 4708591d8aa8 -r 90352a46dbcb
> xen/drivers/passthrough/amd/pci_amd_iommu.c
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -164,7 +164,7 @@ static void amd_iommu_setup_domain_devic
> !pci_ats_enabled(iommu->seg, bus, pdev->devfn) )
> {
> if ( devfn == pdev->devfn )
> - enable_ats_device(iommu->seg, bus, devfn);
> + enable_ats_device(iommu, iommu->seg, bus, devfn);
>
> amd_iommu_flush_iotlb(devfn, pdev,
> INV_IOMMU_ALL_PAGES_ADDRESS, 0);
> }
> diff -r 4708591d8aa8 -r 90352a46dbcb xen/drivers/passthrough/ats.h
> --- a/xen/drivers/passthrough/ats.h
> +++ b/xen/drivers/passthrough/ats.h
> @@ -24,6 +24,7 @@ struct pci_ats_dev {
> u8 bus;
> u8 devfn;
> u16 ats_queue_depth; /* ATS device invalidation queue depth */
> + void *iommu;
> };
>
> #define ATS_REG_CAP 4
> @@ -34,7 +35,7 @@ struct pci_ats_dev {
> extern struct list_head ats_devices;
> extern bool_t ats_enabled;
>
> -int enable_ats_device(int seg, int bus, int devfn);
> +int enable_ats_device(void *iommu, int seg, int bus, int devfn);
> void disable_ats_device(int seg, int bus, int devfn);
> struct pci_ats_dev *get_ats_device(int seg, int bus, int devfn);
>
> diff -r 4708591d8aa8 -r 90352a46dbcb xen/drivers/passthrough/vtd/iommu.c
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1442,7 +1442,7 @@ static int domain_context_mapping(
> ret = domain_context_mapping_one(domain, drhd->iommu, bus,
> devfn,
> pdev);
> if ( !ret && devfn == pdev->devfn && ats_device(pdev, drhd) > 0 )
> - enable_ats_device(seg, bus, devfn);
> + enable_ats_device(drhd->iommu, seg, bus, devfn);
>
> break;
>
> @@ -1930,7 +1930,7 @@ static int intel_iommu_enable_device(str
> if ( ret <= 0 )
> return ret;
>
> - ret = enable_ats_device(pdev->seg, pdev->bus, pdev->devfn);
> + ret = enable_ats_device(drhd->iommu, pdev->seg, pdev->bus, pdev-
> >devfn);
>
> return ret >= 0 ? 0 : ret;
> }
> diff -r 4708591d8aa8 -r 90352a46dbcb xen/drivers/passthrough/vtd/x86/ats.c
> --- a/xen/drivers/passthrough/vtd/x86/ats.c
> +++ b/xen/drivers/passthrough/vtd/x86/ats.c
> @@ -120,6 +120,10 @@ int dev_invalidate_iotlb(struct iommu *i
> {
> sid = (pdev->bus << 8) | pdev->devfn;
>
> + /* Only invalidate devices that belong to this IOMMU */
> + if ( !pdev->iommu || pdev->iommu != iommu )
> + continue;
> +
> switch ( type ) {
> case DMA_TLB_DSI_FLUSH:
> if ( !device_in_domain(iommu, pdev, did) )
> diff -r 4708591d8aa8 -r 90352a46dbcb xen/drivers/passthrough/x86/ats.c
> --- a/xen/drivers/passthrough/x86/ats.c
> +++ b/xen/drivers/passthrough/x86/ats.c
> @@ -23,7 +23,7 @@ LIST_HEAD(ats_devices);
> bool_t __read_mostly ats_enabled = 1;
> boolean_param("ats", ats_enabled);
>
> -int enable_ats_device(int seg, int bus, int devfn)
> +int enable_ats_device(void *iommu, int seg, int bus, int devfn)
> {
> struct pci_ats_dev *pdev = NULL;
> u32 value;
> @@ -66,6 +66,7 @@ int enable_ats_device(int seg, int bus,
> pdev->seg = seg;
> pdev->bus = bus;
> pdev->devfn = devfn;
> + pdev->iommu = iommu;
> value = pci_conf_read16(seg, bus, PCI_SLOT(devfn),
> PCI_FUNC(devfn), pos + ATS_REG_CAP);
> pdev->ats_queue_depth = value & ATS_QUEUE_DEPTH_MASK ?:
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] IOMMU: Prevent VT-d device IOTLB operations on wrong IOMMU
2014-06-12 11:40 [PATCH] IOMMU: Prevent VT-d device IOTLB operations on wrong IOMMU Malcolm Crossley
2014-06-12 12:00 ` Paul Durrant
@ 2014-06-12 12:05 ` Andrew Cooper
2014-06-13 11:13 ` Jan Beulich
2014-06-12 19:10 ` Tian, Kevin
2 siblings, 1 reply; 5+ messages in thread
From: Andrew Cooper @ 2014-06-12 12:05 UTC (permalink / raw)
To: Malcolm Crossley
Cc: kevin.tian, suravee.suthikulpanit, xen-devel,
Aravind.Gopalakrishnan, jbeulich, yang.z.zhang
On 12/06/14 12:40, Malcolm Crossley wrote:
> PCIE ATS allows for device's to contain IOTLB's, the VT-d code was iterating
> around all ATS capable devices and issuing IOTLB operations for all IOMMU's,
> even though each ATS device's is only accessible via one particular IOMMU.
>
> Issuing an IOMMU operation to a device not accessible via that IOMMU results
> in an IOMMU timeout because the device does not reply. VT-d IOMMU timeouts
> result in a Xen panic.
>
> Therefore this bug prevents any Intel system with 2 or more ATS enabled IOMMU's,
> each with an ATS device connected to them, from booting Xen.
>
> The patch add's a IOMMU pointer to the ATS device struct so the VT-d code can
> ensure it does not issue IOMMU ATS operations on the wrong IOMMU. A void
> pointer has to be used because AMD and Intel IOMMU implementations do not have
> a common IOMMU structure or indexing mechanism.
>
> Optimised AMD's IOMMU code by leveraging the new IOMMU field in the ATS
> structure.
>
> Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
One comment below, but functionally Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>
>
> diff -r 4708591d8aa8 -r 90352a46dbcb xen/drivers/passthrough/amd/iommu_cmd.c
> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
> @@ -303,8 +303,7 @@ void amd_iommu_flush_iotlb(u8 devfn, con
> if ( !pci_ats_enabled(ats_pdev->seg, ats_pdev->bus, ats_pdev->devfn) )
> return;
>
> - iommu = find_iommu_for_device(ats_pdev->seg,
> - PCI_BDF2(ats_pdev->bus, ats_pdev->devfn));
> + iommu = ats_pdev->iommu;
>
> if ( !iommu )
> {
> diff -r 4708591d8aa8 -r 90352a46dbcb xen/drivers/passthrough/amd/pci_amd_iommu.c
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -164,7 +164,7 @@ static void amd_iommu_setup_domain_devic
> !pci_ats_enabled(iommu->seg, bus, pdev->devfn) )
> {
> if ( devfn == pdev->devfn )
> - enable_ats_device(iommu->seg, bus, devfn);
> + enable_ats_device(iommu, iommu->seg, bus, devfn);
>
> amd_iommu_flush_iotlb(devfn, pdev, INV_IOMMU_ALL_PAGES_ADDRESS, 0);
> }
> diff -r 4708591d8aa8 -r 90352a46dbcb xen/drivers/passthrough/ats.h
> --- a/xen/drivers/passthrough/ats.h
> +++ b/xen/drivers/passthrough/ats.h
> @@ -24,6 +24,7 @@ struct pci_ats_dev {
> u8 bus;
> u8 devfn;
> u16 ats_queue_depth; /* ATS device invalidation queue depth */
> + void *iommu;
This could really do with a comment here explaining why a void *, and
which two types of structure it could end up pointing at.
> };
>
> #define ATS_REG_CAP 4
> @@ -34,7 +35,7 @@ struct pci_ats_dev {
> extern struct list_head ats_devices;
> extern bool_t ats_enabled;
>
> -int enable_ats_device(int seg, int bus, int devfn);
> +int enable_ats_device(void *iommu, int seg, int bus, int devfn);
> void disable_ats_device(int seg, int bus, int devfn);
> struct pci_ats_dev *get_ats_device(int seg, int bus, int devfn);
>
> diff -r 4708591d8aa8 -r 90352a46dbcb xen/drivers/passthrough/vtd/iommu.c
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1442,7 +1442,7 @@ static int domain_context_mapping(
> ret = domain_context_mapping_one(domain, drhd->iommu, bus, devfn,
> pdev);
> if ( !ret && devfn == pdev->devfn && ats_device(pdev, drhd) > 0 )
> - enable_ats_device(seg, bus, devfn);
> + enable_ats_device(drhd->iommu, seg, bus, devfn);
>
> break;
>
> @@ -1930,7 +1930,7 @@ static int intel_iommu_enable_device(str
> if ( ret <= 0 )
> return ret;
>
> - ret = enable_ats_device(pdev->seg, pdev->bus, pdev->devfn);
> + ret = enable_ats_device(drhd->iommu, pdev->seg, pdev->bus, pdev->devfn);
>
> return ret >= 0 ? 0 : ret;
> }
> diff -r 4708591d8aa8 -r 90352a46dbcb xen/drivers/passthrough/vtd/x86/ats.c
> --- a/xen/drivers/passthrough/vtd/x86/ats.c
> +++ b/xen/drivers/passthrough/vtd/x86/ats.c
> @@ -120,6 +120,10 @@ int dev_invalidate_iotlb(struct iommu *i
> {
> sid = (pdev->bus << 8) | pdev->devfn;
>
> + /* Only invalidate devices that belong to this IOMMU */
> + if ( !pdev->iommu || pdev->iommu != iommu )
> + continue;
> +
> switch ( type ) {
> case DMA_TLB_DSI_FLUSH:
> if ( !device_in_domain(iommu, pdev, did) )
> diff -r 4708591d8aa8 -r 90352a46dbcb xen/drivers/passthrough/x86/ats.c
> --- a/xen/drivers/passthrough/x86/ats.c
> +++ b/xen/drivers/passthrough/x86/ats.c
> @@ -23,7 +23,7 @@ LIST_HEAD(ats_devices);
> bool_t __read_mostly ats_enabled = 1;
> boolean_param("ats", ats_enabled);
>
> -int enable_ats_device(int seg, int bus, int devfn)
> +int enable_ats_device(void *iommu, int seg, int bus, int devfn)
> {
> struct pci_ats_dev *pdev = NULL;
> u32 value;
> @@ -66,6 +66,7 @@ int enable_ats_device(int seg, int bus,
> pdev->seg = seg;
> pdev->bus = bus;
> pdev->devfn = devfn;
> + pdev->iommu = iommu;
> value = pci_conf_read16(seg, bus, PCI_SLOT(devfn),
> PCI_FUNC(devfn), pos + ATS_REG_CAP);
> pdev->ats_queue_depth = value & ATS_QUEUE_DEPTH_MASK ?:
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] IOMMU: Prevent VT-d device IOTLB operations on wrong IOMMU
2014-06-12 12:05 ` Andrew Cooper
@ 2014-06-13 11:13 ` Jan Beulich
0 siblings, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2014-06-13 11:13 UTC (permalink / raw)
To: andrew.cooper3, malcolm.crossley
Cc: yang.z.zhang, kevin.tian, Aravind.Gopalakrishnan,
suravee.suthikulpanit, xen-devel
>>> Andrew Cooper <andrew.cooper3@citrix.com> 06/12/14 2:05 PM >>>
>>On 12/06/14 12:40, Malcolm Crossley wrote:
>> --- a/xen/drivers/passthrough/ats.h
>> +++ b/xen/drivers/passthrough/ats.h
>> @@ -24,6 +24,7 @@ struct pci_ats_dev {
>> u8 bus;
>> u8 devfn;
>> u16 ats_queue_depth; /* ATS device invalidation queue depth */
>> + void *iommu;
>
>This could really do with a comment here explaining why a void *, and
>which two types of structure it could end up pointing at.
Right. Or make it a transparent union in the first place. And in any case, making
more clear that it's for nothing but looking at, add "const".
Jan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] IOMMU: Prevent VT-d device IOTLB operations on wrong IOMMU
2014-06-12 11:40 [PATCH] IOMMU: Prevent VT-d device IOTLB operations on wrong IOMMU Malcolm Crossley
2014-06-12 12:00 ` Paul Durrant
2014-06-12 12:05 ` Andrew Cooper
@ 2014-06-12 19:10 ` Tian, Kevin
2 siblings, 0 replies; 5+ messages in thread
From: Tian, Kevin @ 2014-06-12 19:10 UTC (permalink / raw)
To: Malcolm Crossley, xen-devel@lists.xen.org
Cc: Zhang, Yang Z, Aravind.Gopalakrishnan@amd.com,
suravee.suthikulpanit@amd.com, jbeulich@suse.com
> From: Malcolm Crossley [mailto:malcolm.crossley@citrix.com]
> Sent: Thursday, June 12, 2014 4:40 AM
>
> PCIE ATS allows for device's to contain IOTLB's, the VT-d code was iterating
> around all ATS capable devices and issuing IOTLB operations for all IOMMU's,
> even though each ATS device's is only accessible via one particular IOMMU.
>
> Issuing an IOMMU operation to a device not accessible via that IOMMU
> results
> in an IOMMU timeout because the device does not reply. VT-d IOMMU
> timeouts
> result in a Xen panic.
>
> Therefore this bug prevents any Intel system with 2 or more ATS enabled
> IOMMU's,
> each with an ATS device connected to them, from booting Xen.
>
> The patch add's a IOMMU pointer to the ATS device struct so the VT-d code
> can
> ensure it does not issue IOMMU ATS operations on the wrong IOMMU. A void
> pointer has to be used because AMD and Intel IOMMU implementations do
> not have
> a common IOMMU structure or indexing mechanism.
>
> Optimised AMD's IOMMU code by leveraging the new IOMMU field in the ATS
> structure.
>
> Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
>
Acked-by: Kevin Tian <kevin.tian@intel.com>
> diff -r 4708591d8aa8 -r 90352a46dbcb
> xen/drivers/passthrough/amd/iommu_cmd.c
> --- a/xen/drivers/passthrough/amd/iommu_cmd.c
> +++ b/xen/drivers/passthrough/amd/iommu_cmd.c
> @@ -303,8 +303,7 @@ void amd_iommu_flush_iotlb(u8 devfn, con
> if ( !pci_ats_enabled(ats_pdev->seg, ats_pdev->bus, ats_pdev->devfn) )
> return;
>
> - iommu = find_iommu_for_device(ats_pdev->seg,
> - PCI_BDF2(ats_pdev->bus,
> ats_pdev->devfn));
> + iommu = ats_pdev->iommu;
>
> if ( !iommu )
> {
> diff -r 4708591d8aa8 -r 90352a46dbcb
> xen/drivers/passthrough/amd/pci_amd_iommu.c
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -164,7 +164,7 @@ static void amd_iommu_setup_domain_devic
> !pci_ats_enabled(iommu->seg, bus, pdev->devfn) )
> {
> if ( devfn == pdev->devfn )
> - enable_ats_device(iommu->seg, bus, devfn);
> + enable_ats_device(iommu, iommu->seg, bus, devfn);
>
> amd_iommu_flush_iotlb(devfn, pdev,
> INV_IOMMU_ALL_PAGES_ADDRESS, 0);
> }
> diff -r 4708591d8aa8 -r 90352a46dbcb xen/drivers/passthrough/ats.h
> --- a/xen/drivers/passthrough/ats.h
> +++ b/xen/drivers/passthrough/ats.h
> @@ -24,6 +24,7 @@ struct pci_ats_dev {
> u8 bus;
> u8 devfn;
> u16 ats_queue_depth; /* ATS device invalidation queue depth */
> + void *iommu;
> };
>
> #define ATS_REG_CAP 4
> @@ -34,7 +35,7 @@ struct pci_ats_dev {
> extern struct list_head ats_devices;
> extern bool_t ats_enabled;
>
> -int enable_ats_device(int seg, int bus, int devfn);
> +int enable_ats_device(void *iommu, int seg, int bus, int devfn);
> void disable_ats_device(int seg, int bus, int devfn);
> struct pci_ats_dev *get_ats_device(int seg, int bus, int devfn);
>
> diff -r 4708591d8aa8 -r 90352a46dbcb xen/drivers/passthrough/vtd/iommu.c
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -1442,7 +1442,7 @@ static int domain_context_mapping(
> ret = domain_context_mapping_one(domain, drhd->iommu, bus,
> devfn,
> pdev);
> if ( !ret && devfn == pdev->devfn && ats_device(pdev, drhd) > 0 )
> - enable_ats_device(seg, bus, devfn);
> + enable_ats_device(drhd->iommu, seg, bus, devfn);
>
> break;
>
> @@ -1930,7 +1930,7 @@ static int intel_iommu_enable_device(str
> if ( ret <= 0 )
> return ret;
>
> - ret = enable_ats_device(pdev->seg, pdev->bus, pdev->devfn);
> + ret = enable_ats_device(drhd->iommu, pdev->seg, pdev->bus,
> pdev->devfn);
>
> return ret >= 0 ? 0 : ret;
> }
> diff -r 4708591d8aa8 -r 90352a46dbcb xen/drivers/passthrough/vtd/x86/ats.c
> --- a/xen/drivers/passthrough/vtd/x86/ats.c
> +++ b/xen/drivers/passthrough/vtd/x86/ats.c
> @@ -120,6 +120,10 @@ int dev_invalidate_iotlb(struct iommu *i
> {
> sid = (pdev->bus << 8) | pdev->devfn;
>
> + /* Only invalidate devices that belong to this IOMMU */
> + if ( !pdev->iommu || pdev->iommu != iommu )
> + continue;
> +
> switch ( type ) {
> case DMA_TLB_DSI_FLUSH:
> if ( !device_in_domain(iommu, pdev, did) )
> diff -r 4708591d8aa8 -r 90352a46dbcb xen/drivers/passthrough/x86/ats.c
> --- a/xen/drivers/passthrough/x86/ats.c
> +++ b/xen/drivers/passthrough/x86/ats.c
> @@ -23,7 +23,7 @@ LIST_HEAD(ats_devices);
> bool_t __read_mostly ats_enabled = 1;
> boolean_param("ats", ats_enabled);
>
> -int enable_ats_device(int seg, int bus, int devfn)
> +int enable_ats_device(void *iommu, int seg, int bus, int devfn)
> {
> struct pci_ats_dev *pdev = NULL;
> u32 value;
> @@ -66,6 +66,7 @@ int enable_ats_device(int seg, int bus,
> pdev->seg = seg;
> pdev->bus = bus;
> pdev->devfn = devfn;
> + pdev->iommu = iommu;
> value = pci_conf_read16(seg, bus, PCI_SLOT(devfn),
> PCI_FUNC(devfn), pos +
> ATS_REG_CAP);
> pdev->ats_queue_depth = value & ATS_QUEUE_DEPTH_MASK ?:
^ permalink raw reply [flat|nested] 5+ messages in thread