Re: [PATCH v2 05/17] xen/riscv: introduce things necessary for p2m initialization

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.06.2025 10:40, Oleksii Kurochko wrote:
> On 6/25/25 5:53 PM, Jan Beulich wrote:
>> On 25.06.2025 17:31, Oleksii Kurochko wrote:
>>> On 6/18/25 6:08 PM, Jan Beulich wrote:
>>>> On 10.06.2025 15:05, Oleksii Kurochko wrote:
>>>>> @@ -14,6 +18,29 @@
>>>>>    
>>>>>    /* Per-p2m-table state */
>>>>>    struct p2m_domain {
>>>>> +    /*
>>>>> +     * Lock that protects updates to the p2m.
>>>>> +     */
>>>>> +    rwlock_t lock;
>>>>> +
>>>>> +    /* Pages used to construct the p2m */
>>>>> +    struct page_list_head pages;
>>>>> +
>>>>> +    /* Indicate if it is required to clean the cache when writing an 
>>>>> entry */
>>>>> +    bool clean_pte;
>>>>> +
>>>>> +    struct radix_tree_root p2m_type;
>>>> A field with a p2m_ prefix in a p2m struct?
>>> p2m_ prefix could be really dropped.
>>>
>>>>    And is this tree really about
>>>> just a single "type"?
>>> Yes, we don't have enough bits in PTE so we need some extra storage to 
>>> store type.
>> My question wasn't about that, though. My question was whether in the name
>> "type" (singular) is appropriate. I didn't think you need a tree to store 
>> just
>> a single type.
> 
> I need tree to store a pair of <gfn, p2m_type>, where gfn is an index. And it 
> seems
> to me a tree is a good structure for fast insert/search.

Hmm, I'm increasingly puzzled. I tried to emphasize that my question was towards
the singular "type" in the variable name. I can't see any relationship between
that and your reply. (And yes, using a tree here may be appropriate. There is a
concern towards memory consumption, but that's a separate topic.)

Having said that, aiui you don't use the two RSW bits in the PTE. Do you have
any plans there? If not, can't they be used to at least represent the most
commonly used types, such that the number of entries in that tree can be kept
(relatively) low?

>>>>> +    /*
>>>>> +     * Some IOMMUs don't support coherent PT walk. When the p2m is
>>>>> +     * shared with the CPU, Xen has to make sure that the PT changes have
>>>>> +     * reached the memory
>>>>> +     */
>>>>> +    p2m->clean_pte = is_iommu_enabled(d) &&
>>>>> +        !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
>>>> The comment talks about shared page tables, yet you don't check whether
>>>> page table sharing is actually enabled for the domain.
>>> Do we have such function/macros?
>> We have iommu_hap_pt_share, and we have the per-domain hap_pt_share flag.
>>
>>> It is shared by implementation now.
>> I don't understand. There's no IOMMU support yet for RISC-V. Hence it's in
>> neither state - not shared, but also not not shared.
> 
> In downstream there is a support of IOMMU for RISC-V.

And there page tables are unconditionally shared? I'll be surprised if no
want/need for non-shared page tables would ever appear.

Jan