Re: [Patch v3 05/12] iommu/amd: change IOMMU_PTE_P to IOMMU_PTE_V

[Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On 01/29/16 at 10:55am, Wan Zongshun wrote:
> 
> 
> -------- Original Message --------
> >On 01/27/16 at 06:18pm, Wan Zongshun wrote:
> >>
> >>
> >>-------- Original Message --------
> >>>In amd-vi spec the name of bit0 in DTE is V. But in code it's defined
> >>>as IOMMU_PTE_P. Here change it to IOMMU_PTE_V to make it be consistent
> >>>with spec.
> >>
> >>Hi, Baoquan
> >>
> >>This should be PR bit which means present, So maybe you got
> >>confusion between DTE and PTE table, right?
> >
> >Well, I got it. In fact the MACRO definition and current code confused
> >me. IOMMU_PTE_P is not only used for PTE table, but used for DTE entry.
> >
> >You can check functions set_dte_entry()/clear_dte_entry() where
> >IOMMU_PTE_P and IOMMU_PTE_TV are set in DTE entry to indicate the DTE
> >entry and translation is valid. Meanwhile I saw in iommu_map_page
> >IOMMU_PTE_P is used to incidate pte is present. This makes us confused. I
> >will post a patch to clean up this, defining new specific MACRO for DTE
> >use only. Do you agree on this?
> >
> 
> Baoquan,
> 
> It is fine to me, is it better to add a lot of explanation for those
> bits MACRO? like:
> 
> /* PTE bit value definition*/
> #define IOMMU_PTE_P  (1ULL << 0)
> #define IOMMU_PTE_TV (1ULL << 1)
> 
> /* DTE bit value definition*/
> #define IOMMU_DTE_TV (1ULL << 1)

Agree. It makes sense to make code more read-able. Will add.

> 
> >>
> >>So please don't change the IOMMU_PTE_P to IOMMU_PTE_V firstly, maybe
> >>you should rename IOMMU_PTE_TV to IOMMU_DTE_TV, or just keep those
> >>codes here, does make sense?
> >
> >As you said, IOMMU_PTE_P need be kept, also IOMMU_DTE_V/IOMMU_DTE_TV are
> >also necessary to re-define separately.
> >>
> >>Vincent.
> >>
> >>>
> >>>Signed-off-by: Baoquan He <bhe-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>>---
> >>>  drivers/iommu/amd_iommu.c       | 10 +++++-----
> >>>  drivers/iommu/amd_iommu_types.h |  6 +++---
> >>>  2 files changed, 8 insertions(+), 8 deletions(-)
> >>>
> >>>diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
> >>>index 63f4c6b..f02d4b1 100644
> >>>--- a/drivers/iommu/amd_iommu.c
> >>>+++ b/drivers/iommu/amd_iommu.c
> >>>@@ -1331,9 +1331,9 @@ static int iommu_map_page(struct protection_domain *dom,
> >>>
> >>>  	if (count > 1) {
> >>>  		__pte = PAGE_SIZE_PTE(phys_addr, page_size);
> >>>-		__pte |= PM_LEVEL_ENC(7) | IOMMU_PTE_P | IOMMU_PTE_FC;
> >>>+		__pte |= PM_LEVEL_ENC(7) | IOMMU_PTE_V | IOMMU_PTE_FC;
> >>>  	} else
> >>>-		__pte = phys_addr | IOMMU_PTE_P | IOMMU_PTE_FC;
> >>>+		__pte = phys_addr | IOMMU_PTE_V | IOMMU_PTE_FC;
> >>>
> >>>  	if (prot & IOMMU_PROT_IR)
> >>>  		__pte |= IOMMU_PTE_IR;
> >>>@@ -1978,7 +1978,7 @@ static void set_dte_entry(u16 devid, struct protection_domain *domain, bool ats)
> >>>
> >>>  	pte_root |= (domain->mode & DEV_ENTRY_MODE_MASK)
> >>>  		    << DEV_ENTRY_MODE_SHIFT;
> >>>-	pte_root |= IOMMU_PTE_IR | IOMMU_PTE_IW | IOMMU_PTE_P | IOMMU_PTE_TV;
> >>>+	pte_root |= IOMMU_PTE_IR | IOMMU_PTE_IW | IOMMU_PTE_V | IOMMU_PTE_TV;
> >>>
> >>>  	flags = amd_iommu_dev_table[devid].data[1];
> >>>
> >>>@@ -2021,7 +2021,7 @@ static void set_dte_entry(u16 devid, struct protection_domain *domain, bool ats)
> >>>  static void clear_dte_entry(u16 devid)
> >>>  {
> >>>  	/* remove entry from the device table seen by the hardware */
> >>>-	amd_iommu_dev_table[devid].data[0]  = IOMMU_PTE_P | IOMMU_PTE_TV;
> >>>+	amd_iommu_dev_table[devid].data[0]  = IOMMU_PTE_V | IOMMU_PTE_TV;
> >>>  	amd_iommu_dev_table[devid].data[1] &= DTE_FLAG_MASK;
> >>>
> >>>  	amd_iommu_apply_erratum_63(devid);
> >>>@@ -2463,7 +2463,7 @@ static dma_addr_t dma_ops_domain_map(struct dma_ops_domain *dom,
> >>>  	if (!pte)
> >>>  		return DMA_ERROR_CODE;
> >>>
> >>>-	__pte = paddr | IOMMU_PTE_P | IOMMU_PTE_FC;
> >>>+	__pte = paddr | IOMMU_PTE_V | IOMMU_PTE_FC;
> >>>
> >>>  	if (direction == DMA_TO_DEVICE)
> >>>  		__pte |= IOMMU_PTE_IR;
> >>>diff --git a/drivers/iommu/amd_iommu_types.h b/drivers/iommu/amd_iommu_types.h
> >>>index 9b8ace4..65f7988 100644
> >>>--- a/drivers/iommu/amd_iommu_types.h
> >>>+++ b/drivers/iommu/amd_iommu_types.h
> >>>@@ -244,7 +244,7 @@
> >>>  #define PM_LEVEL_INDEX(x, a)	(((a) >> PM_LEVEL_SHIFT((x))) & 0x1ffULL)
> >>>  #define PM_LEVEL_ENC(x)		(((x) << 9) & 0xe00ULL)
> >>>  #define PM_LEVEL_PDE(x, a)	((a) | PM_LEVEL_ENC((x)) | \
> >>>-				 IOMMU_PTE_P | IOMMU_PTE_IR | IOMMU_PTE_IW)
> >>>+				 IOMMU_PTE_V | IOMMU_PTE_IR | IOMMU_PTE_IW)
> >>>  #define PM_PTE_LEVEL(pte)	(((pte) >> 9) & 0x7ULL)
> >>>
> >>>  #define PM_MAP_4k		0
> >>>@@ -293,7 +293,7 @@
> >>>  #define PTE_LEVEL_PAGE_SIZE(level)			\
> >>>  	(1ULL << (12 + (9 * (level))))
> >>>
> >>>-#define IOMMU_PTE_P  (1ULL << 0)
> >>>+#define IOMMU_PTE_V  (1ULL << 0)
> >>>  #define IOMMU_PTE_TV (1ULL << 1)
> >>>  #define IOMMU_PTE_U  (1ULL << 59)
> >>>  #define IOMMU_PTE_FC (1ULL << 60)
> >>>@@ -321,7 +321,7 @@
> >>>  #define GCR3_VALID		0x01ULL
> >>>
> >>>  #define IOMMU_PAGE_MASK (((1ULL << 52) - 1) & ~0xfffULL)
> >>>-#define IOMMU_PTE_PRESENT(pte) ((pte) & IOMMU_PTE_P)
> >>>+#define IOMMU_PTE_PRESENT(pte) ((pte) & IOMMU_PTE_V)
> >>>  #define IOMMU_PTE_PAGE(pte) (phys_to_virt((pte) & IOMMU_PAGE_MASK))
> >>>  #define IOMMU_PTE_MODE(pte) (((pte) >> 9) & 0x07)
> >>>
> >>>
> >