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 <[email protected]>
---
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
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(...))
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(...))
>
>
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:*[email protected]
> [mailto:[email protected]] *On Behalf Of *Kai Huang
> *Sent:* Thursday, December 26, 2013 9:55 AM
> *To:* Li, Zhen-Hua
> *Cc:* David Woodhouse; [email protected];
> [email protected]
> *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 <[email protected]
> <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>>
> ---
> 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
> [email protected] <mailto:[email protected]>
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>
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
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
>
>
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/