* [PATCH 09/11] Drivers: hv: vmbus: Synchronize init_vp_index() vs. CPU hotplug
From: Andrea Parri (Microsoft) @ 2020-04-06 0:15 UTC (permalink / raw)
To: linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, Michael Kelley, Dexuan Cui, Boqun Feng,
Vitaly Kuznetsov, Andrea Parri (Microsoft)
In-Reply-To: <20200406001514.19876-1-parri.andrea@gmail.com>
init_vp_index() may access the cpu_online_mask mask via its calls of
cpumask_of_node(). Make sure to protect these accesses with a
cpus_read_lock() critical section.
Also, remove some (hardcoded) instances of CPU(0) from init_vp_index()
and replace them with VMBUS_CONNECT_CPU. The connect CPU can not go
offline, since Hyper-V does not provide a way to change it.
Finally, order the accesses of target_cpu from init_vp_index() and
hv_synic_cleanup() by relying on the channel_mutex; this is achieved
by moving the call of init_vp_index() into vmbus_process_offer().
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/channel_mgmt.c | 47 ++++++++++++++++++++++++++++-----------
drivers/hv/hv.c | 7 +++---
2 files changed, 38 insertions(+), 16 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 9041e5950e367..3785beead503d 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -18,6 +18,7 @@
#include <linux/module.h>
#include <linux/completion.h>
#include <linux/delay.h>
+#include <linux/cpu.h>
#include <linux/hyperv.h>
#include <asm/mshyperv.h>
@@ -466,13 +467,8 @@ static void vmbus_add_channel_work(struct work_struct *work)
container_of(work, struct vmbus_channel, add_channel_work);
struct vmbus_channel *primary_channel = newchannel->primary_channel;
unsigned long flags;
- u16 dev_type;
int ret;
- dev_type = hv_get_dev_type(newchannel);
-
- init_vp_index(newchannel, dev_type);
-
/*
* This state is used to indicate a successful open
* so that when we do close the channel normally, we
@@ -504,7 +500,7 @@ static void vmbus_add_channel_work(struct work_struct *work)
if (!newchannel->device_obj)
goto err_deq_chan;
- newchannel->device_obj->device_id = dev_type;
+ newchannel->device_obj->device_id = hv_get_dev_type(newchannel);
/*
* Add the new device to the bus. This will kick off device-driver
* binding which eventually invokes the device driver's AddDevice()
@@ -560,6 +556,25 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
unsigned long flags;
bool fnew = true;
+ /*
+ * Initialize the target_CPU before inserting the channel in
+ * the chn_list and sc_list lists, within the channel_mutex
+ * critical section:
+ *
+ * CPU1 CPU2
+ *
+ * [vmbus_process_offer()] [hv_syninc_cleanup()]
+ *
+ * STORE target_cpu LOCK channel_mutex
+ * LOCK channel_mutex SEARCH chn_list
+ * INSERT chn_list LOAD target_cpu
+ * UNLOCK channel_mutex UNLOCK channel_mutex
+ *
+ * Forbids: CPU2's SEARCH from seeing CPU1's INSERT &&
+ * CPU2's LOAD from *not* seing CPU1's STORE
+ */
+ init_vp_index(newchannel, hv_get_dev_type(newchannel));
+
mutex_lock(&vmbus_connection.channel_mutex);
/* Remember the channels that should be cleaned up upon suspend. */
@@ -656,7 +671,7 @@ static DEFINE_SPINLOCK(bind_channel_to_cpu_lock);
* channel interrupt load by binding a channel to VCPU.
*
* For pre-win8 hosts or non-performance critical channels we assign the
- * first CPU in the first NUMA node.
+ * VMBUS_CONNECT_CPU.
*
* Starting with win8, performance critical channels will be distributed
* evenly among all the available NUMA nodes. Once the node is assigned,
@@ -675,17 +690,22 @@ static void init_vp_index(struct vmbus_channel *channel, u16 dev_type)
!alloc_cpumask_var(&available_mask, GFP_KERNEL)) {
/*
* Prior to win8, all channel interrupts are
- * delivered on cpu 0.
+ * delivered on VMBUS_CONNECT_CPU.
* Also if the channel is not a performance critical
- * channel, bind it to cpu 0.
- * In case alloc_cpumask_var() fails, bind it to cpu 0.
+ * channel, bind it to VMBUS_CONNECT_CPU.
+ * In case alloc_cpumask_var() fails, bind it to
+ * VMBUS_CONNECT_CPU.
*/
- channel->numa_node = 0;
- channel->target_cpu = 0;
- channel->target_vp = hv_cpu_number_to_vp_number(0);
+ channel->numa_node = cpu_to_node(VMBUS_CONNECT_CPU);
+ channel->target_cpu = VMBUS_CONNECT_CPU;
+ channel->target_vp =
+ hv_cpu_number_to_vp_number(VMBUS_CONNECT_CPU);
return;
}
+ /* No CPUs can come up or down during this. */
+ cpus_read_lock();
+
/*
* Serializes the accesses to the global variable next_numa_node_id.
* See also the header comment of the spin lock declaration.
@@ -723,6 +743,7 @@ static void init_vp_index(struct vmbus_channel *channel, u16 dev_type)
channel->target_vp = hv_cpu_number_to_vp_number(target_cpu);
spin_unlock(&bind_channel_to_cpu_lock);
+ cpus_read_unlock();
free_cpumask_var(available_mask);
}
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 17bf1f229152b..188b42b07f07b 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -256,9 +256,10 @@ int hv_synic_cleanup(unsigned int cpu)
/*
* Search for channels which are bound to the CPU we're about to
- * cleanup. In case we find one and vmbus is still connected we need to
- * fail, this will effectively prevent CPU offlining. There is no way
- * we can re-bind channels to different CPUs for now.
+ * cleanup. In case we find one and vmbus is still connected, we
+ * fail; this will effectively prevent CPU offlining.
+ *
+ * TODO: Re-bind the channels to different CPUs.
*/
mutex_lock(&vmbus_connection.channel_mutex);
list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
--
2.24.0
^ permalink raw reply related
* [PATCH 06/11] Drivers: hv: vmbus: Use a spin lock for synchronizing channel scheduling vs. channel removal
From: Andrea Parri (Microsoft) @ 2020-04-06 0:15 UTC (permalink / raw)
To: linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, Michael Kelley, Dexuan Cui, Boqun Feng,
Vitaly Kuznetsov, Andrea Parri (Microsoft)
In-Reply-To: <20200406001514.19876-1-parri.andrea@gmail.com>
Since vmbus_chan_sched() dereferences the ring buffer pointer, we have
to make sure that the ring buffer data structures don't get freed while
such dereferencing is happening. Current code does this by sending an
IPI to the CPU that is allowed to access that ring buffer from interrupt
level, cf., vmbus_reset_channel_cb(). But with the new functionality
to allow changing the CPU that a channel will interrupt, we can't be
sure what CPU will be running the vmbus_chan_sched() function for a
particular channel, so the current IPI mechanism is infeasible.
Instead synchronize vmbus_chan_sched() and vmbus_reset_channel_cb() by
using the (newly introduced) per-channel spin lock "sched_lock". Move
the test for onchannel_callback being NULL before the "switch" control
statement in vmbus_chan_sched(), in order to not access the ring buffer
if the vmbus_reset_channel_cb() has been completed on the channel.
Suggested-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
drivers/hv/channel.c | 24 +++++++-----------------
drivers/hv/channel_mgmt.c | 1 +
drivers/hv/vmbus_drv.c | 30 +++++++++++++++++-------------
include/linux/hyperv.h | 6 ++++++
4 files changed, 31 insertions(+), 30 deletions(-)
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index 256ee90c74460..132e476f87b2e 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -594,15 +594,10 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle)
}
EXPORT_SYMBOL_GPL(vmbus_teardown_gpadl);
-static void reset_channel_cb(void *arg)
-{
- struct vmbus_channel *channel = arg;
-
- channel->onchannel_callback = NULL;
-}
-
void vmbus_reset_channel_cb(struct vmbus_channel *channel)
{
+ unsigned long flags;
+
/*
* vmbus_on_event(), running in the per-channel tasklet, can race
* with vmbus_close_internal() in the case of SMP guest, e.g., when
@@ -618,17 +613,12 @@ void vmbus_reset_channel_cb(struct vmbus_channel *channel)
*/
tasklet_disable(&channel->callback_event);
- channel->sc_creation_callback = NULL;
+ /* See the inline comments in vmbus_chan_sched(). */
+ spin_lock_irqsave(&channel->sched_lock, flags);
+ channel->onchannel_callback = NULL;
+ spin_unlock_irqrestore(&channel->sched_lock, flags);
- /* Stop the callback asap */
- if (channel->target_cpu != get_cpu()) {
- put_cpu();
- smp_call_function_single(channel->target_cpu, reset_channel_cb,
- channel, true);
- } else {
- reset_channel_cb(channel);
- put_cpu();
- }
+ channel->sc_creation_callback = NULL;
/* Re-enable tasklet for use on re-open */
tasklet_enable(&channel->callback_event);
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 8c179f6866ef7..46ea978e4222a 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -315,6 +315,7 @@ static struct vmbus_channel *alloc_channel(void)
if (!channel)
return NULL;
+ spin_lock_init(&channel->sched_lock);
spin_lock_init(&channel->lock);
init_completion(&channel->rescind_event);
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 0e5a98ebe23b1..18bd1c0e09a83 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1174,18 +1174,6 @@ static void vmbus_force_channel_rescinded(struct vmbus_channel *channel)
}
#endif /* CONFIG_PM_SLEEP */
-/*
- * Direct callback for channels using other deferred processing
- */
-static void vmbus_channel_isr(struct vmbus_channel *channel)
-{
- void (*callback_fn)(void *);
-
- callback_fn = READ_ONCE(channel->onchannel_callback);
- if (likely(callback_fn != NULL))
- (*callback_fn)(channel->channel_callback_context);
-}
-
/*
* Schedule all channels with events pending
*/
@@ -1216,6 +1204,7 @@ static void vmbus_chan_sched(struct hv_per_cpu_context *hv_cpu)
return;
for_each_set_bit(relid, recv_int_page, maxbits) {
+ void (*callback_fn)(void *context);
struct vmbus_channel *channel;
if (!sync_test_and_clear_bit(relid, recv_int_page))
@@ -1241,13 +1230,26 @@ static void vmbus_chan_sched(struct hv_per_cpu_context *hv_cpu)
if (channel->rescind)
goto sched_unlock_rcu;
+ /*
+ * Make sure that the ring buffer data structure doesn't get
+ * freed while we dereference the ring buffer pointer. Test
+ * for the channel's onchannel_callback being NULL within a
+ * sched_lock critical section. See also the inline comments
+ * in vmbus_reset_channel_cb().
+ */
+ spin_lock(&channel->sched_lock);
+
+ callback_fn = channel->onchannel_callback;
+ if (unlikely(callback_fn == NULL))
+ goto sched_unlock;
+
trace_vmbus_chan_sched(channel);
++channel->interrupts;
switch (channel->callback_mode) {
case HV_CALL_ISR:
- vmbus_channel_isr(channel);
+ (*callback_fn)(channel->channel_callback_context);
break;
case HV_CALL_BATCHED:
@@ -1257,6 +1259,8 @@ static void vmbus_chan_sched(struct hv_per_cpu_context *hv_cpu)
tasklet_schedule(&channel->callback_event);
}
+sched_unlock:
+ spin_unlock(&channel->sched_lock);
sched_unlock_rcu:
rcu_read_unlock();
}
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 6c794fd5c903e..ce32ab186192f 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -771,6 +771,12 @@ struct vmbus_channel {
void (*onchannel_callback)(void *context);
void *channel_callback_context;
+ /*
+ * Synchronize channel scheduling and channel removal; see the inline
+ * comments in vmbus_chan_sched() and vmbus_reset_channel_cb().
+ */
+ spinlock_t sched_lock;
+
/*
* A channel can be marked for one of three modes of reading:
* BATCHED - callback called from taslket and should read
--
2.24.0
^ permalink raw reply related
* [PATCH 11/11] scsi: storvsc: Re-init stor_chns when a channel interrupt is re-assigned
From: Andrea Parri (Microsoft) @ 2020-04-06 0:15 UTC (permalink / raw)
To: linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, Michael Kelley, Dexuan Cui, Boqun Feng,
Vitaly Kuznetsov, Andrea Parri (Microsoft), James E.J. Bottomley,
Martin K. Petersen, linux-scsi
In-Reply-To: <20200406001514.19876-1-parri.andrea@gmail.com>
For each storvsc_device, storvsc keeps track of the channel target CPUs
associated to the device (alloced_cpus) and it uses this information to
fill a "cache" (stor_chns) mapping CPU->channel according to a certain
heuristic. Update the alloced_cpus mask and the stor_chns array when a
channel of the storvsc device is re-assigned to a different CPU.
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: <linux-scsi@vger.kernel.org>
---
drivers/hv/vmbus_drv.c | 4 ++
drivers/scsi/storvsc_drv.c | 95 ++++++++++++++++++++++++++++++++++----
include/linux/hyperv.h | 3 ++
3 files changed, 94 insertions(+), 8 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 0f204640c50c2..f0be41bfcbf57 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1750,6 +1750,10 @@ static ssize_t target_cpu_store(struct vmbus_channel *channel,
* in on a CPU that is different from the channel target_cpu value.
*/
+ if (channel->change_target_cpu_callback)
+ (*channel->change_target_cpu_callback)(channel,
+ channel->target_cpu, target_cpu);
+
channel->target_cpu = target_cpu;
channel->target_vp = hv_cpu_number_to_vp_number(target_cpu);
channel->numa_node = cpu_to_node(target_cpu);
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index fb41636519ee8..a680592b9d32a 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -621,6 +621,63 @@ static inline struct storvsc_device *get_in_stor_device(
}
+void storvsc_change_target_cpu(struct vmbus_channel *channel, u32 old, u32 new)
+{
+ struct storvsc_device *stor_device;
+ struct vmbus_channel *cur_chn;
+ bool old_is_alloced = false;
+ struct hv_device *device;
+ unsigned long flags;
+ int cpu;
+
+ device = channel->primary_channel ?
+ channel->primary_channel->device_obj
+ : channel->device_obj;
+ stor_device = get_out_stor_device(device);
+ if (!stor_device)
+ return;
+
+ /* See storvsc_do_io() -> get_og_chn(). */
+ spin_lock_irqsave(&device->channel->lock, flags);
+
+ /*
+ * Determines if the storvsc device has other channels assigned to
+ * the "old" CPU to update the alloced_cpus mask and the stor_chns
+ * array.
+ */
+ if (device->channel != channel && device->channel->target_cpu == old) {
+ cur_chn = device->channel;
+ old_is_alloced = true;
+ goto old_is_alloced;
+ }
+ list_for_each_entry(cur_chn, &device->channel->sc_list, sc_list) {
+ if (cur_chn == channel)
+ continue;
+ if (cur_chn->target_cpu == old) {
+ old_is_alloced = true;
+ goto old_is_alloced;
+ }
+ }
+
+old_is_alloced:
+ if (old_is_alloced)
+ WRITE_ONCE(stor_device->stor_chns[old], cur_chn);
+ else
+ cpumask_clear_cpu(old, &stor_device->alloced_cpus);
+
+ /* "Flush" the stor_chns array. */
+ for_each_possible_cpu(cpu) {
+ if (stor_device->stor_chns[cpu] && !cpumask_test_cpu(
+ cpu, &stor_device->alloced_cpus))
+ WRITE_ONCE(stor_device->stor_chns[cpu], NULL);
+ }
+
+ WRITE_ONCE(stor_device->stor_chns[new], channel);
+ cpumask_set_cpu(new, &stor_device->alloced_cpus);
+
+ spin_unlock_irqrestore(&device->channel->lock, flags);
+}
+
static void handle_sc_creation(struct vmbus_channel *new_sc)
{
struct hv_device *device = new_sc->primary_channel->device_obj;
@@ -648,6 +705,8 @@ static void handle_sc_creation(struct vmbus_channel *new_sc)
return;
}
+ new_sc->change_target_cpu_callback = storvsc_change_target_cpu;
+
/* Add the sub-channel to the array of available channels. */
stor_device->stor_chns[new_sc->target_cpu] = new_sc;
cpumask_set_cpu(new_sc->target_cpu, &stor_device->alloced_cpus);
@@ -876,6 +935,8 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc)
if (stor_device->stor_chns == NULL)
return -ENOMEM;
+ device->channel->change_target_cpu_callback = storvsc_change_target_cpu;
+
stor_device->stor_chns[device->channel->target_cpu] = device->channel;
cpumask_set_cpu(device->channel->target_cpu,
&stor_device->alloced_cpus);
@@ -1248,8 +1309,10 @@ static struct vmbus_channel *get_og_chn(struct storvsc_device *stor_device,
const struct cpumask *node_mask;
int num_channels, tgt_cpu;
- if (stor_device->num_sc == 0)
+ if (stor_device->num_sc == 0) {
+ stor_device->stor_chns[q_num] = stor_device->device->channel;
return stor_device->device->channel;
+ }
/*
* Our channel array is sparsley populated and we
@@ -1258,7 +1321,6 @@ static struct vmbus_channel *get_og_chn(struct storvsc_device *stor_device,
* The strategy is simple:
* I. Ensure NUMA locality
* II. Distribute evenly (best effort)
- * III. Mapping is persistent.
*/
node_mask = cpumask_of_node(cpu_to_node(q_num));
@@ -1268,8 +1330,10 @@ static struct vmbus_channel *get_og_chn(struct storvsc_device *stor_device,
if (cpumask_test_cpu(tgt_cpu, node_mask))
num_channels++;
}
- if (num_channels == 0)
+ if (num_channels == 0) {
+ stor_device->stor_chns[q_num] = stor_device->device->channel;
return stor_device->device->channel;
+ }
hash_qnum = q_num;
while (hash_qnum >= num_channels)
@@ -1295,6 +1359,7 @@ static int storvsc_do_io(struct hv_device *device,
struct storvsc_device *stor_device;
struct vstor_packet *vstor_packet;
struct vmbus_channel *outgoing_channel, *channel;
+ unsigned long flags;
int ret = 0;
const struct cpumask *node_mask;
int tgt_cpu;
@@ -1308,10 +1373,11 @@ static int storvsc_do_io(struct hv_device *device,
request->device = device;
/*
- * Select an an appropriate channel to send the request out.
+ * Select an appropriate channel to send the request out.
*/
- if (stor_device->stor_chns[q_num] != NULL) {
- outgoing_channel = stor_device->stor_chns[q_num];
+ /* See storvsc_change_target_cpu(). */
+ outgoing_channel = READ_ONCE(stor_device->stor_chns[q_num]);
+ if (outgoing_channel != NULL) {
if (outgoing_channel->target_cpu == q_num) {
/*
* Ideally, we want to pick a different channel if
@@ -1324,7 +1390,10 @@ static int storvsc_do_io(struct hv_device *device,
continue;
if (tgt_cpu == q_num)
continue;
- channel = stor_device->stor_chns[tgt_cpu];
+ channel = READ_ONCE(
+ stor_device->stor_chns[tgt_cpu]);
+ if (channel == NULL)
+ continue;
if (hv_get_avail_to_write_percent(
&channel->outbound)
> ring_avail_percent_lowater) {
@@ -1350,7 +1419,10 @@ static int storvsc_do_io(struct hv_device *device,
for_each_cpu(tgt_cpu, &stor_device->alloced_cpus) {
if (cpumask_test_cpu(tgt_cpu, node_mask))
continue;
- channel = stor_device->stor_chns[tgt_cpu];
+ channel = READ_ONCE(
+ stor_device->stor_chns[tgt_cpu]);
+ if (channel == NULL)
+ continue;
if (hv_get_avail_to_write_percent(
&channel->outbound)
> ring_avail_percent_lowater) {
@@ -1360,7 +1432,14 @@ static int storvsc_do_io(struct hv_device *device,
}
}
} else {
+ spin_lock_irqsave(&device->channel->lock, flags);
+ outgoing_channel = stor_device->stor_chns[q_num];
+ if (outgoing_channel != NULL) {
+ spin_unlock_irqrestore(&device->channel->lock, flags);
+ goto found_channel;
+ }
outgoing_channel = get_og_chn(stor_device, q_num);
+ spin_unlock_irqrestore(&device->channel->lock, flags);
}
found_channel:
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index edfcd42319ef3..9018b89614b78 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -773,6 +773,9 @@ struct vmbus_channel {
void (*onchannel_callback)(void *context);
void *channel_callback_context;
+ void (*change_target_cpu_callback)(struct vmbus_channel *channel,
+ u32 old, u32 new);
+
/*
* Synchronize channel scheduling and channel removal; see the inline
* comments in vmbus_chan_sched() and vmbus_reset_channel_cb().
--
2.24.0
^ permalink raw reply related
* [PATCH 03/11] Drivers: hv: vmbus: Replace the per-CPU channel lists with a global array of channels
From: Andrea Parri (Microsoft) @ 2020-04-06 0:15 UTC (permalink / raw)
To: linux-kernel
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, Michael Kelley, Dexuan Cui, Boqun Feng,
Vitaly Kuznetsov, Andrea Parri (Microsoft)
In-Reply-To: <20200406001514.19876-1-parri.andrea@gmail.com>
When Hyper-V sends an interrupt to the guest, the guest has to figure
out which channel the interrupt is associated with. Hyper-V sets a bit
in a memory page that is shared with the guest, indicating a particular
"relid" that the interrupt is associated with. The current Linux code
then uses a set of per-CPU linked lists to map a given "relid" to a
pointer to a channel structure.
This design introduces a synchronization problem if the CPU that Hyper-V
will interrupt for a certain channel is changed. If the interrupt comes
on the "old CPU" and the channel was already moved to the per-CPU list
of the "new CPU", then the relid -> channel mapping will fail and the
interrupt is dropped. Similarly, if the interrupt comes on the new CPU
but the channel was not moved to the per-CPU list of the new CPU, then
the mapping will fail and the interrupt is dropped.
Relids are integers ranging from 0 to 2047. The mapping from relids to
channel structures can be done by setting up an array with 2048 entries,
each entry being a pointer to a channel structure (hence total size ~16K
bytes, which is not a problem). The array is global, so there are no
per-CPU linked lists to update. The array can be searched and updated
by loading from/storing to the array at the specified index. With no
per-CPU data structures, the above mentioned synchronization problem is
avoided and the relid2channel() function gets simpler.
Suggested-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
---
NOTE. The inline comment in vmbus_channel_map_relid() follows the argument
discussed in the RFC [1]. An attempt has been made to make this argument
more 'explicit' (and, hopefully, more 'robust' against future changes) by
adding a full memory barrier in vmbus_channel_map_relid(): the barrier takes
the role of the 'implicit' full memory barriers from queue_work() and from
check_ready_for_resume_event() referred to in the RFC.
[1] https://lkml.kernel.org/r/20200403133826.GA25401@andrea
drivers/hv/channel_mgmt.c | 179 ++++++++++++++++++++++++--------------
drivers/hv/connection.c | 38 +++-----
drivers/hv/hv.c | 2 -
drivers/hv/hyperv_vmbus.h | 14 +--
drivers/hv/vmbus_drv.c | 48 ++++++----
include/linux/hyperv.h | 5 --
6 files changed, 160 insertions(+), 126 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index aed8db59a5ff8..8c179f6866ef7 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -319,7 +319,6 @@ static struct vmbus_channel *alloc_channel(void)
init_completion(&channel->rescind_event);
INIT_LIST_HEAD(&channel->sc_list);
- INIT_LIST_HEAD(&channel->percpu_list);
tasklet_init(&channel->callback_event,
vmbus_on_event, (unsigned long)channel);
@@ -340,23 +339,49 @@ static void free_channel(struct vmbus_channel *channel)
kobject_put(&channel->kobj);
}
-static void percpu_channel_enq(void *arg)
+void vmbus_channel_map_relid(struct vmbus_channel *channel)
{
- struct vmbus_channel *channel = arg;
- struct hv_per_cpu_context *hv_cpu
- = this_cpu_ptr(hv_context.cpu_context);
-
- list_add_tail_rcu(&channel->percpu_list, &hv_cpu->chan_list);
+ if (WARN_ON(channel->offermsg.child_relid >= MAX_CHANNEL_RELIDS))
+ return;
+ /*
+ * The mapping of the channel's relid is visible from the CPUs that
+ * execute vmbus_chan_sched() by the time that vmbus_chan_sched() will
+ * execute:
+ *
+ * (a) In the "normal (i.e., not resuming from hibernation)" path,
+ * the full barrier in smp_store_mb() guarantees that the store
+ * is propagated to all CPUs before the add_channel_work work
+ * is queued. In turn, add_channel_work is queued before the
+ * channel's ring buffer is allocated/initialized and the
+ * OPENCHANNEL message for the channel is sent in vmbus_open().
+ * Hyper-V won't start sending the interrupts for the channel
+ * before the OPENCHANNEL message is acked. The memory barrier
+ * in vmbus_chan_sched() -> sync_test_and_clear_bit() ensures
+ * that vmbus_chan_sched() must find the channel's relid in
+ * recv_int_page before retrieving the channel pointer from the
+ * array of channels.
+ *
+ * (b) In the "resuming from hibernation" path, the smp_store_mb()
+ * guarantees that the store is propagated to all CPUs before
+ * the VMBus connection is marked as ready for the resume event
+ * (cf. check_ready_for_resume_event()). The interrupt handler
+ * of the VMBus driver and vmbus_chan_sched() can not run before
+ * vmbus_bus_resume() has completed execution (cf. resume_noirq).
+ */
+ smp_store_mb(
+ vmbus_connection.channels[channel->offermsg.child_relid],
+ channel);
}
-static void percpu_channel_deq(void *arg)
+void vmbus_channel_unmap_relid(struct vmbus_channel *channel)
{
- struct vmbus_channel *channel = arg;
-
- list_del_rcu(&channel->percpu_list);
+ if (WARN_ON(channel->offermsg.child_relid >= MAX_CHANNEL_RELIDS))
+ return;
+ WRITE_ONCE(
+ vmbus_connection.channels[channel->offermsg.child_relid],
+ NULL);
}
-
static void vmbus_release_relid(u32 relid)
{
struct vmbus_channel_relid_released msg;
@@ -376,17 +401,25 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
struct vmbus_channel *primary_channel;
unsigned long flags;
- BUG_ON(!mutex_is_locked(&vmbus_connection.channel_mutex));
+ lockdep_assert_held(&vmbus_connection.channel_mutex);
BUG_ON(!channel->rescind);
- if (channel->target_cpu != get_cpu()) {
- put_cpu();
- smp_call_function_single(channel->target_cpu,
- percpu_channel_deq, channel, true);
- } else {
- percpu_channel_deq(channel);
- put_cpu();
- }
+ /*
+ * hv_process_channel_removal() could find INVALID_RELID only for
+ * hv_sock channels. See the inline comments in vmbus_onoffer().
+ */
+ WARN_ON(channel->offermsg.child_relid == INVALID_RELID &&
+ !is_hvsock_channel(channel));
+
+ /*
+ * Upon suspend, an in-use hv_sock channel is removed from the array of
+ * channels and the relid is invalidated. After hibernation, when the
+ * user-space appplication destroys the channel, it's unnecessary and
+ * unsafe to remove the channel from the array of channels. See also
+ * the inline comments before the call of vmbus_release_relid() below.
+ */
+ if (channel->offermsg.child_relid != INVALID_RELID)
+ vmbus_channel_unmap_relid(channel);
if (channel->primary_channel == NULL) {
list_del(&channel->listentry);
@@ -447,16 +480,6 @@ static void vmbus_add_channel_work(struct work_struct *work)
init_vp_index(newchannel, dev_type);
- if (newchannel->target_cpu != get_cpu()) {
- put_cpu();
- smp_call_function_single(newchannel->target_cpu,
- percpu_channel_enq,
- newchannel, true);
- } else {
- percpu_channel_enq(newchannel);
- put_cpu();
- }
-
/*
* This state is used to indicate a successful open
* so that when we do close the channel normally, we
@@ -523,17 +546,10 @@ static void vmbus_add_channel_work(struct work_struct *work)
spin_unlock_irqrestore(&primary_channel->lock, flags);
}
- mutex_unlock(&vmbus_connection.channel_mutex);
+ /* vmbus_process_offer() has mapped the channel. */
+ vmbus_channel_unmap_relid(newchannel);
- if (newchannel->target_cpu != get_cpu()) {
- put_cpu();
- smp_call_function_single(newchannel->target_cpu,
- percpu_channel_deq,
- newchannel, true);
- } else {
- percpu_channel_deq(newchannel);
- put_cpu();
- }
+ mutex_unlock(&vmbus_connection.channel_mutex);
vmbus_release_relid(newchannel->offermsg.child_relid);
@@ -599,6 +615,8 @@ static void vmbus_process_offer(struct vmbus_channel *newchannel)
spin_unlock_irqrestore(&channel->lock, flags);
}
+ vmbus_channel_map_relid(newchannel);
+
mutex_unlock(&vmbus_connection.channel_mutex);
/*
@@ -940,8 +958,6 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
oldchannel = find_primary_channel_by_offer(offer);
if (oldchannel != NULL) {
- atomic_dec(&vmbus_connection.offer_in_progress);
-
/*
* We're resuming from hibernation: all the sub-channel and
* hv_sock channels we had before the hibernation should have
@@ -949,36 +965,65 @@ static void vmbus_onoffer(struct vmbus_channel_message_header *hdr)
* primary channel that we had before the hibernation.
*/
+ /*
+ * { Initially: channel relid = INVALID_RELID,
+ * channels[valid_relid] = NULL }
+ *
+ * CPU1 CPU2
+ *
+ * [vmbus_onoffer()] [vmbus_device_release()]
+ *
+ * LOCK channel_mutex LOCK channel_mutex
+ * STORE channel relid = valid_relid LOAD r1 = channel relid
+ * MAP_RELID channel if (r1 != INVALID_RELID)
+ * UNLOCK channel_mutex UNMAP_RELID channel
+ * UNLOCK channel_mutex
+ *
+ * Forbids: r1 == valid_relid &&
+ * channels[valid_relid] == channel
+ *
+ * Note. r1 can be INVALID_RELID only for an hv_sock channel.
+ * None of the hv_sock channels which were present before the
+ * suspend are re-offered upon the resume. See the WARN_ON()
+ * in hv_process_channel_removal().
+ */
+ mutex_lock(&vmbus_connection.channel_mutex);
+
+ atomic_dec(&vmbus_connection.offer_in_progress);
+
WARN_ON(oldchannel->offermsg.child_relid != INVALID_RELID);
/* Fix up the relid. */
oldchannel->offermsg.child_relid = offer->child_relid;
offer_sz = sizeof(*offer);
- if (memcmp(offer, &oldchannel->offermsg, offer_sz) == 0) {
- check_ready_for_resume_event();
- return;
+ if (memcmp(offer, &oldchannel->offermsg, offer_sz) != 0) {
+ /*
+ * This is not an error, since the host can also change
+ * the other field(s) of the offer, e.g. on WS RS5
+ * (Build 17763), the offer->connection_id of the
+ * Mellanox VF vmbus device can change when the host
+ * reoffers the device upon resume.
+ */
+ pr_debug("vmbus offer changed: relid=%d\n",
+ offer->child_relid);
+
+ print_hex_dump_debug("Old vmbus offer: ",
+ DUMP_PREFIX_OFFSET, 16, 4,
+ &oldchannel->offermsg, offer_sz,
+ false);
+ print_hex_dump_debug("New vmbus offer: ",
+ DUMP_PREFIX_OFFSET, 16, 4,
+ offer, offer_sz, false);
+
+ /* Fix up the old channel. */
+ vmbus_setup_channel_state(oldchannel, offer);
}
- /*
- * This is not an error, since the host can also change the
- * other field(s) of the offer, e.g. on WS RS5 (Build 17763),
- * the offer->connection_id of the Mellanox VF vmbus device
- * can change when the host reoffers the device upon resume.
- */
- pr_debug("vmbus offer changed: relid=%d\n",
- offer->child_relid);
-
- print_hex_dump_debug("Old vmbus offer: ", DUMP_PREFIX_OFFSET,
- 16, 4, &oldchannel->offermsg, offer_sz,
- false);
- print_hex_dump_debug("New vmbus offer: ", DUMP_PREFIX_OFFSET,
- 16, 4, offer, offer_sz, false);
-
- /* Fix up the old channel. */
- vmbus_setup_channel_state(oldchannel, offer);
-
+ /* Add the channel back to the array of channels. */
+ vmbus_channel_map_relid(oldchannel);
check_ready_for_resume_event();
+ mutex_unlock(&vmbus_connection.channel_mutex);
return;
}
@@ -1036,14 +1081,14 @@ static void vmbus_onoffer_rescind(struct vmbus_channel_message_header *hdr)
*
* CPU1 CPU2
*
- * [vmbus_process_offer()] [vmbus_onoffer_rescind()]
+ * [vmbus_onoffer()] [vmbus_onoffer_rescind()]
*
* LOCK channel_mutex WAIT_ON offer_in_progress == 0
* DECREMENT offer_in_progress LOCK channel_mutex
- * INSERT chn_list SEARCH chn_list
+ * STORE channels[] LOAD channels[]
* UNLOCK channel_mutex UNLOCK channel_mutex
*
- * Forbids: CPU2's SEARCH from *not* seeing CPU1's INSERT
+ * Forbids: CPU2's LOAD from *not* seeing CPU1's STORE
*/
while (atomic_read(&vmbus_connection.offer_in_progress) != 0) {
diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index f4bd306d2cef9..11170d9a2e1a5 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -248,6 +248,14 @@ int vmbus_connect(void)
pr_info("Vmbus version:%d.%d\n",
version >> 16, version & 0xFFFF);
+ vmbus_connection.channels = kcalloc(MAX_CHANNEL_RELIDS,
+ sizeof(struct vmbus_channel *),
+ GFP_KERNEL);
+ if (vmbus_connection.channels == NULL) {
+ ret = -ENOMEM;
+ goto cleanup;
+ }
+
kfree(msginfo);
return 0;
@@ -295,33 +303,9 @@ void vmbus_disconnect(void)
*/
struct vmbus_channel *relid2channel(u32 relid)
{
- struct vmbus_channel *channel;
- struct vmbus_channel *found_channel = NULL;
- struct list_head *cur, *tmp;
- struct vmbus_channel *cur_sc;
-
- BUG_ON(!mutex_is_locked(&vmbus_connection.channel_mutex));
-
- list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
- if (channel->offermsg.child_relid == relid) {
- found_channel = channel;
- break;
- } else if (!list_empty(&channel->sc_list)) {
- /*
- * Deal with sub-channels.
- */
- list_for_each_safe(cur, tmp, &channel->sc_list) {
- cur_sc = list_entry(cur, struct vmbus_channel,
- sc_list);
- if (cur_sc->offermsg.child_relid == relid) {
- found_channel = cur_sc;
- break;
- }
- }
- }
- }
-
- return found_channel;
+ if (WARN_ON(relid >= MAX_CHANNEL_RELIDS))
+ return NULL;
+ return READ_ONCE(vmbus_connection.channels[relid]);
}
/*
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index e2b3310454640..17bf1f229152b 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -117,8 +117,6 @@ int hv_synic_alloc(void)
pr_err("Unable to allocate post msg page\n");
goto err;
}
-
- INIT_LIST_HEAD(&hv_cpu->chan_list);
}
return 0;
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index ce19a4c583884..41b7267da529a 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -132,12 +132,6 @@ struct hv_per_cpu_context {
* basis.
*/
struct tasklet_struct msg_dpc;
-
- /*
- * To optimize the mapping of relid to channel, maintain
- * per-cpu list of the channels based on their CPU affinity.
- */
- struct list_head chan_list;
};
struct hv_context {
@@ -202,6 +196,8 @@ int hv_ringbuffer_read(struct vmbus_channel *channel,
/* TODO: Need to make this configurable */
#define MAX_NUM_CHANNELS_SUPPORTED 256
+#define MAX_CHANNEL_RELIDS \
+ max(MAX_NUM_CHANNELS_SUPPORTED, HV_EVENT_FLAGS_COUNT)
enum vmbus_connect_state {
DISCONNECTED,
@@ -251,6 +247,9 @@ struct vmbus_connection {
struct list_head chn_list;
struct mutex channel_mutex;
+ /* Array of channels */
+ struct vmbus_channel **channels;
+
/*
* An offer message is handled first on the work_queue, and then
* is further handled on handle_primary_chan_wq or
@@ -337,6 +336,9 @@ int vmbus_add_channel_kobj(struct hv_device *device_obj,
void vmbus_remove_channel_attr_group(struct vmbus_channel *channel);
+void vmbus_channel_map_relid(struct vmbus_channel *channel);
+void vmbus_channel_unmap_relid(struct vmbus_channel *channel);
+
struct vmbus_channel *relid2channel(u32 relid);
void vmbus_free_channels(void);
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 41ec0c95df33f..0e5a98ebe23b1 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1225,33 +1225,39 @@ static void vmbus_chan_sched(struct hv_per_cpu_context *hv_cpu)
if (relid == 0)
continue;
+ /*
+ * Pairs with the kfree_rcu() in vmbus_chan_release().
+ * Guarantees that the channel data structure doesn't
+ * get freed while the channel pointer below is being
+ * dereferenced.
+ */
rcu_read_lock();
/* Find channel based on relid */
- list_for_each_entry_rcu(channel, &hv_cpu->chan_list, percpu_list) {
- if (channel->offermsg.child_relid != relid)
- continue;
+ channel = relid2channel(relid);
+ if (channel == NULL)
+ goto sched_unlock_rcu;
- if (channel->rescind)
- continue;
+ if (channel->rescind)
+ goto sched_unlock_rcu;
- trace_vmbus_chan_sched(channel);
+ trace_vmbus_chan_sched(channel);
- ++channel->interrupts;
+ ++channel->interrupts;
- switch (channel->callback_mode) {
- case HV_CALL_ISR:
- vmbus_channel_isr(channel);
- break;
+ switch (channel->callback_mode) {
+ case HV_CALL_ISR:
+ vmbus_channel_isr(channel);
+ break;
- case HV_CALL_BATCHED:
- hv_begin_read(&channel->inbound);
- /* fallthrough */
- case HV_CALL_DIRECT:
- tasklet_schedule(&channel->callback_event);
- }
+ case HV_CALL_BATCHED:
+ hv_begin_read(&channel->inbound);
+ fallthrough;
+ case HV_CALL_DIRECT:
+ tasklet_schedule(&channel->callback_event);
}
+sched_unlock_rcu:
rcu_read_unlock();
}
}
@@ -2237,9 +2243,12 @@ static int vmbus_bus_suspend(struct device *dev)
list_for_each_entry(channel, &vmbus_connection.chn_list, listentry) {
/*
- * Invalidate the field. Upon resume, vmbus_onoffer() will fix
- * up the field, and the other fields (if necessary).
+ * Remove the channel from the array of channels and invalidate
+ * the channel's relid. Upon resume, vmbus_onoffer() will fix
+ * up the relid (and other fields, if necessary) and add the
+ * channel back to the array.
*/
+ vmbus_channel_unmap_relid(channel);
channel->offermsg.child_relid = INVALID_RELID;
if (is_hvsock_channel(channel)) {
@@ -2475,6 +2484,7 @@ static void __exit vmbus_exit(void)
hv_debug_rm_all_dir();
vmbus_free_channels();
+ kfree(vmbus_connection.channels);
if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
kmsg_dump_unregister(&hv_kmsg_dumper);
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 692c89ccf5dfd..6c794fd5c903e 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -854,11 +854,6 @@ struct vmbus_channel {
* Support per-channel state for use by vmbus drivers.
*/
void *per_channel_state;
- /*
- * To support per-cpu lookup mapping of relid to channel,
- * link up channels based on their CPU affinity.
- */
- struct list_head percpu_list;
/*
* Defer freeing channel until after all cpu's have
--
2.24.0
^ permalink raw reply related
* [PATCH v2 0/5] Drivers: hv: cleanup VMBus messages handling
From: Vitaly Kuznetsov @ 2020-04-06 10:41 UTC (permalink / raw)
To: linux-hyperv
Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Andrea Parri (Microsoft)
A small cleanup series mostly aimed at sanitizing memory we pass to
message handlers: not passing garbage/lefrtovers from other messages
and making sure we fail early when hypervisor misbehaves.
No (real) functional change intended.
Changes since v1:
- Check that the payload size specified by the host is <= 240 bytes
- Add Michael's R-b tags.
Vitaly Kuznetsov (5):
Drivers: hv: copy from message page only what's needed
Drivers: hv: allocate the exact needed memory for messages
Drivers: hv: avoid passing opaque pointer to vmbus_onmessage()
Drivers: hv: make sure that 'struct vmbus_channel_message_header'
compiles correctly
Drivers: hv: check VMBus messages lengths
drivers/hv/channel_mgmt.c | 61 ++++++++++++++++++++-------------------
drivers/hv/hyperv_vmbus.h | 1 +
drivers/hv/vmbus_drv.c | 40 ++++++++++++++++++++-----
include/linux/hyperv.h | 2 +-
4 files changed, 66 insertions(+), 38 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH v2 2/5] Drivers: hv: allocate the exact needed memory for messages
From: Vitaly Kuznetsov @ 2020-04-06 10:41 UTC (permalink / raw)
To: linux-hyperv
Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Andrea Parri (Microsoft)
In-Reply-To: <20200406104154.45010-1-vkuznets@redhat.com>
When we need to pass a buffer with Hyper-V message we don't need to always
allocate 256 bytes for the message: the real message length is known from
the header. Change 'struct onmessage_work_context' to make it possible to
not over-allocate.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
---
drivers/hv/vmbus_drv.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 943b23beb992..b114bb411d7e 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -991,7 +991,10 @@ static struct bus_type hv_bus = {
struct onmessage_work_context {
struct work_struct work;
- struct hv_message msg;
+ struct {
+ struct hv_message_header header;
+ u8 payload[];
+ } msg;
};
static void vmbus_onmessage_work(struct work_struct *work)
@@ -1044,7 +1047,8 @@ void vmbus_on_msg_dpc(unsigned long data)
goto msg_handled;
if (entry->handler_type == VMHT_BLOCKING) {
- ctx = kmalloc(sizeof(*ctx), GFP_ATOMIC);
+ ctx = kmalloc(sizeof(*ctx) + msg->header.payload_size,
+ GFP_ATOMIC);
if (ctx == NULL)
return;
@@ -1098,10 +1102,11 @@ static void vmbus_force_channel_rescinded(struct vmbus_channel *channel)
WARN_ON(!is_hvsock_channel(channel));
/*
- * sizeof(*ctx) is small and the allocation should really not fail,
+ * Allocation size is small and the allocation should really not fail,
* otherwise the state of the hv_sock connections ends up in limbo.
*/
- ctx = kzalloc(sizeof(*ctx), GFP_KERNEL | __GFP_NOFAIL);
+ ctx = kzalloc(sizeof(*ctx) + sizeof(*rescind),
+ GFP_KERNEL | __GFP_NOFAIL);
/*
* So far, these are not really used by Linux. Just set them to the
@@ -1111,7 +1116,7 @@ static void vmbus_force_channel_rescinded(struct vmbus_channel *channel)
ctx->msg.header.payload_size = sizeof(*rescind);
/* These values are actually used by Linux. */
- rescind = (struct vmbus_channel_rescind_offer *)ctx->msg.u.payload;
+ rescind = (struct vmbus_channel_rescind_offer *)ctx->msg.payload;
rescind->header.msgtype = CHANNELMSG_RESCIND_CHANNELOFFER;
rescind->child_relid = channel->offermsg.child_relid;
--
2.25.1
^ permalink raw reply related
* [PATCH v2 1/5] Drivers: hv: copy from message page only what's needed
From: Vitaly Kuznetsov @ 2020-04-06 10:41 UTC (permalink / raw)
To: linux-hyperv
Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Andrea Parri (Microsoft)
In-Reply-To: <20200406104154.45010-1-vkuznets@redhat.com>
Hyper-V Interrupt Message Page (SIMP) has 16 256-byte slots for
messages. Each message comes with a header (16 bytes) which specifies the
payload length (up to 240 bytes). vmbus_on_msg_dpc(), however, doesn't
look at the real message length and copies the whole slot to a temporary
buffer before passing it to message handlers. This is potentially dangerous
as hypervisor doesn't have to clean the whole slot when putting a new
message there and a message handler can get access to some data which
belongs to a previous message.
Note, this is not currently a problem because all message handlers are
in-kernel but eventually we may e.g. get this exported to userspace.
Note also, that this is not a performance critical path: messages (unlike
events) represent rare events so it doesn't really matter (from performance
point of view) if we copy too much.
Fix the issue by taking into account the real message length. The temporary
buffer allocated by vmbus_on_msg_dpc() remains fixed size for now. Also,
check that the supplied payload length is valid (<= 240 bytes).
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
---
drivers/hv/vmbus_drv.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 029378c27421..943b23beb992 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1032,6 +1032,12 @@ void vmbus_on_msg_dpc(unsigned long data)
goto msg_handled;
}
+ if (msg->header.payload_size > HV_MESSAGE_PAYLOAD_BYTE_COUNT) {
+ WARN_ONCE(1, "payload size is too large (%d)\n",
+ msg->header.payload_size);
+ goto msg_handled;
+ }
+
entry = &channel_message_table[hdr->msgtype];
if (!entry->message_handler)
@@ -1043,7 +1049,8 @@ void vmbus_on_msg_dpc(unsigned long data)
return;
INIT_WORK(&ctx->work, vmbus_onmessage_work);
- memcpy(&ctx->msg, msg, sizeof(*msg));
+ memcpy(&ctx->msg, msg, sizeof(msg->header) +
+ msg->header.payload_size);
/*
* The host can generate a rescind message while we
--
2.25.1
^ permalink raw reply related
* [PATCH v2 3/5] Drivers: hv: avoid passing opaque pointer to vmbus_onmessage()
From: Vitaly Kuznetsov @ 2020-04-06 10:41 UTC (permalink / raw)
To: linux-hyperv
Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Andrea Parri (Microsoft)
In-Reply-To: <20200406104154.45010-1-vkuznets@redhat.com>
vmbus_onmessage() doesn't need the header of the message, it only
uses it to get to the payload, we can pass the pointer to the
payload directly.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
---
drivers/hv/channel_mgmt.c | 7 +------
drivers/hv/vmbus_drv.c | 3 ++-
include/linux/hyperv.h | 2 +-
3 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 0370364169c4..c6bcfee6ac99 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -1360,13 +1360,8 @@ channel_message_table[CHANNELMSG_COUNT] = {
*
* This is invoked in the vmbus worker thread context.
*/
-void vmbus_onmessage(void *context)
+void vmbus_onmessage(struct vmbus_channel_message_header *hdr)
{
- struct hv_message *msg = context;
- struct vmbus_channel_message_header *hdr;
-
- hdr = (struct vmbus_channel_message_header *)msg->u.payload;
-
trace_vmbus_on_message(hdr);
/*
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index b114bb411d7e..4ab2f1511dcb 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1007,7 +1007,8 @@ static void vmbus_onmessage_work(struct work_struct *work)
ctx = container_of(work, struct onmessage_work_context,
work);
- vmbus_onmessage(&ctx->msg);
+ vmbus_onmessage((struct vmbus_channel_message_header *)
+ &ctx->msg.payload);
kfree(ctx);
}
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 692c89ccf5df..cbd24f4e68d1 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -1017,7 +1017,7 @@ static inline void clear_low_latency_mode(struct vmbus_channel *c)
c->low_latency = false;
}
-void vmbus_onmessage(void *context);
+void vmbus_onmessage(struct vmbus_channel_message_header *hdr);
int vmbus_request_offers(void);
--
2.25.1
^ permalink raw reply related
* [PATCH v2 4/5] Drivers: hv: make sure that 'struct vmbus_channel_message_header' compiles correctly
From: Vitaly Kuznetsov @ 2020-04-06 10:43 UTC (permalink / raw)
To: linux-hyperv
Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Andrea Parri (Microsoft)
In-Reply-To: <20200406104154.45010-1-vkuznets@redhat.com>
Strictly speaking, compiler is free to use something different from 'u32'
for 'enum vmbus_channel_message_type' (e.g. char) but it doesn't happen in
real life, just add a BUILD_BUG_ON() guardian.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
---
drivers/hv/vmbus_drv.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 4ab2f1511dcb..94c02433827d 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1023,6 +1023,13 @@ void vmbus_on_msg_dpc(unsigned long data)
struct onmessage_work_context *ctx;
u32 message_type = msg->header.message_type;
+ /*
+ * 'enum vmbus_channel_message_type' is supposed to always be 'u32' as
+ * it is being used in 'struct vmbus_channel_message_header' definition
+ * which is supposed to match hypervisor ABI.
+ */
+ BUILD_BUG_ON(sizeof(enum vmbus_channel_message_type) != sizeof(u32));
+
if (message_type == HVMSG_NONE)
/* no msg */
return;
--
2.25.1
^ permalink raw reply related
* [PATCH v2 5/5] Drivers: hv: check VMBus messages lengths
From: Vitaly Kuznetsov @ 2020-04-06 10:43 UTC (permalink / raw)
To: linux-hyperv
Cc: linux-kernel, Wei Liu, Tianyu Lan, Michael Kelley,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Andrea Parri (Microsoft)
In-Reply-To: <20200406104154.45010-1-vkuznets@redhat.com>
VMBus message handlers (channel_message_table) receive a pointer to
'struct vmbus_channel_message_header' and cast it to a structure of their
choice, which is sometimes longer than the header. We, however, don't check
that the message is long enough so in case hypervisor screws up we'll be
accessing memory beyond what was allocated for temporary buffer.
Previously, we used to always allocate and copy 256 bytes from message page
to temporary buffer but this is hardly better: in case the message is
shorter than we expect we'll be trying to consume garbage as some real
data and no memory guarding technique will be able to identify an issue.
Introduce 'min_payload_len' to 'struct vmbus_channel_message_table_entry'
and check against it in vmbus_on_msg_dpc(). Note, we can't require the
exact length as new hypervisor versions may add extra fields to messages,
we only check that the message is not shorter than we expect.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
---
drivers/hv/channel_mgmt.c | 54 ++++++++++++++++++++++-----------------
drivers/hv/hyperv_vmbus.h | 1 +
drivers/hv/vmbus_drv.c | 6 +++++
3 files changed, 37 insertions(+), 24 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index c6bcfee6ac99..d4ccc9b203fa 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -1329,30 +1329,36 @@ static void vmbus_onversion_response(
/* Channel message dispatch table */
const struct vmbus_channel_message_table_entry
channel_message_table[CHANNELMSG_COUNT] = {
- { CHANNELMSG_INVALID, 0, NULL },
- { CHANNELMSG_OFFERCHANNEL, 0, vmbus_onoffer },
- { CHANNELMSG_RESCIND_CHANNELOFFER, 0, vmbus_onoffer_rescind },
- { CHANNELMSG_REQUESTOFFERS, 0, NULL },
- { CHANNELMSG_ALLOFFERS_DELIVERED, 1, vmbus_onoffers_delivered },
- { CHANNELMSG_OPENCHANNEL, 0, NULL },
- { CHANNELMSG_OPENCHANNEL_RESULT, 1, vmbus_onopen_result },
- { CHANNELMSG_CLOSECHANNEL, 0, NULL },
- { CHANNELMSG_GPADL_HEADER, 0, NULL },
- { CHANNELMSG_GPADL_BODY, 0, NULL },
- { CHANNELMSG_GPADL_CREATED, 1, vmbus_ongpadl_created },
- { CHANNELMSG_GPADL_TEARDOWN, 0, NULL },
- { CHANNELMSG_GPADL_TORNDOWN, 1, vmbus_ongpadl_torndown },
- { CHANNELMSG_RELID_RELEASED, 0, NULL },
- { CHANNELMSG_INITIATE_CONTACT, 0, NULL },
- { CHANNELMSG_VERSION_RESPONSE, 1, vmbus_onversion_response },
- { CHANNELMSG_UNLOAD, 0, NULL },
- { CHANNELMSG_UNLOAD_RESPONSE, 1, vmbus_unload_response },
- { CHANNELMSG_18, 0, NULL },
- { CHANNELMSG_19, 0, NULL },
- { CHANNELMSG_20, 0, NULL },
- { CHANNELMSG_TL_CONNECT_REQUEST, 0, NULL },
- { CHANNELMSG_22, 0, NULL },
- { CHANNELMSG_TL_CONNECT_RESULT, 0, NULL },
+ { CHANNELMSG_INVALID, 0, NULL, 0},
+ { CHANNELMSG_OFFERCHANNEL, 0, vmbus_onoffer,
+ sizeof(struct vmbus_channel_offer_channel)},
+ { CHANNELMSG_RESCIND_CHANNELOFFER, 0, vmbus_onoffer_rescind,
+ sizeof(struct vmbus_channel_rescind_offer) },
+ { CHANNELMSG_REQUESTOFFERS, 0, NULL, 0},
+ { CHANNELMSG_ALLOFFERS_DELIVERED, 1, vmbus_onoffers_delivered, 0},
+ { CHANNELMSG_OPENCHANNEL, 0, NULL, 0},
+ { CHANNELMSG_OPENCHANNEL_RESULT, 1, vmbus_onopen_result,
+ sizeof(struct vmbus_channel_open_result)},
+ { CHANNELMSG_CLOSECHANNEL, 0, NULL, 0},
+ { CHANNELMSG_GPADL_HEADER, 0, NULL, 0},
+ { CHANNELMSG_GPADL_BODY, 0, NULL, 0},
+ { CHANNELMSG_GPADL_CREATED, 1, vmbus_ongpadl_created,
+ sizeof(struct vmbus_channel_gpadl_created)},
+ { CHANNELMSG_GPADL_TEARDOWN, 0, NULL, 0},
+ { CHANNELMSG_GPADL_TORNDOWN, 1, vmbus_ongpadl_torndown,
+ sizeof(struct vmbus_channel_gpadl_torndown) },
+ { CHANNELMSG_RELID_RELEASED, 0, NULL, 0},
+ { CHANNELMSG_INITIATE_CONTACT, 0, NULL, 0},
+ { CHANNELMSG_VERSION_RESPONSE, 1, vmbus_onversion_response,
+ sizeof(struct vmbus_channel_version_response)},
+ { CHANNELMSG_UNLOAD, 0, NULL, 0},
+ { CHANNELMSG_UNLOAD_RESPONSE, 1, vmbus_unload_response, 0},
+ { CHANNELMSG_18, 0, NULL, 0},
+ { CHANNELMSG_19, 0, NULL, 0},
+ { CHANNELMSG_20, 0, NULL, 0},
+ { CHANNELMSG_TL_CONNECT_REQUEST, 0, NULL, 0},
+ { CHANNELMSG_22, 0, NULL, 0},
+ { CHANNELMSG_TL_CONNECT_RESULT, 0, NULL, 0},
};
/*
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index f5fa3b3c9baf..7fd66a4e2951 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -317,6 +317,7 @@ struct vmbus_channel_message_table_entry {
enum vmbus_channel_message_type message_type;
enum vmbus_message_handler_type handler_type;
void (*message_handler)(struct vmbus_channel_message_header *msg);
+ u32 min_payload_len;
};
extern const struct vmbus_channel_message_table_entry
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 94c02433827d..d770a2582dd0 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1054,6 +1054,12 @@ void vmbus_on_msg_dpc(unsigned long data)
if (!entry->message_handler)
goto msg_handled;
+ if (msg->header.payload_size < entry->min_payload_len) {
+ WARN_ONCE(1, "message too short: msgtype=%d len=%d\n",
+ hdr->msgtype, msg->header.payload_size);
+ goto msg_handled;
+ }
+
if (entry->handler_type == VMHT_BLOCKING) {
ctx = kmalloc(sizeof(*ctx) + msg->header.payload_size,
GFP_ATOMIC);
--
2.25.1
^ permalink raw reply related
* [PATCH V4 0/6] x86/Hyper-V: Panic code path fixes
From: ltykernel @ 2020-04-06 15:53 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, liuwe, tglx, mingo, bp, hpa, x86, arnd,
michael.h.kelley
Cc: Tianyu Lan, linux-arch, linux-hyperv, linux-kernel, vkuznets
From: Tianyu Lan <Tianyu.Lan@microsoft.com>
This patchset fixes some issues in the Hyper-V panic code path.
Patch 1 resolves issue that panic system still responses network
packets.
Patch 2-3,5-6 resolves crash enlightenment issues.
Patch 4 is to set crash_kexec_post_notifiers to true for Hyper-V
VM in order to report crash data or kmsg to host before running
kdump kernel.
Tianyu Lan (6):
x86/Hyper-V: Unload vmbus channel in hv panic callback
x86/Hyper-V: Free hv_panic_page when fail to register kmsg dump
x86/Hyper-V: Trigger crash enlightenment only once during
0;136;0c0;136;0c system crash.
x86/Hyper-V: Report crash register data or kmsg before running crash
kernel
x86/Hyper-V: Report crash register data when sysctl_record_panic_msg
is not set
x86/Hyper-V: Report crash data in die() when panic_on_oops is set
arch/x86/hyperv/hv_init.c | 6 +++-
arch/x86/kernel/cpu/mshyperv.c | 10 +++++++
drivers/hv/channel_mgmt.c | 3 ++
drivers/hv/vmbus_drv.c | 62 ++++++++++++++++++++++++++++++------------
include/asm-generic/mshyperv.h | 2 +-
5 files changed, 63 insertions(+), 20 deletions(-)
--
2.14.5
^ permalink raw reply
* [PATCH V4 1/6] x86/Hyper-V: Unload vmbus channel in hv panic callback
From: ltykernel @ 2020-04-06 15:53 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, liuwe, tglx, mingo, bp, hpa, x86, arnd,
michael.h.kelley
Cc: Tianyu Lan, linux-hyperv, linux-kernel, linux-arch, vkuznets
In-Reply-To: <20200406155331.2105-1-Tianyu.Lan@microsoft.com>
From: Tianyu Lan <Tianyu.Lan@microsoft.com>
When kdump is not configured, a Hyper-V VM might still respond to
network traffic after a kernel panic when kernel parameter panic=0.
The panic CPU goes into an infinite loop with interrupts enabled,
and the VMbus driver interrupt handler still works because the
VMbus connection is unloaded only in the kdump path. The network
responses make the other end of the connection think the VM is
still functional even though it has panic'ed, which could affect any
failover actions that should be taken.
Fix this by unloading the VMbus connection during the panic process.
vmbus_initiate_unload() could then be called twice (e.g., by
hyperv_panic_event() and hv_crash_handler(), so reset the connection
state in vmbus_initiate_unload() to ensure the unload is done only
once.
Fixes: 81b18bce48af ("Drivers: HV: Send one page worth of kmsg dump over Hyper-V during panic")
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
---
Change since v1:
- Update change log
- Use xchg() to change vmbus connection status
Change since v2:
- Update comment of registering panic callback.
Change since v3:
- Add fix commit in the change log.
---
drivers/hv/channel_mgmt.c | 3 +++
drivers/hv/vmbus_drv.c | 21 +++++++++++++--------
2 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 0370364169c4..501c43c5851d 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -839,6 +839,9 @@ void vmbus_initiate_unload(bool crash)
{
struct vmbus_channel_message_header hdr;
+ if (xchg(&vmbus_connection.conn_state, DISCONNECTED) == DISCONNECTED)
+ return;
+
/* Pre-Win2012R2 hosts don't support reconnect */
if (vmbus_proto_version < VERSION_WIN8_1)
return;
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 029378c27421..6478240d11ab 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -53,9 +53,12 @@ static int hyperv_panic_event(struct notifier_block *nb, unsigned long val,
{
struct pt_regs *regs;
- regs = current_pt_regs();
+ vmbus_initiate_unload(true);
- hyperv_report_panic(regs, val);
+ if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
+ regs = current_pt_regs();
+ hyperv_report_panic(regs, val);
+ }
return NOTIFY_DONE;
}
@@ -1391,10 +1394,16 @@ static int vmbus_bus_init(void)
}
register_die_notifier(&hyperv_die_block);
- atomic_notifier_chain_register(&panic_notifier_list,
- &hyperv_panic_block);
}
+ /*
+ * Always register the panic notifier because we need to unload
+ * the VMbus channel connection to prevent any VMbus
+ * activity after the VM panics.
+ */
+ atomic_notifier_chain_register(&panic_notifier_list,
+ &hyperv_panic_block);
+
vmbus_request_offers();
return 0;
@@ -2204,8 +2213,6 @@ static int vmbus_bus_suspend(struct device *dev)
vmbus_initiate_unload(false);
- vmbus_connection.conn_state = DISCONNECTED;
-
/* Reset the event for the next resume. */
reinit_completion(&vmbus_connection.ready_for_resume_event);
@@ -2289,7 +2296,6 @@ static void hv_kexec_handler(void)
{
hv_stimer_global_cleanup();
vmbus_initiate_unload(false);
- vmbus_connection.conn_state = DISCONNECTED;
/* Make sure conn_state is set as hv_synic_cleanup checks for it */
mb();
cpuhp_remove_state(hyperv_cpuhp_online);
@@ -2306,7 +2312,6 @@ static void hv_crash_handler(struct pt_regs *regs)
* doing the cleanup for current CPU only. This should be sufficient
* for kdump.
*/
- vmbus_connection.conn_state = DISCONNECTED;
cpu = smp_processor_id();
hv_stimer_cleanup(cpu);
hv_synic_disable_regs(cpu);
--
2.14.5
^ permalink raw reply related
* [PATCH V4 6/6] x86/Hyper-V: Report crash data in die() when panic_on_oops is set
From: ltykernel @ 2020-04-06 15:53 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, liuwe, tglx, mingo, bp, hpa, x86, arnd,
michael.h.kelley
Cc: Tianyu Lan, linux-hyperv, linux-kernel, linux-arch, vkuznets
In-Reply-To: <20200406155331.2105-1-Tianyu.Lan@microsoft.com>
From: Tianyu Lan <Tianyu.Lan@microsoft.com>
When oops happens with panic_on_oops unset, the oops
thread is killed by die() and system continues to run.
In such case, guest should not report crash register
data to host since system still runs. Check panic_on_oops
and return directly in hyperv_report_panic() when the function
is called in the die() and panic_on_oops is unset. Fix it.
Fixes: 7ed4325a44ea ("Drivers: hv: vmbus: Make panic reporting to be more useful")
Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
---
Change since v3:
- Fix compile error.
- Add fix commit in the change log
---
arch/x86/hyperv/hv_init.c | 6 +++++-
drivers/hv/vmbus_drv.c | 5 +++--
include/asm-generic/mshyperv.h | 2 +-
3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index b0da5320bcff..31e1d70f7e03 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -20,6 +20,7 @@
#include <linux/mm.h>
#include <linux/hyperv.h>
#include <linux/slab.h>
+#include <linux/kernel.h>
#include <linux/cpuhotplug.h>
#include <linux/syscore_ops.h>
#include <clocksource/hyperv_timer.h>
@@ -419,11 +420,14 @@ void hyperv_cleanup(void)
}
EXPORT_SYMBOL_GPL(hyperv_cleanup);
-void hyperv_report_panic(struct pt_regs *regs, long err)
+void hyperv_report_panic(struct pt_regs *regs, long err, bool in_die)
{
static bool panic_reported;
u64 guest_id;
+ if (in_die && !panic_on_oops)
+ return;
+
/*
* We prefer to report panic on 'die' chain as we have proper
* registers to report, but if we miss it (e.g. on BUG()) we need
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 172ceae69abb..a68bce4d0ddb 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -31,6 +31,7 @@
#include <linux/kdebug.h>
#include <linux/efi.h>
#include <linux/random.h>
+#include <linux/kernel.h>
#include <linux/syscore_ops.h>
#include <clocksource/hyperv_timer.h>
#include "hyperv_vmbus.h"
@@ -75,7 +76,7 @@ static int hyperv_panic_event(struct notifier_block *nb, unsigned long val,
if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE
&& hyperv_report_reg()) {
regs = current_pt_regs();
- hyperv_report_panic(regs, val);
+ hyperv_report_panic(regs, val, false);
}
return NOTIFY_DONE;
}
@@ -92,7 +93,7 @@ static int hyperv_die_event(struct notifier_block *nb, unsigned long val,
* the notification here.
*/
if (hyperv_report_reg())
- hyperv_report_panic(regs, val);
+ hyperv_report_panic(regs, val, true);
return NOTIFY_DONE;
}
diff --git a/include/asm-generic/mshyperv.h b/include/asm-generic/mshyperv.h
index b3f1082cc435..1c4fd950f091 100644
--- a/include/asm-generic/mshyperv.h
+++ b/include/asm-generic/mshyperv.h
@@ -163,7 +163,7 @@ static inline int cpumask_to_vpset(struct hv_vpset *vpset,
return nr_bank;
}
-void hyperv_report_panic(struct pt_regs *regs, long err);
+void hyperv_report_panic(struct pt_regs *regs, long err, bool in_die);
void hyperv_report_panic_msg(phys_addr_t pa, size_t size);
bool hv_is_hyperv_initialized(void);
bool hv_is_hibernation_supported(void);
--
2.14.5
^ permalink raw reply related
* [PATCH V4 4/6] x86/Hyper-V: Report crash register data or kmsg before running crash kernel
From: ltykernel @ 2020-04-06 15:53 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, liuwe, tglx, mingo, bp, hpa, x86, arnd,
michael.h.kelley
Cc: Tianyu Lan, linux-hyperv, linux-kernel, linux-arch, vkuznets
In-Reply-To: <20200406155331.2105-1-Tianyu.Lan@microsoft.com>
From: Tianyu Lan <Tianyu.Lan@microsoft.com>
We want to notify Hyper-V when a Linux guest VM crash occurs, so
there is a record of the crash even when kdump is enabled. But
crash_kexec_post_notifiers defaults to "false", so the kdump kernel
runs before the notifiers and Hyper-V never gets notified. Fix this by
always setting crash_kexec_post_notifiers to be true for Hyper-V VMs.
Fixes: 81b18bce48af ("Drivers: HV: Send one page worth of kmsg dump over Hyper-V during panic")
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
---
Change since v1:
Update commit log
Change since v3:
- Add fix commit in the change log.
---
arch/x86/kernel/cpu/mshyperv.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index caa032ce3fe3..5e296a7e6036 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -263,6 +263,16 @@ static void __init ms_hyperv_init_platform(void)
cpuid_eax(HYPERV_CPUID_NESTED_FEATURES);
}
+ /*
+ * Hyper-V expects to get crash register data or kmsg when
+ * crash enlightment is available and system crashes. Set
+ * crash_kexec_post_notifiers to be true to make sure that
+ * calling crash enlightment interface before running kdump
+ * kernel.
+ */
+ if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE)
+ crash_kexec_post_notifiers = true;
+
#ifdef CONFIG_X86_LOCAL_APIC
if (ms_hyperv.features & HV_X64_ACCESS_FREQUENCY_MSRS &&
ms_hyperv.misc_features & HV_FEATURE_FREQUENCY_MSRS_AVAILABLE) {
--
2.14.5
^ permalink raw reply related
* [PATCH V4 3/6] x86/Hyper-V: Trigger crash enlightenment only once during 0;136;0c0;136;0c system crash.
From: ltykernel @ 2020-04-06 15:53 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, liuwe, tglx, mingo, bp, hpa, x86, arnd,
michael.h.kelley
Cc: Tianyu Lan, linux-hyperv, linux-kernel, linux-arch, vkuznets
In-Reply-To: <20200406155331.2105-1-Tianyu.Lan@microsoft.com>
From: Tianyu Lan <Tianyu.Lan@microsoft.com>
When a guest VM panics, Hyper-V should be notified only once via the
crash synthetic MSRs. Current Linux code might write these crash MSRs
twice during a system panic:
1) hyperv_panic/die_event() calling hyperv_report_panic()
2) hv_kmsg_dump() calling hyperv_report_panic_msg()
Fix this by not calling hyperv_report_panic() if a kmsg dump has been
successfully registered. The notification will happen later via
hyperv_report_panic_msg().
Fixes: 7ed4325a44ea ("Drivers: hv: vmbus: Make panic reporting to be more useful")
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
---
Change since v1:
- Update commit log
Change since v2:
- Update comment
Change since v3:
- Add fix commit in the change log.
---
drivers/hv/vmbus_drv.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 00a511f15926..333dad39b1c1 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -55,7 +55,13 @@ static int hyperv_panic_event(struct notifier_block *nb, unsigned long val,
vmbus_initiate_unload(true);
- if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) {
+ /*
+ * Hyper-V should be notified only once about a panic. If we will be
+ * doing hyperv_report_panic_msg() later with kmsg data, don't do
+ * the notification here.
+ */
+ if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE
+ && !hv_panic_page) {
regs = current_pt_regs();
hyperv_report_panic(regs, val);
}
@@ -68,7 +74,13 @@ static int hyperv_die_event(struct notifier_block *nb, unsigned long val,
struct die_args *die = (struct die_args *)args;
struct pt_regs *regs = die->regs;
- hyperv_report_panic(regs, val);
+ /*
+ * Hyper-V should be notified only once about a panic. If we will be
+ * doing hyperv_report_panic_msg() later with kmsg data, don't do
+ * the notification here.
+ */
+ if (!hv_panic_page)
+ hyperv_report_panic(regs, val);
return NOTIFY_DONE;
}
--
2.14.5
^ permalink raw reply related
* [PATCH V4 5/6] x86/Hyper-V: Report crash register data when sysctl_record_panic_msg is not set
From: ltykernel @ 2020-04-06 15:53 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, liuwe, tglx, mingo, bp, hpa, x86, arnd,
michael.h.kelley
Cc: Tianyu Lan, linux-hyperv, linux-kernel, linux-arch, vkuznets
In-Reply-To: <20200406155331.2105-1-Tianyu.Lan@microsoft.com>
From: Tianyu Lan <Tianyu.Lan@microsoft.com>
When sysctl_record_panic_msg is not set, the panic will
not be reported to Hyper-V via hyperv_report_panic_msg().
So the crash should be reported via hyperv_report_panic().
Fixes: 81b18bce48af ("Drivers: HV: Send one page worth of kmsg dump over Hyper-V during panic")
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
---
Change since v3:
- Add fix commit in the change log
---
drivers/hv/vmbus_drv.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 333dad39b1c1..172ceae69abb 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -48,6 +48,18 @@ static int hyperv_cpuhp_online;
static void *hv_panic_page;
+/*
+ * Boolean to control whether to report panic messages over Hyper-V.
+ *
+ * It can be set via /proc/sys/kernel/hyperv/record_panic_msg
+ */
+static int sysctl_record_panic_msg = 1;
+
+static int hyperv_report_reg(void)
+{
+ return !sysctl_record_panic_msg || !hv_panic_page;
+}
+
static int hyperv_panic_event(struct notifier_block *nb, unsigned long val,
void *args)
{
@@ -61,7 +73,7 @@ static int hyperv_panic_event(struct notifier_block *nb, unsigned long val,
* the notification here.
*/
if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE
- && !hv_panic_page) {
+ && hyperv_report_reg()) {
regs = current_pt_regs();
hyperv_report_panic(regs, val);
}
@@ -79,7 +91,7 @@ static int hyperv_die_event(struct notifier_block *nb, unsigned long val,
* doing hyperv_report_panic_msg() later with kmsg data, don't do
* the notification here.
*/
- if (!hv_panic_page)
+ if (hyperv_report_reg())
hyperv_report_panic(regs, val);
return NOTIFY_DONE;
}
@@ -1267,13 +1279,6 @@ static void vmbus_isr(void)
add_interrupt_randomness(HYPERVISOR_CALLBACK_VECTOR, 0);
}
-/*
- * Boolean to control whether to report panic messages over Hyper-V.
- *
- * It can be set via /proc/sys/kernel/hyperv/record_panic_msg
- */
-static int sysctl_record_panic_msg = 1;
-
/*
* Callback from kmsg_dump. Grab as much as possible from the end of the kmsg
* buffer and call into Hyper-V to transfer the data.
--
2.14.5
^ permalink raw reply related
* [PATCH V4 2/6] x86/Hyper-V: Free hv_panic_page when fail to register kmsg dump
From: ltykernel @ 2020-04-06 15:53 UTC (permalink / raw)
To: kys, haiyangz, sthemmin, liuwe, tglx, mingo, bp, hpa, x86, arnd,
michael.h.kelley
Cc: Tianyu Lan, linux-hyperv, linux-kernel, linux-arch, vkuznets
In-Reply-To: <20200406155331.2105-1-Tianyu.Lan@microsoft.com>
From: Tianyu Lan <Tianyu.Lan@microsoft.com>
If kmsg_dump_register() fails, hv_panic_page will not be used
anywhere. So free and reset it.
Fixes: 81b18bce48af ("Drivers: HV: Send one page worth of kmsg dump over Hyper-V during panic")
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Tianyu Lan <Tianyu.Lan@microsoft.com>
---
Change since v1:
- Update commit log
- Remove hv_free_hyperv_page() in the error path
Change since v3:
- Add fix commit in the change log.
---
drivers/hv/vmbus_drv.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 6478240d11ab..00a511f15926 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -1385,9 +1385,13 @@ static int vmbus_bus_init(void)
hv_panic_page = (void *)hv_alloc_hyperv_zeroed_page();
if (hv_panic_page) {
ret = kmsg_dump_register(&hv_kmsg_dumper);
- if (ret)
+ if (ret) {
pr_err("Hyper-V: kmsg dump register "
"error 0x%x\n", ret);
+ hv_free_hyperv_page(
+ (unsigned long)hv_panic_page);
+ hv_panic_page = NULL;
+ }
} else
pr_err("Hyper-V: panic message page memory "
"allocation failed");
@@ -1416,7 +1420,6 @@ static int vmbus_bus_init(void)
hv_remove_vmbus_irq();
bus_unregister(&hv_bus);
- hv_free_hyperv_page((unsigned long)hv_panic_page);
unregister_sysctl_table(hv_ctl_table_hdr);
hv_ctl_table_hdr = NULL;
return ret;
--
2.14.5
^ permalink raw reply related
* RE: [PATCH 11/11] scsi: storvsc: Re-init stor_chns when a channel interrupt is re-assigned
From: Long Li @ 2020-04-06 19:54 UTC (permalink / raw)
To: Andrea Parri (Microsoft), linux-kernel@vger.kernel.org
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv@vger.kernel.org, Michael Kelley, Dexuan Cui,
Boqun Feng, vkuznets, James E.J. Bottomley, Martin K. Petersen,
linux-scsi@vger.kernel.org
In-Reply-To: <20200406001514.19876-12-parri.andrea@gmail.com>
>Subject: [PATCH 11/11] scsi: storvsc: Re-init stor_chns when a channel
>interrupt is re-assigned
>
>For each storvsc_device, storvsc keeps track of the channel target CPUs
>associated to the device (alloced_cpus) and it uses this information to fill a
>"cache" (stor_chns) mapping CPU->channel according to a certain heuristic.
>Update the alloced_cpus mask and the stor_chns array when a channel of the
>storvsc device is re-assigned to a different CPU.
>
>Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
>Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
>Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
>Cc: <linux-scsi@vger.kernel.org>
>---
> drivers/hv/vmbus_drv.c | 4 ++
> drivers/scsi/storvsc_drv.c | 95 ++++++++++++++++++++++++++++++++++---
>-
> include/linux/hyperv.h | 3 ++
> 3 files changed, 94 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c index
>0f204640c50c2..f0be41bfcbf57 100644
>--- a/drivers/hv/vmbus_drv.c
>+++ b/drivers/hv/vmbus_drv.c
>@@ -1750,6 +1750,10 @@ static ssize_t target_cpu_store(struct
>vmbus_channel *channel,
> * in on a CPU that is different from the channel target_cpu value.
> */
>
>+ if (channel->change_target_cpu_callback)
>+ (*channel->change_target_cpu_callback)(channel,
>+ channel->target_cpu, target_cpu);
>+
> channel->target_cpu = target_cpu;
> channel->target_vp = hv_cpu_number_to_vp_number(target_cpu);
> channel->numa_node = cpu_to_node(target_cpu); diff --git
>a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c index
>fb41636519ee8..a680592b9d32a 100644
>--- a/drivers/scsi/storvsc_drv.c
>+++ b/drivers/scsi/storvsc_drv.c
>@@ -621,6 +621,63 @@ static inline struct storvsc_device
>*get_in_stor_device(
>
> }
>
>+void storvsc_change_target_cpu(struct vmbus_channel *channel, u32 old,
>+u32 new) {
>+ struct storvsc_device *stor_device;
>+ struct vmbus_channel *cur_chn;
>+ bool old_is_alloced = false;
>+ struct hv_device *device;
>+ unsigned long flags;
>+ int cpu;
>+
>+ device = channel->primary_channel ?
>+ channel->primary_channel->device_obj
>+ : channel->device_obj;
>+ stor_device = get_out_stor_device(device);
>+ if (!stor_device)
>+ return;
>+
>+ /* See storvsc_do_io() -> get_og_chn(). */
>+ spin_lock_irqsave(&device->channel->lock, flags);
>+
>+ /*
>+ * Determines if the storvsc device has other channels assigned to
>+ * the "old" CPU to update the alloced_cpus mask and the stor_chns
>+ * array.
>+ */
>+ if (device->channel != channel && device->channel->target_cpu ==
>old) {
>+ cur_chn = device->channel;
>+ old_is_alloced = true;
>+ goto old_is_alloced;
>+ }
>+ list_for_each_entry(cur_chn, &device->channel->sc_list, sc_list) {
>+ if (cur_chn == channel)
>+ continue;
>+ if (cur_chn->target_cpu == old) {
>+ old_is_alloced = true;
>+ goto old_is_alloced;
>+ }
>+ }
>+
>+old_is_alloced:
>+ if (old_is_alloced)
>+ WRITE_ONCE(stor_device->stor_chns[old], cur_chn);
>+ else
>+ cpumask_clear_cpu(old, &stor_device->alloced_cpus);
If the old cpu is not allocated, is it still necessary to do a cpumask_clear_cpu?
>+
>+ /* "Flush" the stor_chns array. */
>+ for_each_possible_cpu(cpu) {
>+ if (stor_device->stor_chns[cpu] && !cpumask_test_cpu(
>+ cpu, &stor_device->alloced_cpus))
>+ WRITE_ONCE(stor_device->stor_chns[cpu], NULL);
>+ }
>+
>+ WRITE_ONCE(stor_device->stor_chns[new], channel);
>+ cpumask_set_cpu(new, &stor_device->alloced_cpus);
>+
>+ spin_unlock_irqrestore(&device->channel->lock, flags); }
>+
> static void handle_sc_creation(struct vmbus_channel *new_sc) {
> struct hv_device *device = new_sc->primary_channel->device_obj;
>@@ -648,6 +705,8 @@ static void handle_sc_creation(struct vmbus_channel
>*new_sc)
> return;
> }
>
>+ new_sc->change_target_cpu_callback = storvsc_change_target_cpu;
>+
> /* Add the sub-channel to the array of available channels. */
> stor_device->stor_chns[new_sc->target_cpu] = new_sc;
> cpumask_set_cpu(new_sc->target_cpu, &stor_device-
>>alloced_cpus); @@ -876,6 +935,8 @@ static int storvsc_channel_init(struct
>hv_device *device, bool is_fc)
> if (stor_device->stor_chns == NULL)
> return -ENOMEM;
>
>+ device->channel->change_target_cpu_callback =
>+storvsc_change_target_cpu;
>+
> stor_device->stor_chns[device->channel->target_cpu] = device-
>>channel;
> cpumask_set_cpu(device->channel->target_cpu,
> &stor_device->alloced_cpus);
>@@ -1248,8 +1309,10 @@ static struct vmbus_channel *get_og_chn(struct
>storvsc_device *stor_device,
> const struct cpumask *node_mask;
> int num_channels, tgt_cpu;
>
>- if (stor_device->num_sc == 0)
>+ if (stor_device->num_sc == 0) {
>+ stor_device->stor_chns[q_num] = stor_device->device-
>>channel;
> return stor_device->device->channel;
>+ }
>
> /*
> * Our channel array is sparsley populated and we @@ -1258,7 +1321,6
>@@ static struct vmbus_channel *get_og_chn(struct storvsc_device
>*stor_device,
> * The strategy is simple:
> * I. Ensure NUMA locality
> * II. Distribute evenly (best effort)
>- * III. Mapping is persistent.
> */
>
> node_mask = cpumask_of_node(cpu_to_node(q_num));
>@@ -1268,8 +1330,10 @@ static struct vmbus_channel *get_og_chn(struct
>storvsc_device *stor_device,
> if (cpumask_test_cpu(tgt_cpu, node_mask))
> num_channels++;
> }
>- if (num_channels == 0)
>+ if (num_channels == 0) {
>+ stor_device->stor_chns[q_num] = stor_device->device-
>>channel;
> return stor_device->device->channel;
>+ }
>
> hash_qnum = q_num;
> while (hash_qnum >= num_channels)
>@@ -1295,6 +1359,7 @@ static int storvsc_do_io(struct hv_device *device,
> struct storvsc_device *stor_device;
> struct vstor_packet *vstor_packet;
> struct vmbus_channel *outgoing_channel, *channel;
>+ unsigned long flags;
> int ret = 0;
> const struct cpumask *node_mask;
> int tgt_cpu;
>@@ -1308,10 +1373,11 @@ static int storvsc_do_io(struct hv_device *device,
>
> request->device = device;
> /*
>- * Select an an appropriate channel to send the request out.
>+ * Select an appropriate channel to send the request out.
> */
>- if (stor_device->stor_chns[q_num] != NULL) {
>- outgoing_channel = stor_device->stor_chns[q_num];
>+ /* See storvsc_change_target_cpu(). */
>+ outgoing_channel = READ_ONCE(stor_device->stor_chns[q_num]);
>+ if (outgoing_channel != NULL) {
> if (outgoing_channel->target_cpu == q_num) {
> /*
> * Ideally, we want to pick a different channel if @@ -
>1324,7 +1390,10 @@ static int storvsc_do_io(struct hv_device *device,
> continue;
> if (tgt_cpu == q_num)
> continue;
>- channel = stor_device->stor_chns[tgt_cpu];
>+ channel = READ_ONCE(
>+ stor_device->stor_chns[tgt_cpu]);
>+ if (channel == NULL)
>+ continue;
> if (hv_get_avail_to_write_percent(
> &channel->outbound)
> > ring_avail_percent_lowater)
>{
>@@ -1350,7 +1419,10 @@ static int storvsc_do_io(struct hv_device *device,
> for_each_cpu(tgt_cpu, &stor_device->alloced_cpus) {
> if (cpumask_test_cpu(tgt_cpu, node_mask))
> continue;
>- channel = stor_device->stor_chns[tgt_cpu];
>+ channel = READ_ONCE(
>+ stor_device->stor_chns[tgt_cpu]);
>+ if (channel == NULL)
>+ continue;
> if (hv_get_avail_to_write_percent(
> &channel->outbound)
> > ring_avail_percent_lowater)
>{
>@@ -1360,7 +1432,14 @@ static int storvsc_do_io(struct hv_device *device,
> }
> }
> } else {
>+ spin_lock_irqsave(&device->channel->lock, flags);
>+ outgoing_channel = stor_device->stor_chns[q_num];
>+ if (outgoing_channel != NULL) {
>+ spin_unlock_irqrestore(&device->channel->lock,
>flags);
Checking outgoing_channel again seems unnecessary. Why not just call get_og_chn()?
>+ goto found_channel;
>+ }
> outgoing_channel = get_og_chn(stor_device, q_num);
>+ spin_unlock_irqrestore(&device->channel->lock, flags);
> }
With device->channel->lock, now we have one more lock on the I/O issuing path. It doesn't seem optimal as you are trying to protect the code in storvsc_change_target_cpu(), this doesn't need to block concurrent I/O issuers. Maybe moving to RCU is a better approach?
Thanks,
Long
>
> found_channel:
>diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index
>edfcd42319ef3..9018b89614b78 100644
>--- a/include/linux/hyperv.h
>+++ b/include/linux/hyperv.h
>@@ -773,6 +773,9 @@ struct vmbus_channel {
> void (*onchannel_callback)(void *context);
> void *channel_callback_context;
>
>+ void (*change_target_cpu_callback)(struct vmbus_channel *channel,
>+ u32 old, u32 new);
>+
> /*
> * Synchronize channel scheduling and channel removal; see the inline
> * comments in vmbus_chan_sched() and vmbus_reset_channel_cb().
>--
>2.24.0
^ permalink raw reply
* Re: [PATCH 11/11] scsi: storvsc: Re-init stor_chns when a channel interrupt is re-assigned
From: Andrea Parri @ 2020-04-07 0:35 UTC (permalink / raw)
To: Long Li
Cc: linux-kernel@vger.kernel.org, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv@vger.kernel.org,
Michael Kelley, Dexuan Cui, Boqun Feng, vkuznets,
James E.J. Bottomley, Martin K. Petersen,
linux-scsi@vger.kernel.org
In-Reply-To: <BN8PR21MB1155C78AAC02D02EA7A7841DCEC20@BN8PR21MB1155.namprd21.prod.outlook.com>
> >@@ -621,6 +621,63 @@ static inline struct storvsc_device
> >*get_in_stor_device(
> >
> > }
> >
> >+void storvsc_change_target_cpu(struct vmbus_channel *channel, u32 old,
> >+u32 new) {
> >+ struct storvsc_device *stor_device;
> >+ struct vmbus_channel *cur_chn;
> >+ bool old_is_alloced = false;
> >+ struct hv_device *device;
> >+ unsigned long flags;
> >+ int cpu;
> >+
> >+ device = channel->primary_channel ?
> >+ channel->primary_channel->device_obj
> >+ : channel->device_obj;
> >+ stor_device = get_out_stor_device(device);
> >+ if (!stor_device)
> >+ return;
> >+
> >+ /* See storvsc_do_io() -> get_og_chn(). */
> >+ spin_lock_irqsave(&device->channel->lock, flags);
> >+
> >+ /*
> >+ * Determines if the storvsc device has other channels assigned to
> >+ * the "old" CPU to update the alloced_cpus mask and the stor_chns
> >+ * array.
> >+ */
> >+ if (device->channel != channel && device->channel->target_cpu ==
> >old) {
> >+ cur_chn = device->channel;
> >+ old_is_alloced = true;
> >+ goto old_is_alloced;
> >+ }
> >+ list_for_each_entry(cur_chn, &device->channel->sc_list, sc_list) {
> >+ if (cur_chn == channel)
> >+ continue;
> >+ if (cur_chn->target_cpu == old) {
> >+ old_is_alloced = true;
> >+ goto old_is_alloced;
> >+ }
> >+ }
> >+
> >+old_is_alloced:
> >+ if (old_is_alloced)
> >+ WRITE_ONCE(stor_device->stor_chns[old], cur_chn);
> >+ else
> >+ cpumask_clear_cpu(old, &stor_device->alloced_cpus);
>
> If the old cpu is not allocated, is it still necessary to do a cpumask_clear_cpu?
AFAICT, this really depends on how much we "believe" in the current
heuristic (as implemented by get_og_chn()): ;-)
The cpumask_clear_cpu() (and the below, dependent "flush" as well)
are intended to re-initialize alloced_cpus and stor_chns in order
for get_og_chn() to re-process/update them.
Also, notice that (both in the current code and after this series)
alloced_cpus can't be offlined and get_og_chn() does rely on this
property (cf., e.g., the loop/check over alloced_cpus/node_mask).
I suspect that giving up on this invariant/property would require
a certain amount of re-design in the heuristic/code in question...
> >@@ -1360,7 +1432,14 @@ static int storvsc_do_io(struct hv_device *device,
> > }
> > }
> > } else {
> >+ spin_lock_irqsave(&device->channel->lock, flags);
> >+ outgoing_channel = stor_device->stor_chns[q_num];
> >+ if (outgoing_channel != NULL) {
> >+ spin_unlock_irqrestore(&device->channel->lock,
> >flags);
>
> Checking outgoing_channel again seems unnecessary. Why not just call get_og_chn()?
target_cpu_store() might have changed stor_chns (and alloced_cpus)
in the meantime (but before we've acquired the device's lock): the
double check is to make sure we have a "consistent"/an up-to-date
view of stor_chns and alloced_cpus.
>
> >+ goto found_channel;
> >+ }
> > outgoing_channel = get_og_chn(stor_device, q_num);
> >+ spin_unlock_irqrestore(&device->channel->lock, flags);
> > }
>
> With device->channel->lock, now we have one more lock on the I/O issuing path. It doesn't seem optimal as you are trying to protect the code in storvsc_change_target_cpu(), this doesn't need to block concurrent I/O issuers. Maybe moving to RCU is a better approach?
I don't see this as a problem (*and I've validated such conclusion
in experiments, where the "patched kernel" was sometimes performing
slighlty better than the "unpatched kernel" and sometimes slightly
worse...):
On the one hand, the stor_chns array "stabilizes" quite early after
system initialization in "normal" (i.e., common) situations (i.e.,
no channel reassignments, no device hotplugs...); IOW, get_og_chn()
really represents the "rare and slow" path here (but not that slow!
after all...). Furthermore, notice that even in those "rare cases"
the number of "contending" channels is limited to at most 1 per 4
CPUs IIRC (alloced_cpus is "sparsely populated"...).
The latencies of the RCU grace period (in the order of milliseconds)
would be a major concern for the adoption of RCU here (at least, if
we continue to consider get_og_chn() as an "updater"). I'm afraid
that this could be "too slow" even for our slow path... ;-/
What am I missing? ;-)
Thanks,
Andrea
^ permalink raw reply
* hv_hypercall_pg page permissios
From: Christoph Hellwig @ 2020-04-07 6:55 UTC (permalink / raw)
To: K. Y. Srinivasan, Stephen Hemminger, Vitaly Kuznetsov,
Andy Lutomirski, Peter Zijlstra
Cc: x86, linux-hyperv, linux-kernel
Hi all,
The x86 Hyper-V hypercall page (hv_hypercall_pg) is the only allocation
in the kernel using __vmalloc with exectutable persmissions, and the
only user of PAGE_KERNEL_RX. Is there any good reason it needs to
be readable? Otherwise we could use vmalloc_exec and kill off
PAGE_KERNEL_RX. Note that before 372b1e91343e6 ("drivers: hv: Turn off
write permission on the hypercall page") it was even mapped writable..
^ permalink raw reply
* Re: hv_hypercall_pg page permissios
From: Vitaly Kuznetsov @ 2020-04-07 7:28 UTC (permalink / raw)
To: Christoph Hellwig
Cc: x86, linux-hyperv, linux-kernel, K. Y. Srinivasan,
Stephen Hemminger, Andy Lutomirski, Peter Zijlstra
In-Reply-To: <20200407065500.GA28490@lst.de>
Christoph Hellwig <hch@lst.de> writes:
> Hi all,
>
> The x86 Hyper-V hypercall page (hv_hypercall_pg) is the only allocation
> in the kernel using __vmalloc with exectutable persmissions, and the
> only user of PAGE_KERNEL_RX. Is there any good reason it needs to
> be readable? Otherwise we could use vmalloc_exec and kill off
> PAGE_KERNEL_RX. Note that before 372b1e91343e6 ("drivers: hv: Turn off
> write permission on the hypercall page") it was even mapped writable..
[There is nothing secret in the hypercall page, by reading it you can
figure out if you're running on Intel or AMD (VMCALL/VMMCALL) but it's
likely not the only possible way :-)]
I see no reason for hv_hypercall_pg to remain readable. I just
smoke-tested
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 7581cab74acb..17845db67fe2 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -382,7 +382,7 @@ void __init hyperv_init(void)
guest_id = generate_guest_id(0, LINUX_VERSION_CODE, 0);
wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id);
- hv_hypercall_pg = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL_RX);
+ hv_hypercall_pg = vmalloc_exec(PAGE_SIZE);
if (hv_hypercall_pg == NULL) {
wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);
goto remove_cpuhp_state;
on a Hyper-V 2016 guest and nothing broke, feel free to go ahead and
kill PAGE_KERNEL_RX.
--
Vitaly
^ permalink raw reply related
* Re: hv_hypercall_pg page permissios
From: Christoph Hellwig @ 2020-04-07 7:38 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Christoph Hellwig, x86, linux-hyperv, linux-kernel,
K. Y. Srinivasan, Stephen Hemminger, Andy Lutomirski,
Peter Zijlstra
In-Reply-To: <87v9mblpq6.fsf@vitty.brq.redhat.com>
On Tue, Apr 07, 2020 at 09:28:01AM +0200, Vitaly Kuznetsov wrote:
> Christoph Hellwig <hch@lst.de> writes:
>
> > Hi all,
> >
> > The x86 Hyper-V hypercall page (hv_hypercall_pg) is the only allocation
> > in the kernel using __vmalloc with exectutable persmissions, and the
> > only user of PAGE_KERNEL_RX. Is there any good reason it needs to
> > be readable? Otherwise we could use vmalloc_exec and kill off
> > PAGE_KERNEL_RX. Note that before 372b1e91343e6 ("drivers: hv: Turn off
> > write permission on the hypercall page") it was even mapped writable..
>
> [There is nothing secret in the hypercall page, by reading it you can
> figure out if you're running on Intel or AMD (VMCALL/VMMCALL) but it's
> likely not the only possible way :-)]
>
> I see no reason for hv_hypercall_pg to remain readable. I just
> smoke-tested
Thanks, I have the same in my WIP tree, but just wanted to confirm this
makes sense.
^ permalink raw reply
* [PATCH v1] x86: hyperv: report value of misc_features
From: Olaf Hering @ 2020-04-07 17:27 UTC (permalink / raw)
To: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
x86, linux-hyperv, linux-kernel
Cc: Olaf Hering
A few kernel features depend on ms_hyperv.misc_features, but unlike its
siblings ->features and ->hints, the value was never reported during boot.
Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
arch/x86/kernel/cpu/mshyperv.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index caa032ce3fe3..53706fb56433 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -227,8 +227,8 @@ static void __init ms_hyperv_init_platform(void)
ms_hyperv.misc_features = cpuid_edx(HYPERV_CPUID_FEATURES);
ms_hyperv.hints = cpuid_eax(HYPERV_CPUID_ENLIGHTMENT_INFO);
- pr_info("Hyper-V: features 0x%x, hints 0x%x\n",
- ms_hyperv.features, ms_hyperv.hints);
+ pr_info("Hyper-V: features 0x%x, hints 0x%x, misc 0x%x\n",
+ ms_hyperv.features, ms_hyperv.hints, ms_hyperv.misc_features);
ms_hyperv.max_vp_index = cpuid_eax(HYPERV_CPUID_IMPLEMENT_LIMITS);
ms_hyperv.max_lp_index = cpuid_ebx(HYPERV_CPUID_IMPLEMENT_LIMITS);
^ permalink raw reply related
* RE: hv_hypercall_pg page permissios
From: Dexuan Cui @ 2020-04-07 18:10 UTC (permalink / raw)
To: vkuznets, Christoph Hellwig
Cc: x86@kernel.org, linux-hyperv@vger.kernel.org,
linux-kernel@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
Andy Lutomirski, Peter Zijlstra
In-Reply-To: <87v9mblpq6.fsf@vitty.brq.redhat.com>
> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Vitaly Kuznetsov
> Sent: Tuesday, April 7, 2020 12:28 AM
> Christoph Hellwig <hch@lst.de> writes:
>
> > Hi all,
> >
> > The x86 Hyper-V hypercall page (hv_hypercall_pg) is the only allocation
> > in the kernel using __vmalloc with exectutable persmissions, and the
> > only user of PAGE_KERNEL_RX. Is there any good reason it needs to
> > be readable? Otherwise we could use vmalloc_exec and kill off
> > PAGE_KERNEL_RX. Note that before 372b1e91343e6 ("drivers: hv: Turn
> off
> > write permission on the hypercall page") it was even mapped writable..
>
> [There is nothing secret in the hypercall page, by reading it you can
> figure out if you're running on Intel or AMD (VMCALL/VMMCALL) but it's
> likely not the only possible way :-)]
>
> I see no reason for hv_hypercall_pg to remain readable. I just
> smoke-tested
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 7581cab74acb..17845db67fe2 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -382,7 +382,7 @@ void __init hyperv_init(void)
> guest_id = generate_guest_id(0, LINUX_VERSION_CODE, 0);
> wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id);
>
> - hv_hypercall_pg = __vmalloc(PAGE_SIZE, GFP_KERNEL,
> PAGE_KERNEL_RX);
> + hv_hypercall_pg = vmalloc_exec(PAGE_SIZE);
If we try to write into the page, Hyper-V will kill the guest immediately
by a virtual double-fault (or triple fault?), IIRC.
> on a Hyper-V 2016 guest and nothing broke, feel free to go ahead and
> kill PAGE_KERNEL_RX.
> --
> Vitaly
This should be OK. Just remember never try to write into the page, unless
you're trying to use this as a means of forcing a guest reboot. :-)
Thanks,
-- Dexuan
^ permalink raw reply
* Re: hv_hypercall_pg page permissios
From: Wei Liu @ 2020-04-07 20:42 UTC (permalink / raw)
To: Dexuan Cui
Cc: vkuznets, Christoph Hellwig, x86@kernel.org,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
KY Srinivasan, Stephen Hemminger, Andy Lutomirski, Peter Zijlstra,
Wei Liu
In-Reply-To: <HK0P153MB0273278D61381693E022B3ADBFC30@HK0P153MB0273.APCP153.PROD.OUTLOOK.COM>
On Tue, Apr 07, 2020 at 06:10:42PM +0000, Dexuan Cui wrote:
> > From: linux-hyperv-owner@vger.kernel.org
> > <linux-hyperv-owner@vger.kernel.org> On Behalf Of Vitaly Kuznetsov
> > Sent: Tuesday, April 7, 2020 12:28 AM
> > Christoph Hellwig <hch@lst.de> writes:
> >
> > > Hi all,
> > >
> > > The x86 Hyper-V hypercall page (hv_hypercall_pg) is the only allocation
> > > in the kernel using __vmalloc with exectutable persmissions, and the
> > > only user of PAGE_KERNEL_RX. Is there any good reason it needs to
> > > be readable? Otherwise we could use vmalloc_exec and kill off
> > > PAGE_KERNEL_RX. Note that before 372b1e91343e6 ("drivers: hv: Turn
> > off
> > > write permission on the hypercall page") it was even mapped writable..
> >
> > [There is nothing secret in the hypercall page, by reading it you can
> > figure out if you're running on Intel or AMD (VMCALL/VMMCALL) but it's
> > likely not the only possible way :-)]
> >
> > I see no reason for hv_hypercall_pg to remain readable. I just
> > smoke-tested
> >
> > diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> > index 7581cab74acb..17845db67fe2 100644
> > --- a/arch/x86/hyperv/hv_init.c
> > +++ b/arch/x86/hyperv/hv_init.c
> > @@ -382,7 +382,7 @@ void __init hyperv_init(void)
> > guest_id = generate_guest_id(0, LINUX_VERSION_CODE, 0);
> > wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id);
> >
> > - hv_hypercall_pg = __vmalloc(PAGE_SIZE, GFP_KERNEL,
> > PAGE_KERNEL_RX);
> > + hv_hypercall_pg = vmalloc_exec(PAGE_SIZE);
>
> If we try to write into the page, Hyper-V will kill the guest immediately
> by a virtual double-fault (or triple fault?), IIRC.
>
The guest would get injected a #GP fault in that case FWIW. Perhaps
that leads to further double-fault or triple-fault.
Wei.
^ 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