* [PATCH 2/2] PCI: hv: Add support for protocol 1.3 and support PCI_BUS_RELATIONS2
From: longli @ 2019-11-23 1:57 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas, linux-hyperv,
linux-pci, linux-kernel
Cc: Long Li
In-Reply-To: <1574474229-44840-1-git-send-email-longli@linuxonhyperv.com>
From: Long Li <longli@microsoft.com>
Starting with Hyper-V PCI protocol version 1.3, the host VSP can send
PCI_BUS_RELATIONS2 and pass the vNUMA node information for devices on the bus.
The vNUMA node tells which guest NUMA node this device is on based on guest
VM configuration topology and physical device inforamtion.
The patch adds code to negotiate v1.3 and process PCI_BUS_RELATIONS2.
Signed-off-by: Long Li <longli@microsoft.com>
---
drivers/pci/controller/pci-hyperv.c | 107 ++++++++++++++++++++++++++++
1 file changed, 107 insertions(+)
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index f2e028cfa7cd..488235563c7d 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -63,6 +63,7 @@
enum pci_protocol_version_t {
PCI_PROTOCOL_VERSION_1_1 = PCI_MAKE_VERSION(1, 1), /* Win10 */
PCI_PROTOCOL_VERSION_1_2 = PCI_MAKE_VERSION(1, 2), /* RS1 */
+ PCI_PROTOCOL_VERSION_1_3 = PCI_MAKE_VERSION(1, 3), /* VB */
};
#define CPU_AFFINITY_ALL -1ULL
@@ -72,6 +73,7 @@ enum pci_protocol_version_t {
* first.
*/
static enum pci_protocol_version_t pci_protocol_versions[] = {
+ PCI_PROTOCOL_VERSION_1_3,
PCI_PROTOCOL_VERSION_1_2,
PCI_PROTOCOL_VERSION_1_1,
};
@@ -124,6 +126,7 @@ enum pci_message_type {
PCI_RESOURCES_ASSIGNED2 = PCI_MESSAGE_BASE + 0x16,
PCI_CREATE_INTERRUPT_MESSAGE2 = PCI_MESSAGE_BASE + 0x17,
PCI_DELETE_INTERRUPT_MESSAGE2 = PCI_MESSAGE_BASE + 0x18, /* unused */
+ PCI_BUS_RELATIONS2 = PCI_MESSAGE_BASE + 0x19,
PCI_MESSAGE_MAXIMUM
};
@@ -169,6 +172,26 @@ struct pci_function_description {
u32 ser; /* serial number */
} __packed;
+enum pci_device_description_flags {
+ HV_PCI_DEVICE_FLAG_NONE = 0x0,
+ HV_PCI_DEVICE_FLAG_NUMA_AFFINITY = 0x1,
+};
+
+struct pci_function_description2 {
+ u16 v_id; /* vendor ID */
+ u16 d_id; /* device ID */
+ u8 rev;
+ u8 prog_intf;
+ u8 subclass;
+ u8 base_class;
+ u32 subsystem_id;
+ union win_slot_encoding win_slot;
+ u32 ser; /* serial number */
+ u32 flags;
+ u16 virtual_numa_node;
+ u16 reserved;
+} __packed;
+
/**
* struct hv_msi_desc
* @vector: IDT entry
@@ -304,6 +327,12 @@ struct pci_bus_relations {
struct pci_function_description func[0];
} __packed;
+struct pci_bus_relations2 {
+ struct pci_incoming_message incoming;
+ u32 device_count;
+ struct pci_function_description2 func[0];
+} __packed;
+
struct pci_q_res_req_response {
struct vmpacket_descriptor hdr;
s32 status; /* negative values are failures */
@@ -1417,6 +1446,7 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
break;
case PCI_PROTOCOL_VERSION_1_2:
+ case PCI_PROTOCOL_VERSION_1_3:
size = hv_compose_msi_req_v2(&ctxt.int_pkts.v2,
dest,
hpdev->desc.win_slot.slot,
@@ -1798,6 +1828,25 @@ static void hv_pci_remove_slots(struct hv_pcibus_device *hbus)
}
}
+/*
+ * Set NUMA node for the devices on the bus
+ */
+static void pci_assign_numa_node(struct hv_pcibus_device *hbus)
+{
+ struct pci_dev *dev;
+ struct pci_bus *bus = hbus->pci_bus;
+ struct hv_pci_dev *hv_dev;
+
+ list_for_each_entry(dev, &bus->devices, bus_list) {
+ hv_dev = get_pcichild_wslot(hbus, devfn_to_wslot(dev->devfn));
+ if (!hv_dev)
+ continue;
+
+ if (hv_dev->desc.flags & HV_PCI_DEVICE_FLAG_NUMA_AFFINITY)
+ set_dev_node(&dev->dev, hv_dev->desc.virtual_numa_node);
+ }
+}
+
/**
* create_root_hv_pci_bus() - Expose a new root PCI bus
* @hbus: Root PCI bus, as understood by this driver
@@ -1820,6 +1869,7 @@ static int create_root_hv_pci_bus(struct hv_pcibus_device *hbus)
pci_lock_rescan_remove();
pci_scan_child_bus(hbus->pci_bus);
+ pci_assign_numa_node(hbus);
pci_bus_assign_resources(hbus->pci_bus);
hv_pci_assign_slots(hbus);
pci_bus_add_devices(hbus->pci_bus);
@@ -2088,6 +2138,7 @@ static void pci_devices_present_work(struct work_struct *work)
*/
pci_lock_rescan_remove();
pci_scan_child_bus(hbus->pci_bus);
+ pci_assign_numa_node(hbus);
hv_pci_assign_slots(hbus);
pci_unlock_rescan_remove();
break;
@@ -2183,6 +2234,46 @@ static void hv_pci_devices_present(struct hv_pcibus_device *hbus,
kfree(dr);
}
+/**
+ * hv_pci_devices_present2() - Handles list of new children
+ * @hbus: Root PCI bus, as understood by this driver
+ * @relations2: Packet from host listing children
+ *
+ * This function is the v2 version of hv_pci_devices_present()
+ */
+static void hv_pci_devices_present2(struct hv_pcibus_device *hbus,
+ struct pci_bus_relations2 *relations)
+{
+ struct hv_dr_state *dr;
+ int i;
+
+ dr = kzalloc(offsetof(struct hv_dr_state, func) +
+ (sizeof(struct hv_pcidev_description) *
+ (relations->device_count)), GFP_NOWAIT);
+
+ if (!dr)
+ return;
+
+ dr->device_count = relations->device_count;
+ for (i = 0; i < dr->device_count; i++) {
+ dr->func[i].v_id = relations->func[i].v_id;
+ dr->func[i].d_id = relations->func[i].d_id;
+ dr->func[i].rev = relations->func[i].rev;
+ dr->func[i].prog_intf = relations->func[i].prog_intf;
+ dr->func[i].subclass = relations->func[i].subclass;
+ dr->func[i].base_class = relations->func[i].base_class;
+ dr->func[i].subsystem_id = relations->func[i].subsystem_id;
+ dr->func[i].win_slot = relations->func[i].win_slot;
+ dr->func[i].ser = relations->func[i].ser;
+ dr->func[i].flags = relations->func[i].flags;
+ dr->func[i].virtual_numa_node =
+ relations->func[i].virtual_numa_node;
+ }
+
+ if (hv_pci_start_relations_work(hbus, dr))
+ kfree(dr);
+}
+
/**
* hv_eject_device_work() - Asynchronously handles ejection
* @work: Work struct embedded in internal device struct
@@ -2288,6 +2379,7 @@ static void hv_pci_onchannelcallback(void *context)
struct pci_response *response;
struct pci_incoming_message *new_message;
struct pci_bus_relations *bus_rel;
+ struct pci_bus_relations2 *bus_rel2;
struct pci_dev_inval_block *inval;
struct pci_dev_incoming *dev_message;
struct hv_pci_dev *hpdev;
@@ -2355,6 +2447,21 @@ static void hv_pci_onchannelcallback(void *context)
hv_pci_devices_present(hbus, bus_rel);
break;
+ case PCI_BUS_RELATIONS2:
+
+ bus_rel2 = (struct pci_bus_relations2 *)buffer;
+ if (bytes_recvd <
+ offsetof(struct pci_bus_relations2, func) +
+ (sizeof(struct pci_function_description2) *
+ (bus_rel2->device_count))) {
+ dev_err(&hbus->hdev->device,
+ "bus relations v2 too small\n");
+ break;
+ }
+
+ hv_pci_devices_present2(hbus, bus_rel2);
+ break;
+
case PCI_EJECT:
dev_message = (struct pci_dev_incoming *)buffer;
--
2.17.1
^ permalink raw reply related
* [PATCH 1/2] PCI: hv: decouple the func definition in hv_dr_state from VSP message
From: longli @ 2019-11-23 1:57 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Sasha Levin,
Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas, linux-hyperv,
linux-pci, linux-kernel
Cc: Long Li
From: Long Li <longli@microsoft.com>
hv_dr_state is used to find present PCI devices on the bus. The structure
reuses struct pci_function_description from VSP message to describe a device.
To prepare support for pci_function_description v2, we need to decouple this
dependence in hv_dr_state so it can work with both v1 and v2 VSP messages.
There is no functionality change.
Signed-off-by: Long Li <longli@microsoft.com>
---
drivers/pci/controller/pci-hyperv.c | 100 +++++++++++++++++++---------
1 file changed, 69 insertions(+), 31 deletions(-)
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index f1f300218fab..f2e028cfa7cd 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -507,10 +507,24 @@ struct hv_dr_work {
struct hv_pcibus_device *bus;
};
+struct hv_pcidev_description {
+ u16 v_id; /* vendor ID */
+ u16 d_id; /* device ID */
+ u8 rev;
+ u8 prog_intf;
+ u8 subclass;
+ u8 base_class;
+ u32 subsystem_id;
+ union win_slot_encoding win_slot;
+ u32 ser; /* serial number */
+ u32 flags;
+ u16 virtual_numa_node;
+};
+
struct hv_dr_state {
struct list_head list_entry;
u32 device_count;
- struct pci_function_description func[0];
+ struct hv_pcidev_description func[0];
};
enum hv_pcichild_state {
@@ -527,7 +541,7 @@ struct hv_pci_dev {
refcount_t refs;
enum hv_pcichild_state state;
struct pci_slot *pci_slot;
- struct pci_function_description desc;
+ struct hv_pcidev_description desc;
bool reported_missing;
struct hv_pcibus_device *hbus;
struct work_struct wrk;
@@ -1862,7 +1876,7 @@ static void q_resource_requirements(void *context, struct pci_response *resp,
* Return: Pointer to the new tracking struct
*/
static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus,
- struct pci_function_description *desc)
+ struct hv_pcidev_description *desc)
{
struct hv_pci_dev *hpdev;
struct pci_child_message *res_req;
@@ -1973,7 +1987,7 @@ static void pci_devices_present_work(struct work_struct *work)
{
u32 child_no;
bool found;
- struct pci_function_description *new_desc;
+ struct hv_pcidev_description *new_desc;
struct hv_pci_dev *hpdev;
struct hv_pcibus_device *hbus;
struct list_head removed;
@@ -2090,43 +2104,26 @@ static void pci_devices_present_work(struct work_struct *work)
put_hvpcibus(hbus);
kfree(dr);
}
-
/**
- * hv_pci_devices_present() - Handles list of new children
+ * hv_pci_start_relations_work() - Queue work to start device discovery
* @hbus: Root PCI bus, as understood by this driver
- * @relations: Packet from host listing children
+ * @dr: The list of children returned from host
*
- * This function is invoked whenever a new list of devices for
- * this bus appears.
+ * Return: 0 on success, 1 on failure
*/
-static void hv_pci_devices_present(struct hv_pcibus_device *hbus,
- struct pci_bus_relations *relations)
+static int hv_pci_start_relations_work(struct hv_pcibus_device *hbus,
+ struct hv_dr_state *dr)
{
- struct hv_dr_state *dr;
struct hv_dr_work *dr_wrk;
- unsigned long flags;
bool pending_dr;
+ unsigned long flags;
dr_wrk = kzalloc(sizeof(*dr_wrk), GFP_NOWAIT);
if (!dr_wrk)
- return;
-
- dr = kzalloc(offsetof(struct hv_dr_state, func) +
- (sizeof(struct pci_function_description) *
- (relations->device_count)), GFP_NOWAIT);
- if (!dr) {
- kfree(dr_wrk);
- return;
- }
+ return 1;
INIT_WORK(&dr_wrk->wrk, pci_devices_present_work);
dr_wrk->bus = hbus;
- dr->device_count = relations->device_count;
- if (dr->device_count != 0) {
- memcpy(dr->func, relations->func,
- sizeof(struct pci_function_description) *
- dr->device_count);
- }
spin_lock_irqsave(&hbus->device_list_lock, flags);
/*
@@ -2144,6 +2141,46 @@ static void hv_pci_devices_present(struct hv_pcibus_device *hbus,
get_hvpcibus(hbus);
queue_work(hbus->wq, &dr_wrk->wrk);
}
+
+ return 0;
+}
+
+/**
+ * hv_pci_devices_present() - Handles list of new children
+ * @hbus: Root PCI bus, as understood by this driver
+ * @relations: Packet from host listing children
+ *
+ * This function is invoked whenever a new list of devices for
+ * this bus appears.
+ */
+static void hv_pci_devices_present(struct hv_pcibus_device *hbus,
+ struct pci_bus_relations *relations)
+{
+ struct hv_dr_state *dr;
+ int i;
+
+ dr = kzalloc(offsetof(struct hv_dr_state, func) +
+ (sizeof(struct hv_pcidev_description) *
+ (relations->device_count)), GFP_NOWAIT);
+
+ if (!dr)
+ return;
+
+ dr->device_count = relations->device_count;
+ for (i = 0; i < dr->device_count; i++) {
+ dr->func[i].v_id = relations->func[i].v_id;
+ dr->func[i].d_id = relations->func[i].d_id;
+ dr->func[i].rev = relations->func[i].rev;
+ dr->func[i].prog_intf = relations->func[i].prog_intf;
+ dr->func[i].subclass = relations->func[i].subclass;
+ dr->func[i].base_class = relations->func[i].base_class;
+ dr->func[i].subsystem_id = relations->func[i].subsystem_id;
+ dr->func[i].win_slot = relations->func[i].win_slot;
+ dr->func[i].ser = relations->func[i].ser;
+ }
+
+ if (hv_pci_start_relations_work(hbus, dr))
+ kfree(dr);
}
/**
@@ -3018,7 +3055,7 @@ static void hv_pci_bus_exit(struct hv_device *hdev)
struct pci_packet teardown_packet;
u8 buffer[sizeof(struct pci_message)];
} pkt;
- struct pci_bus_relations relations;
+ struct hv_dr_state *dr;
struct hv_pci_compl comp_pkt;
int ret;
@@ -3030,8 +3067,9 @@ static void hv_pci_bus_exit(struct hv_device *hdev)
return;
/* Delete any children which might still exist. */
- memset(&relations, 0, sizeof(relations));
- hv_pci_devices_present(hbus, &relations);
+ dr = kzalloc(sizeof(*dr), GFP_ATOMIC);
+ if (dr && hv_pci_start_relations_work(hbus, dr))
+ kfree(dr);
ret = hv_send_resources_released(hdev);
if (ret)
--
2.17.1
^ permalink raw reply related
* [PATCH v2] video: hyperv: hyperv_fb: Use physical memory for fb on HyperV Gen 1 VMs.
From: Wei Hu @ 2019-11-22 8:24 UTC (permalink / raw)
To: b.zolnierkie, kys, haiyangz, sthemmin, sashal, hch, m.szyprowski,
mchehab+samsung, sam, gregkh, alexandre.belloni, info, arnd,
dri-devel, linux-fbdev, linux-kernel, linux-hyperv, decui,
mikelley
Cc: Wei Hu
On Hyper-V, Generation 1 VMs can directly use VM's physical memory for
their framebuffers. This can improve the efficiency of framebuffer and
overall performence for VM. The physical memory assigned to framebuffer
must be contiguous. We use CMA allocator to get contiguouse physicial
memory when the framebuffer size is greater than 4MB. For size under
4MB, we use alloc_pages to achieve this.
To enable framebuffer memory allocation from CMA, supply a kernel
parameter to give enough space to CMA allocator at boot time. For
example:
cma=130m
This gives 130MB memory to CAM allocator that can be allocated to
framebuffer. If this fails, we fall back to the old way of using
mmio for framebuffer.
Signed-off-by: Wei Hu <weh@microsoft.com>
---
v2: Incorporated review comments form hch@lst.de, Michael Kelley and
Dexuan Cui
- Use dma_alloc_coherent to allocate large contiguous memory
- Use phys_addr_t for physical addresses
- Corrected a few spelling errors and minor cleanups
- Also tested on 32 bit Ubuntu guest
drivers/video/fbdev/Kconfig | 1 +
drivers/video/fbdev/hyperv_fb.c | 196 +++++++++++++++++++++++++-------
2 files changed, 158 insertions(+), 39 deletions(-)
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index aa9541bf964b..87b82de4598d 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -2215,6 +2215,7 @@ config FB_HYPERV
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
select FB_DEFERRED_IO
+ select DMA_CMA if HAVE_DMA_CONTIGUOUS
help
This framebuffer driver supports Microsoft Hyper-V Synthetic Video.
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index 3f60b7bc8589..8ba96764c749 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -31,6 +31,16 @@
* "set-vmvideo" command. For example
* set-vmvideo -vmname name -horizontalresolution:1920 \
* -verticalresolution:1200 -resolutiontype single
+ *
+ * Gen 1 VMs also support direct using VM's physical memory for framebuffer.
+ * It could improve the efficiency and performance for framebuffer and VM.
+ * This requires to allocate contiguous physical memory from Linux kernel's
+ * CMA memory allocator. To enable this, supply a kernel parameter to give
+ * enough memory space to CMA allocator for framebuffer. For example:
+ * cma=130m
+ * This gives 130MB memory to CMA allocator that can be allocated to
+ * framebuffer. For reference, 8K resolution (7680x4320) takes about
+ * 127MB memory.
*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -227,7 +237,6 @@ struct synthvid_msg {
} __packed;
-
/* FB driver definitions and structures */
#define HVFB_WIDTH 1152 /* default screen width */
#define HVFB_HEIGHT 864 /* default screen height */
@@ -256,12 +265,15 @@ struct hvfb_par {
/* If true, the VSC notifies the VSP on every framebuffer change */
bool synchronous_fb;
+ /* If true, need to copy from deferred IO mem to framebuffer mem */
+ bool need_docopy;
+
struct notifier_block hvfb_panic_nb;
/* Memory for deferred IO and frame buffer itself */
unsigned char *dio_vp;
unsigned char *mmio_vp;
- unsigned long mmio_pp;
+ phys_addr_t mmio_pp;
/* Dirty rectangle, protected by delayed_refresh_lock */
int x1, y1, x2, y2;
@@ -432,7 +444,7 @@ static void synthvid_deferred_io(struct fb_info *p,
maxy = max_t(int, maxy, y2);
/* Copy from dio space to mmio address */
- if (par->fb_ready)
+ if (par->fb_ready && par->need_docopy)
hvfb_docopy(par, start, PAGE_SIZE);
}
@@ -749,12 +761,12 @@ static void hvfb_update_work(struct work_struct *w)
return;
/* Copy the dirty rectangle to frame buffer memory */
- for (j = y1; j < y2; j++) {
- hvfb_docopy(par,
- j * info->fix.line_length +
- (x1 * screen_depth / 8),
- (x2 - x1) * screen_depth / 8);
- }
+ if (par->need_docopy)
+ for (j = y1; j < y2; j++)
+ hvfb_docopy(par,
+ j * info->fix.line_length +
+ (x1 * screen_depth / 8),
+ (x2 - x1) * screen_depth / 8);
/* Refresh */
if (par->fb_ready && par->update)
@@ -799,7 +811,8 @@ static int hvfb_on_panic(struct notifier_block *nb,
par = container_of(nb, struct hvfb_par, hvfb_panic_nb);
par->synchronous_fb = true;
info = par->info;
- hvfb_docopy(par, 0, dio_fb_size);
+ if (par->need_docopy)
+ hvfb_docopy(par, 0, dio_fb_size);
synthvid_update(info, 0, 0, INT_MAX, INT_MAX);
return NOTIFY_DONE;
@@ -938,6 +951,74 @@ static void hvfb_get_option(struct fb_info *info)
return;
}
+/*
+ * Allocate enough contiguous physical memory.
+ * Return physical address if succeeded or -1 if failed.
+ */
+static phys_addr_t hvfb_get_phymem(struct hv_device *hdev,
+ unsigned int request_size)
+{
+ struct page *page = NULL;
+ dma_addr_t dma_handle;
+ void *vmem;
+ unsigned int request_pages;
+ phys_addr_t paddr = 0;
+ unsigned int order = get_order(request_size);
+
+ if (request_size == 0)
+ return -1;
+
+ /* Try to call alloc_pages if the size is less than 2^MAX_ORDER */
+ if (order < MAX_ORDER) {
+ page = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
+ if (!page)
+ return -1;
+
+ paddr = (page_to_pfn(page) << PAGE_SHIFT);
+ request_pages = (1 << order);
+ goto get_phymem1;
+ }
+
+ /* Allocate from CMA */
+ if (hdev == NULL)
+ return -1;
+
+ hdev->device.coherent_dma_mask = DMA_BIT_MASK(64);
+
+ request_pages = (round_up(request_size, PAGE_SIZE) >> PAGE_SHIFT);
+
+ vmem = dma_alloc_coherent(&hdev->device,
+ request_pages * PAGE_SIZE,
+ &dma_handle,
+ GFP_KERNEL | __GFP_NOWARN);
+
+ if (!vmem)
+ return -1;
+
+ paddr = virt_to_phys(vmem);
+
+get_phymem1:
+ pr_info("Allocated %d pages starts at physical addr 0x%llx\n",
+ request_pages, paddr);
+
+ return paddr;
+}
+
+/* Release contiguous physical memory */
+static void hvfb_release_phymem(struct hv_device *hdev,
+ phys_addr_t paddr, unsigned int size)
+{
+ unsigned int order = get_order(size);
+
+ if (order < MAX_ORDER)
+ __free_pages(pfn_to_page(paddr >> PAGE_SHIFT), order);
+ else
+ dma_free_coherent(&hdev->device,
+ round_up(size, PAGE_SIZE),
+ phys_to_virt(paddr),
+ paddr);
+}
+
/* Get framebuffer memory from Hyper-V video pci space */
static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
@@ -947,8 +1028,57 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
void __iomem *fb_virt;
int gen2vm = efi_enabled(EFI_BOOT);
resource_size_t pot_start, pot_end;
+ phys_addr_t paddr;
int ret;
+ if (!gen2vm) {
+ pdev = pci_get_device(PCI_VENDOR_ID_MICROSOFT,
+ PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
+ if (!pdev) {
+ pr_err("Unable to find PCI Hyper-V video\n");
+ return -ENODEV;
+ }
+ }
+
+ info->apertures = alloc_apertures(1);
+ if (!info->apertures)
+ goto err1;
+
+ if (gen2vm) {
+ info->apertures->ranges[0].base = screen_info.lfb_base;
+ info->apertures->ranges[0].size = screen_info.lfb_size;
+ } else {
+ info->apertures->ranges[0].base = pci_resource_start(pdev, 0);
+ info->apertures->ranges[0].size = pci_resource_len(pdev, 0);
+ }
+
+ /*
+ * For Gen 1 VM, we can directly use the contiguous memory
+ * from VM. If we succeed, deferred IO happens directly
+ * on this allocated framebuffer memory, avoiding extra
+ * memory copy.
+ */
+ if (!gen2vm) {
+ paddr = hvfb_get_phymem(hdev, screen_fb_size);
+ if (paddr != (phys_addr_t) -1) {
+ par->mmio_pp = paddr;
+ par->mmio_vp = par->dio_vp = __va(paddr);
+
+ info->fix.smem_start = paddr;
+ info->fix.smem_len = screen_fb_size;
+ info->screen_base = par->mmio_vp;
+ info->screen_size = screen_fb_size;
+
+ par->need_docopy = false;
+ goto getmem1;
+ }
+ pr_info("Unable to allocate enough contiguous physical memory on Gen 1 VM. Use MMIO instead.\n");
+ }
+
+ /*
+ * Cannot use the contiguous physical memory.
+ * Allocate mmio space for framebuffer.
+ */
dio_fb_size =
screen_width * screen_height * screen_depth / 8;
@@ -956,13 +1086,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
pot_start = 0;
pot_end = -1;
} else {
- pdev = pci_get_device(PCI_VENDOR_ID_MICROSOFT,
- PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
- if (!pdev) {
- pr_err("Unable to find PCI Hyper-V video\n");
- return -ENODEV;
- }
-
if (!(pci_resource_flags(pdev, 0) & IORESOURCE_MEM) ||
pci_resource_len(pdev, 0) < screen_fb_size) {
pr_err("Resource not available or (0x%lx < 0x%lx)\n",
@@ -991,20 +1114,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
if (par->dio_vp == NULL)
goto err3;
- info->apertures = alloc_apertures(1);
- if (!info->apertures)
- goto err4;
-
- if (gen2vm) {
- info->apertures->ranges[0].base = screen_info.lfb_base;
- info->apertures->ranges[0].size = screen_info.lfb_size;
- remove_conflicting_framebuffers(info->apertures,
- KBUILD_MODNAME, false);
- } else {
- info->apertures->ranges[0].base = pci_resource_start(pdev, 0);
- info->apertures->ranges[0].size = pci_resource_len(pdev, 0);
- }
-
/* Physical address of FB device */
par->mmio_pp = par->mem->start;
/* Virtual address of FB device */
@@ -1015,13 +1124,15 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
info->screen_base = par->dio_vp;
info->screen_size = dio_fb_size;
+getmem1:
+ remove_conflicting_framebuffers(info->apertures,
+ KBUILD_MODNAME, false);
+
if (!gen2vm)
pci_dev_put(pdev);
return 0;
-err4:
- vfree(par->dio_vp);
err3:
iounmap(fb_virt);
err2:
@@ -1035,13 +1146,19 @@ static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
}
/* Release the framebuffer */
-static void hvfb_putmem(struct fb_info *info)
+static void hvfb_putmem(struct hv_device *hdev, struct fb_info *info)
{
struct hvfb_par *par = info->par;
- vfree(par->dio_vp);
- iounmap(info->screen_base);
- vmbus_free_mmio(par->mem->start, screen_fb_size);
+ if (par->need_docopy) {
+ vfree(par->dio_vp);
+ iounmap(info->screen_base);
+ vmbus_free_mmio(par->mem->start, screen_fb_size);
+ } else {
+ hvfb_release_phymem(hdev, info->fix.smem_start,
+ screen_fb_size);
+ }
+
par->mem = NULL;
}
@@ -1060,6 +1177,7 @@ static int hvfb_probe(struct hv_device *hdev,
par = info->par;
par->info = info;
par->fb_ready = false;
+ par->need_docopy = true;
init_completion(&par->wait);
INIT_DELAYED_WORK(&par->dwork, hvfb_update_work);
@@ -1145,7 +1263,7 @@ static int hvfb_probe(struct hv_device *hdev,
error:
fb_deferred_io_cleanup(info);
- hvfb_putmem(info);
+ hvfb_putmem(hdev, info);
error2:
vmbus_close(hdev->channel);
error1:
@@ -1175,7 +1293,7 @@ static int hvfb_remove(struct hv_device *hdev)
vmbus_close(hdev->channel);
hv_set_drvdata(hdev, NULL);
- hvfb_putmem(info);
+ hvfb_putmem(hdev, info);
framebuffer_release(info);
return 0;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net,v2 0/2] Fix send indirection table offset
From: David Miller @ 2019-11-22 8:04 UTC (permalink / raw)
To: haiyangz
Cc: sashal, linux-hyperv, netdev, kys, sthemmin, olaf, vkuznets,
linux-kernel
In-Reply-To: <1574372021-29439-1-git-send-email-haiyangz@microsoft.com>
From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Thu, 21 Nov 2019 13:33:39 -0800
> Fix send indirection table offset issues related to guest and
> host bugs.
Series applied, thank you.
^ permalink raw reply
* Re: [PATCH net,v2 1/2] hv_netvsc: Fix offset usage in netvsc_send_table()
From: Jakub Kicinski @ 2019-11-22 1:03 UTC (permalink / raw)
To: Haiyang Zhang
Cc: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, davem@davemloft.net,
linux-kernel@vger.kernel.org
In-Reply-To: <MN2PR21MB13750EBD53CFDFCBA36CF1D8CA490@MN2PR21MB1375.namprd21.prod.outlook.com>
On Fri, 22 Nov 2019 00:54:20 +0000, Haiyang Zhang wrote:
> > >
> > > - tab = (u32 *)((unsigned long)&nvmsg->msg.v5_msg.send_table +
> > > - nvmsg->msg.v5_msg.send_table.offset);
> > > + if (offset > msglen - count * sizeof(u32)) {
> >
> > Can't this underflow now? What if msglen is small?
> msglen came from the vmbus container message. We trust it to be big
> enough for the data region.
Ok, it looked like it was read from some descriptor which could
potentially be controlled by "the other side" but I trust your
judgement :)
Both patches LGTM, then.
^ permalink raw reply
* RE: [PATCH net,v2 1/2] hv_netvsc: Fix offset usage in netvsc_send_table()
From: Haiyang Zhang @ 2019-11-22 0:54 UTC (permalink / raw)
To: Jakub Kicinski
Cc: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, davem@davemloft.net,
linux-kernel@vger.kernel.org
In-Reply-To: <20191121150445.47fc3358@cakuba.netronome.com>
> -----Original Message-----
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Sent: Thursday, November 21, 2019 6:05 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org; netdev@vger.kernel.org;
> KY Srinivasan <kys@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> <vkuznets@redhat.com>; davem@davemloft.net; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net,v2 1/2] hv_netvsc: Fix offset usage in
> netvsc_send_table()
>
> On Thu, 21 Nov 2019 13:33:40 -0800, Haiyang Zhang wrote:
> > To reach the data region, the existing code adds offset in struct
> > nvsp_5_send_indirect_table on the beginning of this struct. But the
> > offset should be based on the beginning of its container,
> > struct nvsp_message. This bug causes the first table entry missing,
> > and adds an extra zero from the zero pad after the data region.
> > This can put extra burden on the channel 0.
> >
> > So, correct the offset usage. Also add a boundary check to ensure
> > not reading beyond data region.
>
> Please provide a change log at the end of the commit message when
> posting new version in the future.
Sure. Will do that in the future.
>
> > Fixes: 5b54dac856cb ("hyperv: Add support for virtual Receive Side Scaling
> (vRSS)")
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
>
> >
> > - tab = (u32 *)((unsigned long)&nvmsg->msg.v5_msg.send_table +
> > - nvmsg->msg.v5_msg.send_table.offset);
> > + if (offset > msglen - count * sizeof(u32)) {
>
> Can't this underflow now? What if msglen is small?
msglen came from the vmbus container message. We trust it to be big
enough for the data region.
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCH v2] HID: hyperv: Add the support of hibernation
From: Sasha Levin @ 2019-11-22 0:26 UTC (permalink / raw)
To: Jiri Kosina
Cc: Dexuan Cui, kys, haiyangz, sthemmin, benjamin.tissoires,
linux-hyperv, linux-input, linux-kernel, mikelley,
Alexander.Levin
In-Reply-To: <nycvar.YFH.7.76.1911211533430.1799@cbobk.fhfr.pm>
On Thu, Nov 21, 2019 at 03:34:10PM +0100, Jiri Kosina wrote:
>On Tue, 19 Nov 2019, Dexuan Cui wrote:
>
>> During the suspend process and resume process, if there is any mouse
>> event, there is a small chance the suspend and the resume process can be
>> aborted because of mousevsc_on_receive() -> pm_wakeup_hard_event().
>>
>> This behavior can be avoided by disabling the Hyper-V mouse device as
>> a wakeup source:
>>
>> echo disabled > /sys/bus/vmbus/drivers/hid_hyperv/XXX/power/wakeup
>> (XXX is the device's GUID).
>>
>> Signed-off-by: Dexuan Cui <decui@microsoft.com>
>> Acked-by: Jiri Kosina <jkosina@suse.cz>
>
>My Ack still holds for v2. Sasha, this is going to be merged through your
>tree, right?
Yup, queued up for hyperv-next, thanks!
--
Thanks,
Sasha
^ permalink raw reply
* Re: [PATCH net,v2 1/2] hv_netvsc: Fix offset usage in netvsc_send_table()
From: Jakub Kicinski @ 2019-11-21 23:04 UTC (permalink / raw)
To: Haiyang Zhang
Cc: sashal, linux-hyperv, netdev, kys, sthemmin, olaf, vkuznets,
davem, linux-kernel
In-Reply-To: <1574372021-29439-2-git-send-email-haiyangz@microsoft.com>
On Thu, 21 Nov 2019 13:33:40 -0800, Haiyang Zhang wrote:
> To reach the data region, the existing code adds offset in struct
> nvsp_5_send_indirect_table on the beginning of this struct. But the
> offset should be based on the beginning of its container,
> struct nvsp_message. This bug causes the first table entry missing,
> and adds an extra zero from the zero pad after the data region.
> This can put extra burden on the channel 0.
>
> So, correct the offset usage. Also add a boundary check to ensure
> not reading beyond data region.
Please provide a change log at the end of the commit message when
posting new version in the future.
> Fixes: 5b54dac856cb ("hyperv: Add support for virtual Receive Side Scaling (vRSS)")
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
>
> - tab = (u32 *)((unsigned long)&nvmsg->msg.v5_msg.send_table +
> - nvmsg->msg.v5_msg.send_table.offset);
> + if (offset > msglen - count * sizeof(u32)) {
Can't this underflow now? What if msglen is small?
> + netdev_err(ndev, "Received send-table offset too big:%u\n",
> + offset);
> + return;
> + }
> +
> + tab = (void *)nvmsg + offset;
>
> for (i = 0; i < count; i++)
> net_device_ctx->tx_table[i] = tab[i];
^ permalink raw reply
* [PATCH net,v2 2/2] hv_netvsc: Fix send_table offset in case of a host bug
From: Haiyang Zhang @ 2019-11-21 21:33 UTC (permalink / raw)
To: sashal, linux-hyperv, netdev
Cc: haiyangz, kys, sthemmin, olaf, vkuznets, davem, linux-kernel
In-Reply-To: <1574372021-29439-1-git-send-email-haiyangz@microsoft.com>
If negotiated NVSP version <= NVSP_PROTOCOL_VERSION_6, the offset may
be wrong (too small) due to a host bug. This can cause missing the
end of the send indirection table, and add multiple zero entries from
leading zeros before the data region. This bug adds extra burden on
channel 0.
So fix the offset by computing it from the data structure sizes. This
will ensure netvsc driver runs normally on unfixed hosts, and future
fixed hosts.
Fixes: 5b54dac856cb ("hyperv: Add support for virtual Receive Side Scaling (vRSS)")
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/net/hyperv/netvsc.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 9b0532e..eab83e7 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -1178,6 +1178,7 @@ static int netvsc_receive(struct net_device *ndev,
}
static void netvsc_send_table(struct net_device *ndev,
+ struct netvsc_device *nvscdev,
const struct nvsp_message *nvmsg,
u32 msglen)
{
@@ -1193,6 +1194,16 @@ static void netvsc_send_table(struct net_device *ndev,
return;
}
+ /* If negotiated version <= NVSP_PROTOCOL_VERSION_6, the offset may be
+ * wrong due to a host bug. So fix the offset here.
+ */
+ if (nvscdev->nvsp_version <= NVSP_PROTOCOL_VERSION_6 &&
+ msglen >= sizeof(struct nvsp_message_header) +
+ sizeof(union nvsp_6_message_uber) + count * sizeof(u32))
+ offset = sizeof(struct nvsp_message_header) +
+ sizeof(union nvsp_6_message_uber);
+
+ /* Boundary check for all versions */
if (offset > msglen - count * sizeof(u32)) {
netdev_err(ndev, "Received send-table offset too big:%u\n",
offset);
@@ -1218,12 +1229,13 @@ static void netvsc_send_vf(struct net_device *ndev,
}
static void netvsc_receive_inband(struct net_device *ndev,
+ struct netvsc_device *nvscdev,
const struct nvsp_message *nvmsg,
u32 msglen)
{
switch (nvmsg->hdr.msg_type) {
case NVSP_MSG5_TYPE_SEND_INDIRECTION_TABLE:
- netvsc_send_table(ndev, nvmsg, msglen);
+ netvsc_send_table(ndev, nvscdev, nvmsg, msglen);
break;
case NVSP_MSG4_TYPE_SEND_VF_ASSOCIATION:
@@ -1257,7 +1269,7 @@ static int netvsc_process_raw_pkt(struct hv_device *device,
break;
case VM_PKT_DATA_INBAND:
- netvsc_receive_inband(ndev, nvmsg, msglen);
+ netvsc_receive_inband(ndev, net_device, nvmsg, msglen);
break;
default:
--
1.8.3.1
^ permalink raw reply related
* [PATCH net,v2 1/2] hv_netvsc: Fix offset usage in netvsc_send_table()
From: Haiyang Zhang @ 2019-11-21 21:33 UTC (permalink / raw)
To: sashal, linux-hyperv, netdev
Cc: haiyangz, kys, sthemmin, olaf, vkuznets, davem, linux-kernel
In-Reply-To: <1574372021-29439-1-git-send-email-haiyangz@microsoft.com>
To reach the data region, the existing code adds offset in struct
nvsp_5_send_indirect_table on the beginning of this struct. But the
offset should be based on the beginning of its container,
struct nvsp_message. This bug causes the first table entry missing,
and adds an extra zero from the zero pad after the data region.
This can put extra burden on the channel 0.
So, correct the offset usage. Also add a boundary check to ensure
not reading beyond data region.
Fixes: 5b54dac856cb ("hyperv: Add support for virtual Receive Side Scaling (vRSS)")
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/net/hyperv/hyperv_net.h | 3 ++-
drivers/net/hyperv/netvsc.c | 26 ++++++++++++++++++--------
2 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 670ef68..fb547f3 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -609,7 +609,8 @@ struct nvsp_5_send_indirect_table {
/* The number of entries in the send indirection table */
u32 count;
- /* The offset of the send indirection table from top of this struct.
+ /* The offset of the send indirection table from the beginning of
+ * struct nvsp_message.
* The send indirection table tells which channel to put the send
* traffic on. Each entry is a channel number.
*/
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index d22a36f..9b0532e 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -1178,20 +1178,28 @@ static int netvsc_receive(struct net_device *ndev,
}
static void netvsc_send_table(struct net_device *ndev,
- const struct nvsp_message *nvmsg)
+ const struct nvsp_message *nvmsg,
+ u32 msglen)
{
struct net_device_context *net_device_ctx = netdev_priv(ndev);
- u32 count, *tab;
+ u32 count, offset, *tab;
int i;
count = nvmsg->msg.v5_msg.send_table.count;
+ offset = nvmsg->msg.v5_msg.send_table.offset;
+
if (count != VRSS_SEND_TAB_SIZE) {
netdev_err(ndev, "Received wrong send-table size:%u\n", count);
return;
}
- tab = (u32 *)((unsigned long)&nvmsg->msg.v5_msg.send_table +
- nvmsg->msg.v5_msg.send_table.offset);
+ if (offset > msglen - count * sizeof(u32)) {
+ netdev_err(ndev, "Received send-table offset too big:%u\n",
+ offset);
+ return;
+ }
+
+ tab = (void *)nvmsg + offset;
for (i = 0; i < count; i++)
net_device_ctx->tx_table[i] = tab[i];
@@ -1209,12 +1217,13 @@ static void netvsc_send_vf(struct net_device *ndev,
net_device_ctx->vf_alloc ? "added" : "removed");
}
-static void netvsc_receive_inband(struct net_device *ndev,
- const struct nvsp_message *nvmsg)
+static void netvsc_receive_inband(struct net_device *ndev,
+ const struct nvsp_message *nvmsg,
+ u32 msglen)
{
switch (nvmsg->hdr.msg_type) {
case NVSP_MSG5_TYPE_SEND_INDIRECTION_TABLE:
- netvsc_send_table(ndev, nvmsg);
+ netvsc_send_table(ndev, nvmsg, msglen);
break;
case NVSP_MSG4_TYPE_SEND_VF_ASSOCIATION:
@@ -1232,6 +1241,7 @@ static int netvsc_process_raw_pkt(struct hv_device *device,
{
struct vmbus_channel *channel = nvchan->channel;
const struct nvsp_message *nvmsg = hv_pkt_data(desc);
+ u32 msglen = hv_pkt_datalen(desc);
trace_nvsp_recv(ndev, channel, nvmsg);
@@ -1247,7 +1257,7 @@ static int netvsc_process_raw_pkt(struct hv_device *device,
break;
case VM_PKT_DATA_INBAND:
- netvsc_receive_inband(ndev, nvmsg);
+ netvsc_receive_inband(ndev, nvmsg, msglen);
break;
default:
--
1.8.3.1
^ permalink raw reply related
* [PATCH net,v2 0/2] Fix send indirection table offset
From: Haiyang Zhang @ 2019-11-21 21:33 UTC (permalink / raw)
To: sashal, linux-hyperv, netdev
Cc: haiyangz, kys, sthemmin, olaf, vkuznets, davem, linux-kernel
Fix send indirection table offset issues related to guest and
host bugs.
Haiyang Zhang (2):
hv_netvsc: Fix offset usage in netvsc_send_table()
hv_netvsc: Fix send_table offset in case of a host bug
drivers/net/hyperv/hyperv_net.h | 3 ++-
drivers/net/hyperv/netvsc.c | 38 ++++++++++++++++++++++++++++++--------
2 files changed, 32 insertions(+), 9 deletions(-)
--
1.8.3.1
^ permalink raw reply
* Re: [PATCH v2] HID: hyperv: Add the support of hibernation
From: Jiri Kosina @ 2019-11-21 14:34 UTC (permalink / raw)
To: Dexuan Cui
Cc: kys, haiyangz, sthemmin, sashal, benjamin.tissoires, linux-hyperv,
linux-input, linux-kernel, mikelley, Alexander.Levin
In-Reply-To: <1574234096-48767-1-git-send-email-decui@microsoft.com>
On Tue, 19 Nov 2019, Dexuan Cui wrote:
> During the suspend process and resume process, if there is any mouse
> event, there is a small chance the suspend and the resume process can be
> aborted because of mousevsc_on_receive() -> pm_wakeup_hard_event().
>
> This behavior can be avoided by disabling the Hyper-V mouse device as
> a wakeup source:
>
> echo disabled > /sys/bus/vmbus/drivers/hid_hyperv/XXX/power/wakeup
> (XXX is the device's GUID).
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> Acked-by: Jiri Kosina <jkosina@suse.cz>
My Ack still holds for v2. Sasha, this is going to be merged through your
tree, right?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 2/4] PCI: hv: Add the support of hibernation
From: Lorenzo Pieralisi @ 2019-11-21 11:44 UTC (permalink / raw)
To: Dexuan Cui
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, bhelgaas@google.com,
linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley, Sasha Levin
In-Reply-To: <PU1P153MB0169D0A99D5687FBDB02536BBF4E0@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM>
On Thu, Nov 21, 2019 at 12:50:17AM +0000, Dexuan Cui wrote:
> > From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Sent: Wednesday, November 20, 2019 9:20 AM
> >
> > On Tue, Nov 19, 2019 at 11:16:56PM -0800, Dexuan Cui wrote:
> > > Implement the suspend/resume callbacks.
> > >
> > > We must make sure there is no pending work items before we call
> > > vmbus_close().
> >
> > Where ? Why ? Imagine a developer reading this log to try to understand
> > why you made this change, do you really think this commit log is
> > informative in its current form ?
> >
> > I am not asking a book but this is a significant feature please make
> > an effort to explain it (I can update the log for you but please
> > write one and I shall do it).
> >
> > Lorenzo
>
> Sorry for being sloppy on this patch's changelog! Can you please use the
> below? I can also post v3 with the new changelog if that's better.
As you wish but more importantly get hyper-V maintainers to ACK these
changes since time is running out for v5.5.
Lorenzo
> PCI: hv: Add the support of hibernation
>
> hv_pci_suspend() runs in a process context as a callback in dpm_suspend().
> When it starts to run, the channel callback hv_pci_onchannelcallback(),
> which runs in a tasklet context, can be still running concurrently and
> scheduling new work items onto hbus->wq in hv_pci_devices_present() and
> hv_pci_eject_device(), and the work item handlers can access the vmbus
> channel, which can be being closed by hv_pci_suspend(), e.g. the work item
> handler pci_devices_present_work() -> new_pcichild_device() writes to
> the vmbus channel.
>
> To eliminate the race, hv_pci_suspend() disables the channel callback
> tasklet, sets hbus->state to hv_pcibus_removing, and re-enables the tasklet.
>
> This way, when hv_pci_suspend() proceeds, it knows that no new work item
> can be scheduled, and then it flushes hbus->wq and safely closes the vmbus
> channel.
>
> Thanks,
> -- Dexuan
^ permalink raw reply
* RE: [PATCH] video: hyperv_fb: Fix hibernation for the deferred IO feature
From: Wei Hu @ 2019-11-21 6:04 UTC (permalink / raw)
To: Dexuan Cui, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, b.zolnierkie@samsung.com,
linux-hyperv@vger.kernel.org, dri-devel@lists.freedesktop.org,
linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Michael Kelley, Sasha Levin
In-Reply-To: <PU1P153MB01699A8B75901F896F1E4503BB4E0@PU1P153MB0169.APCP153.PROD.OUTLOOK.COM>
> -----Original Message-----
> From: Wei Hu
> Sent: 2019年11月21日 10:47
> To: Dexuan Cui <decui@microsoft.com>; KY Srinivasan <kys@microsoft.com>;
> Haiyang Zhang <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; sashal@kernel.org; b.zolnierkie@samsung.com;
> linux-hyperv@vger.kernel.org; dri-devel@lists.freedesktop.org; linux-
> fbdev@vger.kernel.org; linux-kernel@vger.kernel.org; Michael Kelley
> <mikelley@microsoft.com>; Sasha Levin <Alexander.Levin@microsoft.com>
> Subject: RE: [PATCH] video: hyperv_fb: Fix hibernation for the deferred IO
> feature
>
> > -----Original Message-----
> > From: Dexuan Cui <decui@microsoft.com>
> > Sent: Wednesday, November 20, 2019 3:14 PM
> > To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>;
> > sashal@kernel.org; b.zolnierkie@samsung.com; linux-hyperv@vger.kernel.org;
> > dri-devel@lists.freedesktop.org; linux-fbdev@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Michael Kelley <mikelley@microsoft.com>; Sasha
> Levin
> > <Alexander.Levin@microsoft.com>
> > Cc: Wei Hu <weh@microsoft.com>; Dexuan Cui <decui@microsoft.com>
> > Subject: [PATCH] video: hyperv_fb: Fix hibernation for the deferred IO feature
> >
> > fb_deferred_io_work() can access the vmbus ringbuffer by calling
> > fbdefio->deferred_io() -> synthvid_deferred_io() -> synthvid_update().
> >
> > Because the vmbus ringbuffer is inaccessible between hvfb_suspend() and
> > hvfb_resume(), we must cancel info->deferred_work before calling
> > vmbus_close() and then reschedule it after we reopen the channel in
> > hvfb_resume().
> >
> > Fixes: a4ddb11d297e ("video: hyperv: hyperv_fb: Support deferred IO for
> > Hyper-V frame buffer driver")
> > Fixes: 824946a8b6fb ("video: hyperv_fb: Add the support of hibernation")
> > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > ---
> >
> > This patch fixes the 2 aforementioned patches on Sasha Levin's Hyper-V tree's
> > hyperv-next branch:
> >
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern
> > el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fhyperv%2Flinux.git%2Flog
> > %2F%3Fh%3Dhyperv-
> >
> next&data=02%7C01%7Cweh%40microsoft.com%7C451143ff78f04401d9
> > 6f08d76d893a84%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637
> >
> 098308493217121&sdata=P2fo%2F1TJUMIj5FtJCOp2QwDrghhVfPSCEJ4f1
> > vkOXvI%3D&reserved=0
> >
> > The 2 aforementioned patches have not appeared in the mainline yet, so
> please
> > pick up this patch onto he same hyperv-next branch.
> >
> > drivers/video/fbdev/hyperv_fb.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> > index 4cd27e5172a1..08bc0dfb5ce7 100644
> > --- a/drivers/video/fbdev/hyperv_fb.c
> > +++ b/drivers/video/fbdev/hyperv_fb.c
> > @@ -1194,6 +1194,7 @@ static int hvfb_suspend(struct hv_device *hdev)
> > fb_set_suspend(info, 1);
> >
> > cancel_delayed_work_sync(&par->dwork);
> > + cancel_delayed_work_sync(&info->deferred_work);
> >
> > par->update_saved = par->update;
> > par->update = false;
> > @@ -1227,6 +1228,7 @@ static int hvfb_resume(struct hv_device *hdev)
> > par->fb_ready = true;
> > par->update = par->update_saved;
> >
> > + schedule_delayed_work(&info->deferred_work, info->fbdefio->delay);
> > schedule_delayed_work(&par->dwork, HVFB_UPDATE_DELAY);
> >
> > /* 0 means do resume */
> > --
> > 2.19.1
>
> Signed-off-by: Wei Hu <weh@microsoft.com>
Sorry, please disregard the Signed-off-by line I added above. It was my mistake.
should be:
Reviewed-by: Wei Hu <weh@microsoft.com>
^ permalink raw reply
* RE: [PATCH] video: hyperv_fb: Fix hibernation for the deferred IO feature
From: Wei Hu @ 2019-11-21 2:47 UTC (permalink / raw)
To: Dexuan Cui, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, b.zolnierkie@samsung.com,
linux-hyperv@vger.kernel.org, dri-devel@lists.freedesktop.org,
linux-fbdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Michael Kelley, Sasha Levin
In-Reply-To: <1574234028-48574-1-git-send-email-decui@microsoft.com>
> -----Original Message-----
> From: Dexuan Cui <decui@microsoft.com>
> Sent: Wednesday, November 20, 2019 3:14 PM
> To: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> sashal@kernel.org; b.zolnierkie@samsung.com; linux-hyperv@vger.kernel.org;
> dri-devel@lists.freedesktop.org; linux-fbdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Michael Kelley <mikelley@microsoft.com>; Sasha Levin
> <Alexander.Levin@microsoft.com>
> Cc: Wei Hu <weh@microsoft.com>; Dexuan Cui <decui@microsoft.com>
> Subject: [PATCH] video: hyperv_fb: Fix hibernation for the deferred IO feature
>
> fb_deferred_io_work() can access the vmbus ringbuffer by calling
> fbdefio->deferred_io() -> synthvid_deferred_io() -> synthvid_update().
>
> Because the vmbus ringbuffer is inaccessible between hvfb_suspend() and
> hvfb_resume(), we must cancel info->deferred_work before calling
> vmbus_close() and then reschedule it after we reopen the channel in
> hvfb_resume().
>
> Fixes: a4ddb11d297e ("video: hyperv: hyperv_fb: Support deferred IO for
> Hyper-V frame buffer driver")
> Fixes: 824946a8b6fb ("video: hyperv_fb: Add the support of hibernation")
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>
> This patch fixes the 2 aforementioned patches on Sasha Levin's Hyper-V tree's
> hyperv-next branch:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kern
> el.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fhyperv%2Flinux.git%2Flog
> %2F%3Fh%3Dhyperv-
> next&data=02%7C01%7Cweh%40microsoft.com%7C451143ff78f04401d9
> 6f08d76d893a84%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637
> 098308493217121&sdata=P2fo%2F1TJUMIj5FtJCOp2QwDrghhVfPSCEJ4f1
> vkOXvI%3D&reserved=0
>
> The 2 aforementioned patches have not appeared in the mainline yet, so please
> pick up this patch onto he same hyperv-next branch.
>
> drivers/video/fbdev/hyperv_fb.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index 4cd27e5172a1..08bc0dfb5ce7 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -1194,6 +1194,7 @@ static int hvfb_suspend(struct hv_device *hdev)
> fb_set_suspend(info, 1);
>
> cancel_delayed_work_sync(&par->dwork);
> + cancel_delayed_work_sync(&info->deferred_work);
>
> par->update_saved = par->update;
> par->update = false;
> @@ -1227,6 +1228,7 @@ static int hvfb_resume(struct hv_device *hdev)
> par->fb_ready = true;
> par->update = par->update_saved;
>
> + schedule_delayed_work(&info->deferred_work, info->fbdefio->delay);
> schedule_delayed_work(&par->dwork, HVFB_UPDATE_DELAY);
>
> /* 0 means do resume */
> --
> 2.19.1
Signed-off-by: Wei Hu <weh@microsoft.com>
^ permalink raw reply
* RE: [PATCH v2 4/4] PCI: hv: kmemleak: Track the page allocations for struct hv_pcibus_device
From: Dexuan Cui @ 2019-11-21 0:53 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, bhelgaas@google.com,
linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley, Sasha Levin
In-Reply-To: <20191120171212.GD3279@e121166-lin.cambridge.arm.com>
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Sent: Wednesday, November 20, 2019 9:12 AM
>
> On Tue, Nov 19, 2019 at 11:16:58PM -0800, Dexuan Cui wrote:
> > The page allocated for struct hv_pcibus_device contains pointers to other
> > slab allocations in new_pcichild_device(). Since kmemleak does not track
> > and scan page allocations, the slab objects will be reported as leaks
> > (false positives). Use the kmemleak APIs to allow tracking of such pages.
> >
> > Reported-by: Lili Deng <v-lide@microsoft.com>
> > Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > ---
> >
> > This is actually v1. I use "v2" in the Subject just to be consistent with
> > the other patches in the patchset.
>
> That's a mistake, you should have posted patches separately. I need
Got it. I'll remember to do it separately in future.
> hyper-V ACKs on this series to get it through.
>
> Thanks,
> Lorenzo
Sure. I'm asking the Hyper-V maintainers to review the patchset.
Thanks,
-- Dexuan
^ permalink raw reply
* RE: [PATCH v2 2/4] PCI: hv: Add the support of hibernation
From: Dexuan Cui @ 2019-11-21 0:50 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, bhelgaas@google.com,
linux-hyperv@vger.kernel.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley, Sasha Levin
In-Reply-To: <20191120172026.GE3279@e121166-lin.cambridge.arm.com>
> From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Sent: Wednesday, November 20, 2019 9:20 AM
>
> On Tue, Nov 19, 2019 at 11:16:56PM -0800, Dexuan Cui wrote:
> > Implement the suspend/resume callbacks.
> >
> > We must make sure there is no pending work items before we call
> > vmbus_close().
>
> Where ? Why ? Imagine a developer reading this log to try to understand
> why you made this change, do you really think this commit log is
> informative in its current form ?
>
> I am not asking a book but this is a significant feature please make
> an effort to explain it (I can update the log for you but please
> write one and I shall do it).
>
> Lorenzo
Sorry for being sloppy on this patch's changelog! Can you please use the
below? I can also post v3 with the new changelog if that's better.
PCI: hv: Add the support of hibernation
hv_pci_suspend() runs in a process context as a callback in dpm_suspend().
When it starts to run, the channel callback hv_pci_onchannelcallback(),
which runs in a tasklet context, can be still running concurrently and
scheduling new work items onto hbus->wq in hv_pci_devices_present() and
hv_pci_eject_device(), and the work item handlers can access the vmbus
channel, which can be being closed by hv_pci_suspend(), e.g. the work item
handler pci_devices_present_work() -> new_pcichild_device() writes to
the vmbus channel.
To eliminate the race, hv_pci_suspend() disables the channel callback
tasklet, sets hbus->state to hv_pcibus_removing, and re-enables the tasklet.
This way, when hv_pci_suspend() proceeds, it knows that no new work item
can be scheduled, and then it flushes hbus->wq and safely closes the vmbus
channel.
Thanks,
-- Dexuan
^ permalink raw reply
* Re: [PATCH] VFIO/VMBUS: Add VFIO VMBUS driver support
From: Stephen Hemminger @ 2019-11-20 23:18 UTC (permalink / raw)
To: Alex Williamson
Cc: lantianyu1986, cohuck, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, sashal, mchehab+samsung, davem, gregkh, robh,
Jonathan.Cameron, paulmck, Michael Kelley, Tianyu Lan,
linux-kernel, kvm, linux-hyperv, vkuznets
In-Reply-To: <20191120133147.1d627348@x1.home>
On Wed, 20 Nov 2019 13:31:47 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:
> On Wed, 20 Nov 2019 11:46:11 -0800
> Stephen Hemminger <stephen@networkplumber.org> wrote:
>
> > On Wed, 20 Nov 2019 12:07:15 -0700
> > Alex Williamson <alex.williamson@redhat.com> wrote:
> >
> > > On Wed, 20 Nov 2019 10:35:03 -0800
> > > Stephen Hemminger <stephen@networkplumber.org> wrote:
> > >
> > > > On Tue, 19 Nov 2019 15:56:20 -0800
> > > > "Alex Williamson" <alex.williamson@redhat.com> wrote:
> > > >
> > > > > On Mon, 11 Nov 2019 16:45:07 +0800
> > > > > lantianyu1986@gmail.com wrote:
> > > > >
> > > > > > From: Tianyu Lan <Tianyu.Lan@microsoft.com>
> > > > > >
> > > > > > This patch is to add VFIO VMBUS driver support in order to expose
> > > > > > VMBUS devices to user space drivers(Reference Hyper-V UIO driver).
> > > > > > DPDK now has netvsc PMD driver support and it may get VMBUS resources
> > > > > > via VFIO interface with new driver support.
> > > > > >
> > > > > > So far, Hyper-V doesn't provide virtual IOMMU support and so this
> > > > > > driver needs to be used with VFIO noiommu mode.
> > > > >
> > > > > Let's be clear here, vfio no-iommu mode taints the kernel and was a
> > > > > compromise that we can re-use vfio-pci in its entirety, so it had a
> > > > > high code reuse value for minimal code and maintenance investment. It
> > > > > was certainly not intended to provoke new drivers that rely on this mode
> > > > > of operation. In fact, no-iommu should be discouraged as it provides
> > > > > absolutely no isolation. I'd therefore ask, why should this be in the
> > > > > kernel versus any other unsupportable out of tree driver? It appears
> > > > > almost entirely self contained. Thanks,
> > > > >
> > > > > Alex
> > > >
> > > > The current VMBUS access from userspace is from uio_hv_generic
> > > > there is (and will not be) any out of tree driver for this.
> > >
> > > I'm talking about the driver proposed here. It can only be used in a
> > > mode that taints the kernel that its running on, so why would we sign
> > > up to support 400 lines of code that has no safe way to use it?
> > >
> > > > The new driver from Tianyu is to make VMBUS behave like PCI.
> > > > This simplifies the code for DPDK and other usermode device drivers
> > > > because it can use the same API's for VMBus as is done for PCI.
> > >
> > > But this doesn't re-use the vfio-pci API at all, it explicitly defines
> > > a new vfio-vmbus API over the vfio interfaces. So a user mode driver
> > > might be able to reuse some vfio support, but I don't see how this has
> > > anything to do with PCI.
> > >
> > > > Unfortunately, since Hyper-V does not support virtual IOMMU yet,
> > > > the only usage modle is with no-iommu taint.
> > >
> > > Which is what makes it unsupportable and prompts the question why it
> > > should be included in the mainline kernel as it introduces a
> > > maintenance burden and normalizes a usage model that's unsafe. Thanks,
> >
> > Many existing userspace drivers are unsafe:
> > - out of tree DPDK igb_uio is unsafe.
> Why is it out of tree?
Agree, it really shouldn't be. The original developers hoped that
VFIO and VFIO-noiommu would replace it. But since DPDK has to run
on ancient distro's and other non VFIO hardware it still lives.
Because it is not suitable for merging for many reasons.
Mostly because it allows MSI and other don't want that.
>
>
> > - VFIO with noiommu is unsafe.
>
> Which taints the kernel and requires raw I/O user privs.
>
> > - hv_uio_generic is unsafe.
>
> Gosh, it's pretty coy about this, no kernel tainting, no user
> capability tests, no scary dmesg or Kconfig warnings. Do users know
> it's unsafe?
It should taint in same way as VFIO with noiommu.
Yes it is documented as unsafe (but not in kernel source).
It really has same unsafeness as uio_pci_generic, and there is not warnings
around that.
>
> > This new driver is not any better or worse. This sounds like a complete
> > repeat of the discussion that occurred before introducing VFIO noiommu mode.
> >
> > Shouldn't vmbus vfio taint the kernel in the same way as vfio noiommu does?
>
> Yes, the no-iommu interaction happens at the vfio-core level. I can't
> speak for any of the uio interfaces you mention, but I know that
> uio_pci_generic is explicitly intended for non-DMA use cases and in
> fact the efforts to enable MSI/X support in that driver and the
> objections raised for breaking that usage model by the maintainer, is
> what triggered no-iommu support for vfio. IIRC, the rationale was
> largely for code reuse both at the kernel and userspace driver level,
> while imposing a minimal burden in vfio-core for this dummy iommu
> driver. vfio explicitly does not provide a DMA mapping solution for
> no-iommu use cases because I'm not willing to maintain any more lines
> of code to support this usage model. The tainting imposed by this model
> and incomplete API was intended to be a big warning to discourage its
> use and as features like vIOMMU become more prevalent and bare metal
> platforms without physical IOMMUs hopefully become less prevalent,
> maybe no-iommu could be phased out or removed.
Doing vIOMMU at scale with a non-Linux host, take a a long time.
Tainting doesn't make it happen any sooner. It just makes users
live harder. Sorry blaming the user and giving a bad experience doesn't help anyone.
> You might consider this a re-hashing of those previous discussions, but
> to me it seems like taking advantage of and promoting an interface that
> should have plenty of warning signs that this is not a safe way to use
> the device from userspace. Without some way to take advantage of the
> code in a safe way, this just seems to be normalizing an unsupportable
> usage model. Thanks,
The use case for all this stuff has been dedicated infrastructure.
It would be good if security was more baked in but it isn't.
Most users cover it over by either being dedicated applicances
or use LSM to protect UIO.
^ permalink raw reply
* Re: [PATCH] VFIO/VMBUS: Add VFIO VMBUS driver support
From: Alex Williamson @ 2019-11-20 20:31 UTC (permalink / raw)
To: Stephen Hemminger
Cc: lantianyu1986, cohuck, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, sashal, mchehab+samsung, davem, gregkh, robh,
Jonathan.Cameron, paulmck, Michael Kelley, Tianyu Lan,
linux-kernel, kvm, linux-hyperv, vkuznets
In-Reply-To: <20191120114611.4721a7e9@hermes.lan>
On Wed, 20 Nov 2019 11:46:11 -0800
Stephen Hemminger <stephen@networkplumber.org> wrote:
> On Wed, 20 Nov 2019 12:07:15 -0700
> Alex Williamson <alex.williamson@redhat.com> wrote:
>
> > On Wed, 20 Nov 2019 10:35:03 -0800
> > Stephen Hemminger <stephen@networkplumber.org> wrote:
> >
> > > On Tue, 19 Nov 2019 15:56:20 -0800
> > > "Alex Williamson" <alex.williamson@redhat.com> wrote:
> > >
> > > > On Mon, 11 Nov 2019 16:45:07 +0800
> > > > lantianyu1986@gmail.com wrote:
> > > >
> > > > > From: Tianyu Lan <Tianyu.Lan@microsoft.com>
> > > > >
> > > > > This patch is to add VFIO VMBUS driver support in order to expose
> > > > > VMBUS devices to user space drivers(Reference Hyper-V UIO driver).
> > > > > DPDK now has netvsc PMD driver support and it may get VMBUS resources
> > > > > via VFIO interface with new driver support.
> > > > >
> > > > > So far, Hyper-V doesn't provide virtual IOMMU support and so this
> > > > > driver needs to be used with VFIO noiommu mode.
> > > >
> > > > Let's be clear here, vfio no-iommu mode taints the kernel and was a
> > > > compromise that we can re-use vfio-pci in its entirety, so it had a
> > > > high code reuse value for minimal code and maintenance investment. It
> > > > was certainly not intended to provoke new drivers that rely on this mode
> > > > of operation. In fact, no-iommu should be discouraged as it provides
> > > > absolutely no isolation. I'd therefore ask, why should this be in the
> > > > kernel versus any other unsupportable out of tree driver? It appears
> > > > almost entirely self contained. Thanks,
> > > >
> > > > Alex
> > >
> > > The current VMBUS access from userspace is from uio_hv_generic
> > > there is (and will not be) any out of tree driver for this.
> >
> > I'm talking about the driver proposed here. It can only be used in a
> > mode that taints the kernel that its running on, so why would we sign
> > up to support 400 lines of code that has no safe way to use it?
> >
> > > The new driver from Tianyu is to make VMBUS behave like PCI.
> > > This simplifies the code for DPDK and other usermode device drivers
> > > because it can use the same API's for VMBus as is done for PCI.
> >
> > But this doesn't re-use the vfio-pci API at all, it explicitly defines
> > a new vfio-vmbus API over the vfio interfaces. So a user mode driver
> > might be able to reuse some vfio support, but I don't see how this has
> > anything to do with PCI.
> >
> > > Unfortunately, since Hyper-V does not support virtual IOMMU yet,
> > > the only usage modle is with no-iommu taint.
> >
> > Which is what makes it unsupportable and prompts the question why it
> > should be included in the mainline kernel as it introduces a
> > maintenance burden and normalizes a usage model that's unsafe. Thanks,
>
> Many existing userspace drivers are unsafe:
> - out of tree DPDK igb_uio is unsafe.
Why is it out of tree?
> - VFIO with noiommu is unsafe.
Which taints the kernel and requires raw I/O user privs.
> - hv_uio_generic is unsafe.
Gosh, it's pretty coy about this, no kernel tainting, no user
capability tests, no scary dmesg or Kconfig warnings. Do users know
it's unsafe?
> This new driver is not any better or worse. This sounds like a complete
> repeat of the discussion that occurred before introducing VFIO noiommu mode.
>
> Shouldn't vmbus vfio taint the kernel in the same way as vfio noiommu does?
Yes, the no-iommu interaction happens at the vfio-core level. I can't
speak for any of the uio interfaces you mention, but I know that
uio_pci_generic is explicitly intended for non-DMA use cases and in
fact the efforts to enable MSI/X support in that driver and the
objections raised for breaking that usage model by the maintainer, is
what triggered no-iommu support for vfio. IIRC, the rationale was
largely for code reuse both at the kernel and userspace driver level,
while imposing a minimal burden in vfio-core for this dummy iommu
driver. vfio explicitly does not provide a DMA mapping solution for
no-iommu use cases because I'm not willing to maintain any more lines
of code to support this usage model. The tainting imposed by this model
and incomplete API was intended to be a big warning to discourage its
use and as features like vIOMMU become more prevalent and bare metal
platforms without physical IOMMUs hopefully become less prevalent,
maybe no-iommu could be phased out or removed.
You might consider this a re-hashing of those previous discussions, but
to me it seems like taking advantage of and promoting an interface that
should have plenty of warning signs that this is not a safe way to use
the device from userspace. Without some way to take advantage of the
code in a safe way, this just seems to be normalizing an unsupportable
usage model. Thanks,
Alex
^ permalink raw reply
* Re: [PATCH] VFIO/VMBUS: Add VFIO VMBUS driver support
From: Stephen Hemminger @ 2019-11-20 19:46 UTC (permalink / raw)
To: Alex Williamson
Cc: lantianyu1986, cohuck, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, sashal, mchehab+samsung, davem, gregkh, robh,
Jonathan.Cameron, paulmck, Michael Kelley, Tianyu Lan,
linux-kernel, kvm, linux-hyperv, vkuznets
In-Reply-To: <20191120120715.0cecf5ea@x1.home>
On Wed, 20 Nov 2019 12:07:15 -0700
Alex Williamson <alex.williamson@redhat.com> wrote:
> On Wed, 20 Nov 2019 10:35:03 -0800
> Stephen Hemminger <stephen@networkplumber.org> wrote:
>
> > On Tue, 19 Nov 2019 15:56:20 -0800
> > "Alex Williamson" <alex.williamson@redhat.com> wrote:
> >
> > > On Mon, 11 Nov 2019 16:45:07 +0800
> > > lantianyu1986@gmail.com wrote:
> > >
> > > > From: Tianyu Lan <Tianyu.Lan@microsoft.com>
> > > >
> > > > This patch is to add VFIO VMBUS driver support in order to expose
> > > > VMBUS devices to user space drivers(Reference Hyper-V UIO driver).
> > > > DPDK now has netvsc PMD driver support and it may get VMBUS resources
> > > > via VFIO interface with new driver support.
> > > >
> > > > So far, Hyper-V doesn't provide virtual IOMMU support and so this
> > > > driver needs to be used with VFIO noiommu mode.
> > >
> > > Let's be clear here, vfio no-iommu mode taints the kernel and was a
> > > compromise that we can re-use vfio-pci in its entirety, so it had a
> > > high code reuse value for minimal code and maintenance investment. It
> > > was certainly not intended to provoke new drivers that rely on this mode
> > > of operation. In fact, no-iommu should be discouraged as it provides
> > > absolutely no isolation. I'd therefore ask, why should this be in the
> > > kernel versus any other unsupportable out of tree driver? It appears
> > > almost entirely self contained. Thanks,
> > >
> > > Alex
> >
> > The current VMBUS access from userspace is from uio_hv_generic
> > there is (and will not be) any out of tree driver for this.
>
> I'm talking about the driver proposed here. It can only be used in a
> mode that taints the kernel that its running on, so why would we sign
> up to support 400 lines of code that has no safe way to use it?
>
> > The new driver from Tianyu is to make VMBUS behave like PCI.
> > This simplifies the code for DPDK and other usermode device drivers
> > because it can use the same API's for VMBus as is done for PCI.
>
> But this doesn't re-use the vfio-pci API at all, it explicitly defines
> a new vfio-vmbus API over the vfio interfaces. So a user mode driver
> might be able to reuse some vfio support, but I don't see how this has
> anything to do with PCI.
>
> > Unfortunately, since Hyper-V does not support virtual IOMMU yet,
> > the only usage modle is with no-iommu taint.
>
> Which is what makes it unsupportable and prompts the question why it
> should be included in the mainline kernel as it introduces a
> maintenance burden and normalizes a usage model that's unsafe. Thanks,
Many existing userspace drivers are unsafe:
- out of tree DPDK igb_uio is unsafe.
- VFIO with noiommu is unsafe.
- hv_uio_generic is unsafe.
This new driver is not any better or worse. This sounds like a complete
repeat of the discussion that occurred before introducing VFIO noiommu mode.
Shouldn't vmbus vfio taint the kernel in the same way as vfio noiommu does?
^ permalink raw reply
* Re: [PATCH] VFIO/VMBUS: Add VFIO VMBUS driver support
From: Alex Williamson @ 2019-11-20 19:07 UTC (permalink / raw)
To: Stephen Hemminger
Cc: lantianyu1986, cohuck, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, sashal, mchehab+samsung, davem, gregkh, robh,
Jonathan.Cameron, paulmck, Michael Kelley, Tianyu Lan,
linux-kernel, kvm, linux-hyperv, vkuznets
In-Reply-To: <20191120103503.5f7bd7c4@hermes.lan>
On Wed, 20 Nov 2019 10:35:03 -0800
Stephen Hemminger <stephen@networkplumber.org> wrote:
> On Tue, 19 Nov 2019 15:56:20 -0800
> "Alex Williamson" <alex.williamson@redhat.com> wrote:
>
> > On Mon, 11 Nov 2019 16:45:07 +0800
> > lantianyu1986@gmail.com wrote:
> >
> > > From: Tianyu Lan <Tianyu.Lan@microsoft.com>
> > >
> > > This patch is to add VFIO VMBUS driver support in order to expose
> > > VMBUS devices to user space drivers(Reference Hyper-V UIO driver).
> > > DPDK now has netvsc PMD driver support and it may get VMBUS resources
> > > via VFIO interface with new driver support.
> > >
> > > So far, Hyper-V doesn't provide virtual IOMMU support and so this
> > > driver needs to be used with VFIO noiommu mode.
> >
> > Let's be clear here, vfio no-iommu mode taints the kernel and was a
> > compromise that we can re-use vfio-pci in its entirety, so it had a
> > high code reuse value for minimal code and maintenance investment. It
> > was certainly not intended to provoke new drivers that rely on this mode
> > of operation. In fact, no-iommu should be discouraged as it provides
> > absolutely no isolation. I'd therefore ask, why should this be in the
> > kernel versus any other unsupportable out of tree driver? It appears
> > almost entirely self contained. Thanks,
> >
> > Alex
>
> The current VMBUS access from userspace is from uio_hv_generic
> there is (and will not be) any out of tree driver for this.
I'm talking about the driver proposed here. It can only be used in a
mode that taints the kernel that its running on, so why would we sign
up to support 400 lines of code that has no safe way to use it?
> The new driver from Tianyu is to make VMBUS behave like PCI.
> This simplifies the code for DPDK and other usermode device drivers
> because it can use the same API's for VMBus as is done for PCI.
But this doesn't re-use the vfio-pci API at all, it explicitly defines
a new vfio-vmbus API over the vfio interfaces. So a user mode driver
might be able to reuse some vfio support, but I don't see how this has
anything to do with PCI.
> Unfortunately, since Hyper-V does not support virtual IOMMU yet,
> the only usage modle is with no-iommu taint.
Which is what makes it unsupportable and prompts the question why it
should be included in the mainline kernel as it introduces a
maintenance burden and normalizes a usage model that's unsafe. Thanks,
Alex
^ permalink raw reply
* Re: [PATCH] VFIO/VMBUS: Add VFIO VMBUS driver support
From: Stephen Hemminger @ 2019-11-20 18:35 UTC (permalink / raw)
To: Alex Williamson
Cc: lantianyu1986, cohuck, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, sashal, mchehab+samsung, davem, gregkh, robh,
Jonathan.Cameron, paulmck, Michael Kelley, Tianyu Lan,
linux-kernel, kvm, linux-hyperv, vkuznets
In-Reply-To: <20191119165620.0f42e5ba@x1.home>
On Tue, 19 Nov 2019 15:56:20 -0800
"Alex Williamson" <alex.williamson@redhat.com> wrote:
> On Mon, 11 Nov 2019 16:45:07 +0800
> lantianyu1986@gmail.com wrote:
>
> > From: Tianyu Lan <Tianyu.Lan@microsoft.com>
> >
> > This patch is to add VFIO VMBUS driver support in order to expose
> > VMBUS devices to user space drivers(Reference Hyper-V UIO driver).
> > DPDK now has netvsc PMD driver support and it may get VMBUS resources
> > via VFIO interface with new driver support.
> >
> > So far, Hyper-V doesn't provide virtual IOMMU support and so this
> > driver needs to be used with VFIO noiommu mode.
>
> Let's be clear here, vfio no-iommu mode taints the kernel and was a
> compromise that we can re-use vfio-pci in its entirety, so it had a
> high code reuse value for minimal code and maintenance investment. It
> was certainly not intended to provoke new drivers that rely on this mode
> of operation. In fact, no-iommu should be discouraged as it provides
> absolutely no isolation. I'd therefore ask, why should this be in the
> kernel versus any other unsupportable out of tree driver? It appears
> almost entirely self contained. Thanks,
>
> Alex
The current VMBUS access from userspace is from uio_hv_generic
there is (and will not be) any out of tree driver for this.
The new driver from Tianyu is to make VMBUS behave like PCI.
This simplifies the code for DPDK and other usermode device drivers
because it can use the same API's for VMBus as is done for PCI.
Unfortunately, since Hyper-V does not support virtual IOMMU yet,
the only usage modle is with no-iommu taint.
^ permalink raw reply
* Re: [PATCH v2 2/4] PCI: hv: Add the support of hibernation
From: Lorenzo Pieralisi @ 2019-11-20 17:20 UTC (permalink / raw)
To: Dexuan Cui
Cc: kys, haiyangz, sthemmin, sashal, bhelgaas, linux-hyperv,
linux-pci, linux-kernel, mikelley, Alexander.Levin
In-Reply-To: <1574234218-49195-3-git-send-email-decui@microsoft.com>
On Tue, Nov 19, 2019 at 11:16:56PM -0800, Dexuan Cui wrote:
> Implement the suspend/resume callbacks.
>
> We must make sure there is no pending work items before we call
> vmbus_close().
Where ? Why ? Imagine a developer reading this log to try to understand
why you made this change, do you really think this commit log is
informative in its current form ?
I am not asking a book but this is a significant feature please make
an effort to explain it (I can update the log for you but please
write one and I shall do it).
Lorenzo
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>
> Changes in v2: this patch is a simple merge of 2 previous smaller patches,
> accordign to the suggestion of Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>.
>
> drivers/pci/controller/pci-hyperv.c | 107 +++++++++++++++++++++++++++-
> 1 file changed, 105 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index 65f18f840ce9..e71eb6e0bfdd 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -455,6 +455,7 @@ enum hv_pcibus_state {
> hv_pcibus_init = 0,
> hv_pcibus_probed,
> hv_pcibus_installed,
> + hv_pcibus_removing,
> hv_pcibus_removed,
> hv_pcibus_maximum
> };
> @@ -1681,6 +1682,23 @@ static void prepopulate_bars(struct hv_pcibus_device *hbus)
>
> spin_lock_irqsave(&hbus->device_list_lock, flags);
>
> + /*
> + * Clear the memory enable bit, in case it's already set. This occurs
> + * in the suspend path of hibernation, where the device is suspended,
> + * resumed and suspended again: see hibernation_snapshot() and
> + * hibernation_platform_enter().
> + *
> + * If the memory enable bit is already set, Hyper-V sliently ignores
> + * the below BAR updates, and the related PCI device driver can not
> + * work, because reading from the device register(s) always returns
> + * 0xFFFFFFFF.
> + */
> + list_for_each_entry(hpdev, &hbus->children, list_entry) {
> + _hv_pcifront_read_config(hpdev, PCI_COMMAND, 2, &command);
> + command &= ~PCI_COMMAND_MEMORY;
> + _hv_pcifront_write_config(hpdev, PCI_COMMAND, 2, command);
> + }
> +
> /* Pick addresses for the BARs. */
> do {
> list_for_each_entry(hpdev, &hbus->children, list_entry) {
> @@ -2107,6 +2125,12 @@ static void hv_pci_devices_present(struct hv_pcibus_device *hbus,
> unsigned long flags;
> bool pending_dr;
>
> + if (hbus->state == hv_pcibus_removing) {
> + dev_info(&hbus->hdev->device,
> + "PCI VMBus BUS_RELATIONS: ignored\n");
> + return;
> + }
> +
> dr_wrk = kzalloc(sizeof(*dr_wrk), GFP_NOWAIT);
> if (!dr_wrk)
> return;
> @@ -2223,11 +2247,19 @@ static void hv_eject_device_work(struct work_struct *work)
> */
> static void hv_pci_eject_device(struct hv_pci_dev *hpdev)
> {
> + struct hv_pcibus_device *hbus = hpdev->hbus;
> + struct hv_device *hdev = hbus->hdev;
> +
> + if (hbus->state == hv_pcibus_removing) {
> + dev_info(&hdev->device, "PCI VMBus EJECT: ignored\n");
> + return;
> + }
> +
> hpdev->state = hv_pcichild_ejecting;
> get_pcichild(hpdev);
> INIT_WORK(&hpdev->wrk, hv_eject_device_work);
> - get_hvpcibus(hpdev->hbus);
> - queue_work(hpdev->hbus->wq, &hpdev->wrk);
> + get_hvpcibus(hbus);
> + queue_work(hbus->wq, &hpdev->wrk);
> }
>
> /**
> @@ -3107,6 +3139,75 @@ static int hv_pci_remove(struct hv_device *hdev)
> return ret;
> }
>
> +static int hv_pci_suspend(struct hv_device *hdev)
> +{
> + struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
> + enum hv_pcibus_state old_state;
> + int ret;
> +
> + tasklet_disable(&hdev->channel->callback_event);
> +
> + /* Change the hbus state to prevent new work items. */
> + old_state = hbus->state;
> + if (hbus->state == hv_pcibus_installed)
> + hbus->state = hv_pcibus_removing;
> +
> + tasklet_enable(&hdev->channel->callback_event);
> +
> + if (old_state != hv_pcibus_installed)
> + return -EINVAL;
> +
> + flush_workqueue(hbus->wq);
> +
> + ret = hv_pci_bus_exit(hdev, true);
> + if (ret)
> + return ret;
> +
> + vmbus_close(hdev->channel);
> +
> + return 0;
> +}
> +
> +static int hv_pci_resume(struct hv_device *hdev)
> +{
> + struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
> + enum pci_protocol_version_t version[1];
> + int ret;
> +
> + hbus->state = hv_pcibus_init;
> +
> + ret = vmbus_open(hdev->channel, pci_ring_size, pci_ring_size, NULL, 0,
> + hv_pci_onchannelcallback, hbus);
> + if (ret)
> + return ret;
> +
> + /* Only use the version that was in use before hibernation. */
> + version[0] = pci_protocol_version;
> + ret = hv_pci_protocol_negotiation(hdev, version, 1);
> + if (ret)
> + goto out;
> +
> + ret = hv_pci_query_relations(hdev);
> + if (ret)
> + goto out;
> +
> + ret = hv_pci_enter_d0(hdev);
> + if (ret)
> + goto out;
> +
> + ret = hv_send_resources_allocated(hdev);
> + if (ret)
> + goto out;
> +
> + prepopulate_bars(hbus);
> +
> + hbus->state = hv_pcibus_installed;
> + return 0;
> +out:
> + vmbus_close(hdev->channel);
> + return ret;
> +}
> +
> static const struct hv_vmbus_device_id hv_pci_id_table[] = {
> /* PCI Pass-through Class ID */
> /* 44C4F61D-4444-4400-9D52-802E27EDE19F */
> @@ -3121,6 +3222,8 @@ static struct hv_driver hv_pci_drv = {
> .id_table = hv_pci_id_table,
> .probe = hv_pci_probe,
> .remove = hv_pci_remove,
> + .suspend = hv_pci_suspend,
> + .resume = hv_pci_resume,
> };
>
> static void __exit exit_hv_pci_drv(void)
> --
> 2.19.1
>
^ permalink raw reply
* Re: [PATCH v2 4/4] PCI: hv: kmemleak: Track the page allocations for struct hv_pcibus_device
From: Lorenzo Pieralisi @ 2019-11-20 17:12 UTC (permalink / raw)
To: Dexuan Cui
Cc: kys, haiyangz, sthemmin, sashal, bhelgaas, linux-hyperv,
linux-pci, linux-kernel, mikelley, Alexander.Levin
In-Reply-To: <1574234218-49195-5-git-send-email-decui@microsoft.com>
On Tue, Nov 19, 2019 at 11:16:58PM -0800, Dexuan Cui wrote:
> The page allocated for struct hv_pcibus_device contains pointers to other
> slab allocations in new_pcichild_device(). Since kmemleak does not track
> and scan page allocations, the slab objects will be reported as leaks
> (false positives). Use the kmemleak APIs to allow tracking of such pages.
>
> Reported-by: Lili Deng <v-lide@microsoft.com>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>
> This is actually v1. I use "v2" in the Subject just to be consistent with
> the other patches in the patchset.
That's a mistake, you should have posted patches separately. I need
hyper-V ACKs on this series to get it through.
Thanks,
Lorenzo
> Without the patch, we can see the below warning in dmesg, if kmemleak is
> enabled:
>
> kmemleak: 1 new suspected memory leaks (see /sys/kernel/debug/kmemleak)
>
> and "cat /sys/kernel/debug/kmemleak" shows:
>
> unreferenced object 0xffff9217d1f2bec0 (size 192):
> comm "kworker/u256:7", pid 100821, jiffies 4501481057 (age 61409.997s)
> hex dump (first 32 bytes):
> a8 60 b1 63 17 92 ff ff a8 60 b1 63 17 92 ff ff .`.c.....`.c....
> 02 00 00 00 00 00 00 00 80 92 cd 61 17 92 ff ff ...........a....
> backtrace:
> [<0000000006f7ae93>] pci_devices_present_work+0x326/0x5e0 [pci_hyperv]
> [<00000000278eb88a>] process_one_work+0x15f/0x360
> [<00000000c59501e6>] worker_thread+0x49/0x3c0
> [<000000000a0a7a94>] kthread+0xf8/0x130
> [<000000007075c2e7>] ret_from_fork+0x35/0x40
>
> drivers/pci/controller/pci-hyperv.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> index d7e05d436b5e..cc73f49c9e03 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -46,6 +46,7 @@
> #include <asm/irqdomain.h>
> #include <asm/apic.h>
> #include <linux/irq.h>
> +#include <linux/kmemleak.h>
> #include <linux/msi.h>
> #include <linux/hyperv.h>
> #include <linux/refcount.h>
> @@ -2907,6 +2908,16 @@ static int hv_pci_probe(struct hv_device *hdev,
> hbus = (struct hv_pcibus_device *)get_zeroed_page(GFP_KERNEL);
> if (!hbus)
> return -ENOMEM;
> +
> + /*
> + * kmemleak doesn't track page allocations as they are not commonly
> + * used for kernel data structures, but here hbus->children indeed
> + * contains pointers to hv_pci_dev structs, which are dynamically
> + * created in new_pcichild_device(). Allow kmemleak to scan the page
> + * so we don't get a false warning of memory leak.
> + */
> + kmemleak_alloc(hbus, PAGE_SIZE, 1, GFP_KERNEL);
> +
> hbus->state = hv_pcibus_init;
>
> /*
> @@ -3133,6 +3144,8 @@ static int hv_pci_remove(struct hv_device *hdev)
>
> hv_put_dom_num(hbus->sysdata.domain);
>
> + /* Remove kmemleak object previously allocated in hv_pci_probe() */
> + kmemleak_free(hbus);
> free_page((unsigned long)hbus);
> return ret;
> }
> --
> 2.19.1
>
^ permalink raw reply
* Re: [PATCH v2 0/2] Implement hv_is_hibernation_supported() and enhance hv_balloon for hibernation
From: Sasha Levin @ 2019-11-20 14:09 UTC (permalink / raw)
To: Dexuan Cui
Cc: kys, haiyangz, sthemmin, linux-hyperv, linux-kernel, mikelley,
david, arnd, bp, daniel.lezcano, hpa, mingo, tglx, x86,
Alexander.Levin, vkuznets, linux-arch
In-Reply-To: <1574234165-49066-1-git-send-email-decui@microsoft.com>
On Tue, Nov 19, 2019 at 11:16:03PM -0800, Dexuan Cui wrote:
>v2 is actually the same as v1. This is just a resend.
>
>I suggest both the patches should go through the Hyper-V tree:
>https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/log/?h=hyperv-next
>because the first patch is needed by the second one.
>
>This first patch doesn't conflict with any patch in the tip.git tree.
Queued for hyperv-next, thanks!
--
Thanks,
Sasha
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox