Re: [PATCH for 4.20? v3 3/3] xen/riscv: update mfn calculation in pt_mapping_level()

[Oleksii Kurochko]

On 2/10/25 5:53 PM, Jan Beulich wrote:

On 07.02.2025 14:13, Oleksii Kurochko wrote:

--- a/xen/arch/riscv/pt.c
+++ b/xen/arch/riscv/pt.c
@@ -238,11 +238,10 @@ pte_t pt_walk(vaddr_t va, unsigned int *pte_level)

/* Update an entry at the level @target. */

  static int pt_update_entry(mfn_t root, vaddr_t virt,
-                           mfn_t mfn, unsigned int target,
+                           mfn_t mfn, unsigned int *target,
                             unsigned int flags)
  {
      int rc;
-    unsigned int level = HYP_PT_ROOT_LEVEL;
      pte_t *table;

Considering the lack of an initializer here, ...

@@ -256,39 +255,45 @@ static int pt_update_entry(mfn_t root, vaddr_t virt,
      bool alloc_tbl = !mfn_eq(mfn, INVALID_MFN) || (flags & PTE_POPULATE);
      pte_t pte, *entry;

- /* convenience aliases */

-    DECLARE_OFFSETS(offsets, virt);
-
-    table = map_table(root);
-    for ( ; level > target; level-- )
+    if ( *target == CONFIG_PAGING_LEVELS )
+        entry = _pt_walk(virt, target);
+    else
      {
-        rc = pt_next_level(alloc_tbl, &table, offsets[level]);
-        if ( rc == XEN_TABLE_MAP_NOMEM )
+        unsigned int level = HYP_PT_ROOT_LEVEL;
+        /* convenience aliases */
+        DECLARE_OFFSETS(offsets, virt);
+
+        table = map_table(root);
+        for ( ; level > *target; level-- )
          {
-            rc = -ENOMEM;
-            goto out;
+            rc = pt_next_level(alloc_tbl, &table, offsets[level]);
+            if ( rc == XEN_TABLE_MAP_NOMEM )
+            {
+                rc = -ENOMEM;
+                goto out;
+            }
+
+            if ( rc == XEN_TABLE_MAP_NONE )
+            {
+                rc = 0;
+                goto out;
+            }
+
+            if ( rc != XEN_TABLE_NORMAL )
+                break;
          }

- if ( rc == XEN_TABLE_MAP_NONE )

+        if ( level != *target )
          {
-            rc = 0;
+            dprintk(XENLOG_ERR,
+                    "%s: Shattering superpage is not supported\n", __func__);
+            rc = -EOPNOTSUPP;
              goto out;
          }

- if ( rc != XEN_TABLE_NORMAL )

-            break;
-    }
-
-    if ( level != target )
-    {
-        dprintk(XENLOG_ERR,
-                "%s: Shattering superpage is not supported\n", __func__);
-        rc = -EOPNOTSUPP;
-        goto out;
+        entry = table + offsets[level];
      }

- entry = table + offsets[level];

-
      rc = -EINVAL;
      if ( !pt_check_entry(*entry, mfn, flags) )
          goto out;

... I'm surprised the compiler doesn't complain about use of a possibly
uninitialized variable at

  out:
     unmap_table(table);

For the new path you're adding the variable is uninitialized afaict.
Which implies that you're again failing to unmap what _pt_walk() has
handed you.

Thanks, unmapping of table and entry (in the case of the new patch) should be
really updated. I'll take care of it in the next patch version.

~ Oleksii