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
