On 6/20/22 15:27, Igor Mammedov wrote:
> On Fri, 17 Jun 2022 14:33:02 +0100
> Joao Martins <joao.m.mart...@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.mart...@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.mart...@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);
