| Message ID | vsm1ly2eX009LRKgURcMp6qTYHWw1bZd3zg2GUbd4M90T91QvJRdKxiRS3rPl8PR96y2r890Am3Ajf4kQrwihn7-7hKBU9VicRPPtIRv_GI=@szczek.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/i386/pc: Warn about unsatisfied vmport deps | expand |
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.
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.
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 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.
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.
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.
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(-)