| Message ID | 20240327165456.34716-1-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCH-for-9.0,v2] hw/i386/pc: Deprecate 64-bit CPUs on ISA-only PC machine | expand |
W dniu 27.03.2024 o 17:54, Philippe Mathieu-Daudé pisze: > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index 7b548519b5..345c35507f 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -208,6 +208,13 @@ is no longer packaged in any distro making it harder to run the > ``check-tcg`` tests. Unless we can improve the testing situation there > is a chance the code will bitrot without anyone noticing. > > +64-bit (x86_64) CPUs on the ``isapc`` machine (since 9.0) > +''''''''''''''''''''''''''''''''''''''''''''''''''''''''' > + > +The ``isapc`` machine aims to emulate old PC machine without PCI was > +generalized, so hardware available around 1995, before 64-bit intel > +CPUs were produced. Can you s/Intel/x86-64/ here? Intel was not first with x86-64 (AMD invented it). Also "64-bit Intel" smells of Itanium too much.
On 27/3/24 18:29, Marcin Juszkiewicz wrote: > W dniu 27.03.2024 o 17:54, Philippe Mathieu-Daudé pisze: >> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst >> index 7b548519b5..345c35507f 100644 >> --- a/docs/about/deprecated.rst >> +++ b/docs/about/deprecated.rst >> @@ -208,6 +208,13 @@ is no longer packaged in any distro making it >> harder to run the >> ``check-tcg`` tests. Unless we can improve the testing situation there >> is a chance the code will bitrot without anyone noticing. >> +64-bit (x86_64) CPUs on the ``isapc`` machine (since 9.0) >> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''' >> + >> +The ``isapc`` machine aims to emulate old PC machine without PCI was >> +generalized, so hardware available around 1995, before 64-bit intel >> +CPUs were produced. > > Can you s/Intel/x86-64/ here? Intel was not first with x86-64 (AMD > invented it). Also "64-bit Intel" smells of Itanium too much. OK ;)
On 27/03/2024 16:54, Philippe Mathieu-Daudé wrote: > Per Daniel suggestion [*]: > > > isapc could arguably be restricted to just 32-bit CPU models, > > because we should not need it to support any feature that didn't > > exist prior to circa 1995. eg refuse to start with isapc, if 'lm' > > is present in the CPU model for example. > > Display a warning when such CPU is used: > > $ qemu-system-x86_64 -monitor stdio -S -M isapc -cpu Westmere > qemu-system-x86_64: warning: Use of 64-bit CPU 'Westmere' is deprecated on the ISA-only PC machine > QEMU 8.2.91 monitor - type 'help' for more information > (qemu) q > > $ qemu-system-x86_64 -monitor stdio -S -M isapc -cpu athlon > QEMU 8.2.91 monitor - type 'help' for more information > (qemu) q > > [*] https://lore.kernel.org/qemu-devel/ZgQkS4RPmSt5Xa08@redhat.com/ > > Suggested-by: Daniel P. Berrangé <berrange@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > docs/about/deprecated.rst | 7 +++++++ > include/hw/i386/pc.h | 1 + > hw/i386/pc_piix.c | 14 ++++++++++++++ > 3 files changed, 22 insertions(+) > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index 7b548519b5..345c35507f 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -208,6 +208,13 @@ is no longer packaged in any distro making it harder to run the > ``check-tcg`` tests. Unless we can improve the testing situation there > is a chance the code will bitrot without anyone noticing. > > +64-bit (x86_64) CPUs on the ``isapc`` machine (since 9.0) > +''''''''''''''''''''''''''''''''''''''''''''''''''''''''' > + > +The ``isapc`` machine aims to emulate old PC machine without PCI was > +generalized, so hardware available around 1995, before 64-bit intel > +CPUs were produced. > + > System emulator machines > ------------------------ > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index 27a68071d7..2d202b9549 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -96,6 +96,7 @@ struct PCMachineClass { > const char *default_south_bridge; > > /* Compat options: */ > + bool deprecate_64bit_cpu; /* Specific to the 'isapc' machine */ > > /* Default CPU model version. See x86_cpu_set_default_version(). */ > int default_cpu_version; > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 18ba076609..2e5b2efc33 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -182,7 +182,20 @@ static void pc_init1(MachineState *machine, const char *pci_type) > } > > pc_machine_init_sgx_epc(pcms); > + > x86_cpus_init(x86ms, pcmc->default_cpu_version); > + if (pcmc->deprecate_64bit_cpu) { > + X86CPU *cpu = X86_CPU(first_cpu); > + > + if (cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) { > + const char *cpu_type = object_get_typename(OBJECT(first_cpu)); > + int cpu_len = strlen(cpu_type) - strlen(X86_CPU_TYPE_SUFFIX); > + > + warn_report("Use of 64-bit CPU '%.*s' is deprecated" > + " on the ISA-only PC machine", > + cpu_len, cpu_type); > + } > + } > > if (kvm_enabled()) { > kvmclock_create(pcmc->kvmclock_create_always); > @@ -918,6 +931,7 @@ static void isapc_machine_options(MachineClass *m) > pcmc->gigabyte_align = false; > pcmc->smbios_legacy_mode = true; > pcmc->has_reserved_memory = false; > + pcmc->deprecate_64bit_cpu = true; > m->default_nic = "ne2k_isa"; > m->default_cpu_type = X86_CPU_TYPE_NAME("486"); > m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL); The logic around checking CPUID_EXT2_LM looks good to me. Slightly curious as to whether people feel updating PCMachineClass is necessary, or you can simply do qdev_get_machine() and use object_dynamic_cast() to see if the machine matches MACHINE_NAME("isapc") and warn that way? FWIW I'd be amazed if anyone were actually overriding the default and trying to do this, but I guess that's what the warn_report() is for.... anyhow: Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> ATB, Mark.
On Wed, Mar 27, 2024 at 05:54:56PM +0100, Philippe Mathieu-Daudé wrote: > Per Daniel suggestion [*]: > > > isapc could arguably be restricted to just 32-bit CPU models, > > because we should not need it to support any feature that didn't > > exist prior to circa 1995. eg refuse to start with isapc, if 'lm' > > is present in the CPU model for example. > > Display a warning when such CPU is used: > > $ qemu-system-x86_64 -monitor stdio -S -M isapc -cpu Westmere > qemu-system-x86_64: warning: Use of 64-bit CPU 'Westmere' is deprecated on the ISA-only PC machine > QEMU 8.2.91 monitor - type 'help' for more information > (qemu) q > > $ qemu-system-x86_64 -monitor stdio -S -M isapc -cpu athlon > QEMU 8.2.91 monitor - type 'help' for more information > (qemu) q I've thought of a possible problem here.. $ qemu-system-x86_64 -monitor stdio -S -M isapc -cpu max is going to enable 'lm' (and a bazillion other new features) for 'isapc', which is a shame, as 'max' is something we want to be usable in general. I'm not sure how to square that circle. I might suggest that 'isapc' instead only makes sense in the context of qemu-system-i386, since that only has 32-bit CPUs. We wanted to kill that binary in favour of qemu-system-x86_64 for both 32 & 64 bit though, so we can't block 'isapc' from qemu-system-x86_64. > > [*] https://lore.kernel.org/qemu-devel/ZgQkS4RPmSt5Xa08@redhat.com/ > > Suggested-by: Daniel P. Berrangé <berrange@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > docs/about/deprecated.rst | 7 +++++++ > include/hw/i386/pc.h | 1 + > hw/i386/pc_piix.c | 14 ++++++++++++++ > 3 files changed, 22 insertions(+) > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index 7b548519b5..345c35507f 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -208,6 +208,13 @@ is no longer packaged in any distro making it harder to run the > ``check-tcg`` tests. Unless we can improve the testing situation there > is a chance the code will bitrot without anyone noticing. > > +64-bit (x86_64) CPUs on the ``isapc`` machine (since 9.0) > +''''''''''''''''''''''''''''''''''''''''''''''''''''''''' > + > +The ``isapc`` machine aims to emulate old PC machine without PCI was > +generalized, so hardware available around 1995, before 64-bit intel > +CPUs were produced. > + > System emulator machines > ------------------------ > > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > index 27a68071d7..2d202b9549 100644 > --- a/include/hw/i386/pc.h > +++ b/include/hw/i386/pc.h > @@ -96,6 +96,7 @@ struct PCMachineClass { > const char *default_south_bridge; > > /* Compat options: */ > + bool deprecate_64bit_cpu; /* Specific to the 'isapc' machine */ > > /* Default CPU model version. See x86_cpu_set_default_version(). */ > int default_cpu_version; > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 18ba076609..2e5b2efc33 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -182,7 +182,20 @@ static void pc_init1(MachineState *machine, const char *pci_type) > } > > pc_machine_init_sgx_epc(pcms); > + > x86_cpus_init(x86ms, pcmc->default_cpu_version); > + if (pcmc->deprecate_64bit_cpu) { > + X86CPU *cpu = X86_CPU(first_cpu); > + > + if (cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) { > + const char *cpu_type = object_get_typename(OBJECT(first_cpu)); > + int cpu_len = strlen(cpu_type) - strlen(X86_CPU_TYPE_SUFFIX); > + > + warn_report("Use of 64-bit CPU '%.*s' is deprecated" > + " on the ISA-only PC machine", > + cpu_len, cpu_type); > + } > + } > > if (kvm_enabled()) { > kvmclock_create(pcmc->kvmclock_create_always); > @@ -918,6 +931,7 @@ static void isapc_machine_options(MachineClass *m) > pcmc->gigabyte_align = false; > pcmc->smbios_legacy_mode = true; > pcmc->has_reserved_memory = false; > + pcmc->deprecate_64bit_cpu = true; > m->default_nic = "ne2k_isa"; > m->default_cpu_type = X86_CPU_TYPE_NAME("486"); > m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL); > -- > 2.41.0 > With regards, Daniel
On 28/03/2024 16.12, Mark Cave-Ayland wrote: > On 27/03/2024 16:54, Philippe Mathieu-Daudé wrote: > >> Per Daniel suggestion [*]: >> >> > isapc could arguably be restricted to just 32-bit CPU models, >> > because we should not need it to support any feature that didn't >> > exist prior to circa 1995. eg refuse to start with isapc, if 'lm' >> > is present in the CPU model for example. >> >> Display a warning when such CPU is used: >> >> $ qemu-system-x86_64 -monitor stdio -S -M isapc -cpu Westmere >> qemu-system-x86_64: warning: Use of 64-bit CPU 'Westmere' is deprecated >> on the ISA-only PC machine >> QEMU 8.2.91 monitor - type 'help' for more information >> (qemu) q >> >> $ qemu-system-x86_64 -monitor stdio -S -M isapc -cpu athlon >> QEMU 8.2.91 monitor - type 'help' for more information >> (qemu) q >> >> [*] https://lore.kernel.org/qemu-devel/ZgQkS4RPmSt5Xa08@redhat.com/ >> >> Suggested-by: Daniel P. Berrangé <berrange@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> docs/about/deprecated.rst | 7 +++++++ >> include/hw/i386/pc.h | 1 + >> hw/i386/pc_piix.c | 14 ++++++++++++++ >> 3 files changed, 22 insertions(+) >> >> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst >> index 7b548519b5..345c35507f 100644 >> --- a/docs/about/deprecated.rst >> +++ b/docs/about/deprecated.rst >> @@ -208,6 +208,13 @@ is no longer packaged in any distro making it harder >> to run the >> ``check-tcg`` tests. Unless we can improve the testing situation there >> is a chance the code will bitrot without anyone noticing. >> +64-bit (x86_64) CPUs on the ``isapc`` machine (since 9.0) >> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''' >> + >> +The ``isapc`` machine aims to emulate old PC machine without PCI was >> +generalized, so hardware available around 1995, before 64-bit intel >> +CPUs were produced. >> + >> System emulator machines >> ------------------------ >> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h >> index 27a68071d7..2d202b9549 100644 >> --- a/include/hw/i386/pc.h >> +++ b/include/hw/i386/pc.h >> @@ -96,6 +96,7 @@ struct PCMachineClass { >> const char *default_south_bridge; >> /* Compat options: */ >> + bool deprecate_64bit_cpu; /* Specific to the 'isapc' machine */ >> /* Default CPU model version. See x86_cpu_set_default_version(). */ >> int default_cpu_version; >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >> index 18ba076609..2e5b2efc33 100644 >> --- a/hw/i386/pc_piix.c >> +++ b/hw/i386/pc_piix.c >> @@ -182,7 +182,20 @@ static void pc_init1(MachineState *machine, const >> char *pci_type) >> } >> pc_machine_init_sgx_epc(pcms); >> + >> x86_cpus_init(x86ms, pcmc->default_cpu_version); >> + if (pcmc->deprecate_64bit_cpu) { >> + X86CPU *cpu = X86_CPU(first_cpu); >> + >> + if (cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) { >> + const char *cpu_type = object_get_typename(OBJECT(first_cpu)); >> + int cpu_len = strlen(cpu_type) - strlen(X86_CPU_TYPE_SUFFIX); >> + >> + warn_report("Use of 64-bit CPU '%.*s' is deprecated" >> + " on the ISA-only PC machine", >> + cpu_len, cpu_type); >> + } >> + } >> if (kvm_enabled()) { >> kvmclock_create(pcmc->kvmclock_create_always); >> @@ -918,6 +931,7 @@ static void isapc_machine_options(MachineClass *m) >> pcmc->gigabyte_align = false; >> pcmc->smbios_legacy_mode = true; >> pcmc->has_reserved_memory = false; >> + pcmc->deprecate_64bit_cpu = true; >> m->default_nic = "ne2k_isa"; >> m->default_cpu_type = X86_CPU_TYPE_NAME("486"); >> m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL); > > The logic around checking CPUID_EXT2_LM looks good to me. Slightly curious > as to whether people feel updating PCMachineClass is necessary, or you can > simply do qdev_get_machine() and use object_dynamic_cast() to see if the > machine matches MACHINE_NAME("isapc") and warn that way? Why don't you simply pass it as a parameter from pc_init_isa() instead? Or do the whole check in pc_init_isa() instead? Thomas
On 28/3/24 16:39, Thomas Huth wrote: > On 28/03/2024 16.12, Mark Cave-Ayland wrote: >> On 27/03/2024 16:54, Philippe Mathieu-Daudé wrote: >> >>> Per Daniel suggestion [*]: >>> >>> > isapc could arguably be restricted to just 32-bit CPU models, >>> > because we should not need it to support any feature that didn't >>> > exist prior to circa 1995. eg refuse to start with isapc, if 'lm' >>> > is present in the CPU model for example. >>> >>> Display a warning when such CPU is used: >>> >>> $ qemu-system-x86_64 -monitor stdio -S -M isapc -cpu Westmere >>> qemu-system-x86_64: warning: Use of 64-bit CPU 'Westmere' is >>> deprecated on the ISA-only PC machine >>> QEMU 8.2.91 monitor - type 'help' for more information >>> (qemu) q >>> >>> $ qemu-system-x86_64 -monitor stdio -S -M isapc -cpu athlon >>> QEMU 8.2.91 monitor - type 'help' for more information >>> (qemu) q >>> >>> [*] https://lore.kernel.org/qemu-devel/ZgQkS4RPmSt5Xa08@redhat.com/ >>> >>> Suggested-by: Daniel P. Berrangé <berrange@redhat.com> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> docs/about/deprecated.rst | 7 +++++++ >>> include/hw/i386/pc.h | 1 + >>> hw/i386/pc_piix.c | 14 ++++++++++++++ >>> 3 files changed, 22 insertions(+) >>> >>> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst >>> index 7b548519b5..345c35507f 100644 >>> --- a/docs/about/deprecated.rst >>> +++ b/docs/about/deprecated.rst >>> @@ -208,6 +208,13 @@ is no longer packaged in any distro making it >>> harder to run the >>> ``check-tcg`` tests. Unless we can improve the testing situation there >>> is a chance the code will bitrot without anyone noticing. >>> +64-bit (x86_64) CPUs on the ``isapc`` machine (since 9.0) >>> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''' >>> + >>> +The ``isapc`` machine aims to emulate old PC machine without PCI was >>> +generalized, so hardware available around 1995, before 64-bit intel >>> +CPUs were produced. >>> + >>> System emulator machines >>> ------------------------ >>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h >>> index 27a68071d7..2d202b9549 100644 >>> --- a/include/hw/i386/pc.h >>> +++ b/include/hw/i386/pc.h >>> @@ -96,6 +96,7 @@ struct PCMachineClass { >>> const char *default_south_bridge; >>> /* Compat options: */ >>> + bool deprecate_64bit_cpu; /* Specific to the 'isapc' machine */ >>> /* Default CPU model version. See >>> x86_cpu_set_default_version(). */ >>> int default_cpu_version; >>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >>> index 18ba076609..2e5b2efc33 100644 >>> --- a/hw/i386/pc_piix.c >>> +++ b/hw/i386/pc_piix.c >>> @@ -182,7 +182,20 @@ static void pc_init1(MachineState *machine, >>> const char *pci_type) >>> } >>> pc_machine_init_sgx_epc(pcms); >>> + >>> x86_cpus_init(x86ms, pcmc->default_cpu_version); >>> + if (pcmc->deprecate_64bit_cpu) { >>> + X86CPU *cpu = X86_CPU(first_cpu); >>> + >>> + if (cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) { >>> + const char *cpu_type = >>> object_get_typename(OBJECT(first_cpu)); >>> + int cpu_len = strlen(cpu_type) - >>> strlen(X86_CPU_TYPE_SUFFIX); >>> + >>> + warn_report("Use of 64-bit CPU '%.*s' is deprecated" >>> + " on the ISA-only PC machine", >>> + cpu_len, cpu_type); >>> + } >>> + } >>> if (kvm_enabled()) { >>> kvmclock_create(pcmc->kvmclock_create_always); >>> @@ -918,6 +931,7 @@ static void isapc_machine_options(MachineClass *m) >>> pcmc->gigabyte_align = false; >>> pcmc->smbios_legacy_mode = true; >>> pcmc->has_reserved_memory = false; >>> + pcmc->deprecate_64bit_cpu = true; >>> m->default_nic = "ne2k_isa"; >>> m->default_cpu_type = X86_CPU_TYPE_NAME("486"); >>> m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL); >> >> The logic around checking CPUID_EXT2_LM looks good to me. Slightly >> curious as to whether people feel updating PCMachineClass is >> necessary, or you can simply do qdev_get_machine() and use >> object_dynamic_cast() to see if the machine matches >> MACHINE_NAME("isapc") and warn that way? > > Why don't you simply pass it as a parameter from pc_init_isa() instead? > Or do the whole check in pc_init_isa() instead? Because the CPU isn't instantiated so we can't check the CPUID_EXT2_LM feature :/
diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst index 7b548519b5..345c35507f 100644 --- a/docs/about/deprecated.rst +++ b/docs/about/deprecated.rst @@ -208,6 +208,13 @@ is no longer packaged in any distro making it harder to run the ``check-tcg`` tests. Unless we can improve the testing situation there is a chance the code will bitrot without anyone noticing. +64-bit (x86_64) CPUs on the ``isapc`` machine (since 9.0) +''''''''''''''''''''''''''''''''''''''''''''''''''''''''' + +The ``isapc`` machine aims to emulate old PC machine without PCI was +generalized, so hardware available around 1995, before 64-bit intel +CPUs were produced. + System emulator machines ------------------------ diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h index 27a68071d7..2d202b9549 100644 --- a/include/hw/i386/pc.h +++ b/include/hw/i386/pc.h @@ -96,6 +96,7 @@ struct PCMachineClass { const char *default_south_bridge; /* Compat options: */ + bool deprecate_64bit_cpu; /* Specific to the 'isapc' machine */ /* Default CPU model version. See x86_cpu_set_default_version(). */ int default_cpu_version; diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 18ba076609..2e5b2efc33 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -182,7 +182,20 @@ static void pc_init1(MachineState *machine, const char *pci_type) } pc_machine_init_sgx_epc(pcms); + x86_cpus_init(x86ms, pcmc->default_cpu_version); + if (pcmc->deprecate_64bit_cpu) { + X86CPU *cpu = X86_CPU(first_cpu); + + if (cpu->env.features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) { + const char *cpu_type = object_get_typename(OBJECT(first_cpu)); + int cpu_len = strlen(cpu_type) - strlen(X86_CPU_TYPE_SUFFIX); + + warn_report("Use of 64-bit CPU '%.*s' is deprecated" + " on the ISA-only PC machine", + cpu_len, cpu_type); + } + } if (kvm_enabled()) { kvmclock_create(pcmc->kvmclock_create_always); @@ -918,6 +931,7 @@ static void isapc_machine_options(MachineClass *m) pcmc->gigabyte_align = false; pcmc->smbios_legacy_mode = true; pcmc->has_reserved_memory = false; + pcmc->deprecate_64bit_cpu = true; m->default_nic = "ne2k_isa"; m->default_cpu_type = X86_CPU_TYPE_NAME("486"); m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL);