amd iommu: Add ATS support (v3)

PATCH New Wei Wang 2 participants 13 messages Nov 3, 2011

Share & tools

Wei Wang

author

2011-11-03 10:13 UTC #0 lore raw

PATCH 1/1 amd iommu: Add ATS support (v3)

This patch set enables ats devices on amd systems with following changes:
1) Make vendor independent ATS codes more public.
2) Add additional helper functions for ats.
3) Add amd specific enablement.

Jan, this version should have fixed all issues from my 2nd try. Please check it.

Thanks,
Wei

Wei Wang

author

2011-11-03 10:13 UTC #1 lore raw

PATCH 1/1 ats: Move some ats functions to a new directory

# HG changeset patch
# User Wei Wang <[email protected]>
# Date 1320314537 -3600
# Node ID d422e3cf7976c76c57fc2d455b784d0fcc24d06b
# Parent 119bccb1cc5eee1364bbbd3bd1a30f22e9db703a
ats: Move some ats functions to a new directory. Remove VTD prefix from debug output.
passhrough/x86 holds vendor neutral codes for x86 architecture.

Signed-off-by Wei Wang <[email protected]>

xen/drivers/passthrough/Makefile +1 -0

diff -r 119bccb1cc5e -r d422e3cf7976 xen/drivers/passthrough/Makefile
@@ -1,6 +1,7 @@
 subdir-$(x86) += vtd
 subdir-$(ia64) += vtd
 subdir-$(x86) += amd
+subdir-$(x86) += x86
 
 obj-y += iommu.o
 obj-y += io.o

xen/drivers/passthrough/ats.h new file +39 -0

diff -r 119bccb1cc5e -r d422e3cf7976 xen/drivers/passthrough/ats.h
@@ -0,0 +1,39 @@
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ */
+
+#ifndef _ATS_H_
+#define _ATS_H_
+
+#define ATS_REG_CAP    4
+#define ATS_REG_CTL    6
+#define ATS_QUEUE_DEPTH_MASK     0xF
+#define ATS_ENABLE               (1<<15)
+
+struct pci_ats_dev {
+    struct list_head list;
+    u16 seg;
+    u8 bus;
+    u8 devfn;
+    u16 ats_queue_depth;    /* ATS device invalidation queue depth */
+};
+
+extern struct list_head ats_devices;
+extern bool_t ats_enabled;
+
+int enable_ats_device(int seg, int bus, int devfn);
+void disable_ats_device(int seg, int bus, int devfn);
+
+#endif /* _ATS_H_ */
+

xen/drivers/passthrough/vtd/extern.h +0 -4

diff -r 119bccb1cc5e -r d422e3cf7976 xen/drivers/passthrough/vtd/extern.h
@@ -57,13 +57,9 @@ struct acpi_drhd_unit * iommu_to_drhd(st
 struct acpi_rhsa_unit * drhd_to_rhsa(struct acpi_drhd_unit *drhd);
 
 #ifdef CONFIG_X86_64
-extern bool_t ats_enabled;
-
 struct acpi_drhd_unit * find_ats_dev_drhd(struct iommu *iommu);
 
 int ats_device(const struct pci_dev *, const struct acpi_drhd_unit *);
-int enable_ats_device(int seg, int bus, int devfn);
-void disable_ats_device(int seg, int bus, int devfn);
 
 int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
                          u64 addr, unsigned int size_order, u64 type);

xen/drivers/passthrough/vtd/iommu.c +1 -0

diff -r 119bccb1cc5e -r d422e3cf7976 xen/drivers/passthrough/vtd/iommu.c
@@ -40,6 +40,7 @@
 #include "dmar.h"
 #include "extern.h"
 #include "vtd.h"
+#include "../ats.h"
 
 #ifdef __ia64__
 #define nr_ioapics              iosapic_get_nr_iosapics()

xen/drivers/passthrough/vtd/x86/ats.c +1 ~~-133~~

149 lines not on this page · raw message

xen/drivers/passthrough/x86/Makefile new file +1 -0

diff -r 119bccb1cc5e -r d422e3cf7976 xen/drivers/passthrough/x86/Makefile
@@ -0,0 +1,1 @@
+obj-y += ats.o
\ No newline at end of file

xen/drivers/passthrough/x86/ats.c new file +139 -0

139 lines not on this page · raw message

Wei Wang

author

2011-11-03 10:13 UTC #3 lore raw

PATCH 1/1 amd iommu: Fix iommu page size encoding when page order > 0

In reply to Wei Wang

# HG changeset patch
# User Wei Wang <[email protected]>
# Date 1320314617 -3600
# Node ID d0c38cb215cd96e01de27eadf5ec0a5e711de448 # Parent d422e3cf7976c76c57fc2d455b784d0fcc24d06b amd iommu: Fix iommu page size encoding when page order > 0

Signed-off-by Wei Wang <[email protected]>

xen/drivers/passthrough/amd/iommu_map.c +14 ~~-13~~

diff -r d422e3cf7976 -r d0c38cb215cd xen/drivers/passthrough/amd/iommu_map.c
@@ -77,23 +77,24 @@ static void invalidate_iommu_pages(struc
 {
     u64 addr_lo, addr_hi;
     u32 cmd[4], entry;
-    u64 mask = 0;
     int sflag = 0, pde = 0;
 
+    ASSERT ( order == 0 || order == 9 || order == 18 );
+
+    /* All pages associated with the domainID are invalidated */
+    if ( order || (io_addr == INV_IOMMU_ALL_PAGES_ADDRESS ) )
+    {
+        sflag = 1;
+        pde = 1;
+    }
+
     /* If sflag == 1, the size of the invalidate command is determined
      by the first zero bit in the address starting from Address[12] */
-    if ( order == 9 || order == 18 )
+    if ( order )
     {
-        mask = ((1ULL << (order - 1)) - 1) << PAGE_SHIFT;
-        io_addr |= mask;
-        sflag = 1;
-    }
-
-    /* All pages associated with the domainID are invalidated */
-    else if ( io_addr == 0x7FFFFFFFFFFFF000ULL )
-    {
-        sflag = 1;
-        pde = 1;
+        u64 mask = 1ULL << (order - 1 + PAGE_SHIFT);
+        io_addr &= ~mask;
+        io_addr |= mask - 1;
     }
 
     addr_lo = io_addr & DMA_32BIT_MASK;

@@ -917,7 +918,7 @@ static void _amd_iommu_flush_pages(struc
 
 void amd_iommu_flush_all_pages(struct domain *d)
 {
-    _amd_iommu_flush_pages(d, 0x7FFFFFFFFFFFFULL, 0);
+    _amd_iommu_flush_pages(d, INV_IOMMU_ALL_PAGES_ADDRESS, 0);
 }
 
 void amd_iommu_flush_pages(struct domain *d,

xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +2 -0

diff -r d422e3cf7976 -r d0c38cb215cd xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
@@ -407,4 +407,6 @@
 #define INT_REMAP_ENTRY_VECTOR_MASK     0x00FF0000
 #define INT_REMAP_ENTRY_VECTOR_SHIFT    16
 
+#define INV_IOMMU_ALL_PAGES_ADDRESS      0x7FFFFFFFFFFFFFFFULL
+
 #endif /* _ASM_X86_64_AMD_IOMMU_DEFS_H */

Wei Wang

author

2011-11-03 10:13 UTC #10 lore raw

PATCH 1/1 ats: Add new ATS helper functions

In reply to Wei Wang

# HG changeset patch
# User Wei Wang <[email protected]>
# Date 1320314663 -3600
# Node ID c839972486f86a0833595bebae0baa0cd2c0d15e # Parent d0c38cb215cd96e01de27eadf5ec0a5e711de448
ats: Add new ATS helper functions

Signed-off-by Wei Wang <[email protected]>

xen/drivers/passthrough/ats.h +23 -0

diff -r d0c38cb215cd -r c839972486f8 xen/drivers/passthrough/ats.h
@@ -21,6 +21,8 @@
 #define ATS_QUEUE_DEPTH_MASK     0xF
 #define ATS_ENABLE               (1<<15)
 
+#include <xen/pci_regs.h>
+
 struct pci_ats_dev {
     struct list_head list;
     u16 seg;

@@ -34,6 +36,27 @@ extern bool_t ats_enabled;
 
 int enable_ats_device(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);
 
+static inline int pci_ats_enabled(int seg, int bus, int devfn)
+{
+    u32 value;
+    int pos;
+
+    pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
+    BUG_ON(!pos);
+
+    value = pci_conf_read16(seg, bus, PCI_SLOT(devfn),
+                            PCI_FUNC(devfn), pos + ATS_REG_CTL);
+    return value & ATS_ENABLE;
+}
+
+static inline int pci_ats_device(int seg, int bus, int devfn)
+{
+    if ( !ats_enabled )
+        return 0;
+
+    return pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
+}
 #endif /* _ATS_H_ */

xen/drivers/passthrough/x86/ats.c +16 -0

diff -r d0c38cb215cd -r c839972486f8 xen/drivers/passthrough/x86/ats.c
@@ -137,3 +137,19 @@ void disable_ats_device(int seg, int bus
                 "%04x:%02x:%02x.%u: ATS is disabled\n",
                 seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
 }
+
+struct pci_ats_dev *get_ats_device(int seg, int bus, int devfn)
+{
+    struct pci_ats_dev *pdev;
+
+    if ( !pci_ats_device(seg, bus, devfn) )
+        return NULL;
+
+    list_for_each_entry ( pdev, &ats_devices, list )
+    {
+        if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
+            return pdev;
+    }
+
+    return NULL;
+}

Wei Wang

author

2011-11-03 10:13 UTC #11 lore raw

PATCH 1/1 amd iommu: add iotlb invalidation command

In reply to Wei Wang

# HG changeset patch
# User Wei Wang <[email protected]>
# Date 1320314694 -3600
# Node ID c9dcbe6d80a355f1e50ff910d618660a78b40653 # Parent c839972486f86a0833595bebae0baa0cd2c0d15e amd iommu: add iotlb invalidation command

Signed-off-by Wei Wang <[email protected]>

xen/drivers/passthrough/amd/iommu_map.c +131 -0

diff -r c839972486f8 -r c9dcbe6d80a3 xen/drivers/passthrough/amd/iommu_map.c
@@ -23,6 +23,8 @@
 #include <xen/hvm/iommu.h>
 #include <asm/amd-iommu.h>
 #include <asm/hvm/svm/amd-iommu-proto.h>
+#include "../ats.h"
+#include <xen/pci.h>
 
 static int queue_iommu_command(struct amd_iommu *iommu, u32 cmd[])
 {

@@ -128,6 +130,75 @@ static void invalidate_iommu_pages(struc
     send_iommu_command(iommu, cmd);
 }
 
+static void invalidate_iotlb_pages(struct amd_iommu *iommu,
+                                   u16 maxpend, u32 pasid, u16 queueid,
+                                   u64 io_addr, u16 dev_id, u16 order)
+{
+    u64 addr_lo, addr_hi;
+    u32 cmd[4], entry;
+    int sflag = 0;
+
+    ASSERT ( order == 0 || order == 9 || order == 18 );
+
+    if ( order || (io_addr == INV_IOMMU_ALL_PAGES_ADDRESS ) )
+        sflag = 1;
+
+    /* If sflag == 1, the size of the invalidate command is determined
+     by the first zero bit in the address starting from Address[12] */
+    if ( order )
+    {
+        u64 mask = 1ULL << (order - 1 + PAGE_SHIFT);
+        io_addr &= ~mask;
+        io_addr |= mask - 1;
+    }
+
+    addr_lo = io_addr & DMA_32BIT_MASK;
+    addr_hi = io_addr >> 32;
+
+    set_field_in_reg_u32(dev_id, 0,
+                         IOMMU_INV_IOTLB_PAGES_DEVICE_ID_MASK,
+                         IOMMU_INV_IOTLB_PAGES_DEVICE_ID_SHIFT, &entry);
+
+    set_field_in_reg_u32(maxpend, entry,
+                         IOMMU_INV_IOTLB_PAGES_MAXPEND_MASK,
+                         IOMMU_INV_IOTLB_PAGES_MAXPEND_SHIFT, &entry);
+
+    set_field_in_reg_u32(pasid & 0xff, entry,
+                         IOMMU_INV_IOTLB_PAGES_PASID1_MASK,
+                         IOMMU_INV_IOTLB_PAGES_PASID1_SHIFT, &entry);
+    cmd[0] = entry;
+
+    set_field_in_reg_u32(IOMMU_CMD_INVALIDATE_IOTLB_PAGES, 0,
+                         IOMMU_CMD_OPCODE_MASK, IOMMU_CMD_OPCODE_SHIFT,
+                         &entry);
+
+    set_field_in_reg_u32(pasid >> 8, entry,
+                         IOMMU_INV_IOTLB_PAGES_PASID2_MASK, 
+                         IOMMU_INV_IOTLB_PAGES_PASID2_SHIFT,
+                         &entry);
+
+    set_field_in_reg_u32(queueid, entry,
+                         IOMMU_INV_IOTLB_PAGES_QUEUEID_MASK, 
+                         IOMMU_INV_IOTLB_PAGES_QUEUEID_SHIFT,
+                         &entry);
+    cmd[1] = entry;
+
+    set_field_in_reg_u32(sflag, 0,
+                         IOMMU_INV_IOTLB_PAGES_S_FLAG_MASK,
+                         IOMMU_INV_IOTLB_PAGES_S_FLAG_MASK, &entry);
+
+    set_field_in_reg_u32((u32)addr_lo >> PAGE_SHIFT, entry,
+                         IOMMU_INV_IOTLB_PAGES_ADDR_LOW_MASK,
+                         IOMMU_INV_IOTLB_PAGES_ADDR_LOW_SHIFT, &entry);
+    cmd[2] = entry;
+
+    set_field_in_reg_u32((u32)addr_hi, 0,
+                         IOMMU_INV_IOTLB_PAGES_ADDR_HIGH_MASK,
+                         IOMMU_INV_IOTLB_PAGES_ADDR_HIGH_SHIFT, &entry);
+    cmd[3] = entry;
+
+    send_iommu_command(iommu, cmd);
+}
 void flush_command_buffer(struct amd_iommu *iommu)
 {
     u32 cmd[4], status;

@@ -896,6 +967,63 @@ int amd_iommu_reserve_domain_unity_map(s
     return 0;
 }
 
+void amd_iommu_flush_iotlb(struct pci_dev *pdev,
+                           uint64_t gaddr, unsigned int order)
+{
+    unsigned long flags;
+    struct amd_iommu *iommu;
+    unsigned int bdf, req_id, queueid, maxpend;
+    struct pci_ats_dev *ats_pdev;
+
+    if ( !ats_enabled )
+        return;
+
+    ats_pdev = get_ats_device(pdev->seg, pdev->bus, pdev->devfn);
+    if ( ats_pdev == NULL )
+        return;
+
+    if ( !pci_ats_enabled(ats_pdev->seg, 
+                          ats_pdev->bus, ats_pdev->devfn) )
+        return;
+
+    bdf = PCI_BDF2(ats_pdev->bus, ats_pdev->devfn);
+    iommu = find_iommu_for_device(ats_pdev->seg, bdf);
+
+    if ( !iommu )
+    {
+        AMD_IOMMU_DEBUG("%s: Fail to find iommu for device %04x:%02x:%02x.%u\n",
+                        __func__, ats_pdev->seg, ats_pdev->bus, 
+                        PCI_SLOT(ats_pdev->devfn),
+                        PCI_FUNC(ats_pdev->devfn));
+        return;
+    }
+
+    if ( !iommu->iotlb_support )
+        return;
+
+    req_id = get_dma_requestor_id(iommu->seg, bdf);
+    queueid = req_id;
+    maxpend = (ats_pdev->ats_queue_depth + 32) & 0xff;
+
+    /* send INVALIDATE_IOTLB_PAGES command */
+    spin_lock_irqsave(&iommu->lock, flags);
+    invalidate_iotlb_pages(iommu, maxpend, 0, queueid,
+                           gaddr, req_id, order);
+    flush_command_buffer(iommu);
+    spin_unlock_irqrestore(&iommu->lock, flags);
+}
+
+static void amd_iommu_flush_all_iotlbs(struct domain *d, 
+                                       uint64_t gaddr, unsigned int order)
+{
+    struct pci_dev *pdev;
+
+    if ( !ats_enabled )
+        return;
+
+    for_each_pdev( d, pdev )
+        amd_iommu_flush_iotlb(pdev, gaddr, order);
+}
 
 /* Flush iommu cache after p2m changes. */
 static void _amd_iommu_flush_pages(struct domain *d,

@@ -914,6 +1042,9 @@ static void _amd_iommu_flush_pages(struc
         flush_command_buffer(iommu);
         spin_unlock_irqrestore(&iommu->lock, flags);
     }
+
+    if ( ats_enabled )
+        amd_iommu_flush_all_iotlbs(d, gaddr, order);
 }
 
 void amd_iommu_flush_all_pages(struct domain *d)

xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +18 -0

diff -r c839972486f8 -r c9dcbe6d80a3 xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
@@ -233,6 +233,24 @@
 #define IOMMU_INV_INT_TABLE_DEVICE_ID_MASK   0x0000FFFF
 #define IOMMU_INV_INT_TABLE_DEVICE_ID_SHIFT  0
 
+/* INVALIDATE_IOTLB_PAGES command */
+#define IOMMU_INV_IOTLB_PAGES_MAXPEND_MASK          0xff000000
+#define IOMMU_INV_IOTLB_PAGES_MAXPEND_SHIFT         24
+#define IOMMU_INV_IOTLB_PAGES_PASID1_MASK           0x00ff0000
+#define IOMMU_INV_IOTLB_PAGES_PASID1_SHIFT          16
+#define IOMMU_INV_IOTLB_PAGES_PASID2_MASK           0x0fff0000
+#define IOMMU_INV_IOTLB_PAGES_PASID2_SHIFT          16
+#define IOMMU_INV_IOTLB_PAGES_QUEUEID_MASK          0x0000ffff
+#define IOMMU_INV_IOTLB_PAGES_QUEUEID_SHIFT         0
+#define IOMMU_INV_IOTLB_PAGES_DEVICE_ID_MASK        0x0000FFFF
+#define IOMMU_INV_IOTLB_PAGES_DEVICE_ID_SHIFT       0
+#define IOMMU_INV_IOTLB_PAGES_ADDR_LOW_MASK         0xFFFFF000
+#define IOMMU_INV_IOTLB_PAGES_ADDR_LOW_SHIFT        12
+#define IOMMU_INV_IOTLB_PAGES_ADDR_HIGH_MASK        0xFFFFFFFF
+#define IOMMU_INV_IOTLB_PAGES_ADDR_HIGH_SHIFT       0
+#define IOMMU_INV_IOTLB_PAGES_S_FLAG_MASK           0x00000001
+#define IOMMU_INV_IOTLB_PAGES_S_FLAG_SHIFT          0
+
 /* Event Log */
 #define IOMMU_EVENT_LOG_BASE_LOW_OFFSET		0x10
 #define IOMMU_EVENT_LOG_BASE_HIGH_OFFSET	0x14

xen/include/asm-x86/hvm/svm/amd-iommu-proto.h +2 -0

diff -r c839972486f8 -r c9dcbe6d80a3 xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
@@ -55,6 +55,8 @@ int amd_iommu_unmap_page(struct domain *
 void amd_iommu_flush_pages(struct domain *d, unsigned long gfn,
                            unsigned int order);
 void amd_iommu_flush_all_pages(struct domain *d);
+void amd_iommu_flush_iotlb(struct pci_dev *pdev,
+                           uint64_t gaddr, unsigned int order);
 
 u64 amd_iommu_get_next_table_from_pte(u32 *entry);
 int amd_iommu_reserve_domain_unity_map(struct domain *domain,

Wei Wang

author

2011-11-03 10:13 UTC #12 lore raw

PATCH 1/1 amd iommu: enable ats devices

In reply to Wei Wang

# HG changeset patch
# User Wei Wang <[email protected]>
# Date 1320314731 -3600
# Node ID 7316595e8f47fcf5a106d897f99105a14d7e646c # Parent c9dcbe6d80a355f1e50ff910d618660a78b40653 amd iommu: enable ats devices

Signed-off-by Wei Wang <[email protected]>

xen/drivers/passthrough/amd/iommu_map.c +11 -0

diff -r c9dcbe6d80a3 -r 7316595e8f47 xen/drivers/passthrough/amd/iommu_map.c
@@ -370,6 +370,17 @@ void amd_iommu_set_root_page_table(
     dte[0] = entry;
 }
 
+void iommu_dte_set_iotlb(u32 *dte, u8 i)
+{
+    u32 entry;
+
+    entry = dte[3];
+    set_field_in_reg_u32(!!i, entry,
+                         IOMMU_DEV_TABLE_IOTLB_SUPPORT_MASK,
+                         IOMMU_DEV_TABLE_IOTLB_SUPPORT_SHIFT, &entry);
+    dte[3] = entry;
+}
+
 void __init amd_iommu_set_intremap_table(
     u32 *dte, u64 intremap_ptr, u8 int_valid)
 {

xen/drivers/passthrough/amd/pci_amd_iommu.c +46 -6

126 lines not on this page · raw message

xen/include/asm-x86/hvm/svm/amd-iommu-proto.h +1 -0

diff -r c9dcbe6d80a3 -r 7316595e8f47 xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
@@ -75,6 +75,7 @@ void amd_iommu_set_intremap_table(
     u32 *dte, u64 intremap_ptr, u8 int_valid);
 void amd_iommu_set_root_page_table(
     u32 *dte, u64 root_ptr, u16 domain_id, u8 paging_mode, u8 valid);
+void iommu_dte_set_iotlb(u32 *dte, u8 i);
 void invalidate_dev_table_entry(struct amd_iommu *iommu, u16 devic_id);
 
 /* send cmd to iommu */

Jan Beulich

2011-11-03 12:46 UTC #2 lore raw

Re: [PATCH 1 of 5] ats: Move some ats functions to a new directory

In reply to Wei Wang

Quoted 73 lines

Quoted 1 line
Quoted 1 line · Wei Wang
# HG changeset patch
# User Wei Wang <[email protected]>
# Date 1320314537 -3600
# Node ID d422e3cf7976c76c57fc2d455b784d0fcc24d06b
# Parent 119bccb1cc5eee1364bbbd3bd1a30f22e9db703a
ats: Move some ats functions to a new directory. Remove VTD prefix from debug output.
passhrough/x86 holds vendor neutral codes for x86 architecture.
Signed-off-by Wei Wang <[email protected]>
diff -r 119bccb1cc5e -r d422e3cf7976 xen/drivers/passthrough/Makefile
xen/drivers/passthrough/Makefile +1 -0
@@ -1,6 +1,7 @@
 subdir-$(x86) += vtd
 subdir-$(ia64) += vtd
 subdir-$(x86) += amd
+subdir-$(x86) += x86
 
 obj-y += iommu.o
 obj-y += io.o
diff -r 119bccb1cc5e -r d422e3cf7976 xen/drivers/passthrough/ats.h
xen/drivers/passthrough/ats.h new file +8 -0
@@ -0,0 +1,39 @@
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License 
for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ */
xen/drivers/passthrough/ats.h new file +25 -0
+
+#ifndef _ATS_H_
+#define _ATS_H_
+
+#define ATS_REG_CAP    4
+#define ATS_REG_CTL    6
+#define ATS_QUEUE_DEPTH_MASK     0xF
+#define ATS_ENABLE               (1<<15)
+
+struct pci_ats_dev {
+    struct list_head list;
+    u16 seg;
+    u8 bus;
+    u8 devfn;
+    u16 ats_queue_depth;    /* ATS device invalidation queue depth */
+};
+
+extern struct list_head ats_devices;
+extern bool_t ats_enabled;
+
+int enable_ats_device(int seg, int bus, int devfn);
+void disable_ats_device(int seg, int bus, int devfn);
+
+#endif /* _ATS_H_ */
+
diff -r 119bccb1cc5e -r d422e3cf7976 xen/drivers/passthrough/vtd/extern.h

xen/drivers/passthrough/vtd/extern.h struct acpi_drhd_unit * iommu_to_drhd(st · line 59

Quoted 3 lines

xen/drivers/passthrough/vtd/extern.h +0 -0

@@ -57,13 +57,9 @@ struct acpi_drhd_unit * iommu_to_drhd(st
 struct acpi_rhsa_unit * drhd_to_rhsa(struct acpi_drhd_unit *drhd);
 
 #ifdef CONFIG_X86_64

Did you overlook this conditional? It and the respective inline functions and #define-s must be added/moved to the new header too.

Quoted 180 lines

xen/drivers/passthrough/vtd/extern.h +0 -4

-extern bool_t ats_enabled;
-
 struct acpi_drhd_unit * find_ats_dev_drhd(struct iommu *iommu);
 
 int ats_device(const struct pci_dev *, const struct acpi_drhd_unit *);
-int enable_ats_device(int seg, int bus, int devfn);
-void disable_ats_device(int seg, int bus, int devfn);
 
 int dev_invalidate_iotlb(struct iommu *iommu, u16 did,
                          u64 addr, unsigned int size_order, u64 type);

diff -r 119bccb1cc5e -r d422e3cf7976 xen/drivers/passthrough/vtd/iommu.c

xen/drivers/passthrough/vtd/iommu.c +1 -0

@@ -40,6 +40,7 @@
 #include "dmar.h"
 #include "extern.h"
 #include "vtd.h"
+#include "../ats.h"
 
 #ifdef __ia64__
 #define nr_ioapics              iosapic_get_nr_iosapics()

diff -r 119bccb1cc5e -r d422e3cf7976 xen/drivers/passthrough/vtd/x86/ats.c

xen/drivers/passthrough/vtd/x86/ats.c +1 ~~-133~~

@@ -27,51 +27,10 @@
 #include "../dmar.h"
 #include "../vtd.h"
 #include "../extern.h"
+#include "../../ats.h"
 
 static LIST_HEAD(ats_dev_drhd_units);
 
-#define ATS_REG_CAP    4
-#define ATS_REG_CTL    6
-#define ATS_QUEUE_DEPTH_MASK     0xF
-#define ATS_ENABLE               (1<<15)
-
-struct pci_ats_dev {
-    struct list_head list;
-    u16 seg;
-    u8 bus;
-    u8 devfn;
-    u16 ats_queue_depth;    /* ATS device invalidation queue depth */
-};
-static LIST_HEAD(ats_devices);
-
-static void parse_ats_param(char *s);
-custom_param("ats", parse_ats_param);
-
-bool_t __read_mostly ats_enabled = 1;
-
-static void __init parse_ats_param(char *s)
-{
-    char *ss;
-
-    do {
-        ss = strchr(s, ',');
-        if ( ss )
-            *ss = '\0';
-
-        switch ( parse_bool(s) )
-        {
-        case 0:
-            ats_enabled = 0;
-            break;
-        case 1:
-            ats_enabled = 1;
-            break;
-        }
-
-        s = ss + 1;
-    } while ( ss );
-}
-
 struct acpi_drhd_unit * find_ats_dev_drhd(struct iommu *iommu)
 {
     struct acpi_drhd_unit *drhd;

@@ -113,97 +72,6 @@ int ats_device(const struct pci_dev *pde
     return pos;
 }
 
-int enable_ats_device(int seg, int bus, int devfn)
-{
-    struct pci_ats_dev *pdev = NULL;
-    u32 value;
-    int pos;
-
-    pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
-    BUG_ON(!pos);
-
-    if ( iommu_verbose )
-        dprintk(XENLOG_INFO VTDPREFIX,
-                "%04x:%02x:%02x.%u: ATS capability found\n",
-                seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
-
-    value = pci_conf_read16(seg, bus, PCI_SLOT(devfn),
-                            PCI_FUNC(devfn), pos + ATS_REG_CTL);
-    if ( value & ATS_ENABLE )
-    {
-        list_for_each_entry ( pdev, &ats_devices, list )
-        {
-            if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
-            {
-                pos = 0;
-                break;
-            }
-        }
-    }
-    if ( pos )
-        pdev = xmalloc(struct pci_ats_dev);
-    if ( !pdev )
-        return -ENOMEM;
-
-    if ( !(value & ATS_ENABLE) )
-    {
-        value |= ATS_ENABLE;
-        pci_conf_write16(seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
-                         pos + ATS_REG_CTL, value);
-    }
-
-    if ( pos )
-    {
-        pdev->seg = seg;
-        pdev->bus = bus;
-        pdev->devfn = devfn;
-        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;
-        list_add(&pdev->list, &ats_devices);
-    }
-
-    if ( iommu_verbose )
-        dprintk(XENLOG_INFO VTDPREFIX,
-                "%04x:%02x:%02x.%u: ATS %s enabled\n",
-                seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
-                pos ? "is" : "was");
-
-    return pos;
-}
-
-void disable_ats_device(int seg, int bus, int devfn)
-{
-    struct pci_ats_dev *pdev;
-    u32 value;
-    int pos;
-
-    pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
-    BUG_ON(!pos);
-
-    value = pci_conf_read16(seg, bus, PCI_SLOT(devfn),
-                            PCI_FUNC(devfn), pos + ATS_REG_CTL);
-    value &= ~ATS_ENABLE;
-    pci_conf_write16(seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
-                     pos + ATS_REG_CTL, value);
-
-    list_for_each_entry ( pdev, &ats_devices, list )
-    {
-        if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
-        {
-            list_del(&pdev->list);
-            xfree(pdev);
-            break;
-        }
-    }
-
-    if ( iommu_verbose )
-        dprintk(XENLOG_INFO VTDPREFIX,
-                "%04x:%02x:%02x.%u: ATS is disabled\n",
-                seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
-}
-
-
 static int device_in_domain(struct iommu *iommu, struct pci_ats_dev *pdev, 
 u16 did)
 {

     struct root_entry *root_entry = NULL;

diff -r 119bccb1cc5e -r d422e3cf7976 xen/drivers/passthrough/x86/Makefile

xen/drivers/passthrough/x86/Makefile line 1

Quoted 1 line

xen/drivers/passthrough/x86/Makefile new file +1 -0
@@ -0,0 +1,1 @@
+obj-y += ats.o

obj-$(CONFIG_X86_64) += ats.o

The original file got built for 64-bit only, too.

Quoted 1 line

\ No newline at end of file

This is self-explanatory, isn't it?

Quoted 59 lines

diff -r 119bccb1cc5e -r d422e3cf7976 xen/drivers/passthrough/x86/ats.c

xen/drivers/passthrough/x86/ats.c new file +8 -0

@@ -0,0 +1,139 @@
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License

for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ */

xen/drivers/passthrough/x86/ats.c new file +38 -0

+
+#include <xen/sched.h>
+#include <xen/pci.h>
+#include <xen/pci_regs.h>
+#include "../ats.h"
+
+LIST_HEAD(ats_devices);
+
+static void parse_ats_param(char *s);
+custom_param("ats", parse_ats_param);
+
+bool_t __read_mostly ats_enabled = 1;
+
+static void __init parse_ats_param(char *s)
+{
+    char *ss;
+
+    do {
+        ss = strchr(s, ',');
+        if ( ss )
+            *ss = '\0';
+
+        switch ( parse_bool(s) )
+        {
+        case 0:
+            ats_enabled = 0;
+            break;
+        case 1:
+            ats_enabled = 1;
+            break;
+        }
+
+        s = ss + 1;
+    } while ( ss );
+}
+
+int enable_ats_device(int seg, int bus, int devfn)
+{

xen/drivers/passthrough/x86/ats.c line 62

Quoted 10 lines

xen/drivers/passthrough/x86/ats.c new file +10 -0

+    struct pci_ats_dev *pdev = NULL;
+    u32 value;
+    int pos;
+
+    pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
+    BUG_ON(!pos);
+
+    if ( iommu_verbose )
+        dprintk(XENLOG_INFO,
+                "%04x:%02x:%02x.%u: ATS capability found\n",

Here and below, the two lines can be folded (together they don't exceed 80 chars per line) now that the VT-d prefix is gone.

Jan

Quoted 77 lines

xen/drivers/passthrough/x86/ats.c new file +77 -0

+                seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
+
+    value = pci_conf_read16(seg, bus, PCI_SLOT(devfn),
+                            PCI_FUNC(devfn), pos + ATS_REG_CTL);
+    if ( value & ATS_ENABLE )
+    {
+        list_for_each_entry ( pdev, &ats_devices, list )
+        {
+            if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
+            {
+                pos = 0;
+                break;
+            }
+        }
+    }
+    if ( pos )
+        pdev = xmalloc(struct pci_ats_dev);
+    if ( !pdev )
+        return -ENOMEM;
+
+    if ( !(value & ATS_ENABLE) )
+    {
+        value |= ATS_ENABLE;
+        pci_conf_write16(seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
+                         pos + ATS_REG_CTL, value);
+    }
+
+    if ( pos )
+    {
+        pdev->seg = seg;
+        pdev->bus = bus;
+        pdev->devfn = devfn;
+        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;
+        list_add(&pdev->list, &ats_devices);
+    }
+
+    if ( iommu_verbose )
+        dprintk(XENLOG_INFO,
+                "%04x:%02x:%02x.%u: ATS %s enabled\n",
+                seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
+                pos ? "is" : "was");
+
+    return pos;
+}
+
+void disable_ats_device(int seg, int bus, int devfn)
+{
+    struct pci_ats_dev *pdev;
+    u32 value;
+    int pos;
+
+    pos = pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
+    BUG_ON(!pos);
+
+    value = pci_conf_read16(seg, bus, PCI_SLOT(devfn),
+                            PCI_FUNC(devfn), pos + ATS_REG_CTL);
+    value &= ~ATS_ENABLE;
+    pci_conf_write16(seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
+                     pos + ATS_REG_CTL, value);
+
+    list_for_each_entry ( pdev, &ats_devices, list )
+    {
+        if ( pdev->seg == seg && pdev->bus == bus && pdev->devfn == devfn )
+        {
+            list_del(&pdev->list);
+            xfree(pdev);
+            break;
+        }
+    }
+
+    if ( iommu_verbose )
+        dprintk(XENLOG_INFO,
+                "%04x:%02x:%02x.%u: ATS is disabled\n",
+                seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn));
+}

Jan Beulich

2011-11-03 12:49 UTC #4 lore raw

Re: [PATCH 2 of 5] amd iommu: Fix iommu page size encoding when page order > 0

In reply to Wei Wang

Quoted 31 lines

Quoted 1 line
Quoted 1 line · Wei Wang
# HG changeset patch
# User Wei Wang <[email protected]>
# Date 1320314617 -3600
# Node ID d0c38cb215cd96e01de27eadf5ec0a5e711de448 # Parent d422e3cf7976c76c57fc2d455b784d0fcc24d06b amd iommu: Fix iommu page size encoding when page order > 0
Signed-off-by Wei Wang <[email protected]>
diff -r d422e3cf7976 -r d0c38cb215cd xen/drivers/passthrough/amd/iommu_map.c
xen/drivers/passthrough/amd/iommu_map.c +9 -1
@@ -77,23 +77,24 @@ static void invalidate_iommu_pages(struc
 {
     u64 addr_lo, addr_hi;
     u32 cmd[4], entry;
-    u64 mask = 0;
     int sflag = 0, pde = 0;
 
+    ASSERT ( order == 0 || order == 9 || order == 18 );
+
+    /* All pages associated with the domainID are invalidated */
+    if ( order || (io_addr == INV_IOMMU_ALL_PAGES_ADDRESS ) )
+    {
+        sflag = 1;
+        pde = 1;
+    }
+
     /* If sflag == 1, the size of the invalidate command is determined
      by the first zero bit in the address starting from Address[12] */

xen/drivers/passthrough/amd/iommu_map.c static void invalidate_iommu_pages(struc · line 93

Quoted 10 lines

xen/drivers/passthrough/amd/iommu_map.c +1 -8

-    if ( order == 9 || order == 18 )
+    if ( order )
     {
-        mask = ((1ULL << (order - 1)) - 1) << PAGE_SHIFT;
-        io_addr |= mask;
-        sflag = 1;
-    }
-
-    /* All pages associated with the domainID are invalidated */
-    else if ( io_addr == 0x7FFFFFFFFFFFF000ULL )

Note the difference between this and your adjusted definition of INV_IOMMU_ALL_PAGES_ADDRESS. I don't think this is correct (or else your patch description should say why).

Quoted 10 lines

xen/drivers/passthrough/amd/iommu_map.c +3 -3

-    {
-        sflag = 1;
-        pde = 1;
+        u64 mask = 1ULL << (order - 1 + PAGE_SHIFT);
+        io_addr &= ~mask;
+        io_addr |= mask - 1;
     }
 
     addr_lo = io_addr & DMA_32BIT_MASK;

xen/drivers/passthrough/amd/iommu_map.c static void _amd_iommu_flush_pages(struc · line 921

Quoted 5 lines

xen/drivers/passthrough/amd/iommu_map.c +1 -1

@@ -917,7 +918,7 @@ static void _amd_iommu_flush_pages(struc
 
 void amd_iommu_flush_all_pages(struct domain *d)
 {
-    _amd_iommu_flush_pages(d, 0x7FFFFFFFFFFFFULL, 0);
+    _amd_iommu_flush_pages(d, INV_IOMMU_ALL_PAGES_ADDRESS, 0);

Same here, for a slightly different reason.

Jan

Quoted 14 lines

xen/drivers/passthrough/amd/iommu_map.c +0 -0
 }
 
 void amd_iommu_flush_pages(struct domain *d,
diff -r d422e3cf7976 -r d0c38cb215cd xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +3 -0
+0100
@@ -407,4 +407,6 @@
 #define INT_REMAP_ENTRY_VECTOR_MASK     0x00FF0000
 #define INT_REMAP_ENTRY_VECTOR_SHIFT    16
 
+#define INV_IOMMU_ALL_PAGES_ADDRESS      0x7FFFFFFFFFFFFFFFULL
+
 #endif /* _ASM_X86_64_AMD_IOMMU_DEFS_H */

Jan Beulich

2011-11-03 12:52 UTC #5 lore raw

Re: [PATCH 2 of 5] amd iommu: Fix iommu page size encoding when page order > 0

Quoted 78 lines

Quoted 32 lines
Quoted 2 lines · Jan Beulich
Quoted 1 line · Wei Wang
# HG changeset patch
# User Wei Wang <[email protected]>
# Date 1320314617 -3600
# Node ID d0c38cb215cd96e01de27eadf5ec0a5e711de448 # Parent d422e3cf7976c76c57fc2d455b784d0fcc24d06b amd iommu: Fix iommu page size encoding when page order > 0
Signed-off-by Wei Wang <[email protected]>
diff -r d422e3cf7976 -r d0c38cb215cd xen/drivers/passthrough/amd/iommu_map.c
xen/drivers/passthrough/amd/iommu_map.c +9 -1
@@ -77,23 +77,24 @@ static void invalidate_iommu_pages(struc
 {
     u64 addr_lo, addr_hi;
     u32 cmd[4], entry;
-    u64 mask = 0;
     int sflag = 0, pde = 0;
 
+    ASSERT ( order == 0 || order == 9 || order == 18 );
+
+    /* All pages associated with the domainID are invalidated */
+    if ( order || (io_addr == INV_IOMMU_ALL_PAGES_ADDRESS ) )
+    {
+        sflag = 1;
+        pde = 1;
+    }
+
     /* If sflag == 1, the size of the invalidate command is determined
      by the first zero bit in the address starting from Address[12] */
xen/drivers/passthrough/amd/iommu_map.c static void invalidate_iommu_pages(struc · line 93
Quoted 10 lines
xen/drivers/passthrough/amd/iommu_map.c +1 -8
-    if ( order == 9 || order == 18 )
+    if ( order )
     {
-        mask = ((1ULL << (order - 1)) - 1) << PAGE_SHIFT;
-        io_addr |= mask;
-        sflag = 1;
-    }
-
-    /* All pages associated with the domainID are invalidated */
-    else if ( io_addr == 0x7FFFFFFFFFFFF000ULL )
Note the difference between this and your adjusted definition of INV_IOMMU_ALL_PAGES_ADDRESS. I don't think this is correct (or else your patch description should say why).
Quoted 10 lines
xen/drivers/passthrough/amd/iommu_map.c +3 -3
-    {
-        sflag = 1;
-        pde = 1;
+        u64 mask = 1ULL << (order - 1 + PAGE_SHIFT);
+        io_addr &= ~mask;
+        io_addr |= mask - 1;
     }
 
     addr_lo = io_addr & DMA_32BIT_MASK;
xen/drivers/passthrough/amd/iommu_map.c static void _amd_iommu_flush_pages(struc · line 921
Quoted 5 lines
xen/drivers/passthrough/amd/iommu_map.c +1 -1
@@ -917,7 +918,7 @@ static void _amd_iommu_flush_pages(struc
 
 void amd_iommu_flush_all_pages(struct domain *d)
 {
-    _amd_iommu_flush_pages(d, 0x7FFFFFFFFFFFFULL, 0);
+    _amd_iommu_flush_pages(d, INV_IOMMU_ALL_PAGES_ADDRESS, 0);
Same here, for a slightly different reason.
Jan
Quoted 11 lines
xen/drivers/passthrough/amd/iommu_map.c +0 -0
 }
 
 void amd_iommu_flush_pages(struct domain *d,
diff -r d422e3cf7976 -r d0c38cb215cd xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +1 -0
+0100
@@ -407,4 +407,6 @@
 #define INT_REMAP_ENTRY_VECTOR_MASK     0x00FF0000
 #define INT_REMAP_ENTRY_VECTOR_SHIFT    16
 

Quoted 2 lines

xen/include/asm-x86/hvm/svm/amd-iommu-defs.h line 411
Quoted 2 lines
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +2 -0
+#define INV_IOMMU_ALL_PAGES_ADDRESS      0x7FFFFFFFFFFFFFFFULL
+

Oh, additionally, this still isn't being expressed by a shift expression, which makes it still badly readable (one has to count F-s in order to know what this actually represents).

Jan

Quoted 9 lines

xen/include/asm-x86/hvm/svm/amd-iommu-defs.h line 412
Quoted 1 line
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +0 -0
 #endif /* _ASM_X86_64_AMD_IOMMU_DEFS_H */
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Wei Wang2

author

2011-11-03 13:26 UTC #8 lore raw

Re: [PATCH 2 of 5] amd iommu: Fix iommu page size encoding when page order > 0

In reply to Jan Beulich

Quoted 59 lines · On Thursday 03 November 2011 13:49:39 Jan Beulich wrote:

Quoted 33 lines
Quoted 1 line
Quoted 1 line · Wei Wang
# HG changeset patch
# User Wei Wang <[email protected]>
# Date 1320314617 -3600
# Node ID d0c38cb215cd96e01de27eadf5ec0a5e711de448 # Parent d422e3cf7976c76c57fc2d455b784d0fcc24d06b amd iommu: Fix iommu page size encoding when page order > 0
Signed-off-by Wei Wang <[email protected]>
diff -r d422e3cf7976 -r d0c38cb215cd xen/drivers/passthrough/amd/iommu_map.c ---
a/xen/drivers/passthrough/amd/iommu_map.c Thu Nov 03 11:02:17 2011 +0100
+++ b/xen/drivers/passthrough/amd/iommu_map.c Thu Nov 03 11:03:37 2011
+0100 @@ -77,23 +77,24 @@ static void invalidate_iommu_pages(struc
 {
     u64 addr_lo, addr_hi;
     u32 cmd[4], entry;
xen/drivers/passthrough/amd/iommu_map.c +9 -1
-    u64 mask = 0;
     int sflag = 0, pde = 0;
 
+    ASSERT ( order == 0 || order == 9 || order == 18 );
+
+    /* All pages associated with the domainID are invalidated */
+    if ( order || (io_addr == INV_IOMMU_ALL_PAGES_ADDRESS ) )
+    {
+        sflag = 1;
+        pde = 1;
+    }
+
     /* If sflag == 1, the size of the invalidate command is determined
      by the first zero bit in the address starting from Address[12] */
xen/drivers/passthrough/amd/iommu_map.c static void invalidate_iommu_pages(struc · line 93
Quoted 10 lines
xen/drivers/passthrough/amd/iommu_map.c +1 -8
-    if ( order == 9 || order == 18 )
+    if ( order )
     {
-        mask = ((1ULL << (order - 1)) - 1) << PAGE_SHIFT;
-        io_addr |= mask;
-        sflag = 1;
-    }
-
-    /* All pages associated with the domainID are invalidated */
-    else if ( io_addr == 0x7FFFFFFFFFFFF000ULL )
Note the difference between this and your adjusted definition of INV_IOMMU_ALL_PAGES_ADDRESS. I don't think this is correct (or else your patch description should say why).
Quoted 11 lines
xen/drivers/passthrough/amd/iommu_map.c +3 -3
-    {
-        sflag = 1;
-        pde = 1;
+        u64 mask = 1ULL << (order - 1 + PAGE_SHIFT);
+        io_addr &= ~mask;
+        io_addr |= mask - 1;
     }
 
     addr_lo = io_addr & DMA_32BIT_MASK;
@@ -917,7 +918,7 @@ static void _amd_iommu_flush_pages(struc
 

Quoted 6 lines

xen/drivers/passthrough/amd/iommu_map.c static void _amd_iommu_flush_pages(struc · line 921
Quoted 4 lines
xen/drivers/passthrough/amd/iommu_map.c +1 -1
 void amd_iommu_flush_all_pages(struct domain *d)
 {
-    _amd_iommu_flush_pages(d, 0x7FFFFFFFFFFFFULL, 0);
+    _amd_iommu_flush_pages(d, INV_IOMMU_ALL_PAGES_ADDRESS, 0);
Same here, for a slightly different reason.

The old code is wrong, here should be 0x7FFFFFFFFFFFF000ULL (or 0x7FFFFFFFFFFFFULL << PAGE_SHIFT)
iommu does not care about the last 12 bits. So in this patch I just use INV_IOMMU_ALL_PAGES_ADDRESS = 0x7FFFFFFFFFFFFFFFULL

Wei

Quoted 17 lines

Jan
Quoted 15 lines
xen/drivers/passthrough/amd/iommu_map.c +0 -0
 }
 
 void amd_iommu_flush_pages(struct domain *d,
diff -r d422e3cf7976 -r d0c38cb215cd
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h ---
a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Thu Nov 03 11:02:17 2011
+0100 +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Thu Nov 03
11:03:37 2011 +0100
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +2 -0
@@ -407,4 +407,6 @@
 #define INT_REMAP_ENTRY_VECTOR_MASK     0x00FF0000
 #define INT_REMAP_ENTRY_VECTOR_SHIFT    16
 
+#define INV_IOMMU_ALL_PAGES_ADDRESS      0x7FFFFFFFFFFFFFFFULL
+
 #endif /* _ASM_X86_64_AMD_IOMMU_DEFS_H */

Jan Beulich

2011-11-03 13:28 UTC #9 lore raw

Re: [PATCH 2 of 5] amd iommu: Fix iommu page size encoding when page order> 0

Quoted 68 lines

Quoted 1 line
Quoted 1 line · On 03.11.11 at 14:26, Wei Wang2 <[email protected]> wrote:
Quoted 59 lines · On Thursday 03 November 2011 13:49:39 Jan Beulich wrote:
Quoted 33 lines
Quoted 1 line
Quoted 1 line · Wei Wang
# HG changeset patch
# User Wei Wang <[email protected]>
# Date 1320314617 -3600
# Node ID d0c38cb215cd96e01de27eadf5ec0a5e711de448 # Parent d422e3cf7976c76c57fc2d455b784d0fcc24d06b amd iommu: Fix iommu page size encoding when page order > 0
Signed-off-by Wei Wang <[email protected]>
diff -r d422e3cf7976 -r d0c38cb215cd xen/drivers/passthrough/amd/iommu_map.c ---
a/xen/drivers/passthrough/amd/iommu_map.c Thu Nov 03 11:02:17 2011 +0100
+++ b/xen/drivers/passthrough/amd/iommu_map.c Thu Nov 03 11:03:37 2011
+0100 @@ -77,23 +77,24 @@ static void invalidate_iommu_pages(struc
 {
     u64 addr_lo, addr_hi;
     u32 cmd[4], entry;
xen/drivers/passthrough/amd/iommu_map.c +9 -1
-    u64 mask = 0;
     int sflag = 0, pde = 0;
 
+    ASSERT ( order == 0 || order == 9 || order == 18 );
+
+    /* All pages associated with the domainID are invalidated */
+    if ( order || (io_addr == INV_IOMMU_ALL_PAGES_ADDRESS ) )
+    {
+        sflag = 1;
+        pde = 1;
+    }
+
     /* If sflag == 1, the size of the invalidate command is determined
      by the first zero bit in the address starting from Address[12] */
xen/drivers/passthrough/amd/iommu_map.c static void invalidate_iommu_pages(struc · line 93
Quoted 10 lines
xen/drivers/passthrough/amd/iommu_map.c +1 -8
-    if ( order == 9 || order == 18 )
+    if ( order )
     {
-        mask = ((1ULL << (order - 1)) - 1) << PAGE_SHIFT;
-        io_addr |= mask;
-        sflag = 1;
-    }
-
-    /* All pages associated with the domainID are invalidated */
-    else if ( io_addr == 0x7FFFFFFFFFFFF000ULL )
Note the difference between this and your adjusted definition of INV_IOMMU_ALL_PAGES_ADDRESS. I don't think this is correct (or else your patch description should say why).
Quoted 11 lines
xen/drivers/passthrough/amd/iommu_map.c +3 -3
-    {
-        sflag = 1;
-        pde = 1;
+        u64 mask = 1ULL << (order - 1 + PAGE_SHIFT);
+        io_addr &= ~mask;
+        io_addr |= mask - 1;
     }
 
     addr_lo = io_addr & DMA_32BIT_MASK;
@@ -917,7 +918,7 @@ static void _amd_iommu_flush_pages(struc
 
Quoted 6 lines
xen/drivers/passthrough/amd/iommu_map.c static void _amd_iommu_flush_pages(struc · line 921
Quoted 4 lines
xen/drivers/passthrough/amd/iommu_map.c +1 -1
 void amd_iommu_flush_all_pages(struct domain *d)
 {
-    _amd_iommu_flush_pages(d, 0x7FFFFFFFFFFFFULL, 0);
+    _amd_iommu_flush_pages(d, INV_IOMMU_ALL_PAGES_ADDRESS, 0);
Same here, for a slightly different reason.

Quoted 4 lines

The old code is wrong, here should be 0x7FFFFFFFFFFFF000ULL (or 0x7FFFFFFFFFFFFULL << PAGE_SHIFT)
iommu does not care about the last 12 bits. So in this patch I just use INV_IOMMU_ALL_PAGES_ADDRESS = 0x7FFFFFFFFFFFFFFFULL

Okay, in this case please state so in the patch description.

Jan

Wei Wang2

author

2011-11-03 13:29 UTC #6 lore raw

Re: Re: [PATCH 2 of 5] amd iommu: Fix iommu page size encoding when page order > 0

In reply to Jan Beulich

Quoted 81 lines · On Thursday 03 November 2011 13:52:18 Jan Beulich wrote:

Quoted 81 lines
Quoted 34 lines
Quoted 2 lines · Jan Beulich
Quoted 1 line · Wei Wang
# HG changeset patch
# User Wei Wang <[email protected]>
# Date 1320314617 -3600
# Node ID d0c38cb215cd96e01de27eadf5ec0a5e711de448 # Parent d422e3cf7976c76c57fc2d455b784d0fcc24d06b amd iommu: Fix iommu page size encoding when page order > 0
Signed-off-by Wei Wang <[email protected]>
diff -r d422e3cf7976 -r d0c38cb215cd xen/drivers/passthrough/amd/iommu_map.c ---
a/xen/drivers/passthrough/amd/iommu_map.c Thu Nov 03 11:02:17 2011 +0100
+++ b/xen/drivers/passthrough/amd/iommu_map.c Thu Nov 03 11:03:37 2011
+0100 @@ -77,23 +77,24 @@ static void invalidate_iommu_pages(struc
 {
     u64 addr_lo, addr_hi;
     u32 cmd[4], entry;
xen/drivers/passthrough/amd/iommu_map.c +9 -1
-    u64 mask = 0;
     int sflag = 0, pde = 0;
 
+    ASSERT ( order == 0 || order == 9 || order == 18 );
+
+    /* All pages associated with the domainID are invalidated */
+    if ( order || (io_addr == INV_IOMMU_ALL_PAGES_ADDRESS ) )
+    {
+        sflag = 1;
+        pde = 1;
+    }
+
     /* If sflag == 1, the size of the invalidate command is determined
      by the first zero bit in the address starting from Address[12] */
xen/drivers/passthrough/amd/iommu_map.c static void invalidate_iommu_pages(struc · line 93
Quoted 10 lines
xen/drivers/passthrough/amd/iommu_map.c +1 -8
-    if ( order == 9 || order == 18 )
+    if ( order )
     {
-        mask = ((1ULL << (order - 1)) - 1) << PAGE_SHIFT;
-        io_addr |= mask;
-        sflag = 1;
-    }
-
-    /* All pages associated with the domainID are invalidated */
-    else if ( io_addr == 0x7FFFFFFFFFFFF000ULL )
Note the difference between this and your adjusted definition of INV_IOMMU_ALL_PAGES_ADDRESS. I don't think this is correct (or else your patch description should say why).
Quoted 10 lines
xen/drivers/passthrough/amd/iommu_map.c +3 -3
-    {
-        sflag = 1;
-        pde = 1;
+        u64 mask = 1ULL << (order - 1 + PAGE_SHIFT);
+        io_addr &= ~mask;
+        io_addr |= mask - 1;
     }
 
     addr_lo = io_addr & DMA_32BIT_MASK;
xen/drivers/passthrough/amd/iommu_map.c static void _amd_iommu_flush_pages(struc · line 921
Quoted 5 lines
xen/drivers/passthrough/amd/iommu_map.c +1 -1
@@ -917,7 +918,7 @@ static void _amd_iommu_flush_pages(struc
 
 void amd_iommu_flush_all_pages(struct domain *d)
 {
-    _amd_iommu_flush_pages(d, 0x7FFFFFFFFFFFFULL, 0);
+    _amd_iommu_flush_pages(d, INV_IOMMU_ALL_PAGES_ADDRESS, 0);
Same here, for a slightly different reason.
Jan
Quoted 12 lines
xen/drivers/passthrough/amd/iommu_map.c +0 -0
 }
 
 void amd_iommu_flush_pages(struct domain *d,
diff -r d422e3cf7976 -r d0c38cb215cd
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h ---
a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Thu Nov 03 11:02:17 2011
+0100 +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Thu Nov 03
11:03:37 2011 +0100
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +0 -0
@@ -407,4 +407,6 @@
 #define INT_REMAP_ENTRY_VECTOR_MASK     0x00FF0000
 #define INT_REMAP_ENTRY_VECTOR_SHIFT    16
 

Quoted 6 lines

Quoted 2 lines
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h line 411
Quoted 2 lines
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +2 -0
+#define INV_IOMMU_ALL_PAGES_ADDRESS      0x7FFFFFFFFFFFFFFFULL
+
Oh, additionally, this still isn't being expressed by a shift expression, which makes it still badly readable (one has to count F-s in order to know what this actually represents).

Well, this is a copy from the spec.. How about using (1<<63) -1? Thanks,
Wei

Quoted 8 lines

Jan
Quoted 6 lines
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h line 412
Quoted 1 line
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +0 -0
 #endif /* _ASM_X86_64_AMD_IOMMU_DEFS_H */
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Jan Beulich

2011-11-03 13:35 UTC #7 lore raw

Re: Re: [PATCH 2 of 5] amd iommu: Fix iommu page size encoding when page order > 0

Quoted 13 lines

Quoted 1 line
Quoted 1 line · On 03.11.11 at 14:29, Wei Wang2 <[email protected]> wrote:
Quoted 11 lines · On Thursday 03 November 2011 13:52:18 Jan Beulich wrote:
Quoted 11 lines
Quoted 11 lines
Quoted 2 lines · Jan Beulich
Quoted 1 line · Wei Wang
diff -r d422e3cf7976 -r d0c38cb215cd
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h ---
a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Thu Nov 03 11:02:17 2011
+0100 +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h Thu Nov 03
11:03:37 2011 +0100
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +0 -0
@@ -407,4 +407,6 @@
 #define INT_REMAP_ENTRY_VECTOR_MASK     0x00FF0000
 #define INT_REMAP_ENTRY_VECTOR_SHIFT    16
 

Quoted 7 lines

Quoted 6 lines
Quoted 2 lines
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h line 411
Quoted 2 lines
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h +2 -0
+#define INV_IOMMU_ALL_PAGES_ADDRESS      0x7FFFFFFFFFFFFFFFULL
+
Oh, additionally, this still isn't being expressed by a shift expression, which makes it still badly readable (one has to count F-s in order to know what this actually represents).
Well, this is a copy from the spec.. How about using (1<<63) -1?

Yes, that's what I asked for. Ideally with 63 also given a meaningful #define (matchable to the spec).

Jan