Hi Michal,
> On 20 May 2024, at 12:16, Michal Orzel <michal.or...@amd.com> wrote:
>
> Hi Luca,
>
> On 15/05/2024 16:26, Luca Fancellu wrote:
>>
>>
>> This commit implements the logic to have the static shared memory banks
>> from the Xen heap instead of having the host physical address passed from
>> the user.
>>
>> When the host physical address is not supplied, the physical memory is
>> taken from the Xen heap using allocate_domheap_memory, the allocation
>> needs to occur at the first handled DT node and the allocated banks
>> need to be saved somewhere, so introduce the 'shm_heap_banks' static
>> global variable of type 'struct meminfo' that will hold the banks
>> allocated from the heap, its field .shmem_extra will be used to point
>> to the bootinfo shared memory banks .shmem_extra space, so that there
>> is not further allocation of memory and every bank in shm_heap_banks
>> can be safely identified by the shm_id to reconstruct its traceability
>> and if it was allocated or not.
> NIT for the future: it's better to split 10 lines long sentence into multiple
> ones.
> Otherwise it reads difficult.
I’ll do,
>>
>> xen/arch/arm/static-shmem.c | 186 ++++++++++++++++++++++++++++++------
>> 1 file changed, 155 insertions(+), 31 deletions(-)
>>
>> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
>> index ddaacbc77740..9c3a83042d8b 100644
>> --- a/xen/arch/arm/static-shmem.c
>> +++ b/xen/arch/arm/static-shmem.c
>> @@ -9,6 +9,22 @@
>> #include <asm/static-memory.h>
>> #include <asm/static-shmem.h>
>>
>> +typedef struct {
>> + struct domain *d;
>> + paddr_t gbase;
>> + const char *role_str;
> You could swap role_str and gbase to avoid a 4B hole on arm32
Sure I will,
>
>> + struct shmem_membank_extra *bank_extra_info;
>> +} alloc_heap_pages_cb_extra;
>> +
>> +static struct meminfo __initdata shm_heap_banks = {
>> + .common.max_banks = NR_MEM_BANKS
> Do we expect that many banks?
Not really, but I was trying to don’t introduce another type, do you think it’s
better instead to
introduce a new type only here, with a lower amount of banks?
Because if we take struct shared_meminfo, we would waste mem for its ‘extra’
member.
>>
>> static int __init assign_shared_memory(struct domain *d, paddr_t gbase,
>> + bool bank_from_heap,
>> const struct membank *shm_bank)
>> {
>> mfn_t smfn;
>> @@ -109,10 +138,7 @@ static int __init assign_shared_memory(struct domain
>> *d, paddr_t gbase,
>> psize = shm_bank->size;
>> nr_borrowers = shm_bank->shmem_extra->nr_shm_borrowers;
>>
>> - printk("%pd: allocate static shared memory BANK
>> %#"PRIpaddr"-%#"PRIpaddr".\n",
>> - d, pbase, pbase + psize);
>> -
>> - smfn = acquire_shared_memory_bank(d, pbase, psize);
>> + smfn = acquire_shared_memory_bank(d, pbase, psize, bank_from_heap);
>> if ( mfn_eq(smfn, INVALID_MFN) )
>> return -EINVAL;
>>
>> @@ -183,6 +209,7 @@ append_shm_bank_to_domain(struct kernel_info *kinfo,
>> paddr_t start,
>>
>> static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase,
>> const char *role_str,
>> + bool bank_from_heap,
>> const struct membank *shm_bank)
>> {
>> bool owner_dom_io = true;
>> @@ -192,6 +219,9 @@ static int __init handle_shared_mem_bank(struct domain
>> *d, paddr_t gbase,
>> pbase = shm_bank->start;
>> psize = shm_bank->size;
>>
>> + printk("%pd: SHMEM map from %s: mphys 0x%"PRIpaddr" -> gphys
>> 0x%"PRIpaddr", size 0x%"PRIpaddr"\n",
>> + d, bank_from_heap ? "Xen heap" : "Host", pbase, gbase, psize);
> This looks more like a debug print since I don't expect user to want to see a
> machine address.
printk(XENLOG_DEBUG ?
>>
>> int __init process_shm(struct domain *d, struct kernel_info *kinfo,
>> const struct dt_device_node *node)
>> {
>> @@ -265,37 +329,97 @@ int __init process_shm(struct domain *d, struct
>> kernel_info *kinfo,
>> pbase = boot_shm_bank->start;
>> psize = boot_shm_bank->size;
>>
>> - if ( INVALID_PADDR == pbase )
>> - {
>> - printk("%pd: host physical address must be chosen by users at
>> the moment", d);
>> - return -EINVAL;
>> - }
>> + /* "role" property is optional */
>> + dt_property_read_string(shm_node, "role", &role_str);
> This function returns a value but you seem to ignore it
Sure, I’ll handle that
>>
>> - ret = handle_shared_mem_bank(d, gbase, role_str, boot_shm_bank);
>> - if ( ret )
>> - return ret;
>> + if ( !alloc_bank )
>> + {
>> + alloc_heap_pages_cb_extra cb_arg = { d, gbase, role_str,
>> + boot_shm_bank->shmem_extra };
>> +
>> + /* shm_id identified bank is not yet allocated */
>> + if ( !allocate_domheap_memory(NULL, psize,
>> save_map_heap_pages,
>> + &cb_arg) )
>> + {
>> + printk(XENLOG_ERR
>> + "Failed to allocate (%"PRIpaddr"MB) pages as
>> static shared memory from heap\n",
> Why limiting to MB?
I think I used it from domain_build.c, do you think it’s better to limit it on
KB instead?
>>
>> + for ( ; alloc_bank < end_bank; alloc_bank++ )
>> + {
>> + if ( strncmp(shm_id, alloc_bank->shmem_extra->shm_id,
>> + MAX_SHM_ID_LENGTH) != 0 )
> shm_id has been already validated above, hence no need for a safe version of
> strcmp
>
I always try to use the safe version, even when redundant, I feel that if
someone is copying part of the code,
at least it would copy a safe version. Anyway I will change it if it’s not
desirable.
Cheers,
Luca
