Hi julien
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: Saturday, April 9, 2022 6:59 AM
> To: Penny Zheng <penny.zh...@arm.com>; xen-devel@lists.xenproject.org
> Cc: nd <n...@arm.com>; Stefano Stabellini <sstabell...@kernel.org>; Bertrand
> Marquis <bertrand.marq...@arm.com>; Volodymyr Babchuk
> <volodymyr_babc...@epam.com>
> Subject: Re: [PATCH v1 06/13] xen/arm: set up shared memory foreign
> mapping for borrower domain
>
> Hi Penny,
>
> On 11/03/2022 06:11, Penny Zheng wrote:
> > From: Penny Zheng <penny.zh...@arm.com>
> >
> > 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.zh...@arm.com>
> > ---
> > 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) {
> > + struct page_info **pages = NULL;
> > + p2m_type_t p2mt, t;
> > + int ret = 0;
>
> You don't need to initialize ret.
>
> > +
> > + 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;
>
> Where would we release the references?
>
> > + }
> > +
> > + /* Set up guest foreign map. */
> > + ret = guest_physmap_add_pages(bd, _gfn(b_gfn), page_to_mfn(pages[0]),
> > + nr_gfns, t);
>
> A few things:
> - The beginning of the code assumes that the MFN may be discontiguous in
> the physical memory. But here, you are assuming they are contiguous.
> If you want the latter, then you should check the MFNs are contiguous.
> That said, I am not sure if this restriction is really necessary.
>
Oh,, caught me, it never occurred to me that the pages in array `pages` could
be discontinuous.
I definitely want the latter. This function is always dealing with a memory
region
(contiguous MFNS) each time. So maybe a loop to check
page_to_mfn(pages[i]) == mfn_add(smfn, i) is needed.
> - IIRC, guest_physmap_add_pages() doesn't revert the mappings. So you
> need to revert it in case of failure.
>
Yes, both failure points you are referring to are requiring reverting the
mappings.
> Cheers,
>
> --
> Julien Grall
