|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 15/17] xen/riscv: Implement superpage splitting for p2m mappings
On 7/21/25 3:34 PM, Jan Beulich wrote:
On 17.07.2025 18:37, Oleksii Kurochko wrote:On 7/2/25 11:25 AM, Jan Beulich wrote:On 10.06.2025 15:05, Oleksii Kurochko wrote: I think that I still don't understand why what I wrote above will work as long
as global flushes is working and will stop to work when more fine-grained flushing
is used.
Inside p2m_split_superpage() we don't need any kind of TLB flush operation as
it is allocation a new page for a table and works with it, so no one could use
this page at the moment and cache it in TLB.
The only question is that if it is needed BBM before staring using splitted entry:
....
if ( !p2m_split_superpage(p2m, &split_pte, level, target, offsets) )
{
/* Free the allocated sub-tree */
p2m_free_subtree(p2m, split_pte, level);
rc = -ENOMEM;
goto out;
}
------> /* Should be BBM used here ? */
p2m_write_pte(entry, split_pte, p2m->clean_pte);
And I can't find anything in the spec what tells me to use BBM here like Arm
does:
/*
* Follow the break-before-sequence to update the entry.
* For more details see (D4.7.1 in ARM DDI 0487A.j).
*/
p2m_remove_pte(entry, p2m->clean_pte);
p2m_force_tlb_flush_sync(p2m);
p2m_write_pte(entry, split_pte, p2m->clean_pte);
I agree that even BBM isn't mandated explicitly sometime it could be useful, but
here I am not really sure what is the point to do TLB flush before p2m_write_pte()
as nothing serious will happen if and old mapping will be used for a some time
as we are keeping the same rights for smaller (splited) mapping.
The reason why Arm does p2m_remove_pte() & p2m_force_tlb_flush() before updating
an entry here as it is doesn't guarantee that everything will be okay and they
explicitly tells:
This situation can possibly break coherency and ordering guarantees, leading to
inconsistent unwanted behavior in our Processing Element (PE).
As to (no need for) BBM: I couldn't find anything to that effect in the privileged spec. Can you provide some pointer? What I found instead is e.g. this sentence: "To ensure that implicit reads observe writes to the same memory locations, an SFENCE.VMA instruction must be executed after the writes to flush the relevant cached translations." And this: "Accessing the same location using different cacheability attributes may cause loss of coherence." (This may not only occur when the same physical address is mapped twice at different VAs, but also after the shattering of a superpage when the new entry differs in cacheability.)I also couldn't find that RISC-V spec mandates BBM explicitly as Arm does it. What I meant that on RISC-V can do: - Write new PTE - Flush TLB While on Arm it is almost always needed to do: - Write zero to PTE - Flush TLB - Write new PTE For example, the common CoW code path where you copy from a read only page to a new page, then map that new page as writable just works on RISC-V without extra considerations and on Arm it requires BBM.CoW is a specific sub-case with increasing privilege. Could you please explain why it matters increasing of privilege in terms of TLB flushing and PTE clearing before writing a new PTE? It seems to me that BBM is mandated for Arm only because that TLB is shared among cores, so there is no any guarantee that no prior entry for same VA remains in TLB. In case of RISC-V's TLB isn't shared and after a flush it is guaranteed that no prior entry for the same VA remains in the TLB.Not sure that's the sole reason. But again the question is: Is this written down explicitly anywhere? Generally there can be multiple levels of TLBs, and while some of them may be per-core, others may be shared. Spec. mentions that: If a hart employs an address-translation cache, that cache must appear to be private to that hart. + /* + * Even if we failed, we should install the newly allocated PTE + * entry. The caller will be in charge to free the sub-tree. + */ + p2m_write_pte(entry, page_to_p2m_table(p2m, page), p2m->clean_pte);Why would it be wrong to free the page right here, vacating the entry at the same time (or leaving just that to the caller)? (IOW - if this is an implementation decision of yours, I think the word "should" would want dropping.) After all, the caller invoking p2m_free_entry() on the thus split PTE is less efficient (needs to iterate over all entries) than on the original one (where it's just a single superpage).I think that I didn't get your idea.Well, first and foremost this was a question to you, as it's not clear to me whether "should" is the correct word to use here. It would be appropriate if the spec mandated this behavior. It would seem less appropriate if this arrangement was an implementation choice of yours. And it looks to me as if the latter was the case here. No, the spec doesn't mandate such behavior, it is just implementation specific. I can mention that in the comment.
Yes, the caller determines expected level and then asks __p2m_set_entry() to map on this level. ~ Oleksii
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |