[PATCH v2 0/3] ppc: Check for bad Radix configs

[PATCH v2 0/3] ppc: Check for bad Radix configs

[Leandro Lupori]

Changes from v1:
- Use proper format defines in logs
- Optimized ppc_radix64_is_valid_level() and added a comment with
  instructions on how to proceed when adding new Radix CPUs with
  different configurations
- Moved calls to ppc_radix64_is_valid_level() outside of
  ppc_radix64_next_level(). This also avoids calling it twice for
  level 0, through ppc_radix64_walk_tree().
- Removed debug ifdefs from PDE/PTE alignment checks

Leandro Lupori (3):
  ppc: Check partition and process table alignment
  target/ppc: Improve Radix xlate level validation
  target/ppc: Check page dir/table base alignment

 hw/ppc/spapr.c             |  5 +++
 hw/ppc/spapr_hcall.c       |  9 +++++
 target/ppc/mmu-book3s-v3.c |  5 +++
 target/ppc/mmu-radix64.c   | 79 +++++++++++++++++++++++++++++++-------
 4 files changed, 85 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] ppc: Check partition and process table alignment

[Leandro Lupori]

Check if partition and process tables are properly aligned, in
their size, according to PowerISA 3.1B, Book III 6.7.6 programming
note. Hardware and KVM also raise an exception in these cases.

Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
---
 hw/ppc/spapr.c             |  5 +++++
 hw/ppc/spapr_hcall.c       |  9 +++++++++
 target/ppc/mmu-book3s-v3.c |  5 +++++
 target/ppc/mmu-radix64.c   | 17 +++++++++++++----
 4 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index fd4942e881..4b1f346087 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1329,6 +1329,11 @@ static bool spapr_get_pate(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu,
         patb = spapr->nested_ptcr & PTCR_PATB;
         pats = spapr->nested_ptcr & PTCR_PATS;
 
+        /* Check if partition table is properly aligned */
+        if (patb & MAKE_64BIT_MASK(0, pats + 12)) {
+            return false;
+        }
+
         /* Calculate number of entries */
         pats = 1ull << (pats + 12 - 4);
         if (pats <= lpid) {
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index d761a7d0c3..a8d4a6bcf0 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -920,6 +920,7 @@ static target_ulong h_register_process_table(PowerPCCPU *cpu,
     target_ulong page_size = args[2];
     target_ulong table_size = args[3];
     target_ulong update_lpcr = 0;
+    target_ulong table_byte_size;
     uint64_t cproc;
 
     if (flags & ~FLAGS_MASK) { /* Check no reserved bits are set */
@@ -927,6 +928,14 @@ static target_ulong h_register_process_table(PowerPCCPU *cpu,
     }
     if (flags & FLAG_MODIFY) {
         if (flags & FLAG_REGISTER) {
+            /* Check process table alignment */
+            table_byte_size = 1ULL << (table_size + 12);
+            if (proc_tbl & (table_byte_size - 1)) {
+                qemu_log_mask(LOG_GUEST_ERROR,
+                    "%s: process table not properly aligned: proc_tbl 0x"
+                    TARGET_FMT_lx" proc_tbl_size 0x"TARGET_FMT_lx"\n",
+                    __func__, proc_tbl, table_byte_size);
+            }
             if (flags & FLAG_RADIX) { /* Register new RADIX process table */
                 if (proc_tbl & 0xfff || proc_tbl >> 60) {
                     return H_P2;
diff --git a/target/ppc/mmu-book3s-v3.c b/target/ppc/mmu-book3s-v3.c
index f4985bae78..c8f69b3df9 100644
--- a/target/ppc/mmu-book3s-v3.c
+++ b/target/ppc/mmu-book3s-v3.c
@@ -28,6 +28,11 @@ bool ppc64_v3_get_pate(PowerPCCPU *cpu, target_ulong lpid, ppc_v3_pate_t *entry)
     uint64_t patb = cpu->env.spr[SPR_PTCR] & PTCR_PATB;
     uint64_t pats = cpu->env.spr[SPR_PTCR] & PTCR_PATS;
 
+    /* Check if partition table is properly aligned */
+    if (patb & MAKE_64BIT_MASK(0, pats + 12)) {
+        return false;
+    }
+
     /* Calculate number of entries */
     pats = 1ull << (pats + 12 - 4);
     if (pats <= lpid) {
diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
index 21ac958e48..9a8a2e2875 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -383,7 +383,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
 {
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
-    uint64_t offset, size, prtbe_addr, prtbe0, base_addr, nls, index, pte;
+    uint64_t offset, size, prtb, prtbe_addr, prtbe0, base_addr, nls, index, pte;
     int fault_cause = 0, h_page_size, h_prot;
     hwaddr h_raddr, pte_addr;
     int ret;
@@ -393,9 +393,18 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
                   __func__, access_str(access_type),
                   eaddr, mmu_idx, pid);
 
+    prtb = (pate.dw1 & PATE1_R_PRTB);
+    size = 1ULL << ((pate.dw1 & PATE1_R_PRTS) + 12);
+    if (prtb & (size - 1)) {
+        /* Process Table not properly aligned */
+        if (guest_visible) {
+            ppc_radix64_raise_si(cpu, access_type, eaddr, DSISR_R_BADCONFIG);
+        }
+        return 1;
+    }
+
     /* Index Process Table by PID to Find Corresponding Process Table Entry */
     offset = pid * sizeof(struct prtb_entry);
-    size = 1ULL << ((pate.dw1 & PATE1_R_PRTS) + 12);
     if (offset >= size) {
         /* offset exceeds size of the process table */
         if (guest_visible) {
@@ -403,7 +412,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
         }
         return 1;
     }
-    prtbe_addr = (pate.dw1 & PATE1_R_PRTB) + offset;
+    prtbe_addr = prtb + offset;
 
     if (vhyp_flat_addressing(cpu)) {
         prtbe0 = ldq_phys(cs->as, prtbe_addr);
@@ -568,7 +577,7 @@ static bool ppc_radix64_xlate_impl(PowerPCCPU *cpu, vaddr eaddr,
         return false;
     }
 
-    /* Get Process Table */
+    /* Get Partition Table */
     if (cpu->vhyp) {
         PPCVirtualHypervisorClass *vhc;
         vhc = PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
-- 
2.25.1

[PATCH v2 2/3] target/ppc: Improve Radix xlate level validation

[Leandro Lupori]

Check if the number and size of Radix levels are valid on
POWER9/POWER10 CPUs, according to the supported Radix Tree
Configurations described in their User Manuals.

Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
---
 target/ppc/mmu-radix64.c | 51 +++++++++++++++++++++++++++++++---------
 1 file changed, 40 insertions(+), 11 deletions(-)

diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
index 9a8a2e2875..339cf5b4d8 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -236,17 +236,39 @@ static void ppc_radix64_set_rc(PowerPCCPU *cpu, MMUAccessType access_type,
     }
 }
 
+static bool ppc_radix64_is_valid_level(int level, int psize, uint64_t nls)
+{
+    /*
+     * Check if this is a valid level, according to POWER9 and POWER10
+     * Processor User's Manuals, sections 4.10.4.1 and 5.10.6.1, respectively:
+     * Supported Radix Tree Configurations and Resulting Page Sizes.
+     *
+     * NOTE: these checks are valid for POWER9 and POWER10 CPUs only. If
+     *       new CPUs that support other Radix configurations are added
+     *       (e.g., Microwatt), then a new method should be added to
+     *       PowerPCCPUClass, with this function being the POWER9/POWER10
+     *       implementation.
+     */
+    switch (level) {
+    case 0:     /* Root Page Dir */
+        return psize == 52 && nls == 13;
+    case 1:
+    case 2:
+        return nls == 9;
+    case 3:
+        return nls == 9 || nls == 5;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR, "invalid radix level: %d\n", level);
+        return false;
+    }
+}
+
 static int ppc_radix64_next_level(AddressSpace *as, vaddr eaddr,
                                   uint64_t *pte_addr, uint64_t *nls,
                                   int *psize, uint64_t *pte, int *fault_cause)
 {
     uint64_t index, pde;
 
-    if (*nls < 5) { /* Directory maps less than 2**5 entries */
-        *fault_cause |= DSISR_R_BADCONFIG;
-        return 1;
-    }
-
     /* Read page <directory/table> entry from guest address space */
     pde = ldq_phys(as, *pte_addr);
     if (!(pde & R_PTE_VALID)) {         /* Invalid Entry */
@@ -270,12 +292,8 @@ static int ppc_radix64_walk_tree(AddressSpace *as, vaddr eaddr,
                                  hwaddr *raddr, int *psize, uint64_t *pte,
                                  int *fault_cause, hwaddr *pte_addr)
 {
-    uint64_t index, pde, rpn , mask;
-
-    if (nls < 5) { /* Directory maps less than 2**5 entries */
-        *fault_cause |= DSISR_R_BADCONFIG;
-        return 1;
-    }
+    uint64_t index, pde, rpn, mask;
+    int level = 0;
 
     index = eaddr >> (*psize - nls);    /* Shift */
     index &= ((1UL << nls) - 1);       /* Mask */
@@ -283,6 +301,11 @@ static int ppc_radix64_walk_tree(AddressSpace *as, vaddr eaddr,
     do {
         int ret;
 
+        if (!ppc_radix64_is_valid_level(level++, *psize, nls)) {
+            *fault_cause |= DSISR_R_BADCONFIG;
+            return 1;
+        }
+
         ret = ppc_radix64_next_level(as, eaddr, pte_addr, &nls, psize, &pde,
                                      fault_cause);
         if (ret) {
@@ -456,6 +479,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
         }
     } else {
         uint64_t rpn, mask;
+        int level = 0;
 
         index = (eaddr & R_EADDR_MASK) >> (*g_page_size - nls); /* Shift */
         index &= ((1UL << nls) - 1);                            /* Mask */
@@ -475,6 +499,11 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
                 return ret;
             }
 
+            if (!ppc_radix64_is_valid_level(level++, *g_page_size, nls)) {
+                fault_cause |= DSISR_R_BADCONFIG;
+                return 1;
+            }
+
             ret = ppc_radix64_next_level(cs->as, eaddr & R_EADDR_MASK, &h_raddr,
                                          &nls, g_page_size, &pte, &fault_cause);
             if (ret) {
-- 
2.25.1

[PATCH v2 3/3] target/ppc: Check page dir/table base alignment

[Leandro Lupori]

Check if each page dir/table base address is properly aligned and
log a guest error if not, as real hardware behave incorrectly in
this case.

Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
---
 target/ppc/mmu-radix64.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
index 339cf5b4d8..1e7d932893 100644
--- a/target/ppc/mmu-radix64.c
+++ b/target/ppc/mmu-radix64.c
@@ -280,6 +280,14 @@ static int ppc_radix64_next_level(AddressSpace *as, vaddr eaddr,
     *psize -= *nls;
     if (!(pde & R_PTE_LEAF)) { /* Prepare for next iteration */
         *nls = pde & R_PDE_NLS;
+
+        if ((pde & R_PDE_NLB) & MAKE_64BIT_MASK(0, *nls + 3)) {
+            qemu_log_mask(LOG_GUEST_ERROR,
+                "%s: misaligned page dir/table base: 0x%"VADDR_PRIx
+                " page dir size: 0x"TARGET_FMT_lx"\n",
+                __func__, (pde & R_PDE_NLB), BIT(*nls + 3));
+        }
+
         index = eaddr >> (*psize - *nls);       /* Shift */
         index &= ((1UL << *nls) - 1);           /* Mask */
         *pte_addr = (pde & R_PDE_NLB) + (index * sizeof(pde));
@@ -295,6 +303,13 @@ static int ppc_radix64_walk_tree(AddressSpace *as, vaddr eaddr,
     uint64_t index, pde, rpn, mask;
     int level = 0;
 
+    if (base_addr & MAKE_64BIT_MASK(0, nls + 3)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+            "%s: misaligned page dir base: 0x%"VADDR_PRIx
+            " page dir size: 0x"TARGET_FMT_lx"\n",
+            __func__, base_addr, BIT(nls + 3));
+    }
+
     index = eaddr >> (*psize - nls);    /* Shift */
     index &= ((1UL << nls) - 1);       /* Mask */
     *pte_addr = base_addr + (index * sizeof(pde));
-- 
2.25.1

Re: [PATCH v2 3/3] target/ppc: Check page dir/table base alignment

[Richard Henderson]

On 6/24/22 10:16, Leandro Lupori wrote:
> Check if each page dir/table base address is properly aligned and
> log a guest error if not, as real hardware behave incorrectly in
> this case.
> 
> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
> ---
>   target/ppc/mmu-radix64.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index 339cf5b4d8..1e7d932893 100644
> --- a/target/ppc/mmu-radix64.c
> +++ b/target/ppc/mmu-radix64.c
> @@ -280,6 +280,14 @@ static int ppc_radix64_next_level(AddressSpace *as, vaddr eaddr,
>       *psize -= *nls;
>       if (!(pde & R_PTE_LEAF)) { /* Prepare for next iteration */
>           *nls = pde & R_PDE_NLS;
> +
> +        if ((pde & R_PDE_NLB) & MAKE_64BIT_MASK(0, *nls + 3)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "%s: misaligned page dir/table base: 0x%"VADDR_PRIx
> +                " page dir size: 0x"TARGET_FMT_lx"\n",
> +                __func__, (pde & R_PDE_NLB), BIT(*nls + 3));
> +        }
> +
>           index = eaddr >> (*psize - *nls);       /* Shift */
>           index &= ((1UL << *nls) - 1);           /* Mask */
>           *pte_addr = (pde & R_PDE_NLB) + (index * sizeof(pde));

In your response to my question on v1, you said that it appears that the cpu ignores bits 
*nls+3. This isn't ignoring them -- it's including [nls+2 : nls] into pte_addr.

It would be better to compute this as

     index = ...
     index &= ...
     *pte_addr = ...
     if (*pte_addr & 7) {
         qemu_log(...);
     }


r~

Re: [PATCH v2 2/3] target/ppc: Improve Radix xlate level validation

[Fabiano Rosas]

Leandro Lupori <leandro.lupori@eldorado.org.br> writes:

> Check if the number and size of Radix levels are valid on
> POWER9/POWER10 CPUs, according to the supported Radix Tree
> Configurations described in their User Manuals.
>
> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
> ---
>  target/ppc/mmu-radix64.c | 51 +++++++++++++++++++++++++++++++---------
>  1 file changed, 40 insertions(+), 11 deletions(-)
>
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index 9a8a2e2875..339cf5b4d8 100644
> --- a/target/ppc/mmu-radix64.c
> +++ b/target/ppc/mmu-radix64.c
> @@ -236,17 +236,39 @@ static void ppc_radix64_set_rc(PowerPCCPU *cpu, MMUAccessType access_type,
>      }
>  }
>  
> +static bool ppc_radix64_is_valid_level(int level, int psize, uint64_t nls)
> +{
> +    /*
> +     * Check if this is a valid level, according to POWER9 and POWER10
> +     * Processor User's Manuals, sections 4.10.4.1 and 5.10.6.1, respectively:
> +     * Supported Radix Tree Configurations and Resulting Page Sizes.
> +     *
> +     * NOTE: these checks are valid for POWER9 and POWER10 CPUs only. If
> +     *       new CPUs that support other Radix configurations are added
> +     *       (e.g., Microwatt), then a new method should be added to
> +     *       PowerPCCPUClass, with this function being the POWER9/POWER10
> +     *       implementation.
> +     */

Sorry, this got too specific now. I could not respond in time before you
sent the v2. Let's cut the mentions to the code:

  Note: these checks are specific to POWER9 and POWER10 CPUs. Any future
  CPUs that supports a different Radix MMU configuration will need their
  own implementation.

> +    switch (level) {
> +    case 0:     /* Root Page Dir */
> +        return psize == 52 && nls == 13;
> +    case 1:
> +    case 2:
> +        return nls == 9;
> +    case 3:
> +        return nls == 9 || nls == 5;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "invalid radix level: %d\n", level);
> +        return false;
> +    }
> +}
> +
>  static int ppc_radix64_next_level(AddressSpace *as, vaddr eaddr,
>                                    uint64_t *pte_addr, uint64_t *nls,
>                                    int *psize, uint64_t *pte, int *fault_cause)
>  {
>      uint64_t index, pde;
>  
> -    if (*nls < 5) { /* Directory maps less than 2**5 entries */
> -        *fault_cause |= DSISR_R_BADCONFIG;
> -        return 1;
> -    }
> -
>      /* Read page <directory/table> entry from guest address space */
>      pde = ldq_phys(as, *pte_addr);
>      if (!(pde & R_PTE_VALID)) {         /* Invalid Entry */
> @@ -270,12 +292,8 @@ static int ppc_radix64_walk_tree(AddressSpace *as, vaddr eaddr,
>                                   hwaddr *raddr, int *psize, uint64_t *pte,
>                                   int *fault_cause, hwaddr *pte_addr)
>  {
> -    uint64_t index, pde, rpn , mask;
> -
> -    if (nls < 5) { /* Directory maps less than 2**5 entries */
> -        *fault_cause |= DSISR_R_BADCONFIG;
> -        return 1;
> -    }
> +    uint64_t index, pde, rpn, mask;
> +    int level = 0;
>  
>      index = eaddr >> (*psize - nls);    /* Shift */
>      index &= ((1UL << nls) - 1);       /* Mask */
> @@ -283,6 +301,11 @@ static int ppc_radix64_walk_tree(AddressSpace *as, vaddr eaddr,
>      do {
>          int ret;
>  
> +        if (!ppc_radix64_is_valid_level(level++, *psize, nls)) {
> +            *fault_cause |= DSISR_R_BADCONFIG;
> +            return 1;
> +        }
> +
>          ret = ppc_radix64_next_level(as, eaddr, pte_addr, &nls, psize, &pde,
>                                       fault_cause);
>          if (ret) {
> @@ -456,6 +479,7 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
>          }
>      } else {
>          uint64_t rpn, mask;
> +        int level = 0;
>  
>          index = (eaddr & R_EADDR_MASK) >> (*g_page_size - nls); /* Shift */
>          index &= ((1UL << nls) - 1);                            /* Mask */
> @@ -475,6 +499,11 @@ static int ppc_radix64_process_scoped_xlate(PowerPCCPU *cpu,
>                  return ret;
>              }
>  
> +            if (!ppc_radix64_is_valid_level(level++, *g_page_size, nls)) {
> +                fault_cause |= DSISR_R_BADCONFIG;
> +                return 1;
> +            }
> +
>              ret = ppc_radix64_next_level(cs->as, eaddr & R_EADDR_MASK, &h_raddr,
>                                           &nls, g_page_size, &pte, &fault_cause);
>              if (ret) {

Re: [PATCH v2 1/3] ppc: Check partition and process table alignment

[Fabiano Rosas]

Leandro Lupori <leandro.lupori@eldorado.org.br> writes:

> Check if partition and process tables are properly aligned, in
> their size, according to PowerISA 3.1B, Book III 6.7.6 programming
> note. Hardware and KVM also raise an exception in these cases.
>
> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>

Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>

Re: [PATCH v2 3/3] target/ppc: Check page dir/table base alignment

[Fabiano Rosas]

Leandro Lupori <leandro.lupori@eldorado.org.br> writes:

> Check if each page dir/table base address is properly aligned and
> log a guest error if not, as real hardware behave incorrectly in
> this case.

I think the commit message could be clearer, something like:

According to PowerISA 3.1B, Book III 6.7.6 programming note, the page
directory base addresses are expected to be aligned to their size. Real
hardware seems to rely on that and will access the wrong address if they
are misaligned. This results in a translation failure even if the page
tables seem to be properly populated.

Let's make sure we capture this assumption in the code to help anyone
implementing page tables.

>
> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
> ---
>  target/ppc/mmu-radix64.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
> index 339cf5b4d8..1e7d932893 100644
> --- a/target/ppc/mmu-radix64.c
> +++ b/target/ppc/mmu-radix64.c
> @@ -280,6 +280,14 @@ static int ppc_radix64_next_level(AddressSpace *as, vaddr eaddr,
>      *psize -= *nls;
>      if (!(pde & R_PTE_LEAF)) { /* Prepare for next iteration */
>          *nls = pde & R_PDE_NLS;
> +
> +        if ((pde & R_PDE_NLB) & MAKE_64BIT_MASK(0, *nls + 3)) {
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                "%s: misaligned page dir/table base: 0x%"VADDR_PRIx
> +                " page dir size: 0x"TARGET_FMT_lx"\n",
> +                __func__, (pde & R_PDE_NLB), BIT(*nls + 3));
> +        }
> +
>          index = eaddr >> (*psize - *nls);       /* Shift */
>          index &= ((1UL << *nls) - 1);           /* Mask */
>          *pte_addr = (pde & R_PDE_NLB) + (index * sizeof(pde));
> @@ -295,6 +303,13 @@ static int ppc_radix64_walk_tree(AddressSpace *as, vaddr eaddr,
>      uint64_t index, pde, rpn, mask;
>      int level = 0;
>  
> +    if (base_addr & MAKE_64BIT_MASK(0, nls + 3)) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +            "%s: misaligned page dir base: 0x%"VADDR_PRIx
> +            " page dir size: 0x"TARGET_FMT_lx"\n",
> +            __func__, base_addr, BIT(nls + 3));
> +    }
> +
>      index = eaddr >> (*psize - nls);    /* Shift */
>      index &= ((1UL << nls) - 1);       /* Mask */
>      *pte_addr = base_addr + (index * sizeof(pde));

Re: [PATCH v2 3/3] target/ppc: Check page dir/table base alignment

[Leandro Lupori]

On 6/24/22 15:04, Richard Henderson wrote:
> 
> On 6/24/22 10:16, Leandro Lupori wrote:
>> Check if each page dir/table base address is properly aligned and
>> log a guest error if not, as real hardware behave incorrectly in
>> this case.
>>
>> Signed-off-by: Leandro Lupori <leandro.lupori@eldorado.org.br>
>> ---
>>   target/ppc/mmu-radix64.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/target/ppc/mmu-radix64.c b/target/ppc/mmu-radix64.c
>> index 339cf5b4d8..1e7d932893 100644
>> --- a/target/ppc/mmu-radix64.c
>> +++ b/target/ppc/mmu-radix64.c
>> @@ -280,6 +280,14 @@ static int ppc_radix64_next_level(AddressSpace 
>> *as, vaddr eaddr,
>>       *psize -= *nls;
>>       if (!(pde & R_PTE_LEAF)) { /* Prepare for next iteration */
>>           *nls = pde & R_PDE_NLS;
>> +
>> +        if ((pde & R_PDE_NLB) & MAKE_64BIT_MASK(0, *nls + 3)) {
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>> +                "%s: misaligned page dir/table base: 0x%"VADDR_PRIx
>> +                " page dir size: 0x"TARGET_FMT_lx"\n",
>> +                __func__, (pde & R_PDE_NLB), BIT(*nls + 3));
>> +        }
>> +
>>           index = eaddr >> (*psize - *nls);       /* Shift */
>>           index &= ((1UL << *nls) - 1);           /* Mask */
>>           *pte_addr = (pde & R_PDE_NLB) + (index * sizeof(pde));
> 
> In your response to my question on v1, you said that it appears that the 
> cpu ignores bits
> *nls+3. This isn't ignoring them -- it's including [nls+2 : nls] into 
> pte_addr.
> 
> It would be better to compute this as
> 
>      index = ...
>      index &= ...
>      *pte_addr = ...
>      if (*pte_addr & 7) {
>          qemu_log(...);
>      }
> 

Right, I wanted to warn about the invalid alignment but I ended up 
forgetting to make QEMU match the CPU behavior.

The CPU seems to ignore bits [nls+2 : 0] of NLB.

The multiplication of index by sizeof(pde) discards the 3 lower bits and 
it's not possible for NLB to have its 8 lower bits set, as these are 
used for NLS plus some reserved bits in the PDE.
Then we need to make sure that bits [nls+2 : 8] of NLB are also 0.

So maybe something like this would do it:

     index = eaddr >> (*psize - *nls);       /* Shift */
     index &= ((1UL << *nls) - 1);           /* Mask */
     *pte_addr = pde & R_PDE_NLB;
     mask = MAKE_64BIT_MASK(0, *nls + 3);
     if (*pte_addr & mask) {
         qemu_log(...);
         *pte_addr &= ~mask;
     }
     *pte_addr += index * sizeof(pde);

Thanks,
Leandro

> 
> r~