* [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
@ 2019-09-24 7:58 ` guoren
0 siblings, 0 replies; 11+ messages in thread
From: guoren @ 2019-09-24 7:58 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair23, palmer, alistair.francis, Guo Ren
From: Guo Ren <ren_guo@c-sky.com>
Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we
need to ignore them. They can not be a part of ppn.
1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
4.4 Sv39: Page-Based 39-bit Virtual-Memory System
4.5 Sv48: Page-Based 48-bit Virtual-Memory System
Changelog V2:
- Bugfix pte destroyed cause boot fail
- Change to AND with a mask instead of shifting both directions
Signed-off-by: Guo Ren <ren_guo@c-sky.com>
Reviewed-by: Liu Zhiwei <zhiwei_liu@c-sky.com>
---
target/riscv/cpu_bits.h | 3 +++
target/riscv/cpu_helper.c | 3 ++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index e998348..ae8aa0f 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -470,6 +470,9 @@
#define PTE_D 0x080 /* Dirty */
#define PTE_SOFT 0x300 /* Reserved for Software */
+/* Reserved highest 10 bits in PTE */
+#define PTE_RESERVED ((target_ulong)0x3ff << 54)
+
/* Page table PPN shift amount */
#define PTE_PPN_SHIFT 10
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 87dd6a6..7a540cc 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -258,10 +258,11 @@ restart:
}
#if defined(TARGET_RISCV32)
target_ulong pte = ldl_phys(cs->as, pte_addr);
+ hwaddr ppn = pte >> PTE_PPN_SHIFT;
#elif defined(TARGET_RISCV64)
target_ulong pte = ldq_phys(cs->as, pte_addr);
+ hwaddr ppn = (pte & ~PTE_RESERVED) >> PTE_PPN_SHIFT;
#endif
- hwaddr ppn = pte >> PTE_PPN_SHIFT;
if (!(pte & PTE_V)) {
/* Invalid PTE */
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
@ 2019-09-24 7:58 ` guoren
0 siblings, 0 replies; 11+ messages in thread
From: guoren @ 2019-09-24 7:58 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: alistair.francis, palmer, alistair23, Guo Ren
From: Guo Ren <ren_guo@c-sky.com>
Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we
need to ignore them. They can not be a part of ppn.
1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
4.4 Sv39: Page-Based 39-bit Virtual-Memory System
4.5 Sv48: Page-Based 48-bit Virtual-Memory System
Changelog V2:
- Bugfix pte destroyed cause boot fail
- Change to AND with a mask instead of shifting both directions
Signed-off-by: Guo Ren <ren_guo@c-sky.com>
Reviewed-by: Liu Zhiwei <zhiwei_liu@c-sky.com>
---
target/riscv/cpu_bits.h | 3 +++
target/riscv/cpu_helper.c | 3 ++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index e998348..ae8aa0f 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -470,6 +470,9 @@
#define PTE_D 0x080 /* Dirty */
#define PTE_SOFT 0x300 /* Reserved for Software */
+/* Reserved highest 10 bits in PTE */
+#define PTE_RESERVED ((target_ulong)0x3ff << 54)
+
/* Page table PPN shift amount */
#define PTE_PPN_SHIFT 10
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 87dd6a6..7a540cc 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -258,10 +258,11 @@ restart:
}
#if defined(TARGET_RISCV32)
target_ulong pte = ldl_phys(cs->as, pte_addr);
+ hwaddr ppn = pte >> PTE_PPN_SHIFT;
#elif defined(TARGET_RISCV64)
target_ulong pte = ldq_phys(cs->as, pte_addr);
+ hwaddr ppn = (pte & ~PTE_RESERVED) >> PTE_PPN_SHIFT;
#endif
- hwaddr ppn = pte >> PTE_PPN_SHIFT;
if (!(pte & PTE_V)) {
/* Invalid PTE */
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
2019-09-24 7:58 ` guoren
(?)
@ 2019-09-24 8:02 ` Guo Ren
-1 siblings, 0 replies; 11+ messages in thread
From: Guo Ren @ 2019-09-24 8:02 UTC (permalink / raw)
To: qemu-devel, qemu-riscv
Cc: alistair23, Palmer Dabbelt, Alistair Francis, Guo Ren
I only tested it on qemu-3.1.0, pls have a try before merge.
On Tue, Sep 24, 2019 at 4:00 PM <guoren@kernel.org> wrote:
>
> From: Guo Ren <ren_guo@c-sky.com>
>
> Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we
> need to ignore them. They can not be a part of ppn.
>
> 1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
> 4.4 Sv39: Page-Based 39-bit Virtual-Memory System
> 4.5 Sv48: Page-Based 48-bit Virtual-Memory System
>
> Changelog V2:
> - Bugfix pte destroyed cause boot fail
> - Change to AND with a mask instead of shifting both directions
>
> Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> Reviewed-by: Liu Zhiwei <zhiwei_liu@c-sky.com>
> ---
> target/riscv/cpu_bits.h | 3 +++
> target/riscv/cpu_helper.c | 3 ++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index e998348..ae8aa0f 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -470,6 +470,9 @@
> #define PTE_D 0x080 /* Dirty */
> #define PTE_SOFT 0x300 /* Reserved for Software */
>
> +/* Reserved highest 10 bits in PTE */
> +#define PTE_RESERVED ((target_ulong)0x3ff << 54)
> +
> /* Page table PPN shift amount */
> #define PTE_PPN_SHIFT 10
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 87dd6a6..7a540cc 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -258,10 +258,11 @@ restart:
> }
> #if defined(TARGET_RISCV32)
> target_ulong pte = ldl_phys(cs->as, pte_addr);
> + hwaddr ppn = pte >> PTE_PPN_SHIFT;
> #elif defined(TARGET_RISCV64)
> target_ulong pte = ldq_phys(cs->as, pte_addr);
> + hwaddr ppn = (pte & ~PTE_RESERVED) >> PTE_PPN_SHIFT;
> #endif
> - hwaddr ppn = pte >> PTE_PPN_SHIFT;
>
> if (!(pte & PTE_V)) {
> /* Invalid PTE */
> --
> 2.7.4
>
>
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
2019-09-24 7:58 ` guoren
@ 2019-09-24 23:33 ` Alistair Francis
-1 siblings, 0 replies; 11+ messages in thread
From: Alistair Francis @ 2019-09-24 23:33 UTC (permalink / raw)
To: guoren
Cc: Alistair Francis, Palmer Dabbelt, open list:RISC-V,
qemu-devel@nongnu.org Developers, Guo Ren
On Tue, Sep 24, 2019 at 12:58 AM <guoren@kernel.org> wrote:
>
> From: Guo Ren <ren_guo@c-sky.com>
>
> Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we
> need to ignore them. They can not be a part of ppn.
>
> 1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
> 4.4 Sv39: Page-Based 39-bit Virtual-Memory System
> 4.5 Sv48: Page-Based 48-bit Virtual-Memory System
Thanks for the patch!
The spec says "must be zeroed by software for forward compatibility"
so I don't think it's correct for QEMU to zero out the bits.
Does this fix a bug you are seeing?
>
> Changelog V2:
> - Bugfix pte destroyed cause boot fail
> - Change to AND with a mask instead of shifting both directions
The changelog shouldn't be in the commit, it should be kept under the
line line below.
>
> Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> Reviewed-by: Liu Zhiwei <zhiwei_liu@c-sky.com>
> ---
The change log should go here.
> target/riscv/cpu_bits.h | 3 +++
> target/riscv/cpu_helper.c | 3 ++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index e998348..ae8aa0f 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -470,6 +470,9 @@
> #define PTE_D 0x080 /* Dirty */
> #define PTE_SOFT 0x300 /* Reserved for Software */
>
> +/* Reserved highest 10 bits in PTE */
> +#define PTE_RESERVED ((target_ulong)0x3ff << 54)
I think it's just easier to define this as 0xFFC0000000000000ULL and
remove the cast.
> +
> /* Page table PPN shift amount */
> #define PTE_PPN_SHIFT 10
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 87dd6a6..7a540cc 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -258,10 +258,11 @@ restart:
> }
> #if defined(TARGET_RISCV32)
> target_ulong pte = ldl_phys(cs->as, pte_addr);
> + hwaddr ppn = pte >> PTE_PPN_SHIFT;
> #elif defined(TARGET_RISCV64)
> target_ulong pte = ldq_phys(cs->as, pte_addr);
> + hwaddr ppn = (pte & ~PTE_RESERVED) >> PTE_PPN_SHIFT;
> #endif
> - hwaddr ppn = pte >> PTE_PPN_SHIFT;
You don't have to move this shift
>
> if (!(pte & PTE_V)) {
> /* Invalid PTE */
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
@ 2019-09-24 23:33 ` Alistair Francis
0 siblings, 0 replies; 11+ messages in thread
From: Alistair Francis @ 2019-09-24 23:33 UTC (permalink / raw)
To: guoren
Cc: qemu-devel@nongnu.org Developers, open list:RISC-V,
Alistair Francis, Palmer Dabbelt, Guo Ren
On Tue, Sep 24, 2019 at 12:58 AM <guoren@kernel.org> wrote:
>
> From: Guo Ren <ren_guo@c-sky.com>
>
> Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we
> need to ignore them. They can not be a part of ppn.
>
> 1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
> 4.4 Sv39: Page-Based 39-bit Virtual-Memory System
> 4.5 Sv48: Page-Based 48-bit Virtual-Memory System
Thanks for the patch!
The spec says "must be zeroed by software for forward compatibility"
so I don't think it's correct for QEMU to zero out the bits.
Does this fix a bug you are seeing?
>
> Changelog V2:
> - Bugfix pte destroyed cause boot fail
> - Change to AND with a mask instead of shifting both directions
The changelog shouldn't be in the commit, it should be kept under the
line line below.
>
> Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> Reviewed-by: Liu Zhiwei <zhiwei_liu@c-sky.com>
> ---
The change log should go here.
> target/riscv/cpu_bits.h | 3 +++
> target/riscv/cpu_helper.c | 3 ++-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index e998348..ae8aa0f 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -470,6 +470,9 @@
> #define PTE_D 0x080 /* Dirty */
> #define PTE_SOFT 0x300 /* Reserved for Software */
>
> +/* Reserved highest 10 bits in PTE */
> +#define PTE_RESERVED ((target_ulong)0x3ff << 54)
I think it's just easier to define this as 0xFFC0000000000000ULL and
remove the cast.
> +
> /* Page table PPN shift amount */
> #define PTE_PPN_SHIFT 10
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 87dd6a6..7a540cc 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -258,10 +258,11 @@ restart:
> }
> #if defined(TARGET_RISCV32)
> target_ulong pte = ldl_phys(cs->as, pte_addr);
> + hwaddr ppn = pte >> PTE_PPN_SHIFT;
> #elif defined(TARGET_RISCV64)
> target_ulong pte = ldq_phys(cs->as, pte_addr);
> + hwaddr ppn = (pte & ~PTE_RESERVED) >> PTE_PPN_SHIFT;
> #endif
> - hwaddr ppn = pte >> PTE_PPN_SHIFT;
You don't have to move this shift
>
> if (!(pte & PTE_V)) {
> /* Invalid PTE */
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
2019-09-24 23:33 ` Alistair Francis
@ 2019-09-25 0:13 ` Guo Ren
-1 siblings, 0 replies; 11+ messages in thread
From: Guo Ren @ 2019-09-25 0:13 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Alistair Francis, Palmer Dabbelt, guoren,
qemu-devel@nongnu.org Developers
> 在 2019年9月25日,上午7:33,Alistair Francis <alistair23@gmail.com> 写道:
>
> On Tue, Sep 24, 2019 at 12:58 AM <guoren@kernel.org> wrote:
>>
>> From: Guo Ren <ren_guo@c-sky.com>
>>
>> Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we
>> need to ignore them. They can not be a part of ppn.
>>
>> 1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
>> 4.4 Sv39: Page-Based 39-bit Virtual-Memory System
>> 4.5 Sv48: Page-Based 48-bit Virtual-Memory System
>
> Thanks for the patch!
>
> The spec says "must be zeroed by software for forward compatibility"
> so I don't think it's correct for QEMU to zero out the bits.
QEMU don’t zero out the bits, QEMU just ignore the bits for ppn.
>
> Does this fix a bug you are seeing?
Yes, because we try to reuse these bits as attributes.
>
>>
>> Changelog V2:
>> - Bugfix pte destroyed cause boot fail
>> - Change to AND with a mask instead of shifting both directions
>
> The changelog shouldn't be in the commit, it should be kept under the
> line line below.
I just prefer to save them in commit.
>
>>
>> Signed-off-by: Guo Ren <ren_guo@c-sky.com>
>> Reviewed-by: Liu Zhiwei <zhiwei_liu@c-sky.com>
>> ---
>
> The change log should go here.
OK, but git am we’ll lose them.
>
>> target/riscv/cpu_bits.h | 3 +++
>> target/riscv/cpu_helper.c | 3 ++-
>> 2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>> index e998348..ae8aa0f 100644
>> --- a/target/riscv/cpu_bits.h
>> +++ b/target/riscv/cpu_bits.h
>> @@ -470,6 +470,9 @@
>> #define PTE_D 0x080 /* Dirty */
>> #define PTE_SOFT 0x300 /* Reserved for Software */
>>
>> +/* Reserved highest 10 bits in PTE */
>> +#define PTE_RESERVED ((target_ulong)0x3ff << 54)
>
> I think it's just easier to define this as 0xFFC0000000000000ULL and
> remove the cast.
OK follow your rule, but I still prefer prior.
>
>> +
>> /* Page table PPN shift amount */
>> #define PTE_PPN_SHIFT 10
>>
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index 87dd6a6..7a540cc 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -258,10 +258,11 @@ restart:
>> }
>> #if defined(TARGET_RISCV32)
>> target_ulong pte = ldl_phys(cs->as, pte_addr);
>> + hwaddr ppn = pte >> PTE_PPN_SHIFT;
>> #elif defined(TARGET_RISCV64)
>> target_ulong pte = ldq_phys(cs->as, pte_addr);
>> + hwaddr ppn = (pte & ~PTE_RESERVED) >> PTE_PPN_SHIFT;
>> #endif
>> - hwaddr ppn = pte >> PTE_PPN_SHIFT;
>
> You don't have to move this shift
En… Do you want this: ?
#if defined(TARGET_RISCV32)
target_ulong pte = ldl_phys(cs->as, pte_addr);
+ hwaddr ppn = pte;
#elif defined(TARGET_RISCV64)
target_ulong pte = ldq_phys(cs->as, pte_addr);
+ hwaddr ppn = (pte & ~PTE_RESERVED);
#endif
+ ppn = ppn >> PTE_PPN_SHIFT;
The pte couldn’t be destroyed, just ppn ignore the RESERVED bits.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
@ 2019-09-25 0:13 ` Guo Ren
0 siblings, 0 replies; 11+ messages in thread
From: Guo Ren @ 2019-09-25 0:13 UTC (permalink / raw)
To: Alistair Francis
Cc: guoren, qemu-devel@nongnu.org Developers, open list:RISC-V,
Alistair Francis, Palmer Dabbelt
> 在 2019年9月25日,上午7:33,Alistair Francis <alistair23@gmail.com> 写道:
>
> On Tue, Sep 24, 2019 at 12:58 AM <guoren@kernel.org> wrote:
>>
>> From: Guo Ren <ren_guo@c-sky.com>
>>
>> Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we
>> need to ignore them. They can not be a part of ppn.
>>
>> 1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
>> 4.4 Sv39: Page-Based 39-bit Virtual-Memory System
>> 4.5 Sv48: Page-Based 48-bit Virtual-Memory System
>
> Thanks for the patch!
>
> The spec says "must be zeroed by software for forward compatibility"
> so I don't think it's correct for QEMU to zero out the bits.
QEMU don’t zero out the bits, QEMU just ignore the bits for ppn.
>
> Does this fix a bug you are seeing?
Yes, because we try to reuse these bits as attributes.
>
>>
>> Changelog V2:
>> - Bugfix pte destroyed cause boot fail
>> - Change to AND with a mask instead of shifting both directions
>
> The changelog shouldn't be in the commit, it should be kept under the
> line line below.
I just prefer to save them in commit.
>
>>
>> Signed-off-by: Guo Ren <ren_guo@c-sky.com>
>> Reviewed-by: Liu Zhiwei <zhiwei_liu@c-sky.com>
>> ---
>
> The change log should go here.
OK, but git am we’ll lose them.
>
>> target/riscv/cpu_bits.h | 3 +++
>> target/riscv/cpu_helper.c | 3 ++-
>> 2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>> index e998348..ae8aa0f 100644
>> --- a/target/riscv/cpu_bits.h
>> +++ b/target/riscv/cpu_bits.h
>> @@ -470,6 +470,9 @@
>> #define PTE_D 0x080 /* Dirty */
>> #define PTE_SOFT 0x300 /* Reserved for Software */
>>
>> +/* Reserved highest 10 bits in PTE */
>> +#define PTE_RESERVED ((target_ulong)0x3ff << 54)
>
> I think it's just easier to define this as 0xFFC0000000000000ULL and
> remove the cast.
OK follow your rule, but I still prefer prior.
>
>> +
>> /* Page table PPN shift amount */
>> #define PTE_PPN_SHIFT 10
>>
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index 87dd6a6..7a540cc 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -258,10 +258,11 @@ restart:
>> }
>> #if defined(TARGET_RISCV32)
>> target_ulong pte = ldl_phys(cs->as, pte_addr);
>> + hwaddr ppn = pte >> PTE_PPN_SHIFT;
>> #elif defined(TARGET_RISCV64)
>> target_ulong pte = ldq_phys(cs->as, pte_addr);
>> + hwaddr ppn = (pte & ~PTE_RESERVED) >> PTE_PPN_SHIFT;
>> #endif
>> - hwaddr ppn = pte >> PTE_PPN_SHIFT;
>
> You don't have to move this shift
En… Do you want this: ?
#if defined(TARGET_RISCV32)
target_ulong pte = ldl_phys(cs->as, pte_addr);
+ hwaddr ppn = pte;
#elif defined(TARGET_RISCV64)
target_ulong pte = ldq_phys(cs->as, pte_addr);
+ hwaddr ppn = (pte & ~PTE_RESERVED);
#endif
+ ppn = ppn >> PTE_PPN_SHIFT;
The pte couldn’t be destroyed, just ppn ignore the RESERVED bits.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
2019-09-25 0:13 ` Guo Ren
@ 2019-09-25 0:21 ` Alistair Francis
-1 siblings, 0 replies; 11+ messages in thread
From: Alistair Francis @ 2019-09-25 0:21 UTC (permalink / raw)
To: Guo Ren
Cc: open list:RISC-V, Alistair Francis, Palmer Dabbelt, guoren,
qemu-devel@nongnu.org Developers
On Tue, Sep 24, 2019 at 5:13 PM Guo Ren <ren_guo@c-sky.com> wrote:
>
>
> > 在 2019年9月25日,上午7:33,Alistair Francis <alistair23@gmail.com> 写道:
> >
> > On Tue, Sep 24, 2019 at 12:58 AM <guoren@kernel.org> wrote:
> >>
> >> From: Guo Ren <ren_guo@c-sky.com>
> >>
> >> Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we
> >> need to ignore them. They can not be a part of ppn.
> >>
> >> 1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
> >> 4.4 Sv39: Page-Based 39-bit Virtual-Memory System
> >> 4.5 Sv48: Page-Based 48-bit Virtual-Memory System
> >
> > Thanks for the patch!
> >
> > The spec says "must be zeroed by software for forward compatibility"
> > so I don't think it's correct for QEMU to zero out the bits.
> QEMU don’t zero out the bits, QEMU just ignore the bits for ppn.
Yes, from reading the spec that seems to be the correct behaviour.
>
> >
> > Does this fix a bug you are seeing?
> Yes, because we try to reuse these bits as attributes.
That isn't really a bug then, the spec says not to do that.
>
> >
> >>
> >> Changelog V2:
> >> - Bugfix pte destroyed cause boot fail
> >> - Change to AND with a mask instead of shifting both directions
> >
> > The changelog shouldn't be in the commit, it should be kept under the
> > line line below.
> I just prefer to save them in commit.
Fair, but in QEMU we don't commit the change log in the commit.
>
> >
> >>
> >> Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> >> Reviewed-by: Liu Zhiwei <zhiwei_liu@c-sky.com>
> >> ---
> >
> > The change log should go here.
> OK, but git am we’ll lose them.
>
> >
> >> target/riscv/cpu_bits.h | 3 +++
> >> target/riscv/cpu_helper.c | 3 ++-
> >> 2 files changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> >> index e998348..ae8aa0f 100644
> >> --- a/target/riscv/cpu_bits.h
> >> +++ b/target/riscv/cpu_bits.h
> >> @@ -470,6 +470,9 @@
> >> #define PTE_D 0x080 /* Dirty */
> >> #define PTE_SOFT 0x300 /* Reserved for Software */
> >>
> >> +/* Reserved highest 10 bits in PTE */
> >> +#define PTE_RESERVED ((target_ulong)0x3ff << 54)
> >
> > I think it's just easier to define this as 0xFFC0000000000000ULL and
> > remove the cast.
> OK follow your rule, but I still prefer prior.
>
> >
> >> +
> >> /* Page table PPN shift amount */
> >> #define PTE_PPN_SHIFT 10
> >>
> >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> >> index 87dd6a6..7a540cc 100644
> >> --- a/target/riscv/cpu_helper.c
> >> +++ b/target/riscv/cpu_helper.c
> >> @@ -258,10 +258,11 @@ restart:
> >> }
> >> #if defined(TARGET_RISCV32)
> >> target_ulong pte = ldl_phys(cs->as, pte_addr);
> >> + hwaddr ppn = pte >> PTE_PPN_SHIFT;
> >> #elif defined(TARGET_RISCV64)
> >> target_ulong pte = ldq_phys(cs->as, pte_addr);
> >> + hwaddr ppn = (pte & ~PTE_RESERVED) >> PTE_PPN_SHIFT;
> >> #endif
> >> - hwaddr ppn = pte >> PTE_PPN_SHIFT;
> >
> > You don't have to move this shift
> En… Do you want this: ?
>
> #if defined(TARGET_RISCV32)
> target_ulong pte = ldl_phys(cs->as, pte_addr);
> + hwaddr ppn = pte;
> #elif defined(TARGET_RISCV64)
> target_ulong pte = ldq_phys(cs->as, pte_addr);
> + hwaddr ppn = (pte & ~PTE_RESERVED);
> #endif
> + ppn = ppn >> PTE_PPN_SHIFT;
>
Yeah, it seems a little cleaner.
Alistair
> The pte couldn’t be destroyed, just ppn ignore the RESERVED bits.
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
@ 2019-09-25 0:21 ` Alistair Francis
0 siblings, 0 replies; 11+ messages in thread
From: Alistair Francis @ 2019-09-25 0:21 UTC (permalink / raw)
To: Guo Ren
Cc: guoren, qemu-devel@nongnu.org Developers, open list:RISC-V,
Alistair Francis, Palmer Dabbelt
On Tue, Sep 24, 2019 at 5:13 PM Guo Ren <ren_guo@c-sky.com> wrote:
>
>
> > 在 2019年9月25日,上午7:33,Alistair Francis <alistair23@gmail.com> 写道:
> >
> > On Tue, Sep 24, 2019 at 12:58 AM <guoren@kernel.org> wrote:
> >>
> >> From: Guo Ren <ren_guo@c-sky.com>
> >>
> >> Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we
> >> need to ignore them. They can not be a part of ppn.
> >>
> >> 1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
> >> 4.4 Sv39: Page-Based 39-bit Virtual-Memory System
> >> 4.5 Sv48: Page-Based 48-bit Virtual-Memory System
> >
> > Thanks for the patch!
> >
> > The spec says "must be zeroed by software for forward compatibility"
> > so I don't think it's correct for QEMU to zero out the bits.
> QEMU don’t zero out the bits, QEMU just ignore the bits for ppn.
Yes, from reading the spec that seems to be the correct behaviour.
>
> >
> > Does this fix a bug you are seeing?
> Yes, because we try to reuse these bits as attributes.
That isn't really a bug then, the spec says not to do that.
>
> >
> >>
> >> Changelog V2:
> >> - Bugfix pte destroyed cause boot fail
> >> - Change to AND with a mask instead of shifting both directions
> >
> > The changelog shouldn't be in the commit, it should be kept under the
> > line line below.
> I just prefer to save them in commit.
Fair, but in QEMU we don't commit the change log in the commit.
>
> >
> >>
> >> Signed-off-by: Guo Ren <ren_guo@c-sky.com>
> >> Reviewed-by: Liu Zhiwei <zhiwei_liu@c-sky.com>
> >> ---
> >
> > The change log should go here.
> OK, but git am we’ll lose them.
>
> >
> >> target/riscv/cpu_bits.h | 3 +++
> >> target/riscv/cpu_helper.c | 3 ++-
> >> 2 files changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> >> index e998348..ae8aa0f 100644
> >> --- a/target/riscv/cpu_bits.h
> >> +++ b/target/riscv/cpu_bits.h
> >> @@ -470,6 +470,9 @@
> >> #define PTE_D 0x080 /* Dirty */
> >> #define PTE_SOFT 0x300 /* Reserved for Software */
> >>
> >> +/* Reserved highest 10 bits in PTE */
> >> +#define PTE_RESERVED ((target_ulong)0x3ff << 54)
> >
> > I think it's just easier to define this as 0xFFC0000000000000ULL and
> > remove the cast.
> OK follow your rule, but I still prefer prior.
>
> >
> >> +
> >> /* Page table PPN shift amount */
> >> #define PTE_PPN_SHIFT 10
> >>
> >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> >> index 87dd6a6..7a540cc 100644
> >> --- a/target/riscv/cpu_helper.c
> >> +++ b/target/riscv/cpu_helper.c
> >> @@ -258,10 +258,11 @@ restart:
> >> }
> >> #if defined(TARGET_RISCV32)
> >> target_ulong pte = ldl_phys(cs->as, pte_addr);
> >> + hwaddr ppn = pte >> PTE_PPN_SHIFT;
> >> #elif defined(TARGET_RISCV64)
> >> target_ulong pte = ldq_phys(cs->as, pte_addr);
> >> + hwaddr ppn = (pte & ~PTE_RESERVED) >> PTE_PPN_SHIFT;
> >> #endif
> >> - hwaddr ppn = pte >> PTE_PPN_SHIFT;
> >
> > You don't have to move this shift
> En… Do you want this: ?
>
> #if defined(TARGET_RISCV32)
> target_ulong pte = ldl_phys(cs->as, pte_addr);
> + hwaddr ppn = pte;
> #elif defined(TARGET_RISCV64)
> target_ulong pte = ldq_phys(cs->as, pte_addr);
> + hwaddr ppn = (pte & ~PTE_RESERVED);
> #endif
> + ppn = ppn >> PTE_PPN_SHIFT;
>
Yeah, it seems a little cleaner.
Alistair
> The pte couldn’t be destroyed, just ppn ignore the RESERVED bits.
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
2019-09-25 0:21 ` Alistair Francis
@ 2019-09-25 1:02 ` Guo Ren
-1 siblings, 0 replies; 11+ messages in thread
From: Guo Ren @ 2019-09-25 1:02 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Alistair Francis, Palmer Dabbelt, guoren,
qemu-devel@nongnu.org Developers
在 2019/9/25 上午8:21, Alistair Francis 写道:
> On Tue, Sep 24, 2019 at 5:13 PM Guo Ren <ren_guo@c-sky.com> wrote:
>>
>>
>>> 在 2019年9月25日,上午7:33,Alistair Francis <alistair23@gmail.com> 写道:
>>>
>>> On Tue, Sep 24, 2019 at 12:58 AM <guoren@kernel.org> wrote:
>>>>
>>>> From: Guo Ren <ren_guo@c-sky.com>
>>>>
>>>> Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we
>>>> need to ignore them. They can not be a part of ppn.
>>>>
>>>> 1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
>>>> 4.4 Sv39: Page-Based 39-bit Virtual-Memory System
>>>> 4.5 Sv48: Page-Based 48-bit Virtual-Memory System
>>>
>>> Thanks for the patch!
>>>
>>> The spec says "must be zeroed by software for forward compatibility"
>>> so I don't think it's correct for QEMU to zero out the bits.
>> QEMU don’t zero out the bits, QEMU just ignore the bits for ppn.
>
> Yes, from reading the spec that seems to be the correct behaviour.
Thank you very much :)
>
>>
>>>
>>> Does this fix a bug you are seeing?
>> Yes, because we try to reuse these bits as attributes.
>
> That isn't really a bug then, the spec says not to do that.
Yes, I'll change the tile that "Ignore reserved bits in PTE for RV64"
>
>>
>>>
>>>>
>>>> Changelog V2:
>>>> - Bugfix pte destroyed cause boot fail
>>>> - Change to AND with a mask instead of shifting both directions
>>>
>>> The changelog shouldn't be in the commit, it should be kept under the
>>> line line below.
>> I just prefer to save them in commit.
>
> Fair, but in QEMU we don't commit the change log in the commit.
Ok.
>
>>
>>>
>>>>
>>>> Signed-off-by: Guo Ren <ren_guo@c-sky.com>
>>>> Reviewed-by: Liu Zhiwei <zhiwei_liu@c-sky.com>
>>>> ---
>>>
>>> The change log should go here.
>> OK, but git am we’ll lose them.
>>
>>>
>>>> target/riscv/cpu_bits.h | 3 +++
>>>> target/riscv/cpu_helper.c | 3 ++-
>>>> 2 files changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>>>> index e998348..ae8aa0f 100644
>>>> --- a/target/riscv/cpu_bits.h
>>>> +++ b/target/riscv/cpu_bits.h
>>>> @@ -470,6 +470,9 @@
>>>> #define PTE_D 0x080 /* Dirty */
>>>> #define PTE_SOFT 0x300 /* Reserved for Software */
>>>>
>>>> +/* Reserved highest 10 bits in PTE */
>>>> +#define PTE_RESERVED ((target_ulong)0x3ff << 54)
>>>
>>> I think it's just easier to define this as 0xFFC0000000000000ULL and
>>> remove the cast.
>> OK follow your rule, but I still prefer prior.
>>
>>>
>>>> +
>>>> /* Page table PPN shift amount */
>>>> #define PTE_PPN_SHIFT 10
>>>>
>>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>>> index 87dd6a6..7a540cc 100644
>>>> --- a/target/riscv/cpu_helper.c
>>>> +++ b/target/riscv/cpu_helper.c
>>>> @@ -258,10 +258,11 @@ restart:
>>>> }
>>>> #if defined(TARGET_RISCV32)
>>>> target_ulong pte = ldl_phys(cs->as, pte_addr);
>>>> + hwaddr ppn = pte >> PTE_PPN_SHIFT;
>>>> #elif defined(TARGET_RISCV64)
>>>> target_ulong pte = ldq_phys(cs->as, pte_addr);
>>>> + hwaddr ppn = (pte & ~PTE_RESERVED) >> PTE_PPN_SHIFT;
>>>> #endif
>>>> - hwaddr ppn = pte >> PTE_PPN_SHIFT;
>>>
>>> You don't have to move this shift
>> En… Do you want this: ?
>>
>> #if defined(TARGET_RISCV32)
>> target_ulong pte = ldl_phys(cs->as, pte_addr);
>> + hwaddr ppn = pte;
>> #elif defined(TARGET_RISCV64)
>> target_ulong pte = ldq_phys(cs->as, pte_addr);
>> + hwaddr ppn = (pte & ~PTE_RESERVED);
>> #endif
>> + ppn = ppn >> PTE_PPN_SHIFT;
>>
>
> Yeah, it seems a little cleaner.
:)
Best Regards
Guo Ren
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
@ 2019-09-25 1:02 ` Guo Ren
0 siblings, 0 replies; 11+ messages in thread
From: Guo Ren @ 2019-09-25 1:02 UTC (permalink / raw)
To: Alistair Francis
Cc: guoren, qemu-devel@nongnu.org Developers, open list:RISC-V,
Alistair Francis, Palmer Dabbelt
在 2019/9/25 上午8:21, Alistair Francis 写道:
> On Tue, Sep 24, 2019 at 5:13 PM Guo Ren <ren_guo@c-sky.com> wrote:
>>
>>
>>> 在 2019年9月25日,上午7:33,Alistair Francis <alistair23@gmail.com> 写道:
>>>
>>> On Tue, Sep 24, 2019 at 12:58 AM <guoren@kernel.org> wrote:
>>>>
>>>> From: Guo Ren <ren_guo@c-sky.com>
>>>>
>>>> Highest 10 bits of PTE are reserved in riscv-privileged, ref: [1], so we
>>>> need to ignore them. They can not be a part of ppn.
>>>>
>>>> 1: The RISC-V Instruction Set Manual, Volume II: Privileged Architecture
>>>> 4.4 Sv39: Page-Based 39-bit Virtual-Memory System
>>>> 4.5 Sv48: Page-Based 48-bit Virtual-Memory System
>>>
>>> Thanks for the patch!
>>>
>>> The spec says "must be zeroed by software for forward compatibility"
>>> so I don't think it's correct for QEMU to zero out the bits.
>> QEMU don’t zero out the bits, QEMU just ignore the bits for ppn.
>
> Yes, from reading the spec that seems to be the correct behaviour.
Thank you very much :)
>
>>
>>>
>>> Does this fix a bug you are seeing?
>> Yes, because we try to reuse these bits as attributes.
>
> That isn't really a bug then, the spec says not to do that.
Yes, I'll change the tile that "Ignore reserved bits in PTE for RV64"
>
>>
>>>
>>>>
>>>> Changelog V2:
>>>> - Bugfix pte destroyed cause boot fail
>>>> - Change to AND with a mask instead of shifting both directions
>>>
>>> The changelog shouldn't be in the commit, it should be kept under the
>>> line line below.
>> I just prefer to save them in commit.
>
> Fair, but in QEMU we don't commit the change log in the commit.
Ok.
>
>>
>>>
>>>>
>>>> Signed-off-by: Guo Ren <ren_guo@c-sky.com>
>>>> Reviewed-by: Liu Zhiwei <zhiwei_liu@c-sky.com>
>>>> ---
>>>
>>> The change log should go here.
>> OK, but git am we’ll lose them.
>>
>>>
>>>> target/riscv/cpu_bits.h | 3 +++
>>>> target/riscv/cpu_helper.c | 3 ++-
>>>> 2 files changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
>>>> index e998348..ae8aa0f 100644
>>>> --- a/target/riscv/cpu_bits.h
>>>> +++ b/target/riscv/cpu_bits.h
>>>> @@ -470,6 +470,9 @@
>>>> #define PTE_D 0x080 /* Dirty */
>>>> #define PTE_SOFT 0x300 /* Reserved for Software */
>>>>
>>>> +/* Reserved highest 10 bits in PTE */
>>>> +#define PTE_RESERVED ((target_ulong)0x3ff << 54)
>>>
>>> I think it's just easier to define this as 0xFFC0000000000000ULL and
>>> remove the cast.
>> OK follow your rule, but I still prefer prior.
>>
>>>
>>>> +
>>>> /* Page table PPN shift amount */
>>>> #define PTE_PPN_SHIFT 10
>>>>
>>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>>> index 87dd6a6..7a540cc 100644
>>>> --- a/target/riscv/cpu_helper.c
>>>> +++ b/target/riscv/cpu_helper.c
>>>> @@ -258,10 +258,11 @@ restart:
>>>> }
>>>> #if defined(TARGET_RISCV32)
>>>> target_ulong pte = ldl_phys(cs->as, pte_addr);
>>>> + hwaddr ppn = pte >> PTE_PPN_SHIFT;
>>>> #elif defined(TARGET_RISCV64)
>>>> target_ulong pte = ldq_phys(cs->as, pte_addr);
>>>> + hwaddr ppn = (pte & ~PTE_RESERVED) >> PTE_PPN_SHIFT;
>>>> #endif
>>>> - hwaddr ppn = pte >> PTE_PPN_SHIFT;
>>>
>>> You don't have to move this shift
>> En… Do you want this: ?
>>
>> #if defined(TARGET_RISCV32)
>> target_ulong pte = ldl_phys(cs->as, pte_addr);
>> + hwaddr ppn = pte;
>> #elif defined(TARGET_RISCV64)
>> target_ulong pte = ldq_phys(cs->as, pte_addr);
>> + hwaddr ppn = (pte & ~PTE_RESERVED);
>> #endif
>> + ppn = ppn >> PTE_PPN_SHIFT;
>>
>
> Yeah, it seems a little cleaner.
:)
Best Regards
Guo Ren
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-09-25 1:04 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-24 7:58 [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64 guoren
2019-09-24 7:58 ` guoren
2019-09-24 8:02 ` Guo Ren
2019-09-24 23:33 ` Alistair Francis
2019-09-24 23:33 ` Alistair Francis
2019-09-25 0:13 ` Guo Ren
2019-09-25 0:13 ` Guo Ren
2019-09-25 0:21 ` Alistair Francis
2019-09-25 0:21 ` Alistair Francis
2019-09-25 1:02 ` Guo Ren
2019-09-25 1:02 ` Guo Ren
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.