Re: [PATCH v3 11/20] xen/riscv: implement function to map memory in guest p2m

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.08.2025 11:29, Oleksii Kurochko wrote:
> 
> On 8/11/25 9:28 AM, Jan Beulich wrote:
>> On 08.08.2025 15:46, Oleksii Kurochko wrote:
>>> On 8/5/25 5:20 PM, Jan Beulich wrote:
>>>> On 31.07.2025 17:58, Oleksii Kurochko wrote:
>>>>> +/* Unlock the flush and do a P2M TLB flush if necessary */
>>>>> +void p2m_write_unlock(struct p2m_domain *p2m)
>>>>> +{
>>>>> +    /*
>>>>> +     * The final flush is done with the P2M write lock taken to avoid
>>>>> +     * someone else modifying the P2M wbefore the TLB invalidation has
>>>> Nit: Stray 'w'.
>>>>
>>>>> +     * completed.
>>>>> +     */
>>>>> +    p2m_tlb_flush_sync(p2m);
>>>> Wasn't the plan to have this be conditional?
>>> Not really, probably, I misunderstood you before.
>>>
>>> Previously, I only had|p2m_force_tlb_flush_sync()| here, instead of
>>> |p2m_tlb_flush_sync()|, and the latter includes a condition check on
>>> |p2m->need_flush|.
>> Just to re-iterate my point: Not every unlock will require a flush. Hence
>> why I expect the flush to be conditional upon there being an indication
>> that some change was done that requires flushing.
>>
> The flush is actually conditional; the condition is inside
> |p2m_tlb_flush_sync()|:
>    void p2m_tlb_flush_sync(struct p2m_domain *p2m)
>    {
>      if ( p2m->need_flush )
>        p2m_force_tlb_flush_sync(p2m);
>    }

Hmm, I'd consider this misleading function naming then. Especially with
"force" and "sync" being kind of redundant with one another already anyway.
See x86'es naming.

Jan