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;
     }
 
     /* 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

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.

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, should I wait for it to be pulled in and post a follow-up patch or post another revision of this patch?

>
> > /* 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.

On Sat, Aug 17, 2024 at 08:49:05AM +0000, Kamil Szczęk wrote:
> 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, should I wait for it to be pulled in and post a follow-up patch or post another revision of this patch?


I rebase with now issues - that's why it's a tag, easy to drop.
So feel free to post v3.

> >
> > > /* 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.

On Saturday, August 17th, 2024 at 14:19, Michael S. Tsirkin <mst@redhat.com> wrote:

> 
> On Sat, Aug 17, 2024 at 08:49:05AM +0000, Kamil Szczęk wrote:
> 
> > 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, should I wait for it to be pulled in and post a follow-up patch or post another revision of this patch?
> 
> 
> 
> I rebase with now issues - that's why it's a tag, easy to drop.
> So feel free to post v3.

Good to know, will do.

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 .

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.

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/>

>
>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.

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.

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?

> +        }
> +
>           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.

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.

On 15/8/24 20:22, Kamil Szczęk wrote:
> 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.

Better fail early on incoherent config options rather than keep
going with a broken machine IMHO, so I'd rather a v2.