On Wed, Sep 03, 2025 at 02:46:28PM -0300, Jason Gunthorpe wrote:
> +/**
> + * pt_entry_oa_lg2sz() - Return the size of a OA entry
an OA
> + * @pts: Entry to query
> + *
> + * If the entry is not contiguous this returns pt_table_item_lg2sz(), otherwise
> + * it returns the total VA/OA size of the entire contiguous entry.
> + */
> +static inline unsigned int pt_entry_oa_lg2sz(const struct pt_state *pts)
> +{
> + return pt_entry_num_contig_lg2(pts) + pt_table_item_lg2sz(pts);
> +}
------
> + * level
> + * The number of table hops from the lowest leaf. Level 0
> + * is always a table of only leaves of the least significant VA bits. The
Hmm, I am a bit confused here. I thought "leaf" was meant to be a
"leaf" table? But here "a table of only leaves" makes it feel like
a "leaf" table entry?
Also, isn't "the least significant VA bits" the page offset?
> + * table
> + * A linear array of entries representing the translation items for that
> + * level.
> + * index
> + * The position in a table of an element: item = table[index]
> + * item
> + * A single position in a table
> + * entry
> + * A single logical element in a table. If contiguous pages are not
> + * supported then item and entry are the same thing, otherwise entry refers
> + * to the all the items that comprise a single contiguous translation.
So, an "entry" is a group of "items" if contiguous pages (huge
page?) are supported. Then, the "entry" sounds like a physical
(v.s. "logical") table entry, e.g. a PTE that we usually say?
> +#if !IS_ENABLED(CONFIG_GENERIC_ATOMIC64)
> +static inline bool pt_table_install64(struct pt_state *pts, u64 table_entry)
> +{
> + u64 *entryp = pt_cur_table(pts, u64) + pts->index;
> + u64 old_entry = pts->entry;
> + bool ret;
> +
> + /*
> + * Ensure the zero'd table content itself is visible before its PTE can
> + * be. release is a NOP on !SMP, but the HW is still doing an acquire.
> + */
> + if (!IS_ENABLED(CONFIG_SMP))
> + dma_wmb();
Mind elaborating why SMP doesn't need this?
> +/*
> + * PT_WARN_ON is used for invariants that the kunit should be checking can't
> + * happen.
> + */
> +#if IS_ENABLED(CONFIG_DEBUG_GENERIC_PT)
> +#define PT_WARN_ON WARN_ON
> +#else
> +static inline bool PT_WARN_ON(bool condition)
> +{
> + return false;
Should it "return condition"?
Otherwise, these validations wouldn't be effective?
drivers/iommu/generic_pt/pt_iter.h:388: if (PT_WARN_ON(!pts->table_lower))
drivers/iommu/generic_pt/pt_iter.h-389- return -EINVAL;
--
drivers/iommu/generic_pt/pt_iter.h-429-
drivers/iommu/generic_pt/pt_iter.h:430: if (PT_WARN_ON(!pt_can_have_table(pts)) ||
drivers/iommu/generic_pt/pt_iter.h:431: PT_WARN_ON(!pts->table_lower))
drivers/iommu/generic_pt/pt_iter.h-432- return -EINVAL;
> +/**
> + * pt_load_entry() - Read from the location pts points at into the pts
> + * @pts: Table index to load
> + *
> + * Set the type of entry that was loaded. pts->entry and pts->table_lower
> + * will be filled in with the entry's content.
> + */
> +static inline void pt_load_entry(struct pt_state *pts)
> +{
> + pts->type = pt_load_entry_raw(pts);
> + if (pts->type == PT_ENTRY_TABLE)
> + pts->table_lower = pt_table_ptr(pts);
> +}
I see a couple of callers check pts->type. Maybe it could return
pts->type, matching with pt_load_entry_raw()?
> diff --git a/drivers/iommu/generic_pt/pt_fmt_defaults.h b/drivers/iommu/generic_pt/pt_fmt_defaults.h
> new file mode 100644
> index 00000000000000..19e8f820c1dccf
> --- /dev/null
> +++ b/drivers/iommu/generic_pt/pt_fmt_defaults.h
> @@ -0,0 +1,193 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2024-2025, NVIDIA CORPORATION & AFFILIATES
> + *
> + * Default definitions for formats that don't define these functions.
> + */
> +#ifndef __GENERIC_PT_PT_FMT_DEFAULTS_H
> +#define __GENERIC_PT_PT_FMT_DEFAULTS_H
> +
> +#include "pt_defs.h"
> +#include <linux/log2.h>
<> precedes ""
> +#ifndef pt_pgsz_lg2_to_level
> +static inline unsigned int pt_pgsz_lg2_to_level(struct pt_common *common,
> + unsigned int pgsize_lg2)
> +{
> + return (pgsize_lg2 - PT_GRANULE_LG2SZ) /
> + (PT_TABLEMEM_LG2SZ - ilog2(PT_ITEM_WORD_SIZE));
> + return 0;
"return 0" should likely be dropped.
> +/*
> + * Format supplies either:
> + * pt_entry_oa - OA is at the start of a contiguous entry
> + * or
> + * pt_item_oa - OA is correct for every item in a contiguous entry
What does the "correct" mean here?
> +/**
> + * pt_range_to_end_index() - Ending index iteration
> + * @pts: Iteration State
> + *
> + * Return: the last index for the iteration in pts.
> + */
> +static inline unsigned int pt_range_to_end_index(const struct pt_state *pts)
> +{
> + unsigned int isz_lg2 = pt_table_item_lg2sz(pts);
> + struct pt_range *range = pts->range;
> + unsigned int num_entries_lg2;
> +
> + if (range->va == range->last_va)
> + return pts->index + 1;
> +
> + if (pts->range->top_level == pts->level)
> + return log2_div(fvalog2_mod(pts->range->last_va,
> + pts->range->max_vasz_lg2),
> + isz_lg2) +
> + 1;
How about:
return 1 + log2_div(...);
?
> +static __always_inline struct pt_range _pt_top_range(struct pt_common *common,
> + uintptr_t top_of_table)
> +{
> + struct pt_range range = {
> + .common = common,
> + .top_table =
> + (struct pt_table_p *)(top_of_table &
> + ~(uintptr_t)PT_TOP_LEVEL_MASK),
> + .top_level = top_of_table % (1 << PT_TOP_LEVEL_BITS),
Since top_level is unsigned, would it be faster to do bitwise:
.top_level = top_of_table & PT_TOP_LEVEL_MASK,
?
> +/*
/**
> + * pt_walk_descend_all() - Recursively invoke the walker for a table item
> + * @parent_pts: Iteration State
> + * @fn: Walker function to call
> + * @arg: Value to pass to the function
> + *
> + * With pts pointing at a table item this will descend and over the entire lower
> + * table. This creates a new walk and does not alter pts or pts->range.
> + */
> +static __always_inline int
> +pt_walk_descend_all(const struct pt_state *parent_pts, pt_level_fn_t fn,
> + void *arg)
-------
> +/**
> + * pt_compute_best_pgsize() - Determine the best page size for leaf entries
> + * @pgsz_bitmap: Permitted page sizes
> + * @va: Starting virtual address for the leaf entry
> + * @last_va: Last virtual address for the leaf entry, sets the max page size
> + * @oa: Starting output address for the leaf entry
> + *
> + * Compute the largest page size for va, last_va, and oa together and return it
> + * in lg2. The largest page size depends on the format's supported page sizes at
> + * this level, and the relative alignment of the VA and OA addresses. 0 means
> + * the OA cannot be stored with the provided pgsz_bitmap.
> + */
> +static inline unsigned int pt_compute_best_pgsize(pt_vaddr_t pgsz_bitmap,
> + pt_vaddr_t va,
> + pt_vaddr_t last_va,
> + pt_oaddr_t oa)
> +{
> + unsigned int best_pgsz_lg2;
> + unsigned int pgsz_lg2;
> + pt_vaddr_t len = last_va - va + 1;
> + pt_vaddr_t mask;
> +
> + if (PT_WARN_ON(va >= last_va))
> + return 0;
> +
> + /*
> + * Given a VA/OA pair the best page size is the largest page side
> + * where:
> + *
> + * 1) VA and OA start at the page. Bitwise this is the count of least
> + * significant 0 bits.
> + * This also implies that last_va/oa has the same prefix as va/oa.
> + */
> + mask = va | oa;
> +
> + /*
> + * 2) The page size is not larger than the last_va (length). Since page
> + * sizes are always power of two this can't be larger than the
> + * largest power of two factor of the length.
> + */
> + mask |= log2_to_int(log2_fls(len) - 1);
> +
> + best_pgsz_lg2 = log2_ffs(mask);
> +
> + /* Choose the higest bit <= best_pgsz_lg2 */
highest
> +/* Compute a */
> +#define log2_to_int_t(type, a_lg2) ((type)(((type)1) << (a_lg2)))
> +static_assert(log2_to_int_t(unsigned int, 0) == 1);
> +
> +/* Compute a - 1 (aka all low bits set) */
> +#define log2_to_max_int_t(type, a_lg2) ((type)(log2_to_int_t(type, a_lg2) - 1))
> +
> +/* Compute a / b */
> +#define log2_div_t(type, a, b_lg2) ((type)(((type)a) >> (b_lg2)))
> +static_assert(log2_div_t(unsigned int, 4, 2) == 1);
I skimmed through these macros and its callers. They are mostly
dealing with VA, OA, and mask, which feels like straightforward
bit-masking/shifting operations.
E.g.
log2_to_int_t = 64bit ? BIT_ULL(lg2) : BIT(lg2);
log2_to_max_int_t = 64bit ? GENMASK_ULL(lg2 - 1, 0) : GENMASK(lg2 - 1, 0);
What's the benefit from making them as arithmetic ones?
> diff --git a/include/linux/generic_pt/common.h b/include/linux/generic_pt/common.h
> new file mode 100644
> index 00000000000000..a29bdd7b244de6
> --- /dev/null
> +++ b/include/linux/generic_pt/common.h
> @@ -0,0 +1,134 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2024-2025, NVIDIA CORPORATION & AFFILIATES
> + */
> +#ifndef __GENERIC_PT_COMMON_H
> +#define __GENERIC_PT_COMMON_H
> +
> +#include <linux/types.h>
> +#include <linux/build_bug.h>
> +#include <linux/bits.h>
In alphabetical order.
Thanks
Nicolin
