Re: [PATCH] hw/i386/pc: Warn about unsatisfied vmport deps

[Bernhard Beschow]

Am 14. August 2024 11:10:16 UTC schrieb "Kamil Szczęk" <kamil@szczek.dev>:
>Since commit 4ccd5fe22feb95137d325f422016a6473541fe9f ('pc: add option
>to disable PS/2 mouse/keyboard'), the vmport will not be created unless
>the i8042 PS/2 controller is enabled. To not confuse users, let's add a
>warning if vmport was explicitly requested, but the i8042 controller is
>disabled. This also changes the behavior of vmport=auto to take i8042
>controller availability into account.
>
>Signed-off-by: Kamil Szczęk <kamil@szczek.dev>
>---
> hw/i386/pc.c      | 4 ++++
> hw/i386/pc_piix.c | 3 ++-
> hw/i386/pc_q35.c  | 2 +-
> qemu-options.hx   | 4 ++--
> 4 files changed, 9 insertions(+), 4 deletions(-)
>
>diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>index c74931d577..5bd8dd0350 100644
>--- a/hw/i386/pc.c
>+++ b/hw/i386/pc.c
>@@ -1100,6 +1100,10 @@ static void pc_superio_init(ISABus *isa_bus, bool 
>create_fdctrl,
>     }
> 
>     if (!create_i8042) {
>+        if (!no_vmport) {
>+            warn_report("vmport requires the i8042 controller to be enabled");
>+        }
>+
>         return;
>     }
> 
>diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
>index d9e69243b4..cf2e2e3e30 100644
>--- a/hw/i386/pc_piix.c
>+++ b/hw/i386/pc_piix.c
>@@ -312,7 +312,8 @@ static void pc_init1(MachineState *machine, const char 
>*pci_type)
> 
>     assert(pcms->vmport != ON_OFF_AUTO__MAX);
>     if (pcms->vmport == ON_OFF_AUTO_AUTO) {
>-        pcms->vmport = xen_enabled() ? ON_OFF_AUTO_OFF : ON_OFF_AUTO_ON;
>+        pcms->vmport = (xen_enabled() || !pcms->i8042_enabled)
>+            ? ON_OFF_AUTO_OFF : ON_OFF_AUTO_ON;
>     }

I think it makes sense to consolidate this handling into 
pc_basic_devices_init() before doing this change. Maybe just in front of the 
call to pc_superio_init()? The additional handling of xen_enabled() shouldn't 
hurt there for q35: Even though q35 doesn't (yet) support Xen there are already 
code paths where this check is done.

> 
>     /* init basic PC hardware */
>diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
>index 9d108b194e..6c112d804d 100644
>--- a/hw/i386/pc_q35.c
>+++ b/hw/i386/pc_q35.c
>@@ -278,7 +278,7 @@ static void pc_q35_init(MachineState *machine)
> 
>     assert(pcms->vmport != ON_OFF_AUTO__MAX);
>     if (pcms->vmport == ON_OFF_AUTO_AUTO) {
>-        pcms->vmport = ON_OFF_AUTO_ON;
>+        pcms->vmport = pcms->i8042_enabled ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>     }
> 
>     /* init basic PC hardware */
>diff --git a/qemu-options.hx b/qemu-options.hx
>index cee0da2014..0bc780a669 100644
>--- a/qemu-options.hx
>+++ b/qemu-options.hx
>@@ -68,8 +68,8 @@ SRST
> 
>     ``vmport=on|off|auto``
>         Enables emulation of VMWare IO port, for vmmouse etc. auto says
>-        to select the value based on accel. For accel=xen the default is
>-        off otherwise the default is on.
>+        to select the value based on accel and i8042. For accel=xen
>+        and/or i8042=off the default is off otherwise the default is on.

I'd do s#and/or#or# for readability.

Best regards,
Bernhard

> 
>     ``dump-guest-core=on|off``
>         Include guest memory in a core dump. The default is on.