[PATCH v6 06/10] i386/pc: factor out cxl range start 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 06/10] i386/pc: factor out cxl range start to helper
Posted by Joao Martins 3 years, 7 months ago
Factor out the calculation of the base address of the MR. It will be
used later on for the cxl range end counterpart calculation and as
well in pc_memory_init() CXL mr initialization, thus avoiding
duplication.

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

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0abbf81841a9..8655cc3b8894 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -825,6 +825,24 @@ 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_start(PCMachineState *pcms)
+{
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+    MachineState *machine = MACHINE(pcms);
+    hwaddr cxl_base;
+
+    if (pcmc->has_reserved_memory && machine->device_memory->base) {
+        cxl_base = machine->device_memory->base;
+        if (!pcmc->broken_reserved_end) {
+            cxl_base += memory_region_size(&machine->device_memory->mr);
+        }
+    } else {
+        cxl_base = pc_above_4g_end(pcms);
+    }
+
+    return cxl_base;
+}
+
 static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
 {
     uint64_t start = 0;
@@ -946,15 +964,7 @@ void pc_memory_init(PCMachineState *pcms,
         MemoryRegion *mr = &pcms->cxl_devices_state.host_mr;
         hwaddr cxl_size = MiB;
 
-        if (pcmc->has_reserved_memory && machine->device_memory->base) {
-            cxl_base = machine->device_memory->base;
-            if (!pcmc->broken_reserved_end) {
-                cxl_base += memory_region_size(&machine->device_memory->mr);
-            }
-        } else {
-            cxl_base = pc_above_4g_end(pcms);
-        }
-
+        cxl_base = pc_get_cxl_range_start(pcms);
         e820_add_entry(cxl_base, cxl_size, E820_RESERVED);
         memory_region_init(mr, OBJECT(machine), "cxl_host_reg", cxl_size);
         memory_region_add_subregion(system_memory, cxl_base, mr);
-- 
2.17.2
Re: [PATCH v6 06/10] i386/pc: factor out cxl range start to helper
Posted by Igor Mammedov 3 years, 7 months ago
On Fri,  1 Jul 2022 17:10:10 +0100
Joao Martins <joao.m.martins@oracle.com> wrote:

> Factor out the calculation of the base address of the MR. It will be
> used later on for the cxl range end counterpart calculation and as
> well in pc_memory_init() CXL mr initialization, thus avoiding
> duplication.
> 
> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>

needs to be rebased on top of 


[PATCH 2/3] hw/i386/pc: Always place CXL Memory Regions after device_memory

> ---
>  hw/i386/pc.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 0abbf81841a9..8655cc3b8894 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -825,6 +825,24 @@ 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_start(PCMachineState *pcms)
> +{
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +    MachineState *machine = MACHINE(pcms);
> +    hwaddr cxl_base;
> +
> +    if (pcmc->has_reserved_memory && machine->device_memory->base) {
> +        cxl_base = machine->device_memory->base;
> +        if (!pcmc->broken_reserved_end) {
> +            cxl_base += memory_region_size(&machine->device_memory->mr);
> +        }
> +    } else {
> +        cxl_base = pc_above_4g_end(pcms);
> +    }
> +
> +    return cxl_base;
> +}
> +
>  static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
>  {
>      uint64_t start = 0;
> @@ -946,15 +964,7 @@ void pc_memory_init(PCMachineState *pcms,
>          MemoryRegion *mr = &pcms->cxl_devices_state.host_mr;
>          hwaddr cxl_size = MiB;
>  
> -        if (pcmc->has_reserved_memory && machine->device_memory->base) {
> -            cxl_base = machine->device_memory->base;
> -            if (!pcmc->broken_reserved_end) {
> -                cxl_base += memory_region_size(&machine->device_memory->mr);
> -            }
> -        } else {
> -            cxl_base = pc_above_4g_end(pcms);
> -        }
> -
> +        cxl_base = pc_get_cxl_range_start(pcms);
>          e820_add_entry(cxl_base, cxl_size, E820_RESERVED);
>          memory_region_init(mr, OBJECT(machine), "cxl_host_reg", cxl_size);
>          memory_region_add_subregion(system_memory, cxl_base, mr);
Re: [PATCH v6 06/10] i386/pc: factor out cxl range start to helper
Posted by Joao Martins 3 years, 7 months ago

On 7/7/22 14:00, Igor Mammedov wrote:
> On Fri,  1 Jul 2022 17:10:10 +0100
> Joao Martins <joao.m.martins@oracle.com> wrote:
> 
>> Factor out the calculation of the base address of the MR. It will be
>> used later on for the cxl range end counterpart calculation and as
>> well in pc_memory_init() CXL mr initialization, thus avoiding
>> duplication.
>>
>> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> 
> needs to be rebased on top of 
> 
> 
> [PATCH 2/3] hw/i386/pc: Always place CXL Memory Regions after device_memory
> 
Is Michael merging these or should I just respin v7 with the assumption
that these patches are there?

I can't see anything in his tree yet.

>> ---
>>  hw/i386/pc.c | 28 +++++++++++++++++++---------
>>  1 file changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 0abbf81841a9..8655cc3b8894 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -825,6 +825,24 @@ 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_start(PCMachineState *pcms)
>> +{
>> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>> +    MachineState *machine = MACHINE(pcms);
>> +    hwaddr cxl_base;
>> +
>> +    if (pcmc->has_reserved_memory && machine->device_memory->base) {
>> +        cxl_base = machine->device_memory->base;
>> +        if (!pcmc->broken_reserved_end) {
>> +            cxl_base += memory_region_size(&machine->device_memory->mr);
>> +        }
>> +    } else {
>> +        cxl_base = pc_above_4g_end(pcms);
>> +    }
>> +
>> +    return cxl_base;
>> +}
>> +
>>  static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
>>  {
>>      uint64_t start = 0;
>> @@ -946,15 +964,7 @@ void pc_memory_init(PCMachineState *pcms,
>>          MemoryRegion *mr = &pcms->cxl_devices_state.host_mr;
>>          hwaddr cxl_size = MiB;
>>  
>> -        if (pcmc->has_reserved_memory && machine->device_memory->base) {
>> -            cxl_base = machine->device_memory->base;
>> -            if (!pcmc->broken_reserved_end) {
>> -                cxl_base += memory_region_size(&machine->device_memory->mr);
>> -            }
>> -        } else {
>> -            cxl_base = pc_above_4g_end(pcms);
>> -        }
>> -
>> +        cxl_base = pc_get_cxl_range_start(pcms);
>>          e820_add_entry(cxl_base, cxl_size, E820_RESERVED);
>>          memory_region_init(mr, OBJECT(machine), "cxl_host_reg", cxl_size);
>>          memory_region_add_subregion(system_memory, cxl_base, mr);
>
Re: [PATCH v6 06/10] i386/pc: factor out cxl range start to helper
Posted by Igor Mammedov 3 years, 7 months ago
On Thu, 7 Jul 2022 16:18:43 +0100
Joao Martins <joao.m.martins@oracle.com> wrote:

> On 7/7/22 14:00, Igor Mammedov wrote:
> > On Fri,  1 Jul 2022 17:10:10 +0100
> > Joao Martins <joao.m.martins@oracle.com> wrote:
> >   
> >> Factor out the calculation of the base address of the MR. It will be
> >> used later on for the cxl range end counterpart calculation and as
> >> well in pc_memory_init() CXL mr initialization, thus avoiding
> >> duplication.
> >>
> >> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>  
> > 
> > needs to be rebased on top of 
> > 
> > 
> > [PATCH 2/3] hw/i386/pc: Always place CXL Memory Regions after device_memory
> >   
> Is Michael merging these or should I just respin v7 with the assumption
> that these patches are there?

I'd do the later (just mention dependency in cover letter)
 
> I can't see anything in his tree yet.
> 
> >> ---
> >>  hw/i386/pc.c | 28 +++++++++++++++++++---------
> >>  1 file changed, 19 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index 0abbf81841a9..8655cc3b8894 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -825,6 +825,24 @@ 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_start(PCMachineState *pcms)
> >> +{
> >> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> >> +    MachineState *machine = MACHINE(pcms);
> >> +    hwaddr cxl_base;
> >> +
> >> +    if (pcmc->has_reserved_memory && machine->device_memory->base) {
> >> +        cxl_base = machine->device_memory->base;
> >> +        if (!pcmc->broken_reserved_end) {
> >> +            cxl_base += memory_region_size(&machine->device_memory->mr);
> >> +        }
> >> +    } else {
> >> +        cxl_base = pc_above_4g_end(pcms);
> >> +    }
> >> +
> >> +    return cxl_base;
> >> +}
> >> +
> >>  static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
> >>  {
> >>      uint64_t start = 0;
> >> @@ -946,15 +964,7 @@ void pc_memory_init(PCMachineState *pcms,
> >>          MemoryRegion *mr = &pcms->cxl_devices_state.host_mr;
> >>          hwaddr cxl_size = MiB;
> >>  
> >> -        if (pcmc->has_reserved_memory && machine->device_memory->base) {
> >> -            cxl_base = machine->device_memory->base;
> >> -            if (!pcmc->broken_reserved_end) {
> >> -                cxl_base += memory_region_size(&machine->device_memory->mr);
> >> -            }
> >> -        } else {
> >> -            cxl_base = pc_above_4g_end(pcms);
> >> -        }
> >> -
> >> +        cxl_base = pc_get_cxl_range_start(pcms);
> >>          e820_add_entry(cxl_base, cxl_size, E820_RESERVED);
> >>          memory_region_init(mr, OBJECT(machine), "cxl_host_reg", cxl_size);
> >>          memory_region_add_subregion(system_memory, cxl_base, mr);  
> >   
>
Re: [PATCH v6 06/10] i386/pc: factor out cxl range start to helper
Posted by Joao Martins 3 years, 7 months ago
On 7/11/22 13:47, Igor Mammedov wrote:
> On Thu, 7 Jul 2022 16:18:43 +0100
> Joao Martins <joao.m.martins@oracle.com> wrote:
> 
>> On 7/7/22 14:00, Igor Mammedov wrote:
>>> On Fri,  1 Jul 2022 17:10:10 +0100
>>> Joao Martins <joao.m.martins@oracle.com> wrote:
>>>   
>>>> Factor out the calculation of the base address of the MR. It will be
>>>> used later on for the cxl range end counterpart calculation and as
>>>> well in pc_memory_init() CXL mr initialization, thus avoiding
>>>> duplication.
>>>>
>>>> Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
>>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>  
>>>
>>> needs to be rebased on top of 
>>>
>>>
>>> [PATCH 2/3] hw/i386/pc: Always place CXL Memory Regions after device_memory
>>>   
>> Is Michael merging these or should I just respin v7 with the assumption
>> that these patches are there?
> 
> I'd do the later (just mention dependency in cover letter)
>  

Yeap -- Will do.

>> I can't see anything in his tree yet.
>>
>>>> ---
>>>>  hw/i386/pc.c | 28 +++++++++++++++++++---------
>>>>  1 file changed, 19 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>>> index 0abbf81841a9..8655cc3b8894 100644
>>>> --- a/hw/i386/pc.c
>>>> +++ b/hw/i386/pc.c
>>>> @@ -825,6 +825,24 @@ 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_start(PCMachineState *pcms)
>>>> +{
>>>> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>>>> +    MachineState *machine = MACHINE(pcms);
>>>> +    hwaddr cxl_base;
>>>> +
>>>> +    if (pcmc->has_reserved_memory && machine->device_memory->base) {
>>>> +        cxl_base = machine->device_memory->base;
>>>> +        if (!pcmc->broken_reserved_end) {
>>>> +            cxl_base += memory_region_size(&machine->device_memory->mr);
>>>> +        }
>>>> +    } else {
>>>> +        cxl_base = pc_above_4g_end(pcms);
>>>> +    }
>>>> +
>>>> +    return cxl_base;
>>>> +}
>>>> +
>>>>  static uint64_t pc_get_cxl_range_end(PCMachineState *pcms)
>>>>  {
>>>>      uint64_t start = 0;
>>>> @@ -946,15 +964,7 @@ void pc_memory_init(PCMachineState *pcms,
>>>>          MemoryRegion *mr = &pcms->cxl_devices_state.host_mr;
>>>>          hwaddr cxl_size = MiB;
>>>>  
>>>> -        if (pcmc->has_reserved_memory && machine->device_memory->base) {
>>>> -            cxl_base = machine->device_memory->base;
>>>> -            if (!pcmc->broken_reserved_end) {
>>>> -                cxl_base += memory_region_size(&machine->device_memory->mr);
>>>> -            }
>>>> -        } else {
>>>> -            cxl_base = pc_above_4g_end(pcms);
>>>> -        }
>>>> -
>>>> +        cxl_base = pc_get_cxl_range_start(pcms);
>>>>          e820_add_entry(cxl_base, cxl_size, E820_RESERVED);
>>>>          memory_region_init(mr, OBJECT(machine), "cxl_host_reg", cxl_size);
>>>>          memory_region_add_subregion(system_memory, cxl_base, mr);  
>>>   
>>
>