1. Patchew
  2. QEMU
  3. View series

[PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64

guoren@kernel.org posted 1 patch 4 years, 7 months ago
Test docker-clang@ubuntu passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test checkpatch passed
Test asan passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1569311902-12173-1-git-send-email-guoren@kernel.org
Maintainers: Palmer Dabbelt <palmer@sifive.com>, Alistair Francis <Alistair.Francis@wdc.com>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Sagar Karandikar <sagark@eecs.berkeley.edu>
There is a newer version of this series
target/riscv/cpu_bits.h   | 3 +++
target/riscv/cpu_helper.c | 3 ++-
2 files changed, 5 insertions(+), 1 deletion(-)
[PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
Posted by guoren@kernel.org 4 years, 7 months ago 
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
Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
Posted by Guo Ren 4 years, 7 months ago 
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/
Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
Posted by Alistair Francis 4 years, 7 months ago 
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
>
Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
Posted by Guo Ren 4 years, 7 months ago 
> 在 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.
Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
Posted by Alistair Francis 4 years, 7 months ago 
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.
>
>
Re: [PATCH V2] target/riscv: Bugfix reserved bits in PTE for RV64
Posted by Guo Ren 4 years, 7 months ago 
在 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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.