Re: [Qemu-devel] [PATCH] hw/input/pckbd: The i8042 device should not be user_creatable - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Thomas Huth <thuth@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	qemu-devel@nongnu.org, "Michael S. Tsirkin" <mst@redhat.com>
Cc: qemu-trivial@nongnu.org
Subject: Re: [Qemu-devel] [PATCH] hw/input/pckbd: The i8042 device should not be user_creatable
Date: Thu, 4 Apr 2019 18:40:56 +0200	[thread overview]
Message-ID: <fda6bc0b-9ea0-2770-f104-568b3f16ab90@redhat.com> (raw)
In-Reply-To: <3ba3c765-589f-4c0a-38dc-d9126f515e7b@redhat.com>

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
> 