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.
> @@ -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)?
Jan
