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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 29 Mar 2022, Penny Zheng wrote:
> 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.

Yes, I think I understand. I think your suggestion is OK too. However,
your suggestion is much more work than mine: I was only suggesting a
small improvement limited to guest_physmap_add_shm and
get_pages_from_gfn. Your suggestion involves a device tree change that
would have a larger impact on the patch series. So if you are up for it,
I am happy to review it. I am also fine just to have a small improvement
on guest_physmap_add_shm/get_pages_from_gfn.