Re: [RFC v3 3/5] KVM: implement wire protocol

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2791 bytes --]

On Fri, Mar 26, 2021 at 02:21:29PM +0800, Jason Wang wrote:
> 
> 在 2021/3/17 下午9:08, Elena Afanasova 写道:
> > On Tue, 2021-03-09 at 14:19 +0800, Jason Wang wrote:
> > > On 2021/2/21 8:04 下午, Elena Afanasova wrote:
> > > > Add ioregionfd blocking read/write operations.
> > > > 
> > > > Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
> > > > ---
> > > > v3:
> > > >    - change wire protocol license
> > > >    - remove ioregionfd_cmd info and drop appropriate macros
> > > >    - fix ioregionfd state machine
> > > >    - add sizeless ioregions support
> > > >    - drop redundant check in ioregion_read/write()
> > > > 
> > > >    include/uapi/linux/ioregion.h |  30 +++++++
> > > >    virt/kvm/ioregion.c           | 162
> > > > +++++++++++++++++++++++++++++++++-
> > > >    2 files changed, 190 insertions(+), 2 deletions(-)
> > > >    create mode 100644 include/uapi/linux/ioregion.h
> > > > 
> > > > diff --git a/include/uapi/linux/ioregion.h
> > > > b/include/uapi/linux/ioregion.h
> > > > new file mode 100644
> > > > index 000000000000..58f9b5ba6186
> > > > --- /dev/null
> > > > +++ b/include/uapi/linux/ioregion.h
> > > > @@ -0,0 +1,30 @@
> > > > +/* SPDX-License-Identifier: ((GPL-2.0-only WITH Linux-syscall-
> > > > note) OR BSD-3-Clause) */
> > > > +#ifndef _UAPI_LINUX_IOREGION_H
> > > > +#define _UAPI_LINUX_IOREGION_H
> > > > +
> > > > +/* Wire protocol */
> > > > +
> > > > +struct ioregionfd_cmd {
> > > > +	__u8 cmd;
> > > > +	__u8 size_exponent : 4;
> > > > +	__u8 resp : 1;
> > > > +	__u8 padding[6];
> > > > +	__u64 user_data;
> > > > +	__u64 offset;
> > > > +	__u64 data;
> > > > +};
> > > Sorry if I've asked this before. Do we need a id for each
> > > request/response? E.g an ioregion fd could be used by multiple
> > > vCPUS.
> > > VCPU needs to have a way to find which request belongs to itself or
> > > not?
> > > 
> > I don’t think the id is necessary here since all requests/responses are
> > serialized.
> 
> 
> It's probably fine for the first version but it will degrate the performance
> e.g if the ioregionfd is used for multiple queues (e.g doorbell). The design
> should have the capability to allow the extension like this.

If there is only one doorbell register and one ioregionfd then trying to
process multiple queues in userspace is going to be slow. Adding cmd IDs
doesn't fix this because userspace won't be able to destribute cmds to
multiple queue threads efficiently.

Multiple queues should either use multiple doorbell registers or
ioregionfd needs something like datamatch to dispatch the MMIO/PIO
access to the appropriate queue's ioregionfd. In both cases cmd IDs
aren't necessary.

Can you think of a case where cmd IDs are needed?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]