Re: [PATCH v6 01/19] hw/i386/pc_piix.c: restrict isapc machine to 32-bit CPUs

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.
> 
>