[PATCH 1/1] x86/iommu: use bit structures for context_entry

[PATCH 1/1] x86/iommu: use bit structures for context_entry

[Li, Zhen-Hua]

There is a structure named context_entry used by intel iommu, and there
are some bit operations on it. Use bit structure may make these operations
easy.

Signed-off-by: Li, Zhen-Hua <zhen-hual-VXdhtT5mjnY@public.gmane.org>
---
 drivers/iommu/intel-iommu.c |   88 +++++++++++++++----------------------------
 1 file changed, 31 insertions(+), 57 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index b4f0e28..ae10471 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -221,55 +221,28 @@ get_context_addr_from_root(struct root_entry *root)
  * 8-23: domain id
  */
 struct context_entry {
-	u64 lo;
-	u64 hi;
+	union {
+		struct {
+			u64	present:1,
+				fpd:1,
+				translation_type:2,
+				reserved_l:8,
+				asr:52;
+		};
+		u64 lo;
+	};
+	union {
+		struct {
+			u64	aw:3,
+				avail:4,
+				reserved_h1:1,
+				did:16,
+				reserved_h2:40;
+		};
+		u64 hi;
+	};
 };
 
-static inline bool context_present(struct context_entry *context)
-{
-	return (context->lo & 1);
-}
-static inline void context_set_present(struct context_entry *context)
-{
-	context->lo |= 1;
-}
-
-static inline void context_set_fault_enable(struct context_entry *context)
-{
-	context->lo &= (((u64)-1) << 2) | 1;
-}
-
-static inline void context_set_translation_type(struct context_entry *context,
-						unsigned long value)
-{
-	context->lo &= (((u64)-1) << 4) | 3;
-	context->lo |= (value & 3) << 2;
-}
-
-static inline void context_set_address_root(struct context_entry *context,
-					    unsigned long value)
-{
-	context->lo |= value & VTD_PAGE_MASK;
-}
-
-static inline void context_set_address_width(struct context_entry *context,
-					     unsigned long value)
-{
-	context->hi |= value & 7;
-}
-
-static inline void context_set_domain_id(struct context_entry *context,
-					 unsigned long value)
-{
-	context->hi |= (value & ((1 << 16) - 1)) << 8;
-}
-
-static inline void context_clear_entry(struct context_entry *context)
-{
-	context->lo = 0;
-	context->hi = 0;
-}
-
 /*
  * 0: readable
  * 1: writable
@@ -727,7 +700,7 @@ static int device_context_mapped(struct intel_iommu *iommu, u8 bus, u8 devfn)
 		ret = 0;
 		goto out;
 	}
-	ret = context_present(&context[devfn]);
+	ret = context->present;
 out:
 	spin_unlock_irqrestore(&iommu->lock, flags);
 	return ret;
@@ -743,7 +716,8 @@ static void clear_context_table(struct intel_iommu *iommu, u8 bus, u8 devfn)
 	root = &iommu->root_entry[bus];
 	context = get_context_addr_from_root(root);
 	if (context) {
-		context_clear_entry(&context[devfn]);
+		context[devfn].lo = 0;
+		context[devfn].hi = 0;
 		__iommu_flush_cache(iommu, &context[devfn], \
 			sizeof(*context));
 	}
@@ -1573,7 +1547,7 @@ static int domain_context_mapping_one(struct dmar_domain *domain, int segment,
 	if (!context)
 		return -ENOMEM;
 	spin_lock_irqsave(&iommu->lock, flags);
-	if (context_present(context)) {
+	if (context->present) {
 		spin_unlock_irqrestore(&iommu->lock, flags);
 		return 0;
 	}
@@ -1623,7 +1597,7 @@ static int domain_context_mapping_one(struct dmar_domain *domain, int segment,
 		}
 	}
 
-	context_set_domain_id(context, id);
+	context->did = id;
 
 	if (translation != CONTEXT_TT_PASS_THROUGH) {
 		info = iommu_support_dev_iotlb(domain, segment, bus, devfn);
@@ -1635,15 +1609,15 @@ static int domain_context_mapping_one(struct dmar_domain *domain, int segment,
 	 * AGAW value supported by hardware. And ASR is ignored by hardware.
 	 */
 	if (unlikely(translation == CONTEXT_TT_PASS_THROUGH))
-		context_set_address_width(context, iommu->msagaw);
+		context->aw = iommu->msagaw;
 	else {
-		context_set_address_root(context, virt_to_phys(pgd));
-		context_set_address_width(context, iommu->agaw);
+		context->asr = virt_to_phys(pgd) >> VTD_PAGE_SHIFT;
+		context->aw  = iommu->agaw;
 	}
 
-	context_set_translation_type(context, translation);
-	context_set_fault_enable(context);
-	context_set_present(context);
+	context->translation_type = translation;
+	context->fpd = 0;
+	context->present = 1;
 	domain_flush_cache(domain, context, sizeof(*context));
 
 	/*
-- 
1.7.10.4

Re: [PATCH 1/1] x86/iommu: use bit structures for context_entry

[Joe Perches]

On Fri, 2013-05-24 at 08:22 +0800, Li, Zhen-Hua wrote:
> There is a structure named context_entry used by intel iommu, and there
> are some bit operations on it. Use bit structure may make these operations
> easy.
[]
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
[]
> @@ -221,55 +221,28 @@ get_context_addr_from_root(struct root_entry *root)
>   * 8-23: domain id
>   */
>  struct context_entry {
> -	u64 lo;
> -	u64 hi;
> +	union {
> +		struct {
> +			u64	present:1,

why use struct and union at all?

Why not just:

struct context_entry {
	u64	present:1,
		fpd:1,
		translation_type:2,
		reserved_l:8,
		asr:52;
	u64	aw:3,
		avail:4,
		reserved_h1:1,
		did:16,
		reserved_h2:40;
}; 

> @@ -743,7 +716,8 @@ static void clear_context_table(struct intel_iommu *iommu, u8 bus, u8 devfn)
[]
>  	if (context) {
> -		context_clear_entry(&context[devfn]);
> +		context[devfn].lo = 0;
> +		context[devfn].hi = 0;

		memset(&context[devfn], 0, sizeof(...))

Re: [PATCH 1/1] x86/iommu: use bit structures for context_entry

[ZhenHua]

Use lo and hi for clear, may run faster than memset.

But it is not a big problem.  Never mind.

Thanks
ZhenHua

On 05/24/2013 08:36 AM, Joe Perches wrote:
> On Fri, 2013-05-24 at 08:22 +0800, Li, Zhen-Hua wrote:
>> There is a structure named context_entry used by intel iommu, and there
>> are some bit operations on it. Use bit structure may make these operations
>> easy.
> []
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> []
>> @@ -221,55 +221,28 @@ get_context_addr_from_root(struct root_entry *root)
>>    * 8-23: domain id
>>    */
>>   struct context_entry {
>> -	u64 lo;
>> -	u64 hi;
>> +	union {
>> +		struct {
>> +			u64	present:1,
> why use struct and union at all?
>
> Why not just:
>
> struct context_entry {
> 	u64	present:1,
> 		fpd:1,
> 		translation_type:2,
> 		reserved_l:8,
> 		asr:52;
> 	u64	aw:3,
> 		avail:4,
> 		reserved_h1:1,
> 		did:16,
> 		reserved_h2:40;
> };
>
>> @@ -743,7 +716,8 @@ static void clear_context_table(struct intel_iommu *iommu, u8 bus, u8 devfn)
> []
>>   	if (context) {
>> -		context_clear_entry(&context[devfn]);
>> +		context[devfn].lo = 0;
>> +		context[devfn].hi = 0;
> 		memset(&context[devfn], 0, sizeof(...))
>
>

[PATCH 1/1] x86/iommu: use bit structures for context_entry

[Li, Zhen-Hua]

There is a structure named context_entry used by intel iommu, and there
are some bit operations on it. Use bit structure may make these operations
easy.
Also the function context_set_address_root may cause problem because it uses
 |= operation, not set the new value. And this patch can fix it.

Signed-off-by: Li, Zhen-Hua <zhen-hual-VXdhtT5mjnY@public.gmane.org>
---
 drivers/iommu/intel-iommu.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 43b9bfe..65cd480 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -221,47 +221,64 @@ get_context_addr_from_root(struct root_entry *root)
  * 8-23: domain id
  */
 struct context_entry {
-	u64 lo;
-	u64 hi;
+	union {
+		struct {
+			u64	present:1,
+				fpd:1,
+				translation_type:2,
+				reserved_l:8,
+				asr:52;
+		};
+		u64 lo;
+	};
+	union {
+		struct {
+			u64	aw:3,
+				avail:4,
+				reserved_h1:1,
+				did:16,
+				reserved_h2:40;
+		};
+		u64 hi;
+	};
 };
 
 static inline bool context_present(struct context_entry *context)
 {
-	return (context->lo & 1);
+	return context->present;
 }
 static inline void context_set_present(struct context_entry *context)
 {
-	context->lo |= 1;
+	context->present = 1;
 }
 
 static inline void context_set_fault_enable(struct context_entry *context)
 {
-	context->lo &= (((u64)-1) << 2) | 1;
+	context->fpd = 1;
 }
 
 static inline void context_set_translation_type(struct context_entry *context,
 						unsigned long value)
 {
-	context->lo &= (((u64)-1) << 4) | 3;
-	context->lo |= (value & 3) << 2;
+	context->translation_type = value;
 }
 
 static inline void context_set_address_root(struct context_entry *context,
 					    unsigned long value)
 {
-	context->lo |= value & VTD_PAGE_MASK;
+	context->asr = value >> VTD_PAGE_SHIFT
 }
 
 static inline void context_set_address_width(struct context_entry *context,
 					     unsigned long value)
 {
-	context->hi |= value & 7;
+	context->aw  = value;
 }
 
 static inline void context_set_domain_id(struct context_entry *context,
 					 unsigned long value)
 {
-	context->hi |= (value & ((1 << 16) - 1)) << 8;
+	context->did = value;
 }
 
 static inline void context_clear_entry(struct context_entry *context)
-- 
1.8.4.3

Re: [PATCH 1/1] x86/iommu: use bit structures for context_entry

[Kai Huang]

[-- Attachment #1.1: Type: text/plain, Size: 3459 bytes --]

Just curious, why would |= operation cause problem? :)


On Fri, Dec 20, 2013 at 4:45 PM, Li, Zhen-Hua <zhen-hual-VXdhtT5mjnY@public.gmane.org> wrote:

> There is a structure named context_entry used by intel iommu, and there
> are some bit operations on it. Use bit structure may make these operations
> easy.
> Also the function context_set_address_root may cause problem because it
> uses
>  |= operation, not set the new value. And this patch can fix it.
>
> Signed-off-by: Li, Zhen-Hua <zhen-hual-VXdhtT5mjnY@public.gmane.org>
> ---
>  drivers/iommu/intel-iommu.c | 37 +++++++++++++++++++++++++++----------
>  1 file changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 43b9bfe..65cd480 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -221,47 +221,64 @@ get_context_addr_from_root(struct root_entry *root)
>   * 8-23: domain id
>   */
>  struct context_entry {
> -       u64 lo;
> -       u64 hi;
> +       union {
> +               struct {
> +                       u64     present:1,
> +                               fpd:1,
> +                               translation_type:2,
> +                               reserved_l:8,
> +                               asr:52;
> +               };
> +               u64 lo;
> +       };
> +       union {
> +               struct {
> +                       u64     aw:3,
> +                               avail:4,
> +                               reserved_h1:1,
> +                               did:16,
> +                               reserved_h2:40;
> +               };
> +               u64 hi;
> +       };
>  };
>
>  static inline bool context_present(struct context_entry *context)
>  {
> -       return (context->lo & 1);
> +       return context->present;
>  }
>  static inline void context_set_present(struct context_entry *context)
>  {
> -       context->lo |= 1;
> +       context->present = 1;
>  }
>
>  static inline void context_set_fault_enable(struct context_entry *context)
>  {
> -       context->lo &= (((u64)-1) << 2) | 1;
> +       context->fpd = 1;
>  }
>
>  static inline void context_set_translation_type(struct context_entry
> *context,
>                                                 unsigned long value)
>  {
> -       context->lo &= (((u64)-1) << 4) | 3;
> -       context->lo |= (value & 3) << 2;
> +       context->translation_type = value;
>  }
>
>  static inline void context_set_address_root(struct context_entry *context,
>                                             unsigned long value)
>  {
> -       context->lo |= value & VTD_PAGE_MASK;
> +       context->asr = value >> VTD_PAGE_SHIFT
>  }
>
>  static inline void context_set_address_width(struct context_entry
> *context,
>                                              unsigned long value)
>  {
> -       context->hi |= value & 7;
> +       context->aw  = value;
>  }
>
>  static inline void context_set_domain_id(struct context_entry *context,
>                                          unsigned long value)
>  {
> -       context->hi |= (value & ((1 << 16) - 1)) << 8;
> +       context->did = value;
>  }
>
>  static inline void context_clear_entry(struct context_entry *context)
> --
> 1.8.4.3
>
> _______________________________________________
> iommu mailing list
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>

[-- Attachment #1.2: Type: text/html, Size: 4475 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

RE: [PATCH 1/1] x86/iommu: use bit structures for context_entry

[Wu, Feng]

[-- Attachment #1.1: Type: text/plain, Size: 4073 bytes --]

I think if using |= operation, it should use &= operation to clear those bits first.

Thanks,
Feng

From: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org [mailto:iommu-bounces@lists.linux-foundation.org] On Behalf Of Kai Huang
Sent: Thursday, December 26, 2013 9:55 AM
To: Li, Zhen-Hua
Cc: David Woodhouse; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; linux-kernel-fy+rA21nqHI@public.gmane.orgrnel.org
Subject: Re: [PATCH 1/1] x86/iommu: use bit structures for context_entry

Just curious, why would |= operation cause problem? :)

On Fri, Dec 20, 2013 at 4:45 PM, Li, Zhen-Hua <zhen-hual-VXdhtT5mjnY@public.gmane.org<mailto:zhen-hual-VXdhtT5mjnY@public.gmane.org>> wrote:
There is a structure named context_entry used by intel iommu, and there
are some bit operations on it. Use bit structure may make these operations
easy.
Also the function context_set_address_root may cause problem because it uses
 |= operation, not set the new value. And this patch can fix it.

Signed-off-by: Li, Zhen-Hua <zhen-hual-VXdhtT5mjnY@public.gmane.org<mailto:zhen-hual-VXdhtT5mjnY@public.gmane.org>>
---
 drivers/iommu/intel-iommu.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 43b9bfe..65cd480 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -221,47 +221,64 @@ get_context_addr_from_root(struct root_entry *root)
  * 8-23: domain id
  */
 struct context_entry {
-       u64 lo;
-       u64 hi;
+       union {
+               struct {
+                       u64     present:1,
+                               fpd:1,
+                               translation_type:2,
+                               reserved_l:8,
+                               asr:52;
+               };
+               u64 lo;
+       };
+       union {
+               struct {
+                       u64     aw:3,
+                               avail:4,
+                               reserved_h1:1,
+                               did:16,
+                               reserved_h2:40;
+               };
+               u64 hi;
+       };
 };

 static inline bool context_present(struct context_entry *context)
 {
-       return (context->lo & 1);
+       return context->present;
 }
 static inline void context_set_present(struct context_entry *context)
 {
-       context->lo |= 1;
+       context->present = 1;
 }

 static inline void context_set_fault_enable(struct context_entry *context)
 {
-       context->lo &= (((u64)-1) << 2) | 1;
+       context->fpd = 1;
 }

 static inline void context_set_translation_type(struct context_entry *context,
                                                unsigned long value)
 {
-       context->lo &= (((u64)-1) << 4) | 3;
-       context->lo |= (value & 3) << 2;
+       context->translation_type = value;
 }

 static inline void context_set_address_root(struct context_entry *context,
                                            unsigned long value)
 {
-       context->lo |= value & VTD_PAGE_MASK;
+       context->asr = value >> VTD_PAGE_SHIFT
 }

 static inline void context_set_address_width(struct context_entry *context,
                                             unsigned long value)
 {
-       context->hi |= value & 7;
+       context->aw  = value;
 }

 static inline void context_set_domain_id(struct context_entry *context,
                                         unsigned long value)
 {
-       context->hi |= (value & ((1 << 16) - 1)) << 8;
+       context->did = value;
 }

 static inline void context_clear_entry(struct context_entry *context)
--
1.8.4.3

_______________________________________________
iommu mailing list
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org<mailto:iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>
https://lists.linuxfoundation.org/mailman/listinfo/iommu


[-- Attachment #1.2: Type: text/html, Size: 10678 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/1] x86/iommu: use bit structures for context_entry

[Li, ZhenHua]

Yes, that's the problem. And some other structures like this, see "union 
irte".

On 12/26/2013 01:12 PM, Wu, Feng wrote:
> I think if using |= operation, it should use &= operation to clear those
> bits first.
>
> Thanks,
>
> Feng
>
> *From:*iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> [mailto:iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org] *On Behalf Of *Kai Huang
> *Sent:* Thursday, December 26, 2013 9:55 AM
> *To:* Li, Zhen-Hua
> *Cc:* David Woodhouse; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org;
> linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> *Subject:* Re: [PATCH 1/1] x86/iommu: use bit structures for context_entry
>
> Just curious, why would |= operation cause problem? :)
>
> On Fri, Dec 20, 2013 at 4:45 PM, Li, Zhen-Hua <zhen-hual-VXdhtT5mjnY@public.gmane.org
> <mailto:zhen-hual-VXdhtT5mjnY@public.gmane.org>> wrote:
>
> There is a structure named context_entry used by intel iommu, and there
> are some bit operations on it. Use bit structure may make these operations
> easy.
> Also the function context_set_address_root may cause problem because it uses
>   |= operation, not set the new value. And this patch can fix it.
>
> Signed-off-by: Li, Zhen-Hua <zhen-hual-VXdhtT5mjnY@public.gmane.org <mailto:zhen-hual-VXdhtT5mjnY@public.gmane.org>>
> ---
>   drivers/iommu/intel-iommu.c | 37 +++++++++++++++++++++++++++----------
>   1 file changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 43b9bfe..65cd480 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -221,47 +221,64 @@ get_context_addr_from_root(struct root_entry *root)
>    * 8-23: domain id
>    */
>   struct context_entry {
> -       u64 lo;
> -       u64 hi;
> +       union {
> +               struct {
> +                       u64     present:1,
> +                               fpd:1,
> +                               translation_type:2,
> +                               reserved_l:8,
> +                               asr:52;
> +               };
> +               u64 lo;
> +       };
> +       union {
> +               struct {
> +                       u64     aw:3,
> +                               avail:4,
> +                               reserved_h1:1,
> +                               did:16,
> +                               reserved_h2:40;
> +               };
> +               u64 hi;
> +       };
>   };
>
>   static inline bool context_present(struct context_entry *context)
>   {
> -       return (context->lo & 1);
> +       return context->present;
>   }
>   static inline void context_set_present(struct context_entry *context)
>   {
> -       context->lo |= 1;
> +       context->present = 1;
>   }
>
>   static inline void context_set_fault_enable(struct context_entry *context)
>   {
> -       context->lo &= (((u64)-1) << 2) | 1;
> +       context->fpd = 1;
>   }
>
>   static inline void context_set_translation_type(struct context_entry
> *context,
>                                                  unsigned long value)
>   {
> -       context->lo &= (((u64)-1) << 4) | 3;
> -       context->lo |= (value & 3) << 2;
> +       context->translation_type = value;
>   }
>
>   static inline void context_set_address_root(struct context_entry *context,
>                                              unsigned long value)
>   {
> -       context->lo |= value & VTD_PAGE_MASK;
> +       context->asr = value >> VTD_PAGE_SHIFT
>   }
>
>   static inline void context_set_address_width(struct context_entry
> *context,
>                                               unsigned long value)
>   {
> -       context->hi |= value & 7;
> +       context->aw  = value;
>   }
>
>   static inline void context_set_domain_id(struct context_entry *context,
>                                           unsigned long value)
>   {
> -       context->hi |= (value & ((1 << 16) - 1)) << 8;
> +       context->did = value;
>   }
>
>   static inline void context_clear_entry(struct context_entry *context)
> --
> 1.8.4.3
>
> _______________________________________________
> iommu mailing list
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org <mailto:iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>

Re: [PATCH 1/1] x86/iommu: use bit structures for context_entry

[Joerg Roedel]

On Fri, Dec 20, 2013 at 04:45:01PM +0800, Li, Zhen-Hua wrote:
> There is a structure named context_entry used by intel iommu, and there
> are some bit operations on it. Use bit structure may make these operations
> easy.
> Also the function context_set_address_root may cause problem because it uses
>  |= operation, not set the new value. And this patch can fix it.

What is the problem you are trying to fix here? Is it an actual bug?


	Joerg

Re: [PATCH 1/1] x86/iommu: use bit structures for context_entry

[Li, ZhenHua]

I have not seen such a bug yet . but obviously a '=' should be used when 
you want to set a value.

for example, if x != 0,
	x=10
and
	x|=10
will cause different result.

ZhenHua

On 01/07/2014 10:41 PM, Joerg Roedel wrote:
> On Fri, Dec 20, 2013 at 04:45:01PM +0800, Li, Zhen-Hua wrote:
>> There is a structure named context_entry used by intel iommu, and there
>> are some bit operations on it. Use bit structure may make these operations
>> easy.
>> Also the function context_set_address_root may cause problem because it uses
>>   |= operation, not set the new value. And this patch can fix it.
>
> What is the problem you are trying to fix here? Is it an actual bug?
>
>
> 	Joerg
>
>

Re: [PATCH 1/1] x86/iommu: use bit structures for context_entry

[Jiang Liu]

I have noticed the same issue too. But after second check, seems
it's safe with current implementation, but obviously it's not
future safe.

On 2014/1/10 15:29, Li, ZhenHua wrote:
> I have not seen such a bug yet . but obviously a '=' should be used when
> you want to set a value.
> 
> for example, if x != 0,
>     x=10
> and
>     x|=10
> will cause different result.
> 
> ZhenHua
> 
> On 01/07/2014 10:41 PM, Joerg Roedel wrote:
>> On Fri, Dec 20, 2013 at 04:45:01PM +0800, Li, Zhen-Hua wrote:
>>> There is a structure named context_entry used by intel iommu, and there
>>> are some bit operations on it. Use bit structure may make these
>>> operations
>>> easy.
>>> Also the function context_set_address_root may cause problem because
>>> it uses
>>>   |= operation, not set the new value. And this patch can fix it.
>>
>> What is the problem you are trying to fix here? Is it an actual bug?
>>
>>
>>     Joerg
>>
>>
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/1] x86/iommu: use bit structures for context_entry

[Li, Zhen-Hua]

There is a structure named context_entry used by intel iommu, and there
are some bit operations on it. Use bit structure may make these operations
easy.
Also the function context_set_address_root may cause problem because it uses
 |= operation, not set the new value. And this patch can fix it.

Signed-off-by: Li, Zhen-Hua <zhen-hual-VXdhtT5mjnY@public.gmane.org>
---
 drivers/iommu/intel-iommu.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 43b9bfe..65cd480 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -221,47 +221,64 @@ get_context_addr_from_root(struct root_entry *root)
  * 8-23: domain id
  */
 struct context_entry {
-	u64 lo;
-	u64 hi;
+	union {
+		struct {
+			u64	present:1,
+				fpd:1,
+				translation_type:2,
+				reserved_l:8,
+				asr:52;
+		};
+		u64 lo;
+	};
+	union {
+		struct {
+			u64	aw:3,
+				avail:4,
+				reserved_h1:1,
+				did:16,
+				reserved_h2:40;
+		};
+		u64 hi;
+	};
 };
 
 static inline bool context_present(struct context_entry *context)
 {
-	return (context->lo & 1);
+	return context->present;
 }
 static inline void context_set_present(struct context_entry *context)
 {
-	context->lo |= 1;
+	context->present = 1;
 }
 
 static inline void context_set_fault_enable(struct context_entry *context)
 {
-	context->lo &= (((u64)-1) << 2) | 1;
+	context->fpd = 1;
 }
 
 static inline void context_set_translation_type(struct context_entry *context,
 						unsigned long value)
 {
-	context->lo &= (((u64)-1) << 4) | 3;
-	context->lo |= (value & 3) << 2;
+	context->translation_type = value;
 }
 
 static inline void context_set_address_root(struct context_entry *context,
 					    unsigned long value)
 {
-	context->lo |= value & VTD_PAGE_MASK;
+	context->asr = value >> VTD_PAGE_SHIFT;
 }
 
 static inline void context_set_address_width(struct context_entry *context,
 					     unsigned long value)
 {
-	context->hi |= value & 7;
+	context->aw  = value;
 }
 
 static inline void context_set_domain_id(struct context_entry *context,
 					 unsigned long value)
 {
-	context->hi |= (value & ((1 << 16) - 1)) << 8;
+	context->did = value;
 }
 
 static inline void context_clear_entry(struct context_entry *context)
-- 
1.8.4.3

Re: [PATCH 1/1] x86/iommu: use bit structures for context_entry

[Li, ZhenHua]

This patch is sent again because a ';' was missed in my last mail.

 > +	context->asr = value >> VTD_PAGE_SHIFT

I corrected it and created a new one. And it is tested .
Please use the latest one.


Sorry for that.

ZhenHua

On 01/10/2014 04:27 PM, Li, Zhen-Hua wrote:
> There is a structure named context_entry used by intel iommu, and there
> are some bit operations on it. Use bit structure may make these operations
> easy.
> Also the function context_set_address_root may cause problem because it uses
>   |= operation, not set the new value. And this patch can fix it.
>
> Signed-off-by: Li, Zhen-Hua <zhen-hual-VXdhtT5mjnY@public.gmane.org>
> ---
>   drivers/iommu/intel-iommu.c | 37 +++++++++++++++++++++++++++----------
>   1 file changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 43b9bfe..65cd480 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -221,47 +221,64 @@ get_context_addr_from_root(struct root_entry *root)
>    * 8-23: domain id
>    */
>   struct context_entry {
> -	u64 lo;
> -	u64 hi;
> +	union {
> +		struct {
> +			u64	present:1,
> +				fpd:1,
> +				translation_type:2,
> +				reserved_l:8,
> +				asr:52;
> +		};
> +		u64 lo;
> +	};
> +	union {
> +		struct {
> +			u64	aw:3,
> +				avail:4,
> +				reserved_h1:1,
> +				did:16,
> +				reserved_h2:40;
> +		};
> +		u64 hi;
> +	};
>   };
>
>   static inline bool context_present(struct context_entry *context)
>   {
> -	return (context->lo & 1);
> +	return context->present;
>   }
>   static inline void context_set_present(struct context_entry *context)
>   {
> -	context->lo |= 1;
> +	context->present = 1;
>   }
>
>   static inline void context_set_fault_enable(struct context_entry *context)
>   {
> -	context->lo &= (((u64)-1) << 2) | 1;
> +	context->fpd = 1;
>   }
>
>   static inline void context_set_translation_type(struct context_entry *context,
>   						unsigned long value)
>   {
> -	context->lo &= (((u64)-1) << 4) | 3;
> -	context->lo |= (value & 3) << 2;
> +	context->translation_type = value;
>   }
>
>   static inline void context_set_address_root(struct context_entry *context,
>   					    unsigned long value)
>   {
> -	context->lo |= value & VTD_PAGE_MASK;
> +	context->asr = value >> VTD_PAGE_SHIFT;
>   }
>
>   static inline void context_set_address_width(struct context_entry *context,
>   					     unsigned long value)
>   {
> -	context->hi |= value & 7;
> +	context->aw  = value;
>   }
>
>   static inline void context_set_domain_id(struct context_entry *context,
>   					 unsigned long value)
>   {
> -	context->hi |= (value & ((1 << 16) - 1)) << 8;
> +	context->did = value;
>   }
>
>   static inline void context_clear_entry(struct context_entry *context)
>