Re: [PATCH RFCv2 3/4] i386/pc: warn if phys-bits is too low
On 2/24/22 09:01, Igor Mammedov wrote: > On Wed, 23 Feb 2022 17:18:55 + > Joao Martins wrote: > >> On 2/14/22 15:18, Joao Martins wrote: >>> On 2/14/22 15:03, Igor Mammedov wrote: On Mon, 7 Feb 2022 20:24:21 + Joao Martins wrote: > Default phys-bits on Qemu is TCG_PHYS_BITS (40) which is enough > to address 1Tb (0xff ). On AMD platforms, if a > ram-above-4g relocation happens and the CPU wasn't configured > with a big enough phys-bits, warn the user. There isn't a > catastrophic failure exactly, the guest will still boot, but > most likely won't be able to use more than ~4G of RAM. how 'unable to use" would manifest? It might be better to prevent QEMU startup with broken setup (CLI) rather then letting guest run and trying to figure out what's going wrong when users start to complain. >>> Sounds better to be conservative here. >>> >>> I will change from warn_report() to error_report() >>> and exit. >>> >> >> I was running through x86_64 qtests prior to submission >> and it seems that the inclusion of a pci_hole64_size in >> the check added by this patch would break tests if we were >> to error out. So far, I'm keeping it as a warning over >> compatibility concerns, not limited these 5 test failures >> below. Let me know otherwise if you disagree, or if you >> prefer another way. > > can you check what exactly breaks tests? > The test prematuralely fails with the above check that. And on a second look, the problem is obvious. Essentially, I am not handling the 32-bit case correctly :( I will revert back what I submitted in v3 to be an error_report() and exit() and will restrict this 64-bit only (i.e. for memory above-4g) qemu-system-x86_64: Address space above 4G at 1-1 phys-bits too low (32) qemu-system-x86_64: Address space above 4G at 1-1 phys-bits too low (32) qemu-system-x86_64: Address space above 4G at 1-1 phys-bits too low (32) # child process (/x86/cpuid/parsing-plus-minus/subprocess [188634]) stderr: "qemu-system-x86_64: warning: Ambiguous CPU model string. Don't mix both \"-mce\" and \"mce=on\"\nqemu-system-x86_64: warning: Ambiguous CPU model string. Don't mix both \"+cx8\" and \"cx8=off\"\nqemu-system-x86_64: warning: Compatibility of ambiguous CPU model strings won't be kept on future QEMU versions\nqemu-system-x86_64: Address space above 4G at 1-18000 phys-bits too low (32)\nBroken pipe\n" qemu-system-x86_64: Address space above 4G at 1-18000 phys-bits too low (32) >> Summary of Failures: >> >> 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 >> >
Re: [PATCH RFCv2 3/4] i386/pc: warn if phys-bits is too low
On Wed, 23 Feb 2022 17:18:55 + Joao Martins wrote: > On 2/14/22 15:18, Joao Martins wrote: > > On 2/14/22 15:03, Igor Mammedov wrote: > >> On Mon, 7 Feb 2022 20:24:21 + > >> Joao Martins wrote: > >> > >>> Default phys-bits on Qemu is TCG_PHYS_BITS (40) which is enough > >>> to address 1Tb (0xff ). On AMD platforms, if a > >>> ram-above-4g relocation happens and the CPU wasn't configured > >>> with a big enough phys-bits, warn the user. There isn't a > >>> catastrophic failure exactly, the guest will still boot, but > >>> most likely won't be able to use more than ~4G of RAM. > >> > >> how 'unable to use" would manifest? > >> It might be better to prevent QEMU startup with broken setup (CLI) > >> rather then letting guest run and trying to figure out what's > >> going wrong when users start to complain. > >> > > Sounds better to be conservative here. > > > > I will change from warn_report() to error_report() > > and exit. > > > > I was running through x86_64 qtests prior to submission > and it seems that the inclusion of a pci_hole64_size in > the check added by this patch would break tests if we were > to error out. So far, I'm keeping it as a warning over > compatibility concerns, not limited these 5 test failures > below. Let me know otherwise if you disagree, or if you > prefer another way. can you check what exactly breaks tests? > Summary of Failures: > > 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 >
Re: [PATCH RFCv2 3/4] i386/pc: warn if phys-bits is too low
On 2/14/22 15:18, Joao Martins wrote: > On 2/14/22 15:03, Igor Mammedov wrote: >> On Mon, 7 Feb 2022 20:24:21 + >> Joao Martins wrote: >> >>> Default phys-bits on Qemu is TCG_PHYS_BITS (40) which is enough >>> to address 1Tb (0xff ). On AMD platforms, if a >>> ram-above-4g relocation happens and the CPU wasn't configured >>> with a big enough phys-bits, warn the user. There isn't a >>> catastrophic failure exactly, the guest will still boot, but >>> most likely won't be able to use more than ~4G of RAM. >> >> how 'unable to use" would manifest? >> It might be better to prevent QEMU startup with broken setup (CLI) >> rather then letting guest run and trying to figure out what's >> going wrong when users start to complain. >> > Sounds better to be conservative here. > > I will change from warn_report() to error_report() > and exit. > I was running through x86_64 qtests prior to submission and it seems that the inclusion of a pci_hole64_size in the check added by this patch would break tests if we were to error out. So far, I'm keeping it as a warning over compatibility concerns, not limited these 5 test failures below. Let me know otherwise if you disagree, or if you prefer another way. Summary of Failures: 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
Re: [PATCH RFCv2 3/4] i386/pc: warn if phys-bits is too low
On 2/14/22 15:41, Igor Mammedov wrote:
> On Mon, 14 Feb 2022 15:18:43 +
> Joao Martins wrote:
>> On 2/14/22 15:03, Igor Mammedov wrote:
>>> On Mon, 7 Feb 2022 20:24:21 +
>>> Joao Martins wrote:
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index b060aedd38f3..f8712eb8427e 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -842,6 +842,7 @@ static void relocate_4g(MachineState *machine,
PCMachineState *pcms)
X86MachineState *x86ms = X86_MACHINE(pcms);
ram_addr_t device_mem_size = 0;
uint32_t eax, vendor[3];
+hwaddr maxphysaddr;
host_cpuid(0x0, 0, , [0], [2], [1]);
if (!IS_AMD_VENDOR(vendor)) {
@@ -858,6 +859,12 @@ static void relocate_4g(MachineState *machine,
PCMachineState *pcms)
return;
}
+maxphysaddr = ((hwaddr)1 << X86_CPU(first_cpu)->phys_bits) - 1;
+if (maxphysaddr < AMD_ABOVE_1TB_START)
+warn_report("Relocated RAM above 4G to start at %lu "
+"phys-bits too low (%u)",
+AMD_ABOVE_1TB_START, X86_CPU(first_cpu)->phys_bits);
>>>
>>> perhaps this hunk belongs to the end of pc_memory_init(),
>>> it's not HT fixup specific at all?
>>>
>> It is HT fixup related. Because we are relocating the whole above-4g-ram,
>> on what used to be enough with just 40 phys bits (default).
>
> what if user starts QEMU with amount of RAM that won't fit into
> configured phys bits (whether it's default one or one that comes from host)
> and not on AMD host at that so no relocation happens but we still have
> broken configuration. What matters here is the end address that might
> be used by guest (pci64_bit hole end) is reachable.
>
> That's why I suggested to make it generic check instead of AMD specific one.
>
OK, I see.
If I'm being dense, that would require replacing AMD_ABOVE_1TB_START in the
comparison to something that computes the max used addr -- Let me try that then.
Re: [PATCH RFCv2 3/4] i386/pc: warn if phys-bits is too low
On 2/14/22 15:03, Igor Mammedov wrote:
> On Mon, 7 Feb 2022 20:24:21 +
> Joao Martins wrote:
>
>> Default phys-bits on Qemu is TCG_PHYS_BITS (40) which is enough
>> to address 1Tb (0xff ). On AMD platforms, if a
>> ram-above-4g relocation happens and the CPU wasn't configured
>> with a big enough phys-bits, warn the user. There isn't a
>> catastrophic failure exactly, the guest will still boot, but
>> most likely won't be able to use more than ~4G of RAM.
>
> how 'unable to use" would manifest?
> It might be better to prevent QEMU startup with broken setup (CLI)
> rather then letting guest run and trying to figure out what's
> going wrong when users start to complain.
>
Sounds better to be conservative here.
I will change from warn_report() to error_report()
and exit.
>>
>> Signed-off-by: Joao Martins
>> ---
>> hw/i386/pc.c | 7 +++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index b060aedd38f3..f8712eb8427e 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -842,6 +842,7 @@ static void relocate_4g(MachineState *machine,
>> PCMachineState *pcms)
>> X86MachineState *x86ms = X86_MACHINE(pcms);
>> ram_addr_t device_mem_size = 0;
>> uint32_t eax, vendor[3];
>> +hwaddr maxphysaddr;
>>
>> host_cpuid(0x0, 0, , [0], [2], [1]);
>> if (!IS_AMD_VENDOR(vendor)) {
>> @@ -858,6 +859,12 @@ static void relocate_4g(MachineState *machine,
>> PCMachineState *pcms)
>> return;
>> }
>>
>> +maxphysaddr = ((hwaddr)1 << X86_CPU(first_cpu)->phys_bits) - 1;
>> +if (maxphysaddr < AMD_ABOVE_1TB_START)
>> +warn_report("Relocated RAM above 4G to start at %lu "
>> +"phys-bits too low (%u)",
>> +AMD_ABOVE_1TB_START, X86_CPU(first_cpu)->phys_bits);
>
> perhaps this hunk belongs to the end of pc_memory_init(),
> it's not HT fixup specific at all?
>
It is HT fixup related. Because we are relocating the whole above-4g-ram,
on what used to be enough with just 40 phys bits (default).
> Also I'm not sure but there are host_phys_bits/host_phys_bits_limit
> properties,
> perhaps they need to be checked/verified as well
When booted with +host-phys-bits and/or with a host-phys-bits-limit=X, the
@phys_bits
value will be either set to host, and ultimately bound to a maximum of
host_phys_bits_limit (if at all set).
So essentially the selected phys_bits that we're checking above is the only
thing
we need to care about IIUC.
Re: [PATCH RFCv2 3/4] i386/pc: warn if phys-bits is too low
On Mon, 14 Feb 2022 15:18:43 +
Joao Martins wrote:
> On 2/14/22 15:03, Igor Mammedov wrote:
> > On Mon, 7 Feb 2022 20:24:21 +
> > Joao Martins wrote:
> >
> >> Default phys-bits on Qemu is TCG_PHYS_BITS (40) which is enough
> >> to address 1Tb (0xff ). On AMD platforms, if a
> >> ram-above-4g relocation happens and the CPU wasn't configured
> >> with a big enough phys-bits, warn the user. There isn't a
> >> catastrophic failure exactly, the guest will still boot, but
> >> most likely won't be able to use more than ~4G of RAM.
> >
> > how 'unable to use" would manifest?
> > It might be better to prevent QEMU startup with broken setup (CLI)
> > rather then letting guest run and trying to figure out what's
> > going wrong when users start to complain.
> >
> Sounds better to be conservative here.
>
> I will change from warn_report() to error_report()
> and exit.
>
> >>
> >> Signed-off-by: Joao Martins
> >> ---
> >> hw/i386/pc.c | 7 +++
> >> 1 file changed, 7 insertions(+)
> >>
> >> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> >> index b060aedd38f3..f8712eb8427e 100644
> >> --- a/hw/i386/pc.c
> >> +++ b/hw/i386/pc.c
> >> @@ -842,6 +842,7 @@ static void relocate_4g(MachineState *machine,
> >> PCMachineState *pcms)
> >> X86MachineState *x86ms = X86_MACHINE(pcms);
> >> ram_addr_t device_mem_size = 0;
> >> uint32_t eax, vendor[3];
> >> +hwaddr maxphysaddr;
> >>
> >> host_cpuid(0x0, 0, , [0], [2], [1]);
> >> if (!IS_AMD_VENDOR(vendor)) {
> >> @@ -858,6 +859,12 @@ static void relocate_4g(MachineState *machine,
> >> PCMachineState *pcms)
> >> return;
> >> }
> >>
> >> +maxphysaddr = ((hwaddr)1 << X86_CPU(first_cpu)->phys_bits) - 1;
> >> +if (maxphysaddr < AMD_ABOVE_1TB_START)
> >> +warn_report("Relocated RAM above 4G to start at %lu "
> >> +"phys-bits too low (%u)",
> >> +AMD_ABOVE_1TB_START, X86_CPU(first_cpu)->phys_bits);
> >
> > perhaps this hunk belongs to the end of pc_memory_init(),
> > it's not HT fixup specific at all?
> >
> It is HT fixup related. Because we are relocating the whole above-4g-ram,
> on what used to be enough with just 40 phys bits (default).
what if user starts QEMU with amount of RAM that won't fit into
configured phys bits (whether it's default one or one that comes from host)
and not on AMD host at that so no relocation happens but we still have
broken configuration. What matters here is the end address that might
be used by guest (pci64_bit hole end) is reachable.
That's why I suggested to make it generic check instead of AMD specific one.
> > Also I'm not sure but there are host_phys_bits/host_phys_bits_limit
> > properties,
> > perhaps they need to be checked/verified as well
>
> When booted with +host-phys-bits and/or with a host-phys-bits-limit=X, the
> @phys_bits
> value will be either set to host, and ultimately bound to a maximum of
> host_phys_bits_limit (if at all set).
>
> So essentially the selected phys_bits that we're checking above is the only
> thing
> we need to care about IIUC.
>
Re: [PATCH RFCv2 3/4] i386/pc: warn if phys-bits is too low
On Mon, 7 Feb 2022 20:24:21 +
Joao Martins wrote:
> Default phys-bits on Qemu is TCG_PHYS_BITS (40) which is enough
> to address 1Tb (0xff ). On AMD platforms, if a
> ram-above-4g relocation happens and the CPU wasn't configured
> with a big enough phys-bits, warn the user. There isn't a
> catastrophic failure exactly, the guest will still boot, but
> most likely won't be able to use more than ~4G of RAM.
how 'unable to use" would manifest?
It might be better to prevent QEMU startup with broken setup (CLI)
rather then letting guest run and trying to figure out what's
going wrong when users start to complain.
>
> Signed-off-by: Joao Martins
> ---
> hw/i386/pc.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b060aedd38f3..f8712eb8427e 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -842,6 +842,7 @@ static void relocate_4g(MachineState *machine,
> PCMachineState *pcms)
> X86MachineState *x86ms = X86_MACHINE(pcms);
> ram_addr_t device_mem_size = 0;
> uint32_t eax, vendor[3];
> +hwaddr maxphysaddr;
>
> host_cpuid(0x0, 0, , [0], [2], [1]);
> if (!IS_AMD_VENDOR(vendor)) {
> @@ -858,6 +859,12 @@ static void relocate_4g(MachineState *machine,
> PCMachineState *pcms)
> return;
> }
>
> +maxphysaddr = ((hwaddr)1 << X86_CPU(first_cpu)->phys_bits) - 1;
> +if (maxphysaddr < AMD_ABOVE_1TB_START)
> +warn_report("Relocated RAM above 4G to start at %lu "
> +"phys-bits too low (%u)",
> +AMD_ABOVE_1TB_START, X86_CPU(first_cpu)->phys_bits);
perhaps this hunk belongs to the end of pc_memory_init(),
it's not HT fixup specific at all?
Also I'm not sure but there are host_phys_bits/host_phys_bits_limit properties,
perhaps they need to be checked/verified as well
> x86ms->above_4g_mem_start = AMD_ABOVE_1TB_START;
> }
>
Re: [PATCH RFCv2 3/4] i386/pc: warn if phys-bits is too low
On 2/14/22 13:15, David Edmondson wrote:
> On Monday, 2022-02-07 at 20:24:21 GMT, Joao Martins wrote:
>
>> Default phys-bits on Qemu is TCG_PHYS_BITS (40) which is enough
>> to address 1Tb (0xff ). On AMD platforms, if a
>> ram-above-4g relocation happens and the CPU wasn't configured
>> with a big enough phys-bits, warn the user. There isn't a
>> catastrophic failure exactly, the guest will still boot, but
>> most likely won't be able to use more than ~4G of RAM.
>>
>> Signed-off-by: Joao Martins
>> ---
>> hw/i386/pc.c | 7 +++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index b060aedd38f3..f8712eb8427e 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -842,6 +842,7 @@ static void relocate_4g(MachineState *machine,
>> PCMachineState *pcms)
>> X86MachineState *x86ms = X86_MACHINE(pcms);
>> ram_addr_t device_mem_size = 0;
>> uint32_t eax, vendor[3];
>> +hwaddr maxphysaddr;
>>
>> host_cpuid(0x0, 0, , [0], [2], [1]);
>> if (!IS_AMD_VENDOR(vendor)) {
>> @@ -858,6 +859,12 @@ static void relocate_4g(MachineState *machine,
>> PCMachineState *pcms)
>> return;
>> }
>>
>> +maxphysaddr = ((hwaddr)1 << X86_CPU(first_cpu)->phys_bits) - 1;
>> +if (maxphysaddr < AMD_ABOVE_1TB_START)
>
> Braces around the block are required, I believe.
>
Hmmm, my distration sorry about that -- checkpatch.pl wasn't so keen on warn me.
>> +warn_report("Relocated RAM above 4G to start at %lu "
>
> Should use PRIu64?
>
Yeap, will do.
>> +"phys-bits too low (%u)",
>> +AMD_ABOVE_1TB_START, X86_CPU(first_cpu)->phys_bits);
>> +
>> x86ms->above_4g_mem_start = AMD_ABOVE_1TB_START;
>
> And a real nit - until above_4g_mem_start is modified, the number of
> phys_bits is fine, so I would have put the warning after the assignment.
>
Good point. I'll reverse the order.
Thanks!
Joao
Re: [PATCH RFCv2 3/4] i386/pc: warn if phys-bits is too low
On Monday, 2022-02-07 at 20:24:21 GMT, Joao Martins wrote:
> Default phys-bits on Qemu is TCG_PHYS_BITS (40) which is enough
> to address 1Tb (0xff ). On AMD platforms, if a
> ram-above-4g relocation happens and the CPU wasn't configured
> with a big enough phys-bits, warn the user. There isn't a
> catastrophic failure exactly, the guest will still boot, but
> most likely won't be able to use more than ~4G of RAM.
>
> Signed-off-by: Joao Martins
> ---
> hw/i386/pc.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b060aedd38f3..f8712eb8427e 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -842,6 +842,7 @@ static void relocate_4g(MachineState *machine,
> PCMachineState *pcms)
> X86MachineState *x86ms = X86_MACHINE(pcms);
> ram_addr_t device_mem_size = 0;
> uint32_t eax, vendor[3];
> +hwaddr maxphysaddr;
>
> host_cpuid(0x0, 0, , [0], [2], [1]);
> if (!IS_AMD_VENDOR(vendor)) {
> @@ -858,6 +859,12 @@ static void relocate_4g(MachineState *machine,
> PCMachineState *pcms)
> return;
> }
>
> +maxphysaddr = ((hwaddr)1 << X86_CPU(first_cpu)->phys_bits) - 1;
> +if (maxphysaddr < AMD_ABOVE_1TB_START)
Braces around the block are required, I believe.
> +warn_report("Relocated RAM above 4G to start at %lu "
Should use PRIu64?
> +"phys-bits too low (%u)",
> +AMD_ABOVE_1TB_START, X86_CPU(first_cpu)->phys_bits);
> +
> x86ms->above_4g_mem_start = AMD_ABOVE_1TB_START;
And a real nit - until above_4g_mem_start is modified, the number of
phys_bits is fine, so I would have put the warning after the assignment.
> }
dme.
--
Tonight I'm gonna bury that horse in the ground.
