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

[John Johnson]

> On Nov 19, 2021, at 2:42 PM, Alex Williamson <alex.williamson@redhat.com> 
> wrote:
> 
> On Mon,  8 Nov 2021 16:46:36 -0800
> John Johnson <john.g.johnson@oracle.com> wrote:
> 
>> Add infrastructure needed to receive incoming messages
>> 
>> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
>> Signed-off-by: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> Signed-off-by: Jagannathan Raman <jag.raman@oracle.com>
>> ---
>> 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 0000000..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