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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/26/25 1:01 PM, Jan Beulich wrote:

> 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.)
>     
Oh, I got your initial intention. For sure, it should be "types".

> 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?
>     
It could be really an option for optimization.

In this case I have to p2m_type_t by adding a new type p2m_tree_type:
typedef enum {
    p2m_invalid = 0,    /* Nothing mapped here */
    p2m_ram_rw,         /* Normal read/write domain RAM */
    p2m_ram_ro,         /* Read-only */
    
    + p2m_tree_type,    /* The types below p2m_free_type will be stored outside PTE's bits */

    p2m_mmio_direct_dev,/* Read/write mapping of genuine Device MMIO area */
    p2m_grant_map_rw,   /* Read/write grant mapping */
    p2m_grant_map_ro,   /* Read-only grant mapping */
} p2m_type_t;

Probably, it make sense to switch p2m_ram_ro and p2m_mmio_direct_dev. I think device mapping
is more often operations.

> > > > > > +    /*
> > > > > > +     * 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.
>     
At the moment, yes, but it isn't strict limitation. So yes, it should be page
tables should be conditionally shared.

~ Oleksii