Re: [PATCH v5 4/5] i386/pc: relocate 4g start to 1T where applicable

[Joao Martins]

On 6/17/22 13:18, Joao Martins wrote:
> On 6/16/22 15:23, Igor Mammedov wrote:
>> On Fri, 20 May 2022 11:45:31 +0100
>> Joao Martins <joao.m.martins@oracle.com> wrote:
>>> +    }
>>> +
>>> +    if (pcmc->has_reserved_memory &&
>>> +       (machine->ram_size < machine->maxram_size)) {
>>> +        device_mem_size = machine->maxram_size - machine->ram_size;
>>> +    }
>>> +
>>> +    base = ROUND_UP(above_4g_mem_start + x86ms->above_4g_mem_size +
>>> +                    pcms->sgx_epc.size, 1 * GiB);
>>> +
>>> +    return base + device_mem_size + pci_hole64_size;
>>
>> it's not guarantied that pci64 hole starts right away device_mem,
>> but you are not 1st doing this assumption in code, maybe instead of
>> all above use existing 
>>    pc_pci_hole64_start() + pci_hole64_size
>> to gestimate max address 
>>
> I've switched the block above to that instead.
> 

I had done this, albeit on a second look (and confirmed with testing) this
will crash, provided @device_memory isn't yet initialized. And even without
hotplug, CXL might have had issues.

The problem is largely that pc_pci_hole64_start() that the above check relies
on info we only populate later on in pc_memory_init(), and I don't think I can
move this done to a later point as definitely don't want to re-initialize
MRs or anything.

So we might be left with manually calculating as I was doing in this patch
but maybe try to arrange some form of new helper that has somewhat shared
logic with pc_pci_hole64_start().

  1114  uint64_t pc_pci_hole64_start(void)
  1115  {
  1116      PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
  1117      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
  1118      MachineState *ms = MACHINE(pcms);
  1119      X86MachineState *x86ms = X86_MACHINE(pcms);
  1120      uint64_t hole64_start = 0;
  1121
  1122      if (pcms->cxl_devices_state.host_mr.addr) {
  1123          hole64_start = pcms->cxl_devices_state.host_mr.addr +
  1124              memory_region_size(&pcms->cxl_devices_state.host_mr);
  1125          if (pcms->cxl_devices_state.fixed_windows) {
  1126              GList *it;
  1127              for (it = pcms->cxl_devices_state.fixed_windows; it; it = 
it->next) {
  1128                  CXLFixedWindow *fw = it->data;
  1129                  hole64_start = fw->mr.addr + 
memory_region_size(&fw->mr);
  1130              }
  1131          }
* 1132      } else if (pcmc->has_reserved_memory && ms->device_memory->base) {
  1133          hole64_start = ms->device_memory->base;
  1134          if (!pcmc->broken_reserved_end) {
  1135              hole64_start += memory_region_size(&ms->device_memory->mr);
  1136          }
  1137      } else if (pcms->sgx_epc.size != 0) {
  1138              hole64_start = sgx_epc_above_4g_end(&pcms->sgx_epc);
  1139      } else {
  1140          hole64_start = x86ms->above_4g_mem_start + 
x86ms->above_4g_mem_size;
  1141      }