* [PATCH v2 1/2] x86/hyperv: Implement hv_is_hibernation_supported()
From: Dexuan Cui @ 2019-11-20 7:16 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, sashal, linux-hyperv, linux-kernel,
mikelley, david, arnd, bp, daniel.lezcano, hpa, mingo, tglx, x86,
Alexander.Levin, vkuznets
Cc: linux-arch, Dexuan Cui
In-Reply-To: <1574234165-49066-1-git-send-email-decui@microsoft.com>
The API will be used by the hv_balloon and hv_vmbus drivers.
Balloon up/down and hot-add of memory must not be active if the user
wants the Linux VM to support hibernation, because they are incompatible
with hibernation according to Hyper-V team, e.g. upon suspend the
balloon VSP doesn't save any info about the ballooned-out pages (if any);
so, after Linux resumes, Linux balloon VSC expects that the VSP will
return the pages if Linux is under memory pressure, but the VSP will
never do that, since the VSP thinks it never stole the pages from the VM.
So, if the user wants Linux VM to support hibernation, Linux must forbid
balloon up/down and hot-add, and the only functionality of the balloon VSC
driver is reporting the VM's memory pressure to the host.
Ideally, when Linux detects that the user wants it to support hibernation,
the balloon VSC should tell the VSP that it does not support ballooning
and hot-add. However, the current version of the VSP requires the VSC
should support these capabilities, otherwise the capability negotiation
fails and the VSC can not load at all, so with the later changes to the
VSC driver, Linux VM still reports to the VSP that the VSC supports these
capabilities, but the VSC ignores the VSP's requests of balloon up/down
and hot add, and reports an error to the VSP, when applicable. BTW, in
the future the balloon VSP driver will allow the VSC to not support the
capabilities of balloon up/down and hot add.
The ACPI S4 state is not a must for hibernation to work, because Linux is
able to hibernate as long as the system can shut down. However in practice
we decide to artificially use the presence of the virtual ACPI S4 state as
an indicator of the user's intent of using hibernation, because Linux VM
must find a way to know if the user wants to use the hibernation feature
or not.
By default, Hyper-V does not enable the virtual ACPI S4 state; on recent
Hyper-V hosts (e.g. RS5, 19H1), the administrator is able to enable the
state for a VM by WMI commands.
Once all the vmbus and VSC patches for the hibernation feature are
accepted, an extra patch will be submitted to forbid hibernation if the
virtual ACPI S4 state is absent, i.e. hv_is_hibernation_supported() is
false.
Signed-off-by: Dexuan Cui <decui@microsoft.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
---
v2 is actually the same as v1. This is just a resend.
I suggest this patch should go through the Hyper-V tree:
https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/log/?h=hyperv-next
because it's needed by
[PATCH v2 2/2] hv_balloon: Add the support of hibernation
and some upcoming patches.
This patch doesn't conflict with any patch in the tip.git tree.
arch/x86/hyperv/hv_init.c | 7 +++++++
include/asm-generic/mshyperv.h | 2 ++
2 files changed, 9 insertions(+)
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index c170653da589..24a62d33067c 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -7,6 +7,7 @@
* Author : K. Y. Srinivasan <kys@microsoft.com>
*/
+#include <linux/acpi.h>
#include <linux/efi.h>
#include <linux/types.h>
#include <asm/apic.h>
@@ -493,3 +494,9 @@ bool hv_is_hyperv_initialized(void)
return hypercall_msr.enable;
}
EXPORT_SYMBOL_GPL(hv_is_hyperv_initialized);
+
+bool hv_is_hibernation_supported(void)
+{
+ return acpi_sleep_state_supported(ACPI_STATE_S4);
+}
+EXPORT_SYMBOL_GPL(hv_is_hibernation_supported);
diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
index 18d8e2d8210f..b3f1082cc435 100644
--- a/include/asm-generic/mshyperv.h
+++ b/include/asm-generic/mshyperv.h
@@ -166,10 +166,12 @@ static inline int cpumask_to_vpset(struct hv_vpset *vpset,
void hyperv_report_panic(struct pt_regs *regs, long err);
void hyperv_report_panic_msg(phys_addr_t pa, size_t size);
bool hv_is_hyperv_initialized(void);
+bool hv_is_hibernation_supported(void);
void hyperv_cleanup(void);
void hv_setup_sched_clock(void *sched_clock);
#else /* CONFIG_HYPERV */
static inline bool hv_is_hyperv_initialized(void) { return false; }
+static inline bool hv_is_hibernation_supported(void) { return false; }
static inline void hyperv_cleanup(void) {}
#endif /* CONFIG_HYPERV */
--
2.19.1
^ permalink raw reply related
* [PATCH v2 3/4] PCI: hv: Change pci_protocol_version to per-hbus
From: Dexuan Cui @ 2019-11-20 7:16 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, sashal, lorenzo.pieralisi, bhelgaas,
linux-hyperv, linux-pci, linux-kernel, mikelley, Alexander.Levin
Cc: Dexuan Cui
In-Reply-To: <1574234218-49195-1-git-send-email-decui@microsoft.com>
A VM can have multiple Hyper-V hbus. It's incorrect to set the global
variable 'pci_protocol_version' when *every* hbus is initialized in
hv_pci_protocol_negotiation(). This is not an issue in practice since
every hbus should have the same value of hbus->protocol_version, but
we should make the variable per-hbus, so in case we have busses
with different protocol_versions, the driver can still work correctly.
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
changes in v2: Improved the changelog by making it clear that this patch
fixes a potential bug if we have busses with different protocol_versions.
drivers/pci/controller/pci-hyperv.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index e71eb6e0bfdd..d7e05d436b5e 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -76,11 +76,6 @@ static enum pci_protocol_version_t pci_protocol_versions[] = {
PCI_PROTOCOL_VERSION_1_1,
};
-/*
- * Protocol version negotiated by hv_pci_protocol_negotiation().
- */
-static enum pci_protocol_version_t pci_protocol_version;
-
#define PCI_CONFIG_MMIO_LENGTH 0x2000
#define CFG_PAGE_OFFSET 0x1000
#define CFG_PAGE_SIZE (PCI_CONFIG_MMIO_LENGTH - CFG_PAGE_OFFSET)
@@ -462,6 +457,8 @@ enum hv_pcibus_state {
struct hv_pcibus_device {
struct pci_sysdata sysdata;
+ /* Protocol version negotiated with the host */
+ enum pci_protocol_version_t protocol_version;
enum hv_pcibus_state state;
refcount_t remove_lock;
struct hv_device *hdev;
@@ -1225,7 +1222,7 @@ static void hv_irq_unmask(struct irq_data *data)
* negative effect (yet?).
*/
- if (pci_protocol_version >= PCI_PROTOCOL_VERSION_1_2) {
+ if (hbus->protocol_version >= PCI_PROTOCOL_VERSION_1_2) {
/*
* PCI_PROTOCOL_VERSION_1_2 supports the VP_SET version of the
* HVCALL_RETARGET_INTERRUPT hypercall, which also coincides
@@ -1395,7 +1392,7 @@ static void hv_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
ctxt.pci_pkt.completion_func = hv_pci_compose_compl;
ctxt.pci_pkt.compl_ctxt = ∁
- switch (pci_protocol_version) {
+ switch (hbus->protocol_version) {
case PCI_PROTOCOL_VERSION_1_1:
size = hv_compose_msi_req_v1(&ctxt.int_pkts.v1,
dest,
@@ -2415,6 +2412,7 @@ static int hv_pci_protocol_negotiation(struct hv_device *hdev,
enum pci_protocol_version_t version[],
int num_version)
{
+ struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
struct pci_version_request *version_req;
struct hv_pci_compl comp_pkt;
struct pci_packet *pkt;
@@ -2454,10 +2452,10 @@ static int hv_pci_protocol_negotiation(struct hv_device *hdev,
}
if (comp_pkt.completion_status >= 0) {
- pci_protocol_version = version[i];
+ hbus->protocol_version = version[i];
dev_info(&hdev->device,
"PCI VMBus probing: Using version %#x\n",
- pci_protocol_version);
+ hbus->protocol_version);
goto exit;
}
@@ -2741,7 +2739,7 @@ static int hv_send_resources_allocated(struct hv_device *hdev)
u32 wslot;
int ret;
- size_res = (pci_protocol_version < PCI_PROTOCOL_VERSION_1_2)
+ size_res = (hbus->protocol_version < PCI_PROTOCOL_VERSION_1_2)
? sizeof(*res_assigned) : sizeof(*res_assigned2);
pkt = kmalloc(sizeof(*pkt) + size_res, GFP_KERNEL);
@@ -2760,7 +2758,7 @@ static int hv_send_resources_allocated(struct hv_device *hdev)
pkt->completion_func = hv_pci_generic_compl;
pkt->compl_ctxt = &comp_pkt;
- if (pci_protocol_version < PCI_PROTOCOL_VERSION_1_2) {
+ if (hbus->protocol_version < PCI_PROTOCOL_VERSION_1_2) {
res_assigned =
(struct pci_resources_assigned *)&pkt->message;
res_assigned->message_type.type =
@@ -3182,7 +3180,7 @@ static int hv_pci_resume(struct hv_device *hdev)
return ret;
/* Only use the version that was in use before hibernation. */
- version[0] = pci_protocol_version;
+ version[0] = hbus->protocol_version;
ret = hv_pci_protocol_negotiation(hdev, version, 1);
if (ret)
goto out;
--
2.19.1
^ permalink raw reply related
* [PATCH v2 4/4] PCI: hv: kmemleak: Track the page allocations for struct hv_pcibus_device
From: Dexuan Cui @ 2019-11-20 7:16 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, sashal, lorenzo.pieralisi, bhelgaas,
linux-hyperv, linux-pci, linux-kernel, mikelley, Alexander.Levin
Cc: Dexuan Cui
In-Reply-To: <1574234218-49195-1-git-send-email-decui@microsoft.com>
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.
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 related
* [PATCH v2 2/4] PCI: hv: Add the support of hibernation
From: Dexuan Cui @ 2019-11-20 7:16 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, sashal, lorenzo.pieralisi, bhelgaas,
linux-hyperv, linux-pci, linux-kernel, mikelley, Alexander.Levin
Cc: Dexuan Cui
In-Reply-To: <1574234218-49195-1-git-send-email-decui@microsoft.com>
Implement the suspend/resume callbacks.
We must make sure there is no pending work items before we call
vmbus_close().
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 related
* [PATCH v2 1/4] PCI: hv: Reorganize the code in preparation of hibernation
From: Dexuan Cui @ 2019-11-20 7:16 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, sashal, lorenzo.pieralisi, bhelgaas,
linux-hyperv, linux-pci, linux-kernel, mikelley, Alexander.Levin
Cc: Dexuan Cui
In-Reply-To: <1574234218-49195-1-git-send-email-decui@microsoft.com>
There is no functional change. This is just preparatory to a later
patch which adds the hibernation support for the pci-hyperv driver.
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
No change in v2.
drivers/pci/controller/pci-hyperv.c | 43 +++++++++++++++++++----------
1 file changed, 28 insertions(+), 15 deletions(-)
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index f1f300218fab..65f18f840ce9 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -2379,7 +2379,9 @@ static void hv_pci_onchannelcallback(void *context)
* failing if the host doesn't support the necessary protocol
* level.
*/
-static int hv_pci_protocol_negotiation(struct hv_device *hdev)
+static int hv_pci_protocol_negotiation(struct hv_device *hdev,
+ enum pci_protocol_version_t version[],
+ int num_version)
{
struct pci_version_request *version_req;
struct hv_pci_compl comp_pkt;
@@ -2403,8 +2405,8 @@ static int hv_pci_protocol_negotiation(struct hv_device *hdev)
version_req = (struct pci_version_request *)&pkt->message;
version_req->message_type.type = PCI_QUERY_PROTOCOL_VERSION;
- for (i = 0; i < ARRAY_SIZE(pci_protocol_versions); i++) {
- version_req->protocol_version = pci_protocol_versions[i];
+ for (i = 0; i < num_version; i++) {
+ version_req->protocol_version = version[i];
ret = vmbus_sendpacket(hdev->channel, version_req,
sizeof(struct pci_version_request),
(unsigned long)pkt, VM_PKT_DATA_INBAND,
@@ -2420,7 +2422,7 @@ static int hv_pci_protocol_negotiation(struct hv_device *hdev)
}
if (comp_pkt.completion_status >= 0) {
- pci_protocol_version = pci_protocol_versions[i];
+ pci_protocol_version = version[i];
dev_info(&hdev->device,
"PCI VMBus probing: Using version %#x\n",
pci_protocol_version);
@@ -2930,7 +2932,8 @@ static int hv_pci_probe(struct hv_device *hdev,
hv_set_drvdata(hdev, hbus);
- ret = hv_pci_protocol_negotiation(hdev);
+ ret = hv_pci_protocol_negotiation(hdev, pci_protocol_versions,
+ ARRAY_SIZE(pci_protocol_versions));
if (ret)
goto close;
@@ -3011,7 +3014,7 @@ static int hv_pci_probe(struct hv_device *hdev,
return ret;
}
-static void hv_pci_bus_exit(struct hv_device *hdev)
+static int hv_pci_bus_exit(struct hv_device *hdev, bool hibernating)
{
struct hv_pcibus_device *hbus = hv_get_drvdata(hdev);
struct {
@@ -3027,16 +3030,20 @@ static void hv_pci_bus_exit(struct hv_device *hdev)
* access the per-channel ringbuffer any longer.
*/
if (hdev->channel->rescind)
- return;
+ return 0;
- /* Delete any children which might still exist. */
- memset(&relations, 0, sizeof(relations));
- hv_pci_devices_present(hbus, &relations);
+ if (!hibernating) {
+ /* Delete any children which might still exist. */
+ memset(&relations, 0, sizeof(relations));
+ hv_pci_devices_present(hbus, &relations);
+ }
ret = hv_send_resources_released(hdev);
- if (ret)
+ if (ret) {
dev_err(&hdev->device,
"Couldn't send resources released packet(s)\n");
+ return ret;
+ }
memset(&pkt.teardown_packet, 0, sizeof(pkt.teardown_packet));
init_completion(&comp_pkt.host_event);
@@ -3049,8 +3056,13 @@ static void hv_pci_bus_exit(struct hv_device *hdev)
(unsigned long)&pkt.teardown_packet,
VM_PKT_DATA_INBAND,
VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
- if (!ret)
- wait_for_completion_timeout(&comp_pkt.host_event, 10 * HZ);
+ if (ret)
+ return ret;
+
+ if (wait_for_completion_timeout(&comp_pkt.host_event, 10 * HZ) == 0)
+ return -ETIMEDOUT;
+
+ return 0;
}
/**
@@ -3062,6 +3074,7 @@ static void hv_pci_bus_exit(struct hv_device *hdev)
static int hv_pci_remove(struct hv_device *hdev)
{
struct hv_pcibus_device *hbus;
+ int ret;
hbus = hv_get_drvdata(hdev);
if (hbus->state == hv_pcibus_installed) {
@@ -3074,7 +3087,7 @@ static int hv_pci_remove(struct hv_device *hdev)
hbus->state = hv_pcibus_removed;
}
- hv_pci_bus_exit(hdev);
+ ret = hv_pci_bus_exit(hdev, false);
vmbus_close(hdev->channel);
@@ -3091,7 +3104,7 @@ static int hv_pci_remove(struct hv_device *hdev)
hv_put_dom_num(hbus->sysdata.domain);
free_page((unsigned long)hbus);
- return 0;
+ return ret;
}
static const struct hv_vmbus_device_id hv_pci_id_table[] = {
--
2.19.1
^ permalink raw reply related
* [PATCH v2 0/4] Enhance pci-hyperv to support hibernation, and 2 misc fixes
From: Dexuan Cui @ 2019-11-20 7:16 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, sashal, lorenzo.pieralisi, bhelgaas,
linux-hyperv, linux-pci, linux-kernel, mikelley, Alexander.Levin
Cc: Dexuan Cui
I suggest the patchset goes through the pci.git tree.
Patch #1: no functional change.
Patch #2 enhances the pci-hyperv driver to support hibernation.
Patch #3 is unrelated to hibernation.
Patch #4 is unrelated to hibernation.
Dexuan Cui (4):
PCI: hv: Reorganize the code in preparation of hibernation
PCI: hv: Add the support of hibernation
PCI: hv: Change pci_protocol_version to per-hbus
PCI: hv: kmemleak: Track the page allocations for struct
hv_pcibus_device
drivers/pci/controller/pci-hyperv.c | 179 ++++++++++++++++++++++++----
1 file changed, 153 insertions(+), 26 deletions(-)
--
2.19.1
^ permalink raw reply
* Re: [PATCH v2 1/2] x86/hyperv: Implement hv_is_hibernation_supported()
From: Thomas Gleixner @ 2019-11-20 9:49 UTC (permalink / raw)
To: Dexuan Cui
Cc: kys, haiyangz, sthemmin, sashal, linux-hyperv, linux-kernel,
mikelley, david, arnd, bp, daniel.lezcano, hpa, mingo, x86,
Alexander.Levin, vkuznets, linux-arch
In-Reply-To: <1574234165-49066-2-git-send-email-decui@microsoft.com>
On Tue, 19 Nov 2019, Dexuan Cui wrote:
> Once all the vmbus and VSC patches for the hibernation feature are
> accepted, an extra patch will be submitted to forbid hibernation if the
> virtual ACPI S4 state is absent, i.e. hv_is_hibernation_supported() is
> false.
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> ---
>
> v2 is actually the same as v1. This is just a resend.
>
> I suggest this patch should go through the Hyper-V tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/log/?h=hyperv-next
> because it's needed by
> [PATCH v2 2/2] hv_balloon: Add the support of hibernation
> and some upcoming patches.
Acked-by: Thomas Gleixner <tglx@linutronix.de>
^ 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
* 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 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] 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] 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 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 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 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 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 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] 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] 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 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 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
* [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
* [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 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
* 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
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