| Message ID | 20220620051114.210118-4-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | static shared memory on dom0less system | expand |
Hi Penny, On 20/06/2022 06:11, Penny Zheng wrote: > If owner property is defined, then owner domain of a static shared memory > region is not the default dom_io anymore, but a specific domain. > > This commit implements allocating static shared memory to a specific domain > when owner property is defined. > > Coding flow for dealing borrower domain will be introduced later in the > following commits. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > v5 change: > - no change > --- > v4 change: > - no changes > --- > v3 change: > - simplify the code since o_gbase is not used if the domain is dom_io > --- > v2 change: > - P2M mapping is restricted to normal domain > - in-code comment fix > --- > xen/arch/arm/domain_build.c | 44 +++++++++++++++++++++++++++---------- > 1 file changed, 33 insertions(+), 11 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 91a5ace851..d4fd64e2bd 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -805,9 +805,11 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d, > */ > static int __init allocate_shared_memory(struct domain *d, > u32 addr_cells, u32 size_cells, > - paddr_t pbase, paddr_t psize) > + paddr_t pbase, paddr_t psize, > + paddr_t gbase) > { > mfn_t smfn; > + int ret = 0; > > dprintk(XENLOG_INFO, > "Allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n", > @@ -822,8 +824,18 @@ static int __init allocate_shared_memory(struct domain *d, > * DOMID_IO is the domain, like DOMID_XEN, that is not auto-translated. > * It sees RAM 1:1 and we do not need to create P2M mapping for it > */ > - ASSERT(d == dom_io); > - return 0; > + if ( d != dom_io ) > + { > + ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn, PFN_DOWN(psize)); Coding style: this line is over 80 characters. And... > + if ( ret ) > + { > + printk(XENLOG_ERR > + "Failed to map shared memory to %pd.\n", d); ... this line could be merged with the previous one. > + return ret; > + } > + } > + > + return ret; > } > > static int __init process_shm(struct domain *d, > @@ -836,6 +848,8 @@ static int __init process_shm(struct domain *d, > u32 shm_id; > u32 addr_cells, size_cells; > paddr_t gbase, pbase, psize; > + const char *role_str; > + bool owner_dom_io = true; I think it would be best if role_str and owner_dom_io are defined within the loop. Same goes for all the other declarations. > > dt_for_each_child_node(node, shm_node) > { > @@ -862,19 +876,27 @@ static int __init process_shm(struct domain *d, > ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE)); > gbase = dt_read_number(cells, addr_cells); > > - /* TODO: Consider owner domain is not the default dom_io. */ > + /* > + * "role" property is optional and if it is defined explicitly, > + * then the owner domain is not the default "dom_io" domain. > + */ > + if ( dt_property_read_string(shm_node, "role", &role_str) == 0 ) > + owner_dom_io = false IIUC, the role is per-region. However, owner_dom_io is first initialized to false outside the loop. Therefore, the variable may not be correct on the next region. So I think you want to write: owner_dom_io = !dt_property_read_string(...); This can also be avoided if you reduce the scope of the variable (it is meant to only be used in the loop). > + > /* > * Per static shared memory region could be shared between multiple > * domains. > - * In case re-allocating the same shared memory region, we check > - * if it is already allocated to the default owner dom_io before > - * the actual allocation. > + * So when owner domain is the default dom_io, in case re-allocating > + * the same shared memory region, we check if it is already allocated > + * to the default owner dom_io before the actual allocation. > */ > - if ( !is_shm_allocated_to_domio(pbase) ) > + if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) || > + (!owner_dom_io && strcmp(role_str, "owner") == 0) ) > { > - /* Allocate statically shared pages to the default owner dom_io. */ > - ret = allocate_shared_memory(dom_io, addr_cells, size_cells, > - pbase, psize); > + /* Allocate statically shared pages to the owner domain. */ > + ret = allocate_shared_memory(owner_dom_io ? dom_io : d, > + addr_cells, size_cells, > + pbase, psize, gbase); > if ( ret ) > return ret; > } Cheers,
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Saturday, June 25, 2022 3:07 AM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org > Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > Subject: Re: [PATCH v5 3/8] xen/arm: allocate static shared memory to a > specific owner domain > > Hi Penny, > > On 20/06/2022 06:11, Penny Zheng wrote: > > If owner property is defined, then owner domain of a static shared > > memory region is not the default dom_io anymore, but a specific domain. > > > > This commit implements allocating static shared memory to a specific > > domain when owner property is defined. > > > > Coding flow for dealing borrower domain will be introduced later in > > the following commits. > > > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > --- > > v5 change: > > - no change > > --- > > v4 change: > > - no changes > > --- > > v3 change: > > - simplify the code since o_gbase is not used if the domain is dom_io > > --- > > v2 change: > > - P2M mapping is restricted to normal domain > > - in-code comment fix > > --- > > xen/arch/arm/domain_build.c | 44 +++++++++++++++++++++++++++-------- > -- > > 1 file changed, 33 insertions(+), 11 deletions(-) > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index 91a5ace851..d4fd64e2bd 100644 > > --- a/xen/arch/arm/domain_build.c > > +++ b/xen/arch/arm/domain_build.c > > @@ -805,9 +805,11 @@ static mfn_t __init > acquire_shared_memory_bank(struct domain *d, > > */ > > static int __init allocate_shared_memory(struct domain *d, > > u32 addr_cells, u32 size_cells, > > - paddr_t pbase, paddr_t psize) > > + paddr_t pbase, paddr_t psize, > > + paddr_t gbase) > > { > > mfn_t smfn; > > + int ret = 0; > > > > dprintk(XENLOG_INFO, > > "Allocate static shared memory BANK > > %#"PRIpaddr"-%#"PRIpaddr".\n", @@ -822,8 +824,18 @@ static int __init > allocate_shared_memory(struct domain *d, > > * DOMID_IO is the domain, like DOMID_XEN, that is not auto- > translated. > > * It sees RAM 1:1 and we do not need to create P2M mapping for it > > */ > > - ASSERT(d == dom_io); > > - return 0; > > + if ( d != dom_io ) > > + { > > + ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn, > > + PFN_DOWN(psize)); > > Coding style: this line is over 80 characters. And... > > > + if ( ret ) > > + { > > + printk(XENLOG_ERR > > + "Failed to map shared memory to %pd.\n", d); > > ... this line could be merged with the previous one. > > > + return ret; > > + } > > + } > > + > > + return ret; > > } > > > > static int __init process_shm(struct domain *d, @@ -836,6 +848,8 @@ > > static int __init process_shm(struct domain *d, > > u32 shm_id; > > u32 addr_cells, size_cells; > > paddr_t gbase, pbase, psize; > > + const char *role_str; > > + bool owner_dom_io = true; > > I think it would be best if role_str and owner_dom_io are defined within the > loop. Same goes for all the other declarations. > > > > > dt_for_each_child_node(node, shm_node) > > { > > @@ -862,19 +876,27 @@ static int __init process_shm(struct domain *d, > > ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, > PAGE_SIZE)); > > gbase = dt_read_number(cells, addr_cells); > > > > - /* TODO: Consider owner domain is not the default dom_io. */ > > + /* > > + * "role" property is optional and if it is defined explicitly, > > + * then the owner domain is not the default "dom_io" domain. > > + */ > > + if ( dt_property_read_string(shm_node, "role", &role_str) == 0 ) > > + owner_dom_io = false > IIUC, the role is per-region. However, owner_dom_io is first initialized to > false outside the loop. Therefore, the variable may not be correct on the next > region. > > So I think you want to write: > > owner_dom_io = !dt_property_read_string(...); > > This can also be avoided if you reduce the scope of the variable (it is meant > to only be used in the loop). > Yes, it is a bug, thx!!! I'll reduce the scope. > > + > > /* > > * Per static shared memory region could be shared between multiple > > * domains. > > - * In case re-allocating the same shared memory region, we check > > - * if it is already allocated to the default owner dom_io before > > - * the actual allocation. > > + * So when owner domain is the default dom_io, in case re-allocating > > + * the same shared memory region, we check if it is already allocated > > + * to the default owner dom_io before the actual allocation. > > */ > > - if ( !is_shm_allocated_to_domio(pbase) ) > > + if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) || > > + (!owner_dom_io && strcmp(role_str, "owner") == 0) ) > > { > > - /* Allocate statically shared pages to the default owner dom_io. */ > > - ret = allocate_shared_memory(dom_io, addr_cells, size_cells, > > - pbase, psize); > > + /* Allocate statically shared pages to the owner domain. */ > > + ret = allocate_shared_memory(owner_dom_io ? dom_io : d, > > + addr_cells, size_cells, > > + pbase, psize, gbase); > > if ( ret ) > > return ret; > > } > > Cheers, > > -- > Julien Grall
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 91a5ace851..d4fd64e2bd 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -805,9 +805,11 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d, */ static int __init allocate_shared_memory(struct domain *d, u32 addr_cells, u32 size_cells, - paddr_t pbase, paddr_t psize) + paddr_t pbase, paddr_t psize, + paddr_t gbase) { mfn_t smfn; + int ret = 0; dprintk(XENLOG_INFO, "Allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n", @@ -822,8 +824,18 @@ static int __init allocate_shared_memory(struct domain *d, * DOMID_IO is the domain, like DOMID_XEN, that is not auto-translated. * It sees RAM 1:1 and we do not need to create P2M mapping for it */ - ASSERT(d == dom_io); - return 0; + if ( d != dom_io ) + { + ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn, PFN_DOWN(psize)); + if ( ret ) + { + printk(XENLOG_ERR + "Failed to map shared memory to %pd.\n", d); + return ret; + } + } + + return ret; } static int __init process_shm(struct domain *d, @@ -836,6 +848,8 @@ static int __init process_shm(struct domain *d, u32 shm_id; u32 addr_cells, size_cells; paddr_t gbase, pbase, psize; + const char *role_str; + bool owner_dom_io = true; dt_for_each_child_node(node, shm_node) { @@ -862,19 +876,27 @@ static int __init process_shm(struct domain *d, ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE)); gbase = dt_read_number(cells, addr_cells); - /* TODO: Consider owner domain is not the default dom_io. */ + /* + * "role" property is optional and if it is defined explicitly, + * then the owner domain is not the default "dom_io" domain. + */ + if ( dt_property_read_string(shm_node, "role", &role_str) == 0 ) + owner_dom_io = false; + /* * Per static shared memory region could be shared between multiple * domains. - * In case re-allocating the same shared memory region, we check - * if it is already allocated to the default owner dom_io before - * the actual allocation. + * So when owner domain is the default dom_io, in case re-allocating + * the same shared memory region, we check if it is already allocated + * to the default owner dom_io before the actual allocation. */ - if ( !is_shm_allocated_to_domio(pbase) ) + if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) || + (!owner_dom_io && strcmp(role_str, "owner") == 0) ) { - /* Allocate statically shared pages to the default owner dom_io. */ - ret = allocate_shared_memory(dom_io, addr_cells, size_cells, - pbase, psize); + /* Allocate statically shared pages to the owner domain. */ + ret = allocate_shared_memory(owner_dom_io ? dom_io : d, + addr_cells, size_cells, + pbase, psize, gbase); if ( ret ) return ret; }