Re: [RFC v3 08/19] vfio-user: define socket receive functions

2021-12-06 Thread John Johnson


> On Nov 19, 2021, at 2:42 PM, Alex Williamson  
> wrote:
> 
> On Mon,  8 Nov 2021 16:46:36 -0800
> John Johnson  wrote:
> 
>> Add infrastructure needed to receive incoming messages
>> 
>> Signed-off-by: John G Johnson 
>> Signed-off-by: Elena Ufimtseva 
>> Signed-off-by: Jagannathan Raman 
>> ---
>> hw/vfio/pci.h   |   2 +-
>> hw/vfio/user-protocol.h |  62 +
>> hw/vfio/user.h  |   9 +-
>> hw/vfio/pci.c   |  12 +-
>> hw/vfio/user.c  | 326 
>> 
>> MAINTAINERS |   1 +
>> 6 files changed, 409 insertions(+), 3 deletions(-)
>> create mode 100644 hw/vfio/user-protocol.h
>> 
>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>> index 08ac647..ec9f345 100644
>> --- a/hw/vfio/pci.h
>> +++ b/hw/vfio/pci.h
>> @@ -193,7 +193,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(VFIOUserPCIDevice, 
>> VFIO_USER_PCI)
>> struct VFIOUserPCIDevice {
>> VFIOPCIDevice device;
>> char *sock_name;
>> -bool secure_dma; /* disable shared mem for DMA */
> 
> Don't introduce it into the series to start with, confusing to review.
> 

ok

>> +bool send_queued;   /* all sends are queued */
>> };
>> 
>> /* Use uin32_t for vendor & device so PCI_ANY_ID expands and cannot match hw 
>> */
>> diff --git a/hw/vfio/user-protocol.h b/hw/vfio/user-protocol.h
>> new file mode 100644
>> index 000..27062cb
>> --- /dev/null
>> +++ b/hw/vfio/user-protocol.h
>> @@ -0,0 +1,62 @@
>> +#ifndef VFIO_USER_PROTOCOL_H
>> +#define VFIO_USER_PROTOCOL_H
>> +
>> +/*
>> + * vfio protocol over a UNIX socket.
>> + *
>> + * Copyright © 2018, 2021 Oracle and/or its affiliates.
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>> + * the COPYING file in the top-level directory.
>> + *
>> + * Each message has a standard header that describes the command
>> + * being sent, which is almost always a VFIO ioctl().
>> + *
>> + * The header may be followed by command-specific data, such as the
>> + * region and offset info for read and write commands.
>> + */
>> +
>> +typedef struct {
>> +uint16_t id;
>> +uint16_t command;
>> +uint32_t size;
>> +uint32_t flags;
>> +uint32_t error_reply;
>> +} VFIOUserHdr;
>> +
> 
> A comment referencing the doc would probably be a good idea about here.
> 

ok

>> +/* VFIOUserHdr commands */
>> +enum vfio_user_command {
>> +VFIO_USER_VERSION   = 1,
>> +VFIO_USER_DMA_MAP   = 2,
>> +VFIO_USER_DMA_UNMAP = 3,
>> +VFIO_USER_DEVICE_GET_INFO   = 4,
>> +VFIO_USER_DEVICE_GET_REGION_INFO= 5,
>> +VFIO_USER_DEVICE_GET_REGION_IO_FDS  = 6,
>> +VFIO_USER_DEVICE_GET_IRQ_INFO   = 7,
>> +VFIO_USER_DEVICE_SET_IRQS   = 8,
>> +VFIO_USER_REGION_READ   = 9,
>> +VFIO_USER_REGION_WRITE  = 10,
>> +VFIO_USER_DMA_READ  = 11,
>> +VFIO_USER_DMA_WRITE = 12,
>> +VFIO_USER_DEVICE_RESET  = 13,
>> +VFIO_USER_DIRTY_PAGES   = 14,
>> +VFIO_USER_MAX,
>> +};
>> +
>> +/* VFIOUserHdr flags */
>> +#define VFIO_USER_REQUEST   0x0
>> +#define VFIO_USER_REPLY 0x1
>> +#define VFIO_USER_TYPE  0xF
>> +
>> +#define VFIO_USER_NO_REPLY  0x10
>> +#define VFIO_USER_ERROR 0x20
>> +
>> +
>> +#define VFIO_USER_DEF_MAX_FDS   8
>> +#define VFIO_USER_MAX_MAX_FDS   16
>> +
>> +#define VFIO_USER_DEF_MAX_XFER  (1024 * 1024)
>> +#define VFIO_USER_MAX_MAX_XFER  (64 * 1024 * 1024)
> 
> These are essentially magic numbers, some discussion of how these
> limits are derived would be useful for future contributors, but also
> only DEV_MAX_XFER is used in this patch and it's confusing why the
> macro isn't used directly.  Most of the logic surrounding these is
> added in the next patch, so it doesn't really make sense to add them
> here.  Thanks,
> 

ok

JJ





Re: [RFC v3 08/19] vfio-user: define socket receive functions

2021-11-19 Thread Alex Williamson
On Mon,  8 Nov 2021 16:46:36 -0800
John Johnson  wrote:

> Add infrastructure needed to receive incoming messages
> 
> Signed-off-by: John G Johnson 
> Signed-off-by: Elena Ufimtseva 
> Signed-off-by: Jagannathan Raman 
> ---
>  hw/vfio/pci.h   |   2 +-
>  hw/vfio/user-protocol.h |  62 +
>  hw/vfio/user.h  |   9 +-
>  hw/vfio/pci.c   |  12 +-
>  hw/vfio/user.c  | 326 
> 
>  MAINTAINERS |   1 +
>  6 files changed, 409 insertions(+), 3 deletions(-)
>  create mode 100644 hw/vfio/user-protocol.h
> 
> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
> index 08ac647..ec9f345 100644
> --- a/hw/vfio/pci.h
> +++ b/hw/vfio/pci.h
> @@ -193,7 +193,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(VFIOUserPCIDevice, 
> VFIO_USER_PCI)
>  struct VFIOUserPCIDevice {
>  VFIOPCIDevice device;
>  char *sock_name;
> -bool secure_dma; /* disable shared mem for DMA */

Don't introduce it into the series to start with, confusing to review.

> +bool send_queued;   /* all sends are queued */
>  };
>  
>  /* Use uin32_t for vendor & device so PCI_ANY_ID expands and cannot match hw 
> */
> diff --git a/hw/vfio/user-protocol.h b/hw/vfio/user-protocol.h
> new file mode 100644
> index 000..27062cb
> --- /dev/null
> +++ b/hw/vfio/user-protocol.h
> @@ -0,0 +1,62 @@
> +#ifndef VFIO_USER_PROTOCOL_H
> +#define VFIO_USER_PROTOCOL_H
> +
> +/*
> + * vfio protocol over a UNIX socket.
> + *
> + * Copyright © 2018, 2021 Oracle and/or its affiliates.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + * Each message has a standard header that describes the command
> + * being sent, which is almost always a VFIO ioctl().
> + *
> + * The header may be followed by command-specific data, such as the
> + * region and offset info for read and write commands.
> + */
> +
> +typedef struct {
> +uint16_t id;
> +uint16_t command;
> +uint32_t size;
> +uint32_t flags;
> +uint32_t error_reply;
> +} VFIOUserHdr;
> +

A comment referencing the doc would probably be a good idea about here.

> +/* VFIOUserHdr commands */
> +enum vfio_user_command {
> +VFIO_USER_VERSION   = 1,
> +VFIO_USER_DMA_MAP   = 2,
> +VFIO_USER_DMA_UNMAP = 3,
> +VFIO_USER_DEVICE_GET_INFO   = 4,
> +VFIO_USER_DEVICE_GET_REGION_INFO= 5,
> +VFIO_USER_DEVICE_GET_REGION_IO_FDS  = 6,
> +VFIO_USER_DEVICE_GET_IRQ_INFO   = 7,
> +VFIO_USER_DEVICE_SET_IRQS   = 8,
> +VFIO_USER_REGION_READ   = 9,
> +VFIO_USER_REGION_WRITE  = 10,
> +VFIO_USER_DMA_READ  = 11,
> +VFIO_USER_DMA_WRITE = 12,
> +VFIO_USER_DEVICE_RESET  = 13,
> +VFIO_USER_DIRTY_PAGES   = 14,
> +VFIO_USER_MAX,
> +};
> +
> +/* VFIOUserHdr flags */
> +#define VFIO_USER_REQUEST   0x0
> +#define VFIO_USER_REPLY 0x1
> +#define VFIO_USER_TYPE  0xF
> +
> +#define VFIO_USER_NO_REPLY  0x10
> +#define VFIO_USER_ERROR 0x20
> +
> +
> +#define VFIO_USER_DEF_MAX_FDS   8
> +#define VFIO_USER_MAX_MAX_FDS   16
> +
> +#define VFIO_USER_DEF_MAX_XFER  (1024 * 1024)
> +#define VFIO_USER_MAX_MAX_XFER  (64 * 1024 * 1024)

These are essentially magic numbers, some discussion of how these
limits are derived would be useful for future contributors, but also
only DEV_MAX_XFER is used in this patch and it's confusing why the
macro isn't used directly.  Most of the logic surrounding these is
added in the next patch, so it doesn't really make sense to add them
here.  Thanks,

Alex

> +
> +
> +#endif /* VFIO_USER_PROTOCOL_H */
> diff --git a/hw/vfio/user.h b/hw/vfio/user.h
> index 301ef6a..bd3717f 100644
> --- a/hw/vfio/user.h
> +++ b/hw/vfio/user.h
> @@ -11,6 +11,8 @@
>   *
>   */
>  
> +#include "user-protocol.h"
> +
>  typedef struct {
>  int send_fds;
>  int recv_fds;
> @@ -27,6 +29,7 @@ enum msg_type {
>  
>  typedef struct VFIOUserMsg {
>  QTAILQ_ENTRY(VFIOUserMsg) next;
> +VFIOUserHdr *hdr;
>  VFIOUserFDs *fds;
>  uint32_t rsize;
>  uint32_t id;
> @@ -70,9 +73,13 @@ typedef struct VFIOProxy {
>  } VFIOProxy;
>  
>  /* VFIOProxy flags */
> -#define VFIO_PROXY_CLIENT   0x1
> +#define VFIO_PROXY_CLIENT0x1
> +#define VFIO_PROXY_FORCE_QUEUED  0x4
>  
>  VFIOProxy *vfio_user_connect_dev(SocketAddress *addr, Error **errp);
>  void vfio_user_disconnect(VFIOProxy *proxy);
> +void vfio_user_set_handler(VFIODevice *vbasedev,
> +   void (*handler)(void *opaque, VFIOUserMsg *msg),
> +   void *reqarg);
>  
>  #endif /* VFIO_USER_H */
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index ebfabb1..db45179 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3448,6 +3448,11 @@ struct VFIOValidOps vfio_pci_valid_ops = {
>   * vfio-user routines.
>   */
>  
>