* [PATCH] amd-iommu: use correct constants in amd_iommu_get_next_table_from_pte()...
@ 2018-09-26 12:56 Paul Durrant
2018-09-26 13:32 ` Jan Beulich
0 siblings, 1 reply; 3+ messages in thread
From: Paul Durrant @ 2018-09-26 12:56 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Brian Woods, Suravee Suthikulpanit
...and change the name to amd_iommu_get_address_from_pte() since the
address read is not necessarily a 'next level' page table.
The constants in use prior to this patch relate to device table entries
rather than page table entries. Although they do have the same value, it
makes the code confusing to read.
This patch also changes the PDE/PTE pointer argument to void *, and
removes any u32/uint32_t casts in the call sites. Unnecessary casts
surrounding call sites are also removed.
No functional change.
NOTE: The patch also adds emacs boilerplate to iommu_map.c
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
Cc: Brian Woods <brian.woods@amd.com>
---
xen/drivers/passthrough/amd/iommu_map.c | 40 ++++++++++++++++-----------
xen/drivers/passthrough/amd/pci_amd_iommu.c | 10 +++----
xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 2 +-
3 files changed, 30 insertions(+), 22 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu_map.c b/xen/drivers/passthrough/amd/iommu_map.c
index 70b4345b37..41247d8e9f 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -285,19 +285,18 @@ void iommu_dte_set_guest_cr3(u32 *dte, u16 dom_id, u64 gcr3,
dte[1] = entry;
}
-u64 amd_iommu_get_next_table_from_pte(u32 *entry)
+uint64_t amd_iommu_get_address_from_pte(void *pte)
{
- u64 addr_lo, addr_hi, ptr;
+ uint32_t *entry = pte;
+ uint64_t addr_lo, addr_hi, ptr;
- addr_lo = get_field_from_reg_u32(
- entry[0],
- IOMMU_DEV_TABLE_PAGE_TABLE_PTR_LOW_MASK,
- IOMMU_DEV_TABLE_PAGE_TABLE_PTR_LOW_SHIFT);
+ addr_lo = get_field_from_reg_u32(entry[0],
+ IOMMU_PDE_ADDR_LOW_MASK,
+ IOMMU_PDE_ADDR_LOW_SHIFT);
- addr_hi = get_field_from_reg_u32(
- entry[1],
- IOMMU_DEV_TABLE_PAGE_TABLE_PTR_HIGH_MASK,
- IOMMU_DEV_TABLE_PAGE_TABLE_PTR_HIGH_SHIFT);
+ addr_hi = get_field_from_reg_u32(entry[1],
+ IOMMU_PDE_ADDR_HIGH_MASK,
+ IOMMU_PDE_ADDR_HIGH_SHIFT);
ptr = (addr_hi << 32) | (addr_lo << PAGE_SHIFT);
return ptr;
@@ -350,11 +349,11 @@ static int iommu_update_pde_count(struct domain *d, unsigned long pt_mfn,
pde = table + pfn_to_pde_idx(gfn, merge_level);
/* get page table of next level */
- ntable_maddr = amd_iommu_get_next_table_from_pte((u32*)pde);
+ ntable_maddr = amd_iommu_get_address_from_pte(pde);
ntable = map_domain_page(_mfn(paddr_to_pfn(ntable_maddr)));
/* get the first mfn of next level */
- first_mfn = amd_iommu_get_next_table_from_pte((u32*)ntable) >> PAGE_SHIFT;
+ first_mfn = amd_iommu_get_address_from_pte(ntable) >> PAGE_SHIFT;
if ( first_mfn == 0 )
goto out;
@@ -401,7 +400,7 @@ static int iommu_merge_pages(struct domain *d, unsigned long pt_mfn,
pde = table + pfn_to_pde_idx(gfn, merge_level);
/* get first mfn */
- ntable_mfn = amd_iommu_get_next_table_from_pte((u32*)pde) >> PAGE_SHIFT;
+ ntable_mfn = amd_iommu_get_address_from_pte(pde) >> PAGE_SHIFT;
if ( ntable_mfn == 0 )
{
@@ -410,7 +409,7 @@ static int iommu_merge_pages(struct domain *d, unsigned long pt_mfn,
}
ntable = map_domain_page(_mfn(ntable_mfn));
- first_mfn = amd_iommu_get_next_table_from_pte((u32*)ntable) >> PAGE_SHIFT;
+ first_mfn = amd_iommu_get_address_from_pte(ntable) >> PAGE_SHIFT;
if ( first_mfn == 0 )
{
@@ -468,8 +467,7 @@ static int iommu_pde_from_gfn(struct domain *d, unsigned long pfn,
pde = next_table_vaddr + pfn_to_pde_idx(pfn, level);
/* Here might be a super page frame */
- next_table_mfn = amd_iommu_get_next_table_from_pte((uint32_t*)pde)
- >> PAGE_SHIFT;
+ next_table_mfn = amd_iommu_get_address_from_pte(pde) >> PAGE_SHIFT;
/* Split super page frame into smaller pieces.*/
if ( iommu_is_pte_present((u32*)pde) &&
@@ -815,3 +813,13 @@ void amd_iommu_share_p2m(struct domain *d)
mfn_x(pgd_mfn));
}
}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 4a633ca940..80d9ae6561 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -430,11 +430,11 @@ 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_next_table_from_pte(pde);
- next_level = iommu_next_level((u32*)pde);
+ next_table_maddr = amd_iommu_get_address_from_pte(pde);
+ next_level = iommu_next_level(pde);
if ( (next_table_maddr != 0) && (next_level != 0) &&
- iommu_is_pte_present((u32*)pde) )
+ iommu_is_pte_present(pde) )
{
/* We do not support skip levels yet */
ASSERT(next_level == level - 1);
@@ -559,8 +559,8 @@ static void amd_dump_p2m_table_level(struct page_info* pg, int level,
process_pending_softirqs();
pde = table_vaddr + (index * IOMMU_PAGE_TABLE_ENTRY_SIZE);
- next_table_maddr = amd_iommu_get_next_table_from_pte(pde);
- entry = (u32*)pde;
+ next_table_maddr = amd_iommu_get_address_from_pte(pde);
+ entry = pde;
present = get_field_from_reg_u32(entry[0],
IOMMU_PDE_PRESENT_MASK,
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 99bc21c7b3..a6ba0445da 100644
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
@@ -55,7 +55,7 @@ int amd_iommu_update_ivrs_mapping_acpi(void);
int __must_check amd_iommu_map_page(struct domain *d, unsigned long gfn,
unsigned long mfn, unsigned int flags);
int __must_check amd_iommu_unmap_page(struct domain *d, unsigned long gfn);
-u64 amd_iommu_get_next_table_from_pte(u32 *entry);
+uint64_t amd_iommu_get_address_from_pte(void *pte);
int __must_check amd_iommu_alloc_root(struct domain_iommu *hd);
int amd_iommu_reserve_domain_unity_map(struct domain *domain,
u64 phys_addr, unsigned long size,
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] amd-iommu: use correct constants in amd_iommu_get_next_table_from_pte()...
2018-09-26 12:56 [PATCH] amd-iommu: use correct constants in amd_iommu_get_next_table_from_pte() Paul Durrant
@ 2018-09-26 13:32 ` Jan Beulich
2018-09-26 13:37 ` Paul Durrant
0 siblings, 1 reply; 3+ messages in thread
From: Jan Beulich @ 2018-09-26 13:32 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Brian Woods, Suravee Suthikulpanit
>>> On 26.09.18 at 14:56, <paul.durrant@citrix.com> wrote:
> ...and change the name to amd_iommu_get_address_from_pte() since the
> address read is not necessarily a 'next level' page table.
There was no "level" in the original name. Which isn't meant to be an
objection to the rename.
> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -285,19 +285,18 @@ void iommu_dte_set_guest_cr3(u32 *dte, u16 dom_id, u64 gcr3,
> dte[1] = entry;
> }
>
> -u64 amd_iommu_get_next_table_from_pte(u32 *entry)
> +uint64_t amd_iommu_get_address_from_pte(void *pte)
> {
> - u64 addr_lo, addr_hi, ptr;
> + uint32_t *entry = pte;
> + uint64_t addr_lo, addr_hi, ptr;
>
> - addr_lo = get_field_from_reg_u32(
> - entry[0],
> - IOMMU_DEV_TABLE_PAGE_TABLE_PTR_LOW_MASK,
> - IOMMU_DEV_TABLE_PAGE_TABLE_PTR_LOW_SHIFT);
> + addr_lo = get_field_from_reg_u32(entry[0],
> + IOMMU_PDE_ADDR_LOW_MASK,
> + IOMMU_PDE_ADDR_LOW_SHIFT);
>
> - addr_hi = get_field_from_reg_u32(
> - entry[1],
> - IOMMU_DEV_TABLE_PAGE_TABLE_PTR_HIGH_MASK,
> - IOMMU_DEV_TABLE_PAGE_TABLE_PTR_HIGH_SHIFT);
> + addr_hi = get_field_from_reg_u32(entry[1],
> + IOMMU_PDE_ADDR_HIGH_MASK,
> + IOMMU_PDE_ADDR_HIGH_SHIFT);
With the function parameter named pte, perhaps better to also use
IOMMU_PTE_ADDR_* (which is also more generic imo, as it covers
both leaf and non-leaf entries).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] amd-iommu: use correct constants in amd_iommu_get_next_table_from_pte()...
2018-09-26 13:32 ` Jan Beulich
@ 2018-09-26 13:37 ` Paul Durrant
0 siblings, 0 replies; 3+ messages in thread
From: Paul Durrant @ 2018-09-26 13:37 UTC (permalink / raw)
To: 'Jan Beulich'; +Cc: xen-devel, Brian Woods, Suravee Suthikulpanit
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 26 September 2018 14:32
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Brian Woods <brian.woods@amd.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: Re: [Xen-devel] [PATCH] amd-iommu: use correct constants in
> amd_iommu_get_next_table_from_pte()...
>
> >>> On 26.09.18 at 14:56, <paul.durrant@citrix.com> wrote:
> > ...and change the name to amd_iommu_get_address_from_pte() since the
> > address read is not necessarily a 'next level' page table.
>
> There was no "level" in the original name. Which isn't meant to be an
> objection to the rename.
Ok. I'll re-word it a bit.
>
> > --- a/xen/drivers/passthrough/amd/iommu_map.c
> > +++ b/xen/drivers/passthrough/amd/iommu_map.c
> > @@ -285,19 +285,18 @@ void iommu_dte_set_guest_cr3(u32 *dte, u16 dom_id,
> u64 gcr3,
> > dte[1] = entry;
> > }
> >
> > -u64 amd_iommu_get_next_table_from_pte(u32 *entry)
> > +uint64_t amd_iommu_get_address_from_pte(void *pte)
> > {
> > - u64 addr_lo, addr_hi, ptr;
> > + uint32_t *entry = pte;
> > + uint64_t addr_lo, addr_hi, ptr;
> >
> > - addr_lo = get_field_from_reg_u32(
> > - entry[0],
> > - IOMMU_DEV_TABLE_PAGE_TABLE_PTR_LOW_MASK,
> > - IOMMU_DEV_TABLE_PAGE_TABLE_PTR_LOW_SHIFT);
> > + addr_lo = get_field_from_reg_u32(entry[0],
> > + IOMMU_PDE_ADDR_LOW_MASK,
> > + IOMMU_PDE_ADDR_LOW_SHIFT);
> >
> > - addr_hi = get_field_from_reg_u32(
> > - entry[1],
> > - IOMMU_DEV_TABLE_PAGE_TABLE_PTR_HIGH_MASK,
> > - IOMMU_DEV_TABLE_PAGE_TABLE_PTR_HIGH_SHIFT);
> > + addr_hi = get_field_from_reg_u32(entry[1],
> > + IOMMU_PDE_ADDR_HIGH_MASK,
> > + IOMMU_PDE_ADDR_HIGH_SHIFT);
>
> With the function parameter named pte, perhaps better to also use
> IOMMU_PTE_ADDR_* (which is also more generic imo, as it covers
> both leaf and non-leaf entries).
>
Yes, agreed.
Paul
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-09-26 13:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-26 12:56 [PATCH] amd-iommu: use correct constants in amd_iommu_get_next_table_from_pte() Paul Durrant
2018-09-26 13:32 ` Jan Beulich
2018-09-26 13:37 ` Paul Durrant
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.