On 20/05/2024 14:44, Luca Fancellu wrote:
>
>
> 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?
I'd be ok with meminfo provided you add a reasoning behind this being meminfo
and not shared_meminfo.
>
> Because if we take struct shared_meminfo, we would waste mem for its ‘extra’
> member.
Would it result in a smaller footprint overall?
>
>>>
>>> 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 ?
Why would you want user to know the machine physical address? It's a heap
address.
>
>
>>>
>>> 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?
User can request static shmem region of as little as 1 page.
>
>>>
>>> + 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
>
>
~Michal
