On 2/19/25 12:28 PM, Jan Beulich wrote:
On 12.02.2025 17:50, Oleksii Kurochko wrote:
--- a/xen/arch/riscv/pt.c
+++ b/xen/arch/riscv/pt.c
@@ -249,12 +249,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;
/*
* The intermediate page table shouldn't be allocated when MFN isn't
* valid and we are not populating page table.
@@ -265,41 +263,48 @@ static int pt_update_entry(mfn_t root, vaddr_t virt,
* combinations of (mfn, flags).
*/
bool alloc_tbl = !mfn_eq(mfn, INVALID_MFN) || (flags & PTE_POPULATE);
- pte_t pte, *entry;
-
- /* convenience aliases */
- DECLARE_OFFSETS(offsets, virt);
+ pte_t pte, *entry = NULL;
With there also being "table" below, "entry" isn't quite as bad as in the
other patch. Yet I'd still like to ask that you consider renaming.
- table = map_table(root);
- for ( ; level > target; level-- )
+ if ( *target == CONFIG_PAGING_LEVELS )
+ entry = _pt_walk(virt, target);
Imo it's quite important for the comment ahead of the function to be updated
to mention this special case.
+ else
{
- rc = pt_next_level(alloc_tbl, &table, offsets[level]);
- if ( rc == XEN_TABLE_MAP_NOMEM )
+ pte_t *table;
+ unsigned int level = HYP_PT_ROOT_LEVEL;
+ /* convenience aliases */
Nit: Style.
From the 'Comments' section of CODING_STYLE, I see that the comment should start
with capital letter. Do you mean that?
@@ -331,7 +336,8 @@ static int pt_update_entry(mfn_t root, vaddr_t virt,
rc = 0;
out:
- unmap_table(table);
+ if ( entry )
+ unmap_table(entry);
Would it perhaps be worth for unmap_table() to gracefully handle being passed
NULL, to avoid such conditionals (there may be more in the future)?
Agree, it would be more safe to move this check inside unmap_table(). I will
update
that.
Thanks.
~ Oleksii
