[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH] Update virtio input device specification
On Thu, Apr 6, 2017 at 12:34 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Apr 05, 2017 at 02:21:06PM +0200, Ladi Prosek wrote:
>> Apologies for the delay.
>>
>> On Fri, Mar 24, 2017 at 9:55 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Fri, Mar 24, 2017 at 11:17:59AM +0100, Ladi Prosek wrote:
>> >> * added VIRTIO_INPUT_CFG_ID_DEVIDS / virtio_input_devids
>> >> * added normative statements and moved them all to
>> >> \devicenormative and \drivernormative sections
>> >> * minor tweaks
>> >>
>> >> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
>> >
>> > Thanks for taking this up!
>> >
>> > I think you should start sending non-incremental
>> > patches eventually. Soe comments below.
>>
>> Ok, I'll send the next version against master.
>>
>> >
>> >> ---
>> >>
>> >> This patch is based on Gerd's
>> >> https://www.kraxel.org/cgit/virtio-spec/log/?h=virtio-input
>> >>
>> >> It is addressing feedback given in
>> >> https://lists.oasis-open.org/archives/virtio-dev/201605/msg00003.html
>> >> and adding some more content.
>> >>
>> >> Thanks!
>> >> Ladi
>> >>
>> >>
>> >> conformance.tex | 18 ++++++++++++
>> >> virtio-input.tex | 87 ++++++++++++++++++++++++++++++++++++++++++++------------
>> >> 2 files changed, 87 insertions(+), 18 deletions(-)
>> >>
>> >> diff --git a/conformance.tex b/conformance.tex
>> >> index 7b7df32..2486e9a 100644
>> >> --- a/conformance.tex
>> >> +++ b/conformance.tex
>> >> @@ -145,6 +145,15 @@ An SCSI host driver MUST conform to the following normative statements:
>> >> \item \ref{drivernormative:Device Types / SCSI Host Device / Device Operation / Device Operation: eventq}
>> >> \end{itemize}
>> >>
>> >> +\subsection{Input Driver Conformance}\label{sec:Conformance / Driver Conformance / Input Driver Conformance}
>> >> +
>> >> +An input driver MUST conform to the following normative statements:
>> >> +
>> >> +\begin{itemize}
>> >> +\item \ref{drivernormative:Device Types / Input Device / Device Initialization}
>> >> +\item \ref{drivernormative:Device Types / Input Device / Device Operation}
>> >> +\end{itemize}
>> >> +
>> >> \section{Device Conformance}\label{sec:Conformance / Device Conformance}
>> >>
>> >> A device MUST conform to the following normative statements:
>> >> @@ -265,6 +274,15 @@ An SCSI host device MUST conform to the following normative statements:
>> >> \item \ref{devicenormative:Device Types / SCSI Host Device / Device Operation / Device Operation: eventq}
>> >> \end{itemize}
>> >>
>> >> +\subsection{Input Device Conformance}\label{sec:Conformance / Device Conformance / Input Device Conformance}
>> >> +
>> >> +An input device MUST conform to the following normative statements:
>> >> +
>> >> +\begin{itemize}
>> >> +\item \ref{devicenormative:Device Types / Input Device / Device Initialization}
>> >> +\item \ref{devicenormative:Device Types / Input Device / Device Operation}
>> >> +\end{itemize}
>> >> +
>> >> \section{Legacy Interface: Transitional Device and
>> >> Transitional Driver Conformance}\label{sec:Conformance / Legacy
>> >> Interface: Transitional Device and
>> >> diff --git a/virtio-input.tex b/virtio-input.tex
>> >> index 9151880..eb3e154 100644
>> >> --- a/virtio-input.tex
>> >> +++ b/virtio-input.tex
>> >> @@ -1,7 +1,8 @@
>> >> \section{Input Device}\label{sec:Device Types / Input Device}
>> >>
>> >> The virtio input device can be used to create virtual human interface
>> >> -devices such as keyboards, mice and tablets. It basically sends linux
>> >> +devices such as keyboards, mice and tablets. An instance of the virtio
>> >> +device represents one such input device. It sends Linux
>> >> input layer events over virtio.
>> >> See \href{https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input.h}{include/uapi/linux/input.h}
>> >
>> > which parts of the above are of interest?
>>
>> Of input.h? It has IOCTL definitions referenced further down in this
>> document.
>
> Like this?
> #define EVIOCGMTSLOTS(len) _IOC(_IOC_READ, 'E', 0x0a, len)
>
> And how are the ioctl numbers relevant here?
Not the numbers but the comments are.
> Are the codes actually in
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input-event-codes.h
> by any chance?
>
> Is anything besides codes relevant?
Depends. Yes, we can drop the link to input.h if you'd prefer that.
>> But in general it's hard to be any more specific since it's
>> a live header file.
>
> How will readers of our spec figure it out?
> You can refer to a fixed version too if that helps.
Which kind of brings us back to the discussion we had last year about
the spec-worthiness of all this. What Gerd did was define virtio
transport for Linux evdev. This is cool because it allows for very
straightforward pass-through functionality, as long as the guest also
runs Linux. And it leaves all the details about event codes up to
evdev, it doesn't consider them part of the device specification.
The downside is that this is hard to specify formally, the same way we
do other devices. Say we do that and point to a fixed version of the
relevant Linux headers and the headers evolve and add or change
something. Then we would either go change the spec to be in sync with
the headers or build a compatibility layer in the implementation. The
former would be wasted work because it can be done by pointing to the
latest version of the header, the latter is wasted work because evdev
needs to maintain some reasonable backwards compatibility by itself so
it's very unlikely that things would break.
I don't know .. so either we walk this thin ice of writing a pragmatic
spec than hand-waves all the details because meticulously listing
everything wouldn't bring much benefit. Or we drop this altogether
because maybe it doesn't really belong in the virtio spec. Maybe from
virtio point of view the device should be virtio-vsock, what Gerd
defined would sit on top of it and essentially define the format of
messages exchanged over the virtio-vsock transport. And it would be
"specified" only by Linux headers, alongside the IOCTLs that exist
today.
>> >> and \href{https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input-event-codes.h}{include/uapi/linux/input-event-codes.h}
>> >
>> > One thing missing is how should driver handle events
>> > it does not recognize.
>>
>> Ok, will add an explicit clause that the driver should ignore events
>> it doesn't recognize.
>>
>> > Also, is there a chance device will be interested in knowing
>> > which events does driver recognize?
>>
>> I don't think that the Linux input backend can do anything useful with
>> that kind of information. It's probably not worth trying to invent a
>> sophisticated feature negotiation scheme here.
>
> Let's assume linux gains ability to generate new events on top of old
> ones.
> Do you want to send both in all cases?
> If you only send multitouch how do you handle old guests?
> If you send both how does new guest know it should
> ignore old events?
I think that these are valid concerns but they need to be addressed at
the evdev layer, not in virtio. You can have a newer Linux with an
older user space program issuing IOCTLs and not understanding
multitouch or whatever. If evdev has or will have an IOCTL for this
kind of compat knobs, we'll wrap them but that's as far as we should
go IMO.
>> >> @@ -32,6 +33,7 @@ enum virtio_input_config_select {
>> >> VIRTIO_INPUT_CFG_UNSET = 0x00,
>> >> VIRTIO_INPUT_CFG_ID_NAME = 0x01,
>> >> VIRTIO_INPUT_CFG_ID_SERIAL = 0x02,
>> >> + VIRTIO_INPUT_CFG_ID_DEVIDS = 0x03,
>> >> VIRTIO_INPUT_CFG_PROP_BITS = 0x10,
>> >> VIRTIO_INPUT_CFG_EV_BITS = 0x11,
>> >> VIRTIO_INPUT_CFG_ABS_INFO = 0x12,
>> >> @@ -44,6 +46,13 @@ struct virtio_input_absinfo {
>> >> le32 flat;
>> >> };
>> >>
>> >> +struct virtio_input_devids {
>> >> + le16 bustype;
>> >> + le16 vendor;
>> >> + le16 product;
>> >> + le16 version;
>> >> +};
>> >> +
>> >> struct virtio_input_config {
>> >> u8 select;
>> >> u8 subsel;
>> >> @@ -53,44 +62,53 @@ struct virtio_input_config {
>> >> char string[128];
>> >> u8 bitmap[128];
>> >> struct virtio_input_absinfo abs;
>> >> + struct virtio_input_devids ids;
>> >> } u;
>> >> };
>> >> \end{lstlisting}
>> >>
>> >> -To query a specific piece of information the driver MUST set
>> >> -\field{select} and \field{subsel} accordingly, then check \field{size}
>> >> -to see and how much information is available. \field{size} can be
>> >> +To query a specific piece of information the driver sets
>> >> +\field{select} and \field{subsel} accordingly, then checks \field{size}
>> >> +to see how much information is available. \field{size} can be
>> >> zero if no information is available. Strings do not include a
>> >> -terminating NUL byte.
>> >> +NUL terminator.
>> >>
>> >> \begin{description}
>> >>
>> >> \item[VIRTIO_INPUT_CFG_ID_NAME]
>> >> -\field{subsel} is not used and MUST be zero.
>> >> +\field{subsel} is zero.
>> >> Returns the name of the device, in \field{u.string}.
>> >>
>> >> -Same as EVIOCGNAME ioctl for linux evdev devices.
>> >> +Same as EVIOCGNAME ioctl for Linux evdev devices.
>> >>
>> >> \item[VIRTIO_INPUT_CFG_ID_SERIAL]
>> >> -\field{subsel} is not used and MUST be zero.
>> >> +\field{subsel} is zero.
>> >> Returns the serial number of the device, in \field{u.string}.
>> >>
>> >> +\item[VIRTIO_INPUT_CFG_ID_DEVIDS]
>> >> +\field{subsel} is zero.
>> >> +Returns ID information of the device, in \field{u.ids}.
>> >> +
>> >> +Same as EVIOCGID ioctl for Linux evdev devices.
>> >> +
>> >
>> > Above line confuses more than it clarifies IMHO.
>> >
>> > Looks like that ioctl returns
>> > 42 struct input_id {
>> > 43 __u16 bustype;
>> > 44 __u16 vendor;
>> > 45 __u16 product;
>> > 46 __u16 version;
>> > 47 };
>> >
>> > Hos is this the same? Maybe "similar to"?
>>
>> Ok, will change to "similar to".
>
> The point is simple - we need to say what it does.
>
>
>> >> \item[VIRTIO_INPUT_CFG_PROP_BITS]
>> >> -\field{subsel} is not used and MUST be zero.
>> >> +\field{subsel} is zero.
>> >> Returns input properties (INPUT_PROP_*) of the device, in \field{u.bitmap}.
>> >>
>> >> +Same as EVIOCGPROP ioctl for Linux evdev devices.
>> >> +
>> >
>> > Above line confuses more than it clarifies IMHO.
>> >
>> > Referred to headers contain no information about this ioctl.
>>
>> I see EVIOCGPROP in input.h. With no further details, but that's true
>> for most of the stuff in the headers.
>>
>> Ideally evdev would have a good documentation that we can point to.
>> The headers are not sufficient, readers will have to dig into Linux
>> source code to understand the semantics. That's unfortunate but the
>> alternative would be to provide an exhaustive documentation of the
>> Linux input layer in this spec and keep it up to date with the
>> implementation. And that's not feasible.
>
> I'm not really sure what to do here. Are we really exposing
> such a large interface that we can't document it?
> This is what the spec is about, if people have to dig into
> code they can and will interpret it incorrectly, they
> will make assumptions based on a specific implementation and
> then their implementation will not work when you make
> changes to the host.
As I wrote above, if we cannot go pragmatic and abstract evdev away as
much as we can, then maybe we shouldn't be including this device in
the spec.
For what it's worth, I didn't have any major issues implementing the
Windows driver based on the previous version of Gerd's spec.
>> We can try to put a carefully worded sentence here that would be
>> general enough so that no reasonable changes to the Linux input layer
>> will invalidate it. Something like
>>
>> "The semantics of this device configuration follows that of the
>> EVIOCGPROP ioctl for Linux evdev devices. Input device properties are
>> returned in a bitmap which is to be interpreted using the INPUT_PROP_*
>> constants."
>>
>> Would that be acceptable?
>
> Second sentence seems to be dufficient provided bitmap is
> defined in the header. What does this sentence about ioctl mean?
> Can we just drop it?
As above, it's just us trying to make it clear that this is a thin
layer above evdev, and hence referencing it heavily.
>> >> \item[VIRTIO_INPUT_CFG_EV_BITS]
>> >> \field{subsel} specifies the event type (EV_*). If \field{size} is
>> >> -non-zero the event type is supported and a bitmap the of supported
>> >> +non-zero the event type is supported and a bitmap of supported
>> >> event codes is returned in \field{u.bitmap}.
>> >>
>> >> -Same as EVIOCGBIT ioctl.
>> >> +Same as EVIOCGBIT ioctl for Linux evdev devices.
>> >>
>> >> \item[VIRTIO_INPUT_CFG_ABS_INFO]
>> >> \field{subsel} specifies the absolute axes (ABS_*).
>> >> -Informations about the axis will be returned in \field{u.abs}.
>> >> +Information about the axis will be returned in \field{u.abs}.
>> >>
>> >> -Same as EVIOCGABS ioctl.
>> >> +Same as EVIOCGABS ioctl for Linux evdev devices.
>> >>
>> >> \end{description}
>> >>
>> >> @@ -101,19 +119,33 @@ Same as EVIOCGABS ioctl.
>> >> \item The eventq is populated with receive buffers.
>> >> \end{enumerate}
>> >>
>> >> +\drivernormative{\subsubsection}{Device Initialization}{Device Types / Input Device / Device Initialization}
>> >> +A driver MUST set both \field{select} and \field{subsel} when querying
>> >> + device configuration, in any order.
>> >> +
>> >> +A driver MUST NOT write to configuration fields other than \field{select}
>> >> + and \field{subsel}.
>> >> +
>> >> +A driver SHOULD check the \field{size} field before accessing the
>> >> + configuration information.
>> >> +
>> >> +\devicenormative{\subsubsection}{Device Initialization}{Device Types / Input Device / Device Initialization}
>> >> +A device MUST set the \field{size} field to zero if it doesn't support a
>> >> + given \field{select} and \field{subsel} combination.
>> >> +
>> >> \subsection{Device Operation}\label{sec:Device Types / Input Device / Device Operation}
>> >>
>> >> \begin{enumerate}
>> >> \item Input events such as press and release events for keys and
>> >> - buttons and motion events are sent from the device to the driver
>> >> - using the eventq.
>> >> -\item Status feedback such as keyboard led updates are sent from the
>> >> + buttons, and motion events for pointing devices are sent from
>> >> + the device to the driver using the eventq.
>> >> +\item Status feedback such as keyboard LED updates are sent from the
>> >> driver to the device using the statusq.
>> >> \item Both queues use the same virtio_input_event struct.
>> >> \field{type}, \field{code} and \field{value} are filled according to
>> >> - the linux input layer (evdev) interface, except that the fields are
>> >> + the Linux input layer (evdev) interface, except that the fields are
>> >> in little endian byte order whereas the evdev ioctl interface uses
>> >> - native endian.
>> >> + native endian-ness.
>> >> \end{enumerate}
>> >>
>> >> \begin{lstlisting}
>> >> @@ -123,3 +155,22 @@ struct virtio_input_event {
>> >> le32 value;
>> >> };
>> >> \end{lstlisting}
>> >> +
>> >> +\drivernormative{\subsubsection}{Device Operation}{Device Types / Input Device / Device Operation}
>> >> +
>> >> +A driver SHOULD keep the eventq populated with buffers. These buffers
>> >> + MUST be device-writable and MUST be at least the size of
>> >> + struct virtio_input_event.
>> >> +
>> >> +Buffers placed into the statusq by a driver MUST be at least the size
>> >> + of struct virtio_input_event.
>> >> +
>> >> +\devicenormative{\subsubsection}{Device Operation}{Device Types / Input Device / Device Operation}
>> >> +
>> >> +A device MAY drop input events if the eventq does not have enough
>> >> + available buffers. It SHOULD NOT drop individual input events if
>> >> + they are part of a sequence forming one input device update. For
>> >> + example, a pointing device update typically consists of several
>> >> + input events, one for each axis, and a terminating EV_SYN event.
>> >> + A device SHOULD either buffer or drop the entire sequence.
>> >> +
>> >> --
>> >> 2.7.4
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]