| Message ID | 20220311061123.1883189-7-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Static shared memory on dom0less system | expand |
On Fri, 11 Mar 2022, Penny Zheng wrote: > From: Penny Zheng <penny.zheng@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.zheng@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) They should probably be gfn_t type > +{ > + 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. > + /* 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 >
Hi Stefano Sorry for the late response, got sidetracked an emergency issue. ;/ > -----Original Message----- > From: Stefano Stabellini <sstabelliHini@kernel.org> > Sent: Friday, March 18, 2022 10:00 AM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: xen-devel@lists.xenproject.org; nd <nd@arm.com>; Stefano Stabellini > <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Bertrand Marquis > <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > <Volodymyr_Babchuk@epam.com> > 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@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.zheng@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) > > 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
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@kernel.org> > > Sent: Friday, March 18, 2022 10:00 AM > > To: Penny Zheng <Penny.Zheng@arm.com> > > Cc: xen-devel@lists.xenproject.org; nd <nd@arm.com>; Stefano Stabellini > > <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; Bertrand Marquis > > <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > > <Volodymyr_Babchuk@epam.com> > > 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@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.zheng@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) > > > > 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.
Hi Stefano, On 08/04/2022 23:18, Stefano Stabellini wrote: > On Tue, 29 Mar 2022, Penny Zheng wrote: >> 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. AFAIU, your proposal would mean that the behavior for get_pages_from_gfn() and get_page_from_gfn() will differ. This is not great. I don't think we can easily change the behavior of get_page_from_gfn() because some callers can accept multiple types. Furthermore, while today we only check p2m_ram_rw. It might be possible that we would want to check different type. For instance, if we want to use read-only mapping, it would be fine to accept p2m_ram_ro and p2m_ram_rw. So overall, I am not in favor of initializing p2mt and let get_pages_from_gfn() to check the type. Cheers,
Hi Penny, On 11/03/2022 06:11, Penny Zheng wrote: > From: Penny Zheng <penny.zheng@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.zheng@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. - IIRC, guest_physmap_add_pages() doesn't revert the mappings. So you need to revert it in case of failure. Cheers,
On Fri, 8 Apr 2022, Julien Grall wrote: > On 08/04/2022 23:18, Stefano Stabellini wrote: > > On Tue, 29 Mar 2022, Penny Zheng wrote: > > > 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. > > AFAIU, your proposal would mean that the behavior for get_pages_from_gfn() and > get_page_from_gfn() will differ. This is not great. > > I don't think we can easily change the behavior of get_page_from_gfn() because > some callers can accept multiple types. > > Furthermore, while today we only check p2m_ram_rw. It might be possible that > we would want to check different type. For instance, if we want to use > read-only mapping, it would be fine to accept p2m_ram_ro and p2m_ram_rw. > > So overall, I am not in favor of initializing p2mt and let > get_pages_from_gfn() to check the type. OK. In that case, I suggest to leave the code pretty much as is in regards of how get_pages_from_gfn is called from guest_physmap_add_shm.
Hi, On 08/04/2022 23:59, Julien Grall wrote: > On 11/03/2022 06:11, Penny Zheng wrote: >> From: Penny Zheng <penny.zheng@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.zheng@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. > > - IIRC, guest_physmap_add_pages() doesn't revert the mappings. So you > need to revert it in case of failure. There is another issue here. guest_physmap_add_pages() may use superpage mapping. The P2M code is currently assuming the foreing mapping will be using L3 mapping (4KB). Do you need to use superpage mapping here? Cheers,
Hi julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Saturday, April 9, 2022 6:59 AM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org > Cc: nd <nd@arm.com>; Stefano Stabellini <sstabellini@kernel.org>; Bertrand > Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > <Volodymyr_Babchuk@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.zheng@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.zheng@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
Hi > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Saturday, April 9, 2022 5:31 PM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org > Cc: nd <nd@arm.com>; Stefano Stabellini <sstabellini@kernel.org>; Bertrand > Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > <Volodymyr_Babchuk@epam.com> > Subject: Re: [PATCH v1 06/13] xen/arm: set up shared memory foreign > mapping for borrower domain > > Hi, > > On 08/04/2022 23:59, Julien Grall wrote: > > On 11/03/2022 06:11, Penny Zheng wrote: > >> From: Penny Zheng <penny.zheng@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.zheng@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. > > > > - IIRC, guest_physmap_add_pages() doesn't revert the mappings. So > > you need to revert it in case of failure. > > > There is another issue here. guest_physmap_add_pages() may use superpage > mapping. The P2M code is currently assuming the foreing mapping will be > using L3 mapping (4KB). > > Do you need to use superpage mapping here? > Right now, there is no user case on my side needing superpage mapping. > Cheers, > > -- > Julien Grall
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; + + 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; + } + + /* 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;