On 2/25/25 8:59 AM, Jan Beulich wrote:
On 20.02.2025 18:44, Oleksii Kurochko wrote:
When pt_update() is called with arguments (..., INVALID_MFN, ..., 0 or 1),
it indicates that a mapping is being destroyed/modifyed.
In the case when modifying or destroying a mapping, it is necessary to
search until a leaf node is found, instead of searching for a page table
entry based on the precalculated `level` and `order`(look at pt_update()).
This is because when `mfn` == INVALID_MFN, the `mask` (in pt_mapping_level())
will take into account only `vfn`, which could accidentally return an
incorrect level, leading to the discovery of an incorrect page table entry.
For example, if `vfn` is page table level 1 aligned, but it was mapped as
page table level 0, then pt_mapping_level() will return `level` = 1, since
only `vfn` (which is page table level 1 aligned) is taken into account when
`mfn` == INVALID_MFN (look at pt_mapping_level()).
Fixes: c2f1ded524 ("xen/riscv: page table handling")
Signed-off-by: Oleksii Kurochko<[email protected]>
---
Changes in v5:
- Rename *entry to *ptep in pt_update_entry().
- Fix code style issue in the comment.
- Move NULL check of pointer to `table` inside unmap_table and then drop
it in pt_update_entry().
Imo this last aspect wants mentioning in the description.
Agree, considering that it is a change in the code of previously introduced
function,
it makes to mention that.
@@ -422,17 +439,40 @@ static int pt_update(vaddr_t virt, mfn_t mfn,
while ( left )
{
- unsigned int order, level;
-
- level = pt_mapping_level(vfn, mfn, left, flags);
- order = XEN_PT_LEVEL_ORDER(level);
+ unsigned int order, level = CONFIG_PAGING_LEVELS;
- ASSERT(left >= BIT(order, UL));
+ /*
+ * In the case when modifying or destroying a mapping, it is necessary
+ * to search until a leaf node is found, instead of searching for
+ * a page table entry based on the precalculated `level` and `order`
+ * (look at pt_update()).
+ * This is because when `mfn` == INVALID_MFN, the `mask`(in
+ * pt_mapping_level()) will take into account only `vfn`, which could
+ * accidentally return an incorrect level, leading to the discovery of
+ * an incorrect page table entry.
+ *
+ * For example, if `vfn` is page table level 1 aligned, but it was
+ * mapped as page table level 0, then pt_mapping_level() will return
+ * `level` = 1, since only `vfn` (which is page table level 1 aligned)
+ * is taken into account when `mfn` == INVALID_MFN
+ * (look at pt_mapping_level()).
+ *
+ * To force searching until a leaf node is found is necessary to have
+ * `level` == CONFIG_PAGING_LEVELS which is a default value for
+ * `level`.
There looks to be an "it" missing before the 2nd "is". I'm also unconvinced the
part starting with "which" is really necessary.
Lets then just drop this part. I added only to explicitly show that it is the
value
which is used to define `level`.
Preferably with these small adjustments (I'd be happy to do so while committing)
Reviewed-by: Jan Beulich<[email protected]>
Thanks!
~ Oleksii
