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

[Joao Martins]

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) {
>