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

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

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