QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Qemu-devel] [PATCH] riscv: rv32: Root page table address can be larger than 32-bit
@ 2019-07-31 12:45 Bin Meng
  2019-07-31 17:35 ` Richard Henderson
  2019-08-08  2:49 ` [Qemu-devel] [PATCH v2] " Bin Meng
  0 siblings, 2 replies; 12+ messages in thread
From: Bin Meng @ 2019-07-31 12:45 UTC (permalink / raw)
  To: Alistair Francis, Palmer Dabbelt, Bastian Koppelmann,
	Sagar Karandikar, QEMU devel, QEMU riscv

For RV32, the root page table's PPN has 22 bits hence its address
bits could be larger than the maximum bits that target_ulong is
able to represent. Use hwaddr instead.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
---

 target/riscv/cpu_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index e32b612..3150a6a 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -176,7 +176,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
 
     *prot = 0;
 
-    target_ulong base;
+    hwaddr base;
     int levels, ptidxbits, ptesize, vm, sum;
     int mxr = get_field(env->mstatus, MSTATUS_MXR);
 
@@ -239,7 +239,7 @@ restart:
                            ((1 << ptidxbits) - 1);
 
         /* check that physical address of PTE is legal */
-        target_ulong pte_addr = base + idx * ptesize;
+        hwaddr pte_addr = base + idx * ptesize;
 
         if (riscv_feature(env, RISCV_FEATURE_PMP) &&
             !pmp_hart_has_privs(env, pte_addr, sizeof(target_ulong),
-- 
2.7.4



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] riscv: rv32: Root page table address can be larger than 32-bit
  2019-07-31 12:45 [Qemu-devel] [PATCH] riscv: rv32: Root page table address can be larger than 32-bit Bin Meng
@ 2019-07-31 17:35 ` Richard Henderson
  2019-08-01  1:53   ` Bin Meng
  2019-08-08  2:49 ` [Qemu-devel] [PATCH v2] " Bin Meng
  1 sibling, 1 reply; 12+ messages in thread
From: Richard Henderson @ 2019-07-31 17:35 UTC (permalink / raw)
  To: Bin Meng, Alistair Francis, Palmer Dabbelt, Bastian Koppelmann,
	Sagar Karandikar, QEMU devel, QEMU riscv

On 7/31/19 5:45 AM, Bin Meng wrote:
> -    target_ulong base;
> +    hwaddr base;
...
> -        target_ulong pte_addr = base + idx * ptesize;
> +        hwaddr pte_addr = base + idx * ptesize;

I believe that you either need

    base + (hwaddr)idx * ptesize

or change the type of idx to hwaddr above.

Otherwise the multiply overflows before it gets promoted with the add.


r~


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] riscv: rv32: Root page table address can be larger than 32-bit
  2019-07-31 17:35 ` Richard Henderson
@ 2019-08-01  1:53   ` Bin Meng
  2019-08-01 14:16     ` Richard Henderson
  0 siblings, 1 reply; 12+ messages in thread
From: Bin Meng @ 2019-08-01  1:53 UTC (permalink / raw)
  To: Richard Henderson
  Cc: QEMU riscv, Sagar Karandikar, Bastian Koppelmann, Palmer Dabbelt,
	QEMU devel, Alistair Francis

Hi Richard,

On Thu, Aug 1, 2019 at 1:35 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 7/31/19 5:45 AM, Bin Meng wrote:
> > -    target_ulong base;
> > +    hwaddr base;
> ...
> > -        target_ulong pte_addr = base + idx * ptesize;
> > +        hwaddr pte_addr = base + idx * ptesize;
>
> I believe that you either need
>
>     base + (hwaddr)idx * ptesize
>
> or change the type of idx to hwaddr above.
>
> Otherwise the multiply overflows before it gets promoted with the add.
>

I am not sure how (idx * ptesize) could overflow. It represents the
offset by a page table which is [0, 4096).

Regards,
Bin


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] riscv: rv32: Root page table address can be larger than 32-bit
  2019-08-01  1:53   ` Bin Meng
@ 2019-08-01 14:16     ` Richard Henderson
  2019-08-01 14:57       ` Bin Meng
  0 siblings, 1 reply; 12+ messages in thread
From: Richard Henderson @ 2019-08-01 14:16 UTC (permalink / raw)
  To: Bin Meng
  Cc: QEMU riscv, Sagar Karandikar, Bastian Koppelmann, Palmer Dabbelt,
	QEMU devel, Alistair Francis

On 7/31/19 6:53 PM, Bin Meng wrote:
> I am not sure how (idx * ptesize) could overflow. It represents the
> offset by a page table which is [0, 4096).

You're right, I mis-read what was going on there.

However, lower down, "target_ulong ppn" needs to be promoted to hwaddr, so that

    ppn = pte >> PTE_PPN_SHIFT;
    ...
    base = ppn << PGSHIFT;

does not overflow.  (Which is the part of the page table walk that I thought I
had gleaned from the patch without actually reading the entire function.)


r~


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] riscv: rv32: Root page table address can be larger than 32-bit
  2019-08-01 14:16     ` Richard Henderson
@ 2019-08-01 14:57       ` Bin Meng
  2019-08-07 20:55         ` Palmer Dabbelt
  0 siblings, 1 reply; 12+ messages in thread
From: Bin Meng @ 2019-08-01 14:57 UTC (permalink / raw)
  To: Richard Henderson
  Cc: QEMU riscv, Sagar Karandikar, Bastian Koppelmann, Palmer Dabbelt,
	QEMU devel, Alistair Francis

On Thu, Aug 1, 2019 at 10:16 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 7/31/19 6:53 PM, Bin Meng wrote:
> > I am not sure how (idx * ptesize) could overflow. It represents the
> > offset by a page table which is [0, 4096).
>
> You're right, I mis-read what was going on there.
>
> However, lower down, "target_ulong ppn" needs to be promoted to hwaddr, so that
>
>     ppn = pte >> PTE_PPN_SHIFT;
>     ...
>     base = ppn << PGSHIFT;
>
> does not overflow.  (Which is the part of the page table walk that I thought I
> had gleaned from the patch without actually reading the entire function.)

Ah, yes. ppn should be promoted. Thanks for the review!

Regards,
Bin


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] riscv: rv32: Root page table address can be larger than 32-bit
  2019-08-01 14:57       ` Bin Meng
@ 2019-08-07 20:55         ` Palmer Dabbelt
  2019-08-08  1:49           ` Bin Meng
  0 siblings, 1 reply; 12+ messages in thread
From: Palmer Dabbelt @ 2019-08-07 20:55 UTC (permalink / raw)
  To: Bin Meng
  Cc: QEMU riscv, Sagar Karandikar, Bastian Koppelmann, Palmer Dabbelt,
	Richard Henderson, QEMU devel, Alistair Francis

On Thu, Aug 1, 2019 at 7:58 AM Bin Meng <bmeng.cn@gmail.com> wrote:

> On Thu, Aug 1, 2019 at 10:16 PM Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > On 7/31/19 6:53 PM, Bin Meng wrote:
> > > I am not sure how (idx * ptesize) could overflow. It represents the
> > > offset by a page table which is [0, 4096).
> >
> > You're right, I mis-read what was going on there.
> >
> > However, lower down, "target_ulong ppn" needs to be promoted to hwaddr,
> so that
> >
> >     ppn = pte >> PTE_PPN_SHIFT;
> >     ...
> >     base = ppn << PGSHIFT;
> >
> > does not overflow.  (Which is the part of the page table walk that I
> thought I
> > had gleaned from the patch without actually reading the entire function.)
>
> Ah, yes. ppn should be promoted. Thanks for the review!
>

Did I miss a v2?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH] riscv: rv32: Root page table address can be larger than 32-bit
  2019-08-07 20:55         ` Palmer Dabbelt
@ 2019-08-08  1:49           ` Bin Meng
  0 siblings, 0 replies; 12+ messages in thread
From: Bin Meng @ 2019-08-08  1:49 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: QEMU riscv, Sagar Karandikar, Bastian Koppelmann,
	Richard Henderson, QEMU devel, Alistair Francis

Hi Palmer,

On Thu, Aug 8, 2019 at 4:55 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Thu, Aug 1, 2019 at 7:58 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>> On Thu, Aug 1, 2019 at 10:16 PM Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>> >
>> > On 7/31/19 6:53 PM, Bin Meng wrote:
>> > > I am not sure how (idx * ptesize) could overflow. It represents the
>> > > offset by a page table which is [0, 4096).
>> >
>> > You're right, I mis-read what was going on there.
>> >
>> > However, lower down, "target_ulong ppn" needs to be promoted to hwaddr, so that
>> >
>> >     ppn = pte >> PTE_PPN_SHIFT;
>> >     ...
>> >     base = ppn << PGSHIFT;
>> >
>> > does not overflow.  (Which is the part of the page table walk that I thought I
>> > had gleaned from the patch without actually reading the entire function.)
>>
>> Ah, yes. ppn should be promoted. Thanks for the review!
>
>
> Did I miss a v2?

No, I will send a v2 soon.

Regards,
Bin


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PATCH v2] riscv: rv32: Root page table address can be larger than 32-bit
  2019-07-31 12:45 [Qemu-devel] [PATCH] riscv: rv32: Root page table address can be larger than 32-bit Bin Meng
  2019-07-31 17:35 ` Richard Henderson
@ 2019-08-08  2:49 ` " Bin Meng
  2019-08-10  1:49   ` Alistair Francis
  1 sibling, 1 reply; 12+ messages in thread
From: Bin Meng @ 2019-08-08  2:49 UTC (permalink / raw)
  To: Alistair Francis, Bastian Koppelmann, Palmer Dabbelt,
	Sagar Karandikar, Richard Henderson, qemu-devel, qemu-riscv

For RV32, the root page table's PPN has 22 bits hence its address
bits could be larger than the maximum bits that target_ulong is
able to represent. Use hwaddr instead.

Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

Changes in v2:
- promote ppn, env->satp/env->sptbl to hwaddr otherwise the page
  table base will not be correctly calculated

 target/riscv/cpu_helper.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index e32b612..b2b4f3a 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -176,12 +176,12 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
 
     *prot = 0;
 
-    target_ulong base;
+    hwaddr base;
     int levels, ptidxbits, ptesize, vm, sum;
     int mxr = get_field(env->mstatus, MSTATUS_MXR);
 
     if (env->priv_ver >= PRIV_VERSION_1_10_0) {
-        base = get_field(env->satp, SATP_PPN) << PGSHIFT;
+        base = (hwaddr)get_field(env->satp, SATP_PPN) << PGSHIFT;
         sum = get_field(env->mstatus, MSTATUS_SUM);
         vm = get_field(env->satp, SATP_MODE);
         switch (vm) {
@@ -201,7 +201,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
           g_assert_not_reached();
         }
     } else {
-        base = env->sptbr << PGSHIFT;
+        base = (hwaddr)(env->sptbr) << PGSHIFT;
         sum = !get_field(env->mstatus, MSTATUS_PUM);
         vm = get_field(env->mstatus, MSTATUS_VM);
         switch (vm) {
@@ -239,7 +239,7 @@ restart:
                            ((1 << ptidxbits) - 1);
 
         /* check that physical address of PTE is legal */
-        target_ulong pte_addr = base + idx * ptesize;
+        hwaddr pte_addr = base + idx * ptesize;
 
         if (riscv_feature(env, RISCV_FEATURE_PMP) &&
             !pmp_hart_has_privs(env, pte_addr, sizeof(target_ulong),
@@ -251,7 +251,7 @@ restart:
 #elif defined(TARGET_RISCV64)
         target_ulong pte = ldq_phys(cs->as, pte_addr);
 #endif
-        target_ulong ppn = pte >> PTE_PPN_SHIFT;
+        hwaddr ppn = pte >> PTE_PPN_SHIFT;
 
         if (!(pte & PTE_V)) {
             /* Invalid PTE */
-- 
2.7.4



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH v2] riscv: rv32: Root page table address can be larger than 32-bit
  2019-08-08  2:49 ` [Qemu-devel] [PATCH v2] " Bin Meng
@ 2019-08-10  1:49   ` Alistair Francis
  2019-08-14  9:46     ` Bin Meng
  0 siblings, 1 reply; 12+ messages in thread
From: Alistair Francis @ 2019-08-10  1:49 UTC (permalink / raw)
  To: Bin Meng
  Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
	Palmer Dabbelt, Richard Henderson,
	qemu-devel@nongnu.org Developers, Alistair Francis

On Wed, Aug 7, 2019 at 7:50 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> For RV32, the root page table's PPN has 22 bits hence its address
> bits could be larger than the maximum bits that target_ulong is
> able to represent. Use hwaddr instead.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

>
> ---
>
> Changes in v2:
> - promote ppn, env->satp/env->sptbl to hwaddr otherwise the page
>   table base will not be correctly calculated
>
>  target/riscv/cpu_helper.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index e32b612..b2b4f3a 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -176,12 +176,12 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>
>      *prot = 0;
>
> -    target_ulong base;
> +    hwaddr base;
>      int levels, ptidxbits, ptesize, vm, sum;
>      int mxr = get_field(env->mstatus, MSTATUS_MXR);
>
>      if (env->priv_ver >= PRIV_VERSION_1_10_0) {
> -        base = get_field(env->satp, SATP_PPN) << PGSHIFT;
> +        base = (hwaddr)get_field(env->satp, SATP_PPN) << PGSHIFT;
>          sum = get_field(env->mstatus, MSTATUS_SUM);
>          vm = get_field(env->satp, SATP_MODE);
>          switch (vm) {
> @@ -201,7 +201,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>            g_assert_not_reached();
>          }
>      } else {
> -        base = env->sptbr << PGSHIFT;
> +        base = (hwaddr)(env->sptbr) << PGSHIFT;
>          sum = !get_field(env->mstatus, MSTATUS_PUM);
>          vm = get_field(env->mstatus, MSTATUS_VM);
>          switch (vm) {
> @@ -239,7 +239,7 @@ restart:
>                             ((1 << ptidxbits) - 1);
>
>          /* check that physical address of PTE is legal */
> -        target_ulong pte_addr = base + idx * ptesize;
> +        hwaddr pte_addr = base + idx * ptesize;
>
>          if (riscv_feature(env, RISCV_FEATURE_PMP) &&
>              !pmp_hart_has_privs(env, pte_addr, sizeof(target_ulong),
> @@ -251,7 +251,7 @@ restart:
>  #elif defined(TARGET_RISCV64)
>          target_ulong pte = ldq_phys(cs->as, pte_addr);
>  #endif
> -        target_ulong ppn = pte >> PTE_PPN_SHIFT;
> +        hwaddr ppn = pte >> PTE_PPN_SHIFT;
>
>          if (!(pte & PTE_V)) {
>              /* Invalid PTE */
> --
> 2.7.4
>
>


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH v2] riscv: rv32: Root page table address can be larger than 32-bit
  2019-08-10  1:49   ` Alistair Francis
@ 2019-08-14  9:46     ` Bin Meng
  2019-08-19  6:00       ` Bin Meng
  0 siblings, 1 reply; 12+ messages in thread
From: Bin Meng @ 2019-08-14  9:46 UTC (permalink / raw)
  To: Alistair Francis
  Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
	Palmer Dabbelt, Richard Henderson,
	qemu-devel@nongnu.org Developers, Alistair Francis

Hi Palmer,

On Sat, Aug 10, 2019 at 9:49 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Wed, Aug 7, 2019 at 7:50 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > For RV32, the root page table's PPN has 22 bits hence its address
> > bits could be larger than the maximum bits that target_ulong is
> > able to represent. Use hwaddr instead.
> >
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>

Would you take this one too?

Regards,
Bin


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH v2] riscv: rv32: Root page table address can be larger than 32-bit
  2019-08-14  9:46     ` Bin Meng
@ 2019-08-19  6:00       ` Bin Meng
  2019-08-27 23:37         ` Palmer Dabbelt
  0 siblings, 1 reply; 12+ messages in thread
From: Bin Meng @ 2019-08-19  6:00 UTC (permalink / raw)
  To: Alistair Francis
  Cc: open list:RISC-V, Sagar Karandikar, Bastian Koppelmann,
	Palmer Dabbelt, Richard Henderson,
	qemu-devel@nongnu.org Developers, Alistair Francis

On Wed, Aug 14, 2019 at 5:46 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Palmer,
>
> On Sat, Aug 10, 2019 at 9:49 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Wed, Aug 7, 2019 at 7:50 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > For RV32, the root page table's PPN has 22 bits hence its address
> > > bits could be larger than the maximum bits that target_ulong is
> > > able to represent. Use hwaddr instead.
> > >
> > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> >
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> >
>
> Would you take this one too?
>

Ping?

What's the status of this patch?

Regards,
Bin


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PATCH v2] riscv: rv32: Root page table address can be larger than 32-bit
  2019-08-19  6:00       ` Bin Meng
@ 2019-08-27 23:37         ` Palmer Dabbelt
  0 siblings, 0 replies; 12+ messages in thread
From: Palmer Dabbelt @ 2019-08-27 23:37 UTC (permalink / raw)
  To: bmeng.cn
  Cc: qemu-riscv, sagark, Bastian Koppelmann, richard.henderson,
	qemu-devel, Alistair Francis, alistair23

On Sun, 18 Aug 2019 23:00:40 PDT (-0700), bmeng.cn@gmail.com wrote:
> On Wed, Aug 14, 2019 at 5:46 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>> Hi Palmer,
>>
>> On Sat, Aug 10, 2019 at 9:49 AM Alistair Francis <alistair23@gmail.com> wrote:
>> >
>> > On Wed, Aug 7, 2019 at 7:50 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>> > >
>> > > For RV32, the root page table's PPN has 22 bits hence its address
>> > > bits could be larger than the maximum bits that target_ulong is
>> > > able to represent. Use hwaddr instead.
>> > >
>> > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> >
>> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> >
>>
>> Would you take this one too?
>>
>
> Ping?
>
> What's the status of this patch?

Also in the patch queue.


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-31 12:45 [Qemu-devel] [PATCH] riscv: rv32: Root page table address can be larger than 32-bit Bin Meng
2019-07-31 17:35 ` Richard Henderson
2019-08-01  1:53   ` Bin Meng
2019-08-01 14:16     ` Richard Henderson
2019-08-01 14:57       ` Bin Meng
2019-08-07 20:55         ` Palmer Dabbelt
2019-08-08  1:49           ` Bin Meng
2019-08-08  2:49 ` [Qemu-devel] [PATCH v2] " Bin Meng
2019-08-10  1:49   ` Alistair Francis
2019-08-14  9:46     ` Bin Meng
2019-08-19  6:00       ` Bin Meng
2019-08-27 23:37         ` Palmer Dabbelt

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox