[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH-for-9.1] hw/i386/pc: Warn about unsatisfied vmport deps
From: |
Kamil Szczęk |
Subject: |
Re: [PATCH-for-9.1] hw/i386/pc: Warn about unsatisfied vmport deps |
Date: |
Thu, 15 Aug 2024 18:22:16 +0000 |
Hi Philippe and sorry for the delay!
On Wednesday, August 14th, 2024 at 16:02, Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
>
> Hi Kamil,
>
> On 14/8/24 13:10, 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 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");
>
>
> Should we fail instead?
I think failing would be preferrable over a warning, but I opted for the latter
to maintain backward compatibility in this specific configuration.
But now that I think about it, this explicit configuration
(vmport=on,i8042=off) is probably very rare in the real world, if it is
exercised at all. So failing may not be as big of a breaking change as I first
thought.
If you're fine with introducing this "breaking" change, then I'm down for it
too. Let me know if I should post v2.
>
> > + }
> > +
> > 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;
> > }
> >
> > /* 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.