Re: [win-pv-devel] [PATCH 3/5] Define new IOCTLs for user-mode clients

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx [mailto:win-pv-devel-
> bounces@xxxxxxxxxxxxxxxxxxxx] On Behalf Of Rafal Wojdyla
> Sent: 07 October 2015 05:49
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: [win-pv-devel] [PATCH 3/5] Define new IOCTLs for user-mode
> clients
> 
> IOCTL input is defined as XENIFACE_*_IN structs.
> IOCTL output is defined as XENIFACE_*_OUT structs.
> 
> Signed-off-by: Rafal Wojdyla <omeg@xxxxxxxxxxxxxxxxxxxxxx>
> ---
>  include/xeniface_ioctls.h | 190
> ++++++++++++++++++++++++++++++++++++++++++++++
>  src/xeniface/driver.h     |   2 +-
>  src/xeniface/ioctls.h     |   2 +
>  3 files changed, 193 insertions(+), 1 deletion(-)
> 
> diff --git a/include/xeniface_ioctls.h b/include/xeniface_ioctls.h
> index 1367f67..c6d9893 100644
> --- a/include/xeniface_ioctls.h
> +++ b/include/xeniface_ioctls.h
> @@ -35,14 +35,204 @@
>  DEFINE_GUID(GUID_INTERFACE_XENIFACE, \
>      0xb2cfb085, 0xaa5e, 0x47e1, 0x8b, 0xf7, 0x97, 0x93, 0xf3, 0x15, 0x45, 
> 0x65);
> 
> +/*********************************************************
> ***************/
> +/* store ioctls                                                         */
> +/*********************************************************
> ***************/

Not generally the comment style used. Could you use the interface headers as a 
style-guide?

> +// Define only for user mode clients.
> +#ifndef XENIFACE_KERNEL_MODE
> +

Could you not use _WINDLL to determine kernel-mode vs. user-mode inclusion, the 
same as the interface headers do?

> +typedef enum _XENBUS_STORE_PERMISSION_MASK {
> +    XENBUS_STORE_PERM_NONE = 0,
> +    XENBUS_STORE_PERM_READ = 1,
> +    XENBUS_STORE_PERM_WRITE = 2,
> +} XENBUS_STORE_PERMISSION_MASK;
> +
> +typedef struct _XENBUS_STORE_PERMISSION {
> +    USHORT Domain;
> +    XENBUS_STORE_PERMISSION_MASK Mask;
> +} XENBUS_STORE_PERMISSION, *PXENBUS_STORE_PERMISSION;
> +
> +#endif

Shouldn't these be prefixed XENIFACE_ rather than XENBUS_?

> +
> +#define XENIFACE_STORE_ALLOWED_PERMISSIONS
> (XENBUS_STORE_PERM_NONE | XENBUS_STORE_PERM_READ |
> XENBUS_STORE_PERM_WRITE)
> +
> +// TODO: document input/output format of these IOCTLs?

Yes, please. Could you add some doxygen tags?

>  #define IOCTL_XENIFACE_STORE_READ \
>      CTL_CODE(FILE_DEVICE_UNKNOWN, 0x800, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> +
>  #define IOCTL_XENIFACE_STORE_WRITE \
>      CTL_CODE(FILE_DEVICE_UNKNOWN, 0x801, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> +
>  #define IOCTL_XENIFACE_STORE_DIRECTORY \
>      CTL_CODE(FILE_DEVICE_UNKNOWN, 0x802, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> +
>  #define IOCTL_XENIFACE_STORE_REMOVE \
>      CTL_CODE(FILE_DEVICE_UNKNOWN, 0x803, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> 
> +#define IOCTL_XENIFACE_STORE_SET_PERMISSIONS \
> +    CTL_CODE(FILE_DEVICE_UNKNOWN, 0x804, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> +
> +#pragma warning(push)
> +#pragma warning(disable:4200) // nonstandard extension used : zero-sized
> array in struct/union

Could you not just specify an array length of one to avoid the need for this 
#pragma. That's generally how Windows APIs of this nature specify their 
structures.

> +typedef struct _XENIFACE_STORE_SET_PERMISSIONS_IN {
> +    PCHAR Path;
> +    ULONG PathLength; // number of bytes, including the null terminator
> +    ULONG NumberPermissions;
> +    XENBUS_STORE_PERMISSION Permissions[0];
> +} XENIFACE_STORE_SET_PERMISSIONS_IN,
> *PXENIFACE_STORE_SET_PERMISSIONS_IN;
> +#pragma warning(pop)
> +
> +#define IOCTL_XENIFACE_STORE_ADD_WATCH \
> +    CTL_CODE(FILE_DEVICE_UNKNOWN, 0x805, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> +
> +typedef struct _XENIFACE_STORE_ADD_WATCH_IN {
> +    PCHAR Path;
> +    ULONG PathLength; // number of bytes, including the null terminator
> +    HANDLE Event;
> +} XENIFACE_STORE_ADD_WATCH_IN,
> *PXENIFACE_STORE_ADD_WATCH_IN;
> +
> +typedef struct _XENIFACE_STORE_ADD_WATCH_OUT {
> +    PVOID Context;
> +} XENIFACE_STORE_ADD_WATCH_OUT,
> *PXENIFACE_STORE_ADD_WATCH_OUT;
> +
> +#define IOCTL_XENIFACE_STORE_REMOVE_WATCH \
> +    CTL_CODE(FILE_DEVICE_UNKNOWN, 0x806, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> +
> +typedef struct _XENIFACE_STORE_REMOVE_WATCH_IN {
> +    PVOID Context;
> +} XENIFACE_STORE_REMOVE_WATCH_IN,
> *PXENIFACE_STORE_REMOVE_WATCH_IN;
> +
> +/*********************************************************
> ***************/
> +/* evtchn ioctls                                                        */
> +/*********************************************************
> ***************/
> +#define IOCTL_XENIFACE_EVTCHN_BIND_INTERDOMAIN \
> +    CTL_CODE(FILE_DEVICE_UNKNOWN, 0x810, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> +
> +typedef struct _XENIFACE_EVTCHN_BIND_INTERDOMAIN_IN {
> +    USHORT RemoteDomain;
> +    ULONG RemotePort;
> +    BOOLEAN Mask;
> +    HANDLE Event;
> +} XENIFACE_EVTCHN_BIND_INTERDOMAIN_IN,
> *PXENIFACE_EVTCHN_BIND_INTERDOMAIN_IN;
> +
> +typedef struct _XENIFACE_EVTCHN_BIND_INTERDOMAIN_OUT {
> +    ULONG LocalPort;
> +} XENIFACE_EVTCHN_BIND_INTERDOMAIN_OUT,
> *PXENIFACE_EVTCHN_BIND_INTERDOMAIN_OUT;
> +
> +#define IOCTL_XENIFACE_EVTCHN_BIND_UNBOUND \
> +    CTL_CODE(FILE_DEVICE_UNKNOWN, 0x811, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> +
> +typedef struct _XENIFACE_EVTCHN_BIND_UNBOUND_IN {
> +    USHORT RemoteDomain;
> +    BOOLEAN Mask;
> +    HANDLE Event;
> +} XENIFACE_EVTCHN_BIND_UNBOUND_IN,
> *PXENIFACE_EVTCHN_BIND_UNBOUND_IN;
> +
> +typedef struct _XENIFACE_EVTCHN_BIND_UNBOUND_OUT {
> +    ULONG LocalPort;
> +} XENIFACE_EVTCHN_BIND_UNBOUND_OUT,
> *PXENIFACE_EVTCHN_BIND_UNBOUND_OUT;
> +
> +#define IOCTL_XENIFACE_EVTCHN_CLOSE \
> +    CTL_CODE(FILE_DEVICE_UNKNOWN, 0x812, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> +
> +typedef struct _XENIFACE_EVTCHN_CLOSE_IN {
> +    ULONG LocalPort;
> +} XENIFACE_EVTCHN_CLOSE_IN, *PXENIFACE_EVTCHN_CLOSE_IN;
> +
> +#define IOCTL_XENIFACE_EVTCHN_NOTIFY \
> +    CTL_CODE(FILE_DEVICE_UNKNOWN, 0x813, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> +
> +typedef struct _XENIFACE_EVTCHN_NOTIFY_IN {
> +    ULONG LocalPort;
> +} XENIFACE_EVTCHN_NOTIFY_IN, *PXENIFACE_EVTCHN_NOTIFY_IN;
> +
> +#define IOCTL_XENIFACE_EVTCHN_UNMASK \
> +    CTL_CODE(FILE_DEVICE_UNKNOWN, 0x814, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> +
> +typedef struct _XENIFACE_EVTCHN_UNMASK_IN {
> +    ULONG LocalPort;
> +} XENIFACE_EVTCHN_UNMASK_IN, *PXENIFACE_EVTCHN_UNMASK_IN;
> +
> +/*********************************************************
> ***************/
> +/* gntmem ioctls                                                        */
> +/*********************************************************
> ***************/
> +// This IOCTL is pended forever, use
> IOCTL_XENIFACE_GNTTAB_GET_GRANT_RESULT
> +// to get the result. This IOCTL must be asynchronous.
> +#define IOCTL_XENIFACE_GNTTAB_PERMIT_FOREIGN_ACCESS \
> +    CTL_CODE(FILE_DEVICE_UNKNOWN, 0x820, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> +
> +typedef enum _XENIFACE_GNTTAB_PAGE_FLAGS {
> +    XENIFACE_GNTTAB_READONLY          = 1 << 0,
> +    XENIFACE_GNTTAB_USE_NOTIFY_OFFSET = 1 << 1,
> +    XENIFACE_GNTTAB_USE_NOTIFY_PORT   = 1 << 2,
> +} XENIFACE_GNTTAB_PAGE_FLAGS;
> +
> +typedef struct _XENIFACE_GNTTAB_PERMIT_FOREIGN_ACCESS_IN {
> +    ULONG RequestId; // should be unique for each request

Is that unique globally, per-process or per-thread? Per-thread would be most 
convenient for the user, I guess.

> +    USHORT RemoteDomain;
> +    ULONG NumberPages;
> +    XENIFACE_GNTTAB_PAGE_FLAGS Flags;
> +    ULONG NotifyOffset;
> +    ULONG NotifyPort;
> +} XENIFACE_GNTTAB_PERMIT_FOREIGN_ACCESS_IN,
> *PXENIFACE_GNTTAB_PERMIT_FOREIGN_ACCESS_IN;
> +
> +#define IOCTL_XENIFACE_GNTTAB_GET_GRANT_RESULT \
> +    CTL_CODE(FILE_DEVICE_UNKNOWN, 0x821, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> +
> +typedef struct _XENIFACE_GNTTAB_GET_GRANT_RESULT_IN {
> +    ULONG RequestId;
> +} XENIFACE_GNTTAB_GET_GRANT_RESULT_IN,
> *PXENIFACE_GNTTAB_GET_GRANT_RESULT_IN;
> +
> +#pragma warning(push)
> +#pragma warning(disable:4200) // nonstandard extension used : zero-sized
> array in struct/union
> +typedef struct _XENIFACE_GNTTAB_GET_GRANT_RESULT_OUT {
> +    PVOID Address;
> +    ULONG References[0];

Again, use array length 1 to avoid the #pragma

> +} XENIFACE_GNTTAB_GET_GRANT_RESULT_OUT,
> *PXENIFACE_GNTTAB_GET_GRANT_RESULT_OUT;
> +#pragma warning(pop)
> +
> +#define IOCTL_XENIFACE_GNTTAB_REVOKE_FOREIGN_ACCESS \
> +    CTL_CODE(FILE_DEVICE_UNKNOWN, 0x822, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> +
> +typedef struct _XENIFACE_GNTTAB_REVOKE_FOREIGN_ACCESS_IN {
> +    ULONG RequestId;
> +} XENIFACE_GNTTAB_REVOKE_FOREIGN_ACCESS_IN,
> *PXENIFACE_GNTTAB_REVOKE_FOREIGN_ACCESS_IN;
> +
> +// This IOCTL is pended forever, use
> IOCTL_XENIFACE_GNTTAB_GET_MAP_RESULT
> +// to get the result. This IOCTL must be asynchronous.
> +#define IOCTL_XENIFACE_GNTTAB_MAP_FOREIGN_PAGES \
> +    CTL_CODE(FILE_DEVICE_UNKNOWN, 0x823, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> +
> +#pragma warning(push)
> +#pragma warning(disable:4200) // nonstandard extension used : zero-sized
> array in struct/union
> +typedef struct _XENIFACE_GNTTAB_MAP_FOREIGN_PAGES_IN {
> +    ULONG RequestId; // should be unique for each request
> +    USHORT RemoteDomain;
> +    ULONG NumberPages;
> +    XENIFACE_GNTTAB_PAGE_FLAGS Flags;
> +    ULONG NotifyOffset;
> +    ULONG NotifyPort;
> +    ULONG References[0];

And again.

  Paul

> +} XENIFACE_GNTTAB_MAP_FOREIGN_PAGES_IN,
> *PXENIFACE_GNTTAB_MAP_FOREIGN_PAGES_IN;
> +#pragma warning(pop)
> +
> +#define IOCTL_XENIFACE_GNTTAB_GET_MAP_RESULT \
> +    CTL_CODE(FILE_DEVICE_UNKNOWN, 0x824, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> +
> +typedef struct _XENIFACE_GNTTAB_GET_MAP_RESULT_IN {
> +    ULONG RequestId;
> +} XENIFACE_GNTTAB_GET_MAP_RESULT_IN,
> *PXENIFACE_GNTTAB_GET_MAP_RESULT_IN;
> +
> +typedef struct _XENIFACE_GNTTAB_GET_MAP_RESULT_OUT {
> +    PVOID Address;
> +} XENIFACE_GNTTAB_GET_MAP_RESULT_OUT,
> *PXENIFACE_GNTTAB_GET_MAP_RESULT_OUT;
> +
> +#define IOCTL_XENIFACE_GNTTAB_UNMAP_FOREIGN_PAGES \
> +    CTL_CODE(FILE_DEVICE_UNKNOWN, 0x825, METHOD_BUFFERED,
> FILE_ANY_ACCESS)
> +
> +typedef struct _XENIFACE_GNTTAB_UNMAP_FOREIGN_PAGES_IN {
> +    ULONG RequestId;
> +} XENIFACE_GNTTAB_UNMAP_FOREIGN_PAGES_IN,
> *PXENIFACE_GNTTAB_UNMAP_FOREIGN_PAGES_IN;
> +
>  #endif // _XENIFACE_IOCTLS_H_
> 
> diff --git a/src/xeniface/driver.h b/src/xeniface/driver.h
> index 00f2d8f..313f297 100644
> --- a/src/xeniface/driver.h
> +++ b/src/xeniface/driver.h
> @@ -32,7 +32,7 @@
>  #ifndef _XENIFACE_DRIVER_H
>  #define _XENIFACE_DRIVER_H
> 
> -
> +#define XENIFACE_KERNEL_MODE
> 
>  #include "fdo.h"
>  #include "types.h"
> diff --git a/src/xeniface/ioctls.h b/src/xeniface/ioctls.h
> index 63de9eb..7ee7801 100644
> --- a/src/xeniface/ioctls.h
> +++ b/src/xeniface/ioctls.h
> @@ -32,6 +32,8 @@
>  #ifndef _IOCTLS_H_
>  #define _IOCTLS_H_
> 
> +#define XENIFACE_KERNEL_MODE
> +
>  NTSTATUS
>  XenIFaceIoctl(
>      __in  PXENIFACE_FDO         Fdo,
> --
> 1.8.1.msysgit.1
> 
> _______________________________________________
> win-pv-devel mailing list
> win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel

_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
http://lists.xenproject.org/cgi-bin/mailman/listinfo/win-pv-devel