The spec states that on sv39x4 guest physical  "address bits 63:41 must
all be zeros, or else a guest-page-fault exception occurs.".  However,
the check performed for these top bits of the virtual address on the
second stage is the same as the one performed for virtual addresses on
the first stage except with the 2-bit extension, effectively creating
the same kind of "hole" in the guest's physical address space. I believe
the following patch fixes this issue:

Signed-off-by: Jose Martins <josemartins90@gmail.com>
---
 target/riscv/cpu_helper.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 247304d850..ae22c30bdd 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -426,15 +426,21 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
     int va_bits = PGSHIFT + levels * ptidxbits + widened;
     target_ulong mask, masked_msbs;
 
-    if (TARGET_LONG_BITS > (va_bits - 1)) {
-        mask = (1L << (TARGET_LONG_BITS - (va_bits - 1))) - 1;
+    if (!first_stage) {
+        if ((addr >> va_bits) != 0) {
+            return TRANSLATE_FAIL;
+        }
     } else {
-        mask = 0;
-    }
-    masked_msbs = (addr >> (va_bits - 1)) & mask;
+        if (TARGET_LONG_BITS > (va_bits - 1)) {
+            mask = (1L << (TARGET_LONG_BITS - (va_bits - 1))) - 1;
+        } else {
+            mask = 0;
+        }
+        masked_msbs = (addr >> (va_bits - 1)) & mask;
 
-    if (masked_msbs != 0 && masked_msbs != mask) {
-        return TRANSLATE_FAIL;
+        if (masked_msbs != 0 && masked_msbs != mask) {
+            return TRANSLATE_FAIL;
+        }
     }
 
     int ptshift = (levels - 1) * ptidxbits;
-- 
2.25.1

On Fri, May 1, 2020 at 11:51 AM Jose Martins <josemartins90@gmail.com> wrote:
>
> The spec states that on sv39x4 guest physical  "address bits 63:41 must
> all be zeros, or else a guest-page-fault exception occurs.".  However,
> the check performed for these top bits of the virtual address on the
> second stage is the same as the one performed for virtual addresses on
> the first stage except with the 2-bit extension, effectively creating
> the same kind of "hole" in the guest's physical address space. I believe
> the following patch fixes this issue:
>
> Signed-off-by: Jose Martins <josemartins90@gmail.com>

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

Applied to RISC-V tree.

Alistair

> ---
>  target/riscv/cpu_helper.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 247304d850..ae22c30bdd 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -426,15 +426,21 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
>      int va_bits = PGSHIFT + levels * ptidxbits + widened;
>      target_ulong mask, masked_msbs;
>
> -    if (TARGET_LONG_BITS > (va_bits - 1)) {
> -        mask = (1L << (TARGET_LONG_BITS - (va_bits - 1))) - 1;
> +    if (!first_stage) {
> +        if ((addr >> va_bits) != 0) {
> +            return TRANSLATE_FAIL;
> +        }
>      } else {
> -        mask = 0;
> -    }
> -    masked_msbs = (addr >> (va_bits - 1)) & mask;
> +        if (TARGET_LONG_BITS > (va_bits - 1)) {
> +            mask = (1L << (TARGET_LONG_BITS - (va_bits - 1))) - 1;
> +        } else {
> +            mask = 0;
> +        }
> +        masked_msbs = (addr >> (va_bits - 1)) & mask;
>
> -    if (masked_msbs != 0 && masked_msbs != mask) {
> -        return TRANSLATE_FAIL;
> +        if (masked_msbs != 0 && masked_msbs != mask) {
> +            return TRANSLATE_FAIL;
> +        }
>      }
>
>      int ptshift = (levels - 1) * ptidxbits;
> --
> 2.25.1
>
>

On Tue, May 5, 2020 at 1:40 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Fri, May 1, 2020 at 11:51 AM Jose Martins <josemartins90@gmail.com> wrote:
> >
> > The spec states that on sv39x4 guest physical  "address bits 63:41 must
> > all be zeros, or else a guest-page-fault exception occurs.".  However,
> > the check performed for these top bits of the virtual address on the
> > second stage is the same as the one performed for virtual addresses on
> > the first stage except with the 2-bit extension, effectively creating
> > the same kind of "hole" in the guest's physical address space. I believe
> > the following patch fixes this issue:
> >
> > Signed-off-by: Jose Martins <josemartins90@gmail.com>
>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>
> Applied to RISC-V tree.

This breaks 32-bit Hypervisors, can you look into it?

Alistair

>
> Alistair
>
> > ---
> >  target/riscv/cpu_helper.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 247304d850..ae22c30bdd 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -426,15 +426,21 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
> >      int va_bits = PGSHIFT + levels * ptidxbits + widened;
> >      target_ulong mask, masked_msbs;
> >
> > -    if (TARGET_LONG_BITS > (va_bits - 1)) {
> > -        mask = (1L << (TARGET_LONG_BITS - (va_bits - 1))) - 1;
> > +    if (!first_stage) {
> > +        if ((addr >> va_bits) != 0) {
> > +            return TRANSLATE_FAIL;
> > +        }
> >      } else {
> > -        mask = 0;
> > -    }
> > -    masked_msbs = (addr >> (va_bits - 1)) & mask;
> > +        if (TARGET_LONG_BITS > (va_bits - 1)) {
> > +            mask = (1L << (TARGET_LONG_BITS - (va_bits - 1))) - 1;
> > +        } else {
> > +            mask = 0;
> > +        }
> > +        masked_msbs = (addr >> (va_bits - 1)) & mask;
> >
> > -    if (masked_msbs != 0 && masked_msbs != mask) {
> > -        return TRANSLATE_FAIL;
> > +        if (masked_msbs != 0 && masked_msbs != mask) {
> > +            return TRANSLATE_FAIL;
> > +        }
> >      }
> >
> >      int ptshift = (levels - 1) * ptidxbits;
> > --
> > 2.25.1
> >
> >