Re: [PATCH v3 5/9] xen/riscv: introduce asm/pmap.h header

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.07.2024 13:39, oleksii.kurochko@xxxxxxxxx wrote:
> On Tue, 2024-07-30 at 09:56 +0200, Jan Beulich wrote:
>> On 29.07.2024 18:22, oleksii.kurochko@xxxxxxxxx wrote:
>>> On Mon, 2024-07-29 at 16:29 +0200, Jan Beulich wrote:
>>>> On 24.07.2024 17:31, Oleksii Kurochko wrote:
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/riscv/include/asm/pmap.h
>>>>> @@ -0,0 +1,33 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>>> +#ifndef ASM_PMAP_H
>>>>> +#define ASM_PMAP_H
>>>>> +
>>>>> +#include <xen/bug.h>
>>>>> +#include <xen/mm.h>
>>>>> +#include <xen/page-size.h>
>>>>> +
>>>>> +#include <asm/fixmap.h>
>>>>> +#include <asm/flushtlb.h>
>>>>> +#include <asm/system.h>
>>>>> +
>>>>> +static inline void arch_pmap_map(unsigned int slot, mfn_t mfn)
>>>>> +{
>>>>> +    pte_t *entry = &xen_fixmap[slot];
>>>>> +    pte_t pte;
>>>>> +
>>>>> +    ASSERT(!pte_is_valid(*entry));
>>>>> +
>>>>> +    pte = mfn_to_xen_entry(mfn, PAGE_HYPERVISOR_RW);
>>>>> +    write_pte(entry, pte);
>>>>> +}
>>>>
>>>> Perhaps add a comment to clarify why it's safe to omit a TLB
>>>> flush
>>>> here.
>>>> Note that arch_pmap_unmap() having one is a necessary but not
>>>> sufficient
>>>> condition to that. In principle hardware may also cache
>>>> "negative"
>>>> TLB
>>>> entries; I have no idea how RISC-V behaves in this regard, or
>>>> whether
>>>> that aspect is actually left to implementations.
>>> what do you mean by "negative" TLB? an old TLB entry which should
>>> be
>>> invalidated?
>>
>> No, I mean TLB entries saying "no valid translation here". I.e.
>> avoiding
>> recurring walks of something that was once found to have no
>> translation.
> But we can't modify an existent entry because we have
> "ASSERT(!pte_is_valid(*entry))" in pmap_map() function and we are doing
> TLB flush during pmap_unmap().

You _always_ modify an existing entry. That may be a not-present one, but
that's still an entry. And that's part of why I think you still didn't
understand what I said. A particular implementation, if permitted by the
spec, may very well put in place a TLB entry when the result of a page
walk was a non-present entry. So ...

> So when we will be in pmap_map() we are
> sure that there is no TLB entry for the new pte.

..., can you point me at the part of the spec saying that such "negative"
TLB entries are not permitted?

Jan