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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.12.2024 12:14, Oleksii Kurochko wrote:
> 
> 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.

Right, and that is one of the points. In release builds a potential
bad call here wouldn't be prevented if there's just an assertion.
Unlike if there was an if() instead (perhaps with ASSERT_UNREACHABLE()
on its "else" path).

> 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.

I don't think we need to be afraid of performance issues here.

Jan