* [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
From: Colin King @ 2020-04-14 15:23 UTC (permalink / raw)
To: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The pointer primary_channel is being assigned with a value that is never,
The assignment is redundant and can be removed.
Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/hv/channel_mgmt.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index ffd7fffa5f83..f7bbb8dc4b0f 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
if (channel->primary_channel == NULL) {
list_del(&channel->listentry);
-
- primary_channel = channel;
} else {
primary_channel = channel->primary_channel;
spin_lock_irqsave(&primary_channel->lock, flags);
--
2.25.1
^ permalink raw reply related
* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
From: Julia Lawall @ 2020-04-14 15:42 UTC (permalink / raw)
To: Colin King
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, kernel-janitors, linux-kernel
In-Reply-To: <20200414152343.243166-1-colin.king@canonical.com>
On Tue, 14 Apr 2020, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The pointer primary_channel is being assigned with a value that is never,
never -> never used :)
> The assignment is redundant and can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/hv/channel_mgmt.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index ffd7fffa5f83..f7bbb8dc4b0f 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
>
> if (channel->primary_channel == NULL) {
> list_del(&channel->listentry);
> -
> - primary_channel = channel;
> } else {
> primary_channel = channel->primary_channel;
> spin_lock_irqsave(&primary_channel->lock, flags);
> --
> 2.25.1
>
>
^ permalink raw reply
* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
From: Vitaly Kuznetsov @ 2020-04-14 16:51 UTC (permalink / raw)
To: Colin King
Cc: kernel-janitors, linux-kernel, K . Y . Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv
In-Reply-To: <20200414152343.243166-1-colin.king@canonical.com>
Colin King <colin.king@canonical.com> writes:
> From: Colin Ian King <colin.king@canonical.com>
>
> The pointer primary_channel is being assigned with a value that is never,
> The assignment is redundant and can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/hv/channel_mgmt.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index ffd7fffa5f83..f7bbb8dc4b0f 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
>
> if (channel->primary_channel == NULL) {
> list_del(&channel->listentry);
> -
> - primary_channel = channel;
> } else {
> primary_channel = channel->primary_channel;
> spin_lock_irqsave(&primary_channel->lock, flags);
If I'm looking at the right source (5.7-rc1) it *is* beeing used:
if (channel->primary_channel == NULL) {
list_del(&channel->listentry);
primary_channel = channel;
} else {
primary_channel = channel->primary_channel;
spin_lock_irqsave(&primary_channel->lock, flags);
list_del(&channel->sc_list);
spin_unlock_irqrestore(&primary_channel->lock, flags);
}
/*
* We need to free the bit for init_vp_index() to work in the case
* of sub-channel, when we reload drivers like hv_netvsc.
*/
if (channel->affinity_policy == HV_LOCALIZED)
cpumask_clear_cpu(channel->target_cpu,
&primary_channel->alloced_cpus_in_node);
^^^^^ HERE ^^^^^
--
Vitaly
^ permalink raw reply
* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
From: Colin Ian King @ 2020-04-14 16:58 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: kernel-janitors, linux-kernel, K . Y . Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv
In-Reply-To: <87d08axb7k.fsf@vitty.brq.redhat.com>
On 14/04/2020 17:51, Vitaly Kuznetsov wrote:
> Colin King <colin.king@canonical.com> writes:
>
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The pointer primary_channel is being assigned with a value that is never,
>> The assignment is redundant and can be removed.
>>
>> Addresses-Coverity: ("Unused value")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>> drivers/hv/channel_mgmt.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
>> index ffd7fffa5f83..f7bbb8dc4b0f 100644
>> --- a/drivers/hv/channel_mgmt.c
>> +++ b/drivers/hv/channel_mgmt.c
>> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
>>
>> if (channel->primary_channel == NULL) {
>> list_del(&channel->listentry);
>> -
>> - primary_channel = channel;
>> } else {
>> primary_channel = channel->primary_channel;
>> spin_lock_irqsave(&primary_channel->lock, flags);
>
> If I'm looking at the right source (5.7-rc1) it *is* beeing used:
>
> if (channel->primary_channel == NULL) {
> list_del(&channel->listentry);
>
> primary_channel = channel;
> } else {
> primary_channel = channel->primary_channel;
> spin_lock_irqsave(&primary_channel->lock, flags);
> list_del(&channel->sc_list);
> spin_unlock_irqrestore(&primary_channel->lock, flags);
> }
>
> /*
> * We need to free the bit for init_vp_index() to work in the case
> * of sub-channel, when we reload drivers like hv_netvsc.
> */
> if (channel->affinity_policy == HV_LOCALIZED)
> cpumask_clear_cpu(channel->target_cpu,
> &primary_channel->alloced_cpus_in_node);
> ^^^^^ HERE ^^^^^
>
I was basing my change on linux-next that has removed a hunk of code:
commit bcefa400900739310e8ef0cb34cbe029c404455c
Author: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
Date: Mon Apr 6 02:15:11 2020 +0200
Drivers: hv: vmbus: Remove the unused HV_LOCALIZED channel affinity
logic
The logic is unused since commit 509879bdb30b8 ("Drivers: hv: Introduce
a policy for controlling channel affinity").
This logic assumes that a channel target_cpu doesn't change during the
lifetime of a channel, but this assumption is incompatible with the new
functionality that allows changing the vCPU a channel will interrupt.
Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
Link:
https://lore.kernel.org/r/20200406001514.19876-9-parri.andrea@gmail.com
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Wei Liu <wei.liu@kernel.org>
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 7fb6eb647f14..476592b0bc00 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -433,14 +433,6 @@ void hv_process_channel_removal(struct
vmbus_channel *channel)
spin_unlock_irqrestore(&primary_channel->lock, flags);
}
- /*
- * We need to free the bit for init_vp_index() to work in the case
- * of sub-channel, when we reload drivers like hv_netvsc.
- */
- if (channel->affinity_policy == HV_LOCALIZED)
- cpumask_clear_cpu(channel->target_cpu,
- &primary_channel->alloced_cpus_in_node);
-
^ permalink raw reply related
* RE: SCSI low level driver: how to prevent I/O upon hibernation?
From: Dexuan Cui @ 2020-04-14 17:09 UTC (permalink / raw)
To: Bart Van Assche, Ming Lei, Martin K. Petersen
Cc: linux-scsi@vger.kernel.org, Christoph Hellwig,
linux-hyperv@vger.kernel.org, Long Li, vkuznets, Michael Kelley,
KY Srinivasan, Olaf Hering, Stephen Hemminger,
linux-kernel@vger.kernel.org
In-Reply-To: <HK0P153MB0273B8F511D973C2428174EBBFDF0@HK0P153MB0273.APCP153.PROD.OUTLOOK.COM>
> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Dexuan Cui
> Sent: Saturday, April 11, 2020 10:20 AM
> To: Bart Van Assche <bvanassche@acm.org>; Ming Lei
> <tom.leiming@gmail.com>; Martin K. Petersen <martin.petersen@oracle.com>
> Cc: linux-scsi@vger.kernel.org; Christoph Hellwig <hch@lst.de>;
> linux-hyperv@vger.kernel.org; Long Li <longli@microsoft.com>; vkuznets
> <vkuznets@redhat.com>; Michael Kelley <mikelley@microsoft.com>; KY
> Srinivasan <kys@microsoft.com>; Olaf Hering <olaf@aepfle.de>; Stephen
> Hemminger <sthemmin@microsoft.com>; linux-kernel@vger.kernel.org
> Subject: RE: SCSI low level driver: how to prevent I/O upon hibernation?
>
> > From: Bart Van Assche <bvanassche@acm.org>
> > Sent: Saturday, April 11, 2020 8:03 AM
> >
> > On 2020-04-10 23:01, Dexuan Cui wrote:
> > > Please let me know if the change to scsi_device_set_state() is OK.
> >
> > Hadn't Ming Lei already root-caused this issue for you? From his e-mail:
> > "So you can't free related vmbus ringbuffer cause BLK_MQ_REQ_PREEMPT
> > request is still to be handled."
> >
> > Please follow that advice.
> >
> > Bart.
>
> Hi Bart, Ming,
> I agree Ming has root-caused the issue, but it looks the advice can not
> apply to the hibernation scenario. :-)
>
> Sorry for my lack of knowledge of the complex SCSI subsystems -- could
> you please elaborate on what a low level SCSI device driver (like hv_storvsc)
> should do to safely save/restore the device state upon hibernation?
>
> The nature of "free related vmbus ringbuffer" in hv_storvsc is that: the
> driver can not handle any I/O after the device is quiesced in
> software_resume() -> load_image_and_restore() -> hibernation_restore()
> -> dpm_suspend_start() -> ... -> storvsc_suspend(). BTW, after the SCSI
> device is quiesced, the hibernation's resume path also quiesces other
> devices, disables non-boot CPUs, and finally jumps to the old kernel's
> entry point where the old kernel was suspended, and the old kernel will
> resume back.
>
> My intuition is that the upper level SCSI layer should provide an API to
> flush any pending I/O and block any new I/O after a SCSI device is
> "quiesced"? -- it looks scsi_host_block()/scsi_host_unblock() are such
> APIs, which are already used by
> drivers/scsi/aacraid/linit.c: aac_suspend()/aac_resume().
>
> That's why I proposed the patch of the same thing for hv_storvsc, and
> it looks the patch works for me: without the patch I can easily hit the
> panic I reported in the first email; with the patch, I have successfully
> done more than 30 rounds of hibernation without the panic.
>
> However, it looks you implied my intuition is wrong and it's *expected*
> that the upper level SCSI layer can still submit I/O requests with the
> BLK_MQ_REQ_PREEMPT flag after the SCSI device is "quiesced"?
>
> If this is the case, then how is hv_storvsc supposed to handle the I/O
> after the SCSI device is quiesced? I can keep the related vmbus ringbuffer,
> but the real issue is: the driver is unable to handle any I/O at all since the
> vmbus connection to the Hyper-V host is disconnected soon, after
> the SCSI device is quiesced. Should hv_storvsc return an error for such I/O,
> or block such I/O until the SCSI device is resumed? -- These don't look
> good to me, and I really think the upper level SCSI layer should provide
> an API to block any new I/O after a SCSI device is "quiesced" -- again, can
> you please clarify if scsi_host_block()/scsi_host_unblock() are such APIs?
>
> Looking forward to your replies!
>
> Thanks,
> -- Dexuan
It looks scsi_host_block() and scsi_host_unblock() are just the APIs
I need. I have run my hibernation test with the APIs more than 1000 rounds
and the VM is still running fine without the panic I reported previously when
I didn't use the APIs.
FYI: the aacraid driver is the first user of the APIs:
commit 3d3ca53b163914c1397289d0c2ee6d2f52362dcc
Author: Hannes Reinecke <hare@suse.de>
Date: Fri Feb 28 08:53:14 2020 +0100
scsi: aacraid: use scsi_host_(block,unblock) to block I/O
Use scsi_host_block() and scsi_host_unblock() instead of
scsi_block_requests()/scsi_unblock_requests() to block and unblock I/O.
This has the advantage that the block layer will stop sending I/O to the
adapter instead of having the SCSI midlayer requeueing I/O internally.
Link: https://lore.kernel.org/r/20200228075318.91255-10-hare@suse.de
Reviewed-by: Christoph Hellwig <hch@lst.de>
Acked-by: Balsundar P < Balsundar.P@microchip.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
BTW, I suspect aac_suspend() -> scsi_host_block() is not relly working
because scsi_host_block() doesn't allow a state transition from
SDEV_QUIESCE to SDEV_BLOCK and returns -EINVAL for that. aac_suspend()
should check the return value of scsi_host_block(), and my change to
scsi_device_set_state() should be needed to avoid the -EINVAL error.
If there is no objection, I plan to send some patches later.
Thanks,
-- Dexuan
^ permalink raw reply
* Re: [GIT PULL] Hyper-V fixes for 5.7-rc1
From: pr-tracker-bot @ 2020-04-14 19:05 UTC (permalink / raw)
To: Wei Liu
Cc: Linus Torvalds, Wei Liu, kys, sthemmin, linux-kernel,
linux-hyperv, mikelley, haiyangz
In-Reply-To: <20200414102102.7jci4pzsp5tdoifr@debian>
The pull request you sent on Tue, 14 Apr 2020 11:21:02 +0100:
> ssh://git@gitolite.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git tags/hyperv-fixes-signed
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/8632e9b5645bbc2331d21d892b0d6961c1a08429
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH v3] Drivers: hv: vmbus: Fix Suspend-to-Idle for Generation-2 VM
From: Wei Liu @ 2020-04-14 20:18 UTC (permalink / raw)
To: Dexuan Cui
Cc: kys, haiyangz, sthemmin, wei.liu, linux-hyperv, linux-kernel,
mikelley, vkuznets, stable
In-Reply-To: <1586663435-36243-1-git-send-email-decui@microsoft.com>
On Sat, Apr 11, 2020 at 08:50:35PM -0700, Dexuan Cui wrote:
> Before the hibernation patchset (e.g. f53335e3289f), in a Generation-2
> Linux VM on Hyper-V, the user can run "echo freeze > /sys/power/state" to
> freeze the system, i.e. Suspend-to-Idle. The user can press the keyboard
> or move the mouse to wake up the VM.
>
> With the hibernation patchset, Linux VM on Hyper-V can hibernate to disk,
> but Suspend-to-Idle is broken: when the synthetic keyboard/mouse are
> suspended, there is no way to wake up the VM.
>
> Fix the issue by not suspending and resuming the vmbus devices upon
> Suspend-to-Idle.
>
> Fixes: f53335e3289f ("Drivers: hv: vmbus: Suspend/resume the vmbus itself for hibernation")
> Cc: stable@vger.kernel.org
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
Applied to hyperv-fixes. Thanks.
Wei.
^ permalink raw reply
* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
From: Wei Liu @ 2020-04-14 20:24 UTC (permalink / raw)
To: Colin King
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
linux-hyperv, kernel-janitors, linux-kernel
In-Reply-To: <20200414152343.243166-1-colin.king@canonical.com>
On Tue, Apr 14, 2020 at 04:23:43PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The pointer primary_channel is being assigned with a value that is never,
> The assignment is redundant and can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Thanks.
Now that the only user of primary_channel is within the else branch, we
can go one step further to move the definition of primary_channel there.
I can make the adjustment while committing this patch, as well as
updating the commit message.
Wei.
> ---
> drivers/hv/channel_mgmt.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index ffd7fffa5f83..f7bbb8dc4b0f 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
>
> if (channel->primary_channel == NULL) {
> list_del(&channel->listentry);
> -
> - primary_channel = channel;
> } else {
> primary_channel = channel->primary_channel;
> spin_lock_irqsave(&primary_channel->lock, flags);
> --
> 2.25.1
>
^ permalink raw reply
* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
From: Colin Ian King @ 2020-04-14 21:25 UTC (permalink / raw)
To: Wei Liu
Cc: K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger,
linux-hyperv, kernel-janitors, linux-kernel
In-Reply-To: <20200414202412.op5kxcqc7k4bce6z@debian>
On 14/04/2020 21:24, Wei Liu wrote:
> On Tue, Apr 14, 2020 at 04:23:43PM +0100, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The pointer primary_channel is being assigned with a value that is never,
>> The assignment is redundant and can be removed.
>>
>> Addresses-Coverity: ("Unused value")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>
> Thanks.
>
> Now that the only user of primary_channel is within the else branch, we
> can go one step further to move the definition of primary_channel there.
>
> I can make the adjustment while committing this patch, as well as
> updating the commit message.
>
> Wei.
Thanks Wei,
Colin
>
>> ---
>> drivers/hv/channel_mgmt.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
>> index ffd7fffa5f83..f7bbb8dc4b0f 100644
>> --- a/drivers/hv/channel_mgmt.c
>> +++ b/drivers/hv/channel_mgmt.c
>> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
>>
>> if (channel->primary_channel == NULL) {
>> list_del(&channel->listentry);
>> -
>> - primary_channel = channel;
>> } else {
>> primary_channel = channel->primary_channel;
>> spin_lock_irqsave(&primary_channel->lock, flags);
>> --
>> 2.25.1
>>
^ permalink raw reply
* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
From: Vitaly Kuznetsov @ 2020-04-15 10:43 UTC (permalink / raw)
To: Colin Ian King
Cc: kernel-janitors, linux-kernel, K . Y . Srinivasan, Haiyang Zhang,
Stephen Hemminger, Wei Liu, linux-hyperv
In-Reply-To: <606c442c-1923-77d4-c350-e06878172c44@canonical.com>
Colin Ian King <colin.king@canonical.com> writes:
> On 14/04/2020 17:51, Vitaly Kuznetsov wrote:
>> Colin King <colin.king@canonical.com> writes:
>>
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> The pointer primary_channel is being assigned with a value that is never,
>>> The assignment is redundant and can be removed.
>>>
>>> Addresses-Coverity: ("Unused value")
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>> ---
>>> drivers/hv/channel_mgmt.c | 2 --
>>> 1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
>>> index ffd7fffa5f83..f7bbb8dc4b0f 100644
>>> --- a/drivers/hv/channel_mgmt.c
>>> +++ b/drivers/hv/channel_mgmt.c
>>> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
>>>
>>> if (channel->primary_channel == NULL) {
>>> list_del(&channel->listentry);
>>> -
>>> - primary_channel = channel;
>>> } else {
>>> primary_channel = channel->primary_channel;
>>> spin_lock_irqsave(&primary_channel->lock, flags);
>>
>> If I'm looking at the right source (5.7-rc1) it *is* beeing used:
>>
>> if (channel->primary_channel == NULL) {
>> list_del(&channel->listentry);
>>
>> primary_channel = channel;
>> } else {
>> primary_channel = channel->primary_channel;
>> spin_lock_irqsave(&primary_channel->lock, flags);
>> list_del(&channel->sc_list);
>> spin_unlock_irqrestore(&primary_channel->lock, flags);
>> }
>>
>> /*
>> * We need to free the bit for init_vp_index() to work in the case
>> * of sub-channel, when we reload drivers like hv_netvsc.
>> */
>> if (channel->affinity_policy == HV_LOCALIZED)
>> cpumask_clear_cpu(channel->target_cpu,
>> &primary_channel->alloced_cpus_in_node);
>> ^^^^^ HERE ^^^^^
>>
>
> I was basing my change on linux-next that has removed a hunk of code:
>
> commit bcefa400900739310e8ef0cb34cbe029c404455c
> Author: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> Date: Mon Apr 6 02:15:11 2020 +0200
>
> Drivers: hv: vmbus: Remove the unused HV_LOCALIZED channel affinity
> logic
>
Ah, please add the right 'Fixes:' tag then.
--
Vitaly
^ permalink raw reply
* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
From: Wei Liu @ 2020-04-15 14:37 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Colin Ian King, kernel-janitors, linux-kernel, K . Y . Srinivasan,
Haiyang Zhang, Stephen Hemminger, Wei Liu, linux-hyperv
In-Reply-To: <87wo6hvxkz.fsf@vitty.brq.redhat.com>
On Wed, Apr 15, 2020 at 12:43:08PM +0200, Vitaly Kuznetsov wrote:
> Colin Ian King <colin.king@canonical.com> writes:
>
> > On 14/04/2020 17:51, Vitaly Kuznetsov wrote:
> >> Colin King <colin.king@canonical.com> writes:
> >>
> >>> From: Colin Ian King <colin.king@canonical.com>
> >>>
> >>> The pointer primary_channel is being assigned with a value that is never,
> >>> The assignment is redundant and can be removed.
> >>>
> >>> Addresses-Coverity: ("Unused value")
> >>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> >>> ---
> >>> drivers/hv/channel_mgmt.c | 2 --
> >>> 1 file changed, 2 deletions(-)
> >>>
> >>> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> >>> index ffd7fffa5f83..f7bbb8dc4b0f 100644
> >>> --- a/drivers/hv/channel_mgmt.c
> >>> +++ b/drivers/hv/channel_mgmt.c
> >>> @@ -425,8 +425,6 @@ void hv_process_channel_removal(struct vmbus_channel *channel)
> >>>
> >>> if (channel->primary_channel == NULL) {
> >>> list_del(&channel->listentry);
> >>> -
> >>> - primary_channel = channel;
> >>> } else {
> >>> primary_channel = channel->primary_channel;
> >>> spin_lock_irqsave(&primary_channel->lock, flags);
> >>
> >> If I'm looking at the right source (5.7-rc1) it *is* beeing used:
> >>
> >> if (channel->primary_channel == NULL) {
> >> list_del(&channel->listentry);
> >>
> >> primary_channel = channel;
> >> } else {
> >> primary_channel = channel->primary_channel;
> >> spin_lock_irqsave(&primary_channel->lock, flags);
> >> list_del(&channel->sc_list);
> >> spin_unlock_irqrestore(&primary_channel->lock, flags);
> >> }
> >>
> >> /*
> >> * We need to free the bit for init_vp_index() to work in the case
> >> * of sub-channel, when we reload drivers like hv_netvsc.
> >> */
> >> if (channel->affinity_policy == HV_LOCALIZED)
> >> cpumask_clear_cpu(channel->target_cpu,
> >> &primary_channel->alloced_cpus_in_node);
> >> ^^^^^ HERE ^^^^^
> >>
> >
> > I was basing my change on linux-next that has removed a hunk of code:
> >
> > commit bcefa400900739310e8ef0cb34cbe029c404455c
> > Author: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> > Date: Mon Apr 6 02:15:11 2020 +0200
> >
> > Drivers: hv: vmbus: Remove the unused HV_LOCALIZED channel affinity
> > logic
> >
>
> Ah, please add the right 'Fixes:' tag then.
I don't think the Fixes tag is particularly useful in this instance.
Andrea's commit is not yet in Linus' tree. If I rebase hyper-next over
the next 2.5 months the tag is going to have a stale commit hash in it.
Wei.
>
> --
> Vitaly
>
^ permalink raw reply
* Re: [PATCH v1 1/1] x86/kvm/hyper-v: Add support to SYNIC exit on EOM
From: Vitaly Kuznetsov @ 2020-04-16 6:57 UTC (permalink / raw)
To: Jon Doron; +Cc: Jon Doron, Roman Kagan, kvm, linux-hyperv
In-Reply-To: <20200409163745.573547-2-arilou@gmail.com>
Jon Doron <arilou@gmail.com> writes:
> According to the TLFS a write to the EOM register by the guest
> causes the hypervisor to scan for any pending messages and if there
> are any it will try to deliver them.
>
> To do this we must exit so any pending messages can be written.
>
> Signed-off-by: Jon Doron <arilou@gmail.com>
Roman says he's still with us so let's Cc: him.
> ---
> arch/x86/include/asm/kvm_host.h | 1 +
> arch/x86/kvm/hyperv.c | 65 +++++++++++++++++++++++++++++----
> arch/x86/kvm/hyperv.h | 1 +
> arch/x86/kvm/x86.c | 5 +++
> include/uapi/linux/kvm.h | 1 +
> 5 files changed, 65 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 42a2d0d3984a..048a1db488e2 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -548,6 +548,7 @@ struct kvm_vcpu_hv_synic {
> DECLARE_BITMAP(vec_bitmap, 256);
> bool active;
> bool dont_zero_synic_pages;
> + bool enable_eom_exit;
> };
>
> /* Hyper-V per vcpu emulation context */
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index bcefa9d4e57e..7432f67b2746 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -186,6 +186,49 @@ static void kvm_hv_notify_acked_sint(struct kvm_vcpu *vcpu, u32 sint)
> srcu_read_unlock(&kvm->irq_srcu, idx);
> }
>
> +static int synic_read_msg(struct kvm_vcpu_hv_synic *synic, u32 sint,
> + struct hv_message_header *msg)
I'd suggest to rename this to 'synic_read_msg_hdr()' as we don't
actually read the message here, just the header.
> +{
> + struct kvm_vcpu *vcpu = synic_to_vcpu(synic);
> + int msg_off = offsetof(struct hv_message_page, sint_message[sint]);
> + gfn_t msg_page_gfn;
> + int r;
> +
> + if (!(synic->msg_page & HV_SYNIC_SIMP_ENABLE))
> + return -ENOENT;
> +
> + msg_page_gfn = synic->msg_page >> PAGE_SHIFT;
> +
> + r = kvm_vcpu_read_guest_page(vcpu, msg_page_gfn, msg, msg_off,
> + sizeof(*msg));
> + if (r < 0)
> + return r;
> +
> + return 0;
> +}
> +
> +static bool synic_should_exit_for_eom(struct kvm_vcpu_hv_synic *synic)
'for_eom' or 'on_eom'?
> +{
> + int i;
> +
> + if (!synic->enable_eom_exit)
> + return false;
> +
> + for (i = 0; i < ARRAY_SIZE(synic->sint); i++) {
> + struct hv_message_header hv_hdr;
> + /* If we failed to read from the msg slot then we treat this
> + * msg slot as free */
Coding style: multi-line comments should look like
/*
* line1
* line2
* ...
*/
> + if (synic_read_msg(synic, i, &hv_hdr) < 0)
> + continue;
> +
> + /* See if this msg slot has a pending message */
> + if (hv_hdr.message_flags.msg_pending == 1)
> + return true;
> + }
> +
> + return false;
> +}
> +
> static void synic_exit(struct kvm_vcpu_hv_synic *synic, u32 msr)
> {
> struct kvm_vcpu *vcpu = synic_to_vcpu(synic);
> @@ -254,6 +297,9 @@ static int synic_set_msr(struct kvm_vcpu_hv_synic *synic,
>
> for (i = 0; i < ARRAY_SIZE(synic->sint); i++)
> kvm_hv_notify_acked_sint(vcpu, i);
> +
> + if (!host && synic_should_exit_for_eom(synic))
> + synic_exit(synic, msr);
Generally, message communication is not performance critical, however,
we have synthetic timers and in case the new KVM_CAP_HYPERV_SYNIC_EOM
cap is enabled we will be exiting to userspace for every timer
expiration. This will measurably slow down the guest I'm afraid.
Would it be possible to come up with an interface for userspace to tell
KVM that it has a pending message for the guest and only exit to
userspace in this case? Or maybe we need to queue all messages from
userspace in KVM and deliver them when we get a chance and not exit to
userspace at all?
> break;
> }
> case HV_X64_MSR_SINT0 ... HV_X64_MSR_SINT15:
> @@ -571,8 +617,9 @@ static int synic_deliver_msg(struct kvm_vcpu_hv_synic *synic, u32 sint,
> struct hv_message_header hv_hdr;
> int r;
>
> - if (!(synic->msg_page & HV_SYNIC_SIMP_ENABLE))
> - return -ENOENT;
> + r = synic_read_msg(synic, sint, &hv_hdr);
> + if (r < 0)
> + return r;
>
> msg_page_gfn = synic->msg_page >> PAGE_SHIFT;
>
> @@ -582,12 +629,6 @@ static int synic_deliver_msg(struct kvm_vcpu_hv_synic *synic, u32 sint,
> * is only called in vcpu context so the entire update is atomic from
> * guest POV and thus the exact order here doesn't matter.
> */
> - r = kvm_vcpu_read_guest_page(vcpu, msg_page_gfn, &hv_hdr.message_type,
> - msg_off + offsetof(struct hv_message,
> - header.message_type),
> - sizeof(hv_hdr.message_type));
> - if (r < 0)
> - return r;
>
> if (hv_hdr.message_type != HVMSG_NONE) {
> if (no_retry)
> @@ -785,6 +826,14 @@ int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages)
> return 0;
> }
>
> +int kvm_hv_synic_enable_eom(struct kvm_vcpu *vcpu)
> +{
> + struct kvm_vcpu_hv_synic *synic = vcpu_to_synic(vcpu);
> +
> + synic->enable_eom_exit = true;
> + return 0;
> +}
> +
> static bool kvm_hv_msr_partition_wide(u32 msr)
> {
> bool r = false;
> diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
> index 757cb578101c..ff89f0ff103c 100644
> --- a/arch/x86/kvm/hyperv.h
> +++ b/arch/x86/kvm/hyperv.h
> @@ -56,6 +56,7 @@ void kvm_hv_irq_routing_update(struct kvm *kvm);
> int kvm_hv_synic_set_irq(struct kvm *kvm, u32 vcpu_id, u32 sint);
> void kvm_hv_synic_send_eoi(struct kvm_vcpu *vcpu, int vector);
> int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages);
> +int kvm_hv_synic_enable_eom(struct kvm_vcpu *vcpu);
>
> void kvm_hv_vcpu_init(struct kvm_vcpu *vcpu);
> void kvm_hv_vcpu_postcreate(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 027dfd278a97..0def4ab31dc1 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3350,6 +3350,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> case KVM_CAP_HYPERV_SPIN:
> case KVM_CAP_HYPERV_SYNIC:
> case KVM_CAP_HYPERV_SYNIC2:
> + case KVM_CAP_HYPERV_SYNIC_EOM:
> case KVM_CAP_HYPERV_VP_INDEX:
> case KVM_CAP_HYPERV_EVENTFD:
> case KVM_CAP_HYPERV_TLBFLUSH:
> @@ -4209,6 +4210,10 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
> return -EINVAL;
>
> switch (cap->cap) {
> + case KVM_CAP_HYPERV_SYNIC_EOM:
> + kvm_hv_synic_enable_eom(vcpu);
> + return 0;
> +
> case KVM_CAP_HYPERV_SYNIC2:
> if (cap->args[0])
> return -EINVAL;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 428c7dde6b4b..78172ad156d8 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1017,6 +1017,7 @@ struct kvm_ppc_resize_hpt {
> #define KVM_CAP_S390_VCPU_RESETS 179
> #define KVM_CAP_S390_PROTECTED 180
> #define KVM_CAP_PPC_SECURE_GUEST 181
> +#define KVM_CAP_HYPERV_SYNIC_EOM 182
>
> #ifdef KVM_CAP_IRQ_ROUTING
--
Vitaly
^ permalink raw reply
* [PATCH v2 0/1] x86/kvm/hyper-v: Add support to SYNIC exit on EOM
From: Jon Doron @ 2020-04-16 8:38 UTC (permalink / raw)
To: kvm, linux-hyperv; +Cc: vkuznets, rvkagan, Jon Doron
According to the TLFS:
"A write to the end of message (EOM) register by the guest causes the
hypervisor to scan the internal message buffer queue(s) associated with
the virtual processor.
If a message buffer queue contains a queued message buffer, the hypervisor
attempts to deliver the message.
Message delivery succeeds if the SIM page is enabled and the message slot
corresponding to the SINTx is empty (that is, the message type in the
header is set to HvMessageTypeNone).
If a message is successfully delivered, its corresponding internal message
buffer is dequeued and marked free.
If the corresponding SINTx is not masked, an edge-triggered interrupt is
delivered (that is, the corresponding bit in the IRR is set).
This register can be used by guests to poll for messages. It can also be
used as a way to drain the message queue for a SINTx that has
been disabled (that is, masked)."
So basically this means that we need to exit on EOM so the hypervisor
will have a chance to send all the pending messages regardless of the
SCONTROL mechnaisim.
v2:
Minor fixes from code review
Jon Doron (1):
x86/kvm/hyper-v: Add support to SYNIC exit on EOM
arch/x86/include/asm/kvm_host.h | 1 +
arch/x86/kvm/hyperv.c | 67 +++++++++++++++++++++++++++++----
arch/x86/kvm/hyperv.h | 1 +
arch/x86/kvm/x86.c | 5 +++
include/uapi/linux/kvm.h | 1 +
5 files changed, 67 insertions(+), 8 deletions(-)
--
2.24.1
^ permalink raw reply
* [PATCH v2 1/1] x86/kvm/hyper-v: Add support to SYNIC exit on EOM
From: Jon Doron @ 2020-04-16 8:38 UTC (permalink / raw)
To: kvm, linux-hyperv; +Cc: vkuznets, rvkagan, Jon Doron
In-Reply-To: <20200416083847.1776387-1-arilou@gmail.com>
According to the TLFS a write to the EOM register by the guest
causes the hypervisor to scan for any pending messages and if there
are any it will try to deliver them.
To do this we must exit so any pending messages can be written.
Signed-off-by: Jon Doron <arilou@gmail.com>
---
arch/x86/include/asm/kvm_host.h | 1 +
arch/x86/kvm/hyperv.c | 67 +++++++++++++++++++++++++++++----
arch/x86/kvm/hyperv.h | 1 +
arch/x86/kvm/x86.c | 5 +++
include/uapi/linux/kvm.h | 1 +
5 files changed, 67 insertions(+), 8 deletions(-)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 42a2d0d3984a..048a1db488e2 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -548,6 +548,7 @@ struct kvm_vcpu_hv_synic {
DECLARE_BITMAP(vec_bitmap, 256);
bool active;
bool dont_zero_synic_pages;
+ bool enable_eom_exit;
};
/* Hyper-V per vcpu emulation context */
diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
index bcefa9d4e57e..4bf35452ae5c 100644
--- a/arch/x86/kvm/hyperv.c
+++ b/arch/x86/kvm/hyperv.c
@@ -186,6 +186,51 @@ static void kvm_hv_notify_acked_sint(struct kvm_vcpu *vcpu, u32 sint)
srcu_read_unlock(&kvm->irq_srcu, idx);
}
+static int synic_read_msg_hdr(struct kvm_vcpu_hv_synic *synic, u32 sint,
+ struct hv_message_header *msg)
+{
+ struct kvm_vcpu *vcpu = synic_to_vcpu(synic);
+ int msg_off = offsetof(struct hv_message_page, sint_message[sint]);
+ gfn_t msg_page_gfn;
+ int r;
+
+ if (!(synic->msg_page & HV_SYNIC_SIMP_ENABLE))
+ return -ENOENT;
+
+ msg_page_gfn = synic->msg_page >> PAGE_SHIFT;
+
+ r = kvm_vcpu_read_guest_page(vcpu, msg_page_gfn, msg, msg_off,
+ sizeof(*msg));
+ if (r < 0)
+ return r;
+
+ return 0;
+}
+
+static bool synic_should_exit_on_eom(struct kvm_vcpu_hv_synic *synic)
+{
+ int i;
+
+ if (!synic->enable_eom_exit)
+ return false;
+
+ for (i = 0; i < ARRAY_SIZE(synic->sint); i++) {
+ struct hv_message_header hv_hdr;
+ /*
+ * If we failed to read from the msg slot then we treat this
+ * msg slot as free
+ */
+ if (synic_read_msg_hdr(synic, i, &hv_hdr) < 0)
+ continue;
+
+ /* See if this msg slot has a pending message */
+ if (hv_hdr.message_flags.msg_pending == 1)
+ return true;
+ }
+
+ return false;
+}
+
static void synic_exit(struct kvm_vcpu_hv_synic *synic, u32 msr)
{
struct kvm_vcpu *vcpu = synic_to_vcpu(synic);
@@ -254,6 +299,9 @@ static int synic_set_msr(struct kvm_vcpu_hv_synic *synic,
for (i = 0; i < ARRAY_SIZE(synic->sint); i++)
kvm_hv_notify_acked_sint(vcpu, i);
+
+ if (!host && synic_should_exit_on_eom(synic))
+ synic_exit(synic, msr);
break;
}
case HV_X64_MSR_SINT0 ... HV_X64_MSR_SINT15:
@@ -571,8 +619,9 @@ static int synic_deliver_msg(struct kvm_vcpu_hv_synic *synic, u32 sint,
struct hv_message_header hv_hdr;
int r;
- if (!(synic->msg_page & HV_SYNIC_SIMP_ENABLE))
- return -ENOENT;
+ r = synic_read_msg_hdr(synic, sint, &hv_hdr);
+ if (r < 0)
+ return r;
msg_page_gfn = synic->msg_page >> PAGE_SHIFT;
@@ -582,12 +631,6 @@ static int synic_deliver_msg(struct kvm_vcpu_hv_synic *synic, u32 sint,
* is only called in vcpu context so the entire update is atomic from
* guest POV and thus the exact order here doesn't matter.
*/
- r = kvm_vcpu_read_guest_page(vcpu, msg_page_gfn, &hv_hdr.message_type,
- msg_off + offsetof(struct hv_message,
- header.message_type),
- sizeof(hv_hdr.message_type));
- if (r < 0)
- return r;
if (hv_hdr.message_type != HVMSG_NONE) {
if (no_retry)
@@ -785,6 +828,14 @@ int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages)
return 0;
}
+int kvm_hv_synic_enable_eom(struct kvm_vcpu *vcpu)
+{
+ struct kvm_vcpu_hv_synic *synic = vcpu_to_synic(vcpu);
+
+ synic->enable_eom_exit = true;
+ return 0;
+}
+
static bool kvm_hv_msr_partition_wide(u32 msr)
{
bool r = false;
diff --git a/arch/x86/kvm/hyperv.h b/arch/x86/kvm/hyperv.h
index 757cb578101c..ff89f0ff103c 100644
--- a/arch/x86/kvm/hyperv.h
+++ b/arch/x86/kvm/hyperv.h
@@ -56,6 +56,7 @@ void kvm_hv_irq_routing_update(struct kvm *kvm);
int kvm_hv_synic_set_irq(struct kvm *kvm, u32 vcpu_id, u32 sint);
void kvm_hv_synic_send_eoi(struct kvm_vcpu *vcpu, int vector);
int kvm_hv_activate_synic(struct kvm_vcpu *vcpu, bool dont_zero_synic_pages);
+int kvm_hv_synic_enable_eom(struct kvm_vcpu *vcpu);
void kvm_hv_vcpu_init(struct kvm_vcpu *vcpu);
void kvm_hv_vcpu_postcreate(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 3bf2ecafd027..1615be238806 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3350,6 +3350,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
case KVM_CAP_HYPERV_SPIN:
case KVM_CAP_HYPERV_SYNIC:
case KVM_CAP_HYPERV_SYNIC2:
+ case KVM_CAP_HYPERV_SYNIC_EOM:
case KVM_CAP_HYPERV_VP_INDEX:
case KVM_CAP_HYPERV_EVENTFD:
case KVM_CAP_HYPERV_TLBFLUSH:
@@ -4209,6 +4210,10 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
return -EINVAL;
switch (cap->cap) {
+ case KVM_CAP_HYPERV_SYNIC_EOM:
+ kvm_hv_synic_enable_eom(vcpu);
+ return 0;
+
case KVM_CAP_HYPERV_SYNIC2:
if (cap->args[0])
return -EINVAL;
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 428c7dde6b4b..78172ad156d8 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1017,6 +1017,7 @@ struct kvm_ppc_resize_hpt {
#define KVM_CAP_S390_VCPU_RESETS 179
#define KVM_CAP_S390_PROTECTED 180
#define KVM_CAP_PPC_SECURE_GUEST 181
+#define KVM_CAP_HYPERV_SYNIC_EOM 182
#ifdef KVM_CAP_IRQ_ROUTING
--
2.24.1
^ permalink raw reply related
* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
From: Dan Carpenter @ 2020-04-16 10:02 UTC (permalink / raw)
To: Wei Liu
Cc: Vitaly Kuznetsov, Colin Ian King, kernel-janitors, linux-kernel,
K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger,
linux-hyperv
In-Reply-To: <20200415143752.cm3xbesiuksfdbzm@debian>
We have this discussion over and over. I always say it helps to have
the commit mentioned in the commit message but it's not a Fixes tag.
So I think that the commit message should say something like
"commit 1234234 ("blah blah") removed some code so this variable isn't
used any more". I think it helps the review process. But then if we
mention the commit everyone says to use the Fixes tag.
It turns out if you leave out the commit entirely then people still
complain but a lot less frequently. It shouldn't work that way but
reviewers are illogical.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH][next] drivers: hv: remove redundant assignment to pointer primary_channel
From: Colin Ian King @ 2020-04-16 10:08 UTC (permalink / raw)
To: Dan Carpenter, Wei Liu
Cc: Vitaly Kuznetsov, kernel-janitors, linux-kernel,
K . Y . Srinivasan, Haiyang Zhang, Stephen Hemminger,
linux-hyperv
In-Reply-To: <20200416100253.GN1163@kadam>
On 16/04/2020 11:02, Dan Carpenter wrote:
> We have this discussion over and over. I always say it helps to have
> the commit mentioned in the commit message but it's not a Fixes tag.
> So I think that the commit message should say something like
> "commit 1234234 ("blah blah") removed some code so this variable isn't
> used any more". I think it helps the review process. But then if we
> mention the commit everyone says to use the Fixes tag.
>
> It turns out if you leave out the commit entirely then people still
> complain but a lot less frequently. It shouldn't work that way but
> reviewers are illogical.
Yep, to my knowledge the policy for this kind of commit is not described
in any documentation, so it's ambiguous. Either way, one can't win
without it being properly codified.
Colin
>
> regards,
> dan carpenter
>
^ permalink raw reply
* Re: [PATCH v10 7/7] KVM: selftests: update hyperv_cpuid with SynDBG tests
From: Jon Doron @ 2020-04-16 11:05 UTC (permalink / raw)
To: Vitaly Kuznetsov; +Cc: kvm, linux-hyperv
In-Reply-To: <87d09286mx.fsf@vitty.brq.redhat.com>
Did this patchset make it into the merge window?
Thanks,
-- Jon.
On 24/03/2020, Vitaly Kuznetsov wrote:
>Jon Doron <arilou@gmail.com> writes:
>
>> From: Vitaly Kuznetsov <vkuznets () redhat ! com>
>>
>> Test all four combinations with eVMCS and SynDBG capabilities,
>> check that we get the right number of entries and that
>> 0x40000000.EAX always returns the correct max leaf.
>>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>
>Something weird happened to this patch. It fails to apply on kvm/queue
>but it's also a bit different from what I've sent yesterday. I fixed and
>tested it, please find the result attached.
>
>--
>Vitaly
>
>From ae2c688389c20a99d7457861e57ce97054780908 Mon Sep 17 00:00:00 2001
>From: Vitaly Kuznetsov <vkuznets () redhat ! com>
>Date: Tue, 24 Mar 2020 09:43:41 +0200
>Subject: [PATCH v10 7/7 FIXED] KVM: selftests: update hyperv_cpuid with SynDBG
> tests
>
>Test all four combinations with eVMCS and SynDBG capabilities,
>check that we get the right number of entries and that
>0x40000000.EAX always returns the correct max leaf.
>
>Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>---
> .../selftests/kvm/x86_64/hyperv_cpuid.c | 143 ++++++++++++------
> 1 file changed, 95 insertions(+), 48 deletions(-)
>
>diff --git a/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c b/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c
>index 83323f3d7ca0..5268abf9ad80 100644
>--- a/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c
>+++ b/tools/testing/selftests/kvm/x86_64/hyperv_cpuid.c
>@@ -26,18 +26,18 @@ static void guest_code(void)
> {
> }
>
>-static int smt_possible(void)
>+static bool smt_possible(void)
> {
> char buf[16];
> FILE *f;
>- bool res = 1;
>+ bool res = true;
>
> f = fopen("/sys/devices/system/cpu/smt/control", "r");
> if (f) {
> if (fread(buf, sizeof(*buf), sizeof(buf), f) > 0) {
> if (!strncmp(buf, "forceoff", 8) ||
> !strncmp(buf, "notsupported", 12))
>- res = 0;
>+ res = false;
> }
> fclose(f);
> }
>@@ -45,30 +45,48 @@ static int smt_possible(void)
> return res;
> }
>
>+void vcpu_enable_syndbg(struct kvm_vm *vm, int vcpu_id)
>+{
>+ struct kvm_enable_cap enable_syndbg_cap = {
>+ .cap = KVM_CAP_HYPERV_SYNDBG,
>+ };
>+
>+ vcpu_ioctl(vm, vcpu_id, KVM_ENABLE_CAP, &enable_syndbg_cap);
>+}
>+
> static void test_hv_cpuid(struct kvm_cpuid2 *hv_cpuid_entries,
>- int evmcs_enabled)
>+ bool evmcs_enabled, bool syndbg_enabled)
> {
> int i;
>+ int nent = 6;
>+ u32 test_val;
>+
>+ if (evmcs_enabled)
>+ nent += 1; /* 0x4000000A */
>
>- if (!evmcs_enabled)
>- TEST_ASSERT(hv_cpuid_entries->nent == 6,
>- "KVM_GET_SUPPORTED_HV_CPUID should return 6 entries"
>- " when Enlightened VMCS is disabled (returned %d)",
>- hv_cpuid_entries->nent);
>- else
>- TEST_ASSERT(hv_cpuid_entries->nent == 7,
>- "KVM_GET_SUPPORTED_HV_CPUID should return 7 entries"
>- " when Enlightened VMCS is enabled (returned %d)",
>- hv_cpuid_entries->nent);
>+ if (syndbg_enabled)
>+ nent += 3; /* 0x40000080 - 0x40000082 */
>+
>+ TEST_ASSERT(hv_cpuid_entries->nent == nent,
>+ "KVM_GET_SUPPORTED_HV_CPUID should return %d entries"
>+ " with evmcs=%d syndbg=%d (returned %d)",
>+ nent, evmcs_enabled, syndbg_enabled,
>+ hv_cpuid_entries->nent);
>
> for (i = 0; i < hv_cpuid_entries->nent; i++) {
> struct kvm_cpuid_entry2 *entry = &hv_cpuid_entries->entries[i];
>
> TEST_ASSERT((entry->function >= 0x40000000) &&
>- (entry->function <= 0x4000000A),
>+ (entry->function <= 0x40000082),
> "function %x is our of supported range",
> entry->function);
>
>+ TEST_ASSERT(evmcs_enabled || (entry->function != 0x4000000A),
>+ "0x4000000A leaf should not be reported");
>+
>+ TEST_ASSERT(syndbg_enabled || (entry->function <= 0x4000000A),
>+ "SYNDBG leaves should not be reported");
>+
> TEST_ASSERT(entry->index == 0,
> ".index field should be zero");
>
>@@ -78,12 +96,27 @@ static void test_hv_cpuid(struct kvm_cpuid2 *hv_cpuid_entries,
> TEST_ASSERT(!entry->padding[0] && !entry->padding[1] &&
> !entry->padding[2], "padding should be zero");
>
>- if (entry->function == 0x40000004) {
>- int nononarchcs = !!(entry->eax & (1UL << 18));
>-
>- TEST_ASSERT(nononarchcs == !smt_possible(),
>+ switch (entry->function) {
>+ case 0x40000000:
>+ test_val = 0x40000005;
>+ if (evmcs_enabled)
>+ test_val = 0x4000000A;
>+ if (syndbg_enabled)
>+ test_val = 0x40000082;
>+
>+ TEST_ASSERT(entry->eax == test_val,
>+ "Wrong max leaf report in 0x40000000.EAX: %x"
>+ " (evmcs=%d syndbg=%d)",
>+ entry->eax, evmcs_enabled, syndbg_enabled
>+ );
>+ break;
>+ case 0x40000004:
>+ test_val = entry->eax & (1UL << 18);
>+
>+ TEST_ASSERT(!!test_val == !smt_possible(),
> "NoNonArchitecturalCoreSharing bit"
> " doesn't reflect SMT setting");
>+ break;
> }
>
> /*
>@@ -133,8 +166,9 @@ struct kvm_cpuid2 *kvm_get_supported_hv_cpuid(struct kvm_vm *vm)
> int main(int argc, char *argv[])
> {
> struct kvm_vm *vm;
>- int rv;
>+ int rv, stage;
> struct kvm_cpuid2 *hv_cpuid_entries;
>+ bool evmcs_enabled, syndbg_enabled;
>
> /* Tell stdout not to buffer its content */
> setbuf(stdout, NULL);
>@@ -145,36 +179,49 @@ int main(int argc, char *argv[])
> exit(KSFT_SKIP);
> }
>
>- /* Create VM */
>- vm = vm_create_default(VCPU_ID, 0, guest_code);
>-
>- test_hv_cpuid_e2big(vm);
>-
>- hv_cpuid_entries = kvm_get_supported_hv_cpuid(vm);
>- if (!hv_cpuid_entries)
>- return 1;
>-
>- test_hv_cpuid(hv_cpuid_entries, 0);
>-
>- free(hv_cpuid_entries);
>+ for (stage = 0; stage < 5; stage++) {
>+ evmcs_enabled = false;
>+ syndbg_enabled = false;
>+
>+ vm = vm_create_default(VCPU_ID, 0, guest_code);
>+ switch (stage) {
>+ case 0:
>+ test_hv_cpuid_e2big(vm);
>+ continue;
>+ case 1:
>+ break;
>+ case 2:
>+ if (!kvm_check_cap(KVM_CAP_HYPERV_ENLIGHTENED_VMCS)) {
>+ print_skip("Enlightened VMCS is unsupported");
>+ continue;
>+ }
>+ vcpu_enable_evmcs(vm, VCPU_ID);
>+ evmcs_enabled = true;
>+ break;
>+ case 3:
>+ if (!kvm_check_cap(KVM_CAP_HYPERV_SYNDBG)) {
>+ print_skip("Synthetic debugger is unsupported");
>+ continue;
>+ }
>+ vcpu_enable_syndbg(vm, VCPU_ID);
>+ syndbg_enabled = true;
>+ break;
>+ case 4:
>+ if (!kvm_check_cap(KVM_CAP_HYPERV_ENLIGHTENED_VMCS) ||
>+ !kvm_check_cap(KVM_CAP_HYPERV_SYNDBG))
>+ continue;
>+ vcpu_enable_evmcs(vm, VCPU_ID);
>+ vcpu_enable_syndbg(vm, VCPU_ID);
>+ evmcs_enabled = true;
>+ syndbg_enabled = true;
>+ break;
>+ }
>
>- if (!kvm_check_cap(KVM_CAP_HYPERV_ENLIGHTENED_VMCS)) {
>- print_skip("Enlightened VMCS is unsupported");
>- goto vm_free;
>+ hv_cpuid_entries = kvm_get_supported_hv_cpuid(vm);
>+ test_hv_cpuid(hv_cpuid_entries, evmcs_enabled, syndbg_enabled);
>+ free(hv_cpuid_entries);
>+ kvm_vm_free(vm);
> }
>
>- vcpu_enable_evmcs(vm, VCPU_ID);
>-
>- hv_cpuid_entries = kvm_get_supported_hv_cpuid(vm);
>- if (!hv_cpuid_entries)
>- return 1;
>-
>- test_hv_cpuid(hv_cpuid_entries, 1);
>-
>- free(hv_cpuid_entries);
>-
>-vm_free:
>- kvm_vm_free(vm);
>-
> return 0;
> }
>--
>2.25.1
>
^ permalink raw reply
* Re: [PATCH v2 0/1] x86/kvm/hyper-v: Add support to SYNIC exit on EOM
From: Roman Kagan @ 2020-04-16 12:00 UTC (permalink / raw)
To: Jon Doron; +Cc: kvm, linux-hyperv, vkuznets
In-Reply-To: <20200416083847.1776387-1-arilou@gmail.com>
On Thu, Apr 16, 2020 at 11:38:46AM +0300, Jon Doron wrote:
> According to the TLFS:
> "A write to the end of message (EOM) register by the guest causes the
> hypervisor to scan the internal message buffer queue(s) associated with
> the virtual processor.
>
> If a message buffer queue contains a queued message buffer, the hypervisor
> attempts to deliver the message.
>
> Message delivery succeeds if the SIM page is enabled and the message slot
> corresponding to the SINTx is empty (that is, the message type in the
> header is set to HvMessageTypeNone).
> If a message is successfully delivered, its corresponding internal message
> buffer is dequeued and marked free.
> If the corresponding SINTx is not masked, an edge-triggered interrupt is
> delivered (that is, the corresponding bit in the IRR is set).
>
> This register can be used by guests to poll for messages. It can also be
> used as a way to drain the message queue for a SINTx that has
> been disabled (that is, masked)."
Doesn't this work already?
> So basically this means that we need to exit on EOM so the hypervisor
> will have a chance to send all the pending messages regardless of the
> SCONTROL mechnaisim.
I might be misinterpreting the spec, but my understanding is that
SCONTROL {en,dis}ables the message queueing completely. What the quoted
part means is that a write to EOM should trigger the message source to
push a new message into the slot, regardless of whether the SINT was
masked or not.
And this (I think, haven't tested) should already work. The userspace
just keeps using the SINT route as it normally does, posting
notifications to the corresponding irqfd when posting a message, and
waiting on the resamplerfd for the message slot to become free. If the
SINT is masked KVM will skip injecting the interrupt, that's it.
Roman.
^ permalink raw reply
* Re: [PATCH v2 0/1] x86/kvm/hyper-v: Add support to SYNIC exit on EOM
From: Jon Doron @ 2020-04-16 12:54 UTC (permalink / raw)
To: Roman Kagan, kvm, linux-hyperv, vkuznets
In-Reply-To: <20200416120040.GA3745197@rvkaganb>
On 16/04/2020, Roman Kagan wrote:
>On Thu, Apr 16, 2020 at 11:38:46AM +0300, Jon Doron wrote:
>> According to the TLFS:
>> "A write to the end of message (EOM) register by the guest causes the
>> hypervisor to scan the internal message buffer queue(s) associated with
>> the virtual processor.
>>
>> If a message buffer queue contains a queued message buffer, the hypervisor
>> attempts to deliver the message.
>>
>> Message delivery succeeds if the SIM page is enabled and the message slot
>> corresponding to the SINTx is empty (that is, the message type in the
>> header is set to HvMessageTypeNone).
>> If a message is successfully delivered, its corresponding internal message
>> buffer is dequeued and marked free.
>> If the corresponding SINTx is not masked, an edge-triggered interrupt is
>> delivered (that is, the corresponding bit in the IRR is set).
>>
>> This register can be used by guests to poll for messages. It can also be
>> used as a way to drain the message queue for a SINTx that has
>> been disabled (that is, masked)."
>
>Doesn't this work already?
>
Well if you dont have SCONTROL and a GSI associated with the SINT then
it does not...
>> So basically this means that we need to exit on EOM so the hypervisor
>> will have a chance to send all the pending messages regardless of the
>> SCONTROL mechnaisim.
>
>I might be misinterpreting the spec, but my understanding is that
>SCONTROL {en,dis}ables the message queueing completely. What the quoted
>part means is that a write to EOM should trigger the message source to
>push a new message into the slot, regardless of whether the SINT was
>masked or not.
>
>And this (I think, haven't tested) should already work. The userspace
>just keeps using the SINT route as it normally does, posting
>notifications to the corresponding irqfd when posting a message, and
>waiting on the resamplerfd for the message slot to become free. If the
>SINT is masked KVM will skip injecting the interrupt, that's it.
>
>Roman.
That's what I was thinking originally as well, but then i noticed KDNET
as a VMBus client (and it basically runs before anything else) is
working in this polling mode, where SCONTROL is disabled and it just
loops, and if it saw there is a PENDING message flag it will issue an
EOM to indicate it has free the slot.
(There are a bunch of patches i sent on the QEMU mailing list as well
where i CCed you, I will probably revise it a bit but was hoping to get
KVM sorted out first).
Cheers,
-- Jon.
^ permalink raw reply
* Re: [PATCH v10 7/7] KVM: selftests: update hyperv_cpuid with SynDBG tests
From: Vitaly Kuznetsov @ 2020-04-16 13:19 UTC (permalink / raw)
To: Jon Doron, Paolo Bonzini; +Cc: kvm, linux-hyperv
In-Reply-To: <20200416110540.GK7606@jondnuc>
Jon Doron <arilou@gmail.com> writes:
> Did this patchset make it into the merge window?
>
My crystal ball tells me we should ask Paolo (To:) ;-)
--
Vitaly
^ permalink raw reply
* Re: [PATCH 10/28] mm: only allow page table mappings for built-in zsmalloc
From: Minchan Kim @ 2020-04-16 20:37 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Sergey Senozhatsky, Peter Zijlstra, Andrew Morton,
K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu, x86,
David Airlie, Daniel Vetter, Laura Abbott, Sumit Semwal,
Sakari Ailus, Nitin Gupta, Robin Murphy, Christophe Leroy,
linuxppc-dev, linux-hyperv, dri-devel, linaro-mm-sig, linux-arch,
linux-mm, iommu, linux-arm-kernel, linux-s390, bpf, linux-kernel
In-Reply-To: <20200411072052.GA31242@lst.de>
Hi Christoph,
Sorry for the late.
On Sat, Apr 11, 2020 at 09:20:52AM +0200, Christoph Hellwig wrote:
> Hi Minchan,
>
> On Fri, Apr 10, 2020 at 04:11:36PM -0700, Minchan Kim wrote:
> > It doesn't mean we couldn't use zsmalloc as module any longer. It means
> > we couldn't use zsmalloc as module with pgtable mapping whcih was little
> > bit faster on microbenchmark in some architecutre(However, I usually temped
> > to remove it since it had several problems). However, we could still use
> > zsmalloc as module as copy way instead of pgtable mapping. Thus, if someone
> > really want to rollback the feature, they should provide reasonable reason
> > why it doesn't work for them. "A little fast" wouldn't be enough to exports
> > deep internal to the module.
>
> do you have any data how much faster it is on arm (and does that include
> arm64 as well)? Besides the exports which were my prime concern,
https://github.com/sjenning/zsmapbench
I need to recall the memory. IIRC, it was almost 30% faster at that time
in ARM so was not trivial at that time. However, it was story from
several years ago.
> zsmalloc with pgtable mappings also is the only user of map_kernel_range
> outside of vmalloc.c, if it really is another code base for tiny
> improvements we could mark map_kernel_range or in fact remove it entirely
> and open code it in the remaining callers.
I alsh have temped to remove it. Let me have time to revist it in this
chance.
Thanks.
^ permalink raw reply
* Re: [PATCH 11/29] mm: only allow page table mappings for built-in zsmalloc
From: Minchan Kim @ 2020-04-16 20:43 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Andrew Morton, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Wei Liu, x86, David Airlie, Daniel Vetter, Laura Abbott,
Sumit Semwal, Sakari Ailus, Nitin Gupta, Robin Murphy,
Christophe Leroy, Peter Zijlstra, linuxppc-dev, linux-hyperv,
dri-devel, linaro-mm-sig, linux-arch, linux-mm, iommu,
linux-arm-kernel, linux-s390, bpf, linux-kernel,
sergey.senozhatsky.work
In-Reply-To: <20200414131348.444715-12-hch@lst.de>
On Tue, Apr 14, 2020 at 03:13:30PM +0200, Christoph Hellwig wrote:
> This allows to unexport map_vm_area and unmap_kernel_range, which are
> rather deep internal and should not be available to modules, as they for
> example allow fine grained control of mapping permissions, and also
> allow splitting the setup of a vmalloc area and the actual mapping and
> thus expose vmalloc internals.
>
> zsmalloc is typically built-in and continues to work (just like the
> percpu-vm code using a similar patter), while modular zsmalloc also
> continues to work, but must use copies.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Minchan Kim <minchan@kernel.org>
Thanks!
^ permalink raw reply
* [PATCH] x86/hyperv: Suspend/resume the VP assist page for hibernation
From: Dexuan Cui @ 2020-04-17 6:29 UTC (permalink / raw)
To: bp, haiyangz, hpa, kys, linux-hyperv, linux-kernel, mingo,
sthemmin, tglx, x86, mikelley, vkuznets, wei.liu
Cc: Dexuan Cui
Unlike the other CPUs, CPU0 is never offlined during hibernation. So in the
resume path, the "new" kernel's VP assist page is not suspended (i.e.
disabled), and later when we jump to the "old" kernel, the page is not
properly re-enabled for CPU0 with the allocated page from the old kernel.
So far, the VP assist page is only used by hv_apic_eoi_write(). When the
page is not properly re-enabled, hvp->apic_assist is always 0, so the
HV_X64_MSR_EOI MSR is always written. This is not ideal with respect to
performance, but Hyper-V can still correctly handle this.
The issue is: the hypervisor can corrupt the old kernel memory, and hence
sometimes cause unexpected behaviors, e.g. when the old kernel's non-boot
CPUs are being onlined in the resume path, the VM can hang or be killed
due to virtual triple fault.
Fix the issue by calling hv_cpu_die()/hv_cpu_init() in the syscore ops.
Without the fix, hibernation can fail at a rate of 1/300 ~ 1/500.
With the fix, hibernation can pass a long-haul test of 2000 rounds.
Fixes: 05bd330a7fd8 ("x86/hyperv: Suspend/resume the hypercall page for hibernation")
Cc: stable@vger.kernel.org
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
arch/x86/hyperv/hv_init.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index b0da5320bcff..4d3ce86331a3 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -72,7 +72,8 @@ static int hv_cpu_init(unsigned int cpu)
struct page *pg;
input_arg = (void **)this_cpu_ptr(hyperv_pcpu_input_arg);
- pg = alloc_page(GFP_KERNEL);
+ /* hv_cpu_init() can be called with IRQs disabled from hv_resume() */
+ pg = alloc_page(GFP_ATOMIC);
if (unlikely(!pg))
return -ENOMEM;
*input_arg = page_address(pg);
@@ -253,6 +254,7 @@ static int __init hv_pci_init(void)
static int hv_suspend(void)
{
union hv_x64_msr_hypercall_contents hypercall_msr;
+ int ret;
/*
* Reset the hypercall page as it is going to be invalidated
@@ -269,12 +271,17 @@ static int hv_suspend(void)
hypercall_msr.enable = 0;
wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
- return 0;
+ ret = hv_cpu_die(0);
+ return ret;
}
static void hv_resume(void)
{
union hv_x64_msr_hypercall_contents hypercall_msr;
+ int ret;
+
+ ret = hv_cpu_init(0);
+ WARN_ON(ret);
/* Re-enable the hypercall page */
rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
@@ -287,6 +294,7 @@ static void hv_resume(void)
hv_hypercall_pg_saved = NULL;
}
+/* Note: when the ops are called, only CPU0 is online and IRQs are disabled. */
static struct syscore_ops hv_syscore_ops = {
.suspend = hv_suspend,
.resume = hv_resume,
--
2.19.1
^ permalink raw reply related
* Re: [PATCH] x86/hyperv: Suspend/resume the VP assist page for hibernation
From: Wei Liu @ 2020-04-17 9:07 UTC (permalink / raw)
To: Dexuan Cui
Cc: bp, haiyangz, hpa, kys, linux-hyperv, linux-kernel, mingo,
sthemmin, tglx, x86, mikelley, vkuznets, wei.liu
In-Reply-To: <1587104999-28927-1-git-send-email-decui@microsoft.com>
On Thu, Apr 16, 2020 at 11:29:59PM -0700, Dexuan Cui wrote:
> Unlike the other CPUs, CPU0 is never offlined during hibernation. So in the
> resume path, the "new" kernel's VP assist page is not suspended (i.e.
> disabled), and later when we jump to the "old" kernel, the page is not
> properly re-enabled for CPU0 with the allocated page from the old kernel.
>
> So far, the VP assist page is only used by hv_apic_eoi_write(). When the
> page is not properly re-enabled, hvp->apic_assist is always 0, so the
> HV_X64_MSR_EOI MSR is always written. This is not ideal with respect to
> performance, but Hyper-V can still correctly handle this.
>
> The issue is: the hypervisor can corrupt the old kernel memory, and hence
> sometimes cause unexpected behaviors, e.g. when the old kernel's non-boot
> CPUs are being onlined in the resume path, the VM can hang or be killed
> due to virtual triple fault.
>
> Fix the issue by calling hv_cpu_die()/hv_cpu_init() in the syscore ops.
>
> Without the fix, hibernation can fail at a rate of 1/300 ~ 1/500.
> With the fix, hibernation can pass a long-haul test of 2000 rounds.
>
> Fixes: 05bd330a7fd8 ("x86/hyperv: Suspend/resume the hypercall page for hibernation")
> Cc: stable@vger.kernel.org
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> arch/x86/hyperv/hv_init.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index b0da5320bcff..4d3ce86331a3 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -72,7 +72,8 @@ static int hv_cpu_init(unsigned int cpu)
> struct page *pg;
>
> input_arg = (void **)this_cpu_ptr(hyperv_pcpu_input_arg);
> - pg = alloc_page(GFP_KERNEL);
> + /* hv_cpu_init() can be called with IRQs disabled from hv_resume() */
> + pg = alloc_page(GFP_ATOMIC);
IMHO it would be better to only tap into the reserve pool if so
required, e.g.
pg = alloc_page(irqs_disabled() ? GFP_ATOMIC : GFP_KERNEL);
Wei.
^ permalink raw reply
* Re: [PATCH] x86/hyperv: Suspend/resume the VP assist page for hibernation
From: Vitaly Kuznetsov @ 2020-04-17 10:03 UTC (permalink / raw)
To: Dexuan Cui
Cc: bp, haiyangz, hpa, kys, linux-hyperv, linux-kernel, mingo,
sthemmin, tglx, x86, mikelley, wei.liu, kvm
In-Reply-To: <1587104999-28927-1-git-send-email-decui@microsoft.com>
Dexuan Cui <decui@microsoft.com> writes:
> Unlike the other CPUs, CPU0 is never offlined during hibernation. So in the
> resume path, the "new" kernel's VP assist page is not suspended (i.e.
> disabled), and later when we jump to the "old" kernel, the page is not
> properly re-enabled for CPU0 with the allocated page from the old kernel.
>
> So far, the VP assist page is only used by hv_apic_eoi_write().
No, not only for that ('git grep hv_get_vp_assist_page')
KVM on Hyper-V also needs VP assist page to use Enlightened VMCS. In
particular, Enlightened VMPTR is written there.
This makes me wonder: how does hibernation work with KVM in case we use
Enlightened VMCS and we have VMs running? We need to make sure VP Assist
page content is preserved.
--
Vitaly
^ 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