| Message ID | 20240423082532.776623-3-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Static shared memory followup v2 - pt2 | expand |
Hi Luca, On 23/04/2024 10:25, Luca Fancellu wrote: > > > Wrap the code and logic that is calling assign_shared_memory > and map_regions_p2mt into a new function 'handle_shared_mem_bank', > it will become useful later when the code will allow the user to > don't pass the host physical address. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > --- > xen/arch/arm/static-shmem.c | 71 +++++++++++++++++++++++-------------- > 1 file changed, 45 insertions(+), 26 deletions(-) > > diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c > index f6cf74e58a83..24e40495a481 100644 > --- a/xen/arch/arm/static-shmem.c > +++ b/xen/arch/arm/static-shmem.c > @@ -185,6 +185,47 @@ append_shm_bank_to_domain(struct shared_meminfo *kinfo_shm_mem, paddr_t start, > return 0; > } > > +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, > + bool owner_dom_io, > + const char *role_str, > + const struct membank *shm_bank) > +{ > + paddr_t pbase, psize; > + int ret; > + > + BUG_ON(!shm_bank); not needed > + > + pbase = shm_bank->start; > + psize = shm_bank->size; please add empty line here > + /* > + * DOMID_IO is a fake domain and is not described in the Device-Tree. > + * Therefore when the owner of the shared region is DOMID_IO, we will > + * only find the borrowers. > + */ > + if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) || > + (!owner_dom_io && strcmp(role_str, "owner") == 0) ) > + { > + /* > + * We found the first borrower of the region, the owner was not > + * specified, so they should be assigned to dom_io. > + */ > + ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase, shm_bank); > + if ( ret ) > + return ret; > + } > + > + if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) ) > + { > + /* Set up P2M foreign mapping for borrower domain. */ > + ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize), > + _mfn(PFN_UP(pbase)), p2m_map_foreign_rw); > + if ( ret ) > + return ret; > + } > + > + return 0; > +} > + > int __init process_shm(struct domain *d, struct kernel_info *kinfo, > const struct dt_device_node *node) > { > @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, > if ( dt_property_read_string(shm_node, "role", &role_str) == 0 ) > owner_dom_io = false; Looking at owner_dom_io, why don't you move parsing role and setting owner_dom_io accordingly to handle_shared_mem_bank()? ~Michal
Hi Michal, >> >> +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, >> + bool owner_dom_io, >> + const char *role_str, >> + const struct membank *shm_bank) >> +{ >> + paddr_t pbase, psize; >> + int ret; >> + >> + BUG_ON(!shm_bank); > not needed > >> + >> + pbase = shm_bank->start; >> + psize = shm_bank->size; > please add empty line here Will do >> >> int __init process_shm(struct domain *d, struct kernel_info *kinfo, >> const struct dt_device_node *node) >> { >> @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, >> if ( dt_property_read_string(shm_node, "role", &role_str) == 0 ) >> owner_dom_io = false; > Looking at owner_dom_io, why don't you move parsing role and setting owner_dom_io accordingly to handle_shared_mem_bank()? I think I wanted to keep all dt_* functions on the same level inside process_shm, otherwise yes, I could pass down shm_node and do the reading of role_str in handle_shared_mem_bank, or I could derive owner_dom_io from role_str being passed or not, something like: role_str = NULL; dt_property_read_string(shm_node, "role", &role_str) [inside handle_shared_mem_bank]: If ( role_str ) owner_dom_io = false; And pass only role_str to handle_shared_mem_bank. Is this comment to reduce the number of parameters passed? I guess it’s not for where we call dt_property_read_string isn’t it? Cheers, Luca
On 07/05/2024 15:57, Luca Fancellu wrote: > > > Hi Michal, > >>> >>> +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, >>> + bool owner_dom_io, >>> + const char *role_str, >>> + const struct membank *shm_bank) >>> +{ >>> + paddr_t pbase, psize; >>> + int ret; >>> + >>> + BUG_ON(!shm_bank); >> not needed >> >>> + >>> + pbase = shm_bank->start; >>> + psize = shm_bank->size; >> please add empty line here > > Will do >>> >>> int __init process_shm(struct domain *d, struct kernel_info *kinfo, >>> const struct dt_device_node *node) >>> { >>> @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, >>> if ( dt_property_read_string(shm_node, "role", &role_str) == 0 ) >>> owner_dom_io = false; >> Looking at owner_dom_io, why don't you move parsing role and setting owner_dom_io accordingly to handle_shared_mem_bank()? > > I think I wanted to keep all dt_* functions on the same level inside process_shm, otherwise yes, I could > pass down shm_node and do the reading of role_str in handle_shared_mem_bank, or I could derive > owner_dom_io from role_str being passed or not, something like: > > role_str = NULL; > dt_property_read_string(shm_node, "role", &role_str) > > [inside handle_shared_mem_bank]: > If ( role_str ) > owner_dom_io = false; > > And pass only role_str to handle_shared_mem_bank. > > Is this comment to reduce the number of parameters passed? I guess it’s not for where we call In this series as well as the previous one you limit the number of arguments passed to quite a few functions. So naturally I would expect it to be done here as well. owner_dom_io is used only by handle_shared_mem_bank, so it makes more sense to move parsing to this function so that it is self-contained. ~Michal
Hi Michal, >>>> >>>> int __init process_shm(struct domain *d, struct kernel_info *kinfo, >>>> const struct dt_device_node *node) >>>> { >>>> @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, >>>> if ( dt_property_read_string(shm_node, "role", &role_str) == 0 ) >>>> owner_dom_io = false; >>> Looking at owner_dom_io, why don't you move parsing role and setting owner_dom_io accordingly to handle_shared_mem_bank()? >> >> I think I wanted to keep all dt_* functions on the same level inside process_shm, otherwise yes, I could >> pass down shm_node and do the reading of role_str in handle_shared_mem_bank, or I could derive >> owner_dom_io from role_str being passed or not, something like: >> >> role_str = NULL; >> dt_property_read_string(shm_node, "role", &role_str) >> >> [inside handle_shared_mem_bank]: >> If ( role_str ) >> owner_dom_io = false; >> >> And pass only role_str to handle_shared_mem_bank. >> >> Is this comment to reduce the number of parameters passed? I guess it’s not for where we call > In this series as well as the previous one you limit the number of arguments passed to quite a few functions. > So naturally I would expect it to be done here as well. owner_dom_io is used only by handle_shared_mem_bank, so it makes more sense to move parsing to this > function so that it is self-contained. Ok I will, just to be on the same page here, you mean having dt_property_read_string inside handle_shared_mem_bank? Or the above example would work for you as well? That one would have role_str passed instead of shm_node. Cheers, Luca
On 07/05/2024 16:15, Luca Fancellu wrote: > > > Hi Michal, > > >>>>> >>>>> int __init process_shm(struct domain *d, struct kernel_info *kinfo, >>>>> const struct dt_device_node *node) >>>>> { >>>>> @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, >>>>> if ( dt_property_read_string(shm_node, "role", &role_str) == 0 ) >>>>> owner_dom_io = false; >>>> Looking at owner_dom_io, why don't you move parsing role and setting owner_dom_io accordingly to handle_shared_mem_bank()? >>> >>> I think I wanted to keep all dt_* functions on the same level inside process_shm, otherwise yes, I could >>> pass down shm_node and do the reading of role_str in handle_shared_mem_bank, or I could derive >>> owner_dom_io from role_str being passed or not, something like: >>> >>> role_str = NULL; >>> dt_property_read_string(shm_node, "role", &role_str) >>> >>> [inside handle_shared_mem_bank]: >>> If ( role_str ) >>> owner_dom_io = false; >>> >>> And pass only role_str to handle_shared_mem_bank. >>> >>> Is this comment to reduce the number of parameters passed? I guess it’s not for where we call >> In this series as well as the previous one you limit the number of arguments passed to quite a few functions. >> So naturally I would expect it to be done here as well. owner_dom_io is used only by handle_shared_mem_bank, so it makes more sense to move parsing to this >> function so that it is self-contained. > > Ok I will, just to be on the same page here, you mean having dt_property_read_string inside handle_shared_mem_bank? > Or the above example would work for you as well? That one would have role_str passed instead of shm_node. I'm ok with the solution above to pass role_str. ~Michal
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index f6cf74e58a83..24e40495a481 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -185,6 +185,47 @@ append_shm_bank_to_domain(struct shared_meminfo *kinfo_shm_mem, paddr_t start, return 0; } +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, + bool owner_dom_io, + const char *role_str, + const struct membank *shm_bank) +{ + paddr_t pbase, psize; + int ret; + + BUG_ON(!shm_bank); + + pbase = shm_bank->start; + psize = shm_bank->size; + /* + * DOMID_IO is a fake domain and is not described in the Device-Tree. + * Therefore when the owner of the shared region is DOMID_IO, we will + * only find the borrowers. + */ + if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) || + (!owner_dom_io && strcmp(role_str, "owner") == 0) ) + { + /* + * We found the first borrower of the region, the owner was not + * specified, so they should be assigned to dom_io. + */ + ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase, shm_bank); + if ( ret ) + return ret; + } + + if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) ) + { + /* Set up P2M foreign mapping for borrower domain. */ + ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize), + _mfn(PFN_UP(pbase)), p2m_map_foreign_rw); + if ( ret ) + return ret; + } + + return 0; +} + int __init process_shm(struct domain *d, struct kernel_info *kinfo, const struct dt_device_node *node) { @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, if ( dt_property_read_string(shm_node, "role", &role_str) == 0 ) owner_dom_io = false; - /* - * DOMID_IO is a fake domain and is not described in the Device-Tree. - * Therefore when the owner of the shared region is DOMID_IO, we will - * only find the borrowers. - */ - if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) || - (!owner_dom_io && strcmp(role_str, "owner") == 0) ) - { - /* - * We found the first borrower of the region, the owner was not - * specified, so they should be assigned to dom_io. - */ - ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase, - boot_shm_bank); - if ( ret ) - return ret; - } - - if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) ) - { - /* Set up P2M foreign mapping for borrower domain. */ - ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize), - _mfn(PFN_UP(pbase)), p2m_map_foreign_rw); - if ( ret ) - return ret; - } + ret = handle_shared_mem_bank(d, gbase, owner_dom_io, role_str, + boot_shm_bank); + if ( ret ) + return ret; /* * Record static shared memory region info for later setting
Wrap the code and logic that is calling assign_shared_memory and map_regions_p2mt into a new function 'handle_shared_mem_bank', it will become useful later when the code will allow the user to don't pass the host physical address. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- xen/arch/arm/static-shmem.c | 71 +++++++++++++++++++++++-------------- 1 file changed, 45 insertions(+), 26 deletions(-)