On Mon, 22 Sep 2025 14:05:13 +0200 Philippe Mathieu-Daudé <[email protected]> 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 <[email protected]> > >>>> Reviewed-by: Philippe Mathieu-Daudé <[email protected]> > >>>> --- > >>>> 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.
