[PATCH-for-9.1 v3 0/2] hw/i386/pc: Fix vmport option handling

[PATCH-for-9.1 v3 0/2] hw/i386/pc: Fix vmport option handling

[Kamil Szczęk]

Some time ago a new option to disable the built-in i8042 controller was
introduced (commit 4ccd5fe22feb95137d325f422016a6473541fe9f). This
however introduced a side-effect - disabling this controller resulted
in vmport's creation being omitted, regardless if it was enabled
implicitly or explicitly. This patch series aims to clean up vmport
option handling and introduces additional validation for these options.

Changelog:

v2 -> v3:
  - Move vmport=auto -> vmport=on/off conversion to a shared code path
  - Reword documentation for the vmport option, "and/or" => "or"

v1 -> v2:
  - Exit with a useful error message instead of issuing a warning

Kamil Szczęk (2):
  hw/i386/pc: Unify vmport=auto handling
  hw/i386/pc: Ensure vmport prerequisites are fulfilled

 hw/i386/pc.c      | 14 ++++++++++++--
 hw/i386/pc_piix.c |  5 -----
 hw/i386/pc_q35.c  |  5 -----
 qemu-options.hx   |  4 ++--
 4 files changed, 14 insertions(+), 14 deletions(-)

-- 
2.45.0

[PATCH-for-9.1 v3 1/2] hw/i386/pc: Unify vmport=auto handling

[Kamil Szczęk]

The code which translates vmport=auto to on/off is currently separate
for each PC machine variant, while being functionally equivalent.
This moves the translation into a shared initialization function, while
also tightening the enum assertion.

Signed-off-by: Kamil Szczęk <kamil@szczek.dev>
---
 hw/i386/pc.c      | 5 +++++
 hw/i386/pc_piix.c | 5 -----
 hw/i386/pc_q35.c  | 5 -----
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c74931d577..72229a24ff 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1217,6 +1217,11 @@ void pc_basic_device_init(struct PCMachineState *pcms,
         isa_realize_and_unref(pcms->pcspk, isa_bus, &error_fatal);
     }
 
+    assert(pcms->vmport >= 0 && 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;
+    }
+
     /* Super I/O */
     pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
                     pcms->vmport != ON_OFF_AUTO_ON);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index d9e69243b4..347afa4c37 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -310,11 +310,6 @@ static void pc_init1(MachineState *machine, const char *pci_type)
 
     pc_vga_init(isa_bus, pcmc->pci_enabled ? pcms->pcibus : NULL);
 
-    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;
-    }
-
     /* init basic PC hardware */
     pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc,
                          !MACHINE_CLASS(pcmc)->no_floppy, 0x4);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 9d108b194e..f2d8edfa84 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -276,11 +276,6 @@ static void pc_q35_init(MachineState *machine)
         x86_register_ferr_irq(x86ms->gsi[13]);
     }
 
-    assert(pcms->vmport != ON_OFF_AUTO__MAX);
-    if (pcms->vmport == ON_OFF_AUTO_AUTO) {
-        pcms->vmport = ON_OFF_AUTO_ON;
-    }
-
     /* init basic PC hardware */
     pc_basic_device_init(pcms, isa_bus, x86ms->gsi, x86ms->rtc, !mc->no_floppy,
                          0xff0104);
-- 
2.45.0

Re: [PATCH-for-9.1 v3 1/2] hw/i386/pc: Unify vmport=auto handling

[Philippe Mathieu-Daudé]

On 17/8/24 17:25, Kamil Szczęk wrote:
> The code which translates vmport=auto to on/off is currently separate
> for each PC machine variant, while being functionally equivalent.
> This moves the translation into a shared initialization function, while
> also tightening the enum assertion.
> 
> Signed-off-by: Kamil Szczęk <kamil@szczek.dev>
> ---
>   hw/i386/pc.c      | 5 +++++
>   hw/i386/pc_piix.c | 5 -----
>   hw/i386/pc_q35.c  | 5 -----
>   3 files changed, 5 insertions(+), 10 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH-for-9.1 v3 2/2] hw/i386/pc: Ensure vmport prerequisites are fulfilled

[Kamil Szczęk]

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 avoid confusion, let's fail 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    | 11 ++++++++---
 qemu-options.hx |  4 ++--
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 72229a24ff..7779c88a91 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1075,7 +1075,7 @@ static const MemoryRegionOps ioportF0_io_ops = {
 };
 
 static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
-                            bool create_i8042, bool no_vmport)
+                            bool create_i8042, bool no_vmport, Error **errp)
 {
     int i;
     DriveInfo *fd[MAX_FD];
@@ -1100,6 +1100,10 @@ static void pc_superio_init(ISABus *isa_bus, bool create_fdctrl,
     }
 
     if (!create_i8042) {
+        if (!no_vmport) {
+            error_setg(errp,
+                       "vmport requires the i8042 controller to be enabled");
+        }
         return;
     }
 
@@ -1219,12 +1223,13 @@ void pc_basic_device_init(struct PCMachineState *pcms,
 
     assert(pcms->vmport >= 0 && 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;
     }
 
     /* Super I/O */
     pc_superio_init(isa_bus, create_fdctrl, pcms->i8042_enabled,
-                    pcms->vmport != ON_OFF_AUTO_ON);
+                    pcms->vmport != ON_OFF_AUTO_ON, &error_fatal);
 }
 
 void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
diff --git a/qemu-options.hx b/qemu-options.hx
index cee0da2014..9197bb25d3 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 or
+        i8042=off the default is off otherwise the default is on.
 
     ``dump-guest-core=on|off``
         Include guest memory in a core dump. The default is on.
-- 
2.45.0

Re: [PATCH-for-9.1 v3 0/2] hw/i386/pc: Fix vmport option handling

[Bernhard Beschow]

Am 17. August 2024 15:24:31 UTC schrieb "Kamil Szczęk" <kamil@szczek.dev>:
>Some time ago a new option to disable the built-in i8042 controller was
>introduced (commit 4ccd5fe22feb95137d325f422016a6473541fe9f). This
>however introduced a side-effect - disabling this controller resulted
>in vmport's creation being omitted, regardless if it was enabled
>implicitly or explicitly. This patch series aims to clean up vmport
>option handling and introduces additional validation for these options.
>
>Changelog:
>
>v2 -> v3:
>  - Move vmport=auto -> vmport=on/off conversion to a shared code path
>  - Reword documentation for the vmport option, "and/or" => "or"

The vmport handling is now confined to pc.c. Excellent!

Series:
Reviewed-by: Bernhard Beschow <shentey@gmail.com>

>
>v1 -> v2:
>  - Exit with a useful error message instead of issuing a warning
>
>Kamil Szczęk (2):
>  hw/i386/pc: Unify vmport=auto handling
>  hw/i386/pc: Ensure vmport prerequisites are fulfilled
>
> hw/i386/pc.c      | 14 ++++++++++++--
> hw/i386/pc_piix.c |  5 -----
> hw/i386/pc_q35.c  |  5 -----
> qemu-options.hx   |  4 ++--
> 4 files changed, 14 insertions(+), 14 deletions(-)
>

Re: [PATCH-for-9.1 v3 0/2] hw/i386/pc: Fix vmport option handling

[Philippe Mathieu-Daudé]

On 17/8/24 17:24, Kamil Szczęk wrote:

> Kamil Szczęk (2):
>    hw/i386/pc: Unify vmport=auto handling
>    hw/i386/pc: Ensure vmport prerequisites are fulfilled

Series merged, thanks.