Re: [PATCH v2 5/8] xen/riscv: introduce asm/pmap.h header

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.07.2024 10:55, oleksii.kurochko@xxxxxxxxx wrote:
> On Tue, 2024-07-23 at 10:36 +0200, Jan Beulich wrote:
>> On 23.07.2024 10:02, Oleksii Kurochko wrote:
>>> On Mon, Jul 22, 2024 at 7:27 PM Julien Grall <julien@xxxxxxx>
>>> wrote:
>>>>>> On 22/07/2024 15:44, Oleksii Kurochko wrote:
>>>>>     /* Map a 4k page in a fixmap entry */
>>>>>     void set_fixmap(unsigned map, mfn_t mfn, unsigned int
>>>>> flags)
>>>>>     {
>>>>>         pte_t pte;
>>>>>
>>>>>         pte = mfn_to_xen_entry(mfn, flags);
>>>>>         pte.pte |= PTE_LEAF_DEFAULT;
>>>>>         write_pte(&xen_fixmap[pt_index(0, FIXMAP_ADDR(map))],
>>>>> pte);
>>>>
>>>> It would be saner to check if you are not overwriting any
>>>> existing
>>>> mapping as otherwise you will probably need a TLB flush.
>>>>
>>>>>     }
>>>>>
>>>>>     /* Remove a mapping from a fixmap entry */
>>>>>     void clear_fixmap(unsigned map)
>>>>>     {
>>>>>         pte_t pte = {0};
>>>>>         write_pte(&xen_fixmap[pt_index(0, FIXMAP_ADDR(map))],
>>>>> pte);
>>>>
>>>> Don't you need a TLB flush?
>>>>
>>> Inside write_pte() there is "sfence.vma".
>>
>> That's just a fence though, not a TLB flush.
> From the privileged doc:
>    ```
>    SFENCE.VMA is also used to invalidate entries in the
>    address-translation cache associated with a hart (see Section 4.3.2). 
>    ...
>    The SFENCE.VMA is used to flush any local hardware caches related to
>    address translation.
>    It is specified as a fence rather than a TLB flush to provide cleaner
>    semantics with respect to
>    which instructions are affected by the flush operation and to support a
>    wider variety of dynamic
>    caching structures and memory-management schemes. SFENCE.VMA is also
>    used by higher
>    privilege levels to synchronize page table writes and the address
>    translation hardware.
>    ...
>    ```
> I read this as SFENCE.VMA is used not only for ordering of load/stores,
> but also to flush TLB ( which is a type of more general term as
> address-translation cache, IIUIC ).

Oh, I see. Kind of unexpected for an instruction of that name. Yet note
how they talk about the local hart only. You need a wider scope TLB
flush here.

Jan