* [PATCH v1 1/1] target/riscv: Don't set write permissions on dirty PTEs
@ 2020-03-04 1:16 ` Alistair Francis
0 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2020-03-04 1:16 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair.francis, palmer, alistair23
The RISC-V spec specifies that when a write happens and the D bit is
clear the implementation will set the bit in the PTE. It does not
describe that the PTE being dirty means that we should provide write
access. This patch removes the write access granted to pages when the
dirty bit is set.
Following the prot variable we can see that it affects all of these
functions:
riscv_cpu_tlb_fill()
tlb_set_page()
tlb_set_page_with_attrs()
address_space_translate_for_iotlb()
Looking at the cputlb code (tlb_set_page_with_attrs() and
address_space_translate_for_iotlb()) it looks like the main affect of
setting write permissions is that the page can be marked as TLB_NOTDIRTY.
I don't see any other impacts (related to the dirty bit) for giving a
page write permissions.
Setting write permission on dirty PTEs results in userspace inside a
Hypervisor guest (VU) becoming corrupted. This appears to be because it
ends up with write permission in the second stage translation in cases
where we aren't doing a store.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
target/riscv/cpu_helper.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 5ea5d133aa..cc9f20b471 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -572,10 +572,8 @@ restart:
if ((pte & PTE_X)) {
*prot |= PAGE_EXEC;
}
- /* add write permission on stores or if the page is already dirty,
- so that we TLB miss on later writes to update the dirty bit */
- if ((pte & PTE_W) &&
- (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
+ /* add write permission on stores */
+ if ((pte & PTE_W) && (access_type == MMU_DATA_STORE)) {
*prot |= PAGE_WRITE;
}
return TRANSLATE_SUCCESS;
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v1 1/1] target/riscv: Don't set write permissions on dirty PTEs
@ 2020-03-04 1:16 ` Alistair Francis
0 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2020-03-04 1:16 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: palmer, alistair.francis, alistair23
The RISC-V spec specifies that when a write happens and the D bit is
clear the implementation will set the bit in the PTE. It does not
describe that the PTE being dirty means that we should provide write
access. This patch removes the write access granted to pages when the
dirty bit is set.
Following the prot variable we can see that it affects all of these
functions:
riscv_cpu_tlb_fill()
tlb_set_page()
tlb_set_page_with_attrs()
address_space_translate_for_iotlb()
Looking at the cputlb code (tlb_set_page_with_attrs() and
address_space_translate_for_iotlb()) it looks like the main affect of
setting write permissions is that the page can be marked as TLB_NOTDIRTY.
I don't see any other impacts (related to the dirty bit) for giving a
page write permissions.
Setting write permission on dirty PTEs results in userspace inside a
Hypervisor guest (VU) becoming corrupted. This appears to be because it
ends up with write permission in the second stage translation in cases
where we aren't doing a store.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
---
target/riscv/cpu_helper.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 5ea5d133aa..cc9f20b471 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -572,10 +572,8 @@ restart:
if ((pte & PTE_X)) {
*prot |= PAGE_EXEC;
}
- /* add write permission on stores or if the page is already dirty,
- so that we TLB miss on later writes to update the dirty bit */
- if ((pte & PTE_W) &&
- (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
+ /* add write permission on stores */
+ if ((pte & PTE_W) && (access_type == MMU_DATA_STORE)) {
*prot |= PAGE_WRITE;
}
return TRANSLATE_SUCCESS;
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v1 1/1] target/riscv: Don't set write permissions on dirty PTEs
2020-03-04 1:16 ` Alistair Francis
@ 2020-03-04 17:34 ` Richard Henderson
-1 siblings, 0 replies; 14+ messages in thread
From: Richard Henderson @ 2020-03-04 17:34 UTC (permalink / raw)
To: Alistair Francis, qemu-devel, qemu-riscv; +Cc: alistair23, palmer
On 3/3/20 5:16 PM, Alistair Francis wrote:
> The RISC-V spec specifies that when a write happens and the D bit is
> clear the implementation will set the bit in the PTE. It does not
> describe that the PTE being dirty means that we should provide write
> access. This patch removes the write access granted to pages when the
> dirty bit is set.
The W bit by itself says we should provide write access.
It is an implementation detail that we *withhold* write access when D is clear
(etc) so that we can trap, so that we can properly set D in the future.
The page table walk associated with a read is allowed to cache all of the
information it finds in the PTE during that walk. Which includes the D bit.
If D is set, then we do not need to set it in future, so we do not need to
trap, so we can immediately honor the W bit.
If the guest changes R/W/X within a PTE (e.g. for mprotect), it is obvious that
a TLB flush for that page must be done. It is no different if the guest
changes A/D (e.g. for swapping).
> Setting write permission on dirty PTEs results in userspace inside a
> Hypervisor guest (VU) becoming corrupted. This appears to be because it
> ends up with write permission in the second stage translation in cases
> where we aren't doing a store.
You've not really given any more information than last time this patch came around.
I still think this must be a guest (or nested guest) bug related to clearing
PTE bits and failing to flush the TLB properly.
I don't see how it could be a qemu tlb flushing bug. The only primitive,
sfence.vma, is quite heavy-handed and explicitly local to the thread.
It may be a bug in qemu's implementation of second stage paging. Which is not
yet upstream, and I haven't gone digging in the archives to find the patch.
r~
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 1/1] target/riscv: Don't set write permissions on dirty PTEs
@ 2020-03-04 17:34 ` Richard Henderson
0 siblings, 0 replies; 14+ messages in thread
From: Richard Henderson @ 2020-03-04 17:34 UTC (permalink / raw)
To: Alistair Francis, qemu-devel, qemu-riscv; +Cc: palmer, alistair23
On 3/3/20 5:16 PM, Alistair Francis wrote:
> The RISC-V spec specifies that when a write happens and the D bit is
> clear the implementation will set the bit in the PTE. It does not
> describe that the PTE being dirty means that we should provide write
> access. This patch removes the write access granted to pages when the
> dirty bit is set.
The W bit by itself says we should provide write access.
It is an implementation detail that we *withhold* write access when D is clear
(etc) so that we can trap, so that we can properly set D in the future.
The page table walk associated with a read is allowed to cache all of the
information it finds in the PTE during that walk. Which includes the D bit.
If D is set, then we do not need to set it in future, so we do not need to
trap, so we can immediately honor the W bit.
If the guest changes R/W/X within a PTE (e.g. for mprotect), it is obvious that
a TLB flush for that page must be done. It is no different if the guest
changes A/D (e.g. for swapping).
> Setting write permission on dirty PTEs results in userspace inside a
> Hypervisor guest (VU) becoming corrupted. This appears to be because it
> ends up with write permission in the second stage translation in cases
> where we aren't doing a store.
You've not really given any more information than last time this patch came around.
I still think this must be a guest (or nested guest) bug related to clearing
PTE bits and failing to flush the TLB properly.
I don't see how it could be a qemu tlb flushing bug. The only primitive,
sfence.vma, is quite heavy-handed and explicitly local to the thread.
It may be a bug in qemu's implementation of second stage paging. Which is not
yet upstream, and I haven't gone digging in the archives to find the patch.
r~
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 1/1] target/riscv: Don't set write permissions on dirty PTEs
2020-03-04 17:34 ` Richard Henderson
@ 2020-03-12 22:10 ` Alistair Francis
-1 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2020-03-12 22:10 UTC (permalink / raw)
To: Richard Henderson
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers
On Wed, Mar 4, 2020 at 9:34 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/3/20 5:16 PM, Alistair Francis wrote:
> > The RISC-V spec specifies that when a write happens and the D bit is
> > clear the implementation will set the bit in the PTE. It does not
> > describe that the PTE being dirty means that we should provide write
> > access. This patch removes the write access granted to pages when the
> > dirty bit is set.
>
> The W bit by itself says we should provide write access.
>
> It is an implementation detail that we *withhold* write access when D is clear
> (etc) so that we can trap, so that we can properly set D in the future.
>
> The page table walk associated with a read is allowed to cache all of the
> information it finds in the PTE during that walk. Which includes the D bit.
> If D is set, then we do not need to set it in future, so we do not need to
> trap, so we can immediately honor the W bit.
Ok, I understand what is going on here now. I agree that my patch is wrong.
>
> If the guest changes R/W/X within a PTE (e.g. for mprotect), it is obvious that
> a TLB flush for that page must be done. It is no different if the guest
> changes A/D (e.g. for swapping).
Agreed.
>
> > Setting write permission on dirty PTEs results in userspace inside a
> > Hypervisor guest (VU) becoming corrupted. This appears to be because it
> > ends up with write permission in the second stage translation in cases
> > where we aren't doing a store.
>
> You've not really given any more information than last time this patch came around.
>
> I still think this must be a guest (or nested guest) bug related to clearing
> PTE bits and failing to flush the TLB properly.
It think so as well now. I have changed the Linux guest and Hypervisor
to be very aggressive with flushing but still can't get guest user
space working. I'll keep digging and see if I can figure out what's
going on.
>
> I don't see how it could be a qemu tlb flushing bug. The only primitive,
> sfence.vma, is quite heavy-handed and explicitly local to the thread.
Yes, both sfence and hfence flush all TLBs, so that doesn't seem to be
the problem.
>
> It may be a bug in qemu's implementation of second stage paging. Which is not
> yet upstream, and I haven't gone digging in the archives to find the patch.
It's upstream now, I have double checked it though and I can't see
anything wrong.
Alistair
>
>
> r~
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 1/1] target/riscv: Don't set write permissions on dirty PTEs
@ 2020-03-12 22:10 ` Alistair Francis
0 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2020-03-12 22:10 UTC (permalink / raw)
To: Richard Henderson
Cc: Alistair Francis, qemu-devel@nongnu.org Developers,
open list:RISC-V, Palmer Dabbelt
On Wed, Mar 4, 2020 at 9:34 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/3/20 5:16 PM, Alistair Francis wrote:
> > The RISC-V spec specifies that when a write happens and the D bit is
> > clear the implementation will set the bit in the PTE. It does not
> > describe that the PTE being dirty means that we should provide write
> > access. This patch removes the write access granted to pages when the
> > dirty bit is set.
>
> The W bit by itself says we should provide write access.
>
> It is an implementation detail that we *withhold* write access when D is clear
> (etc) so that we can trap, so that we can properly set D in the future.
>
> The page table walk associated with a read is allowed to cache all of the
> information it finds in the PTE during that walk. Which includes the D bit.
> If D is set, then we do not need to set it in future, so we do not need to
> trap, so we can immediately honor the W bit.
Ok, I understand what is going on here now. I agree that my patch is wrong.
>
> If the guest changes R/W/X within a PTE (e.g. for mprotect), it is obvious that
> a TLB flush for that page must be done. It is no different if the guest
> changes A/D (e.g. for swapping).
Agreed.
>
> > Setting write permission on dirty PTEs results in userspace inside a
> > Hypervisor guest (VU) becoming corrupted. This appears to be because it
> > ends up with write permission in the second stage translation in cases
> > where we aren't doing a store.
>
> You've not really given any more information than last time this patch came around.
>
> I still think this must be a guest (or nested guest) bug related to clearing
> PTE bits and failing to flush the TLB properly.
It think so as well now. I have changed the Linux guest and Hypervisor
to be very aggressive with flushing but still can't get guest user
space working. I'll keep digging and see if I can figure out what's
going on.
>
> I don't see how it could be a qemu tlb flushing bug. The only primitive,
> sfence.vma, is quite heavy-handed and explicitly local to the thread.
Yes, both sfence and hfence flush all TLBs, so that doesn't seem to be
the problem.
>
> It may be a bug in qemu's implementation of second stage paging. Which is not
> yet upstream, and I haven't gone digging in the archives to find the patch.
It's upstream now, I have double checked it though and I can't see
anything wrong.
Alistair
>
>
> r~
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 1/1] target/riscv: Don't set write permissions on dirty PTEs
2020-03-12 22:10 ` Alistair Francis
@ 2020-03-13 5:26 ` Richard Henderson
-1 siblings, 0 replies; 14+ messages in thread
From: Richard Henderson @ 2020-03-13 5:26 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers
On 3/12/20 3:10 PM, Alistair Francis wrote:
>> I still think this must be a guest (or nested guest) bug related to clearing
>> PTE bits and failing to flush the TLB properly.
>
> It think so as well now. I have changed the Linux guest and Hypervisor
> to be very aggressive with flushing but still can't get guest user
> space working. I'll keep digging and see if I can figure out what's
> going on.
>
>>
>> I don't see how it could be a qemu tlb flushing bug. The only primitive,
>> sfence.vma, is quite heavy-handed and explicitly local to the thread.
>
> Yes, both sfence and hfence flush all TLBs, so that doesn't seem to be
> the problem.
Here's an idea: change the tlb_flush() calls to tlb_flush_all_cpus_synced().
If that works, it suggests a guest interprocessor interrupt bug in the tlb
shoot-down.
r~
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 1/1] target/riscv: Don't set write permissions on dirty PTEs
@ 2020-03-13 5:26 ` Richard Henderson
0 siblings, 0 replies; 14+ messages in thread
From: Richard Henderson @ 2020-03-13 5:26 UTC (permalink / raw)
To: Alistair Francis
Cc: Alistair Francis, qemu-devel@nongnu.org Developers,
open list:RISC-V, Palmer Dabbelt
On 3/12/20 3:10 PM, Alistair Francis wrote:
>> I still think this must be a guest (or nested guest) bug related to clearing
>> PTE bits and failing to flush the TLB properly.
>
> It think so as well now. I have changed the Linux guest and Hypervisor
> to be very aggressive with flushing but still can't get guest user
> space working. I'll keep digging and see if I can figure out what's
> going on.
>
>>
>> I don't see how it could be a qemu tlb flushing bug. The only primitive,
>> sfence.vma, is quite heavy-handed and explicitly local to the thread.
>
> Yes, both sfence and hfence flush all TLBs, so that doesn't seem to be
> the problem.
Here's an idea: change the tlb_flush() calls to tlb_flush_all_cpus_synced().
If that works, it suggests a guest interprocessor interrupt bug in the tlb
shoot-down.
r~
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 1/1] target/riscv: Don't set write permissions on dirty PTEs
2020-03-13 5:26 ` Richard Henderson
@ 2020-03-13 22:26 ` Alistair Francis
-1 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2020-03-13 22:26 UTC (permalink / raw)
To: Richard Henderson
Cc: open list:RISC-V, Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers
On Thu, Mar 12, 2020 at 10:26 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/12/20 3:10 PM, Alistair Francis wrote:
> >> I still think this must be a guest (or nested guest) bug related to clearing
> >> PTE bits and failing to flush the TLB properly.
> >
> > It think so as well now. I have changed the Linux guest and Hypervisor
> > to be very aggressive with flushing but still can't get guest user
> > space working. I'll keep digging and see if I can figure out what's
> > going on.
> >
> >>
> >> I don't see how it could be a qemu tlb flushing bug. The only primitive,
> >> sfence.vma, is quite heavy-handed and explicitly local to the thread.
> >
> > Yes, both sfence and hfence flush all TLBs, so that doesn't seem to be
> > the problem.
>
> Here's an idea: change the tlb_flush() calls to tlb_flush_all_cpus_synced().
>
> If that works, it suggests a guest interprocessor interrupt bug in the tlb
> shoot-down.
No change. I'll keep looking.
Alistair
>
>
> r~
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 1/1] target/riscv: Don't set write permissions on dirty PTEs
@ 2020-03-13 22:26 ` Alistair Francis
0 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2020-03-13 22:26 UTC (permalink / raw)
To: Richard Henderson
Cc: Alistair Francis, qemu-devel@nongnu.org Developers,
open list:RISC-V, Palmer Dabbelt
On Thu, Mar 12, 2020 at 10:26 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/12/20 3:10 PM, Alistair Francis wrote:
> >> I still think this must be a guest (or nested guest) bug related to clearing
> >> PTE bits and failing to flush the TLB properly.
> >
> > It think so as well now. I have changed the Linux guest and Hypervisor
> > to be very aggressive with flushing but still can't get guest user
> > space working. I'll keep digging and see if I can figure out what's
> > going on.
> >
> >>
> >> I don't see how it could be a qemu tlb flushing bug. The only primitive,
> >> sfence.vma, is quite heavy-handed and explicitly local to the thread.
> >
> > Yes, both sfence and hfence flush all TLBs, so that doesn't seem to be
> > the problem.
>
> Here's an idea: change the tlb_flush() calls to tlb_flush_all_cpus_synced().
>
> If that works, it suggests a guest interprocessor interrupt bug in the tlb
> shoot-down.
No change. I'll keep looking.
Alistair
>
>
> r~
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 1/1] target/riscv: Don't set write permissions on dirty PTEs
2020-03-04 1:16 ` Alistair Francis
@ 2020-03-19 4:52 ` Palmer Dabbelt
-1 siblings, 0 replies; 14+ messages in thread
From: Palmer Dabbelt @ 2020-03-19 4:52 UTC (permalink / raw)
To: Alistair Francis; +Cc: Alistair Francis, qemu-riscv, qemu-devel, alistair23
On Tue, 03 Mar 2020 17:16:59 PST (-0800), Alistair Francis wrote:
> The RISC-V spec specifies that when a write happens and the D bit is
> clear the implementation will set the bit in the PTE. It does not
> describe that the PTE being dirty means that we should provide write
> access. This patch removes the write access granted to pages when the
> dirty bit is set.
>
> Following the prot variable we can see that it affects all of these
> functions:
> riscv_cpu_tlb_fill()
> tlb_set_page()
> tlb_set_page_with_attrs()
> address_space_translate_for_iotlb()
>
> Looking at the cputlb code (tlb_set_page_with_attrs() and
> address_space_translate_for_iotlb()) it looks like the main affect of
> setting write permissions is that the page can be marked as TLB_NOTDIRTY.
>
> I don't see any other impacts (related to the dirty bit) for giving a
> page write permissions.
>
> Setting write permission on dirty PTEs results in userspace inside a
> Hypervisor guest (VU) becoming corrupted. This appears to be because it
> ends up with write permission in the second stage translation in cases
> where we aren't doing a store.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> target/riscv/cpu_helper.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 5ea5d133aa..cc9f20b471 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -572,10 +572,8 @@ restart:
> if ((pte & PTE_X)) {
> *prot |= PAGE_EXEC;
> }
> - /* add write permission on stores or if the page is already dirty,
> - so that we TLB miss on later writes to update the dirty bit */
> - if ((pte & PTE_W) &&
> - (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
> + /* add write permission on stores */
> + if ((pte & PTE_W) && (access_type == MMU_DATA_STORE)) {
> *prot |= PAGE_WRITE;
> }
> return TRANSLATE_SUCCESS;
I remember having seen this patch before and having some objections, but I feel
like I mistakenly had this backwards before or something because it makes sense
now.
Thanks!
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 1/1] target/riscv: Don't set write permissions on dirty PTEs
@ 2020-03-19 4:52 ` Palmer Dabbelt
0 siblings, 0 replies; 14+ messages in thread
From: Palmer Dabbelt @ 2020-03-19 4:52 UTC (permalink / raw)
To: Alistair Francis; +Cc: qemu-devel, qemu-riscv, Alistair Francis, alistair23
On Tue, 03 Mar 2020 17:16:59 PST (-0800), Alistair Francis wrote:
> The RISC-V spec specifies that when a write happens and the D bit is
> clear the implementation will set the bit in the PTE. It does not
> describe that the PTE being dirty means that we should provide write
> access. This patch removes the write access granted to pages when the
> dirty bit is set.
>
> Following the prot variable we can see that it affects all of these
> functions:
> riscv_cpu_tlb_fill()
> tlb_set_page()
> tlb_set_page_with_attrs()
> address_space_translate_for_iotlb()
>
> Looking at the cputlb code (tlb_set_page_with_attrs() and
> address_space_translate_for_iotlb()) it looks like the main affect of
> setting write permissions is that the page can be marked as TLB_NOTDIRTY.
>
> I don't see any other impacts (related to the dirty bit) for giving a
> page write permissions.
>
> Setting write permission on dirty PTEs results in userspace inside a
> Hypervisor guest (VU) becoming corrupted. This appears to be because it
> ends up with write permission in the second stage translation in cases
> where we aren't doing a store.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> target/riscv/cpu_helper.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 5ea5d133aa..cc9f20b471 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -572,10 +572,8 @@ restart:
> if ((pte & PTE_X)) {
> *prot |= PAGE_EXEC;
> }
> - /* add write permission on stores or if the page is already dirty,
> - so that we TLB miss on later writes to update the dirty bit */
> - if ((pte & PTE_W) &&
> - (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
> + /* add write permission on stores */
> + if ((pte & PTE_W) && (access_type == MMU_DATA_STORE)) {
> *prot |= PAGE_WRITE;
> }
> return TRANSLATE_SUCCESS;
I remember having seen this patch before and having some objections, but I feel
like I mistakenly had this backwards before or something because it makes sense
now.
Thanks!
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 1/1] target/riscv: Don't set write permissions on dirty PTEs
2020-03-19 4:52 ` Palmer Dabbelt
@ 2020-03-19 15:56 ` Alistair Francis
-1 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2020-03-19 15:56 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: open list:RISC-V, Alistair Francis, qemu-devel@nongnu.org Developers
On Wed, Mar 18, 2020 at 9:52 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Tue, 03 Mar 2020 17:16:59 PST (-0800), Alistair Francis wrote:
> > The RISC-V spec specifies that when a write happens and the D bit is
> > clear the implementation will set the bit in the PTE. It does not
> > describe that the PTE being dirty means that we should provide write
> > access. This patch removes the write access granted to pages when the
> > dirty bit is set.
> >
> > Following the prot variable we can see that it affects all of these
> > functions:
> > riscv_cpu_tlb_fill()
> > tlb_set_page()
> > tlb_set_page_with_attrs()
> > address_space_translate_for_iotlb()
> >
> > Looking at the cputlb code (tlb_set_page_with_attrs() and
> > address_space_translate_for_iotlb()) it looks like the main affect of
> > setting write permissions is that the page can be marked as TLB_NOTDIRTY.
> >
> > I don't see any other impacts (related to the dirty bit) for giving a
> > page write permissions.
> >
> > Setting write permission on dirty PTEs results in userspace inside a
> > Hypervisor guest (VU) becoming corrupted. This appears to be because it
> > ends up with write permission in the second stage translation in cases
> > where we aren't doing a store.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> > target/riscv/cpu_helper.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 5ea5d133aa..cc9f20b471 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -572,10 +572,8 @@ restart:
> > if ((pte & PTE_X)) {
> > *prot |= PAGE_EXEC;
> > }
> > - /* add write permission on stores or if the page is already dirty,
> > - so that we TLB miss on later writes to update the dirty bit */
> > - if ((pte & PTE_W) &&
> > - (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
> > + /* add write permission on stores */
> > + if ((pte & PTE_W) && (access_type == MMU_DATA_STORE)) {
> > *prot |= PAGE_WRITE;
> > }
> > return TRANSLATE_SUCCESS;
>
> I remember having seen this patch before and having some objections, but I feel
> like I mistakenly had this backwards before or something because it makes sense
> now.
Ha, we have come full circle because I think this is wrong.
This is an optimisation which from what I can tell (and talking to
Richard) is correct.
In saying that this patch is the only thing that I have found that
fixes Hypervisor guest userspace. It shouldn't be applied though.
Alistair
>
> Thanks!
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1 1/1] target/riscv: Don't set write permissions on dirty PTEs
@ 2020-03-19 15:56 ` Alistair Francis
0 siblings, 0 replies; 14+ messages in thread
From: Alistair Francis @ 2020-03-19 15:56 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: Alistair Francis, qemu-devel@nongnu.org Developers, open list:RISC-V
On Wed, Mar 18, 2020 at 9:52 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Tue, 03 Mar 2020 17:16:59 PST (-0800), Alistair Francis wrote:
> > The RISC-V spec specifies that when a write happens and the D bit is
> > clear the implementation will set the bit in the PTE. It does not
> > describe that the PTE being dirty means that we should provide write
> > access. This patch removes the write access granted to pages when the
> > dirty bit is set.
> >
> > Following the prot variable we can see that it affects all of these
> > functions:
> > riscv_cpu_tlb_fill()
> > tlb_set_page()
> > tlb_set_page_with_attrs()
> > address_space_translate_for_iotlb()
> >
> > Looking at the cputlb code (tlb_set_page_with_attrs() and
> > address_space_translate_for_iotlb()) it looks like the main affect of
> > setting write permissions is that the page can be marked as TLB_NOTDIRTY.
> >
> > I don't see any other impacts (related to the dirty bit) for giving a
> > page write permissions.
> >
> > Setting write permission on dirty PTEs results in userspace inside a
> > Hypervisor guest (VU) becoming corrupted. This appears to be because it
> > ends up with write permission in the second stage translation in cases
> > where we aren't doing a store.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > ---
> > target/riscv/cpu_helper.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 5ea5d133aa..cc9f20b471 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -572,10 +572,8 @@ restart:
> > if ((pte & PTE_X)) {
> > *prot |= PAGE_EXEC;
> > }
> > - /* add write permission on stores or if the page is already dirty,
> > - so that we TLB miss on later writes to update the dirty bit */
> > - if ((pte & PTE_W) &&
> > - (access_type == MMU_DATA_STORE || (pte & PTE_D))) {
> > + /* add write permission on stores */
> > + if ((pte & PTE_W) && (access_type == MMU_DATA_STORE)) {
> > *prot |= PAGE_WRITE;
> > }
> > return TRANSLATE_SUCCESS;
>
> I remember having seen this patch before and having some objections, but I feel
> like I mistakenly had this backwards before or something because it makes sense
> now.
Ha, we have come full circle because I think this is wrong.
This is an optimisation which from what I can tell (and talking to
Richard) is correct.
In saying that this patch is the only thing that I have found that
fixes Hypervisor guest userspace. It shouldn't be applied though.
Alistair
>
> Thanks!
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-03-19 16:05 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-04 1:16 [PATCH v1 1/1] target/riscv: Don't set write permissions on dirty PTEs Alistair Francis
2020-03-04 1:16 ` Alistair Francis
2020-03-04 17:34 ` Richard Henderson
2020-03-04 17:34 ` Richard Henderson
2020-03-12 22:10 ` Alistair Francis
2020-03-12 22:10 ` Alistair Francis
2020-03-13 5:26 ` Richard Henderson
2020-03-13 5:26 ` Richard Henderson
2020-03-13 22:26 ` Alistair Francis
2020-03-13 22:26 ` Alistair Francis
2020-03-19 4:52 ` Palmer Dabbelt
2020-03-19 4:52 ` Palmer Dabbelt
2020-03-19 15:56 ` Alistair Francis
2020-03-19 15:56 ` Alistair Francis
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.