[PATCH v6 05/10] i386/pc: factor out cxl range end to helper

Joao Martins posted 10 patches 3 years, 7 months ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <ani@anisinha.ca>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>
There is a newer version of this series
[PATCH v6 05/10] i386/pc: factor out cxl range end to helper
Posted by Joao Martins 3 years, 7 months ago
Move calculation of CXL memory region end to separate helper in
preparation to allow pc_pci_hole64_start() to be called before
any mrs are initialized.

Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 hw/i386/pc.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 6c7c49ca5a32..0abbf81841a9 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -825,6 +825,25 @@ static hwaddr pc_above_4g_end(PCMachineState *pcms)
     return x86ms->above_4g_mem_start + x86ms->above_4g_mem_size;
 }
 
+static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
+{
+    uint64_t start = 0;
+
+    if (pcms->cxl_devices_state.host_mr.addr) {
+        start = pcms->cxl_devices_state.host_mr.addr +
+            memory_region_size(&pcms->cxl_devices_state.host_mr);
+        if (pcms->cxl_devices_state.fixed_windows) {
+            GList *it;
+            for (it = pcms->cxl_devices_state.fixed_windows; it; it = it->next) {
+                CXLFixedWindow *fw = it->data;
+                start = fw->mr.addr + memory_region_size(&fw->mr);
+            }
+        }
+    }
+
+    return start;
+}
+
 void pc_memory_init(PCMachineState *pcms,
                     MemoryRegion *system_memory,
                     MemoryRegion *rom_memory,
@@ -1022,16 +1041,8 @@ uint64_t pc_pci_hole64_start(void)
     MachineState *ms = MACHINE(pcms);
     uint64_t hole64_start = 0;
 
-    if (pcms->cxl_devices_state.host_mr.addr) {
-        hole64_start = pcms->cxl_devices_state.host_mr.addr +
-            memory_region_size(&pcms->cxl_devices_state.host_mr);
-        if (pcms->cxl_devices_state.fixed_windows) {
-            GList *it;
-            for (it = pcms->cxl_devices_state.fixed_windows; it; it = it->next) {
-                CXLFixedWindow *fw = it->data;
-                hole64_start = fw->mr.addr + memory_region_size(&fw->mr);
-            }
-        }
+    if (pcms->cxl_devices_state.is_enabled) {
+        hole64_start = pc_get_cxl_range_end(pcms);
     } else if (pcmc->has_reserved_memory && ms->device_memory->base) {
         hole64_start = ms->device_memory->base;
         if (!pcmc->broken_reserved_end) {
-- 
2.17.2
Re: [PATCH v6 05/10] i386/pc: factor out cxl range end to helper
Posted by Igor Mammedov 3 years, 7 months ago
On Fri,  1 Jul 2022 17:10:09 +0100
Joao Martins <joao.m.martins@oracle.com> wrote:

> Move calculation of CXL memory region end to separate helper in
> preparation to allow pc_pci_hole64_start() to be called before
> any mrs are initialized.
s/mrs/memory regions/



> 
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  hw/i386/pc.c | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 6c7c49ca5a32..0abbf81841a9 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -825,6 +825,25 @@ static hwaddr pc_above_4g_end(PCMachineState *pcms)
>      return x86ms->above_4g_mem_start + x86ms->above_4g_mem_size;
>  }
>  
> +static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
> +{
> +    uint64_t start = 0;
> +
> +    if (pcms->cxl_devices_state.host_mr.addr) {
> +        start = pcms->cxl_devices_state.host_mr.addr +
> +            memory_region_size(&pcms->cxl_devices_state.host_mr);
> +        if (pcms->cxl_devices_state.fixed_windows) {
> +            GList *it;
> +            for (it = pcms->cxl_devices_state.fixed_windows; it; it = it->next) {
> +                CXLFixedWindow *fw = it->data;
> +                start = fw->mr.addr + memory_region_size(&fw->mr);
> +            }

this block deals with 'initialized' memory regions,
so claim 'before any mrs are initialized' in commit message is
confusing at least or outright wrong.

> +        }
> +    }
> +
> +    return start;
> +}
> +
>  void pc_memory_init(PCMachineState *pcms,
>                      MemoryRegion *system_memory,
>                      MemoryRegion *rom_memory,
> @@ -1022,16 +1041,8 @@ uint64_t pc_pci_hole64_start(void)
>      MachineState *ms = MACHINE(pcms);
>      uint64_t hole64_start = 0;
>  
> -    if (pcms->cxl_devices_state.host_mr.addr) {
> -        hole64_start = pcms->cxl_devices_state.host_mr.addr +
> -            memory_region_size(&pcms->cxl_devices_state.host_mr);
> -        if (pcms->cxl_devices_state.fixed_windows) {
> -            GList *it;
> -            for (it = pcms->cxl_devices_state.fixed_windows; it; it = it->next) {
> -                CXLFixedWindow *fw = it->data;
> -                hole64_start = fw->mr.addr + memory_region_size(&fw->mr);
> -            }
> -        }
> +    if (pcms->cxl_devices_state.is_enabled) {
> +        hole64_start = pc_get_cxl_range_end(pcms);
>      } else if (pcmc->has_reserved_memory && ms->device_memory->base) {
>          hole64_start = ms->device_memory->base;
>          if (!pcmc->broken_reserved_end) {
Re: [PATCH v6 05/10] i386/pc: factor out cxl range end to helper
Posted by Joao Martins 3 years, 7 months ago
On 7/7/22 13:57, Igor Mammedov wrote:
> On Fri,  1 Jul 2022 17:10:09 +0100
> Joao Martins <joao.m.martins@oracle.com> wrote:
> 
>> Move calculation of CXL memory region end to separate helper in
>> preparation to allow pc_pci_hole64_start() to be called before
>> any mrs are initialized.
> s/mrs/memory regions/
> 
Will fix.

> 
> 
>>
>> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>>  hw/i386/pc.c | 31 +++++++++++++++++++++----------
>>  1 file changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 6c7c49ca5a32..0abbf81841a9 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -825,6 +825,25 @@ static hwaddr pc_above_4g_end(PCMachineState *pcms)
>>      return x86ms->above_4g_mem_start + x86ms->above_4g_mem_size;
>>  }
>>  
>> +static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
>> +{
>> +    uint64_t start = 0;
>> +
>> +    if (pcms->cxl_devices_state.host_mr.addr) {
>> +        start = pcms->cxl_devices_state.host_mr.addr +
>> +            memory_region_size(&pcms->cxl_devices_state.host_mr);
>> +        if (pcms->cxl_devices_state.fixed_windows) {
>> +            GList *it;
>> +            for (it = pcms->cxl_devices_state.fixed_windows; it; it = it->next) {
>> +                CXLFixedWindow *fw = it->data;
>> +                start = fw->mr.addr + memory_region_size(&fw->mr);
>> +            }
> 
> this block deals with 'initialized' memory regions,
> so claim 'before any mrs are initialized' in commit message is
> confusing at least or outright wrong.
> 

But the commit message is pretty clear on its purpose.

"Move calculation of CXL memory region end to separate helper"

Then it justifies why we are adding.. that is in preparation
for a patch that will come after. I am not implying at all
that I am dealing with unitiliazed MRs in this patch.

Anyhow, I can drop the part after 'in preparation' or just drop the
mention to unitialized MRs if confuses folks.

>> +        }
>> +    }
>> +
>> +    return start;
>> +}
>> +
>>  void pc_memory_init(PCMachineState *pcms,
>>                      MemoryRegion *system_memory,
>>                      MemoryRegion *rom_memory,
>> @@ -1022,16 +1041,8 @@ uint64_t pc_pci_hole64_start(void)
>>      MachineState *ms = MACHINE(pcms);
>>      uint64_t hole64_start = 0;
>>  
>> -    if (pcms->cxl_devices_state.host_mr.addr) {
>> -        hole64_start = pcms->cxl_devices_state.host_mr.addr +
>> -            memory_region_size(&pcms->cxl_devices_state.host_mr);
>> -        if (pcms->cxl_devices_state.fixed_windows) {
>> -            GList *it;
>> -            for (it = pcms->cxl_devices_state.fixed_windows; it; it = it->next) {
>> -                CXLFixedWindow *fw = it->data;
>> -                hole64_start = fw->mr.addr + memory_region_size(&fw->mr);
>> -            }
>> -        }
>> +    if (pcms->cxl_devices_state.is_enabled) {
>> +        hole64_start = pc_get_cxl_range_end(pcms);
>>      } else if (pcmc->has_reserved_memory && ms->device_memory->base) {
>>          hole64_start = ms->device_memory->base;
>>          if (!pcmc->broken_reserved_end) {
>