[drivers][ofw] Fixed fdt_scan_memory() memory adjustment bug #8853

liYony · 2024-04-24T09:22:06Z

拉取/合并请求描述：(PR description)

[

在调用fdt_scan_memory对设备树的内存进行解析后，会根据保留内存对整个内存重新做出一些调整。如下图(添加了一些打印信息)：

如图所示，调整后出现了一个起始地址等于结束地址的一个无效mem_region。

[I/rtdm.ofw]   (NULL)           [0x00000000441581a0, 0x00000000441581a0]

排查发现是对case 0情况的判断条件不妥。

/*
             * case 0:                      case 1:
             *  +------------------+             +----------+
             *  |      memory      |             |  memory  |
             *  +---+----------+---+         +---+----------+---+
             *      | reserved |             |     reserved     |
             *      +----------+             +---+----------+---+
             *
             * case 2:                      case 3:
             *  +------------------+                +------------------+
             *  |      memory      |                |      memory      |
             *  +--------------+---+------+  +------+---+--------------+
             *                 | reserved |  | reserved |
             *                 +----------+  +----------+
             */

如果出现了一个res_region->start = region->start 和 res_region->end < region->end 的情况。

+------------------+
|      memory      |
+----------+-------+
| reserved |    
+----------+

按照目前的代码，是符合case 0 情况的，那么这里调整后会出现两个mem_region，其中一个的起始地址等于结束地址。显然这是不合理的，对于这种情况，直接走case 3 情况处理即可。而且这里这个mem_region将会在下一次遍历为mem_region->name赋值为RT_NULL，那这样做其实也是会影响外层for循环的执行的(for (no = 0; region->name; ++region){})。

这里我做了一个验证，我故意多commit了几个内存保留区域（基于qemu-virt64-aarch64）。

结果，并没有为0x46000000~0x46001000这段空间进行调整，肯定是不符合要求的。

按照我目前pr的修改方式，可以解决这个问题。

]

当前拉取/合并请求的状态 Intent for your PR

必须选择一项 Choose one (Mandatory):

本拉取/合并请求是一个草稿版本 This PR is for a code-review and is intended to get feedback
本拉取/合并请求是一个成熟版本 This PR is mature, and ready to be integrated into the repo

代码质量 Code Quality：

我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:

已经仔细查看过代码改动的对比 Already check the difference between PR and old code
代码风格正确，包括缩进空格，命名及其他风格 Style guide is adhered to, including spacing, naming and other styles
没有垃圾代码，代码尽量精简，不包含#if 0代码，不包含已经被注释了的代码 All redundant code is removed and cleaned up
所有变更均有原因及合理的，并且不会影响到其他软件组件代码或BSP All modifications are justified and not affect other components or BSP
对难懂代码均提供对应的注释 I've commented appropriately where code is tricky
代码是高质量的 Code in this PR is of high quality
已经使用formatting 等源码格式化工具确保格式符合RT-Thread代码规范 This PR complies with RT-Thread code specification

polarvid · 2024-04-26T11:06:42Z

其实这个 case 逻辑可以化简一点的. 参考

rt-thread/components/mm/mm_aspace.c

Lines 983 to 1000 in 75f4172

    
           static int _remove_overlapped_varea(rt_varea_t existed, char *unmap_start, rt_size_t unmap_len) 
        
           { 
        
               int error; 
        
               char *ex_start = existed->start; 
        
               char *ex_end = ex_start + existed->size; 
        
               char *unmap_end = unmap_start + unmap_len; 
        
               if (ex_start < unmap_start) 
        
               { 
        
                   if (ex_end > unmap_end) 
        
                       error = _split_varea(existed, ex_end, unmap_start, unmap_end, unmap_len); 
        
                   else 
        
                       error = _shrink_varea(existed, ex_start, unmap_start - ex_start); 
        
               } 
        
               else if (ex_end > unmap_end) 
        
                   error = _shrink_varea(existed, unmap_end, ex_end - unmap_end); 
        
               else 
        
               {

之后大概就是这样

            if (res_region->start > region->start)
            {
                if (res_region->end < region->end)
                {
                    /* case 0 */
                }
            }
            else
            {
                if (res_region->end >= region->end)
                {
                    if (res_region->start <= region->end)
                    {
                        /* case 2 */
                    }
                    else
                    {
                        /* case 1 */
                    }
                }

                if (res_region->end >= region->start)
                {
                    /* case 3 */
                }
            }

liYony · 2024-04-26T11:09:35Z

其实这个 case 逻辑可以化简一点的. 参考

rt-thread/components/mm/mm_aspace.c

Lines 983 to 1000 in 75f4172

    
           static int _remove_overlapped_varea(rt_varea_t existed, char *unmap_start, rt_size_t unmap_len) 
        
           { 
        
               int error; 
        
               char *ex_start = existed->start; 
        
               char *ex_end = ex_start + existed->size; 
        
               char *unmap_end = unmap_start + unmap_len; 
        
               if (ex_start < unmap_start) 
        
               { 
        
                   if (ex_end > unmap_end) 
        
                       error = _split_varea(existed, ex_end, unmap_start, unmap_end, unmap_len); 
        
                   else 
        
                       error = _shrink_varea(existed, ex_start, unmap_start - ex_start); 
        
               } 
        
               else if (ex_end > unmap_end) 
        
                   error = _shrink_varea(existed, unmap_end, ex_end - unmap_end); 
        
               else 
        
               {

之后大概就是这样

            if (res_region->start > region->start)
            {
                if (res_region->end < region->end)
                {
                    /* case 0 */
                }
            }
            else
            {
                if (res_region->end >= region->end)
                {
                    if (res_region->start <= region->end)
                    {
                        /* case 2 */
                    }
                    else
                    {
                        /* case 1 */
                    }
                }

                if (res_region->end >= region->start)
                {
                    /* case 3 */
                }
            }

OK 感谢建议，我修改一下

polarvid · 2024-04-26T11:25:19Z

其实这个 case 逻辑可以化简一点的. 参考

rt-thread/components/mm/mm_aspace.c

Lines 983 to 1000 in 75f4172

static int _remove_overlapped_varea(rt_varea_t existed, char *unmap_start, rt_size_t unmap_len)

{

int error;

char *ex_start = existed->start;

char *ex_end = ex_start + existed->size;

char *unmap_end = unmap_start + unmap_len;

if (ex_start < unmap_start)

{

if (ex_end > unmap_end)

error = _split_varea(existed, ex_end, unmap_start, unmap_end, unmap_len);

else

error = _shrink_varea(existed, ex_start, unmap_start - ex_start);

}

else if (ex_end > unmap_end)

error = _shrink_varea(existed, unmap_end, ex_end - unmap_end);

else

{

之后大概就是这样
            if (res_region->start > region->start)
            {
                if (res_region->end < region->end)
                {
                    /* case 0 */
                }
            }
            else
            {
                if (res_region->end >= region->end)
                {
                    if (res_region->start <= region->end)
                    {
                        /* case 2 */
                    }
                    else
                    {
                        /* case 1 */
                    }
                }

                if (res_region->end >= region->start)
                {
                    /* case 3 */
                }
            }
OK 感谢建议，我修改一下

你也别完全用这个，我没细看的，哈哈哈。

            if (res_region->start > region->start)
            {
                if (res_region->end < region->end)
                {
                    /* case 0 */
                }
            }
            else /* if res_region->start <= region->start */
            {
                if (res_region->end >= region->end)
                {
                    /* case 1 */
                }

                if (res_region->end >= region->start)
                {
                    /* case 3 */
                }
            }

            if (res_region->start <= region->end && res_region->end >= region->end)
            {
                /* case 2 */
            }

polarvid · 2024-04-26T11:27:35Z

可以用这个改一个 utest 跑一下 examples/utest/testcases/mm/mm_memblock_tc.c

liYony · 2024-04-26T12:44:42Z

重新修改了一下内存调整的逻辑，我这边以qemu-virt64-aarch64做了一下测试：

效果完美！测试通过！

liYony · 2024-04-26T12:49:51Z

可以用这个改一个 utest 跑一下 examples/utest/testcases/mm/mm_memblock_tc.c

utest我还真没用过

Fixed fdt memory adjustment bug

59fba5f

mysterywolf closed this Apr 24, 2024

mysterywolf reopened this Apr 24, 2024

BernardXiong requested a review from polarvid April 25, 2024 11:30

polarvid approved these changes Apr 26, 2024

View reviewed changes

Modify the memory adjustment logic

9b28efc

polarvid approved these changes Apr 26, 2024

View reviewed changes

BernardXiong approved these changes Apr 27, 2024

View reviewed changes

BernardXiong merged commit d30c8b3 into RT-Thread:master Apr 27, 2024
41 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[drivers][ofw] Fixed fdt_scan_memory() memory adjustment bug #8853

[drivers][ofw] Fixed fdt_scan_memory() memory adjustment bug #8853

liYony commented Apr 24, 2024 •

edited

Loading

polarvid commented Apr 26, 2024

liYony commented Apr 26, 2024

polarvid commented Apr 26, 2024

polarvid commented Apr 26, 2024

liYony commented Apr 26, 2024

liYony commented Apr 26, 2024

[drivers][ofw] Fixed fdt_scan_memory() memory adjustment bug #8853

[drivers][ofw] Fixed fdt_scan_memory() memory adjustment bug #8853

Conversation

liYony commented Apr 24, 2024 • edited Loading

拉取/合并请求描述：(PR description)

当前拉取/合并请求的状态 Intent for your PR

代码质量 Code Quality：

polarvid commented Apr 26, 2024

liYony commented Apr 26, 2024

polarvid commented Apr 26, 2024

polarvid commented Apr 26, 2024

liYony commented Apr 26, 2024

liYony commented Apr 26, 2024

liYony commented Apr 24, 2024 •

edited

Loading