| Message ID | a4a970490471035bf49a97257b400da23091fb9a.1737391102.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fixes for vmap_to_mfn() and pt_mapping_level | expand |
On 20.01.2025 17:54, Oleksii Kurochko wrote: > --- a/xen/arch/riscv/pt.c > +++ b/xen/arch/riscv/pt.c > @@ -346,9 +346,33 @@ static int pt_mapping_level(unsigned long vfn, mfn_t mfn, unsigned long nr, > return level; > > /* > - * Don't take into account the MFN when removing mapping (i.e > - * MFN_INVALID) to calculate the correct target order. > + * `mfn` should be set properly in cases when modifying/destroying a > + * mapping to ensure the correct page table `level` is received. In the > + * case of `mfn` == INVALID_MFN, the `mask` will take into account only > + * `vfn` and could accidentally return an incorrect level. For example, > + * if `vfn` is page table level 1 aligned, but it was mapped as page table > + * level 0, then without the check below it will return `level` = 1 > + * because only `vfn`, which is page table level 1 aligned, is taken into > + * account when `mfn` == INVALID_MFN. > * > + * POPULATE shouldn't be considered as `va` hasn't been mapped yet. > + */ > + if ( mfn_eq(mfn, INVALID_MFN) && !(flags & PTE_POPULATE) ) > + { > + vaddr_t va = vfn << PAGE_SHIFT; > + paddr_t pa; > + unsigned long xen_virt_end = (XEN_VIRT_START + XEN_VIRT_SIZE - 1); > + > + if ( ((va >= DIRECTMAP_VIRT_START) && (va <= DIRECTMAP_VIRT_END)) || > + ((va >= XEN_VIRT_START) && (va <= xen_virt_end)) ) > + pa = virt_to_maddr(va); > + else > + pa = pt_walk(va); > + > + mfn = _mfn(paddr_to_pfn(pa)); > + } Doing a walk here and then another in pt_update() feels wasteful. I wonder whether the overall approach to page table updating doesn't want changing. It ought to be possible to tell an "update" operation to walk down to wherever the leaf entry is, and update there. Jan
On 1/27/25 11:24 AM, Jan Beulich wrote: > On 20.01.2025 17:54, Oleksii Kurochko wrote: >> --- a/xen/arch/riscv/pt.c >> +++ b/xen/arch/riscv/pt.c >> @@ -346,9 +346,33 @@ static int pt_mapping_level(unsigned long vfn, mfn_t mfn, unsigned long nr, >> return level; >> >> /* >> - * Don't take into account the MFN when removing mapping (i.e >> - * MFN_INVALID) to calculate the correct target order. >> + * `mfn` should be set properly in cases when modifying/destroying a >> + * mapping to ensure the correct page table `level` is received. In the >> + * case of `mfn` == INVALID_MFN, the `mask` will take into account only >> + * `vfn` and could accidentally return an incorrect level. For example, >> + * if `vfn` is page table level 1 aligned, but it was mapped as page table >> + * level 0, then without the check below it will return `level` = 1 >> + * because only `vfn`, which is page table level 1 aligned, is taken into >> + * account when `mfn` == INVALID_MFN. >> * >> + * POPULATE shouldn't be considered as `va` hasn't been mapped yet. >> + */ >> + if ( mfn_eq(mfn, INVALID_MFN) && !(flags & PTE_POPULATE) ) >> + { >> + vaddr_t va = vfn << PAGE_SHIFT; >> + paddr_t pa; >> + unsigned long xen_virt_end = (XEN_VIRT_START + XEN_VIRT_SIZE - 1); >> + >> + if ( ((va >= DIRECTMAP_VIRT_START) && (va <= DIRECTMAP_VIRT_END)) || >> + ((va >= XEN_VIRT_START) && (va <= xen_virt_end)) ) >> + pa = virt_to_maddr(va); >> + else >> + pa = pt_walk(va); >> + >> + mfn = _mfn(paddr_to_pfn(pa)); >> + } > Doing a walk here and then another in pt_update() feels wasteful. I agree that it is wasteful but, at that moment, it seemed to me the faster way to resolve that. > I > wonder whether the overall approach to page table updating doesn't want > changing. It ought to be possible to tell an "update" operation to walk > down to wherever the leaf entry is, and update there. I think I got your idea. I will try to implement that. Thanks! ~ Oleksii
diff --git a/xen/arch/riscv/pt.c b/xen/arch/riscv/pt.c index 865d60d1af..c8bc6f7e37 100644 --- a/xen/arch/riscv/pt.c +++ b/xen/arch/riscv/pt.c @@ -346,9 +346,33 @@ static int pt_mapping_level(unsigned long vfn, mfn_t mfn, unsigned long nr, return level; /* - * Don't take into account the MFN when removing mapping (i.e - * MFN_INVALID) to calculate the correct target order. + * `mfn` should be set properly in cases when modifying/destroying a + * mapping to ensure the correct page table `level` is received. In the + * case of `mfn` == INVALID_MFN, the `mask` will take into account only + * `vfn` and could accidentally return an incorrect level. For example, + * if `vfn` is page table level 1 aligned, but it was mapped as page table + * level 0, then without the check below it will return `level` = 1 + * because only `vfn`, which is page table level 1 aligned, is taken into + * account when `mfn` == INVALID_MFN. * + * POPULATE shouldn't be considered as `va` hasn't been mapped yet. + */ + if ( mfn_eq(mfn, INVALID_MFN) && !(flags & PTE_POPULATE) ) + { + vaddr_t va = vfn << PAGE_SHIFT; + paddr_t pa; + unsigned long xen_virt_end = (XEN_VIRT_START + XEN_VIRT_SIZE - 1); + + if ( ((va >= DIRECTMAP_VIRT_START) && (va <= DIRECTMAP_VIRT_END)) || + ((va >= XEN_VIRT_START) && (va <= xen_virt_end)) ) + pa = virt_to_maddr(va); + else + pa = pt_walk(va); + + mfn = _mfn(paddr_to_pfn(pa)); + } + + /* * `vfn` and `mfn` must be both superpage aligned. * They are or-ed together and then checked against the size of * each level.
When pt_update() is called with arguments (..., INVALID_MFN, ..., 0 or 1), it indicates that a mapping is being destroyed/modifyed. `mfn` should be set properly in cases when modifying/destroying a mapping to ensure the correct page table `level` is returned. In the case of `mfn` == INVALID_MFN, the `mask` will take into account only `vfn` and could accidentally return an incorrect level. For example, if `vfn` is page table level 1 aligned, but it was mapped as page table level 0, then without the check below it will return `level` = 1 because only `vfn`, which is page table level 1 aligned, is taken into account when `mfn` == INVALID_MFN. To address this issue, during the destruction/modification of a mapping, physical address is calculated for provided `va`. This ensures that the appropriate mask is generated, resulting in the correct calculation of the level. Fixes: c2f1ded524 ("xen/riscv: page table handling") Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- xen/arch/riscv/pt.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-)