Re: [PATCH v1 1/6] xen/riscv: add destroy_xen_mappings() to remove mappings in Xen page tables

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 12/9/24 3:23 PM, Jan Beulich wrote:

> On 27.11.2024 13:50, Oleksii Kurochko wrote:
> > Introduce the destroy_xen_mappings() function, which removes page
> > mappings in Xen's page tables between a start address s and an end
> > address e.
> > The function ensures that both s and e are page-aligned
> > and verifies that the start address is less than or equal to the end
> > address before calling pt_update() to invalidate the mappings.
> > The pt_update() function is called with INVALID_MFN and PTE_VALID=0
> > in the flags, which tell pt_update() to remove mapping. No additional
> > ASSERT() is required to check these arguments, as they are hardcoded in
> > the call to pt_update() within destroy_xen_mappings().
> >
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> >       
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> However, ...
>
> > --- a/xen/arch/riscv/pt.c
> > +++ b/xen/arch/riscv/pt.c
> > @@ -421,6 +421,14 @@ int map_pages_to_xen(unsigned long virt,
> >      return pt_update(virt, mfn, nr_mfns, flags);
> >  }
> >  
> > +int destroy_xen_mappings(unsigned long s, unsigned long e)
> > +{
> > +    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
> > +    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
> > +    ASSERT(s <= e);
> > +    return pt_update(s, INVALID_MFN, PFN_DOWN(e - s), 0);
> > +}
> >       
> ... I'm unconvinced the constraints need to be this strict. You could,
> for example, very well just avoiding to call pt_update() when s > e
> (or really when s >= e). Thoughts?
>     
On one hand, we could simply avoid calling pt_update(), but on the other hand,
this approach might cause us to miss a bug without any notification.
Given that this is an ASSERT() that only triggers in debug builds and is unlikely to occur,
I believe it is not critical to include the ASSERT() here. Additionally, avoiding an extra
if condition helps prevent any potential performance impact. However, the if condition
would likely evaluate to true most of the time, allowing hardware optimizations to handle
it efficiently.
~ Oleksii