The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst it is
possible to specify any CPU via -cpu on the command line, it makes no
sense to allow modern 64-bit CPUs to be used.

Restrict the isapc machine to the available 32-bit CPUs, taking care to
handle the case where if a user inadvertently uses -cpu max then the "best"
32-bit CPU is used (in this case the pentium3).

Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index c03324281b..5720b6b556 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj, int value, Error **errp)
 #ifdef CONFIG_ISAPC
 static void pc_init_isa(MachineState *machine)
 {
+    /*
+     * There is a small chance that someone unintentionally passes "-cpu max"
+     * for the isapc machine, which will provide a much more modern 32-bit
+     * CPU than would be expected for an ISA-era PC. If the "max" cpu type has
+     * been specified, choose the "best" 32-bit cpu possible which we consider
+     * be the pentium3 (deliberately choosing an Intel CPU given that the
+     * default 486 CPU for the isapc machine is also an Intel CPU).
+     */
+    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
+        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
+        warn_report("-cpu max is invalid for isapc machine, using pentium3");
+    }
+
     pc_init1(machine, NULL);
 }
 #endif
@@ -806,7 +819,19 @@ DEFINE_I440FX_MACHINE(2, 6);
 #ifdef CONFIG_ISAPC
 static void isapc_machine_options(MachineClass *m)
 {
+    static const char * const valid_cpu_types[] = {
+        X86_CPU_TYPE_NAME("486"),
+        X86_CPU_TYPE_NAME("athlon"),
+        X86_CPU_TYPE_NAME("kvm32"),
+        X86_CPU_TYPE_NAME("pentium"),
+        X86_CPU_TYPE_NAME("pentium2"),
+        X86_CPU_TYPE_NAME("pentium3"),
+        X86_CPU_TYPE_NAME("qemu32"),
+        X86_CPU_TYPE_NAME("max"),
+        NULL
+    };
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
+
     m->desc = "ISA-only PC";
     m->max_cpus = 1;
     m->option_rom_has_mr = true;
@@ -819,6 +844,7 @@ static void isapc_machine_options(MachineClass *m)
     pcmc->has_reserved_memory = false;
     m->default_nic = "ne2k_isa";
     m->default_cpu_type = X86_CPU_TYPE_NAME("486");
+    m->valid_cpu_types = valid_cpu_types;
     m->no_floppy = !module_object_class_by_name(TYPE_ISA_FDC);
     m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL);
 }
-- 
2.43.0

On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
> The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst it is
> possible to specify any CPU via -cpu on the command line, it makes no
> sense to allow modern 64-bit CPUs to be used.
> 
> Restrict the isapc machine to the available 32-bit CPUs, taking care to
> handle the case where if a user inadvertently uses -cpu max then the "best"
> 32-bit CPU is used (in this case the pentium3).
> 
> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index c03324281b..5720b6b556 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj, int value, Error **errp)
>   #ifdef CONFIG_ISAPC
>   static void pc_init_isa(MachineState *machine)
>   {
> +    /*
> +     * There is a small chance that someone unintentionally passes "-cpu max"
> +     * for the isapc machine, which will provide a much more modern 32-bit
> +     * CPU than would be expected for an ISA-era PC. If the "max" cpu type has
> +     * been specified, choose the "best" 32-bit cpu possible which we consider
> +     * be the pentium3 (deliberately choosing an Intel CPU given that the
> +     * default 486 CPU for the isapc machine is also an Intel CPU).
> +     */
> +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
> +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
> +        warn_report("-cpu max is invalid for isapc machine, using pentium3");
> +    }

Do we need to handle the case of "-cpu host"?

>       pc_init1(machine, NULL);
>   }
>   #endif
> @@ -806,7 +819,19 @@ DEFINE_I440FX_MACHINE(2, 6);
>   #ifdef CONFIG_ISAPC
>   static void isapc_machine_options(MachineClass *m)
>   {
> +    static const char * const valid_cpu_types[] = {
> +        X86_CPU_TYPE_NAME("486"),
> +        X86_CPU_TYPE_NAME("athlon"),
> +        X86_CPU_TYPE_NAME("kvm32"),
> +        X86_CPU_TYPE_NAME("pentium"),
> +        X86_CPU_TYPE_NAME("pentium2"),
> +        X86_CPU_TYPE_NAME("pentium3"),
> +        X86_CPU_TYPE_NAME("qemu32"),
> +        X86_CPU_TYPE_NAME("max"),
> +        NULL
> +    };
>       PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
> +
>       m->desc = "ISA-only PC";
>       m->max_cpus = 1;
>       m->option_rom_has_mr = true;
> @@ -819,6 +844,7 @@ static void isapc_machine_options(MachineClass *m)
>       pcmc->has_reserved_memory = false;
>       m->default_nic = "ne2k_isa";
>       m->default_cpu_type = X86_CPU_TYPE_NAME("486");
> +    m->valid_cpu_types = valid_cpu_types;
>       m->no_floppy = !module_object_class_by_name(TYPE_ISA_FDC);
>       m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL);
>   }

On 26/08/2025 08:25, Xiaoyao Li wrote:

> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
>> The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst it is
>> possible to specify any CPU via -cpu on the command line, it makes no
>> sense to allow modern 64-bit CPUs to be used.
>>
>> Restrict the isapc machine to the available 32-bit CPUs, taking care to
>> handle the case where if a user inadvertently uses -cpu max then the 
>> "best"
>> 32-bit CPU is used (in this case the pentium3).
>>
>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>> index c03324281b..5720b6b556 100644
>> --- a/hw/i386/pc_piix.c
>> +++ b/hw/i386/pc_piix.c
>> @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj, int 
>> value, Error **errp)
>>   #ifdef CONFIG_ISAPC
>>   static void pc_init_isa(MachineState *machine)
>>   {
>> +    /*
>> +     * There is a small chance that someone unintentionally passes "- 
>> cpu max"
>> +     * for the isapc machine, which will provide a much more modern 
>> 32-bit
>> +     * CPU than would be expected for an ISA-era PC. If the "max" cpu 
>> type has
>> +     * been specified, choose the "best" 32-bit cpu possible which we 
>> consider
>> +     * be the pentium3 (deliberately choosing an Intel CPU given that 
>> the
>> +     * default 486 CPU for the isapc machine is also an Intel CPU).
>> +     */
>> +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
>> +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
>> +        warn_report("-cpu max is invalid for isapc machine, using 
>> pentium3");
>> +    }
> 
> Do we need to handle the case of "-cpu host"?

I don't believe so. I wasn't originally planning to support "-cpu max" 
for isapc, however Daniel mentioned that it could possibly be generated 
from libvirt so it makes sense to add the above check to warn in this 
case and then continue.

>>       pc_init1(machine, NULL);
>>   }
>>   #endif
>> @@ -806,7 +819,19 @@ DEFINE_I440FX_MACHINE(2, 6);
>>   #ifdef CONFIG_ISAPC
>>   static void isapc_machine_options(MachineClass *m)
>>   {
>> +    static const char * const valid_cpu_types[] = {
>> +        X86_CPU_TYPE_NAME("486"),
>> +        X86_CPU_TYPE_NAME("athlon"),
>> +        X86_CPU_TYPE_NAME("kvm32"),
>> +        X86_CPU_TYPE_NAME("pentium"),
>> +        X86_CPU_TYPE_NAME("pentium2"),
>> +        X86_CPU_TYPE_NAME("pentium3"),
>> +        X86_CPU_TYPE_NAME("qemu32"),
>> +        X86_CPU_TYPE_NAME("max"),
>> +        NULL
>> +    };
>>       PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>> +
>>       m->desc = "ISA-only PC";
>>       m->max_cpus = 1;
>>       m->option_rom_has_mr = true;
>> @@ -819,6 +844,7 @@ static void isapc_machine_options(MachineClass *m)
>>       pcmc->has_reserved_memory = false;
>>       m->default_nic = "ne2k_isa";
>>       m->default_cpu_type = X86_CPU_TYPE_NAME("486");
>> +    m->valid_cpu_types = valid_cpu_types;
>>       m->no_floppy = !module_object_class_by_name(TYPE_ISA_FDC);
>>       m->no_parallel = !module_object_class_by_name(TYPE_ISA_PARALLEL);
>>   }


ATB,

Mark.

On 8/27/2025 7:10 PM, Mark Cave-Ayland wrote:
> On 26/08/2025 08:25, Xiaoyao Li wrote:
> 
>> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
>>> The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst 
>>> it is
>>> possible to specify any CPU via -cpu on the command line, it makes no
>>> sense to allow modern 64-bit CPUs to be used.
>>>
>>> Restrict the isapc machine to the available 32-bit CPUs, taking care to
>>> handle the case where if a user inadvertently uses -cpu max then the 
>>> "best"
>>> 32-bit CPU is used (in this case the pentium3).
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>>   hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
>>>   1 file changed, 26 insertions(+)
>>>
>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>> index c03324281b..5720b6b556 100644
>>> --- a/hw/i386/pc_piix.c
>>> +++ b/hw/i386/pc_piix.c
>>> @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj, int 
>>> value, Error **errp)
>>>   #ifdef CONFIG_ISAPC
>>>   static void pc_init_isa(MachineState *machine)
>>>   {
>>> +    /*
>>> +     * There is a small chance that someone unintentionally passes 
>>> "- cpu max"
>>> +     * for the isapc machine, which will provide a much more modern 
>>> 32-bit
>>> +     * CPU than would be expected for an ISA-era PC. If the "max" 
>>> cpu type has
>>> +     * been specified, choose the "best" 32-bit cpu possible which 
>>> we consider
>>> +     * be the pentium3 (deliberately choosing an Intel CPU given 
>>> that the
>>> +     * default 486 CPU for the isapc machine is also an Intel CPU).
>>> +     */
>>> +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
>>> +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
>>> +        warn_report("-cpu max is invalid for isapc machine, using 
>>> pentium3");
>>> +    }
>>
>> Do we need to handle the case of "-cpu host"?
> 
> I don't believe so. I wasn't originally planning to support "-cpu max" 
> for isapc, however Daniel mentioned that it could possibly be generated 
> from libvirt so it makes sense to add the above check to warn in this 
> case and then continue.

"host" cpu type is the child of "max", so "-cpu host" will pass in the 
is_cpu_type_supported(), the same as "max".

While we are changing "max" to "pentium3", I think it needs to do the 
same for "host". Otherwise, "-cpu host" won't get any warning and expose 
the native features to the VMs that are mostly not supposed to exist for 
isapc, e.g., the LM CUPID bit.

On 27/08/2025 12:50, Xiaoyao Li wrote:

> On 8/27/2025 7:10 PM, Mark Cave-Ayland wrote:
>> On 26/08/2025 08:25, Xiaoyao Li wrote:
>>
>>> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
>>>> The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst 
>>>> it is
>>>> possible to specify any CPU via -cpu on the command line, it makes no
>>>> sense to allow modern 64-bit CPUs to be used.
>>>>
>>>> Restrict the isapc machine to the available 32-bit CPUs, taking care to
>>>> handle the case where if a user inadvertently uses -cpu max then the 
>>>> "best"
>>>> 32-bit CPU is used (in this case the pentium3).
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>   hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
>>>>   1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>> index c03324281b..5720b6b556 100644
>>>> --- a/hw/i386/pc_piix.c
>>>> +++ b/hw/i386/pc_piix.c
>>>> @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj, 
>>>> int value, Error **errp)
>>>>   #ifdef CONFIG_ISAPC
>>>>   static void pc_init_isa(MachineState *machine)
>>>>   {
>>>> +    /*
>>>> +     * There is a small chance that someone unintentionally passes 
>>>> "- cpu max"
>>>> +     * for the isapc machine, which will provide a much more modern 
>>>> 32-bit
>>>> +     * CPU than would be expected for an ISA-era PC. If the "max" 
>>>> cpu type has
>>>> +     * been specified, choose the "best" 32-bit cpu possible which 
>>>> we consider
>>>> +     * be the pentium3 (deliberately choosing an Intel CPU given 
>>>> that the
>>>> +     * default 486 CPU for the isapc machine is also an Intel CPU).
>>>> +     */
>>>> +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
>>>> +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
>>>> +        warn_report("-cpu max is invalid for isapc machine, using 
>>>> pentium3");
>>>> +    }
>>>
>>> Do we need to handle the case of "-cpu host"?
>>
>> I don't believe so. I wasn't originally planning to support "-cpu max" 
>> for isapc, however Daniel mentioned that it could possibly be 
>> generated from libvirt so it makes sense to add the above check to 
>> warn in this case and then continue.
> 
> "host" cpu type is the child of "max", so "-cpu host" will pass in the 
> is_cpu_type_supported(), the same as "max".
> 
> While we are changing "max" to "pentium3", I think it needs to do the 
> same for "host". Otherwise, "-cpu host" won't get any warning and expose 
> the native features to the VMs that are mostly not supposed to exist for 
> isapc, e.g., the LM CUPID bit.

That makes sense given that for compatibility we would want to hide more 
modern features from the guest, particularly if running legacy OSs.

I'll make this change and likely post a v7 soon as there are quite a lot 
of changes here already, and wait for any further input from Jiri or Daniel.


ATB,

Mark.

On 28/8/25 12:42, Mark Cave-Ayland wrote:
> On 27/08/2025 12:50, Xiaoyao Li wrote:
> 
>> On 8/27/2025 7:10 PM, Mark Cave-Ayland wrote:
>>> On 26/08/2025 08:25, Xiaoyao Li wrote:
>>>
>>>> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
>>>>> The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst 
>>>>> it is
>>>>> possible to specify any CPU via -cpu on the command line, it makes no
>>>>> sense to allow modern 64-bit CPUs to be used.
>>>>>
>>>>> Restrict the isapc machine to the available 32-bit CPUs, taking 
>>>>> care to
>>>>> handle the case where if a user inadvertently uses -cpu max then 
>>>>> the "best"
>>>>> 32-bit CPU is used (in this case the pentium3).
>>>>>
>>>>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>> ---
>>>>>   hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
>>>>>   1 file changed, 26 insertions(+)
>>>>>
>>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>>> index c03324281b..5720b6b556 100644
>>>>> --- a/hw/i386/pc_piix.c
>>>>> +++ b/hw/i386/pc_piix.c
>>>>> @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj, 
>>>>> int value, Error **errp)
>>>>>   #ifdef CONFIG_ISAPC
>>>>>   static void pc_init_isa(MachineState *machine)
>>>>>   {
>>>>> +    /*
>>>>> +     * There is a small chance that someone unintentionally passes 
>>>>> "- cpu max"
>>>>> +     * for the isapc machine, which will provide a much more 
>>>>> modern 32-bit
>>>>> +     * CPU than would be expected for an ISA-era PC. If the "max" 
>>>>> cpu type has
>>>>> +     * been specified, choose the "best" 32-bit cpu possible which 
>>>>> we consider
>>>>> +     * be the pentium3 (deliberately choosing an Intel CPU given 
>>>>> that the
>>>>> +     * default 486 CPU for the isapc machine is also an Intel CPU).
>>>>> +     */
>>>>> +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
>>>>> +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
>>>>> +        warn_report("-cpu max is invalid for isapc machine, using 
>>>>> pentium3");
>>>>> +    }
>>>>
>>>> Do we need to handle the case of "-cpu host"?

"host" is not in valid_cpu_types[].

>>>
>>> I don't believe so. I wasn't originally planning to support "-cpu 
>>> max" for isapc, however Daniel mentioned that it could possibly be 
>>> generated from libvirt so it makes sense to add the above check to 
>>> warn in this case and then continue.
>>
>> "host" cpu type is the child of "max", so "-cpu host" will pass in the 
>> is_cpu_type_supported(), the same as "max".
>>
>> While we are changing "max" to "pentium3", I think it needs to do the 
>> same for "host". Otherwise, "-cpu host" won't get any warning and 
>> expose the native features to the VMs that are mostly not supposed to 
>> exist for isapc, e.g., the LM CUPID bit.
> 
> That makes sense given that for compatibility we would want to hide more 
> modern features from the guest, particularly if running legacy OSs.
> 
> I'll make this change and likely post a v7 soon as there are quite a lot 
> of changes here already, and wait for any further input from Jiri or 
> Daniel.
> 
> 
> ATB,
> 
> Mark.
> 
> 

On 22/09/2025 13:01, Philippe Mathieu-Daudé wrote:

> On 28/8/25 12:42, Mark Cave-Ayland wrote:
>> On 27/08/2025 12:50, Xiaoyao Li wrote:
>>
>>> On 8/27/2025 7:10 PM, Mark Cave-Ayland wrote:
>>>> On 26/08/2025 08:25, Xiaoyao Li wrote:
>>>>
>>>>> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
>>>>>> The isapc machine represents a legacy ISA PC with a 486 CPU. 
>>>>>> Whilst it is
>>>>>> possible to specify any CPU via -cpu on the command line, it makes no
>>>>>> sense to allow modern 64-bit CPUs to be used.
>>>>>>
>>>>>> Restrict the isapc machine to the available 32-bit CPUs, taking 
>>>>>> care to
>>>>>> handle the case where if a user inadvertently uses -cpu max then 
>>>>>> the "best"
>>>>>> 32-bit CPU is used (in this case the pentium3).
>>>>>>
>>>>>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>> ---
>>>>>>   hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
>>>>>>   1 file changed, 26 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>>>> index c03324281b..5720b6b556 100644
>>>>>> --- a/hw/i386/pc_piix.c
>>>>>> +++ b/hw/i386/pc_piix.c
>>>>>> @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj, 
>>>>>> int value, Error **errp)
>>>>>>   #ifdef CONFIG_ISAPC
>>>>>>   static void pc_init_isa(MachineState *machine)
>>>>>>   {
>>>>>> +    /*
>>>>>> +     * There is a small chance that someone unintentionally 
>>>>>> passes "- cpu max"
>>>>>> +     * for the isapc machine, which will provide a much more 
>>>>>> modern 32-bit
>>>>>> +     * CPU than would be expected for an ISA-era PC. If the "max" 
>>>>>> cpu type has
>>>>>> +     * been specified, choose the "best" 32-bit cpu possible 
>>>>>> which we consider
>>>>>> +     * be the pentium3 (deliberately choosing an Intel CPU given 
>>>>>> that the
>>>>>> +     * default 486 CPU for the isapc machine is also an Intel CPU).
>>>>>> +     */
>>>>>> +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
>>>>>> +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
>>>>>> +        warn_report("-cpu max is invalid for isapc machine, using 
>>>>>> pentium3");
>>>>>> +    }
>>>>>
>>>>> Do we need to handle the case of "-cpu host"?
> 
> "host" is not in valid_cpu_types[].

Not in v6, but I added it for v7.

>>>>
>>>> I don't believe so. I wasn't originally planning to support "-cpu 
>>>> max" for isapc, however Daniel mentioned that it could possibly be 
>>>> generated from libvirt so it makes sense to add the above check to 
>>>> warn in this case and then continue.
>>>
>>> "host" cpu type is the child of "max", so "-cpu host" will pass in 
>>> the is_cpu_type_supported(), the same as "max".
>>>
>>> While we are changing "max" to "pentium3", I think it needs to do the 
>>> same for "host". Otherwise, "-cpu host" won't get any warning and 
>>> expose the native features to the VMs that are mostly not supposed to 
>>> exist for isapc, e.g., the LM CUPID bit.
>>
>> That makes sense given that for compatibility we would want to hide 
>> more modern features from the guest, particularly if running legacy OSs.
>>
>> I'll make this change and likely post a v7 soon as there are quite a 
>> lot of changes here already, and wait for any further input from Jiri 
>> or Daniel.


ATB,

Mark.

On Wed, Aug 27, 2025 at 12:10:00PM +0100, Mark Cave-Ayland wrote:
> On 26/08/2025 08:25, Xiaoyao Li wrote:
> 
> > On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
> > > The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst it is
> > > possible to specify any CPU via -cpu on the command line, it makes no
> > > sense to allow modern 64-bit CPUs to be used.
> > > 
> > > Restrict the isapc machine to the available 32-bit CPUs, taking care to
> > > handle the case where if a user inadvertently uses -cpu max then the
> > > "best"
> > > 32-bit CPU is used (in this case the pentium3).
> > > 
> > > Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > >   hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
> > >   1 file changed, 26 insertions(+)
> > > 
> > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > index c03324281b..5720b6b556 100644
> > > --- a/hw/i386/pc_piix.c
> > > +++ b/hw/i386/pc_piix.c
> > > @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj,
> > > int value, Error **errp)
> > >   #ifdef CONFIG_ISAPC
> > >   static void pc_init_isa(MachineState *machine)
> > >   {
> > > +    /*
> > > +     * There is a small chance that someone unintentionally passes
> > > "- cpu max"
> > > +     * for the isapc machine, which will provide a much more modern
> > > 32-bit
> > > +     * CPU than would be expected for an ISA-era PC. If the "max"
> > > cpu type has
> > > +     * been specified, choose the "best" 32-bit cpu possible which
> > > we consider
> > > +     * be the pentium3 (deliberately choosing an Intel CPU given
> > > that the
> > > +     * default 486 CPU for the isapc machine is also an Intel CPU).
> > > +     */
> > > +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
> > > +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
> > > +        warn_report("-cpu max is invalid for isapc machine, using
> > > pentium3");
> > > +    }
> > 
> > Do we need to handle the case of "-cpu host"?
> 
> I don't believe so. I wasn't originally planning to support "-cpu max" for
> isapc, however Daniel mentioned that it could possibly be generated from
> libvirt so it makes sense to add the above check to warn in this case and
> then continue.

Libvirt will support sending any valid -cpu flag, including both
'max' (any config) and 'host' (if KVM).

If 'isapc' still expects to support KVM, then it would be odd to
reject 'host', but KVM presumably has no built-in way to limit to
32-bit without QEMU manually masking many features ?

I'm a little worried about implications of libvirt sending '-cpu max'
and QEMU secretly turning that into '-cpu pentium3', as opposed to
having '-cpu max' expand to equiv to 'pentium3', which might cauase
confusion when libvirt queries the expanded CPU ? Copying Jiri for
an opinion from libvirt side, as I might be worrying about nothing.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On 27/8/25 13:46, Daniel P. Berrangé wrote:
> On Wed, Aug 27, 2025 at 12:10:00PM +0100, Mark Cave-Ayland wrote:
>> On 26/08/2025 08:25, Xiaoyao Li wrote:
>>
>>> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
>>>> The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst it is
>>>> possible to specify any CPU via -cpu on the command line, it makes no
>>>> sense to allow modern 64-bit CPUs to be used.
>>>>
>>>> Restrict the isapc machine to the available 32-bit CPUs, taking care to
>>>> handle the case where if a user inadvertently uses -cpu max then the
>>>> "best"
>>>> 32-bit CPU is used (in this case the pentium3).
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>    hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
>>>>    1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>> index c03324281b..5720b6b556 100644
>>>> --- a/hw/i386/pc_piix.c
>>>> +++ b/hw/i386/pc_piix.c
>>>> @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj,
>>>> int value, Error **errp)
>>>>    #ifdef CONFIG_ISAPC
>>>>    static void pc_init_isa(MachineState *machine)
>>>>    {
>>>> +    /*
>>>> +     * There is a small chance that someone unintentionally passes
>>>> "- cpu max"
>>>> +     * for the isapc machine, which will provide a much more modern
>>>> 32-bit
>>>> +     * CPU than would be expected for an ISA-era PC. If the "max"
>>>> cpu type has
>>>> +     * been specified, choose the "best" 32-bit cpu possible which
>>>> we consider
>>>> +     * be the pentium3 (deliberately choosing an Intel CPU given
>>>> that the
>>>> +     * default 486 CPU for the isapc machine is also an Intel CPU).
>>>> +     */
>>>> +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
>>>> +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
>>>> +        warn_report("-cpu max is invalid for isapc machine, using
>>>> pentium3");
>>>> +    }
>>>
>>> Do we need to handle the case of "-cpu host"?
>>
>> I don't believe so. I wasn't originally planning to support "-cpu max" for
>> isapc, however Daniel mentioned that it could possibly be generated from
>> libvirt so it makes sense to add the above check to warn in this case and
>> then continue.
> 
> Libvirt will support sending any valid -cpu flag, including both
> 'max' (any config) and 'host' (if KVM).
> 
> If 'isapc' still expects to support KVM, then it would be odd to
> reject 'host', but KVM presumably has no built-in way to limit to
> 32-bit without QEMU manually masking many features ?
> 
> I'm a little worried about implications of libvirt sending '-cpu max'
> and QEMU secretly turning that into '-cpu pentium3', as opposed to
> having '-cpu max' expand to equiv to 'pentium3', which might cauase
> confusion when libvirt queries the expanded CPU ? Copying Jiri for
> an opinion from libvirt side, as I might be worrying about nothing.

OK, on 2nd thought, even while warning the user, changing the type
under the hood isn't great.

What about simply removing "max" of valid_cpu_types[], since it is
clearly confusing "max" == "pentium3"...

On Mon, Sep 22, 2025 at 02:05:13PM +0200, Philippe Mathieu-Daudé wrote:
> On 27/8/25 13:46, Daniel P. Berrangé wrote:
> > On Wed, Aug 27, 2025 at 12:10:00PM +0100, Mark Cave-Ayland wrote:
> > > On 26/08/2025 08:25, Xiaoyao Li wrote:
> > > 
> > > > On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
> > > > > The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst it is
> > > > > possible to specify any CPU via -cpu on the command line, it makes no
> > > > > sense to allow modern 64-bit CPUs to be used.
> > > > > 
> > > > > Restrict the isapc machine to the available 32-bit CPUs, taking care to
> > > > > handle the case where if a user inadvertently uses -cpu max then the
> > > > > "best"
> > > > > 32-bit CPU is used (in this case the pentium3).
> > > > > 
> > > > > Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> > > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > > ---
> > > > >    hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
> > > > >    1 file changed, 26 insertions(+)
> > > > > 
> > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > > > index c03324281b..5720b6b556 100644
> > > > > --- a/hw/i386/pc_piix.c
> > > > > +++ b/hw/i386/pc_piix.c
> > > > > @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj,
> > > > > int value, Error **errp)
> > > > >    #ifdef CONFIG_ISAPC
> > > > >    static void pc_init_isa(MachineState *machine)
> > > > >    {
> > > > > +    /*
> > > > > +     * There is a small chance that someone unintentionally passes
> > > > > "- cpu max"
> > > > > +     * for the isapc machine, which will provide a much more modern
> > > > > 32-bit
> > > > > +     * CPU than would be expected for an ISA-era PC. If the "max"
> > > > > cpu type has
> > > > > +     * been specified, choose the "best" 32-bit cpu possible which
> > > > > we consider
> > > > > +     * be the pentium3 (deliberately choosing an Intel CPU given
> > > > > that the
> > > > > +     * default 486 CPU for the isapc machine is also an Intel CPU).
> > > > > +     */
> > > > > +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
> > > > > +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
> > > > > +        warn_report("-cpu max is invalid for isapc machine, using
> > > > > pentium3");
> > > > > +    }
> > > > 
> > > > Do we need to handle the case of "-cpu host"?
> > > 
> > > I don't believe so. I wasn't originally planning to support "-cpu max" for
> > > isapc, however Daniel mentioned that it could possibly be generated from
> > > libvirt so it makes sense to add the above check to warn in this case and
> > > then continue.
> > 
> > Libvirt will support sending any valid -cpu flag, including both
> > 'max' (any config) and 'host' (if KVM).
> > 
> > If 'isapc' still expects to support KVM, then it would be odd to
> > reject 'host', but KVM presumably has no built-in way to limit to
> > 32-bit without QEMU manually masking many features ?
> > 
> > I'm a little worried about implications of libvirt sending '-cpu max'
> > and QEMU secretly turning that into '-cpu pentium3', as opposed to
> > having '-cpu max' expand to equiv to 'pentium3', which might cauase
> > confusion when libvirt queries the expanded CPU ? Copying Jiri for
> > an opinion from libvirt side, as I might be worrying about nothing.
> 
> OK, on 2nd thought, even while warning the user, changing the type
> under the hood isn't great.
> 
> What about simply removing "max" of valid_cpu_types[], since it is
> clearly confusing "max" == "pentium3"...

The goal with "max" was to provide a common CPU model that would
"just work" on any target. It was always likely to show up wierd
edge cases, but the fewer edge cases we can have in that story
the better.

In particular if

  qemu-system-i386 -cpu max -machine isapc

does the right thing (giving the best 32-bit CPU we can emulate),
then we need a story for how that should work with the single
binary model.

Conceptually I think we're saying that 'isapc' is only relevant
for qemu-system-i386 and not qemu-system-x86_64, because we only
want to support 32-bit for it. I'm not sure how we express that
restriction in a combined binary world ? Do we filter machine
types based on i386 vs x86_64 target arch ?

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Mon, 22 Sep 2025 14:05:13 +0200
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:

> On 27/8/25 13:46, Daniel P. Berrangé wrote:
> > On Wed, Aug 27, 2025 at 12:10:00PM +0100, Mark Cave-Ayland wrote:  
> >> On 26/08/2025 08:25, Xiaoyao Li wrote:
> >>  
> >>> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:  
> >>>> The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst it is
> >>>> possible to specify any CPU via -cpu on the command line, it makes no
> >>>> sense to allow modern 64-bit CPUs to be used.
> >>>>
> >>>> Restrict the isapc machine to the available 32-bit CPUs, taking care to
> >>>> handle the case where if a user inadvertently uses -cpu max then the
> >>>> "best"
> >>>> 32-bit CPU is used (in this case the pentium3).
> >>>>
> >>>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> >>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>>> ---
> >>>>    hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
> >>>>    1 file changed, 26 insertions(+)
> >>>>
> >>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >>>> index c03324281b..5720b6b556 100644
> >>>> --- a/hw/i386/pc_piix.c
> >>>> +++ b/hw/i386/pc_piix.c
> >>>> @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj,
> >>>> int value, Error **errp)
> >>>>    #ifdef CONFIG_ISAPC
> >>>>    static void pc_init_isa(MachineState *machine)
> >>>>    {
> >>>> +    /*
> >>>> +     * There is a small chance that someone unintentionally passes
> >>>> "- cpu max"
> >>>> +     * for the isapc machine, which will provide a much more modern
> >>>> 32-bit
> >>>> +     * CPU than would be expected for an ISA-era PC. If the "max"
> >>>> cpu type has
> >>>> +     * been specified, choose the "best" 32-bit cpu possible which
> >>>> we consider
> >>>> +     * be the pentium3 (deliberately choosing an Intel CPU given
> >>>> that the
> >>>> +     * default 486 CPU for the isapc machine is also an Intel CPU).
> >>>> +     */
> >>>> +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
> >>>> +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
> >>>> +        warn_report("-cpu max is invalid for isapc machine, using
> >>>> pentium3");
> >>>> +    }  
> >>>
> >>> Do we need to handle the case of "-cpu host"?  
> >>
> >> I don't believe so. I wasn't originally planning to support "-cpu max" for
> >> isapc, however Daniel mentioned that it could possibly be generated from
> >> libvirt so it makes sense to add the above check to warn in this case and
> >> then continue.  
> > 
> > Libvirt will support sending any valid -cpu flag, including both
> > 'max' (any config) and 'host' (if KVM).
> > 
> > If 'isapc' still expects to support KVM, then it would be odd to
> > reject 'host', but KVM presumably has no built-in way to limit to
> > 32-bit without QEMU manually masking many features ?
> > 
> > I'm a little worried about implications of libvirt sending '-cpu max'
> > and QEMU secretly turning that into '-cpu pentium3', as opposed to
> > having '-cpu max' expand to equiv to 'pentium3', which might cauase
> > confusion when libvirt queries the expanded CPU ? Copying Jiri for
> > an opinion from libvirt side, as I might be worrying about nothing.  
> 
> OK, on 2nd thought, even while warning the user, changing the type
> under the hood isn't great.

I second that,
Please don't do magical mutations of CPUs, just error out.

we used to 'fix|tweak' CPUs using machine compat hack,
however with introduction of versioned cpu models we shouldn't do that anymore.
(aka: existing CPU devices should stay immutable if possible, and any visible
changes should go into new version)


> What about simply removing "max" of valid_cpu_types[], since it is
> clearly confusing "max" == "pentium3"...

it seems that specifying supported cpu models in valid_cpu_types[] is the way to go.

On 22/09/2025 13:35, Igor Mammedov wrote:

> On Mon, 22 Sep 2025 14:05:13 +0200
> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
>> On 27/8/25 13:46, Daniel P. Berrangé wrote:
>>> On Wed, Aug 27, 2025 at 12:10:00PM +0100, Mark Cave-Ayland wrote:
>>>> On 26/08/2025 08:25, Xiaoyao Li wrote:
>>>>   
>>>>> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
>>>>>> The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst it is
>>>>>> possible to specify any CPU via -cpu on the command line, it makes no
>>>>>> sense to allow modern 64-bit CPUs to be used.
>>>>>>
>>>>>> Restrict the isapc machine to the available 32-bit CPUs, taking care to
>>>>>> handle the case where if a user inadvertently uses -cpu max then the
>>>>>> "best"
>>>>>> 32-bit CPU is used (in this case the pentium3).
>>>>>>
>>>>>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>> ---
>>>>>>     hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
>>>>>>     1 file changed, 26 insertions(+)
>>>>>>
>>>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>>>> index c03324281b..5720b6b556 100644
>>>>>> --- a/hw/i386/pc_piix.c
>>>>>> +++ b/hw/i386/pc_piix.c
>>>>>> @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj,
>>>>>> int value, Error **errp)
>>>>>>     #ifdef CONFIG_ISAPC
>>>>>>     static void pc_init_isa(MachineState *machine)
>>>>>>     {
>>>>>> +    /*
>>>>>> +     * There is a small chance that someone unintentionally passes
>>>>>> "- cpu max"
>>>>>> +     * for the isapc machine, which will provide a much more modern
>>>>>> 32-bit
>>>>>> +     * CPU than would be expected for an ISA-era PC. If the "max"
>>>>>> cpu type has
>>>>>> +     * been specified, choose the "best" 32-bit cpu possible which
>>>>>> we consider
>>>>>> +     * be the pentium3 (deliberately choosing an Intel CPU given
>>>>>> that the
>>>>>> +     * default 486 CPU for the isapc machine is also an Intel CPU).
>>>>>> +     */
>>>>>> +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
>>>>>> +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
>>>>>> +        warn_report("-cpu max is invalid for isapc machine, using
>>>>>> pentium3");
>>>>>> +    }
>>>>>
>>>>> Do we need to handle the case of "-cpu host"?
>>>>
>>>> I don't believe so. I wasn't originally planning to support "-cpu max" for
>>>> isapc, however Daniel mentioned that it could possibly be generated from
>>>> libvirt so it makes sense to add the above check to warn in this case and
>>>> then continue.
>>>
>>> Libvirt will support sending any valid -cpu flag, including both
>>> 'max' (any config) and 'host' (if KVM).
>>>
>>> If 'isapc' still expects to support KVM, then it would be odd to
>>> reject 'host', but KVM presumably has no built-in way to limit to
>>> 32-bit without QEMU manually masking many features ?
>>>
>>> I'm a little worried about implications of libvirt sending '-cpu max'
>>> and QEMU secretly turning that into '-cpu pentium3', as opposed to
>>> having '-cpu max' expand to equiv to 'pentium3', which might cauase
>>> confusion when libvirt queries the expanded CPU ? Copying Jiri for
>>> an opinion from libvirt side, as I might be worrying about nothing.
>>
>> OK, on 2nd thought, even while warning the user, changing the type
>> under the hood isn't great.
> 
> I second that,
> Please don't do magical mutations of CPUs, just error out.
> 
> we used to 'fix|tweak' CPUs using machine compat hack,
> however with introduction of versioned cpu models we shouldn't do that anymore.
> (aka: existing CPU devices should stay immutable if possible, and any visible
> changes should go into new version)

The original suggestion for allowing "max"/"host" was so that it 
wouldn't cause any regressions with command lines erroneously including 
-cpu max or -cpu host (which I believe may be possible with libvirt).

>> What about simply removing "max" of valid_cpu_types[], since it is
>> clearly confusing "max" == "pentium3"...
> 
> it seems that specifying supported cpu models in valid_cpu_types[] is the way to go.

That was what I did in v1 and v2 version of the series, but I can submit 
a patch to change this once there is agreement on the desired behaviour.


ATB,

Mark.

On Mon, 22 Sep 2025 14:56:57 +0100
Mark Cave-Ayland <mark.caveayland@nutanix.com> wrote:

> On 22/09/2025 13:35, Igor Mammedov wrote:
> 
> > On Mon, 22 Sep 2025 14:05:13 +0200
> > Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >   
> >> On 27/8/25 13:46, Daniel P. Berrangé wrote:  
> >>> On Wed, Aug 27, 2025 at 12:10:00PM +0100, Mark Cave-Ayland wrote:  
> >>>> On 26/08/2025 08:25, Xiaoyao Li wrote:
> >>>>     
> >>>>> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:  
> >>>>>> The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst it is
> >>>>>> possible to specify any CPU via -cpu on the command line, it makes no
> >>>>>> sense to allow modern 64-bit CPUs to be used.
> >>>>>>
> >>>>>> Restrict the isapc machine to the available 32-bit CPUs, taking care to
> >>>>>> handle the case where if a user inadvertently uses -cpu max then the
> >>>>>> "best"
> >>>>>> 32-bit CPU is used (in this case the pentium3).
> >>>>>>
> >>>>>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> >>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>>>>> ---
> >>>>>>     hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
> >>>>>>     1 file changed, 26 insertions(+)
> >>>>>>
> >>>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >>>>>> index c03324281b..5720b6b556 100644
> >>>>>> --- a/hw/i386/pc_piix.c
> >>>>>> +++ b/hw/i386/pc_piix.c
> >>>>>> @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj,
> >>>>>> int value, Error **errp)
> >>>>>>     #ifdef CONFIG_ISAPC
> >>>>>>     static void pc_init_isa(MachineState *machine)
> >>>>>>     {
> >>>>>> +    /*
> >>>>>> +     * There is a small chance that someone unintentionally passes
> >>>>>> "- cpu max"
> >>>>>> +     * for the isapc machine, which will provide a much more modern
> >>>>>> 32-bit
> >>>>>> +     * CPU than would be expected for an ISA-era PC. If the "max"
> >>>>>> cpu type has
> >>>>>> +     * been specified, choose the "best" 32-bit cpu possible which
> >>>>>> we consider
> >>>>>> +     * be the pentium3 (deliberately choosing an Intel CPU given
> >>>>>> that the
> >>>>>> +     * default 486 CPU for the isapc machine is also an Intel CPU).
> >>>>>> +     */
> >>>>>> +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
> >>>>>> +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
> >>>>>> +        warn_report("-cpu max is invalid for isapc machine, using
> >>>>>> pentium3");
> >>>>>> +    }  
> >>>>>
> >>>>> Do we need to handle the case of "-cpu host"?  
> >>>>
> >>>> I don't believe so. I wasn't originally planning to support "-cpu max" for
> >>>> isapc, however Daniel mentioned that it could possibly be generated from
> >>>> libvirt so it makes sense to add the above check to warn in this case and
> >>>> then continue.  
> >>>
> >>> Libvirt will support sending any valid -cpu flag, including both
> >>> 'max' (any config) and 'host' (if KVM).
> >>>
> >>> If 'isapc' still expects to support KVM, then it would be odd to
> >>> reject 'host', but KVM presumably has no built-in way to limit to
> >>> 32-bit without QEMU manually masking many features ?
> >>>
> >>> I'm a little worried about implications of libvirt sending '-cpu max'
> >>> and QEMU secretly turning that into '-cpu pentium3', as opposed to
> >>> having '-cpu max' expand to equiv to 'pentium3', which might cauase
> >>> confusion when libvirt queries the expanded CPU ? Copying Jiri for
> >>> an opinion from libvirt side, as I might be worrying about nothing.  
> >>
> >> OK, on 2nd thought, even while warning the user, changing the type
> >> under the hood isn't great.  
> > 
> > I second that,
> > Please don't do magical mutations of CPUs, just error out.
> > 
> > we used to 'fix|tweak' CPUs using machine compat hack,
> > however with introduction of versioned cpu models we shouldn't do that anymore.
> > (aka: existing CPU devices should stay immutable if possible, and any visible
> > changes should go into new version)  
> 
> The original suggestion for allowing "max"/"host" was so that it 
> wouldn't cause any regressions with command lines erroneously including 
> -cpu max or -cpu host (which I believe may be possible with libvirt).

looking back and at Daniels reply,
max/host are indeed are 'special' aka mutable as opposed to named cpu models.

if we go by the books, 'host' and by extension 'max' should work with KVM accelerator.
But that (aka reducing it to isapc levels) should be done at 'host' cpu model code
and that part of code is not really aware (nor should be) of machine types.
I'm not sure, whether it's worth the effort and complexity.

I'd be fine with valid_cpu_types[] approach here, i.e. user will get
clear error that her is doing wrong thing trying 'host/max',
and printing suggestion how to remedy error should guide user
to the right config.
 
> 
> >> What about simply removing "max" of valid_cpu_types[], since it is
> >> clearly confusing "max" == "pentium3"...  
> > 
> > it seems that specifying supported cpu models in valid_cpu_types[] is the way to go.  
> 
> That was what I did in v1 and v2 version of the series, but I can submit 
> a patch to change this once there is agreement on the desired behaviour.
> 
> 
> ATB,
> 
> Mark.
> 

On 23/09/2025 10:30, Igor Mammedov wrote:

> On Mon, 22 Sep 2025 14:56:57 +0100
> Mark Cave-Ayland <mark.caveayland@nutanix.com> wrote:
> 
>> On 22/09/2025 13:35, Igor Mammedov wrote:
>>
>>> On Mon, 22 Sep 2025 14:05:13 +0200
>>> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>    
>>>> On 27/8/25 13:46, Daniel P. Berrangé wrote:
>>>>> On Wed, Aug 27, 2025 at 12:10:00PM +0100, Mark Cave-Ayland wrote:
>>>>>> On 26/08/2025 08:25, Xiaoyao Li wrote:
>>>>>>      
>>>>>>> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
>>>>>>>> The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst it is
>>>>>>>> possible to specify any CPU via -cpu on the command line, it makes no
>>>>>>>> sense to allow modern 64-bit CPUs to be used.
>>>>>>>>
>>>>>>>> Restrict the isapc machine to the available 32-bit CPUs, taking care to
>>>>>>>> handle the case where if a user inadvertently uses -cpu max then the
>>>>>>>> "best"
>>>>>>>> 32-bit CPU is used (in this case the pentium3).
>>>>>>>>
>>>>>>>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
>>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>>> ---
>>>>>>>>      hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
>>>>>>>>      1 file changed, 26 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>>>>>>>> index c03324281b..5720b6b556 100644
>>>>>>>> --- a/hw/i386/pc_piix.c
>>>>>>>> +++ b/hw/i386/pc_piix.c
>>>>>>>> @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj,
>>>>>>>> int value, Error **errp)
>>>>>>>>      #ifdef CONFIG_ISAPC
>>>>>>>>      static void pc_init_isa(MachineState *machine)
>>>>>>>>      {
>>>>>>>> +    /*
>>>>>>>> +     * There is a small chance that someone unintentionally passes
>>>>>>>> "- cpu max"
>>>>>>>> +     * for the isapc machine, which will provide a much more modern
>>>>>>>> 32-bit
>>>>>>>> +     * CPU than would be expected for an ISA-era PC. If the "max"
>>>>>>>> cpu type has
>>>>>>>> +     * been specified, choose the "best" 32-bit cpu possible which
>>>>>>>> we consider
>>>>>>>> +     * be the pentium3 (deliberately choosing an Intel CPU given
>>>>>>>> that the
>>>>>>>> +     * default 486 CPU for the isapc machine is also an Intel CPU).
>>>>>>>> +     */
>>>>>>>> +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
>>>>>>>> +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
>>>>>>>> +        warn_report("-cpu max is invalid for isapc machine, using
>>>>>>>> pentium3");
>>>>>>>> +    }
>>>>>>>
>>>>>>> Do we need to handle the case of "-cpu host"?
>>>>>>
>>>>>> I don't believe so. I wasn't originally planning to support "-cpu max" for
>>>>>> isapc, however Daniel mentioned that it could possibly be generated from
>>>>>> libvirt so it makes sense to add the above check to warn in this case and
>>>>>> then continue.
>>>>>
>>>>> Libvirt will support sending any valid -cpu flag, including both
>>>>> 'max' (any config) and 'host' (if KVM).
>>>>>
>>>>> If 'isapc' still expects to support KVM, then it would be odd to
>>>>> reject 'host', but KVM presumably has no built-in way to limit to
>>>>> 32-bit without QEMU manually masking many features ?
>>>>>
>>>>> I'm a little worried about implications of libvirt sending '-cpu max'
>>>>> and QEMU secretly turning that into '-cpu pentium3', as opposed to
>>>>> having '-cpu max' expand to equiv to 'pentium3', which might cauase
>>>>> confusion when libvirt queries the expanded CPU ? Copying Jiri for
>>>>> an opinion from libvirt side, as I might be worrying about nothing.
>>>>
>>>> OK, on 2nd thought, even while warning the user, changing the type
>>>> under the hood isn't great.
>>>
>>> I second that,
>>> Please don't do magical mutations of CPUs, just error out.
>>>
>>> we used to 'fix|tweak' CPUs using machine compat hack,
>>> however with introduction of versioned cpu models we shouldn't do that anymore.
>>> (aka: existing CPU devices should stay immutable if possible, and any visible
>>> changes should go into new version)
>>
>> The original suggestion for allowing "max"/"host" was so that it
>> wouldn't cause any regressions with command lines erroneously including
>> -cpu max or -cpu host (which I believe may be possible with libvirt).
> 
> looking back and at Daniels reply,
> max/host are indeed are 'special' aka mutable as opposed to named cpu models.
> 
> if we go by the books, 'host' and by extension 'max' should work with KVM accelerator.
> But that (aka reducing it to isapc levels) should be done at 'host' cpu model code
> and that part of code is not really aware (nor should be) of machine types.
> I'm not sure, whether it's worth the effort and complexity.
> 
> I'd be fine with valid_cpu_types[] approach here, i.e. user will get
> clear error that her is doing wrong thing trying 'host/max',
> and printing suggestion how to remedy error should guide user
> to the right config.

Okay I've just sent through a patch that removes the -cpu host and -cpu 
max mapping logic for the isapc machine.

As an aside, I think it was originally your proposal a while back to 
deprecate isapc? Now the main series has been merged, is the current 
split sufficient for the tidy-up/improvements that you were planning to 
do for the pc/q35 machines, or is there a way we can improve it?


ATB,

Mark.

On Wed, 24 Sep 2025 13:50:39 +0100
Mark Cave-Ayland <mark.caveayland@nutanix.com> wrote:

> On 23/09/2025 10:30, Igor Mammedov wrote:
> 
> > On Mon, 22 Sep 2025 14:56:57 +0100
> > Mark Cave-Ayland <mark.caveayland@nutanix.com> wrote:
> >   
> >> On 22/09/2025 13:35, Igor Mammedov wrote:
> >>  
> >>> On Mon, 22 Sep 2025 14:05:13 +0200
> >>> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>>      
> >>>> On 27/8/25 13:46, Daniel P. Berrangé wrote:  
> >>>>> On Wed, Aug 27, 2025 at 12:10:00PM +0100, Mark Cave-Ayland wrote:  
> >>>>>> On 26/08/2025 08:25, Xiaoyao Li wrote:
> >>>>>>        
> >>>>>>> On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:  
> >>>>>>>> The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst it is
> >>>>>>>> possible to specify any CPU via -cpu on the command line, it makes no
> >>>>>>>> sense to allow modern 64-bit CPUs to be used.
> >>>>>>>>
> >>>>>>>> Restrict the isapc machine to the available 32-bit CPUs, taking care to
> >>>>>>>> handle the case where if a user inadvertently uses -cpu max then the
> >>>>>>>> "best"
> >>>>>>>> 32-bit CPU is used (in this case the pentium3).
> >>>>>>>>
> >>>>>>>> Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> >>>>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>>>>>>> ---
> >>>>>>>>      hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
> >>>>>>>>      1 file changed, 26 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >>>>>>>> index c03324281b..5720b6b556 100644
> >>>>>>>> --- a/hw/i386/pc_piix.c
> >>>>>>>> +++ b/hw/i386/pc_piix.c
> >>>>>>>> @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj,
> >>>>>>>> int value, Error **errp)
> >>>>>>>>      #ifdef CONFIG_ISAPC
> >>>>>>>>      static void pc_init_isa(MachineState *machine)
> >>>>>>>>      {
> >>>>>>>> +    /*
> >>>>>>>> +     * There is a small chance that someone unintentionally passes
> >>>>>>>> "- cpu max"
> >>>>>>>> +     * for the isapc machine, which will provide a much more modern
> >>>>>>>> 32-bit
> >>>>>>>> +     * CPU than would be expected for an ISA-era PC. If the "max"
> >>>>>>>> cpu type has
> >>>>>>>> +     * been specified, choose the "best" 32-bit cpu possible which
> >>>>>>>> we consider
> >>>>>>>> +     * be the pentium3 (deliberately choosing an Intel CPU given
> >>>>>>>> that the
> >>>>>>>> +     * default 486 CPU for the isapc machine is also an Intel CPU).
> >>>>>>>> +     */
> >>>>>>>> +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
> >>>>>>>> +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
> >>>>>>>> +        warn_report("-cpu max is invalid for isapc machine, using
> >>>>>>>> pentium3");
> >>>>>>>> +    }  
> >>>>>>>
> >>>>>>> Do we need to handle the case of "-cpu host"?  
> >>>>>>
> >>>>>> I don't believe so. I wasn't originally planning to support "-cpu max" for
> >>>>>> isapc, however Daniel mentioned that it could possibly be generated from
> >>>>>> libvirt so it makes sense to add the above check to warn in this case and
> >>>>>> then continue.  
> >>>>>
> >>>>> Libvirt will support sending any valid -cpu flag, including both
> >>>>> 'max' (any config) and 'host' (if KVM).
> >>>>>
> >>>>> If 'isapc' still expects to support KVM, then it would be odd to
> >>>>> reject 'host', but KVM presumably has no built-in way to limit to
> >>>>> 32-bit without QEMU manually masking many features ?
> >>>>>
> >>>>> I'm a little worried about implications of libvirt sending '-cpu max'
> >>>>> and QEMU secretly turning that into '-cpu pentium3', as opposed to
> >>>>> having '-cpu max' expand to equiv to 'pentium3', which might cauase
> >>>>> confusion when libvirt queries the expanded CPU ? Copying Jiri for
> >>>>> an opinion from libvirt side, as I might be worrying about nothing.  
> >>>>
> >>>> OK, on 2nd thought, even while warning the user, changing the type
> >>>> under the hood isn't great.  
> >>>
> >>> I second that,
> >>> Please don't do magical mutations of CPUs, just error out.
> >>>
> >>> we used to 'fix|tweak' CPUs using machine compat hack,
> >>> however with introduction of versioned cpu models we shouldn't do that anymore.
> >>> (aka: existing CPU devices should stay immutable if possible, and any visible
> >>> changes should go into new version)  
> >>
> >> The original suggestion for allowing "max"/"host" was so that it
> >> wouldn't cause any regressions with command lines erroneously including
> >> -cpu max or -cpu host (which I believe may be possible with libvirt).  
> > 
> > looking back and at Daniels reply,
> > max/host are indeed are 'special' aka mutable as opposed to named cpu models.
> > 
> > if we go by the books, 'host' and by extension 'max' should work with KVM accelerator.
> > But that (aka reducing it to isapc levels) should be done at 'host' cpu model code
> > and that part of code is not really aware (nor should be) of machine types.
> > I'm not sure, whether it's worth the effort and complexity.
> > 
> > I'd be fine with valid_cpu_types[] approach here, i.e. user will get
> > clear error that her is doing wrong thing trying 'host/max',
> > and printing suggestion how to remedy error should guide user
> > to the right config.  
> 
> Okay I've just sent through a patch that removes the -cpu host and -cpu 
> max mapping logic for the isapc machine.
> 
> As an aside, I think it was originally your proposal a while back to 
> deprecate isapc? Now the main series has been merged, is the current 
> split sufficient for the tidy-up/improvements that you were planning to 
> do for the pc/q35 machines, or is there a way we can improve it?
Can't answer this right of the bat,
I'd need to through the code again to see it.

Anyways any help with cleaning up old code are welcome.
Thanks putting some effort into it!

> 
> 
> ATB,
> 
> Mark.
> 

On Wed, Aug 27, 2025 at 12:46:22 +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 27, 2025 at 12:10:00PM +0100, Mark Cave-Ayland wrote:
> > On 26/08/2025 08:25, Xiaoyao Li wrote:
> > 
> > > On 8/22/2025 8:11 PM, Mark Cave-Ayland wrote:
> > > > The isapc machine represents a legacy ISA PC with a 486 CPU. Whilst it is
> > > > possible to specify any CPU via -cpu on the command line, it makes no
> > > > sense to allow modern 64-bit CPUs to be used.
> > > > 
> > > > Restrict the isapc machine to the available 32-bit CPUs, taking care to
> > > > handle the case where if a user inadvertently uses -cpu max then the
> > > > "best"
> > > > 32-bit CPU is used (in this case the pentium3).
> > > > 
> > > > Signed-off-by: Mark Cave-Ayland <mark.caveayland@nutanix.com>
> > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > ---
> > > >   hw/i386/pc_piix.c | 26 ++++++++++++++++++++++++++
> > > >   1 file changed, 26 insertions(+)
> > > > 
> > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > > > index c03324281b..5720b6b556 100644
> > > > --- a/hw/i386/pc_piix.c
> > > > +++ b/hw/i386/pc_piix.c
> > > > @@ -436,6 +436,19 @@ static void pc_set_south_bridge(Object *obj,
> > > > int value, Error **errp)
> > > >   #ifdef CONFIG_ISAPC
> > > >   static void pc_init_isa(MachineState *machine)
> > > >   {
> > > > +    /*
> > > > +     * There is a small chance that someone unintentionally passes
> > > > "- cpu max"
> > > > +     * for the isapc machine, which will provide a much more modern
> > > > 32-bit
> > > > +     * CPU than would be expected for an ISA-era PC. If the "max"
> > > > cpu type has
> > > > +     * been specified, choose the "best" 32-bit cpu possible which
> > > > we consider
> > > > +     * be the pentium3 (deliberately choosing an Intel CPU given
> > > > that the
> > > > +     * default 486 CPU for the isapc machine is also an Intel CPU).
> > > > +     */
> > > > +    if (!strcmp(machine->cpu_type, X86_CPU_TYPE_NAME("max"))) {
> > > > +        machine->cpu_type = X86_CPU_TYPE_NAME("pentium3");
> > > > +        warn_report("-cpu max is invalid for isapc machine, using
> > > > pentium3");
> > > > +    }
> > > 
> > > Do we need to handle the case of "-cpu host"?
> > 
> > I don't believe so. I wasn't originally planning to support "-cpu max" for
> > isapc, however Daniel mentioned that it could possibly be generated from
> > libvirt so it makes sense to add the above check to warn in this case and
> > then continue.
> 
> Libvirt will support sending any valid -cpu flag, including both
> 'max' (any config) and 'host' (if KVM).
> 
> If 'isapc' still expects to support KVM, then it would be odd to
> reject 'host', but KVM presumably has no built-in way to limit to
> 32-bit without QEMU manually masking many features ?
> 
> I'm a little worried about implications of libvirt sending '-cpu max'
> and QEMU secretly turning that into '-cpu pentium3', as opposed to
> having '-cpu max' expand to equiv to 'pentium3', which might cauase
> confusion when libvirt queries the expanded CPU ? Copying Jiri for
> an opinion from libvirt side, as I might be worrying about nothing.

When doing CPU expansion or checking a virtual CPU created when starting
a domain, we only care about features and ignore CPU model name. The CPU
expansion returns "base" CPU model name anyway. So we should not really
notice -cpu max was silently changed into something else by QEMU.

Jirka