From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id ABB46A04B6; Mon, 24 Aug 2020 22:53:50 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 4B7EF58C4; Mon, 24 Aug 2020 22:53:49 +0200 (CEST) Received: from mail-lf1-f68.google.com (mail-lf1-f68.google.com [209.85.167.68]) by dpdk.org (Postfix) with ESMTP id DF99531FC for ; Mon, 24 Aug 2020 22:53:47 +0200 (CEST) Received: by mail-lf1-f68.google.com with SMTP id d2so5233901lfj.1 for ; Mon, 24 Aug 2020 13:53:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=+fQS7zXzjfOGd2O19AfRFyYpJmk9pibKozIH6PpVZC0=; b=lup9kS6dWpUXZGx8NghcSqwOPmkSXDtYKtZlx1a4bOlF5cY+ktTZA33+BbZgwOg25J snqbZCToNmNwRd6jDg3u5BqUR8KxFpM+0qnwKao7XmM7BeEQzfR32nOLXKsWAwhrOAqZ bsVLmLxLtuNyX31NbgQXOdnJLzzEGGF6UGrqHZK95PqigEycX7uadzH4Rdr4iCOCPuwC m6QjCnMghNfhfKieTKXx+iuNK7QURk/ckh6+RJ9sBLgMcGXrJX5XtcDJuT/nWUoy+Qes mpSXxBLqlBYFvP15i5i8zrc+7Im1GKLTV8NGc+79ZGiGFGd2hFvIuYC/CC9HWBOC9m8c vvYA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=+fQS7zXzjfOGd2O19AfRFyYpJmk9pibKozIH6PpVZC0=; b=cqlojA0QYHYOo1YpQcbV63bnET20s8zrfYnz3zUYS29qjCjbdopgtmk2XpkaR2q13K yzzRUkaVBAIYF6EBBq1R/vNXD2AENNQReO/XFMW+g1Y7JvVCw8sDd11o7u66qJHRyu1/ 1sD3Ly0bEwVJlkxaueKh0QIS8C9yt9qnPNQVHlgpuO6l1leXM3BggeIgLzHUDPqm3Xqw fIHrfzAl5stg7uA1uaIDyPcwR97xgV+xnba+YqPYn9XrhC3JCnN/nxcT2pQgM1JH43pK XGSAP4oEVWF/PoMjQ646JIUKt7OGPpmmBhBlsmXuEW/XwmUcDtxBaLMWXicv8OLDxz9Q 6Caw== X-Gm-Message-State: AOAM533jIv+kKk0auo3aEQAl8jJAUsvSWHncyUPSAwDXrCq7Rok5ZS/K f/5AGBHbzUpswCr/qObhn3w= X-Google-Smtp-Source: ABdhPJxQmUj6lh6adYukhrexjZmKu4HmlHAitnlm7/sueUHKMoN88WxHpy7x1LCJjuVGKaERExIeKg== X-Received: by 2002:ac2:4911:: with SMTP id n17mr3339949lfi.185.1598302427186; Mon, 24 Aug 2020 13:53:47 -0700 (PDT) Received: from sovereign (broadband-37-110-65-23.ip.moscow.rt.ru. [37.110.65.23]) by smtp.gmail.com with ESMTPSA id x17sm2432720ljm.0.2020.08.24.13.53.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 24 Aug 2020 13:53:46 -0700 (PDT) Date: Mon, 24 Aug 2020 23:53:44 +0300 From: Dmitry Kozlyuk To: Narcisa Ana Maria Vasile Cc: dev@dpdk.org, thomas@monjalon.net, ocardona@microsoft.com, haramakr@linux.microsoft.com, pallavi.kadam@intel.com, ranjit.menon@intel.com, dmitrym@microsoft.com, Narcisa Vasile , Harini Ramakrishnan Message-ID: <20200824235344.5994cd13@sovereign> In-Reply-To: <1597962235-4787-1-git-send-email-navasile@linux.microsoft.com> References: <1597962235-4787-1-git-send-email-navasile@linux.microsoft.com> X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2] windows/netuio: add Windows NetUIO kernel driver X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, 20 Aug 2020 15:23:55 -0700, Narcisa Ana Maria Vasile wrote: > From: Narcisa Vasile > > The Windows NetUIO kernel driver allows the DPDK userspace > application to directly access the hardware. > > Cc: Harini Ramakrishnan > Cc: Omar Cardona > Signed-off-by: Narcisa Vasile > --- Major questions: 1. Does NetUIO still need to allocate and map a contiguous memory segment now, when DPDK has user-mode memory management? 2. IOCTLs require to specify PCI address on each call. This is very inconvenient for DPDK consumers and also seems to serve no purpose. 3. There is a need to document driver's design, preferably in commit message, specifically: 3.1) DMA remapping capability in INF (AFAIK, vendors are notified); 3.2) manual BAR probing instead of using resource lists; 3.3) reason to use EvtIoInCallerContext and IO queues; 3.4) IOCTL format. Also, I agree with everything Ranjit has noted already. General suggestions to cleanup the code a bit. We can do it later if you wish. This also brings up the question, which code style should Windows kernel code for DPDK follow (off-topic for now). * Remove boilerplate code and comments generated by VS wizard. * Place `_Use_decl_annotations` on definitions to make them simpler. * Limit line length, try using shorted variable names (e.g. "netuio_contextdata" may be "context" or "ctx" with no loss). More specific comments inline. > create mode 100644 .gitattributes > create mode 100644 .gitignore Both of these files should be in windows/ directory. > create mode 100644 windows/netuio/kernel/windows/netuio/resource.h This file is not needed. > diff --git a/windows/netuio/kernel/README_NetUIO.rst b/windows/netuio/kernel/README_NetUIO.rst > new file mode 100644 > index 000000000..a290fcf20 > --- /dev/null > +++ b/windows/netuio/kernel/README_NetUIO.rst > @@ -0,0 +1,64 @@ [snip] > +Installing netuio.sys Driver for development > +-------------------------------------------- > +.. note:: > + > +In a development environment, NetUIO driver can be loaded by enabling test-signing. > +Please implement adequate precautionary measures before installing a test-signed driver, replacing a signed-driver. Paragraph under "note" must be indented for RST to peek it up as note content. Like so: .. note:: This line has 3 spaces in the beginning. [snip] > diff --git a/windows/netuio/kernel/windows/netuio/netuio.inf b/windows/netuio/kernel/windows/netuio/netuio.inf > new file mode 100644 > index 000000000..88e90b365 > --- /dev/null > +++ b/windows/netuio/kernel/windows/netuio/netuio.inf > @@ -0,0 +1,78 @@ > +; SPDX-License-Identifier: BSD-3-Clause > +; Copyright (c) Microsoft Corporation. All rights reserved This copyright differs from the one in README. "All rights reserved" is probably unnecessary. [snip] > +[Strings] > +SPSVCINST_ASSOCSERVICE= 0x00000002 > +Intel = "Intel" > +Broadcom = "Broadcom Corporation" IHVs are supposed to add this gradually. > +ClassName = "Windows UIO" > +DiskName = "DPDK netUIO Installation Disk" > +netuio.DeviceDesc = "netuio Device" > +netuio.SVCDESC = "netuio Service" > + > +[DMAr.reg] > +HKR,Parameters,DmaRemappingCompatible,0x00010001,1 > diff --git a/windows/netuio/kernel/windows/netuio/netuio_dev.c b/windows/netuio/kernel/windows/netuio/netuio_dev.c > new file mode 100644 > index 000000000..6394bb5d1 > --- /dev/null > +++ b/windows/netuio/kernel/windows/netuio/netuio_dev.c [snip] > +NTSTATUS > +netuio_create_device(_Inout_ PWDFDEVICE_INIT DeviceInit) > +{ [snip] > + > + status = WdfDeviceCreate(&DeviceInit, &deviceAttributes, &device); > + > + if (NT_SUCCESS(status)) { > + // Create a device interface so that applications can find and talk to us. > + status = WdfDeviceCreateDeviceInterface(device, &GUID_DEVINTERFACE_netUIO, NULL); > + } Mixed tabs and spaces for indent. [snip] > +/* > +Routine Description: > + Free all the resources allocated in AdfEvtDeviceAdd. Typo: Adf -> Wdf. [snip] > +static NTSTATUS > +get_pci_device_info(_In_ WDFOBJECT device) > +{ [snip] > + // Also, retrieve the NUMA node of the device > + USHORT numaNode; > + status = IoGetDeviceNumaNode(pdo, &numaNode); > + if (NT_SUCCESS(status)) { > + netuio_contextdata->dev_numa_node = numaNode; > + } Why is this needed? Userspace has access to this info. > + } > + > + return status; > +} > + > +static NTSTATUS > +create_device_specific_symbolic_link(_In_ WDFOBJECT device) > +{ > + NTSTATUS status = STATUS_UNSUCCESSFUL; > + UNICODE_STRING netuio_symbolic_link; > + > + PNETUIO_CONTEXT_DATA netuio_contextdata; > + netuio_contextdata = netuio_get_context_data(device); > + > + if (!netuio_contextdata) > + return status; > + > + // Build symbolic link name as _BDF (bus/device/func) > + CHAR symbolic_link[64] = { 0 }; > + sprintf_s(symbolic_link, sizeof(symbolic_link), "%s_%04d%02d%02d", > + NETUIO_DEVICE_SYMBOLIC_LINK_ANSI, netuio_contextdata->addr.bus_num, > + netuio_contextdata->addr.dev_num, netuio_contextdata->addr.func_num); > + > + ANSI_STRING ansi_symbolic_link; > + RtlInitAnsiString(&ansi_symbolic_link, symbolic_link); > + > + status = RtlAnsiStringToUnicodeString(&netuio_symbolic_link, &ansi_symbolic_link, TRUE); > + if (!NT_SUCCESS(status)) > + return status; Why not use Unicode directly? > + > + status = WdfDeviceCreateSymbolicLink(device, &netuio_symbolic_link); > + > + RtlFreeUnicodeString(&netuio_symbolic_link); > + > + return status; > +} [snip] > + > +// Local function protoyypes > +static NTSTATUS get_pci_device_info(_In_ WDFOBJECT device); > +static NTSTATUS create_device_specific_symbolic_link(_In_ WDFOBJECT device); > +static NTSTATUS allocate_usermemory_segment(_In_ WDFOBJECT device); > +static VOID free_usermemory_segment(_In_ WDFOBJECT device); Nit: local functions usually don't appear in headers. [snip] > diff --git a/windows/netuio/kernel/windows/netuio/netuio_interface.h b/windows/netuio/kernel/windows/netuio/netuio_interface.h > new file mode 100644 > index 000000000..6674931d2 > --- /dev/null > +++ b/windows/netuio/kernel/windows/netuio/netuio_interface.h > @@ -0,0 +1,73 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright (c) Microsoft Corporation. All rights reserved > + */ > + > +#ifndef NETUIO_INTERFACE_H > +#define NETUIO_INTERFACE_H > + > +// All structures in this file are packed on an 8B boundary. > +#pragma pack(push) > +#pragma pack(8) > + > +// Define an Interface Guid so that any app can find the device and talk to it. > +DEFINE_GUID (GUID_DEVINTERFACE_netUIO, 0x08336f60,0x0679,0x4c6c,0x85,0xd2,0xae,0x7c,0xed,0x65,0xff,0xf7); // {08336f60-0679-4c6c-85d2-ae7ced65fff7} > + > +// Device name definitions > +#define NETUIO_DEVICE_SYMBOLIC_LINK_ANSI "\\DosDevices\\netuio" > + > +// netUIO driver symbolic name (prefix) > +#define NETUIO_DRIVER_NAME _T("netuio") > + > +// IOCTL code definitions > +#define IOCTL_NETUIO_MAP_HW_INTO_USERMODE CTL_CODE(FILE_DEVICE_NETWORK, 51, METHOD_BUFFERED, FILE_READ_ACCESS | FILE_WRITE_ACCESS) > +#define IOCTL_NETUIO_PCI_CONFIG_IO CTL_CODE(FILE_DEVICE_NETWORK, 52, METHOD_BUFFERED, FILE_READ_ACCESS | FILE_WRITE_ACCESS) Input and output of these interfaces has to be documented somewhere. > + > +struct mem_region { > + UINT64 size; // memory region size > + PHYSICAL_ADDRESS phys_addr; // physical address of the memory region > + PVOID virt_addr; // virtual address of the memory region > + PVOID user_mapped_virt_addr; // virtual address of the region mapped into user process context > +}; > + > +struct dev_addr { > + ULONG bus_num; > + USHORT dev_num; > + USHORT func_num; > +}; > + > +enum pci_io { > + PCI_IO_READ = 0, > + PCI_IO_WRITE = 1 > +}; > + > +#define PCI_MAX_BAR 6 > + > +struct dpdk_private_info Nit: "private" usually means something hidden, not exposed like this. How about "device_info", "device_data", or "device_resources"? > +{ > + struct mem_region hw[PCI_MAX_BAR]; > + struct mem_region ms; > + struct dev_addr dev_addr; Why is the address needed? Each device has its own symlink and handle. Such interface is harder to use with no visible benefit. Same below. > + UINT16 dev_id; > + UINT16 sub_dev_id; > + USHORT dev_numa_node; > + USHORT reserved; > +}; > + > +struct dpdk_pci_config_io > +{ > + struct dev_addr dev_addr; > + UINT32 offset; > + enum pci_io op; Driver API should use fixed-size types. Enum size is unspecified. > + UINT32 access_size; // 1, 2, 4, or 8 bytes > + > + union dpdk_pci_config_io_data { > + UINT8 u8; > + UINT16 u16; > + UINT32 u32; > + UINT64 u64; > + } data; > +}; > + > +#pragma pack(pop) > + > +#endif // NETUIO_INTERFACE_H [snip] > +static NTSTATUS > +netuio_map_address_into_user_process(_In_ PNETUIO_CONTEXT_DATA netuio_contextdata) > +{ > + NTSTATUS status = STATUS_SUCCESS; > + > + // Map the scratch memory regions to the user's process context > + MmBuildMdlForNonPagedPool(netuio_contextdata->dpdk_seg.mdl); > + __try { > + netuio_contextdata->dpdk_seg.mem.user_mapped_virt_addr = > + MmMapLockedPagesSpecifyCache(netuio_contextdata->dpdk_seg.mdl, UserMode, > + MmCached, NULL, FALSE, NormalPagePriority); If user does IOCTL twice, they'll get the same memory mapped twice to different addresses, but the driver will only remember the last mapping. After it is removed, the first mapping will remain, exposing kernel memory for user-after-free from userspace. Or do I miss something here? DPDK doesn't need multiple mappings, so you could either return an error or just an already mapped address on the second call. [snip] > +NTSTATUS > +netuio_queue_initialize(_In_ WDFDEVICE Device) > +{ > + WDFQUEUE queue; > + NTSTATUS status; > + WDF_IO_QUEUE_CONFIG queueConfig; > + > + PAGED_CODE(); > + > + // Configure a default queue so that requests that are not > + // configure-fowarded using WdfDeviceConfigureRequestDispatching to goto > + // other queues get dispatched here. > + WDF_IO_QUEUE_CONFIG_INIT_DEFAULT_QUEUE(&queueConfig, WdfIoQueueDispatchParallel); > + > + queueConfig.EvtIoDeviceControl = netuio_evt_IO_device_control; > + queueConfig.EvtIoStop = netuio_evt_IO_stop; EvtIoStop is not needed. [snip] > +_Use_decl_annotations_ > +VOID > +netuio_evt_IO_in_caller_context( > + IN WDFDEVICE Device, > + IN WDFREQUEST Request > +) > +{ > + WDF_REQUEST_PARAMETERS params = { 0 }; > + NTSTATUS status = STATUS_SUCCESS; > + PVOID input_buf = NULL, output_buf = NULL; > + size_t input_buf_size, output_buf_size; > + size_t bytes_returned = 0; > + PNETUIO_CONTEXT_DATA netuio_contextdata = NULL; > + > + netuio_contextdata = netuio_get_context_data(Device); > + > + WDF_REQUEST_PARAMETERS_INIT(¶ms); > + WdfRequestGetParameters(Request, ¶ms); > + > + // We only need to be in the context of the process that initiated the request > + //when we need to map memory to userspace. Otherwise, send the request back to framework. > + if (!((params.Type == WdfRequestTypeDeviceControl) && > + (params.Parameters.DeviceIoControl.IoControlCode == IOCTL_NETUIO_MAP_HW_INTO_USERMODE))) > + { > + status = WdfDeviceEnqueueRequest(Device, Request); > + > + if (!NT_SUCCESS(status)) > + { > + WdfRequestCompleteWithInformation(Request, status, bytes_returned); Nit: WdfRequestComplete(Request, status) would be sufficient. > + } > + return; > + } > + [snip] > diff --git a/windows/netuio/kernel/windows/netuio/netuio_queue.h b/windows/netuio/kernel/windows/netuio/netuio_queue.h > new file mode 100644 > index 000000000..9173a77b9 > --- /dev/null > +++ b/windows/netuio/kernel/windows/netuio/netuio_queue.h > @@ -0,0 +1,31 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright (c) Microsoft Corporation. All rights reserved > + */ > + > +#ifndef NETUIO_QUEUE_H > +#define NETUIO_QUEUE_H > + > +EXTERN_C_START Not needed in C code. > + > +// This is the context that can be placed per queue and would contain per queue information. > +typedef struct _QUEUE_CONTEXT { > + ULONG PrivateDeviceData; // just a placeholder > +} QUEUE_CONTEXT, *PQUEUE_CONTEXT; > + > +WDF_DECLARE_CONTEXT_TYPE_WITH_NAME(QUEUE_CONTEXT, QueueGetContext) Boilerplate, not needed. [snip]