* [PATCH 0/2] amd-iommu: use bitfields for PTE/PDE and DTE
@ 2019-02-04 11:19 Paul Durrant
2019-02-04 11:19 ` [PATCH 1/2] amd-iommu: use a bitfield for PTE/PDE Paul Durrant
2019-02-04 11:19 ` [PATCH 2/2] amd-iommu: use a bitfield for DTE Paul Durrant
0 siblings, 2 replies; 8+ messages in thread
From: Paul Durrant @ 2019-02-04 11:19 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant
Reduce code size by ~300 lines.
Paul Durrant (2):
amd-iommu: use a bitfield for PTE/PDE
amd-iommu: use a bitfield for DTE
xen/drivers/passthrough/amd/iommu_guest.c | 55 +--
xen/drivers/passthrough/amd/iommu_map.c | 338 ++++--------------
xen/drivers/passthrough/amd/pci_amd_iommu.c | 101 ++----
xen/include/asm-x86/amd-iommu.h | 5 -
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h | 167 ++++-----
xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 35 +-
6 files changed, 201 insertions(+), 500 deletions(-)
--
2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] amd-iommu: use a bitfield for PTE/PDE
2019-02-04 11:19 [PATCH 0/2] amd-iommu: use bitfields for PTE/PDE and DTE Paul Durrant
@ 2019-02-04 11:19 ` Paul Durrant
2019-02-12 20:14 ` Woods, Brian
2019-02-04 11:19 ` [PATCH 2/2] amd-iommu: use a bitfield for DTE Paul Durrant
1 sibling, 1 reply; 8+ messages in thread
From: Paul Durrant @ 2019-02-04 11:19 UTC (permalink / raw)
To: xen-devel
Cc: Wei Liu, Suravee Suthikulpanit, Andrew Cooper, Paul Durrant,
Jan Beulich, Brian Woods, Roger Pau Monné
The current use of get/set_field_from/in_reg_u32() is both inefficient and
requires some ugly casting.
This patch defines a new bitfield structure (amd_iommu_pte) and uses this
structure in all PTE/PDE manipulation, resulting in much more readable
and compact code.
NOTE: This commit also fixes one malformed comment in
set_iommu_pte_present().
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Brian Woods <brian.woods@amd.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
xen/drivers/passthrough/amd/iommu_map.c | 143 ++++--------------
xen/drivers/passthrough/amd/pci_amd_iommu.c | 50 +++---
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h | 47 ++----
xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 15 --
4 files changed, 64 insertions(+), 191 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index 67329b0c95..5fda6063df 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -38,100 +38,45 @@ static unsigned int pfn_to_pde_idx(unsigned long pfn, unsigned int level)
static unsigned int clear_iommu_pte_present(unsigned long l1_mfn,
unsigned long dfn)
{
- uint64_t *table, *pte;
+ struct amd_iommu_pte *table, *pte;
unsigned int flush_flags;
table = map_domain_page(_mfn(l1_mfn));
+ pte = &table[pfn_to_pde_idx(dfn, 1)];
- pte = (table + pfn_to_pde_idx(dfn, 1));
+ flush_flags = pte->pr ? IOMMU_FLUSHF_modified : 0;
+ memset(pte, 0, sizeof(*pte));
- flush_flags = get_field_from_reg_u32(*pte, IOMMU_PTE_PRESENT_MASK,
- IOMMU_PTE_PRESENT_SHIFT) ?
- IOMMU_FLUSHF_modified : 0;
-
- *pte = 0;
unmap_domain_page(table);
return flush_flags;
}
-static unsigned int set_iommu_pde_present(uint32_t *pde,
+static unsigned int set_iommu_pde_present(struct amd_iommu_pte *pte,
unsigned long next_mfn,
unsigned int next_level, bool iw,
bool ir)
{
- uint64_t maddr_next;
- uint32_t addr_lo, addr_hi, entry;
- bool old_present;
unsigned int flush_flags = IOMMU_FLUSHF_added;
- maddr_next = __pfn_to_paddr(next_mfn);
-
- old_present = get_field_from_reg_u32(pde[0], IOMMU_PTE_PRESENT_MASK,
- IOMMU_PTE_PRESENT_SHIFT);
- if ( old_present )
- {
- bool old_r, old_w;
- unsigned int old_level;
- uint64_t maddr_old;
-
- addr_hi = get_field_from_reg_u32(pde[1],
- IOMMU_PTE_ADDR_HIGH_MASK,
- IOMMU_PTE_ADDR_HIGH_SHIFT);
- addr_lo = get_field_from_reg_u32(pde[0],
- IOMMU_PTE_ADDR_LOW_MASK,
- IOMMU_PTE_ADDR_LOW_SHIFT);
- old_level = get_field_from_reg_u32(pde[0],
- IOMMU_PDE_NEXT_LEVEL_MASK,
- IOMMU_PDE_NEXT_LEVEL_SHIFT);
- old_w = get_field_from_reg_u32(pde[1],
- IOMMU_PTE_IO_WRITE_PERMISSION_MASK,
- IOMMU_PTE_IO_WRITE_PERMISSION_SHIFT);
- old_r = get_field_from_reg_u32(pde[1],
- IOMMU_PTE_IO_READ_PERMISSION_MASK,
- IOMMU_PTE_IO_READ_PERMISSION_SHIFT);
-
- maddr_old = ((uint64_t)addr_hi << 32) |
- ((uint64_t)addr_lo << PAGE_SHIFT);
-
- if ( maddr_old != maddr_next || iw != old_w || ir != old_r ||
- old_level != next_level )
+ if ( pte->pr &&
+ (pte->mfn != next_mfn ||
+ pte->iw != iw ||
+ pte->ir != ir ||
+ pte->next_level != next_level) )
flush_flags |= IOMMU_FLUSHF_modified;
- }
- addr_lo = maddr_next & DMA_32BIT_MASK;
- addr_hi = maddr_next >> 32;
-
- /* enable read/write permissions,which will be enforced at the PTE */
- set_field_in_reg_u32(addr_hi, 0,
- IOMMU_PDE_ADDR_HIGH_MASK,
- IOMMU_PDE_ADDR_HIGH_SHIFT, &entry);
- set_field_in_reg_u32(iw, entry,
- IOMMU_PDE_IO_WRITE_PERMISSION_MASK,
- IOMMU_PDE_IO_WRITE_PERMISSION_SHIFT, &entry);
- set_field_in_reg_u32(ir, entry,
- IOMMU_PDE_IO_READ_PERMISSION_MASK,
- IOMMU_PDE_IO_READ_PERMISSION_SHIFT, &entry);
-
- /* FC bit should be enabled in PTE, this helps to solve potential
+ /*
+ * FC bit should be enabled in PTE, this helps to solve potential
* issues with ATS devices
*/
- if ( next_level == 0 )
- set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, entry,
- IOMMU_PTE_FC_MASK, IOMMU_PTE_FC_SHIFT, &entry);
- pde[1] = entry;
+ pte->fc = !next_level;
- /* mark next level as 'present' */
- set_field_in_reg_u32(addr_lo >> PAGE_SHIFT, 0,
- IOMMU_PDE_ADDR_LOW_MASK,
- IOMMU_PDE_ADDR_LOW_SHIFT, &entry);
- set_field_in_reg_u32(next_level, entry,
- IOMMU_PDE_NEXT_LEVEL_MASK,
- IOMMU_PDE_NEXT_LEVEL_SHIFT, &entry);
- set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, entry,
- IOMMU_PDE_PRESENT_MASK,
- IOMMU_PDE_PRESENT_SHIFT, &entry);
- pde[0] = entry;
+ pte->mfn = next_mfn;
+ pte->iw = iw;
+ pte->ir = ir;
+ pte->next_level = next_level;
+ pte->pr = 1;
return flush_flags;
}
@@ -142,13 +87,11 @@ static unsigned int set_iommu_pte_present(unsigned long pt_mfn,
int pde_level,
bool iw, bool ir)
{
- uint64_t *table;
- uint32_t *pde;
+ struct amd_iommu_pte *table, *pde;
unsigned int flush_flags;
table = map_domain_page(_mfn(pt_mfn));
-
- pde = (uint32_t *)(table + pfn_to_pde_idx(dfn, pde_level));
+ pde = &table[pfn_to_pde_idx(dfn, pde_level)];
flush_flags = set_iommu_pde_present(pde, next_mfn, 0, iw, ir);
unmap_domain_page(table);
@@ -319,25 +262,6 @@ void iommu_dte_set_guest_cr3(uint32_t *dte, uint16_t dom_id, uint64_t gcr3,
dte[1] = entry;
}
-uint64_t amd_iommu_get_address_from_pte(void *pte)
-{
- uint32_t *entry = pte;
- uint32_t addr_lo, addr_hi;
- uint64_t ptr;
-
- addr_lo = get_field_from_reg_u32(entry[0],
- IOMMU_PTE_ADDR_LOW_MASK,
- IOMMU_PTE_ADDR_LOW_SHIFT);
-
- addr_hi = get_field_from_reg_u32(entry[1],
- IOMMU_PTE_ADDR_HIGH_MASK,
- IOMMU_PTE_ADDR_HIGH_SHIFT);
-
- ptr = ((uint64_t)addr_hi << 32) |
- ((uint64_t)addr_lo << PAGE_SHIFT);
- return ptr;
-}
-
/* Walk io page tables and build level page tables if necessary
* {Re, un}mapping super page frames causes re-allocation of io
* page tables.
@@ -345,7 +269,7 @@ uint64_t amd_iommu_get_address_from_pte(void *pte)
static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
unsigned long pt_mfn[])
{
- uint64_t *pde, *next_table_vaddr;
+ struct amd_iommu_pte *pde, *next_table_vaddr;
unsigned long next_table_mfn;
unsigned int level;
struct page_info *table;
@@ -370,15 +294,13 @@ static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
pt_mfn[level] = next_table_mfn;
next_table_vaddr = map_domain_page(_mfn(next_table_mfn));
- pde = next_table_vaddr + pfn_to_pde_idx(dfn, level);
+ pde = &next_table_vaddr[pfn_to_pde_idx(dfn, level)];
/* Here might be a super page frame */
- next_table_mfn = amd_iommu_get_address_from_pte(pde) >> PAGE_SHIFT;
+ next_table_mfn = pde->mfn;
/* Split super page frame into smaller pieces.*/
- if ( iommu_is_pte_present((uint32_t *)pde) &&
- (iommu_next_level((uint32_t *)pde) == 0) &&
- next_table_mfn != 0 )
+ if ( pde->pr && !pde->next_level && next_table_mfn )
{
int i;
unsigned long mfn, pfn;
@@ -398,13 +320,13 @@ static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
}
next_table_mfn = mfn_x(page_to_mfn(table));
- set_iommu_pde_present((uint32_t *)pde, next_table_mfn, next_level,
- !!IOMMUF_writable, !!IOMMUF_readable);
+ set_iommu_pde_present(pde, next_table_mfn, next_level, true,
+ true);
for ( i = 0; i < PTE_PER_TABLE_SIZE; i++ )
{
set_iommu_pte_present(next_table_mfn, pfn, mfn, next_level,
- !!IOMMUF_writable, !!IOMMUF_readable);
+ true, true);
mfn += page_sz;
pfn += page_sz;
}
@@ -413,7 +335,7 @@ static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
}
/* Install lower level page table for non-present entries */
- else if ( !iommu_is_pte_present((uint32_t *)pde) )
+ else if ( !pde->pr )
{
if ( next_table_mfn == 0 )
{
@@ -425,9 +347,8 @@ static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
return 1;
}
next_table_mfn = mfn_x(page_to_mfn(table));
- set_iommu_pde_present((uint32_t *)pde, next_table_mfn,
- next_level, !!IOMMUF_writable,
- !!IOMMUF_readable);
+ set_iommu_pde_present(pde, next_table_mfn, next_level, true,
+ true);
}
else /* should never reach here */
{
@@ -455,7 +376,7 @@ static int update_paging_mode(struct domain *d, unsigned long dfn)
struct amd_iommu *iommu = NULL;
struct page_info *new_root = NULL;
struct page_info *old_root = NULL;
- void *new_root_vaddr;
+ struct amd_iommu_pte *new_root_vaddr;
unsigned long old_root_mfn;
struct domain_iommu *hd = dom_iommu(d);
@@ -484,7 +405,7 @@ static int update_paging_mode(struct domain *d, unsigned long dfn)
new_root_vaddr = __map_domain_page(new_root);
old_root_mfn = mfn_x(page_to_mfn(old_root));
set_iommu_pde_present(new_root_vaddr, old_root_mfn, level,
- !!IOMMUF_writable, !!IOMMUF_readable);
+ true, true);
level++;
old_root = new_root;
offset >>= PTE_PER_TABLE_SHIFT;
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 33a3798f36..da6748320b 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -376,9 +376,8 @@ static void deallocate_next_page_table(struct page_info *pg, int level)
static void deallocate_page_table(struct page_info *pg)
{
- void *table_vaddr, *pde;
- u64 next_table_maddr;
- unsigned int index, level = PFN_ORDER(pg), next_level;
+ struct amd_iommu_pte *table_vaddr;
+ unsigned int index, level = PFN_ORDER(pg);
PFN_ORDER(pg) = 0;
@@ -392,17 +391,14 @@ static void deallocate_page_table(struct page_info *pg)
for ( index = 0; index < PTE_PER_TABLE_SIZE; index++ )
{
- pde = table_vaddr + (index * IOMMU_PAGE_TABLE_ENTRY_SIZE);
- next_table_maddr = amd_iommu_get_address_from_pte(pde);
- next_level = iommu_next_level(pde);
+ struct amd_iommu_pte *pde = &table_vaddr[index];
- if ( (next_table_maddr != 0) && (next_level != 0) &&
- iommu_is_pte_present(pde) )
+ if ( pde->mfn && pde->next_level && pde->pr )
{
/* We do not support skip levels yet */
- ASSERT(next_level == level - 1);
- deallocate_next_page_table(maddr_to_page(next_table_maddr),
- next_level);
+ ASSERT(pde->next_level == level - 1);
+ deallocate_next_page_table(mfn_to_page(_mfn(pde->mfn)),
+ pde->next_level);
}
}
@@ -500,10 +496,8 @@ static void amd_dump_p2m_table_level(struct page_info* pg, int level,
paddr_t gpa, int indent)
{
paddr_t address;
- void *table_vaddr, *pde;
- paddr_t next_table_maddr;
- int index, next_level, present;
- u32 *entry;
+ struct amd_iommu_pte *table_vaddr;
+ int index;
if ( level < 1 )
return;
@@ -518,42 +512,32 @@ static void amd_dump_p2m_table_level(struct page_info* pg, int level,
for ( index = 0; index < PTE_PER_TABLE_SIZE; index++ )
{
+ struct amd_iommu_pte *pde = &table_vaddr[index];
+
if ( !(index % 2) )
process_pending_softirqs();
- pde = table_vaddr + (index * IOMMU_PAGE_TABLE_ENTRY_SIZE);
- next_table_maddr = amd_iommu_get_address_from_pte(pde);
- entry = pde;
-
- present = get_field_from_reg_u32(entry[0],
- IOMMU_PDE_PRESENT_MASK,
- IOMMU_PDE_PRESENT_SHIFT);
-
- if ( !present )
+ if ( !pde->pr )
continue;
- next_level = get_field_from_reg_u32(entry[0],
- IOMMU_PDE_NEXT_LEVEL_MASK,
- IOMMU_PDE_NEXT_LEVEL_SHIFT);
-
- if ( next_level && (next_level != (level - 1)) )
+ if ( pde->next_level && (pde->next_level != (level - 1)) )
{
printk("IOMMU p2m table error. next_level = %d, expected %d\n",
- next_level, level - 1);
+ pde->next_level, level - 1);
continue;
}
address = gpa + amd_offset_level_address(index, level);
- if ( next_level >= 1 )
+ if ( pde->next_level >= 1 )
amd_dump_p2m_table_level(
- maddr_to_page(next_table_maddr), next_level,
+ mfn_to_page(_mfn(pde->mfn)), pde->next_level,
address, indent + 1);
else
printk("%*sdfn: %08lx mfn: %08lx\n",
indent, "",
(unsigned long)PFN_DOWN(address),
- (unsigned long)PFN_DOWN(next_table_maddr));
+ (unsigned long)PFN_DOWN(pfn_to_paddr(pde->mfn)));
}
unmap_domain_page(table_vaddr);
diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
index a217245249..a3a49f91eb 100644
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
@@ -413,38 +413,21 @@
#define IOMMU_PAGE_TABLE_U32_PER_ENTRY (IOMMU_PAGE_TABLE_ENTRY_SIZE / 4)
#define IOMMU_PAGE_TABLE_ALIGNMENT 4096
-#define IOMMU_PTE_PRESENT_MASK 0x00000001
-#define IOMMU_PTE_PRESENT_SHIFT 0
-#define IOMMU_PTE_NEXT_LEVEL_MASK 0x00000E00
-#define IOMMU_PTE_NEXT_LEVEL_SHIFT 9
-#define IOMMU_PTE_ADDR_LOW_MASK 0xFFFFF000
-#define IOMMU_PTE_ADDR_LOW_SHIFT 12
-#define IOMMU_PTE_ADDR_HIGH_MASK 0x000FFFFF
-#define IOMMU_PTE_ADDR_HIGH_SHIFT 0
-#define IOMMU_PTE_U_MASK 0x08000000
-#define IOMMU_PTE_U_SHIFT 7
-#define IOMMU_PTE_FC_MASK 0x10000000
-#define IOMMU_PTE_FC_SHIFT 28
-#define IOMMU_PTE_IO_READ_PERMISSION_MASK 0x20000000
-#define IOMMU_PTE_IO_READ_PERMISSION_SHIFT 29
-#define IOMMU_PTE_IO_WRITE_PERMISSION_MASK 0x40000000
-#define IOMMU_PTE_IO_WRITE_PERMISSION_SHIFT 30
-
-/* I/O Page Directory */
-#define IOMMU_PAGE_DIRECTORY_ENTRY_SIZE 8
-#define IOMMU_PAGE_DIRECTORY_ALIGNMENT 4096
-#define IOMMU_PDE_PRESENT_MASK 0x00000001
-#define IOMMU_PDE_PRESENT_SHIFT 0
-#define IOMMU_PDE_NEXT_LEVEL_MASK 0x00000E00
-#define IOMMU_PDE_NEXT_LEVEL_SHIFT 9
-#define IOMMU_PDE_ADDR_LOW_MASK 0xFFFFF000
-#define IOMMU_PDE_ADDR_LOW_SHIFT 12
-#define IOMMU_PDE_ADDR_HIGH_MASK 0x000FFFFF
-#define IOMMU_PDE_ADDR_HIGH_SHIFT 0
-#define IOMMU_PDE_IO_READ_PERMISSION_MASK 0x20000000
-#define IOMMU_PDE_IO_READ_PERMISSION_SHIFT 29
-#define IOMMU_PDE_IO_WRITE_PERMISSION_MASK 0x40000000
-#define IOMMU_PDE_IO_WRITE_PERMISSION_SHIFT 30
+struct amd_iommu_pte {
+ uint64_t pr:1;
+ uint64_t ignored0:4;
+ uint64_t a:1;
+ uint64_t d:1;
+ uint64_t ignored1:2;
+ uint64_t next_level:3;
+ uint64_t mfn:40;
+ uint64_t reserved:7;
+ uint64_t u:1;
+ uint64_t fc:1;
+ uint64_t ir:1;
+ uint64_t iw:1;
+ uint64_t ignored2:1;
+};
/* Paging modes */
#define IOMMU_PAGING_MODE_DISABLED 0x0
diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
index c5697565d6..1c1971bb7c 100644
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
@@ -57,7 +57,6 @@ int __must_check amd_iommu_map_page(struct domain *d, dfn_t dfn,
unsigned int *flush_flags);
int __must_check amd_iommu_unmap_page(struct domain *d, dfn_t dfn,
unsigned int *flush_flags);
-uint64_t amd_iommu_get_address_from_pte(void *entry);
int __must_check amd_iommu_alloc_root(struct domain_iommu *hd);
int amd_iommu_reserve_domain_unity_map(struct domain *domain,
paddr_t phys_addr, unsigned long size,
@@ -280,18 +279,4 @@ static inline void iommu_set_addr_hi_to_reg(uint32_t *reg, uint32_t addr)
IOMMU_REG_BASE_ADDR_HIGH_SHIFT, reg);
}
-static inline int iommu_is_pte_present(const u32 *entry)
-{
- return get_field_from_reg_u32(entry[0],
- IOMMU_PDE_PRESENT_MASK,
- IOMMU_PDE_PRESENT_SHIFT);
-}
-
-static inline unsigned int iommu_next_level(const u32 *entry)
-{
- return get_field_from_reg_u32(entry[0],
- IOMMU_PDE_NEXT_LEVEL_MASK,
- IOMMU_PDE_NEXT_LEVEL_SHIFT);
-}
-
#endif /* _ASM_X86_64_AMD_IOMMU_PROTO_H */
--
2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] amd-iommu: use a bitfield for DTE
2019-02-04 11:19 [PATCH 0/2] amd-iommu: use bitfields for PTE/PDE and DTE Paul Durrant
2019-02-04 11:19 ` [PATCH 1/2] amd-iommu: use a bitfield for PTE/PDE Paul Durrant
@ 2019-02-04 11:19 ` Paul Durrant
2019-02-12 20:30 ` Woods, Brian
1 sibling, 1 reply; 8+ messages in thread
From: Paul Durrant @ 2019-02-04 11:19 UTC (permalink / raw)
To: xen-devel
Cc: Wei Liu, Suravee Suthikulpanit, Andrew Cooper, Paul Durrant,
Jan Beulich, Brian Woods, Roger Pau Monné
The current use of get/set_field_from/in_reg_u32() is both inefficient and
requires some ugly casting.
This patch defines a new bitfield structure (amd_iommu_dte) and uses this
structure in all DTE manipulation, resulting in much more readable and
compact code.
NOTE: This patch also includes some clean-up of get_dma_requestor_id() to
change the types of the arguments from u16 to uint16_t.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Brian Woods <brian.woods@amd.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
---
xen/drivers/passthrough/amd/iommu_guest.c | 55 ++---
xen/drivers/passthrough/amd/iommu_map.c | 199 +++++-------------
xen/drivers/passthrough/amd/pci_amd_iommu.c | 51 ++---
xen/include/asm-x86/amd-iommu.h | 5 -
xen/include/asm-x86/hvm/svm/amd-iommu-defs.h | 120 +++++------
xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 20 +-
6 files changed, 139 insertions(+), 311 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
index 96175bb9ac..328e7509d5 100644
--- a/xen/drivers/passthrough/amd/iommu_guest.c
+++ b/xen/drivers/passthrough/amd/iommu_guest.c
@@ -76,39 +76,10 @@ static void guest_iommu_disable(struct guest_iommu *iommu)
iommu->enabled = 0;
}
-static uint64_t get_guest_cr3_from_dte(dev_entry_t *dte)
+static uint64_t get_guest_cr3_from_dte(struct amd_iommu_dte *dte)
{
- uint64_t gcr3_1, gcr3_2, gcr3_3;
-
- gcr3_1 = get_field_from_reg_u32(dte->data[1],
- IOMMU_DEV_TABLE_GCR3_1_MASK,
- IOMMU_DEV_TABLE_GCR3_1_SHIFT);
- gcr3_2 = get_field_from_reg_u32(dte->data[2],
- IOMMU_DEV_TABLE_GCR3_2_MASK,
- IOMMU_DEV_TABLE_GCR3_2_SHIFT);
- gcr3_3 = get_field_from_reg_u32(dte->data[3],
- IOMMU_DEV_TABLE_GCR3_3_MASK,
- IOMMU_DEV_TABLE_GCR3_3_SHIFT);
-
- return ((gcr3_3 << 31) | (gcr3_2 << 15 ) | (gcr3_1 << 12)) >> PAGE_SHIFT;
-}
-
-static uint16_t get_domid_from_dte(dev_entry_t *dte)
-{
- return get_field_from_reg_u32(dte->data[2], IOMMU_DEV_TABLE_DOMAIN_ID_MASK,
- IOMMU_DEV_TABLE_DOMAIN_ID_SHIFT);
-}
-
-static uint16_t get_glx_from_dte(dev_entry_t *dte)
-{
- return get_field_from_reg_u32(dte->data[1], IOMMU_DEV_TABLE_GLX_MASK,
- IOMMU_DEV_TABLE_GLX_SHIFT);
-}
-
-static uint16_t get_gv_from_dte(dev_entry_t *dte)
-{
- return get_field_from_reg_u32(dte->data[1],IOMMU_DEV_TABLE_GV_MASK,
- IOMMU_DEV_TABLE_GV_SHIFT);
+ return ((dte->gcr3_trp_51_31 << 31) | (dte->gcr3_trp_30_15 << 15) |
+ (dte->gcr3_trp_14_12 << 12)) >> PAGE_SHIFT;
}
static unsigned int host_domid(struct domain *d, uint64_t g_domid)
@@ -397,7 +368,7 @@ static int do_completion_wait(struct domain *d, cmd_entry_t *cmd)
static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
{
uint16_t gbdf, mbdf, req_id, gdom_id, hdom_id;
- dev_entry_t *gdte, *mdte, *dte_base;
+ struct amd_iommu_dte *gdte, *mdte, *dte_base;
struct amd_iommu *iommu = NULL;
struct guest_iommu *g_iommu;
uint64_t gcr3_gfn, gcr3_mfn;
@@ -414,23 +385,23 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
return 0;
/* Sometimes guest invalidates devices from non-exists dtes */
- if ( (gbdf * sizeof(dev_entry_t)) > g_iommu->dev_table.size )
+ if ( (gbdf * sizeof(struct amd_iommu_dte)) > g_iommu->dev_table.size )
return 0;
dte_mfn = guest_iommu_get_table_mfn(d,
reg_to_u64(g_iommu->dev_table.reg_base),
- sizeof(dev_entry_t), gbdf);
+ sizeof(struct amd_iommu_dte), gbdf);
ASSERT(mfn_valid(_mfn(dte_mfn)));
/* Read guest dte information */
dte_base = map_domain_page(_mfn(dte_mfn));
- gdte = dte_base + gbdf % (PAGE_SIZE / sizeof(dev_entry_t));
+ gdte = &dte_base[gbdf % (PAGE_SIZE / sizeof(struct amd_iommu_dte))];
- gdom_id = get_domid_from_dte(gdte);
+ gdom_id = gdte->domain_id;
gcr3_gfn = get_guest_cr3_from_dte(gdte);
- glx = get_glx_from_dte(gdte);
- gv = get_gv_from_dte(gdte);
+ glx = gdte->glx;
+ gv = gdte->gv;
unmap_domain_page(dte_base);
@@ -454,11 +425,11 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
/* Setup host device entry */
hdom_id = host_domid(d, gdom_id);
req_id = get_dma_requestor_id(iommu->seg, mbdf);
- mdte = iommu->dev_table.buffer + (req_id * sizeof(dev_entry_t));
+ dte_base = iommu->dev_table.buffer;
+ mdte = &dte_base[req_id];
spin_lock_irqsave(&iommu->lock, flags);
- iommu_dte_set_guest_cr3((u32 *)mdte, hdom_id,
- gcr3_mfn << PAGE_SHIFT, gv, glx);
+ iommu_dte_set_guest_cr3(mdte, hdom_id, gcr3_mfn, gv, glx);
amd_iommu_flush_device(iommu, req_id);
spin_unlock_irqrestore(&iommu->lock, flags);
diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index 5fda6063df..cbf00e9e72 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -99,167 +99,64 @@ static unsigned int set_iommu_pte_present(unsigned long pt_mfn,
return flush_flags;
}
-void amd_iommu_set_root_page_table(uint32_t *dte, uint64_t root_ptr,
- uint16_t domain_id, uint8_t paging_mode,
- uint8_t valid)
+void amd_iommu_set_root_page_table(struct amd_iommu_dte *dte,
+ uint64_t root_ptr, uint16_t domain_id,
+ uint8_t paging_mode, uint8_t valid)
{
- uint32_t addr_hi, addr_lo, entry;
- set_field_in_reg_u32(domain_id, 0,
- IOMMU_DEV_TABLE_DOMAIN_ID_MASK,
- IOMMU_DEV_TABLE_DOMAIN_ID_SHIFT, &entry);
- dte[2] = entry;
-
- addr_lo = root_ptr & DMA_32BIT_MASK;
- addr_hi = root_ptr >> 32;
-
- set_field_in_reg_u32(addr_hi, 0,
- IOMMU_DEV_TABLE_PAGE_TABLE_PTR_HIGH_MASK,
- IOMMU_DEV_TABLE_PAGE_TABLE_PTR_HIGH_SHIFT, &entry);
- set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, entry,
- IOMMU_DEV_TABLE_IO_WRITE_PERMISSION_MASK,
- IOMMU_DEV_TABLE_IO_WRITE_PERMISSION_SHIFT, &entry);
- set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, entry,
- IOMMU_DEV_TABLE_IO_READ_PERMISSION_MASK,
- IOMMU_DEV_TABLE_IO_READ_PERMISSION_SHIFT, &entry);
- dte[1] = entry;
-
- set_field_in_reg_u32(addr_lo >> PAGE_SHIFT, 0,
- IOMMU_DEV_TABLE_PAGE_TABLE_PTR_LOW_MASK,
- IOMMU_DEV_TABLE_PAGE_TABLE_PTR_LOW_SHIFT, &entry);
- set_field_in_reg_u32(paging_mode, entry,
- IOMMU_DEV_TABLE_PAGING_MODE_MASK,
- IOMMU_DEV_TABLE_PAGING_MODE_SHIFT, &entry);
- set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, entry,
- IOMMU_DEV_TABLE_TRANSLATION_VALID_MASK,
- IOMMU_DEV_TABLE_TRANSLATION_VALID_SHIFT, &entry);
- set_field_in_reg_u32(valid ? IOMMU_CONTROL_ENABLED :
- IOMMU_CONTROL_DISABLED, entry,
- IOMMU_DEV_TABLE_VALID_MASK,
- IOMMU_DEV_TABLE_VALID_SHIFT, &entry);
- dte[0] = entry;
-}
-
-void iommu_dte_set_iotlb(uint32_t *dte, uint8_t i)
-{
- uint32_t 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;
+ dte->domain_id = domain_id;
+ dte->pt_root = paddr_to_pfn(root_ptr);
+ dte->iw = 1;
+ dte->ir = 1;
+ dte->paging_mode = paging_mode;
+ dte->tv = 1;
+ dte->v = valid;
}
void __init amd_iommu_set_intremap_table(
- uint32_t *dte, uint64_t intremap_ptr, uint8_t int_valid)
+ struct amd_iommu_dte *dte, uint64_t intremap_ptr, uint8_t int_valid)
{
- uint32_t addr_hi, addr_lo, entry;
-
- addr_lo = intremap_ptr & DMA_32BIT_MASK;
- addr_hi = intremap_ptr >> 32;
-
- entry = dte[5];
- set_field_in_reg_u32(addr_hi, entry,
- IOMMU_DEV_TABLE_INT_TABLE_PTR_HIGH_MASK,
- IOMMU_DEV_TABLE_INT_TABLE_PTR_HIGH_SHIFT, &entry);
- /* Fixed and arbitrated interrupts remapepd */
- set_field_in_reg_u32(2, entry,
- IOMMU_DEV_TABLE_INT_CONTROL_MASK,
- IOMMU_DEV_TABLE_INT_CONTROL_SHIFT, &entry);
- dte[5] = entry;
-
- set_field_in_reg_u32(addr_lo >> 6, 0,
- IOMMU_DEV_TABLE_INT_TABLE_PTR_LOW_MASK,
- IOMMU_DEV_TABLE_INT_TABLE_PTR_LOW_SHIFT, &entry);
- /* 2048 entries */
- set_field_in_reg_u32(0xB, entry,
- IOMMU_DEV_TABLE_INT_TABLE_LENGTH_MASK,
- IOMMU_DEV_TABLE_INT_TABLE_LENGTH_SHIFT, &entry);
-
- /* unmapped interrupt results io page faults*/
- set_field_in_reg_u32(IOMMU_CONTROL_DISABLED, entry,
- IOMMU_DEV_TABLE_INT_TABLE_IGN_UNMAPPED_MASK,
- IOMMU_DEV_TABLE_INT_TABLE_IGN_UNMAPPED_SHIFT, &entry);
- set_field_in_reg_u32(int_valid ? IOMMU_CONTROL_ENABLED :
- IOMMU_CONTROL_DISABLED, entry,
- IOMMU_DEV_TABLE_INT_VALID_MASK,
- IOMMU_DEV_TABLE_INT_VALID_SHIFT, &entry);
- dte[4] = entry;
+ dte->it_root = intremap_ptr >> 6;
+ dte->int_tab_len = 0xb; /* 2048 entries */
+ dte->int_ctl = 2; /* fixed and arbitrated interrupts remapped */
+ dte->ig = 0; /* unmapped interrupt results io page faults */
+ dte->iv = int_valid;
}
-void __init iommu_dte_add_device_entry(uint32_t *dte,
+void __init iommu_dte_add_device_entry(struct amd_iommu_dte *dte,
struct ivrs_mappings *ivrs_dev)
{
- uint32_t entry;
- uint8_t sys_mgt, dev_ex, flags;
- uint8_t mask = ~(0x7 << 3);
-
- dte[7] = dte[6] = dte[4] = dte[2] = dte[1] = dte[0] = 0;
-
- flags = ivrs_dev->device_flags;
- sys_mgt = MASK_EXTR(flags, ACPI_IVHD_SYSTEM_MGMT);
- dev_ex = ivrs_dev->dte_allow_exclusion;
-
- flags &= mask;
- set_field_in_reg_u32(flags, 0,
- IOMMU_DEV_TABLE_IVHD_FLAGS_MASK,
- IOMMU_DEV_TABLE_IVHD_FLAGS_SHIFT, &entry);
- dte[5] = entry;
-
- set_field_in_reg_u32(sys_mgt, 0,
- IOMMU_DEV_TABLE_SYS_MGT_MSG_ENABLE_MASK,
- IOMMU_DEV_TABLE_SYS_MGT_MSG_ENABLE_SHIFT, &entry);
- set_field_in_reg_u32(dev_ex, entry,
- IOMMU_DEV_TABLE_ALLOW_EXCLUSION_MASK,
- IOMMU_DEV_TABLE_ALLOW_EXCLUSION_SHIFT, &entry);
- dte[3] = entry;
+ uint8_t flags = ivrs_dev->device_flags;
+
+ memset(dte, 0, sizeof(*dte));
+
+ dte->init_pass = MASK_EXTR(flags, ACPI_IVHD_INIT_PASS);
+ dte->ext_int_pass = MASK_EXTR(flags, ACPI_IVHD_EINT_PASS);
+ dte->nmi_pass = MASK_EXTR(flags, ACPI_IVHD_NMI_PASS);
+ dte->lint0_pass = MASK_EXTR(flags, ACPI_IVHD_LINT0_PASS);
+ dte->lint1_pass = MASK_EXTR(flags, ACPI_IVHD_LINT1_PASS);
+ dte->sys_mgt = MASK_EXTR(flags, ACPI_IVHD_SYSTEM_MGMT);
+ dte->ex = ivrs_dev->dte_allow_exclusion;
}
-void iommu_dte_set_guest_cr3(uint32_t *dte, uint16_t dom_id, uint64_t gcr3,
- int gv, unsigned int glx)
+void iommu_dte_set_guest_cr3(struct amd_iommu_dte *dte, uint16_t dom_id,
+ uint64_t gcr3_mfn, uint8_t gv, uint8_t glx)
{
- uint32_t entry, gcr3_1, gcr3_2, gcr3_3;
-
- gcr3_3 = gcr3 >> 31;
- gcr3_2 = (gcr3 >> 15) & 0xFFFF;
- gcr3_1 = (gcr3 >> PAGE_SHIFT) & 0x7;
+#define GCR3_MASK(hi, lo) (((1ul << ((hi) + 1)) - 1) & ~((1ul << (lo)) - 1))
+#define GCR3_SHIFT(lo) ((lo) - PAGE_SHIFT)
/* I bit must be set when gcr3 is enabled */
- entry = dte[3];
- set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, entry,
- IOMMU_DEV_TABLE_IOTLB_SUPPORT_MASK,
- IOMMU_DEV_TABLE_IOTLB_SUPPORT_SHIFT, &entry);
- /* update gcr3 */
- set_field_in_reg_u32(gcr3_3, entry,
- IOMMU_DEV_TABLE_GCR3_3_MASK,
- IOMMU_DEV_TABLE_GCR3_3_SHIFT, &entry);
- dte[3] = entry;
-
- set_field_in_reg_u32(dom_id, entry,
- IOMMU_DEV_TABLE_DOMAIN_ID_MASK,
- IOMMU_DEV_TABLE_DOMAIN_ID_SHIFT, &entry);
- /* update gcr3 */
- entry = dte[2];
- set_field_in_reg_u32(gcr3_2, entry,
- IOMMU_DEV_TABLE_GCR3_2_MASK,
- IOMMU_DEV_TABLE_GCR3_2_SHIFT, &entry);
- dte[2] = entry;
-
- entry = dte[1];
- /* Enable GV bit */
- set_field_in_reg_u32(!!gv, entry,
- IOMMU_DEV_TABLE_GV_MASK,
- IOMMU_DEV_TABLE_GV_SHIFT, &entry);
-
- /* 1 level guest cr3 table */
- set_field_in_reg_u32(glx, entry,
- IOMMU_DEV_TABLE_GLX_MASK,
- IOMMU_DEV_TABLE_GLX_SHIFT, &entry);
- /* update gcr3 */
- set_field_in_reg_u32(gcr3_1, entry,
- IOMMU_DEV_TABLE_GCR3_1_MASK,
- IOMMU_DEV_TABLE_GCR3_1_SHIFT, &entry);
- dte[1] = entry;
+ dte->i = 1;
+
+ dte->gcr3_trp_14_12 = (gcr3_mfn & GCR3_MASK(14, 12)) >> GCR3_SHIFT(12);
+ dte->gcr3_trp_30_15 = (gcr3_mfn & GCR3_MASK(30, 15)) >> GCR3_SHIFT(15);
+ dte->gcr3_trp_51_31 = (gcr3_mfn & GCR3_MASK(51, 31)) >> GCR3_SHIFT(31);
+
+ dte->domain_id = dom_id;
+ dte->glx = glx;
+ dte->gv = gv;
+
+#undef GCR3_SHIFT
+#undef GCR3_MASK
}
/* Walk io page tables and build level page tables if necessary
@@ -369,7 +266,7 @@ static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
static int update_paging_mode(struct domain *d, unsigned long dfn)
{
uint16_t bdf;
- void *device_entry;
+ struct amd_iommu_dte *table, *dte;
unsigned int req_id, level, offset;
unsigned long flags;
struct pci_dev *pdev;
@@ -438,11 +335,11 @@ static int update_paging_mode(struct domain *d, unsigned long dfn)
spin_lock_irqsave(&iommu->lock, flags);
do {
req_id = get_dma_requestor_id(pdev->seg, bdf);
- device_entry = iommu->dev_table.buffer +
- (req_id * IOMMU_DEV_TABLE_ENTRY_SIZE);
+ table = iommu->dev_table.buffer;
+ dte = &table[req_id];
/* valid = 0 only works for dom0 passthrough mode */
- amd_iommu_set_root_page_table((uint32_t *)device_entry,
+ amd_iommu_set_root_page_table(dte,
page_to_maddr(hd->arch.root_table),
d->domain_id,
hd->arch.paging_mode, 1);
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index da6748320b..f6c17ba87a 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -71,7 +71,7 @@ struct amd_iommu *find_iommu_for_device(int seg, int bdf)
* Return original device id, if device has valid interrupt remapping
* table setup for both select entry and alias entry.
*/
-int get_dma_requestor_id(u16 seg, u16 bdf)
+int get_dma_requestor_id(uint16_t seg, uint16_t bdf)
{
struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(seg);
int req_id;
@@ -85,35 +85,11 @@ int get_dma_requestor_id(u16 seg, u16 bdf)
return req_id;
}
-static int is_translation_valid(u32 *entry)
-{
- return (get_field_from_reg_u32(entry[0],
- IOMMU_DEV_TABLE_VALID_MASK,
- IOMMU_DEV_TABLE_VALID_SHIFT) &&
- get_field_from_reg_u32(entry[0],
- IOMMU_DEV_TABLE_TRANSLATION_VALID_MASK,
- IOMMU_DEV_TABLE_TRANSLATION_VALID_SHIFT));
-}
-
-static void disable_translation(u32 *dte)
-{
- u32 entry;
-
- entry = dte[0];
- set_field_in_reg_u32(IOMMU_CONTROL_DISABLED, entry,
- IOMMU_DEV_TABLE_TRANSLATION_VALID_MASK,
- IOMMU_DEV_TABLE_TRANSLATION_VALID_SHIFT, &entry);
- set_field_in_reg_u32(IOMMU_CONTROL_DISABLED, entry,
- IOMMU_DEV_TABLE_VALID_MASK,
- IOMMU_DEV_TABLE_VALID_SHIFT, &entry);
- dte[0] = entry;
-}
-
static void amd_iommu_setup_domain_device(
struct domain *domain, struct amd_iommu *iommu,
- u8 devfn, struct pci_dev *pdev)
+ uint8_t devfn, struct pci_dev *pdev)
{
- void *dte;
+ struct amd_iommu_dte *table, *dte;
unsigned long flags;
int req_id, valid = 1;
int dte_i = 0;
@@ -131,20 +107,21 @@ static void amd_iommu_setup_domain_device(
/* get device-table entry */
req_id = get_dma_requestor_id(iommu->seg, PCI_BDF2(bus, devfn));
- dte = iommu->dev_table.buffer + (req_id * IOMMU_DEV_TABLE_ENTRY_SIZE);
+ table = iommu->dev_table.buffer;
+ dte = &table[req_id];
spin_lock_irqsave(&iommu->lock, flags);
- if ( !is_translation_valid((u32 *)dte) )
+ if ( !dte->v || !dte->tv )
{
/* bind DTE to domain page-tables */
amd_iommu_set_root_page_table(
- (u32 *)dte, page_to_maddr(hd->arch.root_table), domain->domain_id,
+ dte, page_to_maddr(hd->arch.root_table), domain->domain_id,
hd->arch.paging_mode, valid);
if ( pci_ats_device(iommu->seg, bus, pdev->devfn) &&
iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) )
- iommu_dte_set_iotlb((u32 *)dte, dte_i);
+ dte->i = dte_i;
amd_iommu_flush_device(iommu, req_id);
@@ -272,23 +249,25 @@ void amd_iommu_disable_domain_device(struct domain *domain,
struct amd_iommu *iommu,
u8 devfn, struct pci_dev *pdev)
{
- void *dte;
+ struct amd_iommu_dte *table, *dte;
unsigned long flags;
int req_id;
u8 bus = pdev->bus;
BUG_ON ( iommu->dev_table.buffer == NULL );
req_id = get_dma_requestor_id(iommu->seg, PCI_BDF2(bus, devfn));
- dte = iommu->dev_table.buffer + (req_id * IOMMU_DEV_TABLE_ENTRY_SIZE);
+ table = iommu->dev_table.buffer;
+ dte = &table[req_id];
spin_lock_irqsave(&iommu->lock, flags);
- if ( is_translation_valid((u32 *)dte) )
+ if ( dte->tv && dte->v )
{
- disable_translation((u32 *)dte);
+ dte->tv = 0;
+ dte->v = 0;
if ( pci_ats_device(iommu->seg, bus, pdev->devfn) &&
iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) )
- iommu_dte_set_iotlb((u32 *)dte, 0);
+ dte->i = 0;
amd_iommu_flush_device(iommu, req_id);
diff --git a/xen/include/asm-x86/amd-iommu.h b/xen/include/asm-x86/amd-iommu.h
index 02715b482b..ad8e4a35a2 100644
--- a/xen/include/asm-x86/amd-iommu.h
+++ b/xen/include/asm-x86/amd-iommu.h
@@ -46,11 +46,6 @@ typedef struct cmd_entry
{
uint32_t data[4];
} cmd_entry_t;
-
-typedef struct dev_entry
-{
- uint32_t data[8];
-} dev_entry_t;
#pragma pack()
struct table_struct {
diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
index a3a49f91eb..40da33b271 100644
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
@@ -107,74 +107,58 @@
#define IOMMU_DEV_TABLE_INT_CONTROL_FORWARDED 0x1
#define IOMMU_DEV_TABLE_INT_CONTROL_TRANSLATED 0x2
-/* DeviceTable Entry[31:0] */
-#define IOMMU_DEV_TABLE_VALID_MASK 0x00000001
-#define IOMMU_DEV_TABLE_VALID_SHIFT 0
-#define IOMMU_DEV_TABLE_TRANSLATION_VALID_MASK 0x00000002
-#define IOMMU_DEV_TABLE_TRANSLATION_VALID_SHIFT 1
-#define IOMMU_DEV_TABLE_PAGING_MODE_MASK 0x00000E00
-#define IOMMU_DEV_TABLE_PAGING_MODE_SHIFT 9
-#define IOMMU_DEV_TABLE_PAGE_TABLE_PTR_LOW_MASK 0xFFFFF000
-#define IOMMU_DEV_TABLE_PAGE_TABLE_PTR_LOW_SHIFT 12
-
-/* DeviceTable Entry[63:32] */
-#define IOMMU_DEV_TABLE_GV_SHIFT 23
-#define IOMMU_DEV_TABLE_GV_MASK 0x800000
-#define IOMMU_DEV_TABLE_GLX_SHIFT 24
-#define IOMMU_DEV_TABLE_GLX_MASK 0x3000000
-#define IOMMU_DEV_TABLE_GCR3_1_SHIFT 26
-#define IOMMU_DEV_TABLE_GCR3_1_MASK 0x1c000000
-
-#define IOMMU_DEV_TABLE_PAGE_TABLE_PTR_HIGH_MASK 0x000FFFFF
-#define IOMMU_DEV_TABLE_PAGE_TABLE_PTR_HIGH_SHIFT 0
-#define IOMMU_DEV_TABLE_IO_READ_PERMISSION_MASK 0x20000000
-#define IOMMU_DEV_TABLE_IO_READ_PERMISSION_SHIFT 29
-#define IOMMU_DEV_TABLE_IO_WRITE_PERMISSION_MASK 0x40000000
-#define IOMMU_DEV_TABLE_IO_WRITE_PERMISSION_SHIFT 30
-
-/* DeviceTable Entry[95:64] */
-#define IOMMU_DEV_TABLE_DOMAIN_ID_MASK 0x0000FFFF
-#define IOMMU_DEV_TABLE_DOMAIN_ID_SHIFT 0
-#define IOMMU_DEV_TABLE_GCR3_2_SHIFT 16
-#define IOMMU_DEV_TABLE_GCR3_2_MASK 0xFFFF0000
-
-/* DeviceTable Entry[127:96] */
-#define IOMMU_DEV_TABLE_IOTLB_SUPPORT_MASK 0x00000001
-#define IOMMU_DEV_TABLE_IOTLB_SUPPORT_SHIFT 0
-#define IOMMU_DEV_TABLE_SUPRESS_LOGGED_PAGES_MASK 0x00000002
-#define IOMMU_DEV_TABLE_SUPRESS_LOGGED_PAGES_SHIFT 1
-#define IOMMU_DEV_TABLE_SUPRESS_ALL_PAGES_MASK 0x00000004
-#define IOMMU_DEV_TABLE_SUPRESS_ALL_PAGES_SHIFT 2
-#define IOMMU_DEV_TABLE_IO_CONTROL_MASK 0x00000018
-#define IOMMU_DEV_TABLE_IO_CONTROL_SHIFT 3
-#define IOMMU_DEV_TABLE_IOTLB_CACHE_HINT_MASK 0x00000020
-#define IOMMU_DEV_TABLE_IOTLB_CACHE_HINT_SHIFT 5
-#define IOMMU_DEV_TABLE_SNOOP_DISABLE_MASK 0x00000040
-#define IOMMU_DEV_TABLE_SNOOP_DISABLE_SHIFT 6
-#define IOMMU_DEV_TABLE_ALLOW_EXCLUSION_MASK 0x00000080
-#define IOMMU_DEV_TABLE_ALLOW_EXCLUSION_SHIFT 7
-#define IOMMU_DEV_TABLE_SYS_MGT_MSG_ENABLE_MASK 0x00000300
-#define IOMMU_DEV_TABLE_SYS_MGT_MSG_ENABLE_SHIFT 8
-
-/* DeviceTable Entry[159:128] */
-#define IOMMU_DEV_TABLE_INT_VALID_MASK 0x00000001
-#define IOMMU_DEV_TABLE_INT_VALID_SHIFT 0
-#define IOMMU_DEV_TABLE_INT_TABLE_LENGTH_MASK 0x0000001E
-#define IOMMU_DEV_TABLE_INT_TABLE_LENGTH_SHIFT 1
-#define IOMMU_DEV_TABLE_INT_TABLE_IGN_UNMAPPED_MASK 0x0000000020
-#define IOMMU_DEV_TABLE_INT_TABLE_IGN_UNMAPPED_SHIFT 5
-#define IOMMU_DEV_TABLE_INT_TABLE_PTR_LOW_MASK 0xFFFFFFC0
-#define IOMMU_DEV_TABLE_INT_TABLE_PTR_LOW_SHIFT 6
-#define IOMMU_DEV_TABLE_GCR3_3_SHIFT 11
-#define IOMMU_DEV_TABLE_GCR3_3_MASK 0xfffff800
-
-/* DeviceTable Entry[191:160] */
-#define IOMMU_DEV_TABLE_INT_TABLE_PTR_HIGH_MASK 0x000FFFFF
-#define IOMMU_DEV_TABLE_INT_TABLE_PTR_HIGH_SHIFT 0
-#define IOMMU_DEV_TABLE_IVHD_FLAGS_SHIFT 24
-#define IOMMU_DEV_TABLE_IVHD_FLAGS_MASK 0xC7000000
-#define IOMMU_DEV_TABLE_INT_CONTROL_MASK 0x30000000
-#define IOMMU_DEV_TABLE_INT_CONTROL_SHIFT 28
+struct amd_iommu_dte {
+ /* 0 - 63 */
+ uint64_t v:1;
+ uint64_t tv:1;
+ uint64_t reserved0:5;
+ uint64_t had:2;
+ uint64_t paging_mode:3;
+ uint64_t pt_root:40;
+ uint64_t ppr:1;
+ uint64_t gprp:1;
+ uint64_t giov:1;
+ uint64_t gv:1;
+ uint64_t glx:2;
+ uint64_t gcr3_trp_14_12:3;
+ uint64_t ir:1;
+ uint64_t iw:1;
+ uint64_t reserved1:1;
+
+ /* 64 - 127 */
+ uint64_t domain_id:16;
+ uint64_t gcr3_trp_30_15:16;
+ uint64_t i:1;
+ uint64_t se:1;
+ uint64_t sa:1;
+ uint64_t ioctl:2;
+ uint64_t cache:1;
+ uint64_t sd:1;
+ uint64_t ex:1;
+ uint64_t sys_mgt:2;
+ uint64_t reserved2:1;
+ uint64_t gcr3_trp_51_31:21;
+
+ /* 128 - 191 */
+ uint64_t iv:1;
+ uint64_t int_tab_len:4;
+ uint64_t ig:1;
+ uint64_t it_root:46;
+ uint64_t reserved3:4;
+ uint64_t init_pass:1;
+ uint64_t ext_int_pass:1;
+ uint64_t nmi_pass:1;
+ uint64_t reserved4:1;
+ uint64_t int_ctl:2;
+ uint64_t lint0_pass:1;
+ uint64_t lint1_pass:1;
+
+ /* 192 - 255 */
+ uint64_t reserved5:54;
+ uint64_t attr_v:1;
+ uint64_t mode0_fc:1;
+ uint64_t snoop_attr:8;
+};
/* Command Buffer */
#define IOMMU_CMD_BUFFER_BASE_LOW_OFFSET 0x08
diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
index 1c1971bb7c..e0d5d23978 100644
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
@@ -70,15 +70,17 @@ int __must_check amd_iommu_flush_iotlb_all(struct domain *d);
void amd_iommu_share_p2m(struct domain *d);
/* device table functions */
-int get_dma_requestor_id(u16 seg, u16 bdf);
-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 iommu_dte_add_device_entry(u32 *dte, struct ivrs_mappings *ivrs_dev);
-void iommu_dte_set_guest_cr3(u32 *dte, u16 dom_id, u64 gcr3,
- int gv, unsigned int glx);
+int get_dma_requestor_id(uint16_t seg, uint16_t bdf);
+void amd_iommu_set_intremap_table(struct amd_iommu_dte *dte,
+ uint64_t intremap_ptr,
+ uint8_t int_valid);
+void amd_iommu_set_root_page_table(struct amd_iommu_dte *dte,
+ uint64_t root_ptr, uint16_t domain_id,
+ uint8_t paging_mode, uint8_t valid);
+void iommu_dte_add_device_entry(struct amd_iommu_dte *dte,
+ struct ivrs_mappings *ivrs_dev);
+void iommu_dte_set_guest_cr3(struct amd_iommu_dte *dte, uint16_t dom_id,
+ uint64_t gcr3_mfn, uint8_t gv, uint8_t glx);
/* send cmd to iommu */
void amd_iommu_flush_all_pages(struct domain *d);
--
2.20.1.2.gb21ebb671
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] amd-iommu: use a bitfield for PTE/PDE
2019-02-04 11:19 ` [PATCH 1/2] amd-iommu: use a bitfield for PTE/PDE Paul Durrant
@ 2019-02-12 20:14 ` Woods, Brian
2019-02-13 9:45 ` Paul Durrant
0 siblings, 1 reply; 8+ messages in thread
From: Woods, Brian @ 2019-02-12 20:14 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Andrew Cooper, Wei Liu, Jan Beulich, Suthikulpanit, Suravee,
Roger Pau Monné
On 2/4/19 5:19 AM, Paul Durrant wrote:
> The current use of get/set_field_from/in_reg_u32() is both inefficient and
> requires some ugly casting.
>
> This patch defines a new bitfield structure (amd_iommu_pte) and uses this
> structure in all PTE/PDE manipulation, resulting in much more readable
> and compact code.
>
> NOTE: This commit also fixes one malformed comment in
> set_iommu_pte_present().
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Sorry about the delay.
Nitpick here, but I'd rather have !!IOMMUF_{writable,readable} than
true. Not worth a revision if there isn't anything else though (and is
debatable).
Acked-by: Brian Woods <brian.woods@amd.com>
> ---
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Brian Woods <brian.woods@amd.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> ---
> xen/drivers/passthrough/amd/iommu_map.c | 143 ++++--------------
> xen/drivers/passthrough/amd/pci_amd_iommu.c | 50 +++---
> xen/include/asm-x86/hvm/svm/amd-iommu-defs.h | 47 ++----
> xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 15 --
> 4 files changed, 64 insertions(+), 191 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
> index 67329b0c95..5fda6063df 100644
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -38,100 +38,45 @@ static unsigned int pfn_to_pde_idx(unsigned long pfn, unsigned int level)
> static unsigned int clear_iommu_pte_present(unsigned long l1_mfn,
> unsigned long dfn)
> {
> - uint64_t *table, *pte;
> + struct amd_iommu_pte *table, *pte;
> unsigned int flush_flags;
>
> table = map_domain_page(_mfn(l1_mfn));
> + pte = &table[pfn_to_pde_idx(dfn, 1)];
>
> - pte = (table + pfn_to_pde_idx(dfn, 1));
> + flush_flags = pte->pr ? IOMMU_FLUSHF_modified : 0;
> + memset(pte, 0, sizeof(*pte));
>
> - flush_flags = get_field_from_reg_u32(*pte, IOMMU_PTE_PRESENT_MASK,
> - IOMMU_PTE_PRESENT_SHIFT) ?
> - IOMMU_FLUSHF_modified : 0;
> -
> - *pte = 0;
> unmap_domain_page(table);
>
> return flush_flags;
> }
>
> -static unsigned int set_iommu_pde_present(uint32_t *pde,
> +static unsigned int set_iommu_pde_present(struct amd_iommu_pte *pte,
> unsigned long next_mfn,
> unsigned int next_level, bool iw,
> bool ir)
> {
> - uint64_t maddr_next;
> - uint32_t addr_lo, addr_hi, entry;
> - bool old_present;
> unsigned int flush_flags = IOMMU_FLUSHF_added;
>
> - maddr_next = __pfn_to_paddr(next_mfn);
> -
> - old_present = get_field_from_reg_u32(pde[0], IOMMU_PTE_PRESENT_MASK,
> - IOMMU_PTE_PRESENT_SHIFT);
> - if ( old_present )
> - {
> - bool old_r, old_w;
> - unsigned int old_level;
> - uint64_t maddr_old;
> -
> - addr_hi = get_field_from_reg_u32(pde[1],
> - IOMMU_PTE_ADDR_HIGH_MASK,
> - IOMMU_PTE_ADDR_HIGH_SHIFT);
> - addr_lo = get_field_from_reg_u32(pde[0],
> - IOMMU_PTE_ADDR_LOW_MASK,
> - IOMMU_PTE_ADDR_LOW_SHIFT);
> - old_level = get_field_from_reg_u32(pde[0],
> - IOMMU_PDE_NEXT_LEVEL_MASK,
> - IOMMU_PDE_NEXT_LEVEL_SHIFT);
> - old_w = get_field_from_reg_u32(pde[1],
> - IOMMU_PTE_IO_WRITE_PERMISSION_MASK,
> - IOMMU_PTE_IO_WRITE_PERMISSION_SHIFT);
> - old_r = get_field_from_reg_u32(pde[1],
> - IOMMU_PTE_IO_READ_PERMISSION_MASK,
> - IOMMU_PTE_IO_READ_PERMISSION_SHIFT);
> -
> - maddr_old = ((uint64_t)addr_hi << 32) |
> - ((uint64_t)addr_lo << PAGE_SHIFT);
> -
> - if ( maddr_old != maddr_next || iw != old_w || ir != old_r ||
> - old_level != next_level )
> + if ( pte->pr &&
> + (pte->mfn != next_mfn ||
> + pte->iw != iw ||
> + pte->ir != ir ||
> + pte->next_level != next_level) )
> flush_flags |= IOMMU_FLUSHF_modified;
> - }
>
> - addr_lo = maddr_next & DMA_32BIT_MASK;
> - addr_hi = maddr_next >> 32;
> -
> - /* enable read/write permissions,which will be enforced at the PTE */
> - set_field_in_reg_u32(addr_hi, 0,
> - IOMMU_PDE_ADDR_HIGH_MASK,
> - IOMMU_PDE_ADDR_HIGH_SHIFT, &entry);
> - set_field_in_reg_u32(iw, entry,
> - IOMMU_PDE_IO_WRITE_PERMISSION_MASK,
> - IOMMU_PDE_IO_WRITE_PERMISSION_SHIFT, &entry);
> - set_field_in_reg_u32(ir, entry,
> - IOMMU_PDE_IO_READ_PERMISSION_MASK,
> - IOMMU_PDE_IO_READ_PERMISSION_SHIFT, &entry);
> -
> - /* FC bit should be enabled in PTE, this helps to solve potential
> + /*
> + * FC bit should be enabled in PTE, this helps to solve potential
> * issues with ATS devices
> */
> - if ( next_level == 0 )
> - set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, entry,
> - IOMMU_PTE_FC_MASK, IOMMU_PTE_FC_SHIFT, &entry);
> - pde[1] = entry;
> + pte->fc = !next_level;
>
> - /* mark next level as 'present' */
> - set_field_in_reg_u32(addr_lo >> PAGE_SHIFT, 0,
> - IOMMU_PDE_ADDR_LOW_MASK,
> - IOMMU_PDE_ADDR_LOW_SHIFT, &entry);
> - set_field_in_reg_u32(next_level, entry,
> - IOMMU_PDE_NEXT_LEVEL_MASK,
> - IOMMU_PDE_NEXT_LEVEL_SHIFT, &entry);
> - set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, entry,
> - IOMMU_PDE_PRESENT_MASK,
> - IOMMU_PDE_PRESENT_SHIFT, &entry);
> - pde[0] = entry;
> + pte->mfn = next_mfn;
> + pte->iw = iw;
> + pte->ir = ir;
> + pte->next_level = next_level;
> + pte->pr = 1;
>
> return flush_flags;
> }
> @@ -142,13 +87,11 @@ static unsigned int set_iommu_pte_present(unsigned long pt_mfn,
> int pde_level,
> bool iw, bool ir)
> {
> - uint64_t *table;
> - uint32_t *pde;
> + struct amd_iommu_pte *table, *pde;
> unsigned int flush_flags;
>
> table = map_domain_page(_mfn(pt_mfn));
> -
> - pde = (uint32_t *)(table + pfn_to_pde_idx(dfn, pde_level));
> + pde = &table[pfn_to_pde_idx(dfn, pde_level)];
>
> flush_flags = set_iommu_pde_present(pde, next_mfn, 0, iw, ir);
> unmap_domain_page(table);
> @@ -319,25 +262,6 @@ void iommu_dte_set_guest_cr3(uint32_t *dte, uint16_t dom_id, uint64_t gcr3,
> dte[1] = entry;
> }
>
> -uint64_t amd_iommu_get_address_from_pte(void *pte)
> -{
> - uint32_t *entry = pte;
> - uint32_t addr_lo, addr_hi;
> - uint64_t ptr;
> -
> - addr_lo = get_field_from_reg_u32(entry[0],
> - IOMMU_PTE_ADDR_LOW_MASK,
> - IOMMU_PTE_ADDR_LOW_SHIFT);
> -
> - addr_hi = get_field_from_reg_u32(entry[1],
> - IOMMU_PTE_ADDR_HIGH_MASK,
> - IOMMU_PTE_ADDR_HIGH_SHIFT);
> -
> - ptr = ((uint64_t)addr_hi << 32) |
> - ((uint64_t)addr_lo << PAGE_SHIFT);
> - return ptr;
> -}
> -
> /* Walk io page tables and build level page tables if necessary
> * {Re, un}mapping super page frames causes re-allocation of io
> * page tables.
> @@ -345,7 +269,7 @@ uint64_t amd_iommu_get_address_from_pte(void *pte)
> static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
> unsigned long pt_mfn[])
> {
> - uint64_t *pde, *next_table_vaddr;
> + struct amd_iommu_pte *pde, *next_table_vaddr;
> unsigned long next_table_mfn;
> unsigned int level;
> struct page_info *table;
> @@ -370,15 +294,13 @@ static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
> pt_mfn[level] = next_table_mfn;
>
> next_table_vaddr = map_domain_page(_mfn(next_table_mfn));
> - pde = next_table_vaddr + pfn_to_pde_idx(dfn, level);
> + pde = &next_table_vaddr[pfn_to_pde_idx(dfn, level)];
>
> /* Here might be a super page frame */
> - next_table_mfn = amd_iommu_get_address_from_pte(pde) >> PAGE_SHIFT;
> + next_table_mfn = pde->mfn;
>
> /* Split super page frame into smaller pieces.*/
> - if ( iommu_is_pte_present((uint32_t *)pde) &&
> - (iommu_next_level((uint32_t *)pde) == 0) &&
> - next_table_mfn != 0 )
> + if ( pde->pr && !pde->next_level && next_table_mfn )
> {
> int i;
> unsigned long mfn, pfn;
> @@ -398,13 +320,13 @@ static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
> }
>
> next_table_mfn = mfn_x(page_to_mfn(table));
> - set_iommu_pde_present((uint32_t *)pde, next_table_mfn, next_level,
> - !!IOMMUF_writable, !!IOMMUF_readable);
> + set_iommu_pde_present(pde, next_table_mfn, next_level, true,
> + true);
>
> for ( i = 0; i < PTE_PER_TABLE_SIZE; i++ )
> {
> set_iommu_pte_present(next_table_mfn, pfn, mfn, next_level,
> - !!IOMMUF_writable, !!IOMMUF_readable);
> + true, true);
> mfn += page_sz;
> pfn += page_sz;
> }
> @@ -413,7 +335,7 @@ static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
> }
>
> /* Install lower level page table for non-present entries */
> - else if ( !iommu_is_pte_present((uint32_t *)pde) )
> + else if ( !pde->pr )
> {
> if ( next_table_mfn == 0 )
> {
> @@ -425,9 +347,8 @@ static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
> return 1;
> }
> next_table_mfn = mfn_x(page_to_mfn(table));
> - set_iommu_pde_present((uint32_t *)pde, next_table_mfn,
> - next_level, !!IOMMUF_writable,
> - !!IOMMUF_readable);
> + set_iommu_pde_present(pde, next_table_mfn, next_level, true,
> + true);
> }
> else /* should never reach here */
> {
> @@ -455,7 +376,7 @@ static int update_paging_mode(struct domain *d, unsigned long dfn)
> struct amd_iommu *iommu = NULL;
> struct page_info *new_root = NULL;
> struct page_info *old_root = NULL;
> - void *new_root_vaddr;
> + struct amd_iommu_pte *new_root_vaddr;
> unsigned long old_root_mfn;
> struct domain_iommu *hd = dom_iommu(d);
>
> @@ -484,7 +405,7 @@ static int update_paging_mode(struct domain *d, unsigned long dfn)
> new_root_vaddr = __map_domain_page(new_root);
> old_root_mfn = mfn_x(page_to_mfn(old_root));
> set_iommu_pde_present(new_root_vaddr, old_root_mfn, level,
> - !!IOMMUF_writable, !!IOMMUF_readable);
> + true, true);
> level++;
> old_root = new_root;
> offset >>= PTE_PER_TABLE_SHIFT;
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index 33a3798f36..da6748320b 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -376,9 +376,8 @@ static void deallocate_next_page_table(struct page_info *pg, int level)
>
> static void deallocate_page_table(struct page_info *pg)
> {
> - void *table_vaddr, *pde;
> - u64 next_table_maddr;
> - unsigned int index, level = PFN_ORDER(pg), next_level;
> + struct amd_iommu_pte *table_vaddr;
> + unsigned int index, level = PFN_ORDER(pg);
>
> PFN_ORDER(pg) = 0;
>
> @@ -392,17 +391,14 @@ static void deallocate_page_table(struct page_info *pg)
>
> for ( index = 0; index < PTE_PER_TABLE_SIZE; index++ )
> {
> - pde = table_vaddr + (index * IOMMU_PAGE_TABLE_ENTRY_SIZE);
> - next_table_maddr = amd_iommu_get_address_from_pte(pde);
> - next_level = iommu_next_level(pde);
> + struct amd_iommu_pte *pde = &table_vaddr[index];
>
> - if ( (next_table_maddr != 0) && (next_level != 0) &&
> - iommu_is_pte_present(pde) )
> + if ( pde->mfn && pde->next_level && pde->pr )
> {
> /* We do not support skip levels yet */
> - ASSERT(next_level == level - 1);
> - deallocate_next_page_table(maddr_to_page(next_table_maddr),
> - next_level);
> + ASSERT(pde->next_level == level - 1);
> + deallocate_next_page_table(mfn_to_page(_mfn(pde->mfn)),
> + pde->next_level);
> }
> }
>
> @@ -500,10 +496,8 @@ static void amd_dump_p2m_table_level(struct page_info* pg, int level,
> paddr_t gpa, int indent)
> {
> paddr_t address;
> - void *table_vaddr, *pde;
> - paddr_t next_table_maddr;
> - int index, next_level, present;
> - u32 *entry;
> + struct amd_iommu_pte *table_vaddr;
> + int index;
>
> if ( level < 1 )
> return;
> @@ -518,42 +512,32 @@ static void amd_dump_p2m_table_level(struct page_info* pg, int level,
>
> for ( index = 0; index < PTE_PER_TABLE_SIZE; index++ )
> {
> + struct amd_iommu_pte *pde = &table_vaddr[index];
> +
> if ( !(index % 2) )
> process_pending_softirqs();
>
> - pde = table_vaddr + (index * IOMMU_PAGE_TABLE_ENTRY_SIZE);
> - next_table_maddr = amd_iommu_get_address_from_pte(pde);
> - entry = pde;
> -
> - present = get_field_from_reg_u32(entry[0],
> - IOMMU_PDE_PRESENT_MASK,
> - IOMMU_PDE_PRESENT_SHIFT);
> -
> - if ( !present )
> + if ( !pde->pr )
> continue;
>
> - next_level = get_field_from_reg_u32(entry[0],
> - IOMMU_PDE_NEXT_LEVEL_MASK,
> - IOMMU_PDE_NEXT_LEVEL_SHIFT);
> -
> - if ( next_level && (next_level != (level - 1)) )
> + if ( pde->next_level && (pde->next_level != (level - 1)) )
> {
> printk("IOMMU p2m table error. next_level = %d, expected %d\n",
> - next_level, level - 1);
> + pde->next_level, level - 1);
>
> continue;
> }
>
> address = gpa + amd_offset_level_address(index, level);
> - if ( next_level >= 1 )
> + if ( pde->next_level >= 1 )
> amd_dump_p2m_table_level(
> - maddr_to_page(next_table_maddr), next_level,
> + mfn_to_page(_mfn(pde->mfn)), pde->next_level,
> address, indent + 1);
> else
> printk("%*sdfn: %08lx mfn: %08lx\n",
> indent, "",
> (unsigned long)PFN_DOWN(address),
> - (unsigned long)PFN_DOWN(next_table_maddr));
> + (unsigned long)PFN_DOWN(pfn_to_paddr(pde->mfn)));
> }
>
> unmap_domain_page(table_vaddr);
> diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> index a217245249..a3a49f91eb 100644
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> @@ -413,38 +413,21 @@
> #define IOMMU_PAGE_TABLE_U32_PER_ENTRY (IOMMU_PAGE_TABLE_ENTRY_SIZE / 4)
> #define IOMMU_PAGE_TABLE_ALIGNMENT 4096
>
> -#define IOMMU_PTE_PRESENT_MASK 0x00000001
> -#define IOMMU_PTE_PRESENT_SHIFT 0
> -#define IOMMU_PTE_NEXT_LEVEL_MASK 0x00000E00
> -#define IOMMU_PTE_NEXT_LEVEL_SHIFT 9
> -#define IOMMU_PTE_ADDR_LOW_MASK 0xFFFFF000
> -#define IOMMU_PTE_ADDR_LOW_SHIFT 12
> -#define IOMMU_PTE_ADDR_HIGH_MASK 0x000FFFFF
> -#define IOMMU_PTE_ADDR_HIGH_SHIFT 0
> -#define IOMMU_PTE_U_MASK 0x08000000
> -#define IOMMU_PTE_U_SHIFT 7
> -#define IOMMU_PTE_FC_MASK 0x10000000
> -#define IOMMU_PTE_FC_SHIFT 28
> -#define IOMMU_PTE_IO_READ_PERMISSION_MASK 0x20000000
> -#define IOMMU_PTE_IO_READ_PERMISSION_SHIFT 29
> -#define IOMMU_PTE_IO_WRITE_PERMISSION_MASK 0x40000000
> -#define IOMMU_PTE_IO_WRITE_PERMISSION_SHIFT 30
> -
> -/* I/O Page Directory */
> -#define IOMMU_PAGE_DIRECTORY_ENTRY_SIZE 8
> -#define IOMMU_PAGE_DIRECTORY_ALIGNMENT 4096
> -#define IOMMU_PDE_PRESENT_MASK 0x00000001
> -#define IOMMU_PDE_PRESENT_SHIFT 0
> -#define IOMMU_PDE_NEXT_LEVEL_MASK 0x00000E00
> -#define IOMMU_PDE_NEXT_LEVEL_SHIFT 9
> -#define IOMMU_PDE_ADDR_LOW_MASK 0xFFFFF000
> -#define IOMMU_PDE_ADDR_LOW_SHIFT 12
> -#define IOMMU_PDE_ADDR_HIGH_MASK 0x000FFFFF
> -#define IOMMU_PDE_ADDR_HIGH_SHIFT 0
> -#define IOMMU_PDE_IO_READ_PERMISSION_MASK 0x20000000
> -#define IOMMU_PDE_IO_READ_PERMISSION_SHIFT 29
> -#define IOMMU_PDE_IO_WRITE_PERMISSION_MASK 0x40000000
> -#define IOMMU_PDE_IO_WRITE_PERMISSION_SHIFT 30
> +struct amd_iommu_pte {
> + uint64_t pr:1;
> + uint64_t ignored0:4;
> + uint64_t a:1;
> + uint64_t d:1;
> + uint64_t ignored1:2;
> + uint64_t next_level:3;
> + uint64_t mfn:40;
> + uint64_t reserved:7;
> + uint64_t u:1;
> + uint64_t fc:1;
> + uint64_t ir:1;
> + uint64_t iw:1;
> + uint64_t ignored2:1;
> +};
>
> /* Paging modes */
> #define IOMMU_PAGING_MODE_DISABLED 0x0
> diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> index c5697565d6..1c1971bb7c 100644
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> @@ -57,7 +57,6 @@ int __must_check amd_iommu_map_page(struct domain *d, dfn_t dfn,
> unsigned int *flush_flags);
> int __must_check amd_iommu_unmap_page(struct domain *d, dfn_t dfn,
> unsigned int *flush_flags);
> -uint64_t amd_iommu_get_address_from_pte(void *entry);
> int __must_check amd_iommu_alloc_root(struct domain_iommu *hd);
> int amd_iommu_reserve_domain_unity_map(struct domain *domain,
> paddr_t phys_addr, unsigned long size,
> @@ -280,18 +279,4 @@ static inline void iommu_set_addr_hi_to_reg(uint32_t *reg, uint32_t addr)
> IOMMU_REG_BASE_ADDR_HIGH_SHIFT, reg);
> }
>
> -static inline int iommu_is_pte_present(const u32 *entry)
> -{
> - return get_field_from_reg_u32(entry[0],
> - IOMMU_PDE_PRESENT_MASK,
> - IOMMU_PDE_PRESENT_SHIFT);
> -}
> -
> -static inline unsigned int iommu_next_level(const u32 *entry)
> -{
> - return get_field_from_reg_u32(entry[0],
> - IOMMU_PDE_NEXT_LEVEL_MASK,
> - IOMMU_PDE_NEXT_LEVEL_SHIFT);
> -}
> -
> #endif /* _ASM_X86_64_AMD_IOMMU_PROTO_H */
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] amd-iommu: use a bitfield for DTE
2019-02-04 11:19 ` [PATCH 2/2] amd-iommu: use a bitfield for DTE Paul Durrant
@ 2019-02-12 20:30 ` Woods, Brian
0 siblings, 0 replies; 8+ messages in thread
From: Woods, Brian @ 2019-02-12 20:30 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Andrew Cooper, Wei Liu, Jan Beulich, Suthikulpanit, Suravee,
Roger Pau Monné
On 2/4/19 5:19 AM, Paul Durrant wrote:
> The current use of get/set_field_from/in_reg_u32() is both inefficient and
> requires some ugly casting.
>
> This patch defines a new bitfield structure (amd_iommu_dte) and uses this
> structure in all DTE manipulation, resulting in much more readable and
> compact code.
>
> NOTE: This patch also includes some clean-up of get_dma_requestor_id() to
> change the types of the arguments from u16 to uint16_t.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Acked-by: Brian Woods <brian.woods@amd.com>
> ---
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Cc: Brian Woods <brian.woods@amd.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> ---
> xen/drivers/passthrough/amd/iommu_guest.c | 55 ++---
> xen/drivers/passthrough/amd/iommu_map.c | 199 +++++-------------
> xen/drivers/passthrough/amd/pci_amd_iommu.c | 51 ++---
> xen/include/asm-x86/amd-iommu.h | 5 -
> xen/include/asm-x86/hvm/svm/amd-iommu-defs.h | 120 +++++------
> xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 20 +-
> 6 files changed, 139 insertions(+), 311 deletions(-)
>
> diff --git a/xen/drivers/passthrough/amd/iommu_guest.c b/xen/drivers/passthrough/amd/iommu_guest.c
> index 96175bb9ac..328e7509d5 100644
> --- a/xen/drivers/passthrough/amd/iommu_guest.c
> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
> @@ -76,39 +76,10 @@ static void guest_iommu_disable(struct guest_iommu *iommu)
> iommu->enabled = 0;
> }
>
> -static uint64_t get_guest_cr3_from_dte(dev_entry_t *dte)
> +static uint64_t get_guest_cr3_from_dte(struct amd_iommu_dte *dte)
> {
> - uint64_t gcr3_1, gcr3_2, gcr3_3;
> -
> - gcr3_1 = get_field_from_reg_u32(dte->data[1],
> - IOMMU_DEV_TABLE_GCR3_1_MASK,
> - IOMMU_DEV_TABLE_GCR3_1_SHIFT);
> - gcr3_2 = get_field_from_reg_u32(dte->data[2],
> - IOMMU_DEV_TABLE_GCR3_2_MASK,
> - IOMMU_DEV_TABLE_GCR3_2_SHIFT);
> - gcr3_3 = get_field_from_reg_u32(dte->data[3],
> - IOMMU_DEV_TABLE_GCR3_3_MASK,
> - IOMMU_DEV_TABLE_GCR3_3_SHIFT);
> -
> - return ((gcr3_3 << 31) | (gcr3_2 << 15 ) | (gcr3_1 << 12)) >> PAGE_SHIFT;
> -}
> -
> -static uint16_t get_domid_from_dte(dev_entry_t *dte)
> -{
> - return get_field_from_reg_u32(dte->data[2], IOMMU_DEV_TABLE_DOMAIN_ID_MASK,
> - IOMMU_DEV_TABLE_DOMAIN_ID_SHIFT);
> -}
> -
> -static uint16_t get_glx_from_dte(dev_entry_t *dte)
> -{
> - return get_field_from_reg_u32(dte->data[1], IOMMU_DEV_TABLE_GLX_MASK,
> - IOMMU_DEV_TABLE_GLX_SHIFT);
> -}
> -
> -static uint16_t get_gv_from_dte(dev_entry_t *dte)
> -{
> - return get_field_from_reg_u32(dte->data[1],IOMMU_DEV_TABLE_GV_MASK,
> - IOMMU_DEV_TABLE_GV_SHIFT);
> + return ((dte->gcr3_trp_51_31 << 31) | (dte->gcr3_trp_30_15 << 15) |
> + (dte->gcr3_trp_14_12 << 12)) >> PAGE_SHIFT;
> }
>
> static unsigned int host_domid(struct domain *d, uint64_t g_domid)
> @@ -397,7 +368,7 @@ static int do_completion_wait(struct domain *d, cmd_entry_t *cmd)
> static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
> {
> uint16_t gbdf, mbdf, req_id, gdom_id, hdom_id;
> - dev_entry_t *gdte, *mdte, *dte_base;
> + struct amd_iommu_dte *gdte, *mdte, *dte_base;
> struct amd_iommu *iommu = NULL;
> struct guest_iommu *g_iommu;
> uint64_t gcr3_gfn, gcr3_mfn;
> @@ -414,23 +385,23 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
> return 0;
>
> /* Sometimes guest invalidates devices from non-exists dtes */
> - if ( (gbdf * sizeof(dev_entry_t)) > g_iommu->dev_table.size )
> + if ( (gbdf * sizeof(struct amd_iommu_dte)) > g_iommu->dev_table.size )
> return 0;
>
> dte_mfn = guest_iommu_get_table_mfn(d,
> reg_to_u64(g_iommu->dev_table.reg_base),
> - sizeof(dev_entry_t), gbdf);
> + sizeof(struct amd_iommu_dte), gbdf);
> ASSERT(mfn_valid(_mfn(dte_mfn)));
>
> /* Read guest dte information */
> dte_base = map_domain_page(_mfn(dte_mfn));
>
> - gdte = dte_base + gbdf % (PAGE_SIZE / sizeof(dev_entry_t));
> + gdte = &dte_base[gbdf % (PAGE_SIZE / sizeof(struct amd_iommu_dte))];
>
> - gdom_id = get_domid_from_dte(gdte);
> + gdom_id = gdte->domain_id;
> gcr3_gfn = get_guest_cr3_from_dte(gdte);
> - glx = get_glx_from_dte(gdte);
> - gv = get_gv_from_dte(gdte);
> + glx = gdte->glx;
> + gv = gdte->gv;
>
> unmap_domain_page(dte_base);
>
> @@ -454,11 +425,11 @@ static int do_invalidate_dte(struct domain *d, cmd_entry_t *cmd)
> /* Setup host device entry */
> hdom_id = host_domid(d, gdom_id);
> req_id = get_dma_requestor_id(iommu->seg, mbdf);
> - mdte = iommu->dev_table.buffer + (req_id * sizeof(dev_entry_t));
> + dte_base = iommu->dev_table.buffer;
> + mdte = &dte_base[req_id];
>
> spin_lock_irqsave(&iommu->lock, flags);
> - iommu_dte_set_guest_cr3((u32 *)mdte, hdom_id,
> - gcr3_mfn << PAGE_SHIFT, gv, glx);
> + iommu_dte_set_guest_cr3(mdte, hdom_id, gcr3_mfn, gv, glx);
>
> amd_iommu_flush_device(iommu, req_id);
> spin_unlock_irqrestore(&iommu->lock, flags);
> diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
> index 5fda6063df..cbf00e9e72 100644
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -99,167 +99,64 @@ static unsigned int set_iommu_pte_present(unsigned long pt_mfn,
> return flush_flags;
> }
>
> -void amd_iommu_set_root_page_table(uint32_t *dte, uint64_t root_ptr,
> - uint16_t domain_id, uint8_t paging_mode,
> - uint8_t valid)
> +void amd_iommu_set_root_page_table(struct amd_iommu_dte *dte,
> + uint64_t root_ptr, uint16_t domain_id,
> + uint8_t paging_mode, uint8_t valid)
> {
> - uint32_t addr_hi, addr_lo, entry;
> - set_field_in_reg_u32(domain_id, 0,
> - IOMMU_DEV_TABLE_DOMAIN_ID_MASK,
> - IOMMU_DEV_TABLE_DOMAIN_ID_SHIFT, &entry);
> - dte[2] = entry;
> -
> - addr_lo = root_ptr & DMA_32BIT_MASK;
> - addr_hi = root_ptr >> 32;
> -
> - set_field_in_reg_u32(addr_hi, 0,
> - IOMMU_DEV_TABLE_PAGE_TABLE_PTR_HIGH_MASK,
> - IOMMU_DEV_TABLE_PAGE_TABLE_PTR_HIGH_SHIFT, &entry);
> - set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, entry,
> - IOMMU_DEV_TABLE_IO_WRITE_PERMISSION_MASK,
> - IOMMU_DEV_TABLE_IO_WRITE_PERMISSION_SHIFT, &entry);
> - set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, entry,
> - IOMMU_DEV_TABLE_IO_READ_PERMISSION_MASK,
> - IOMMU_DEV_TABLE_IO_READ_PERMISSION_SHIFT, &entry);
> - dte[1] = entry;
> -
> - set_field_in_reg_u32(addr_lo >> PAGE_SHIFT, 0,
> - IOMMU_DEV_TABLE_PAGE_TABLE_PTR_LOW_MASK,
> - IOMMU_DEV_TABLE_PAGE_TABLE_PTR_LOW_SHIFT, &entry);
> - set_field_in_reg_u32(paging_mode, entry,
> - IOMMU_DEV_TABLE_PAGING_MODE_MASK,
> - IOMMU_DEV_TABLE_PAGING_MODE_SHIFT, &entry);
> - set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, entry,
> - IOMMU_DEV_TABLE_TRANSLATION_VALID_MASK,
> - IOMMU_DEV_TABLE_TRANSLATION_VALID_SHIFT, &entry);
> - set_field_in_reg_u32(valid ? IOMMU_CONTROL_ENABLED :
> - IOMMU_CONTROL_DISABLED, entry,
> - IOMMU_DEV_TABLE_VALID_MASK,
> - IOMMU_DEV_TABLE_VALID_SHIFT, &entry);
> - dte[0] = entry;
> -}
> -
> -void iommu_dte_set_iotlb(uint32_t *dte, uint8_t i)
> -{
> - uint32_t 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;
> + dte->domain_id = domain_id;
> + dte->pt_root = paddr_to_pfn(root_ptr);
> + dte->iw = 1;
> + dte->ir = 1;
> + dte->paging_mode = paging_mode;
> + dte->tv = 1;
> + dte->v = valid;
> }
>
> void __init amd_iommu_set_intremap_table(
> - uint32_t *dte, uint64_t intremap_ptr, uint8_t int_valid)
> + struct amd_iommu_dte *dte, uint64_t intremap_ptr, uint8_t int_valid)
> {
> - uint32_t addr_hi, addr_lo, entry;
> -
> - addr_lo = intremap_ptr & DMA_32BIT_MASK;
> - addr_hi = intremap_ptr >> 32;
> -
> - entry = dte[5];
> - set_field_in_reg_u32(addr_hi, entry,
> - IOMMU_DEV_TABLE_INT_TABLE_PTR_HIGH_MASK,
> - IOMMU_DEV_TABLE_INT_TABLE_PTR_HIGH_SHIFT, &entry);
> - /* Fixed and arbitrated interrupts remapepd */
> - set_field_in_reg_u32(2, entry,
> - IOMMU_DEV_TABLE_INT_CONTROL_MASK,
> - IOMMU_DEV_TABLE_INT_CONTROL_SHIFT, &entry);
> - dte[5] = entry;
> -
> - set_field_in_reg_u32(addr_lo >> 6, 0,
> - IOMMU_DEV_TABLE_INT_TABLE_PTR_LOW_MASK,
> - IOMMU_DEV_TABLE_INT_TABLE_PTR_LOW_SHIFT, &entry);
> - /* 2048 entries */
> - set_field_in_reg_u32(0xB, entry,
> - IOMMU_DEV_TABLE_INT_TABLE_LENGTH_MASK,
> - IOMMU_DEV_TABLE_INT_TABLE_LENGTH_SHIFT, &entry);
> -
> - /* unmapped interrupt results io page faults*/
> - set_field_in_reg_u32(IOMMU_CONTROL_DISABLED, entry,
> - IOMMU_DEV_TABLE_INT_TABLE_IGN_UNMAPPED_MASK,
> - IOMMU_DEV_TABLE_INT_TABLE_IGN_UNMAPPED_SHIFT, &entry);
> - set_field_in_reg_u32(int_valid ? IOMMU_CONTROL_ENABLED :
> - IOMMU_CONTROL_DISABLED, entry,
> - IOMMU_DEV_TABLE_INT_VALID_MASK,
> - IOMMU_DEV_TABLE_INT_VALID_SHIFT, &entry);
> - dte[4] = entry;
> + dte->it_root = intremap_ptr >> 6;
> + dte->int_tab_len = 0xb; /* 2048 entries */
> + dte->int_ctl = 2; /* fixed and arbitrated interrupts remapped */
> + dte->ig = 0; /* unmapped interrupt results io page faults */
> + dte->iv = int_valid;
> }
>
> -void __init iommu_dte_add_device_entry(uint32_t *dte,
> +void __init iommu_dte_add_device_entry(struct amd_iommu_dte *dte,
> struct ivrs_mappings *ivrs_dev)
> {
> - uint32_t entry;
> - uint8_t sys_mgt, dev_ex, flags;
> - uint8_t mask = ~(0x7 << 3);
> -
> - dte[7] = dte[6] = dte[4] = dte[2] = dte[1] = dte[0] = 0;
> -
> - flags = ivrs_dev->device_flags;
> - sys_mgt = MASK_EXTR(flags, ACPI_IVHD_SYSTEM_MGMT);
> - dev_ex = ivrs_dev->dte_allow_exclusion;
> -
> - flags &= mask;
> - set_field_in_reg_u32(flags, 0,
> - IOMMU_DEV_TABLE_IVHD_FLAGS_MASK,
> - IOMMU_DEV_TABLE_IVHD_FLAGS_SHIFT, &entry);
> - dte[5] = entry;
> -
> - set_field_in_reg_u32(sys_mgt, 0,
> - IOMMU_DEV_TABLE_SYS_MGT_MSG_ENABLE_MASK,
> - IOMMU_DEV_TABLE_SYS_MGT_MSG_ENABLE_SHIFT, &entry);
> - set_field_in_reg_u32(dev_ex, entry,
> - IOMMU_DEV_TABLE_ALLOW_EXCLUSION_MASK,
> - IOMMU_DEV_TABLE_ALLOW_EXCLUSION_SHIFT, &entry);
> - dte[3] = entry;
> + uint8_t flags = ivrs_dev->device_flags;
> +
> + memset(dte, 0, sizeof(*dte));
> +
> + dte->init_pass = MASK_EXTR(flags, ACPI_IVHD_INIT_PASS);
> + dte->ext_int_pass = MASK_EXTR(flags, ACPI_IVHD_EINT_PASS);
> + dte->nmi_pass = MASK_EXTR(flags, ACPI_IVHD_NMI_PASS);
> + dte->lint0_pass = MASK_EXTR(flags, ACPI_IVHD_LINT0_PASS);
> + dte->lint1_pass = MASK_EXTR(flags, ACPI_IVHD_LINT1_PASS);
> + dte->sys_mgt = MASK_EXTR(flags, ACPI_IVHD_SYSTEM_MGMT);
> + dte->ex = ivrs_dev->dte_allow_exclusion;
> }
>
> -void iommu_dte_set_guest_cr3(uint32_t *dte, uint16_t dom_id, uint64_t gcr3,
> - int gv, unsigned int glx)
> +void iommu_dte_set_guest_cr3(struct amd_iommu_dte *dte, uint16_t dom_id,
> + uint64_t gcr3_mfn, uint8_t gv, uint8_t glx)
> {
> - uint32_t entry, gcr3_1, gcr3_2, gcr3_3;
> -
> - gcr3_3 = gcr3 >> 31;
> - gcr3_2 = (gcr3 >> 15) & 0xFFFF;
> - gcr3_1 = (gcr3 >> PAGE_SHIFT) & 0x7;
> +#define GCR3_MASK(hi, lo) (((1ul << ((hi) + 1)) - 1) & ~((1ul << (lo)) - 1))
> +#define GCR3_SHIFT(lo) ((lo) - PAGE_SHIFT)
>
> /* I bit must be set when gcr3 is enabled */
> - entry = dte[3];
> - set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, entry,
> - IOMMU_DEV_TABLE_IOTLB_SUPPORT_MASK,
> - IOMMU_DEV_TABLE_IOTLB_SUPPORT_SHIFT, &entry);
> - /* update gcr3 */
> - set_field_in_reg_u32(gcr3_3, entry,
> - IOMMU_DEV_TABLE_GCR3_3_MASK,
> - IOMMU_DEV_TABLE_GCR3_3_SHIFT, &entry);
> - dte[3] = entry;
> -
> - set_field_in_reg_u32(dom_id, entry,
> - IOMMU_DEV_TABLE_DOMAIN_ID_MASK,
> - IOMMU_DEV_TABLE_DOMAIN_ID_SHIFT, &entry);
> - /* update gcr3 */
> - entry = dte[2];
> - set_field_in_reg_u32(gcr3_2, entry,
> - IOMMU_DEV_TABLE_GCR3_2_MASK,
> - IOMMU_DEV_TABLE_GCR3_2_SHIFT, &entry);
> - dte[2] = entry;
> -
> - entry = dte[1];
> - /* Enable GV bit */
> - set_field_in_reg_u32(!!gv, entry,
> - IOMMU_DEV_TABLE_GV_MASK,
> - IOMMU_DEV_TABLE_GV_SHIFT, &entry);
> -
> - /* 1 level guest cr3 table */
> - set_field_in_reg_u32(glx, entry,
> - IOMMU_DEV_TABLE_GLX_MASK,
> - IOMMU_DEV_TABLE_GLX_SHIFT, &entry);
> - /* update gcr3 */
> - set_field_in_reg_u32(gcr3_1, entry,
> - IOMMU_DEV_TABLE_GCR3_1_MASK,
> - IOMMU_DEV_TABLE_GCR3_1_SHIFT, &entry);
> - dte[1] = entry;
> + dte->i = 1;
> +
> + dte->gcr3_trp_14_12 = (gcr3_mfn & GCR3_MASK(14, 12)) >> GCR3_SHIFT(12);
> + dte->gcr3_trp_30_15 = (gcr3_mfn & GCR3_MASK(30, 15)) >> GCR3_SHIFT(15);
> + dte->gcr3_trp_51_31 = (gcr3_mfn & GCR3_MASK(51, 31)) >> GCR3_SHIFT(31);
> +
> + dte->domain_id = dom_id;
> + dte->glx = glx;
> + dte->gv = gv;
> +
> +#undef GCR3_SHIFT
> +#undef GCR3_MASK
> }
>
> /* Walk io page tables and build level page tables if necessary
> @@ -369,7 +266,7 @@ static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
> static int update_paging_mode(struct domain *d, unsigned long dfn)
> {
> uint16_t bdf;
> - void *device_entry;
> + struct amd_iommu_dte *table, *dte;
> unsigned int req_id, level, offset;
> unsigned long flags;
> struct pci_dev *pdev;
> @@ -438,11 +335,11 @@ static int update_paging_mode(struct domain *d, unsigned long dfn)
> spin_lock_irqsave(&iommu->lock, flags);
> do {
> req_id = get_dma_requestor_id(pdev->seg, bdf);
> - device_entry = iommu->dev_table.buffer +
> - (req_id * IOMMU_DEV_TABLE_ENTRY_SIZE);
> + table = iommu->dev_table.buffer;
> + dte = &table[req_id];
>
> /* valid = 0 only works for dom0 passthrough mode */
> - amd_iommu_set_root_page_table((uint32_t *)device_entry,
> + amd_iommu_set_root_page_table(dte,
> page_to_maddr(hd->arch.root_table),
> d->domain_id,
> hd->arch.paging_mode, 1);
> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> index da6748320b..f6c17ba87a 100644
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -71,7 +71,7 @@ struct amd_iommu *find_iommu_for_device(int seg, int bdf)
> * Return original device id, if device has valid interrupt remapping
> * table setup for both select entry and alias entry.
> */
> -int get_dma_requestor_id(u16 seg, u16 bdf)
> +int get_dma_requestor_id(uint16_t seg, uint16_t bdf)
> {
> struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(seg);
> int req_id;
> @@ -85,35 +85,11 @@ int get_dma_requestor_id(u16 seg, u16 bdf)
> return req_id;
> }
>
> -static int is_translation_valid(u32 *entry)
> -{
> - return (get_field_from_reg_u32(entry[0],
> - IOMMU_DEV_TABLE_VALID_MASK,
> - IOMMU_DEV_TABLE_VALID_SHIFT) &&
> - get_field_from_reg_u32(entry[0],
> - IOMMU_DEV_TABLE_TRANSLATION_VALID_MASK,
> - IOMMU_DEV_TABLE_TRANSLATION_VALID_SHIFT));
> -}
> -
> -static void disable_translation(u32 *dte)
> -{
> - u32 entry;
> -
> - entry = dte[0];
> - set_field_in_reg_u32(IOMMU_CONTROL_DISABLED, entry,
> - IOMMU_DEV_TABLE_TRANSLATION_VALID_MASK,
> - IOMMU_DEV_TABLE_TRANSLATION_VALID_SHIFT, &entry);
> - set_field_in_reg_u32(IOMMU_CONTROL_DISABLED, entry,
> - IOMMU_DEV_TABLE_VALID_MASK,
> - IOMMU_DEV_TABLE_VALID_SHIFT, &entry);
> - dte[0] = entry;
> -}
> -
> static void amd_iommu_setup_domain_device(
> struct domain *domain, struct amd_iommu *iommu,
> - u8 devfn, struct pci_dev *pdev)
> + uint8_t devfn, struct pci_dev *pdev)
> {
> - void *dte;
> + struct amd_iommu_dte *table, *dte;
> unsigned long flags;
> int req_id, valid = 1;
> int dte_i = 0;
> @@ -131,20 +107,21 @@ static void amd_iommu_setup_domain_device(
>
> /* get device-table entry */
> req_id = get_dma_requestor_id(iommu->seg, PCI_BDF2(bus, devfn));
> - dte = iommu->dev_table.buffer + (req_id * IOMMU_DEV_TABLE_ENTRY_SIZE);
> + table = iommu->dev_table.buffer;
> + dte = &table[req_id];
>
> spin_lock_irqsave(&iommu->lock, flags);
>
> - if ( !is_translation_valid((u32 *)dte) )
> + if ( !dte->v || !dte->tv )
> {
> /* bind DTE to domain page-tables */
> amd_iommu_set_root_page_table(
> - (u32 *)dte, page_to_maddr(hd->arch.root_table), domain->domain_id,
> + dte, page_to_maddr(hd->arch.root_table), domain->domain_id,
> hd->arch.paging_mode, valid);
>
> if ( pci_ats_device(iommu->seg, bus, pdev->devfn) &&
> iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) )
> - iommu_dte_set_iotlb((u32 *)dte, dte_i);
> + dte->i = dte_i;
>
> amd_iommu_flush_device(iommu, req_id);
>
> @@ -272,23 +249,25 @@ void amd_iommu_disable_domain_device(struct domain *domain,
> struct amd_iommu *iommu,
> u8 devfn, struct pci_dev *pdev)
> {
> - void *dte;
> + struct amd_iommu_dte *table, *dte;
> unsigned long flags;
> int req_id;
> u8 bus = pdev->bus;
>
> BUG_ON ( iommu->dev_table.buffer == NULL );
> req_id = get_dma_requestor_id(iommu->seg, PCI_BDF2(bus, devfn));
> - dte = iommu->dev_table.buffer + (req_id * IOMMU_DEV_TABLE_ENTRY_SIZE);
> + table = iommu->dev_table.buffer;
> + dte = &table[req_id];
>
> spin_lock_irqsave(&iommu->lock, flags);
> - if ( is_translation_valid((u32 *)dte) )
> + if ( dte->tv && dte->v )
> {
> - disable_translation((u32 *)dte);
> + dte->tv = 0;
> + dte->v = 0;
>
> if ( pci_ats_device(iommu->seg, bus, pdev->devfn) &&
> iommu_has_cap(iommu, PCI_CAP_IOTLB_SHIFT) )
> - iommu_dte_set_iotlb((u32 *)dte, 0);
> + dte->i = 0;
>
> amd_iommu_flush_device(iommu, req_id);
>
> diff --git a/xen/include/asm-x86/amd-iommu.h b/xen/include/asm-x86/amd-iommu.h
> index 02715b482b..ad8e4a35a2 100644
> --- a/xen/include/asm-x86/amd-iommu.h
> +++ b/xen/include/asm-x86/amd-iommu.h
> @@ -46,11 +46,6 @@ typedef struct cmd_entry
> {
> uint32_t data[4];
> } cmd_entry_t;
> -
> -typedef struct dev_entry
> -{
> - uint32_t data[8];
> -} dev_entry_t;
> #pragma pack()
>
> struct table_struct {
> diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> index a3a49f91eb..40da33b271 100644
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> @@ -107,74 +107,58 @@
> #define IOMMU_DEV_TABLE_INT_CONTROL_FORWARDED 0x1
> #define IOMMU_DEV_TABLE_INT_CONTROL_TRANSLATED 0x2
>
> -/* DeviceTable Entry[31:0] */
> -#define IOMMU_DEV_TABLE_VALID_MASK 0x00000001
> -#define IOMMU_DEV_TABLE_VALID_SHIFT 0
> -#define IOMMU_DEV_TABLE_TRANSLATION_VALID_MASK 0x00000002
> -#define IOMMU_DEV_TABLE_TRANSLATION_VALID_SHIFT 1
> -#define IOMMU_DEV_TABLE_PAGING_MODE_MASK 0x00000E00
> -#define IOMMU_DEV_TABLE_PAGING_MODE_SHIFT 9
> -#define IOMMU_DEV_TABLE_PAGE_TABLE_PTR_LOW_MASK 0xFFFFF000
> -#define IOMMU_DEV_TABLE_PAGE_TABLE_PTR_LOW_SHIFT 12
> -
> -/* DeviceTable Entry[63:32] */
> -#define IOMMU_DEV_TABLE_GV_SHIFT 23
> -#define IOMMU_DEV_TABLE_GV_MASK 0x800000
> -#define IOMMU_DEV_TABLE_GLX_SHIFT 24
> -#define IOMMU_DEV_TABLE_GLX_MASK 0x3000000
> -#define IOMMU_DEV_TABLE_GCR3_1_SHIFT 26
> -#define IOMMU_DEV_TABLE_GCR3_1_MASK 0x1c000000
> -
> -#define IOMMU_DEV_TABLE_PAGE_TABLE_PTR_HIGH_MASK 0x000FFFFF
> -#define IOMMU_DEV_TABLE_PAGE_TABLE_PTR_HIGH_SHIFT 0
> -#define IOMMU_DEV_TABLE_IO_READ_PERMISSION_MASK 0x20000000
> -#define IOMMU_DEV_TABLE_IO_READ_PERMISSION_SHIFT 29
> -#define IOMMU_DEV_TABLE_IO_WRITE_PERMISSION_MASK 0x40000000
> -#define IOMMU_DEV_TABLE_IO_WRITE_PERMISSION_SHIFT 30
> -
> -/* DeviceTable Entry[95:64] */
> -#define IOMMU_DEV_TABLE_DOMAIN_ID_MASK 0x0000FFFF
> -#define IOMMU_DEV_TABLE_DOMAIN_ID_SHIFT 0
> -#define IOMMU_DEV_TABLE_GCR3_2_SHIFT 16
> -#define IOMMU_DEV_TABLE_GCR3_2_MASK 0xFFFF0000
> -
> -/* DeviceTable Entry[127:96] */
> -#define IOMMU_DEV_TABLE_IOTLB_SUPPORT_MASK 0x00000001
> -#define IOMMU_DEV_TABLE_IOTLB_SUPPORT_SHIFT 0
> -#define IOMMU_DEV_TABLE_SUPRESS_LOGGED_PAGES_MASK 0x00000002
> -#define IOMMU_DEV_TABLE_SUPRESS_LOGGED_PAGES_SHIFT 1
> -#define IOMMU_DEV_TABLE_SUPRESS_ALL_PAGES_MASK 0x00000004
> -#define IOMMU_DEV_TABLE_SUPRESS_ALL_PAGES_SHIFT 2
> -#define IOMMU_DEV_TABLE_IO_CONTROL_MASK 0x00000018
> -#define IOMMU_DEV_TABLE_IO_CONTROL_SHIFT 3
> -#define IOMMU_DEV_TABLE_IOTLB_CACHE_HINT_MASK 0x00000020
> -#define IOMMU_DEV_TABLE_IOTLB_CACHE_HINT_SHIFT 5
> -#define IOMMU_DEV_TABLE_SNOOP_DISABLE_MASK 0x00000040
> -#define IOMMU_DEV_TABLE_SNOOP_DISABLE_SHIFT 6
> -#define IOMMU_DEV_TABLE_ALLOW_EXCLUSION_MASK 0x00000080
> -#define IOMMU_DEV_TABLE_ALLOW_EXCLUSION_SHIFT 7
> -#define IOMMU_DEV_TABLE_SYS_MGT_MSG_ENABLE_MASK 0x00000300
> -#define IOMMU_DEV_TABLE_SYS_MGT_MSG_ENABLE_SHIFT 8
> -
> -/* DeviceTable Entry[159:128] */
> -#define IOMMU_DEV_TABLE_INT_VALID_MASK 0x00000001
> -#define IOMMU_DEV_TABLE_INT_VALID_SHIFT 0
> -#define IOMMU_DEV_TABLE_INT_TABLE_LENGTH_MASK 0x0000001E
> -#define IOMMU_DEV_TABLE_INT_TABLE_LENGTH_SHIFT 1
> -#define IOMMU_DEV_TABLE_INT_TABLE_IGN_UNMAPPED_MASK 0x0000000020
> -#define IOMMU_DEV_TABLE_INT_TABLE_IGN_UNMAPPED_SHIFT 5
> -#define IOMMU_DEV_TABLE_INT_TABLE_PTR_LOW_MASK 0xFFFFFFC0
> -#define IOMMU_DEV_TABLE_INT_TABLE_PTR_LOW_SHIFT 6
> -#define IOMMU_DEV_TABLE_GCR3_3_SHIFT 11
> -#define IOMMU_DEV_TABLE_GCR3_3_MASK 0xfffff800
> -
> -/* DeviceTable Entry[191:160] */
> -#define IOMMU_DEV_TABLE_INT_TABLE_PTR_HIGH_MASK 0x000FFFFF
> -#define IOMMU_DEV_TABLE_INT_TABLE_PTR_HIGH_SHIFT 0
> -#define IOMMU_DEV_TABLE_IVHD_FLAGS_SHIFT 24
> -#define IOMMU_DEV_TABLE_IVHD_FLAGS_MASK 0xC7000000
> -#define IOMMU_DEV_TABLE_INT_CONTROL_MASK 0x30000000
> -#define IOMMU_DEV_TABLE_INT_CONTROL_SHIFT 28
> +struct amd_iommu_dte {
> + /* 0 - 63 */
> + uint64_t v:1;
> + uint64_t tv:1;
> + uint64_t reserved0:5;
> + uint64_t had:2;
> + uint64_t paging_mode:3;
> + uint64_t pt_root:40;
> + uint64_t ppr:1;
> + uint64_t gprp:1;
> + uint64_t giov:1;
> + uint64_t gv:1;
> + uint64_t glx:2;
> + uint64_t gcr3_trp_14_12:3;
> + uint64_t ir:1;
> + uint64_t iw:1;
> + uint64_t reserved1:1;
> +
> + /* 64 - 127 */
> + uint64_t domain_id:16;
> + uint64_t gcr3_trp_30_15:16;
> + uint64_t i:1;
> + uint64_t se:1;
> + uint64_t sa:1;
> + uint64_t ioctl:2;
> + uint64_t cache:1;
> + uint64_t sd:1;
> + uint64_t ex:1;
> + uint64_t sys_mgt:2;
> + uint64_t reserved2:1;
> + uint64_t gcr3_trp_51_31:21;
> +
> + /* 128 - 191 */
> + uint64_t iv:1;
> + uint64_t int_tab_len:4;
> + uint64_t ig:1;
> + uint64_t it_root:46;
> + uint64_t reserved3:4;
> + uint64_t init_pass:1;
> + uint64_t ext_int_pass:1;
> + uint64_t nmi_pass:1;
> + uint64_t reserved4:1;
> + uint64_t int_ctl:2;
> + uint64_t lint0_pass:1;
> + uint64_t lint1_pass:1;
> +
> + /* 192 - 255 */
> + uint64_t reserved5:54;
> + uint64_t attr_v:1;
> + uint64_t mode0_fc:1;
> + uint64_t snoop_attr:8;
> +};
>
> /* Command Buffer */
> #define IOMMU_CMD_BUFFER_BASE_LOW_OFFSET 0x08
> diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> index 1c1971bb7c..e0d5d23978 100644
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> @@ -70,15 +70,17 @@ int __must_check amd_iommu_flush_iotlb_all(struct domain *d);
> void amd_iommu_share_p2m(struct domain *d);
>
> /* device table functions */
> -int get_dma_requestor_id(u16 seg, u16 bdf);
> -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 iommu_dte_add_device_entry(u32 *dte, struct ivrs_mappings *ivrs_dev);
> -void iommu_dte_set_guest_cr3(u32 *dte, u16 dom_id, u64 gcr3,
> - int gv, unsigned int glx);
> +int get_dma_requestor_id(uint16_t seg, uint16_t bdf);
> +void amd_iommu_set_intremap_table(struct amd_iommu_dte *dte,
> + uint64_t intremap_ptr,
> + uint8_t int_valid);
> +void amd_iommu_set_root_page_table(struct amd_iommu_dte *dte,
> + uint64_t root_ptr, uint16_t domain_id,
> + uint8_t paging_mode, uint8_t valid);
> +void iommu_dte_add_device_entry(struct amd_iommu_dte *dte,
> + struct ivrs_mappings *ivrs_dev);
> +void iommu_dte_set_guest_cr3(struct amd_iommu_dte *dte, uint16_t dom_id,
> + uint64_t gcr3_mfn, uint8_t gv, uint8_t glx);
>
> /* send cmd to iommu */
> void amd_iommu_flush_all_pages(struct domain *d);
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] amd-iommu: use a bitfield for PTE/PDE
2019-02-12 20:14 ` Woods, Brian
@ 2019-02-13 9:45 ` Paul Durrant
2019-02-13 15:33 ` Woods, Brian
0 siblings, 1 reply; 8+ messages in thread
From: Paul Durrant @ 2019-02-13 9:45 UTC (permalink / raw)
To: 'Woods, Brian', xen-devel
Cc: Andrew Cooper, Wei Liu, Jan Beulich, Suthikulpanit, Suravee,
Roger Pau Monne
> -----Original Message-----
> From: Woods, Brian [mailto:Brian.Woods@amd.com]
> Sent: 12 February 2019 20:14
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Suthikulpanit, Suravee <Suravee.Suthikulpanit@amd.com>; Jan Beulich
> <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>
> Subject: Re: [PATCH 1/2] amd-iommu: use a bitfield for PTE/PDE
>
> On 2/4/19 5:19 AM, Paul Durrant wrote:
> > The current use of get/set_field_from/in_reg_u32() is both inefficient
> and
> > requires some ugly casting.
> >
> > This patch defines a new bitfield structure (amd_iommu_pte) and uses
> this
> > structure in all PTE/PDE manipulation, resulting in much more readable
> > and compact code.
> >
> > NOTE: This commit also fixes one malformed comment in
> > set_iommu_pte_present().
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>
> Sorry about the delay.
>
> Nitpick here, but I'd rather have !!IOMMUF_{writable,readable} than
> true.
That's pretty ugly. How about I pass an OR of the flags through to lower level functions rather than a pair of bools? If you're ok with that I'll send a v2.
Paul
> Not worth a revision if there isn't anything else though (and is
> debatable).
>
> Acked-by: Brian Woods <brian.woods@amd.com>
>
> > ---
> > Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> > Cc: Brian Woods <brian.woods@amd.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: "Roger Pau Monné" <roger.pau@citrix.com>
> > ---
> > xen/drivers/passthrough/amd/iommu_map.c | 143 ++++--------------
> > xen/drivers/passthrough/amd/pci_amd_iommu.c | 50 +++---
> > xen/include/asm-x86/hvm/svm/amd-iommu-defs.h | 47 ++----
> > xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 15 --
> > 4 files changed, 64 insertions(+), 191 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/amd/iommu_map.c
> b/xen/drivers/passthrough/amd/iommu_map.c
> > index 67329b0c95..5fda6063df 100644
> > --- a/xen/drivers/passthrough/amd/iommu_map.c
> > +++ b/xen/drivers/passthrough/amd/iommu_map.c
> > @@ -38,100 +38,45 @@ static unsigned int pfn_to_pde_idx(unsigned long
> pfn, unsigned int level)
> > static unsigned int clear_iommu_pte_present(unsigned long l1_mfn,
> > unsigned long dfn)
> > {
> > - uint64_t *table, *pte;
> > + struct amd_iommu_pte *table, *pte;
> > unsigned int flush_flags;
> >
> > table = map_domain_page(_mfn(l1_mfn));
> > + pte = &table[pfn_to_pde_idx(dfn, 1)];
> >
> > - pte = (table + pfn_to_pde_idx(dfn, 1));
> > + flush_flags = pte->pr ? IOMMU_FLUSHF_modified : 0;
> > + memset(pte, 0, sizeof(*pte));
> >
> > - flush_flags = get_field_from_reg_u32(*pte, IOMMU_PTE_PRESENT_MASK,
> > - IOMMU_PTE_PRESENT_SHIFT) ?
> > - IOMMU_FLUSHF_modified : 0;
> > -
> > - *pte = 0;
> > unmap_domain_page(table);
> >
> > return flush_flags;
> > }
> >
> > -static unsigned int set_iommu_pde_present(uint32_t *pde,
> > +static unsigned int set_iommu_pde_present(struct amd_iommu_pte *pte,
> > unsigned long next_mfn,
> > unsigned int next_level,
> bool iw,
> > bool ir)
> > {
> > - uint64_t maddr_next;
> > - uint32_t addr_lo, addr_hi, entry;
> > - bool old_present;
> > unsigned int flush_flags = IOMMU_FLUSHF_added;
> >
> > - maddr_next = __pfn_to_paddr(next_mfn);
> > -
> > - old_present = get_field_from_reg_u32(pde[0],
> IOMMU_PTE_PRESENT_MASK,
> > - IOMMU_PTE_PRESENT_SHIFT);
> > - if ( old_present )
> > - {
> > - bool old_r, old_w;
> > - unsigned int old_level;
> > - uint64_t maddr_old;
> > -
> > - addr_hi = get_field_from_reg_u32(pde[1],
> > - IOMMU_PTE_ADDR_HIGH_MASK,
> > - IOMMU_PTE_ADDR_HIGH_SHIFT);
> > - addr_lo = get_field_from_reg_u32(pde[0],
> > - IOMMU_PTE_ADDR_LOW_MASK,
> > - IOMMU_PTE_ADDR_LOW_SHIFT);
> > - old_level = get_field_from_reg_u32(pde[0],
> > - IOMMU_PDE_NEXT_LEVEL_MASK,
> > - IOMMU_PDE_NEXT_LEVEL_SHIFT);
> > - old_w = get_field_from_reg_u32(pde[1],
> > -
> IOMMU_PTE_IO_WRITE_PERMISSION_MASK,
> > -
> IOMMU_PTE_IO_WRITE_PERMISSION_SHIFT);
> > - old_r = get_field_from_reg_u32(pde[1],
> > -
> IOMMU_PTE_IO_READ_PERMISSION_MASK,
> > -
> IOMMU_PTE_IO_READ_PERMISSION_SHIFT);
> > -
> > - maddr_old = ((uint64_t)addr_hi << 32) |
> > - ((uint64_t)addr_lo << PAGE_SHIFT);
> > -
> > - if ( maddr_old != maddr_next || iw != old_w || ir != old_r ||
> > - old_level != next_level )
> > + if ( pte->pr &&
> > + (pte->mfn != next_mfn ||
> > + pte->iw != iw ||
> > + pte->ir != ir ||
> > + pte->next_level != next_level) )
> > flush_flags |= IOMMU_FLUSHF_modified;
> > - }
> >
> > - addr_lo = maddr_next & DMA_32BIT_MASK;
> > - addr_hi = maddr_next >> 32;
> > -
> > - /* enable read/write permissions,which will be enforced at the PTE
> */
> > - set_field_in_reg_u32(addr_hi, 0,
> > - IOMMU_PDE_ADDR_HIGH_MASK,
> > - IOMMU_PDE_ADDR_HIGH_SHIFT, &entry);
> > - set_field_in_reg_u32(iw, entry,
> > - IOMMU_PDE_IO_WRITE_PERMISSION_MASK,
> > - IOMMU_PDE_IO_WRITE_PERMISSION_SHIFT, &entry);
> > - set_field_in_reg_u32(ir, entry,
> > - IOMMU_PDE_IO_READ_PERMISSION_MASK,
> > - IOMMU_PDE_IO_READ_PERMISSION_SHIFT, &entry);
> > -
> > - /* FC bit should be enabled in PTE, this helps to solve potential
> > + /*
> > + * FC bit should be enabled in PTE, this helps to solve potential
> > * issues with ATS devices
> > */
> > - if ( next_level == 0 )
> > - set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, entry,
> > - IOMMU_PTE_FC_MASK, IOMMU_PTE_FC_SHIFT,
> &entry);
> > - pde[1] = entry;
>
>
> > + pte->fc = !next_level;
> >
> > - /* mark next level as 'present' */
> > - set_field_in_reg_u32(addr_lo >> PAGE_SHIFT, 0,
> > - IOMMU_PDE_ADDR_LOW_MASK,
> > - IOMMU_PDE_ADDR_LOW_SHIFT, &entry);
> > - set_field_in_reg_u32(next_level, entry,
> > - IOMMU_PDE_NEXT_LEVEL_MASK,
> > - IOMMU_PDE_NEXT_LEVEL_SHIFT, &entry);
> > - set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, entry,
> > - IOMMU_PDE_PRESENT_MASK,
> > - IOMMU_PDE_PRESENT_SHIFT, &entry);
> > - pde[0] = entry;
> > + pte->mfn = next_mfn;
> > + pte->iw = iw;
> > + pte->ir = ir;
> > + pte->next_level = next_level;
> > + pte->pr = 1;
> >
> > return flush_flags;
> > }
> > @@ -142,13 +87,11 @@ static unsigned int set_iommu_pte_present(unsigned
> long pt_mfn,
> > int pde_level,
> > bool iw, bool ir)
> > {
> > - uint64_t *table;
> > - uint32_t *pde;
> > + struct amd_iommu_pte *table, *pde;
> > unsigned int flush_flags;
> >
> > table = map_domain_page(_mfn(pt_mfn));
> > -
> > - pde = (uint32_t *)(table + pfn_to_pde_idx(dfn, pde_level));
> > + pde = &table[pfn_to_pde_idx(dfn, pde_level)];
> >
> > flush_flags = set_iommu_pde_present(pde, next_mfn, 0, iw, ir);
> > unmap_domain_page(table);
> > @@ -319,25 +262,6 @@ void iommu_dte_set_guest_cr3(uint32_t *dte,
> uint16_t dom_id, uint64_t gcr3,
> > dte[1] = entry;
> > }
> >
> > -uint64_t amd_iommu_get_address_from_pte(void *pte)
> > -{
> > - uint32_t *entry = pte;
> > - uint32_t addr_lo, addr_hi;
> > - uint64_t ptr;
> > -
> > - addr_lo = get_field_from_reg_u32(entry[0],
> > - IOMMU_PTE_ADDR_LOW_MASK,
> > - IOMMU_PTE_ADDR_LOW_SHIFT);
> > -
> > - addr_hi = get_field_from_reg_u32(entry[1],
> > - IOMMU_PTE_ADDR_HIGH_MASK,
> > - IOMMU_PTE_ADDR_HIGH_SHIFT);
> > -
> > - ptr = ((uint64_t)addr_hi << 32) |
> > - ((uint64_t)addr_lo << PAGE_SHIFT);
> > - return ptr;
> > -}
> > -
> > /* Walk io page tables and build level page tables if necessary
> > * {Re, un}mapping super page frames causes re-allocation of io
> > * page tables.
> > @@ -345,7 +269,7 @@ uint64_t amd_iommu_get_address_from_pte(void *pte)
> > static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
> > unsigned long pt_mfn[])
> > {
> > - uint64_t *pde, *next_table_vaddr;
> > + struct amd_iommu_pte *pde, *next_table_vaddr;
> > unsigned long next_table_mfn;
> > unsigned int level;
> > struct page_info *table;
> > @@ -370,15 +294,13 @@ static int iommu_pde_from_dfn(struct domain *d,
> unsigned long dfn,
> > pt_mfn[level] = next_table_mfn;
> >
> > next_table_vaddr = map_domain_page(_mfn(next_table_mfn));
> > - pde = next_table_vaddr + pfn_to_pde_idx(dfn, level);
> > + pde = &next_table_vaddr[pfn_to_pde_idx(dfn, level)];
> >
> > /* Here might be a super page frame */
> > - next_table_mfn = amd_iommu_get_address_from_pte(pde) >>
> PAGE_SHIFT;
> > + next_table_mfn = pde->mfn;
> >
> > /* Split super page frame into smaller pieces.*/
> > - if ( iommu_is_pte_present((uint32_t *)pde) &&
> > - (iommu_next_level((uint32_t *)pde) == 0) &&
> > - next_table_mfn != 0 )
> > + if ( pde->pr && !pde->next_level && next_table_mfn )
> > {
> > int i;
> > unsigned long mfn, pfn;
> > @@ -398,13 +320,13 @@ static int iommu_pde_from_dfn(struct domain *d,
> unsigned long dfn,
> > }
> >
> > next_table_mfn = mfn_x(page_to_mfn(table));
> > - set_iommu_pde_present((uint32_t *)pde, next_table_mfn,
> next_level,
> > - !!IOMMUF_writable,
> !!IOMMUF_readable);
> > + set_iommu_pde_present(pde, next_table_mfn, next_level,
> true,
> > + true);
> >
> > for ( i = 0; i < PTE_PER_TABLE_SIZE; i++ )
> > {
> > set_iommu_pte_present(next_table_mfn, pfn, mfn,
> next_level,
> > - !!IOMMUF_writable,
> !!IOMMUF_readable);
> > + true, true);
> > mfn += page_sz;
> > pfn += page_sz;
> > }
> > @@ -413,7 +335,7 @@ static int iommu_pde_from_dfn(struct domain *d,
> unsigned long dfn,
> > }
> >
> > /* Install lower level page table for non-present entries */
> > - else if ( !iommu_is_pte_present((uint32_t *)pde) )
> > + else if ( !pde->pr )
> > {
> > if ( next_table_mfn == 0 )
> > {
> > @@ -425,9 +347,8 @@ static int iommu_pde_from_dfn(struct domain *d,
> unsigned long dfn,
> > return 1;
> > }
> > next_table_mfn = mfn_x(page_to_mfn(table));
> > - set_iommu_pde_present((uint32_t *)pde, next_table_mfn,
> > - next_level, !!IOMMUF_writable,
> > - !!IOMMUF_readable);
> > + set_iommu_pde_present(pde, next_table_mfn, next_level,
> true,
> > + true);
> > }
> > else /* should never reach here */
> > {
> > @@ -455,7 +376,7 @@ static int update_paging_mode(struct domain *d,
> unsigned long dfn)
> > struct amd_iommu *iommu = NULL;
> > struct page_info *new_root = NULL;
> > struct page_info *old_root = NULL;
> > - void *new_root_vaddr;
> > + struct amd_iommu_pte *new_root_vaddr;
> > unsigned long old_root_mfn;
> > struct domain_iommu *hd = dom_iommu(d);
> >
> > @@ -484,7 +405,7 @@ static int update_paging_mode(struct domain *d,
> unsigned long dfn)
> > new_root_vaddr = __map_domain_page(new_root);
> > old_root_mfn = mfn_x(page_to_mfn(old_root));
> > set_iommu_pde_present(new_root_vaddr, old_root_mfn, level,
> > - !!IOMMUF_writable, !!IOMMUF_readable);
> > + true, true);
> > level++;
> > old_root = new_root;
> > offset >>= PTE_PER_TABLE_SHIFT;
> > diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> > index 33a3798f36..da6748320b 100644
> > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> > @@ -376,9 +376,8 @@ static void deallocate_next_page_table(struct
> page_info *pg, int level)
> >
> > static void deallocate_page_table(struct page_info *pg)
> > {
> > - void *table_vaddr, *pde;
> > - u64 next_table_maddr;
> > - unsigned int index, level = PFN_ORDER(pg), next_level;
> > + struct amd_iommu_pte *table_vaddr;
> > + unsigned int index, level = PFN_ORDER(pg);
> >
> > PFN_ORDER(pg) = 0;
> >
> > @@ -392,17 +391,14 @@ static void deallocate_page_table(struct page_info
> *pg)
> >
> > for ( index = 0; index < PTE_PER_TABLE_SIZE; index++ )
> > {
> > - pde = table_vaddr + (index * IOMMU_PAGE_TABLE_ENTRY_SIZE);
> > - next_table_maddr = amd_iommu_get_address_from_pte(pde);
> > - next_level = iommu_next_level(pde);
> > + struct amd_iommu_pte *pde = &table_vaddr[index];
> >
> > - if ( (next_table_maddr != 0) && (next_level != 0) &&
> > - iommu_is_pte_present(pde) )
> > + if ( pde->mfn && pde->next_level && pde->pr )
> > {
> > /* We do not support skip levels yet */
> > - ASSERT(next_level == level - 1);
> > - deallocate_next_page_table(maddr_to_page(next_table_maddr),
> > - next_level);
> > + ASSERT(pde->next_level == level - 1);
> > + deallocate_next_page_table(mfn_to_page(_mfn(pde->mfn)),
> > + pde->next_level);
> > }
> > }
> >
> > @@ -500,10 +496,8 @@ static void amd_dump_p2m_table_level(struct
> page_info* pg, int level,
> > paddr_t gpa, int indent)
> > {
> > paddr_t address;
> > - void *table_vaddr, *pde;
> > - paddr_t next_table_maddr;
> > - int index, next_level, present;
> > - u32 *entry;
> > + struct amd_iommu_pte *table_vaddr;
> > + int index;
> >
> > if ( level < 1 )
> > return;
> > @@ -518,42 +512,32 @@ static void amd_dump_p2m_table_level(struct
> page_info* pg, int level,
> >
> > for ( index = 0; index < PTE_PER_TABLE_SIZE; index++ )
> > {
> > + struct amd_iommu_pte *pde = &table_vaddr[index];
> > +
> > if ( !(index % 2) )
> > process_pending_softirqs();
> >
> > - pde = table_vaddr + (index * IOMMU_PAGE_TABLE_ENTRY_SIZE);
> > - next_table_maddr = amd_iommu_get_address_from_pte(pde);
> > - entry = pde;
> > -
> > - present = get_field_from_reg_u32(entry[0],
> > - IOMMU_PDE_PRESENT_MASK,
> > - IOMMU_PDE_PRESENT_SHIFT);
> > -
> > - if ( !present )
> > + if ( !pde->pr )
> > continue;
> >
> > - next_level = get_field_from_reg_u32(entry[0],
> > - IOMMU_PDE_NEXT_LEVEL_MASK,
> > -
> IOMMU_PDE_NEXT_LEVEL_SHIFT);
> > -
> > - if ( next_level && (next_level != (level - 1)) )
> > + if ( pde->next_level && (pde->next_level != (level - 1)) )
> > {
> > printk("IOMMU p2m table error. next_level = %d, expected
> %d\n",
> > - next_level, level - 1);
> > + pde->next_level, level - 1);
> >
> > continue;
> > }
> >
> > address = gpa + amd_offset_level_address(index, level);
> > - if ( next_level >= 1 )
> > + if ( pde->next_level >= 1 )
> > amd_dump_p2m_table_level(
> > - maddr_to_page(next_table_maddr), next_level,
> > + mfn_to_page(_mfn(pde->mfn)), pde->next_level,
> > address, indent + 1);
> > else
> > printk("%*sdfn: %08lx mfn: %08lx\n",
> > indent, "",
> > (unsigned long)PFN_DOWN(address),
> > - (unsigned long)PFN_DOWN(next_table_maddr));
> > + (unsigned long)PFN_DOWN(pfn_to_paddr(pde->mfn)));
> > }
> >
> > unmap_domain_page(table_vaddr);
> > diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> > index a217245249..a3a49f91eb 100644
> > --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> > +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
> > @@ -413,38 +413,21 @@
> > #define IOMMU_PAGE_TABLE_U32_PER_ENTRY
> (IOMMU_PAGE_TABLE_ENTRY_SIZE / 4)
> > #define IOMMU_PAGE_TABLE_ALIGNMENT 4096
> >
> > -#define IOMMU_PTE_PRESENT_MASK 0x00000001
> > -#define IOMMU_PTE_PRESENT_SHIFT 0
> > -#define IOMMU_PTE_NEXT_LEVEL_MASK 0x00000E00
> > -#define IOMMU_PTE_NEXT_LEVEL_SHIFT 9
> > -#define IOMMU_PTE_ADDR_LOW_MASK 0xFFFFF000
> > -#define IOMMU_PTE_ADDR_LOW_SHIFT 12
> > -#define IOMMU_PTE_ADDR_HIGH_MASK 0x000FFFFF
> > -#define IOMMU_PTE_ADDR_HIGH_SHIFT 0
> > -#define IOMMU_PTE_U_MASK 0x08000000
> > -#define IOMMU_PTE_U_SHIFT 7
> > -#define IOMMU_PTE_FC_MASK 0x10000000
> > -#define IOMMU_PTE_FC_SHIFT 28
> > -#define IOMMU_PTE_IO_READ_PERMISSION_MASK 0x20000000
> > -#define IOMMU_PTE_IO_READ_PERMISSION_SHIFT 29
> > -#define IOMMU_PTE_IO_WRITE_PERMISSION_MASK 0x40000000
> > -#define IOMMU_PTE_IO_WRITE_PERMISSION_SHIFT 30
> > -
> > -/* I/O Page Directory */
> > -#define IOMMU_PAGE_DIRECTORY_ENTRY_SIZE 8
> > -#define IOMMU_PAGE_DIRECTORY_ALIGNMENT 4096
> > -#define IOMMU_PDE_PRESENT_MASK 0x00000001
> > -#define IOMMU_PDE_PRESENT_SHIFT 0
> > -#define IOMMU_PDE_NEXT_LEVEL_MASK 0x00000E00
> > -#define IOMMU_PDE_NEXT_LEVEL_SHIFT 9
> > -#define IOMMU_PDE_ADDR_LOW_MASK 0xFFFFF000
> > -#define IOMMU_PDE_ADDR_LOW_SHIFT 12
> > -#define IOMMU_PDE_ADDR_HIGH_MASK 0x000FFFFF
> > -#define IOMMU_PDE_ADDR_HIGH_SHIFT 0
> > -#define IOMMU_PDE_IO_READ_PERMISSION_MASK 0x20000000
> > -#define IOMMU_PDE_IO_READ_PERMISSION_SHIFT 29
> > -#define IOMMU_PDE_IO_WRITE_PERMISSION_MASK 0x40000000
> > -#define IOMMU_PDE_IO_WRITE_PERMISSION_SHIFT 30
> > +struct amd_iommu_pte {
> > + uint64_t pr:1;
> > + uint64_t ignored0:4;
> > + uint64_t a:1;
> > + uint64_t d:1;
> > + uint64_t ignored1:2;
> > + uint64_t next_level:3;
> > + uint64_t mfn:40;
> > + uint64_t reserved:7;
> > + uint64_t u:1;
> > + uint64_t fc:1;
> > + uint64_t ir:1;
> > + uint64_t iw:1;
> > + uint64_t ignored2:1;
> > +};
> >
> > /* Paging modes */
> > #define IOMMU_PAGING_MODE_DISABLED 0x0
> > diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> > index c5697565d6..1c1971bb7c 100644
> > --- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> > +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> > @@ -57,7 +57,6 @@ int __must_check amd_iommu_map_page(struct domain *d,
> dfn_t dfn,
> > unsigned int *flush_flags);
> > int __must_check amd_iommu_unmap_page(struct domain *d, dfn_t dfn,
> > unsigned int *flush_flags);
> > -uint64_t amd_iommu_get_address_from_pte(void *entry);
> > int __must_check amd_iommu_alloc_root(struct domain_iommu *hd);
> > int amd_iommu_reserve_domain_unity_map(struct domain *domain,
> > paddr_t phys_addr, unsigned
> long size,
> > @@ -280,18 +279,4 @@ static inline void
> iommu_set_addr_hi_to_reg(uint32_t *reg, uint32_t addr)
> > IOMMU_REG_BASE_ADDR_HIGH_SHIFT, reg);
> > }
> >
> > -static inline int iommu_is_pte_present(const u32 *entry)
> > -{
> > - return get_field_from_reg_u32(entry[0],
> > - IOMMU_PDE_PRESENT_MASK,
> > - IOMMU_PDE_PRESENT_SHIFT);
> > -}
> > -
> > -static inline unsigned int iommu_next_level(const u32 *entry)
> > -{
> > - return get_field_from_reg_u32(entry[0],
> > - IOMMU_PDE_NEXT_LEVEL_MASK,
> > - IOMMU_PDE_NEXT_LEVEL_SHIFT);
> > -}
> > -
> > #endif /* _ASM_X86_64_AMD_IOMMU_PROTO_H */
> >
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] amd-iommu: use a bitfield for PTE/PDE
2019-02-13 9:45 ` Paul Durrant
@ 2019-02-13 15:33 ` Woods, Brian
2019-02-13 15:50 ` Paul Durrant
0 siblings, 1 reply; 8+ messages in thread
From: Woods, Brian @ 2019-02-13 15:33 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Andrew Cooper, Wei Liu, Jan Beulich, Suthikulpanit, Suravee,
Roger Pau Monne
On 2/13/19 3:45 AM, Paul Durrant wrote:
>> -----Original Message-----
>> From: Woods, Brian [mailto:Brian.Woods@amd.com]
>> Sent: 12 February 2019 20:14
>> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
>> Cc: Suthikulpanit, Suravee <Suravee.Suthikulpanit@amd.com>; Jan Beulich
>> <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
>> <wei.liu2@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>
>> Subject: Re: [PATCH 1/2] amd-iommu: use a bitfield for PTE/PDE
>>
>> On 2/4/19 5:19 AM, Paul Durrant wrote:
>>> The current use of get/set_field_from/in_reg_u32() is both inefficient
>> and
>>> requires some ugly casting.
>>>
>>> This patch defines a new bitfield structure (amd_iommu_pte) and uses
>> this
>>> structure in all PTE/PDE manipulation, resulting in much more readable
>>> and compact code.
>>>
>>> NOTE: This commit also fixes one malformed comment in
>>> set_iommu_pte_present().
>>>
>>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>>
>> Sorry about the delay.
>>
>> Nitpick here, but I'd rather have !!IOMMUF_{writable,readable} than
>> true.
>
> That's pretty ugly. How about I pass an OR of the flags through to lower level functions rather than a pair of bools? If you're ok with that I'll send a v2.
>
> Paul
>
There's no need for a v2 based on that, that's just me nitpicking.
There's no real nice way to do it without turning
IOMMUF_{writable,readable} into bools or your suggested way which has
more code to decode a flag. Assuming everyone else is ok with the
patches as are, it's fine. If there's going to be a v2 for other
reasons, I'll just leave it up to your discretion (other people may have
stronger opinions about it anyway).
Brian
>> Not worth a revision if there isn't anything else though (and is
>> debatable).
>>
>> Acked-by: Brian Woods <brian.woods@amd.com>
>>
>>> ---
>>> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>> Cc: Brian Woods <brian.woods@amd.com>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Cc: Wei Liu <wei.liu2@citrix.com>
>>> Cc: "Roger Pau Monné" <roger.pau@citrix.com>
>>> ---
>>> xen/drivers/passthrough/amd/iommu_map.c | 143 ++++--------------
>>> xen/drivers/passthrough/amd/pci_amd_iommu.c | 50 +++---
>>> xen/include/asm-x86/hvm/svm/amd-iommu-defs.h | 47 ++----
>>> xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 15 --
>>> 4 files changed, 64 insertions(+), 191 deletions(-)
>>>
>>> diff --git a/xen/drivers/passthrough/amd/iommu_map.c
>> b/xen/drivers/passthrough/amd/iommu_map.c
>>> index 67329b0c95..5fda6063df 100644
>>> --- a/xen/drivers/passthrough/amd/iommu_map.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_map.c
>>> @@ -38,100 +38,45 @@ static unsigned int pfn_to_pde_idx(unsigned long
>> pfn, unsigned int level)
>>> static unsigned int clear_iommu_pte_present(unsigned long l1_mfn,
>>> unsigned long dfn)
>>> {
>>> - uint64_t *table, *pte;
>>> + struct amd_iommu_pte *table, *pte;
>>> unsigned int flush_flags;
>>>
>>> table = map_domain_page(_mfn(l1_mfn));
>>> + pte = &table[pfn_to_pde_idx(dfn, 1)];
>>>
>>> - pte = (table + pfn_to_pde_idx(dfn, 1));
>>> + flush_flags = pte->pr ? IOMMU_FLUSHF_modified : 0;
>>> + memset(pte, 0, sizeof(*pte));
>>>
>>> - flush_flags = get_field_from_reg_u32(*pte, IOMMU_PTE_PRESENT_MASK,
>>> - IOMMU_PTE_PRESENT_SHIFT) ?
>>> - IOMMU_FLUSHF_modified : 0;
>>> -
>>> - *pte = 0;
>>> unmap_domain_page(table);
>>>
>>> return flush_flags;
>>> }
>>>
>>> -static unsigned int set_iommu_pde_present(uint32_t *pde,
>>> +static unsigned int set_iommu_pde_present(struct amd_iommu_pte *pte,
>>> unsigned long next_mfn,
>>> unsigned int next_level,
>> bool iw,
>>> bool ir)
>>> {
>>> - uint64_t maddr_next;
>>> - uint32_t addr_lo, addr_hi, entry;
>>> - bool old_present;
>>> unsigned int flush_flags = IOMMU_FLUSHF_added;
>>>
>>> - maddr_next = __pfn_to_paddr(next_mfn);
>>> -
>>> - old_present = get_field_from_reg_u32(pde[0],
>> IOMMU_PTE_PRESENT_MASK,
>>> - IOMMU_PTE_PRESENT_SHIFT);
>>> - if ( old_present )
>>> - {
>>> - bool old_r, old_w;
>>> - unsigned int old_level;
>>> - uint64_t maddr_old;
>>> -
>>> - addr_hi = get_field_from_reg_u32(pde[1],
>>> - IOMMU_PTE_ADDR_HIGH_MASK,
>>> - IOMMU_PTE_ADDR_HIGH_SHIFT);
>>> - addr_lo = get_field_from_reg_u32(pde[0],
>>> - IOMMU_PTE_ADDR_LOW_MASK,
>>> - IOMMU_PTE_ADDR_LOW_SHIFT);
>>> - old_level = get_field_from_reg_u32(pde[0],
>>> - IOMMU_PDE_NEXT_LEVEL_MASK,
>>> - IOMMU_PDE_NEXT_LEVEL_SHIFT);
>>> - old_w = get_field_from_reg_u32(pde[1],
>>> -
>> IOMMU_PTE_IO_WRITE_PERMISSION_MASK,
>>> -
>> IOMMU_PTE_IO_WRITE_PERMISSION_SHIFT);
>>> - old_r = get_field_from_reg_u32(pde[1],
>>> -
>> IOMMU_PTE_IO_READ_PERMISSION_MASK,
>>> -
>> IOMMU_PTE_IO_READ_PERMISSION_SHIFT);
>>> -
>>> - maddr_old = ((uint64_t)addr_hi << 32) |
>>> - ((uint64_t)addr_lo << PAGE_SHIFT);
>>> -
>>> - if ( maddr_old != maddr_next || iw != old_w || ir != old_r ||
>>> - old_level != next_level )
>>> + if ( pte->pr &&
>>> + (pte->mfn != next_mfn ||
>>> + pte->iw != iw ||
>>> + pte->ir != ir ||
>>> + pte->next_level != next_level) )
>>> flush_flags |= IOMMU_FLUSHF_modified;
>>> - }
>>>
>>> - addr_lo = maddr_next & DMA_32BIT_MASK;
>>> - addr_hi = maddr_next >> 32;
>>> -
>>> - /* enable read/write permissions,which will be enforced at the PTE
>> */
>>> - set_field_in_reg_u32(addr_hi, 0,
>>> - IOMMU_PDE_ADDR_HIGH_MASK,
>>> - IOMMU_PDE_ADDR_HIGH_SHIFT, &entry);
>>> - set_field_in_reg_u32(iw, entry,
>>> - IOMMU_PDE_IO_WRITE_PERMISSION_MASK,
>>> - IOMMU_PDE_IO_WRITE_PERMISSION_SHIFT, &entry);
>>> - set_field_in_reg_u32(ir, entry,
>>> - IOMMU_PDE_IO_READ_PERMISSION_MASK,
>>> - IOMMU_PDE_IO_READ_PERMISSION_SHIFT, &entry);
>>> -
>>> - /* FC bit should be enabled in PTE, this helps to solve potential
>>> + /*
>>> + * FC bit should be enabled in PTE, this helps to solve potential
>>> * issues with ATS devices
>>> */
>>> - if ( next_level == 0 )
>>> - set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, entry,
>>> - IOMMU_PTE_FC_MASK, IOMMU_PTE_FC_SHIFT,
>> &entry);
>>> - pde[1] = entry;
>>
>>
>>> + pte->fc = !next_level;
>>>
>>> - /* mark next level as 'present' */
>>> - set_field_in_reg_u32(addr_lo >> PAGE_SHIFT, 0,
>>> - IOMMU_PDE_ADDR_LOW_MASK,
>>> - IOMMU_PDE_ADDR_LOW_SHIFT, &entry);
>>> - set_field_in_reg_u32(next_level, entry,
>>> - IOMMU_PDE_NEXT_LEVEL_MASK,
>>> - IOMMU_PDE_NEXT_LEVEL_SHIFT, &entry);
>>> - set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, entry,
>>> - IOMMU_PDE_PRESENT_MASK,
>>> - IOMMU_PDE_PRESENT_SHIFT, &entry);
>>> - pde[0] = entry;
>>> + pte->mfn = next_mfn;
>>> + pte->iw = iw;
>>> + pte->ir = ir;
>>> + pte->next_level = next_level;
>>> + pte->pr = 1;
>>>
>>> return flush_flags;
>>> }
>>> @@ -142,13 +87,11 @@ static unsigned int set_iommu_pte_present(unsigned
>> long pt_mfn,
>>> int pde_level,
>>> bool iw, bool ir)
>>> {
>>> - uint64_t *table;
>>> - uint32_t *pde;
>>> + struct amd_iommu_pte *table, *pde;
>>> unsigned int flush_flags;
>>>
>>> table = map_domain_page(_mfn(pt_mfn));
>>> -
>>> - pde = (uint32_t *)(table + pfn_to_pde_idx(dfn, pde_level));
>>> + pde = &table[pfn_to_pde_idx(dfn, pde_level)];
>>>
>>> flush_flags = set_iommu_pde_present(pde, next_mfn, 0, iw, ir);
>>> unmap_domain_page(table);
>>> @@ -319,25 +262,6 @@ void iommu_dte_set_guest_cr3(uint32_t *dte,
>> uint16_t dom_id, uint64_t gcr3,
>>> dte[1] = entry;
>>> }
>>>
>>> -uint64_t amd_iommu_get_address_from_pte(void *pte)
>>> -{
>>> - uint32_t *entry = pte;
>>> - uint32_t addr_lo, addr_hi;
>>> - uint64_t ptr;
>>> -
>>> - addr_lo = get_field_from_reg_u32(entry[0],
>>> - IOMMU_PTE_ADDR_LOW_MASK,
>>> - IOMMU_PTE_ADDR_LOW_SHIFT);
>>> -
>>> - addr_hi = get_field_from_reg_u32(entry[1],
>>> - IOMMU_PTE_ADDR_HIGH_MASK,
>>> - IOMMU_PTE_ADDR_HIGH_SHIFT);
>>> -
>>> - ptr = ((uint64_t)addr_hi << 32) |
>>> - ((uint64_t)addr_lo << PAGE_SHIFT);
>>> - return ptr;
>>> -}
>>> -
>>> /* Walk io page tables and build level page tables if necessary
>>> * {Re, un}mapping super page frames causes re-allocation of io
>>> * page tables.
>>> @@ -345,7 +269,7 @@ uint64_t amd_iommu_get_address_from_pte(void *pte)
>>> static int iommu_pde_from_dfn(struct domain *d, unsigned long dfn,
>>> unsigned long pt_mfn[])
>>> {
>>> - uint64_t *pde, *next_table_vaddr;
>>> + struct amd_iommu_pte *pde, *next_table_vaddr;
>>> unsigned long next_table_mfn;
>>> unsigned int level;
>>> struct page_info *table;
>>> @@ -370,15 +294,13 @@ static int iommu_pde_from_dfn(struct domain *d,
>> unsigned long dfn,
>>> pt_mfn[level] = next_table_mfn;
>>>
>>> next_table_vaddr = map_domain_page(_mfn(next_table_mfn));
>>> - pde = next_table_vaddr + pfn_to_pde_idx(dfn, level);
>>> + pde = &next_table_vaddr[pfn_to_pde_idx(dfn, level)];
>>>
>>> /* Here might be a super page frame */
>>> - next_table_mfn = amd_iommu_get_address_from_pte(pde) >>
>> PAGE_SHIFT;
>>> + next_table_mfn = pde->mfn;
>>>
>>> /* Split super page frame into smaller pieces.*/
>>> - if ( iommu_is_pte_present((uint32_t *)pde) &&
>>> - (iommu_next_level((uint32_t *)pde) == 0) &&
>>> - next_table_mfn != 0 )
>>> + if ( pde->pr && !pde->next_level && next_table_mfn )
>>> {
>>> int i;
>>> unsigned long mfn, pfn;
>>> @@ -398,13 +320,13 @@ static int iommu_pde_from_dfn(struct domain *d,
>> unsigned long dfn,
>>> }
>>>
>>> next_table_mfn = mfn_x(page_to_mfn(table));
>>> - set_iommu_pde_present((uint32_t *)pde, next_table_mfn,
>> next_level,
>>> - !!IOMMUF_writable,
>> !!IOMMUF_readable);
>>> + set_iommu_pde_present(pde, next_table_mfn, next_level,
>> true,
>>> + true);
>>>
>>> for ( i = 0; i < PTE_PER_TABLE_SIZE; i++ )
>>> {
>>> set_iommu_pte_present(next_table_mfn, pfn, mfn,
>> next_level,
>>> - !!IOMMUF_writable,
>> !!IOMMUF_readable);
>>> + true, true);
>>> mfn += page_sz;
>>> pfn += page_sz;
>>> }
>>> @@ -413,7 +335,7 @@ static int iommu_pde_from_dfn(struct domain *d,
>> unsigned long dfn,
>>> }
>>>
>>> /* Install lower level page table for non-present entries */
>>> - else if ( !iommu_is_pte_present((uint32_t *)pde) )
>>> + else if ( !pde->pr )
>>> {
>>> if ( next_table_mfn == 0 )
>>> {
>>> @@ -425,9 +347,8 @@ static int iommu_pde_from_dfn(struct domain *d,
>> unsigned long dfn,
>>> return 1;
>>> }
>>> next_table_mfn = mfn_x(page_to_mfn(table));
>>> - set_iommu_pde_present((uint32_t *)pde, next_table_mfn,
>>> - next_level, !!IOMMUF_writable,
>>> - !!IOMMUF_readable);
>>> + set_iommu_pde_present(pde, next_table_mfn, next_level,
>> true,
>>> + true);
>>> }
>>> else /* should never reach here */
>>> {
>>> @@ -455,7 +376,7 @@ static int update_paging_mode(struct domain *d,
>> unsigned long dfn)
>>> struct amd_iommu *iommu = NULL;
>>> struct page_info *new_root = NULL;
>>> struct page_info *old_root = NULL;
>>> - void *new_root_vaddr;
>>> + struct amd_iommu_pte *new_root_vaddr;
>>> unsigned long old_root_mfn;
>>> struct domain_iommu *hd = dom_iommu(d);
>>>
>>> @@ -484,7 +405,7 @@ static int update_paging_mode(struct domain *d,
>> unsigned long dfn)
>>> new_root_vaddr = __map_domain_page(new_root);
>>> old_root_mfn = mfn_x(page_to_mfn(old_root));
>>> set_iommu_pde_present(new_root_vaddr, old_root_mfn, level,
>>> - !!IOMMUF_writable, !!IOMMUF_readable);
>>> + true, true);
>>> level++;
>>> old_root = new_root;
>>> offset >>= PTE_PER_TABLE_SHIFT;
>>> diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>> index 33a3798f36..da6748320b 100644
>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>> @@ -376,9 +376,8 @@ static void deallocate_next_page_table(struct
>> page_info *pg, int level)
>>>
>>> static void deallocate_page_table(struct page_info *pg)
>>> {
>>> - void *table_vaddr, *pde;
>>> - u64 next_table_maddr;
>>> - unsigned int index, level = PFN_ORDER(pg), next_level;
>>> + struct amd_iommu_pte *table_vaddr;
>>> + unsigned int index, level = PFN_ORDER(pg);
>>>
>>> PFN_ORDER(pg) = 0;
>>>
>>> @@ -392,17 +391,14 @@ static void deallocate_page_table(struct page_info
>> *pg)
>>>
>>> for ( index = 0; index < PTE_PER_TABLE_SIZE; index++ )
>>> {
>>> - pde = table_vaddr + (index * IOMMU_PAGE_TABLE_ENTRY_SIZE);
>>> - next_table_maddr = amd_iommu_get_address_from_pte(pde);
>>> - next_level = iommu_next_level(pde);
>>> + struct amd_iommu_pte *pde = &table_vaddr[index];
>>>
>>> - if ( (next_table_maddr != 0) && (next_level != 0) &&
>>> - iommu_is_pte_present(pde) )
>>> + if ( pde->mfn && pde->next_level && pde->pr )
>>> {
>>> /* We do not support skip levels yet */
>>> - ASSERT(next_level == level - 1);
>>> - deallocate_next_page_table(maddr_to_page(next_table_maddr),
>>> - next_level);
>>> + ASSERT(pde->next_level == level - 1);
>>> + deallocate_next_page_table(mfn_to_page(_mfn(pde->mfn)),
>>> + pde->next_level);
>>> }
>>> }
>>>
>>> @@ -500,10 +496,8 @@ static void amd_dump_p2m_table_level(struct
>> page_info* pg, int level,
>>> paddr_t gpa, int indent)
>>> {
>>> paddr_t address;
>>> - void *table_vaddr, *pde;
>>> - paddr_t next_table_maddr;
>>> - int index, next_level, present;
>>> - u32 *entry;
>>> + struct amd_iommu_pte *table_vaddr;
>>> + int index;
>>>
>>> if ( level < 1 )
>>> return;
>>> @@ -518,42 +512,32 @@ static void amd_dump_p2m_table_level(struct
>> page_info* pg, int level,
>>>
>>> for ( index = 0; index < PTE_PER_TABLE_SIZE; index++ )
>>> {
>>> + struct amd_iommu_pte *pde = &table_vaddr[index];
>>> +
>>> if ( !(index % 2) )
>>> process_pending_softirqs();
>>>
>>> - pde = table_vaddr + (index * IOMMU_PAGE_TABLE_ENTRY_SIZE);
>>> - next_table_maddr = amd_iommu_get_address_from_pte(pde);
>>> - entry = pde;
>>> -
>>> - present = get_field_from_reg_u32(entry[0],
>>> - IOMMU_PDE_PRESENT_MASK,
>>> - IOMMU_PDE_PRESENT_SHIFT);
>>> -
>>> - if ( !present )
>>> + if ( !pde->pr )
>>> continue;
>>>
>>> - next_level = get_field_from_reg_u32(entry[0],
>>> - IOMMU_PDE_NEXT_LEVEL_MASK,
>>> -
>> IOMMU_PDE_NEXT_LEVEL_SHIFT);
>>> -
>>> - if ( next_level && (next_level != (level - 1)) )
>>> + if ( pde->next_level && (pde->next_level != (level - 1)) )
>>> {
>>> printk("IOMMU p2m table error. next_level = %d, expected
>> %d\n",
>>> - next_level, level - 1);
>>> + pde->next_level, level - 1);
>>>
>>> continue;
>>> }
>>>
>>> address = gpa + amd_offset_level_address(index, level);
>>> - if ( next_level >= 1 )
>>> + if ( pde->next_level >= 1 )
>>> amd_dump_p2m_table_level(
>>> - maddr_to_page(next_table_maddr), next_level,
>>> + mfn_to_page(_mfn(pde->mfn)), pde->next_level,
>>> address, indent + 1);
>>> else
>>> printk("%*sdfn: %08lx mfn: %08lx\n",
>>> indent, "",
>>> (unsigned long)PFN_DOWN(address),
>>> - (unsigned long)PFN_DOWN(next_table_maddr));
>>> + (unsigned long)PFN_DOWN(pfn_to_paddr(pde->mfn)));
>>> }
>>>
>>> unmap_domain_page(table_vaddr);
>>> diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
>> b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
>>> index a217245249..a3a49f91eb 100644
>>> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
>>> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-defs.h
>>> @@ -413,38 +413,21 @@
>>> #define IOMMU_PAGE_TABLE_U32_PER_ENTRY
>> (IOMMU_PAGE_TABLE_ENTRY_SIZE / 4)
>>> #define IOMMU_PAGE_TABLE_ALIGNMENT 4096
>>>
>>> -#define IOMMU_PTE_PRESENT_MASK 0x00000001
>>> -#define IOMMU_PTE_PRESENT_SHIFT 0
>>> -#define IOMMU_PTE_NEXT_LEVEL_MASK 0x00000E00
>>> -#define IOMMU_PTE_NEXT_LEVEL_SHIFT 9
>>> -#define IOMMU_PTE_ADDR_LOW_MASK 0xFFFFF000
>>> -#define IOMMU_PTE_ADDR_LOW_SHIFT 12
>>> -#define IOMMU_PTE_ADDR_HIGH_MASK 0x000FFFFF
>>> -#define IOMMU_PTE_ADDR_HIGH_SHIFT 0
>>> -#define IOMMU_PTE_U_MASK 0x08000000
>>> -#define IOMMU_PTE_U_SHIFT 7
>>> -#define IOMMU_PTE_FC_MASK 0x10000000
>>> -#define IOMMU_PTE_FC_SHIFT 28
>>> -#define IOMMU_PTE_IO_READ_PERMISSION_MASK 0x20000000
>>> -#define IOMMU_PTE_IO_READ_PERMISSION_SHIFT 29
>>> -#define IOMMU_PTE_IO_WRITE_PERMISSION_MASK 0x40000000
>>> -#define IOMMU_PTE_IO_WRITE_PERMISSION_SHIFT 30
>>> -
>>> -/* I/O Page Directory */
>>> -#define IOMMU_PAGE_DIRECTORY_ENTRY_SIZE 8
>>> -#define IOMMU_PAGE_DIRECTORY_ALIGNMENT 4096
>>> -#define IOMMU_PDE_PRESENT_MASK 0x00000001
>>> -#define IOMMU_PDE_PRESENT_SHIFT 0
>>> -#define IOMMU_PDE_NEXT_LEVEL_MASK 0x00000E00
>>> -#define IOMMU_PDE_NEXT_LEVEL_SHIFT 9
>>> -#define IOMMU_PDE_ADDR_LOW_MASK 0xFFFFF000
>>> -#define IOMMU_PDE_ADDR_LOW_SHIFT 12
>>> -#define IOMMU_PDE_ADDR_HIGH_MASK 0x000FFFFF
>>> -#define IOMMU_PDE_ADDR_HIGH_SHIFT 0
>>> -#define IOMMU_PDE_IO_READ_PERMISSION_MASK 0x20000000
>>> -#define IOMMU_PDE_IO_READ_PERMISSION_SHIFT 29
>>> -#define IOMMU_PDE_IO_WRITE_PERMISSION_MASK 0x40000000
>>> -#define IOMMU_PDE_IO_WRITE_PERMISSION_SHIFT 30
>>> +struct amd_iommu_pte {
>>> + uint64_t pr:1;
>>> + uint64_t ignored0:4;
>>> + uint64_t a:1;
>>> + uint64_t d:1;
>>> + uint64_t ignored1:2;
>>> + uint64_t next_level:3;
>>> + uint64_t mfn:40;
>>> + uint64_t reserved:7;
>>> + uint64_t u:1;
>>> + uint64_t fc:1;
>>> + uint64_t ir:1;
>>> + uint64_t iw:1;
>>> + uint64_t ignored2:1;
>>> +};
>>>
>>> /* Paging modes */
>>> #define IOMMU_PAGING_MODE_DISABLED 0x0
>>> diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
>> b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
>>> index c5697565d6..1c1971bb7c 100644
>>> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
>>> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
>>> @@ -57,7 +57,6 @@ int __must_check amd_iommu_map_page(struct domain *d,
>> dfn_t dfn,
>>> unsigned int *flush_flags);
>>> int __must_check amd_iommu_unmap_page(struct domain *d, dfn_t dfn,
>>> unsigned int *flush_flags);
>>> -uint64_t amd_iommu_get_address_from_pte(void *entry);
>>> int __must_check amd_iommu_alloc_root(struct domain_iommu *hd);
>>> int amd_iommu_reserve_domain_unity_map(struct domain *domain,
>>> paddr_t phys_addr, unsigned
>> long size,
>>> @@ -280,18 +279,4 @@ static inline void
>> iommu_set_addr_hi_to_reg(uint32_t *reg, uint32_t addr)
>>> IOMMU_REG_BASE_ADDR_HIGH_SHIFT, reg);
>>> }
>>>
>>> -static inline int iommu_is_pte_present(const u32 *entry)
>>> -{
>>> - return get_field_from_reg_u32(entry[0],
>>> - IOMMU_PDE_PRESENT_MASK,
>>> - IOMMU_PDE_PRESENT_SHIFT);
>>> -}
>>> -
>>> -static inline unsigned int iommu_next_level(const u32 *entry)
>>> -{
>>> - return get_field_from_reg_u32(entry[0],
>>> - IOMMU_PDE_NEXT_LEVEL_MASK,
>>> - IOMMU_PDE_NEXT_LEVEL_SHIFT);
>>> -}
>>> -
>>> #endif /* _ASM_X86_64_AMD_IOMMU_PROTO_H */
>>>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] amd-iommu: use a bitfield for PTE/PDE
2019-02-13 15:33 ` Woods, Brian
@ 2019-02-13 15:50 ` Paul Durrant
0 siblings, 0 replies; 8+ messages in thread
From: Paul Durrant @ 2019-02-13 15:50 UTC (permalink / raw)
To: 'Woods, Brian', xen-devel
Cc: Andrew Cooper, Wei Liu, Suthikulpanit, Suravee, Jan Beulich,
Roger Pau Monne
> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Woods, Brian
> Sent: 13 February 2019 15:34
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Jan Beulich <jbeulich@suse.com>; Suthikulpanit,
> Suravee <Suravee.Suthikulpanit@amd.com>; Roger Pau Monne
> <roger.pau@citrix.com>
> Subject: Re: [Xen-devel] [PATCH 1/2] amd-iommu: use a bitfield for PTE/PDE
>
> On 2/13/19 3:45 AM, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Woods, Brian [mailto:Brian.Woods@amd.com]
> >> Sent: 12 February 2019 20:14
> >> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-
> devel@lists.xenproject.org
> >> Cc: Suthikulpanit, Suravee <Suravee.Suthikulpanit@amd.com>; Jan Beulich
> >> <jbeulich@suse.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> >> <wei.liu2@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>
> >> Subject: Re: [PATCH 1/2] amd-iommu: use a bitfield for PTE/PDE
> >>
> >> On 2/4/19 5:19 AM, Paul Durrant wrote:
> >>> The current use of get/set_field_from/in_reg_u32() is both inefficient
> >> and
> >>> requires some ugly casting.
> >>>
> >>> This patch defines a new bitfield structure (amd_iommu_pte) and uses
> >> this
> >>> structure in all PTE/PDE manipulation, resulting in much more readable
> >>> and compact code.
> >>>
> >>> NOTE: This commit also fixes one malformed comment in
> >>> set_iommu_pte_present().
> >>>
> >>> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> >>
> >> Sorry about the delay.
> >>
> >> Nitpick here, but I'd rather have !!IOMMUF_{writable,readable} than
> >> true.
> >
> > That's pretty ugly. How about I pass an OR of the flags through to lower
> level functions rather than a pair of bools? If you're ok with that I'll
> send a v2.
> >
> > Paul
> >
>
> There's no need for a v2 based on that, that's just me nitpicking.
> There's no real nice way to do it without turning
> IOMMUF_{writable,readable} into bools or your suggested way which has
> more code to decode a flag. Assuming everyone else is ok with the
> patches as are, it's fine. If there's going to be a v2 for other
> reasons, I'll just leave it up to your discretion (other people may have
> stronger opinions about it anyway).
>
Ok. Thanks Brian,
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-02-13 15:55 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-04 11:19 [PATCH 0/2] amd-iommu: use bitfields for PTE/PDE and DTE Paul Durrant
2019-02-04 11:19 ` [PATCH 1/2] amd-iommu: use a bitfield for PTE/PDE Paul Durrant
2019-02-12 20:14 ` Woods, Brian
2019-02-13 9:45 ` Paul Durrant
2019-02-13 15:33 ` Woods, Brian
2019-02-13 15:50 ` Paul Durrant
2019-02-04 11:19 ` [PATCH 2/2] amd-iommu: use a bitfield for DTE Paul Durrant
2019-02-12 20:30 ` Woods, Brian
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.