| Message ID | CJaQOvoJMl8P04F7-0Pk23paXt29GnSt2ICM-xlruQ9rGsMHocU_xH3RRaRRJEQpqUxGo63sATZb5St7968jHLV0r7NORODN3zHgi_qxpPE=@szczek.dev |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCH-for-9.1,v2] hw/i386/pc: Ensure vmport prerequisites are fulfilled | expand |
On Fri, Aug 16, 2024 at 08:01:26AM +0000, Kamil Szczęk wrote: > 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> tagged, thanks! > --- > hw/i386/pc.c | 8 ++++++-- > hw/i386/pc_piix.c | 3 ++- > hw/i386/pc_q35.c | 2 +- > qemu-options.hx | 4 ++-- > 4 files changed, 11 insertions(+), 6 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index c74931d577..c99f2ce540 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,7 +1223,7 @@ void pc_basic_device_init(struct PCMachineState *pcms, > > /* 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/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; > } > > /* 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. > > ``dump-guest-core=on|off`` > Include guest memory in a core dump. The default is on. > -- > 2.45.0 >
On 16/8/24 10:01, Kamil Szczęk wrote: > 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 | 8 ++++++-- > hw/i386/pc_piix.c | 3 ++- > hw/i386/pc_q35.c | 2 +- > qemu-options.hx | 4 ++-- > 4 files changed, 11 insertions(+), 6 deletions(-) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index c74931d577..c99f2ce540 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, Is 'errp' available? Does this patch compile? Anyway, I think you want to call error_report() & exit(). > + "vmport requires the i8042 controller to be enabled"); > + } > return; > }
On Friday, August 16th, 2024 at 10:22, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > > On 16/8/24 10:01, Kamil Szczęk wrote: > > > 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 | 8 ++++++-- > > hw/i386/pc_piix.c | 3 ++- > > hw/i386/pc_q35.c | 2 +- > > qemu-options.hx | 4 ++-- > > 4 files changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > > index c74931d577..c99f2ce540 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, > > > Is 'errp' available? Does this patch compile? It does and works as expected. > Anyway, I think you want to call error_report() & exit(). Hmm, the error.h suggests that error_report() & exit() is a legacy approach, hence why I've used error_setg & error_fatal ptr. As far as I know both approaches are equivalent, no? > > > + "vmport requires the i8042 controller to be enabled"); > > + } > > return; > > }
On 16/8/24 10:27, Kamil Szczęk wrote: > On Friday, August 16th, 2024 at 10:22, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > >> >> >> On 16/8/24 10:01, Kamil Szczęk wrote: >> >>> 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 | 8 ++++++-- >>> hw/i386/pc_piix.c | 3 ++- >>> hw/i386/pc_q35.c | 2 +- >>> qemu-options.hx | 4 ++-- >>> 4 files changed, 11 insertions(+), 6 deletions(-) >>> >>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c >>> index c74931d577..c99f2ce540 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, >> >> >> Is 'errp' available? Does this patch compile? > > It does and works as expected. My bad I missed the whole context. >> Anyway, I think you want to call error_report() & exit(). > > Hmm, the error.h suggests that error_report() & exit() is a legacy approach, hence why I've used error_setg & error_fatal ptr. As far as I know both approaches are equivalent, no? Yep. Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index c74931d577..c99f2ce540 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,7 +1223,7 @@ void pc_basic_device_init(struct PCMachineState *pcms, /* 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/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; } /* 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. ``dump-guest-core=on|off`` Include guest memory in a core dump. The default is on.
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 | 8 ++++++-- hw/i386/pc_piix.c | 3 ++- hw/i386/pc_q35.c | 2 +- qemu-options.hx | 4 ++-- 4 files changed, 11 insertions(+), 6 deletions(-)