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

[Kamil Szczęk]

On Saturday, August 17th, 2024 at 13:59, Bernhard Beschow <shentey@gmail.com> 
wrote:

> 
> Am 17. August 2024 11:54:42 UTC schrieb Bernhard Beschow shentey@gmail.com:
> 
> > Am 17. August 2024 08:49:05 UTC schrieb "Kamil Szczęk" kamil@szczek.dev:
> > 
> > > On Friday, August 16th, 2024 at 15:14, Bernhard Beschow shentey@gmail.com 
> > > wrote:
> > > 
> > > > 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.
> > > 
> > > Makes sense technically, but since I'm new to the mailing list workflow I 
> > > could use some help with logistics. I've already posted a v2 of this 
> > > patch which was reviewed and accepted,
> > 
> > Ouch, I've missed that.
> > 
> > > should I wait for it to be pulled in and post a follow-up patch or post 
> > > another revision of this patch?
> > 
> > Since Michael already tagged it, it seems safer to follow up with a new 
> > series or patch. You can use the `Based-on:` tag there to make the 
> > dependency of the new series explicit. See [1] for inspiration. To 
> > determine the mail ID look up this series on lore.kernel.org/qemu-devel .
> 
> 
> Of course I meant the tagged version: 
> https://lore.kernel.org/qemu-devel/CJaQOvoJMl8P04F7-0Pk23paXt29GnSt2ICM-xlruQ9rGsMHocU_xH3RRaRRJEQpqUxGo63sATZb5St7968jHLV0r7NORODN3zHgi_qxpPE=@szczek.dev/

I've got the OK from Michael to post v3, but thanks for that nonetheless. Will 
definitely come in handy in the future.

> 
> > Best regards,
> > Bernhard
> > 
> > [1] https://patchew.org/QEMU/20230105143228.244965-1-shentey@gmail.com/
> > 
> > > > > /* 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.