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

[Mark Cave-Ayland <mark.caveayland@nutanix.com>]

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.