On Mon, 4 Jan 2021 at 17:03, Raghu Krishnamurthy via Hafnium < hafnium@lists.trustedfirmware.org> wrote:
Hi All,
I was going through src/mm.c in the hafnium repo and noticed a couple of things that I think are problem:
mm_vm_defrag and mm_defrag, which eventually call mm_ptable_defragappears to be changing page entries into block entries. Per D5.10.1 of ARM ARM, this should use break-before-make, but I don't see it being done in code. Am I missing where this is done in code, or is there some reason break-before-make is not required during defrag(that is not obvious to me from code or comments)? 2. In mm_replace_entry, line 278 (https://git.trustedfirmware.org/hafnium/hafnium.git/tree/src/mm.c#n278) is not followed by a DSB + ISB as required by the break-before-make. Without the DSB, a load/store that uses a VA translated by the PTE being replaced could be reordered before line 278 and fault. Once again, this looks like a bug to me but is there some reason that is not obvious due to which it does not require barriers?
It looks like that code was added in https://hafnium-review.googlesource.com/c/hafnium/+/3260. Maybe Andrew Scull or Wedson can comment, if they can remember?
I was going through src/mm.c in the hafnium repo and noticed a couple of things that I think are problem:
mm_vm_defrag and mm_defrag, which eventually call mm_ptable_defragappears to be changing page entries into block entries. Per D5.10.1 of ARM ARM, this should use break-before-make, but I don't see it being done in code. Am I missing where this is done in code, or is there some reason break-before-make is not required during defrag(that is not obvious to me from code or comments)?
This does seem like it's just wrong. Despite there not being a change in the attributes, the ARM does say that a change of mapping size should also break-before-make, but that's not what it does. The places that mm_ptable_defrag_entry() is being assigned should also be using mm_replace_entry() if there was a change and there are likely some missing barriers there too.
In mm_replace_entry, line 278(https://git.trustedfirmware.org/hafnium/hafnium.git/tree/src/mm.c#n278) is not followed by a DSB + ISB as required by the break-before-make. Without the DSB, a load/store that uses a VA translated by the PTE being replaced could be reordered before line 278 and fault. Once again, this looks like a bug to me but is there some reason that is not obvious due to which it does not require barriers?
From a quick scan, it looks like the barriers in this case are in the
arch code [1] for the TLB invalidation. The intermediate ones happen as the break is being made and there's a final invalidation after the entire mapping has been completed [2].
[1] -- https://git.trustedfirmware.org/hafnium/hafnium.git/tree/src/arch/aarch64/mm... [2] -- https://git.trustedfirmware.org/hafnium/hafnium.git/tree/src/mm.c#n478
Thanks Andrew.
The places that mm_ptable_defrag_entry() is assigned should also be using mm_replace_entry() if there was a change and there are likely some missing barriers there too.
[RK] Agree. Can you elaborate on what you mean by "there are likely some missing barriers there too"? Are you talking about the DSB+ISB missing after line 280 in src/arch/aarch64/mm.c? It seems like if you change the assignments you referred to with mm_replace_entry() and change mm_ptable_defrag to invalidate TLB(possibly the entire TLB for simplicity) or add a DSB+ISB at the end of the function should give a working implementation.
there's a final invalidation after the entire mapping has been completed [2].
[RK] Agree. This works. However, is an invalidation really required if we did use break-before-make or add new entries? Break-before-make invalidates replaced entries and the architecture does not allow invalid translations to be cached in the TLB. With respect to line 478 in src/mm.c, It seemed odd to me to rely on that line for the final barriers, since it is under an if statement. It appears that after one_time_init, we always invalidate page tables whether it is new mappings or replacing mappings. What is the reason we wanted to delay invalidation until end of one_time_init? Did it give a significant performance boost? It seems cleaner to me to explicitly have a DSB+ISB before line 478 or remove the if condition around line 478 and have some code comments about previous PTE writes becoming entirely visible after the invalidation.
Let me know what you think.
Thanks Raghu
-----Original Message----- From: Andrew Scull ascull@google.com Sent: Tuesday, January 5, 2021 8:00 AM To: Andrew Walbran qwandor@google.com Cc: Raghu K raghu.ncstate@icloud.com; Wedson Almeida Filho wedsonaf@google.com; hafnium@lists.trustedfirmware.org Subject: Re: [Hafnium] Possible bugs with page table management
I was going through src/mm.c in the hafnium repo and noticed a couple of things that I think are problem:
mm_vm_defrag and mm_defrag, which eventually call mm_ptable_defragappears to be changing page entries into block entries. Per D5.10.1 of ARM ARM, this should use break-before-make, but I don't see it being done in code. Am I missing where this is done in code, or is there some reason break-before-make is not required during defrag(that is not obvious to me from code or comments)?
This does seem like it's just wrong. Despite there not being a change in the attributes, the ARM does say that a change of mapping size should also break-before-make, but that's not what it does. The places that mm_ptable_defrag_entry() is being assigned should also be using mm_replace_entry() if there was a change and there are likely some missing barriers there too.
In mm_replace_entry, line 278(https://git.trustedfirmware.org/hafnium/hafnium.git/tree/src/mm.c#n2 78) is not followed by a DSB + ISB as required by the break-before-make. Without the DSB, a load/store that uses a VA translated by the PTE being replaced could be reordered before line 278 and fault. Once again, this looks like a bug to me but is there some reason that is not obvious due to which it does not require barriers?
From a quick scan, it looks like the barriers in this case are in the arch code [1] for the TLB invalidation. The intermediate ones happen as the break is being made and there's a final invalidation after the entire mapping has been completed [2].
[1] -- https://git.trustedfirmware.org/hafnium/hafnium.git/tree/src/arch/aarch64/mm... [2] -- https://git.trustedfirmware.org/hafnium/hafnium.git/tree/src/mm.c#n478
there's a final invalidation after the entire mapping has been completed [2].
[RK] Agree. This works. However, is an invalidation really required if we did use break-before-make or add new entries? Break-before-make invalidates replaced entries and the architecture does not allow invalid translations to be cached in the TLB. With respect to line 478 in src/mm.c, It seemed odd to me to rely on that line for the final barriers, since it is under an if statement. It appears that after one_time_init, we always invalidate page tables whether it is new mappings or replacing mappings. What is the reason we wanted to delay invalidation until end of one_time_init? Did it give a significant performance boost? It seems cleaner to me to explicitly have a DSB+ISB before line 478 or remove the if condition around line 478 and have some code comments about previous PTE writes becoming entirely visible after the invalidation.
You're absolutely right that it's a roundabout way of hitting the right instructions (possibly because the non-existent x86 was in mind?). Most of the table updates did two passes, one to check the memory allocations and the second to commit the actual changes to the address space, hence the if statement. I don't know any good reason for the TLB invalidation being done in that way, it certainly wasn't a measured performance benefit
Let me know what you think.
I think things could be made much clearer, especially if an x86 port is no longer going to be an objective. Some of the arch abstraction here makes things more confusing.
I'd also wonder whether an existing page table library, e.g. xlat_v2 from TF-A, would be applicable so we can reduce, or at least concentrate, the amount of buggy PT code in the world. But I suspect there are issues with needing stage 1 and 2 being handled.
Thanks Andrew. I like the xlat_v2 suggestion. Would be interesting to see what others think of using it in hafnium. At first thought, It seems like a fairly large change to import and start using it in hafnium, not to mention having to extend xlat_v2 to have stage 2 support but definitely worth exploring. Assuming xlat_v2 or another alternative is more of a long term thing, in the near term, do we want to submit patches to fix the below issues? I'd be happy to submit patches if you’d like.
-----Original Message----- From: Andrew Scull ascull@google.com Sent: Tuesday, January 5, 2021 11:59 AM To: Raghu K raghu.ncstate@icloud.com Cc: Andrew Walbran qwandor@google.com; Wedson Almeida Filho wedsonaf@google.com; hafnium@lists.trustedfirmware.org Subject: Re: [Hafnium] Possible bugs with page table management
there's a final invalidation after the entire mapping has been completed [2].
[RK] Agree. This works. However, is an invalidation really required if we did use break-before-make or add new entries? Break-before-make invalidates replaced entries and the architecture does not allow invalid translations to be cached in the TLB. With respect to line 478 in src/mm.c, It seemed odd to me to rely on that line for the final barriers, since it is under an if statement. It appears that after one_time_init, we always invalidate page tables whether it is new mappings or replacing mappings. What is the reason we wanted to delay invalidation until end of one_time_init? Did it give a significant performance boost? It seems cleaner to me to explicitly have a DSB+ISB before line 478 or remove the if condition around line 478 and have some code comments about previous PTE writes becoming entirely visible after the invalidation.
You're absolutely right that it's a roundabout way of hitting the right instructions (possibly because the non-existent x86 was in mind?). Most of the table updates did two passes, one to check the memory allocations and the second to commit the actual changes to the address space, hence the if statement. I don't know any good reason for the TLB invalidation being done in that way, it certainly wasn't a measured performance benefit
Let me know what you think.
I think things could be made much clearer, especially if an x86 port is no longer going to be an objective. Some of the arch abstraction here makes things more confusing.
I'd also wonder whether an existing page table library, e.g. xlat_v2 from TF-A, would be applicable so we can reduce, or at least concentrate, the amount of buggy PT code in the world. But I suspect there are issues with needing stage 1 and 2 being handled.
Yes, please do submit patches, that would be great!
On Tue, 5 Jan 2021 at 22:13, raghu.ncstate@icloud.com wrote:
Thanks Andrew. I like the xlat_v2 suggestion. Would be interesting to see what others think of using it in hafnium. At first thought, It seems like a fairly large change to import and start using it in hafnium, not to mention having to extend xlat_v2 to have stage 2 support but definitely worth exploring. Assuming xlat_v2 or another alternative is more of a long term thing, in the near term, do we want to submit patches to fix the below issues? I'd be happy to submit patches if you’d like.
-----Original Message----- From: Andrew Scull ascull@google.com Sent: Tuesday, January 5, 2021 11:59 AM To: Raghu K raghu.ncstate@icloud.com Cc: Andrew Walbran qwandor@google.com; Wedson Almeida Filho wedsonaf@google.com; hafnium@lists.trustedfirmware.org Subject: Re: [Hafnium] Possible bugs with page table management
there's a final invalidation after the entire mapping has been completed [2].
[RK] Agree. This works. However, is an invalidation really required if we did use break-before-make or add new entries? Break-before-make invalidates replaced entries and the architecture does not allow invalid translations to be cached in the TLB. With respect to line 478 in src/mm.c, It seemed odd to me to rely on that line for the final barriers, since it is under an if statement. It appears that after one_time_init, we always invalidate page tables whether it is new mappings or replacing mappings. What is the reason we wanted to delay invalidation until end of one_time_init? Did it give a significant performance boost? It seems cleaner to me to explicitly have a DSB+ISB before line 478 or remove the if condition around line 478 and have some code comments about previous PTE writes becoming entirely visible after the invalidation.
You're absolutely right that it's a roundabout way of hitting the right instructions (possibly because the non-existent x86 was in mind?). Most of the table updates did two passes, one to check the memory allocations and the second to commit the actual changes to the address space, hence the if statement. I don't know any good reason for the TLB invalidation being done in that way, it certainly wasn't a measured performance benefit
Let me know what you think.
I think things could be made much clearer, especially if an x86 port is no longer going to be an objective. Some of the arch abstraction here makes things more confusing.
I'd also wonder whether an existing page table library, e.g. xlat_v2 from TF-A, would be applicable so we can reduce, or at least concentrate, the amount of buggy PT code in the world. But I suspect there are issues with needing stage 1 and 2 being handled.
Uploaded https://review.trustedfirmware.org/c/hafnium/hafnium/+/8089 in an effort to try and fix this issue.
Thanks Raghu
-----Original Message----- From: Andrew Scull ascull@google.com Sent: Wednesday, January 6, 2021 5:02 AM To: Raghu K raghu.ncstate@icloud.com Cc: Andrew Walbran qwandor@google.com; Wedson Almeida Filho wedsonaf@google.com; hafnium@lists.trustedfirmware.org Subject: Re: [Hafnium] Possible bugs with page table management
Yes, please do submit patches, that would be great!
On Tue, 5 Jan 2021 at 22:13, raghu.ncstate@icloud.com wrote:
Thanks Andrew. I like the xlat_v2 suggestion. Would be interesting to see what others think of using it in hafnium. At first thought, It seems like a fairly large change to import and start using it in hafnium, not to mention having to extend xlat_v2 to have stage 2 support but definitely worth exploring. Assuming xlat_v2 or another alternative is more of a long term thing, in the near term, do we want to submit patches to fix the below issues? I'd be happy to submit patches if you’d like.
-----Original Message----- From: Andrew Scull ascull@google.com Sent: Tuesday, January 5, 2021 11:59 AM To: Raghu K raghu.ncstate@icloud.com Cc: Andrew Walbran qwandor@google.com; Wedson Almeida Filho wedsonaf@google.com; hafnium@lists.trustedfirmware.org Subject: Re: [Hafnium] Possible bugs with page table management
there's a final invalidation after the entire mapping has been completed [2].
[RK] Agree. This works. However, is an invalidation really required if we did use break-before-make or add new entries? Break-before-make invalidates replaced entries and the architecture does not allow invalid translations to be cached in the TLB. With respect to line 478 in src/mm.c, It seemed odd to me to rely on that line for the final barriers, since it is under an if statement. It appears that after one_time_init, we always invalidate page tables whether it is new mappings or replacing mappings. What is the reason we wanted to delay invalidation until end of one_time_init? Did it give a significant performance boost? It seems cleaner to me to explicitly have a DSB+ISB before line 478 or remove the if condition around line 478 and have some code comments about previous PTE writes becoming entirely visible after the invalidation.
You're absolutely right that it's a roundabout way of hitting the right instructions (possibly because the non-existent x86 was in mind?). Most of the table updates did two passes, one to check the memory allocations and the second to commit the actual changes to the address space, hence the if statement. I don't know any good reason for the TLB invalidation being done in that way, it certainly wasn't a measured performance benefit
Let me know what you think.
I think things could be made much clearer, especially if an x86 port is no longer going to be an objective. Some of the arch abstraction here makes things more confusing.
I'd also wonder whether an existing page table library, e.g. xlat_v2 from TF-A, would be applicable so we can reduce, or at least concentrate, the amount of buggy PT code in the world. But I suspect there are issues with needing stage 1 and 2 being handled.
Maybe you are better placed than me to review this fix, Andrew?
On Mon, 25 Jan 2021 at 03:46, raghu.ncstate@icloud.com wrote:
Uploaded https://review.trustedfirmware.org/c/hafnium/hafnium/+/8089 in an effort to try and fix this issue.
Thanks Raghu
-----Original Message----- From: Andrew Scull ascull@google.com Sent: Wednesday, January 6, 2021 5:02 AM To: Raghu K raghu.ncstate@icloud.com Cc: Andrew Walbran qwandor@google.com; Wedson Almeida Filho < wedsonaf@google.com>; hafnium@lists.trustedfirmware.org Subject: Re: [Hafnium] Possible bugs with page table management
Yes, please do submit patches, that would be great!
On Tue, 5 Jan 2021 at 22:13, raghu.ncstate@icloud.com wrote:
Thanks Andrew. I like the xlat_v2 suggestion. Would be interesting to
see what others think of using it in hafnium. At first thought, It seems like a fairly large change to import and start using it in hafnium, not to mention having to extend xlat_v2 to have stage 2 support but definitely worth exploring.
Assuming xlat_v2 or another alternative is more of a long term thing, in
the near term, do we want to submit patches to fix the below issues? I'd be happy to submit patches if you’d like.
-----Original Message----- From: Andrew Scull ascull@google.com Sent: Tuesday, January 5, 2021 11:59 AM To: Raghu K raghu.ncstate@icloud.com Cc: Andrew Walbran qwandor@google.com; Wedson Almeida Filho <
wedsonaf@google.com>; hafnium@lists.trustedfirmware.org
Subject: Re: [Hafnium] Possible bugs with page table management
there's a final invalidation after the entire mapping has been
completed [2].
[RK] Agree. This works. However, is an invalidation really required if
we did use break-before-make or add new entries? Break-before-make invalidates replaced entries and the architecture does not allow invalid translations to be cached in the TLB. With respect to line 478 in src/mm.c, It seemed odd to me to rely on that line for the final barriers, since it is under an if statement. It appears that after one_time_init, we always invalidate page tables whether it is new mappings or replacing mappings. What is the reason we wanted to delay invalidation until end of one_time_init? Did it give a significant performance boost?
It seems cleaner to me to explicitly have a DSB+ISB before line 478 or
remove the if condition around line 478 and have some code comments about previous PTE writes becoming entirely visible after the invalidation.
You're absolutely right that it's a roundabout way of hitting the right
instructions (possibly because the non-existent x86 was in mind?). Most of the table updates did two passes, one to check the memory allocations and the second to commit the actual changes to the address space, hence the if statement. I don't know any good reason for the TLB invalidation being done in that way, it certainly wasn't a measured performance benefit
Let me know what you think.
I think things could be made much clearer, especially if an x86 port is
no longer going to be an objective. Some of the arch abstraction here makes things more confusing.
I'd also wonder whether an existing page table library, e.g. xlat_v2
from TF-A, would be applicable so we can reduce, or at least concentrate, the amount of buggy PT code in the world. But I suspect there are issues with needing stage 1 and 2 being handled.
Hi Andrew,
I’ve uploaded new patches based on your comments and had further questions. Would appreciate it if you can respond, when you get a chance.
Thanks
Raghu
From: Andrew Walbran qwandor@google.com Sent: Thursday, January 28, 2021 8:58 AM To: Raghu K raghu.ncstate@icloud.com Cc: Andrew Scull ascull@google.com; Wedson Almeida Filho wedsonaf@google.com; hafnium@lists.trustedfirmware.org Subject: Re: [Hafnium] Possible bugs with page table management
Maybe you are better placed than me to review this fix, Andrew?
On Mon, 25 Jan 2021 at 03:46, <raghu.ncstate@icloud.com mailto:raghu.ncstate@icloud.com > wrote:
Uploaded https://review.trustedfirmware.org/c/hafnium/hafnium/+/8089 in an effort to try and fix this issue.
Thanks Raghu
-----Original Message----- From: Andrew Scull <ascull@google.com mailto:ascull@google.com > Sent: Wednesday, January 6, 2021 5:02 AM To: Raghu K <raghu.ncstate@icloud.com mailto:raghu.ncstate@icloud.com > Cc: Andrew Walbran <qwandor@google.com mailto:qwandor@google.com >; Wedson Almeida Filho <wedsonaf@google.com mailto:wedsonaf@google.com >; hafnium@lists.trustedfirmware.org mailto:hafnium@lists.trustedfirmware.org Subject: Re: [Hafnium] Possible bugs with page table management
Yes, please do submit patches, that would be great!
On Tue, 5 Jan 2021 at 22:13, <raghu.ncstate@icloud.com mailto:raghu.ncstate@icloud.com > wrote:
Thanks Andrew. I like the xlat_v2 suggestion. Would be interesting to see what others think of using it in hafnium. At first thought, It seems like a fairly large change to import and start using it in hafnium, not to mention having to extend xlat_v2 to have stage 2 support but definitely worth exploring. Assuming xlat_v2 or another alternative is more of a long term thing, in the near term, do we want to submit patches to fix the below issues? I'd be happy to submit patches if you’d like.
-----Original Message----- From: Andrew Scull <ascull@google.com mailto:ascull@google.com > Sent: Tuesday, January 5, 2021 11:59 AM To: Raghu K <raghu.ncstate@icloud.com mailto:raghu.ncstate@icloud.com > Cc: Andrew Walbran <qwandor@google.com mailto:qwandor@google.com >; Wedson Almeida Filho <wedsonaf@google.com mailto:wedsonaf@google.com >; hafnium@lists.trustedfirmware.org mailto:hafnium@lists.trustedfirmware.org Subject: Re: [Hafnium] Possible bugs with page table management
there's a final invalidation after the entire mapping has been completed [2].
[RK] Agree. This works. However, is an invalidation really required if we did use break-before-make or add new entries? Break-before-make invalidates replaced entries and the architecture does not allow invalid translations to be cached in the TLB. With respect to line 478 in src/mm.c, It seemed odd to me to rely on that line for the final barriers, since it is under an if statement. It appears that after one_time_init, we always invalidate page tables whether it is new mappings or replacing mappings. What is the reason we wanted to delay invalidation until end of one_time_init? Did it give a significant performance boost? It seems cleaner to me to explicitly have a DSB+ISB before line 478 or remove the if condition around line 478 and have some code comments about previous PTE writes becoming entirely visible after the invalidation.
You're absolutely right that it's a roundabout way of hitting the right instructions (possibly because the non-existent x86 was in mind?). Most of the table updates did two passes, one to check the memory allocations and the second to commit the actual changes to the address space, hence the if statement. I don't know any good reason for the TLB invalidation being done in that way, it certainly wasn't a measured performance benefit
Let me know what you think.
I think things could be made much clearer, especially if an x86 port is no longer going to be an objective. Some of the arch abstraction here makes things more confusing.
I'd also wonder whether an existing page table library, e.g. xlat_v2 from TF-A, would be applicable so we can reduce, or at least concentrate, the amount of buggy PT code in the world. But I suspect there are issues with needing stage 1 and 2 being handled.
hafnium@lists.trustedfirmware.org
-
Andrew Scull -
Andrew Walbran -
raghu.ncstate＠icloud.com