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

[Joao Martins]

On 6/20/22 15:27, Igor Mammedov wrote:
> On Fri, 17 Jun 2022 14:33:02 +0100
> Joao Martins <joao.m.martins@oracle.com> wrote:
>> On 6/17/22 13:32, Igor Mammedov wrote:
>>> On Fri, 17 Jun 2022 13:18:38 +0100
>>> Joao Martins <joao.m.martins@oracle.com> 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:  
>>>>>> +                                hwaddr above_4g_mem_start,
>>>>>> +                                uint64_t pci_hole64_size)
>>>>>> +{
>>>>>> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
>>>>>> +    X86MachineState *x86ms = X86_MACHINE(pcms);
>>>>>> +    MachineState *machine = MACHINE(pcms);
>>>>>> +    ram_addr_t device_mem_size = 0;
>>>>>> +    hwaddr base;
>>>>>> +
>>>>>> +    if (!x86ms->above_4g_mem_size) {
>>>>>> +       /*
>>>>>> +        * 32-bit pci hole goes from
>>>>>> +        * end-of-low-ram (@below_4g_mem_size) to IOAPIC.
>>>>>> +        */
>>>>>> +        return IO_APIC_DEFAULT_ADDRESS - 1;    
>>>>>
>>>>> lack of above_4g_mem, doesn't mean absence of device_mem_size or anything 
>>>>> else
>>>>> that's located above it.
>>>>>     
>>>>
>>>> True. But the intent is to fix 32-bit boundaries as one of the qtests was 
>>>> failing
>>>> otherwise. We won't hit the 1T hole, hence a nop.  
>>>
>>> I don't get the reasoning, can you clarify it pls?
>>>   
>>
>> I was trying to say that what lead me here was a couple of qtests failures 
>> (from v3->v4).
>>
>> I was doing this before based on pci_hole64. phys-bits=32 was for example one
>> of the test failures, and pci-hole64 sits above what 32-bit can reference.
> 
> if user sets phys-bits=32, then nothing above 4Gb should work (be usable)
> (including above-4g-ram, hotplug region or pci64 hole or sgx or cxl)
> 
> and this doesn't look to me as AMD specific issue
> 
> perhaps do a phys-bits check as a separate patch
> that will error out if max_used_gpa is above phys-bits limit
> (maybe at machine_done time)
> (i.e. defining max_gpa and checking if compatible with configured cpu
> are 2 different things)
> 
> (it might be possible that tests need to be fixed too to account for it)
> 

My old notes (from v3) tell me with such a check these tests were exiting early 
thanks to
that error:

 1/56 qemu:qtest+qtest-x86_64 / qtest-x86_64/qom-test               ERROR       
    0.07s
  killed by signal 6 SIGABRT
 4/56 qemu:qtest+qtest-x86_64 / qtest-x86_64/test-hmp               ERROR       
    0.07s
  killed by signal 6 SIGABRT
 7/56 qemu:qtest+qtest-x86_64 / qtest-x86_64/boot-serial-test       ERROR       
    0.07s
  killed by signal 6 SIGABRT
44/56 qemu:qtest+qtest-x86_64 / qtest-x86_64/test-x86-cpuid-compat  ERROR       
    0.09s
  killed by signal 6 SIGABRT
45/56 qemu:qtest+qtest-x86_64 / qtest-x86_64/numa-test              ERROR       
    0.17s
  killed by signal 6 SIGABRT

But the real reason these fail is not at all related to CPU phys bits,
but because we just don't handle the case where no pci_hole64 is supposed to 
exist (which
is what that other check is trying to do) e.g. A VM with -m 1G would
observe the same thing i.e. the computations after that conditional are all for 
the pci
hole64, which acounts for SGX/CXL/hotplug or etc which consequently means it's 
*errousnly*
bigger than phys-bits=32 (by definition). So the error_report is just telling 
me that
pc_max_used_gpa() is just incorrect without the !x86ms->above_4g_mem_size check.

If you're not fond of:

+    if (!x86ms->above_4g_mem_size) {
+       /*
+        * 32-bit pci hole goes from
+        * end-of-low-ram (@below_4g_mem_size) to IOAPIC.
+         */
+        return IO_APIC_DEFAULT_ADDRESS - 1;
+    }

Then what should I use instead of the above?

'IO_APIC_DEFAULT_ADDRESS - 1' is the size of the 32-bit PCI hole, which is
also what is used for i440fx/q35 code. I could move it to a macro (e.g.
PCI_HOST_HOLE32_SIZE) to make it a bit readable and less hardcoded. Or
perhaps your problem is on !x86ms->above_4g_mem_size and maybe I should check
in addition for hotplug/CXL/etc existence?

>>>>  Unless we plan on using
>>>> pc_max_used_gpa() for something else other than this.  
>>>
>>> Even if '!above_4g_mem_sizem', we can still have hotpluggable memory region
>>> present and that can  hit 1Tb. The same goes for pci64_hole if it's 
>>> configured
>>> large enough on CLI.
>>>   
>> So hotpluggable memory seems to assume it sits above 4g mem.
>>
>> pci_hole64 likewise as it uses similar computations as hotplug.
>>
>> Unless I am misunderstanding something here.
>>
>>> Looks like guesstimate we could use is taking pci64_hole_end as max used GPA
>>>   
>> I think this was what I had before (v3[0]) and did not work.
> 
> that had been tied to host's phys-bits directly, all in one patch
> and duplicating existing pc_pci_hole64_start().
>  

Duplicating was sort of my bad attempt in this patch for pc_max_used_gpa()

I was sort of thinking to something like extracting calls to start + size 
"tuple" into
functions -- e.g. for hotplug it is pc_get_device_memory_range() and for CXL it 
would be
maybe pc_get_cxl_range()) -- rather than assuming those values are already 
initialized on
the memory-region @base and its size.

See snippet below. Note I am missing CXL handling, but gives you the idea.

But it is slightly more complex than what I had in this version :( and would 
require
anyone doing changes in pc_memory_init() and pc_pci_hole64_start() to make sure 
it follows
the similar logic.

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index fd088093b5d5..016bc65fcb4b 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -885,6 +885,34 @@ static void pc_set_amd_above_4g_mem_start(PCMachineState 
*pcms,
     x86ms->above_4g_mem_start = start;
 }

+static void pc_get_device_memory_range(PCMachineState *pcms,
+                                       hwaddr *base,
+                                       hwaddr *device_mem_size)
+{
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+    X86MachineState *x86ms = X86_MACHINE(pcms);
+    MachineState *machine = MACHINE(pcms);
+    hwaddr addr, size;
+
+    size = machine->maxram_size - machine->ram_size;
+
+    if (pcms->sgx_epc.size != 0) {
+        addr = sgx_epc_above_4g_end(&pcms->sgx_epc);
+    } else {
+        addr = x86ms->above_4g_mem_start + x86ms->above_4g_mem_size;
+    }
+
+    if (pcmc->enforce_aligned_dimm) {
+        /* size device region assuming 1G page max alignment per slot */
+        size += (1 * GiB) * machine->ram_slots;
+    }
+
+    if (base)
+        *base = addr;
+    if (device_mem_size)
+        *device_mem_size = size;
+}
+
 void pc_memory_init(PCMachineState *pcms,
                     MemoryRegion *system_memory,
                     MemoryRegion *rom_memory,
@@ -962,7 +990,7 @@ void pc_memory_init(PCMachineState *pcms,
     /* initialize device memory address space */
     if (pcmc->has_reserved_memory &&
         (machine->ram_size < machine->maxram_size)) {
-        ram_addr_t device_mem_size = machine->maxram_size - machine->ram_size;
+        ram_addr_t device_mem_size;

         if (machine->ram_slots > ACPI_MAX_RAM_SLOTS) {
             error_report("unsupported amount of memory slots: %"PRIu64,
@@ -977,20 +1005,7 @@ void pc_memory_init(PCMachineState *pcms,
             exit(EXIT_FAILURE);
         }

-        if (pcms->sgx_epc.size != 0) {
-            machine->device_memory->base = 
sgx_epc_above_4g_end(&pcms->sgx_epc);
-        } else {
-            machine->device_memory->base =
-                x86ms->above_4g_mem_start + x86ms->above_4g_mem_size;
-        }
-
-        machine->device_memory->base =
-            ROUND_UP(machine->device_memory->base, 1 * GiB);
-
-        if (pcmc->enforce_aligned_dimm) {
-            /* size device region assuming 1G page max alignment per slot */
-            device_mem_size += (1 * GiB) * machine->ram_slots;
-        }
+        pc_get_device_memory_range(pcms, &machine->device_memory->base, 
&device_mem_size);

         if ((machine->device_memory->base + device_mem_size) <
             device_mem_size) {
@@ -1053,6 +1068,27 @@ void pc_memory_init(PCMachineState *pcms,
     pcms->memhp_io_base = ACPI_MEMORY_HOTPLUG_BASE;
 }

+static uint64_t x86ms_pci_hole64_start(PCMachineState *pcms)
+{
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+    X86MachineState *x86ms = X86_MACHINE(pcms);
+    MachineState *machine = MACHINE(pcms);
+    uint64_t hole64_start, size;
+
+    if (pcmc->has_reserved_memory &&
+        (machine->ram_size < machine->maxram_size)) {
+        pc_get_device_memory_range(pcms, &hole64_start, &size);
+        if (!pcmc->broken_reserved_end) {
+            hole64_start += size;
+        }
+    } else if (pcms->sgx_epc.size != 0) {
+        hole64_start = sgx_epc_above_4g_end(&pcms->sgx_epc);
+    } else {
+        hole64_start = x86ms->above_4g_mem_start + x86ms->above_4g_mem_size;
+    }
+
+    return hole64_start;
+}
 /*
  * The 64bit pci hole starts after "above 4G RAM" and
  * potentially the space reserved for memory hotplug.
@@ -1062,18 +1098,17 @@ uint64_t pc_pci_hole64_start(void)
     PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
     MachineState *ms = MACHINE(pcms);
-    X86MachineState *x86ms = X86_MACHINE(pcms);
     uint64_t hole64_start = 0;

-    if (pcmc->has_reserved_memory && ms->device_memory->base) {
+    if (pcmc->has_reserved_memory &&
+        ms->device_memory && ms->device_memory->base) {
         hole64_start = ms->device_memory->base;
         if (!pcmc->broken_reserved_end) {
             hole64_start += memory_region_size(&ms->device_memory->mr);
         }
-    } else if (pcms->sgx_epc.size != 0) {
-            hole64_start = sgx_epc_above_4g_end(&pcms->sgx_epc);
     } else {
-        hole64_start = x86ms->above_4g_mem_start + x86ms->above_4g_mem_size;
+        /* handles unpopulated memory regions */
+        hole64_start = x86ms_pci_hole64_start(pcms);
     }

     return ROUND_UP(hole64_start, 1 * GiB);