Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vmh: fix config pasing issue #8772

Closed
wants to merge 1 commit into from
Closed

vmh: fix config pasing issue #8772

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 17 additions & 15 deletions zephyr/lib/regions_mm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,23 +69,24 @@ struct vmh_heap *vmh_init_heap(const struct vmh_heap_config *cfg,
goto fail;

/* If no config was specified we will use default one */
if (!cfg) {
if (!cfg)
vmh_get_default_heap_config(new_heap->virtual_region, &new_config);
cfg = &new_config;
}
else
memcpy_s(&new_config, sizeof(struct vmh_heap_config),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sizeof(new_config)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old code resulted in the following behavior:
cfg was read correctly only in the if context and reads done after that returned garbage.
I do not understand the origin of the issue but sticking to new_config which was created and zeroed in the function body seems to fix the issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you give more context in the commit message, it looks like we are changing an object from heap ptr to stack ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old code resulted in the following behavior: cfg was read correctly only in the if context and reads done after that returned garbage. I do not understand the origin of the issue but sticking to new_config which was created and zeroed in the function body seems to fix the issue.

@dabekjakub sorry, I have a substantial issue with fixing something that we don't understand, especially if that's our own open-source software, not some 3rd-party black box.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I agree but it is either this or the code not working. Which do we choose? This can be investigated further but this fixes heap creation.

Both ptr should be stack ptrs since cfg should be created runtime and not assigned space on the heap. At least that is an assumption I am making it is a client that will decide from where he will pass config.

cfg, sizeof(struct vmh_heap_config));

/* Check if configuration provided can fit into virtual regions memory
* and if it is physical page aligned
*/
size_t total_requested_size = 0;

for (i = 0; i < MAX_MEMORY_ALLOCATORS_COUNT; i++) {
if (cfg->block_bundles_table[i].block_size > 0) {
if (new_config.block_bundles_table[i].block_size > 0) {
/* Block sizes can only be powers of 2*/
if (!is_power_of_2(cfg->block_bundles_table[i].block_size))
if (!is_power_of_2(new_config.block_bundles_table[i].block_size))
goto fail;
total_requested_size += cfg->block_bundles_table[i].block_size *
cfg->block_bundles_table[i].number_of_blocks;
total_requested_size += new_config.block_bundles_table[i].block_size *
new_config.block_bundles_table[i].number_of_blocks;

if (total_requested_size > new_heap->virtual_region->size ||
total_requested_size % CONFIG_MM_DRV_PAGE_SIZE)
Expand All @@ -106,16 +107,16 @@ struct vmh_heap *vmh_init_heap(const struct vmh_heap_config *cfg,
*/
for (i = 0; i < MAX_MEMORY_ALLOCATORS_COUNT; i++) {
/* Only run for sizes > 0 */
if (!cfg->block_bundles_table[i].block_size)
if (!new_config.block_bundles_table[i].block_size)
continue;

/* bitarray_size is number of bit bundles that bittaray needs
* to represent memory blocks that's why we div round up number of blocks
* by number of bits in byte
*/
size_t bitarray_size = SOF_DIV_ROUND_UP(
SOF_DIV_ROUND_UP((uint64_t)cfg->block_bundles_table[i].number_of_blocks, 8),
sizeof(uint32_t));
SOF_DIV_ROUND_UP((uint64_t)new_config.block_bundles_table[i].number_of_blocks,
8), sizeof(uint32_t));
size_t bitfield_size = sizeof(uint32_t) * bitarray_size;

/* Need to create structs that are components of mem_blocks and constantly keep
Expand All @@ -132,8 +133,9 @@ struct vmh_heap *vmh_init_heap(const struct vmh_heap_config *cfg,
new_heap->physical_blocks_allocators[i] = new_allocator;

/* Fill allocators data based on config and virtual region data */
new_allocator->info.num_blocks = cfg->block_bundles_table[i].number_of_blocks;
new_allocator->info.blk_sz_shift = ilog2(cfg->block_bundles_table[i].block_size);
new_allocator->info.num_blocks = new_config.block_bundles_table[i].number_of_blocks;
new_allocator->info.blk_sz_shift = ilog2(
new_config.block_bundles_table[i].block_size);
new_allocator->buffer = (uint8_t *)new_heap->virtual_region->addr + offset;

/* Create bit array that is a part of mem_block kept as a ptr */
Expand All @@ -143,7 +145,7 @@ struct vmh_heap *vmh_init_heap(const struct vmh_heap_config *cfg,
if (!allocators_bitarray)
goto fail;

allocators_bitarray->num_bits = cfg->block_bundles_table[i].number_of_blocks;
allocators_bitarray->num_bits = new_config.block_bundles_table[i].number_of_blocks;
allocators_bitarray->num_bundles = bitarray_size;
new_allocator->bitmap = allocators_bitarray;

Expand Down Expand Up @@ -186,8 +188,8 @@ struct vmh_heap *vmh_init_heap(const struct vmh_heap_config *cfg,
/* Update offset, note that offset is CONFIG_MM_DRV_PAGE_SIZE aligned
* since every cfg size was checked against it earlier
*/
offset += cfg->block_bundles_table[i].number_of_blocks
* cfg->block_bundles_table[i].block_size;
offset += new_config.block_bundles_table[i].number_of_blocks
* new_config.block_bundles_table[i].block_size;
}

new_heap->allocating_continuously = allocating_continuously;
Expand Down
Loading