| Message ID | 20190404071421.4891-1-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/input/pckbd: The i8042 device should not be user_creatable | expand |
On 04/04/19 09:14, Thomas Huth wrote: > The i8042 PS/2 controller is part of the chipset on the motherboard. > It is instantiated by the machine init code, and it does not make sense > to allow the user to plug an additional i8042 in any of the free ISA slots. > Thus let's mark the device with user_creatable = false. > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/input/pckbd.c | 2 ++ > 1 file changed, 2 insertions(+) user_creatable is not for devices that are not pluggable in real life; it is for devices that crash QEMU (!) or always fail if plugged by the user. So the question to ask is: would it make sense, and especially work, to add an i8042 to machines that do have an ISA bridge (for example the Alpha?) Paolo > diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c > index 47a606f5e3..af393818fc 100644 > --- a/hw/input/pckbd.c > +++ b/hw/input/pckbd.c > @@ -568,6 +568,8 @@ static void i8042_class_initfn(ObjectClass *klass, void *data) > dc->realize = i8042_realizefn; > dc->vmsd = &vmstate_kbd_isa; > set_bit(DEVICE_CATEGORY_INPUT, dc->categories); > + /* i8042 is a device on the motherboard, and not pluggable by the user */ > + dc->user_creatable = false; > } > > static const TypeInfo i8042_info = { >
On 4/4/19 12:07 PM, Paolo Bonzini wrote: > On 04/04/19 09:14, Thomas Huth wrote: >> The i8042 PS/2 controller is part of the chipset on the motherboard. >> It is instantiated by the machine init code, and it does not make sense >> to allow the user to plug an additional i8042 in any of the free ISA slots. >> Thus let's mark the device with user_creatable = false. >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> hw/input/pckbd.c | 2 ++ >> 1 file changed, 2 insertions(+) > > user_creatable is not for devices that are not pluggable in real life; > it is for devices that crash QEMU (!) or always fail if plugged by the user. > > So the question to ask is: would it make sense, and especially work, to > add an i8042 to machines that do have an ISA bridge (for example the Alpha?) Correct me if I'm wrong but it seems no machine directly use a 8042 on a ISA bus, it is always part of a SuperIO chipset. It is not reflected in the code (in particular the X86 machines, but I'm working on cleaning this). So I think this change is OK and will help to enforce using proper superio chipsets in the future. > > Paolo > > >> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c >> index 47a606f5e3..af393818fc 100644 >> --- a/hw/input/pckbd.c >> +++ b/hw/input/pckbd.c >> @@ -568,6 +568,8 @@ static void i8042_class_initfn(ObjectClass *klass, void *data) >> dc->realize = i8042_realizefn; >> dc->vmsd = &vmstate_kbd_isa; >> set_bit(DEVICE_CATEGORY_INPUT, dc->categories); >> + /* i8042 is a device on the motherboard, and not pluggable by the user */ I'm not sure the comment is accurate, maybe "ISA i8042 are provided by Super I/O devices"? >> + dc->user_creatable = false; >> } >> >> static const TypeInfo i8042_info = { >> > >
On 04/04/2019 15.29, Philippe Mathieu-Daudé wrote: > On 4/4/19 12:07 PM, Paolo Bonzini wrote: >> On 04/04/19 09:14, Thomas Huth wrote: >>> The i8042 PS/2 controller is part of the chipset on the motherboard. >>> It is instantiated by the machine init code, and it does not make sense >>> to allow the user to plug an additional i8042 in any of the free ISA slots. >>> Thus let's mark the device with user_creatable = false. >>> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> hw/input/pckbd.c | 2 ++ >>> 1 file changed, 2 insertions(+) >> >> user_creatable is not for devices that are not pluggable in real life; >> it is for devices that crash QEMU (!) or always fail if plugged by the user. ... hmm, but presenting devices to the user that are clearly not intended for direct use is also not very nice, is it? >> So the question to ask is: would it make sense, and especially work, to >> add an i8042 to machines that do have an ISA bridge (for example the Alpha?) I don't think so. It is a device that is supposed to be part of the chipset on the motherboard, so operating systems certainly don't know how to use this device on other machines. And at least some part of the device have to be set up in source code (see e.g. i8042_setup_a20_line() ...). > Correct me if I'm wrong but it seems no machine directly use a 8042 on a > ISA bus, it is always part of a SuperIO chipset. It is not reflected in > the code (in particular the X86 machines, but I'm working on cleaning this). What about the "isa_create_simple(isa_bus, TYPE_I8042)" in mips_r4k.c ? >>> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c >>> index 47a606f5e3..af393818fc 100644 >>> --- a/hw/input/pckbd.c >>> +++ b/hw/input/pckbd.c >>> @@ -568,6 +568,8 @@ static void i8042_class_initfn(ObjectClass *klass, void *data) >>> dc->realize = i8042_realizefn; >>> dc->vmsd = &vmstate_kbd_isa; >>> set_bit(DEVICE_CATEGORY_INPUT, dc->categories); >>> + /* i8042 is a device on the motherboard, and not pluggable by the user */ > > I'm not sure the comment is accurate, maybe "ISA i8042 are provided by > Super I/O devices"? Fine for me, too ... but what about mips_r4k in that case? Thomas
Thomas Huth <thuth@redhat.com> writes: > On 04/04/2019 15.29, Philippe Mathieu-Daudé wrote: >> On 4/4/19 12:07 PM, Paolo Bonzini wrote: >>> On 04/04/19 09:14, Thomas Huth wrote: >>>> The i8042 PS/2 controller is part of the chipset on the motherboard. >>>> It is instantiated by the machine init code, and it does not make sense >>>> to allow the user to plug an additional i8042 in any of the free ISA slots. >>>> Thus let's mark the device with user_creatable = false. >>>> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> hw/input/pckbd.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>> >>> user_creatable is not for devices that are not pluggable in real life; >>> it is for devices that crash QEMU (!) or always fail if plugged by the user. > > ... hmm, but presenting devices to the user that are clearly not > intended for direct use is also not very nice, is it? Maybe, but hiding them should be separate from marking devices that still defeat device_add. In the ideal world, we'd be able to start with an empty board, then build a machine with by wiring together devices. Say like use device_add to create and plug into parent bus, qom-set link properties to create additional wires. Plenty of devices fail at the device_add stage, or require additional wiring by code. These are marked not user_creatable. See also commit e90f2a8c3e0e677eeea46a9b401c3f98425ffa37. >>> So the question to ask is: would it make sense, and especially work, to >>> add an i8042 to machines that do have an ISA bridge (for example the Alpha?) Scratch the "would it make sense" part, keep the "would it work" part. > I don't think so. It is a device that is supposed to be part of the > chipset on the motherboard, so operating systems certainly don't know > how to use this device on other machines. > > And at least some part of the device have to be set up in source code > (see e.g. i8042_setup_a20_line() ...). If (and only if) the parts that need code are essential to the functioning the device, it should be marked not user_creatable. >> Correct me if I'm wrong but it seems no machine directly use a 8042 on a >> ISA bus, it is always part of a SuperIO chipset. It is not reflected in >> the code (in particular the X86 machines, but I'm working on cleaning this). Known issue: we model a bunch of x86 devices as ISA devices, even though they're actually part of a super i/o device connected via LPC bus. [...]
On 4/4/19 4:19 PM, Thomas Huth wrote: > On 04/04/2019 15.29, Philippe Mathieu-Daudé wrote: >> On 4/4/19 12:07 PM, Paolo Bonzini wrote: >>> On 04/04/19 09:14, Thomas Huth wrote: >>>> The i8042 PS/2 controller is part of the chipset on the motherboard. >>>> It is instantiated by the machine init code, and it does not make sense >>>> to allow the user to plug an additional i8042 in any of the free ISA slots. >>>> Thus let's mark the device with user_creatable = false. >>>> >>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>> --- >>>> hw/input/pckbd.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>> >>> user_creatable is not for devices that are not pluggable in real life; >>> it is for devices that crash QEMU (!) or always fail if plugged by the user. > > ... hmm, but presenting devices to the user that are clearly not > intended for direct use is also not very nice, is it? > >>> So the question to ask is: would it make sense, and especially work, to >>> add an i8042 to machines that do have an ISA bridge (for example the Alpha?) > > I don't think so. It is a device that is supposed to be part of the > chipset on the motherboard, so operating systems certainly don't know > how to use this device on other machines. > > And at least some part of the device have to be set up in source code > (see e.g. i8042_setup_a20_line() ...). > >> Correct me if I'm wrong but it seems no machine directly use a 8042 on a >> ISA bus, it is always part of a SuperIO chipset. It is not reflected in >> the code (in particular the X86 machines, but I'm working on cleaning this). > > What about the "isa_create_simple(isa_bus, TYPE_I8042)" in mips_r4k.c ? There is a comment at the top of the file: " All peripherial devices are attached to this "bus" with the standard PC ISA addresses." So IMO this setup is definitively modelable by an instance of the TYPE_ISA_SUPERIO abstract device. At a quick look the r4k has no parallel port and 4 uarts, so I'd add a such model internal to mips_r4k.c (see fdc37m81x_class_init()). >>>> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c >>>> index 47a606f5e3..af393818fc 100644 >>>> --- a/hw/input/pckbd.c >>>> +++ b/hw/input/pckbd.c >>>> @@ -568,6 +568,8 @@ static void i8042_class_initfn(ObjectClass *klass, void *data) >>>> dc->realize = i8042_realizefn; >>>> dc->vmsd = &vmstate_kbd_isa; >>>> set_bit(DEVICE_CATEGORY_INPUT, dc->categories); >>>> + /* i8042 is a device on the motherboard, and not pluggable by the user */ >> >> I'm not sure the comment is accurate, maybe "ISA i8042 are provided by >> Super I/O devices"? > > Fine for me, too ... but what about mips_r4k in that case? > > Thomas >
On 4/4/19 6:40 PM, Philippe Mathieu-Daudé wrote: > On 4/4/19 4:19 PM, Thomas Huth wrote: >> On 04/04/2019 15.29, Philippe Mathieu-Daudé wrote: >>> On 4/4/19 12:07 PM, Paolo Bonzini wrote: >>>> On 04/04/19 09:14, Thomas Huth wrote: >>>>> The i8042 PS/2 controller is part of the chipset on the motherboard. >>>>> It is instantiated by the machine init code, and it does not make sense >>>>> to allow the user to plug an additional i8042 in any of the free ISA slots. >>>>> Thus let's mark the device with user_creatable = false. >>>>> >>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>>> --- >>>>> hw/input/pckbd.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>> >>>> user_creatable is not for devices that are not pluggable in real life; >>>> it is for devices that crash QEMU (!) or always fail if plugged by the user. >> >> ... hmm, but presenting devices to the user that are clearly not >> intended for direct use is also not very nice, is it? >> >>>> So the question to ask is: would it make sense, and especially work, to >>>> add an i8042 to machines that do have an ISA bridge (for example the Alpha?) >> >> I don't think so. It is a device that is supposed to be part of the >> chipset on the motherboard, so operating systems certainly don't know >> how to use this device on other machines. >> >> And at least some part of the device have to be set up in source code >> (see e.g. i8042_setup_a20_line() ...). >> >>> Correct me if I'm wrong but it seems no machine directly use a 8042 on a >>> ISA bus, it is always part of a SuperIO chipset. It is not reflected in >>> the code (in particular the X86 machines, but I'm working on cleaning this). >> >> What about the "isa_create_simple(isa_bus, TYPE_I8042)" in mips_r4k.c ? > > There is a comment at the top of the file: > > " All peripherial devices are attached to this "bus" with the standard > PC ISA addresses." > > So IMO this setup is definitively modelable by an instance of the > TYPE_ISA_SUPERIO abstract device. > At a quick look the r4k has no parallel port and 4 uarts, so I'd add a > such model internal to mips_r4k.c (see fdc37m81x_class_init()). 4 uarts because the code uses: serial_hds_isa_init(isa_bus, 0, MAX_ISA_SERIAL_PORTS); which seems now an old API use where only instanciate/map uart device if a -serial option is used (API changed in a8d78cd0a...b8846a4d6). While I wonder if someone still use the r4k machine, I have more doubts about users running with more the 2 -serial options... Anyway I'll keep 4 for backward compatibility. >>>>> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c >>>>> index 47a606f5e3..af393818fc 100644 >>>>> --- a/hw/input/pckbd.c >>>>> +++ b/hw/input/pckbd.c >>>>> @@ -568,6 +568,8 @@ static void i8042_class_initfn(ObjectClass *klass, void *data) >>>>> dc->realize = i8042_realizefn; >>>>> dc->vmsd = &vmstate_kbd_isa; >>>>> set_bit(DEVICE_CATEGORY_INPUT, dc->categories); >>>>> + /* i8042 is a device on the motherboard, and not pluggable by the user */ >>> >>> I'm not sure the comment is accurate, maybe "ISA i8042 are provided by >>> Super I/O devices"? >> >> Fine for me, too ... but what about mips_r4k in that case? >> >> Thomas >>
On 04/04/19 22:49, Philippe Mathieu-Daudé wrote: > While I wonder if someone still use the r4k machine, I have more doubts > about users running with more the 2 -serial options... I sort of remember ISA extension cards that had jumpers or dip switches to choose between COM1/COM2/COM3/COM4... Paolo
On 04/04/2019 18.30, Markus Armbruster wrote: > Thomas Huth <thuth@redhat.com> writes: > >> On 04/04/2019 15.29, Philippe Mathieu-Daudé wrote: >>> On 4/4/19 12:07 PM, Paolo Bonzini wrote: >>>> On 04/04/19 09:14, Thomas Huth wrote: >>>>> The i8042 PS/2 controller is part of the chipset on the motherboard. >>>>> It is instantiated by the machine init code, and it does not make sense >>>>> to allow the user to plug an additional i8042 in any of the free ISA slots. >>>>> Thus let's mark the device with user_creatable = false. >>>>> >>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>>> --- >>>>> hw/input/pckbd.c | 2 ++ >>>>> 1 file changed, 2 insertions(+) >>>> >>>> user_creatable is not for devices that are not pluggable in real life; >>>> it is for devices that crash QEMU (!) or always fail if plugged by the user. >> >> ... hmm, but presenting devices to the user that are clearly not >> intended for direct use is also not very nice, is it? > > Maybe, but hiding them should be separate from marking devices that > still defeat device_add. > > In the ideal world, we'd be able to start with an empty board, then > build a machine with by wiring together devices. Say like use > device_add to create and plug into parent bus, qom-set link properties > to create additional wires. I agree with you on the vision of creating a machine dynamically - but I doubt that we will ever do it with the "high level" -device option or device_add HMP/QMP command. You'd likely rather need to introduce a new interface / commands for this anyway, which then simply ignore the normal user_creatable flag. > Plenty of devices fail at the device_add stage, or require additional > wiring by code. These are marked not user_creatable. Sure, I think we all agree on these kind of devices. But what about these devices which obviously don't crash QEMU, but are completely useless with "-device", since they can not be used in any meaningful way? Presenting them to the average user e.g. in the "-device help" output is somewhat gruesome, isn't it? >>>> So the question to ask is: would it make sense, and especially work, to >>>> add an i8042 to machines that do have an ISA bridge (for example the Alpha?) > > Scratch the "would it make sense" part, keep the "would it work" part. I don't see a way how this could really work (in the sense of being usable, not in the sense of not crashing QEMU). The device is only expected to be there on certain chipssets, the guest does not have a possibility to detect this device on systems that do not feature one of these chipssets. So you get a device in such a case that simply does nothing. How is that useful for you? Thomas
Paolo Bonzini <pbonzini@redhat.com> writes: > On 04/04/19 22:49, Philippe Mathieu-Daudé wrote: >> While I wonder if someone still use the r4k machine, I have more doubts >> about users running with more the 2 -serial options... > > I sort of remember ISA extension cards that had jumpers or dip switches > to choose between COM1/COM2/COM3/COM4... They still exist, with I/O ports and IRQ lines selectable via jumpers & DIP switches. Traditionally: COM1: I/O port 0x3F8, IRQ 4 COM2: I/O port 0x2F8, IRQ 3 COM3: I/O port 0x3E8, IRQ 4 COM4: I/O port 0x2E8, IRQ 3
Thomas Huth <thuth@redhat.com> writes: > On 04/04/2019 18.30, Markus Armbruster wrote: >> Thomas Huth <thuth@redhat.com> writes: >> >>> On 04/04/2019 15.29, Philippe Mathieu-Daudé wrote: >>>> On 4/4/19 12:07 PM, Paolo Bonzini wrote: >>>>> On 04/04/19 09:14, Thomas Huth wrote: >>>>>> The i8042 PS/2 controller is part of the chipset on the motherboard. >>>>>> It is instantiated by the machine init code, and it does not make sense >>>>>> to allow the user to plug an additional i8042 in any of the free ISA slots. >>>>>> Thus let's mark the device with user_creatable = false. >>>>>> >>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>>>>> --- >>>>>> hw/input/pckbd.c | 2 ++ >>>>>> 1 file changed, 2 insertions(+) >>>>> >>>>> user_creatable is not for devices that are not pluggable in real life; >>>>> it is for devices that crash QEMU (!) or always fail if plugged by the user. >>> >>> ... hmm, but presenting devices to the user that are clearly not >>> intended for direct use is also not very nice, is it? >> >> Maybe, but hiding them should be separate from marking devices that >> still defeat device_add. >> >> In the ideal world, we'd be able to start with an empty board, then >> build a machine with by wiring together devices. Say like use >> device_add to create and plug into parent bus, qom-set link properties >> to create additional wires. > > I agree with you on the vision of creating a machine dynamically - but I > doubt that we will ever do it with the "high level" -device option or > device_add HMP/QMP command. You'd likely rather need to introduce a new > interface / commands for this anyway, -device is hardly high level. It's a thin wrapper around qdev_device_add(), which adds a bit of convenience to the QOM building blocks. Machine creation would probably use a configuration file, and work with the same QOM building blocks. > which then simply ignore the > normal user_creatable flag. Point taken. However, I'm afraid you've missed my point that hiding devices from the user should be separate from marking devices that still defeat device_add. @user_creatable is about the latter. Downstreams sometimes press it into service for the former, which is probably a bad idea. The naming of @user_creatable is and has always been political. Before it got its current name, it was cannot_instantiate_with_device_add_yet. This expressed its intent. I've stopped caring about its name long ago. If you want to have a flag to hide devices from the user, and you want to name it user_creatable, go right ahead and rename what's now called user_creatable to whatever other name you see fit to get it out of the way. Just don't press what's now called user_creatable into service for something else. >> Plenty of devices fail at the device_add stage, or require additional >> wiring by code. These are marked not user_creatable. > > Sure, I think we all agree on these kind of devices. > > But what about these devices which obviously don't crash QEMU, but are > completely useless with "-device", since they can not be used in any > meaningful way? Presenting them to the average user e.g. in the "-device > help" output is somewhat gruesome, isn't it? Quoting myself some more: "hiding them should be separate from marking devices that still defeat device_add." >>>>> So the question to ask is: would it make sense, and especially work, to >>>>> add an i8042 to machines that do have an ISA bridge (for example the Alpha?) >> >> Scratch the "would it make sense" part, keep the "would it work" part. > > I don't see a way how this could really work (in the sense of being > usable, not in the sense of not crashing QEMU). The device is only > expected to be there on certain chipssets, the guest does not have a > possibility to detect this device on systems that do not feature one of > these chipssets. So you get a device in such a case that simply does > nothing. How is that useful for you? Risking to sound like a broken record... You want to hide certain devices from the user because they are (in your reasoned opinion) of no use to the user. I'm not going to argue about that. All I want is you refrain from abusing what is now called @user_creatable for that purpose. Telling me that hiding is a good idea won't persuade me this abuse would be a good idea.
diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c index 47a606f5e3..af393818fc 100644 --- a/hw/input/pckbd.c +++ b/hw/input/pckbd.c @@ -568,6 +568,8 @@ static void i8042_class_initfn(ObjectClass *klass, void *data) dc->realize = i8042_realizefn; dc->vmsd = &vmstate_kbd_isa; set_bit(DEVICE_CATEGORY_INPUT, dc->categories); + /* i8042 is a device on the motherboard, and not pluggable by the user */ + dc->user_creatable = false; } static const TypeInfo i8042_info = {
The i8042 PS/2 controller is part of the chipset on the motherboard. It is instantiated by the machine init code, and it does not make sense to allow the user to plug an additional i8042 in any of the free ISA slots. Thus let's mark the device with user_creatable = false. Signed-off-by: Thomas Huth <thuth@redhat.com> --- hw/input/pckbd.c | 2 ++ 1 file changed, 2 insertions(+)