diff mbox series

[PULL,06/10] hw/loongarch: Refine fwcfg memory map

Message ID 20240523014637.614872-7-gaosong@loongson.cn (mailing list archive)
State New, archived
Headers show
Series [PULL,01/10] target/loongarch/kvm: Fix VM recovery from disk failures | expand

Commit Message

gaosong May 23, 2024, 1:46 a.m. UTC 
From: Bibo Mao <maobibo@loongson.cn>

Memory map table for fwcfg is used for UEFI BIOS, UEFI BIOS uses the first
entry from fwcfg memory map as the first memory HOB, the second memory HOB
will be used if the first memory HOB is used up.

Memory map table for fwcfg does not care about numa node, however in
generic the first memory HOB is part of numa node0, so that runtime
memory of UEFI which is allocated from the first memory HOB is located
at numa node0.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
Reviewed-by: Song Gao <gaosong@loongson.cn>
Message-Id: <20240515093927.3453674-4-maobibo@loongson.cn>
Signed-off-by: Song Gao <gaosong@loongson.cn>
---
 hw/loongarch/virt.c | 60 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 57 insertions(+), 3 deletions(-)

Comments

Peter Maydell June 7, 2024, 2:31 p.m. UTC | #1 
On Thu, 23 May 2024 at 02:48, Song Gao <gaosong@loongson.cn> wrote:
>
> From: Bibo Mao <maobibo@loongson.cn>
>
> Memory map table for fwcfg is used for UEFI BIOS, UEFI BIOS uses the first
> entry from fwcfg memory map as the first memory HOB, the second memory HOB
> will be used if the first memory HOB is used up.
>
> Memory map table for fwcfg does not care about numa node, however in
> generic the first memory HOB is part of numa node0, so that runtime
> memory of UEFI which is allocated from the first memory HOB is located
> at numa node0.
>
> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
> Reviewed-by: Song Gao <gaosong@loongson.cn>
> Message-Id: <20240515093927.3453674-4-maobibo@loongson.cn>
> Signed-off-by: Song Gao <gaosong@loongson.cn>

Hi; Coverity points out a possible issue with this code
(CID 1546441):

> +static void fw_cfg_add_memory(MachineState *ms)
> +{
> +    hwaddr base, size, ram_size, gap;
> +    int nb_numa_nodes, nodes;
> +    NodeInfo *numa_info;
> +
> +    ram_size = ms->ram_size;
> +    base = VIRT_LOWMEM_BASE;
> +    gap = VIRT_LOWMEM_SIZE;
> +    nodes = nb_numa_nodes = ms->numa_state->num_nodes;
> +    numa_info = ms->numa_state->nodes;
> +    if (!nodes) {
> +        nodes = 1;
> +    }
> +
> +    /* add fw_cfg memory map of node0 */
> +    if (nb_numa_nodes) {
> +        size = numa_info[0].node_mem;
> +    } else {
> +        size = ram_size;
> +    }
> +
> +    if (size >= gap) {
> +        memmap_add_entry(base, gap, 1);
> +        size -= gap;
> +        base = VIRT_HIGHMEM_BASE;
> +        gap = ram_size - VIRT_LOWMEM_SIZE;

In this if() statement we set 'gap'...

> +    }
> +
> +    if (size) {
> +        memmap_add_entry(base, size, 1);
> +        base += size;
> +    }
> +
> +    if (nodes < 2) {
> +        return;
> +    }
> +
> +    /* add fw_cfg memory map of other nodes */
> +    size = ram_size - numa_info[0].node_mem;
> +    gap  = VIRT_LOWMEM_BASE + VIRT_LOWMEM_SIZE;

...but then later here we unconditionally overwrite 'gap',
without ever using it in between, making the previous
assignment useless.

What was the intention here ?

thanks
-- PMM
Bibo Mao June 11, 2024, 1:57 a.m. UTC | #2 
On 2024/6/7 下午10:31, Peter Maydell wrote:
> On Thu, 23 May 2024 at 02:48, Song Gao <gaosong@loongson.cn> wrote:
>>
>> From: Bibo Mao <maobibo@loongson.cn>
>>
>> Memory map table for fwcfg is used for UEFI BIOS, UEFI BIOS uses the first
>> entry from fwcfg memory map as the first memory HOB, the second memory HOB
>> will be used if the first memory HOB is used up.
>>
>> Memory map table for fwcfg does not care about numa node, however in
>> generic the first memory HOB is part of numa node0, so that runtime
>> memory of UEFI which is allocated from the first memory HOB is located
>> at numa node0.
>>
>> Signed-off-by: Bibo Mao <maobibo@loongson.cn>
>> Reviewed-by: Song Gao <gaosong@loongson.cn>
>> Message-Id: <20240515093927.3453674-4-maobibo@loongson.cn>
>> Signed-off-by: Song Gao <gaosong@loongson.cn>
> 
> Hi; Coverity points out a possible issue with this code
> (CID 1546441):
> 
>> +static void fw_cfg_add_memory(MachineState *ms)
>> +{
>> +    hwaddr base, size, ram_size, gap;
>> +    int nb_numa_nodes, nodes;
>> +    NodeInfo *numa_info;
>> +
>> +    ram_size = ms->ram_size;
>> +    base = VIRT_LOWMEM_BASE;
>> +    gap = VIRT_LOWMEM_SIZE;
>> +    nodes = nb_numa_nodes = ms->numa_state->num_nodes;
>> +    numa_info = ms->numa_state->nodes;
>> +    if (!nodes) {
>> +        nodes = 1;
>> +    }
>> +
>> +    /* add fw_cfg memory map of node0 */
>> +    if (nb_numa_nodes) {
>> +        size = numa_info[0].node_mem;
>> +    } else {
>> +        size = ram_size;
>> +    }
>> +
>> +    if (size >= gap) {
>> +        memmap_add_entry(base, gap, 1);
>> +        size -= gap;
>> +        base = VIRT_HIGHMEM_BASE;
>> +        gap = ram_size - VIRT_LOWMEM_SIZE;
> 
> In this if() statement we set 'gap'...
> 
>> +    }
>> +
>> +    if (size) {
>> +        memmap_add_entry(base, size, 1);
>> +        base += size;
>> +    }
>> +
>> +    if (nodes < 2) {
>> +        return;
>> +    }
>> +
>> +    /* add fw_cfg memory map of other nodes */
>> +    size = ram_size - numa_info[0].node_mem;
>> +    gap  = VIRT_LOWMEM_BASE + VIRT_LOWMEM_SIZE;
> 
> ...but then later here we unconditionally overwrite 'gap',
> without ever using it in between, making the previous
> assignment useless.
> 
> What was the intention here ?
It is abuse about variable gap, sometimes it represents low memory size,
sometimes it represents the end address of low memory.

It can be removed at both placed, what is this patch?

--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -1054,7 +1054,6 @@ static void fw_cfg_add_memory(MachineState *ms)
          memmap_add_entry(base, gap, 1);
          size -= gap;
          base = VIRT_HIGHMEM_BASE;
-        gap = ram_size - VIRT_LOWMEM_SIZE;
      }

      if (size) {
@@ -1068,15 +1067,14 @@ static void fw_cfg_add_memory(MachineState *ms)

      /* add fw_cfg memory map of other nodes */
      size = ram_size - numa_info[0].node_mem;
-    gap  = VIRT_LOWMEM_BASE + VIRT_LOWMEM_SIZE;
-    if (base < gap && (base + size) > gap) {
+    if (numa_info[0].node_mem < gap && ram_size > gap) {
          /*
           * memory map for the maining nodes splited into two part
-         *   lowram:  [base, +(gap - base))
-         *   highram: [VIRT_HIGHMEM_BASE, +(size - (gap - base)))
+         * lowram:  [base, +(gap - numa_info[0].node_mem))
+         * highram: [VIRT_HIGHMEM_BASE, +(size - (gap - 
numa_info[0].node_mem)))
           */
-        memmap_add_entry(base, gap - base, 1);
-        size -= gap - base;
+        memmap_add_entry(base, gap - numa_info[0].node_mem, 1);
+        size -= gap - numa_info[0].node_mem;
          base = VIRT_HIGHMEM_BASE;
      }

Regards
Bibo Mao


> 
> thanks
> -- PMM
>
diff mbox series

Patch

diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
index 850729202f..449050cba5 100644
--- a/hw/loongarch/virt.c
+++ b/hw/loongarch/virt.c
@@ -918,6 +918,62 @@  static const MemoryRegionOps virt_iocsr_misc_ops = {
     },
 };
 
+static void fw_cfg_add_memory(MachineState *ms)
+{
+    hwaddr base, size, ram_size, gap;
+    int nb_numa_nodes, nodes;
+    NodeInfo *numa_info;
+
+    ram_size = ms->ram_size;
+    base = VIRT_LOWMEM_BASE;
+    gap = VIRT_LOWMEM_SIZE;
+    nodes = nb_numa_nodes = ms->numa_state->num_nodes;
+    numa_info = ms->numa_state->nodes;
+    if (!nodes) {
+        nodes = 1;
+    }
+
+    /* add fw_cfg memory map of node0 */
+    if (nb_numa_nodes) {
+        size = numa_info[0].node_mem;
+    } else {
+        size = ram_size;
+    }
+
+    if (size >= gap) {
+        memmap_add_entry(base, gap, 1);
+        size -= gap;
+        base = VIRT_HIGHMEM_BASE;
+        gap = ram_size - VIRT_LOWMEM_SIZE;
+    }
+
+    if (size) {
+        memmap_add_entry(base, size, 1);
+        base += size;
+    }
+
+    if (nodes < 2) {
+        return;
+    }
+
+    /* add fw_cfg memory map of other nodes */
+    size = ram_size - numa_info[0].node_mem;
+    gap  = VIRT_LOWMEM_BASE + VIRT_LOWMEM_SIZE;
+    if (base < gap && (base + size) > gap) {
+        /*
+         * memory map for the maining nodes splited into two part
+         *   lowram:  [base, +(gap - base))
+         *   highram: [VIRT_HIGHMEM_BASE, +(size - (gap - base)))
+         */
+        memmap_add_entry(base, gap - base, 1);
+        size -= gap - base;
+        base = VIRT_HIGHMEM_BASE;
+    }
+
+   if (size)
+        memmap_add_entry(base, size, 1);
+}
+
 static void virt_init(MachineState *machine)
 {
     LoongArchCPU *lacpu;
@@ -964,9 +1020,9 @@  static void virt_init(MachineState *machine)
     }
     fdt_add_cpu_nodes(lvms);
     fdt_add_memory_nodes(machine);
+    fw_cfg_add_memory(machine);
 
     /* Node0 memory */
-    memmap_add_entry(VIRT_LOWMEM_BASE, VIRT_LOWMEM_SIZE, 1);
     memory_region_init_alias(&lvms->lowmem, NULL, "loongarch.node0.lowram",
                              machine->ram, offset, VIRT_LOWMEM_SIZE);
     memory_region_add_subregion(address_space_mem, phyAddr, &lvms->lowmem);
@@ -979,7 +1035,6 @@  static void virt_init(MachineState *machine)
         highram_size = ram_size - VIRT_LOWMEM_SIZE;
     }
     phyAddr = VIRT_HIGHMEM_BASE;
-    memmap_add_entry(phyAddr, highram_size, 1);
     memory_region_init_alias(&lvms->highmem, NULL, "loongarch.node0.highram",
                               machine->ram, offset, highram_size);
     memory_region_add_subregion(address_space_mem, phyAddr, &lvms->highmem);
@@ -994,7 +1049,6 @@  static void virt_init(MachineState *machine)
         memory_region_init_alias(nodemem, NULL, ramName, machine->ram,
                                  offset,  numa_info[i].node_mem);
         memory_region_add_subregion(address_space_mem, phyAddr, nodemem);
-        memmap_add_entry(phyAddr, numa_info[i].node_mem, 1);
         offset += numa_info[i].node_mem;
         phyAddr += numa_info[i].node_mem;
     }