RE: [PATCH v1 06/13] xen/arm: set up shared memory foreign mapping for borrower domain

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Stefano

Sorry for the late response, got sidetracked an emergency issue. ;/

> -----Original Message-----
> From: Stefano Stabellini <sstabelliHini@xxxxxxxxxx>
> Sent: Friday, March 18, 2022 10:00 AM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; nd <nd@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Bertrand Marquis
> <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v1 06/13] xen/arm: set up shared memory foreign
> mapping for borrower domain
> 
> On Fri, 11 Mar 2022, Penny Zheng wrote:
> > From: Penny Zheng <penny.zheng@xxxxxxx>
> >
> > This commits introduces a new helper guest_physmap_add_shm to set up
> > shared memory foreign mapping for borrower domain.
> >
> > Firstly it should get and take reference of statically shared pages
> > from owner dom_shared. Then it will setup P2M foreign memory map of
> > these statically shared pages for borrower domain.
> >
> > This commits only considers owner domain is the default dom_shared,
> > the other scenario will be covered in the following patches.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > ---
> >  xen/arch/arm/domain_build.c | 52
> > +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 984e70e5fc..8cee5ffbd1 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -798,6 +798,48 @@ static int __init allocate_shared_memory(struct
> domain *d,
> >      return ret;
> >  }
> >
> > +static int __init guest_physmap_add_shm(struct domain *od, struct domain
> *bd,
> > +                                        unsigned long o_gfn,
> > +                                        unsigned long b_gfn,
> > +                                        unsigned long nr_gfns)
> 
> They should probably be gfn_t type
> 
>
 
Sure, will do.

> > +{
> > +    struct page_info **pages = NULL;
> > +    p2m_type_t p2mt, t;
> > +    int ret = 0;
> > +
> > +    pages = xmalloc_array(struct page_info *, nr_gfns);
> > +    if ( !pages )
> > +        return -ENOMEM;
> > +
> > +    /*
> > +     * Take reference of statically shared pages from owner domain.
> > +     * Reference will be released when destroying shared memory region.
> > +     */
> > +    ret = get_pages_from_gfn(od, o_gfn, nr_gfns, pages, &p2mt, P2M_ALLOC);
> > +    if ( ret )
> > +    {
> > +        ret = -EINVAL;
> > +        goto fail_pages;
> > +    }
> > +
> > +    if ( p2m_is_ram(p2mt) )
> > +        t = (p2mt == p2m_ram_rw) ? p2m_map_foreign_rw :
> p2m_map_foreign_ro;
> > +    else
> > +    {
> > +        ret = -EINVAL;
> > +        goto fail_pages;
> > +    }
> 
> One idea is to initialize p2mt = p2m_ram_rw and pass it to
> get_pages_from_gfn. Then get_pages_from_gfn can return error immediately
> if any of the pages are of different type.
> 
> This way there is no need for checking again here.
>

Right now, the memory attribute of static shared memory is RW as default,
What if we add memory attribute setting in device tree configuration, sometimes,
Users want to specify that borrower domain only has RO right, hmm, then the
Initialization for p2mt could be either p2m_ram_rw or p2m_ram_ro?
In such case, we could add another parameter in guest_physmap_add_shm to
show the p2m type...
Hope I understand what you suggested here.
 
> 
> > +    /* Set up guest foreign map. */
> > +    ret = guest_physmap_add_pages(bd, _gfn(b_gfn), page_to_mfn(pages[0]),
> > +                                  nr_gfns, t);
> > +
> > + fail_pages:
> > +        xfree(pages);
> > +
> > +    return ret;
> > +}
> > +
> >  static int __init process_shm(struct domain *d,
> >                                const struct dt_device_node *node)  {
> > @@ -855,6 +897,16 @@ static int __init process_shm(struct domain *d,
> >
> >              set_bit(shm_id, shm_mask);
> >          }
> > +
> > +        /*
> > +         * All domains are borrower domains when owner domain is the
> > +         * default dom_shared, so here we could just set up P2M foreign
> > +         * mapping for borrower domain immediately.
> > +         */
> > +        ret = guest_physmap_add_shm(dom_shared, d, PFN_DOWN(pbase),
> > +                                    PFN_DOWN(gbase), PFN_DOWN(psize));
> > +        if ( ret )
> > +            return ret;
> >      }
> >
> >      return 0;
> > --
> > 2.25.1
> >

---
Cheers,
Penny Zheng