* Re: [PATCH] hyper-v: Check for ring buffer in hv_get_bytes_to_read/write
From: Mohammed Gamal @ 2019-03-14 12:42 UTC (permalink / raw)
To: Stephen Hemminger, Haiyang Zhang, Michael Kelley,
linux-hyperv@vger.kernel.org, kimbrownkd
Cc: Sasha Levin, Dexuan Cui, Long Li, KY Srinivasan, vkuznets
In-Reply-To: <SN6PR2101MB0912DF97AC683F8DAD5B59FDCC4A0@SN6PR2101MB0912.namprd21.prod.outlook.com>
On Wed, 2019-03-13 at 21:12 +0000, Stephen Hemminger wrote:
> What test are you running?
I am running iperf3 with the following arguments:
iperf3 -u -c ${iperf3 server address} -b 0 -P8 -t 3600
while changing the interface parameters in parallel with the following
script:
cat ./test.sh
#!/bin/bash
device="eth1"
i=0
while [ "$i" -lt 1000 ]
do
ethtool -L $device combined 1
ethtool -L $device combined 2
let "i++"
echo $i
done
>
> -----Original Message-----
> From: Mohammed Gamal <mgamal@redhat.com>
> Sent: Wednesday, March 13, 2019 3:25 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>; Michael Kelley <mikelley@
> microsoft.com>; linux-hyperv@vger.kernel.org; kimbrownkd <kimbrownkd@
> gmail.com>
> Cc: Sasha Levin <Alexander.Levin@microsoft.com>; Dexuan Cui <decui@mi
> crosoft.com>; Stephen Hemminger <sthemmin@microsoft.com>; Long Li <lo
> ngli@microsoft.com>; KY Srinivasan <kys@microsoft.com>; vkuznets <vku
> znets@redhat.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] hyper-v: Check for ring buffer in
> hv_get_bytes_to_read/write
>
> On Tue, 2019-03-12 at 18:02 +0000, Haiyang Zhang wrote:
> >
> >
> > > -----Original Message-----
> > > From: Mohammed Gamal <mgamal@redhat.com>
> > > Sent: Thursday, March 7, 2019 1:32 PM
> > > To: Michael Kelley <mikelley@microsoft.com>; linux-hyperv@vger.ke
> > > rn
> >
> > el.org;
> > > kimbrownkd <kimbrownkd@gmail.com>
> > > Cc: Sasha Levin <Alexander.Levin@microsoft.com>; Dexuan Cui
> > > <decui@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>
> > > ;
> > > Long Li <longli@microsoft.com>; KY Srinivasan <kys@microsoft.com>
> > > ;
> >
> > Haiyang
> > > Zhang <haiyangz@microsoft.com>; vkuznets <vkuznets@redhat.com>;
> >
> > linux-
> > > kernel@vger.kernel.org
> > > Subject: Re: [PATCH] hyper-v: Check for ring buffer in
> > > hv_get_bytes_to_read/write
> > >
> > > On Thu, 2019-03-07 at 17:33 +0000, Michael Kelley wrote:
> > > > From: Mohammed Gamal <mgamal@redhat.com> Sent: Thursday, March
> > > > 7,
> > > > 2019 8:36 AM
> > > > >
> > > > > This patch adds a check for the presence of the ring buffer
> > > > > in
> > > > > hv_get_bytes_to_read/write() to avoid possible NULL pointer
> > > > > dereferences.
> > > > > If the ring buffer is not yet allocated, return 0 bytes to be
> > > > > read/written.
> > > > >
> > > > > The root cause is that code that accesses the ring buffer
> >
> > including
> > > > > hv_get_bytes_to_read/write() could be vulnerable to the race
> > > > > condition discussed in
> > > > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%
> > > > > 2F
> >
> > %2Flk
> > > > >
> > >
> > > ml.org%2Flkml%2F2018%2F10%2F18%2F779&data=02%7C01%7Chaiyangz
> > > %40m
> > > > >
> > >
> > > icrosoft.com%7C73af013c14034bb0b1ad08d6a32b419c%7C72f988bf86f141a
> > > f9
> > > 1
> > > > >
> > >
> > > ab2d7cd011db47%7C1%7C0%7C636875803518430021&sdata=b51Xc5GUN
> > > nHX0K
> > > > > 08LrH3ShTyFcRZ4mYHUATd%2BDpvYDw%3D&reserved=0>;
> > > > >
> > > > > This race is being addressed by the patch series by Kimberly
> >
> > Brown
> > > > > in
> > > > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%
> > > > > 2F
> >
> > %2Flk
> > > > >
> > >
> > > ml.org%2Flkml%2F2019%2F2%2F21%2F1236&data=02%7C01%7Chaiyangz
> > > %40m
> > > > >
> > >
> > > icrosoft.com%7C73af013c14034bb0b1ad08d6a32b419c%7C72f988bf86f141a
> > > f9
> > > 1
> > > > >
> > >
> > > ab2d7cd011db47%7C1%7C0%7C636875803518430021&sdata=js1ff15Gbk7
> > > 0MD
> > > > > A2hkMZExxvAAbDuKDhfBvc5ZrckzM%3D&reserved=0 which is not
> > >
> > > final
> > > > > yet
> > > > >
> > > > > Signed-off-by: Mohammed Gamal <mgamal@redhat.com>
> > > >
> > > > Could you elaborate on the code paths where
> > > > hv_get_bytes_to_read/write() could be called when the ring
> > > > buffer
> > > > isn't yet allocated? My sense is that Kim Brown's patch will
> >
> > address
> > > > all of the code paths that involved sysfs access from outside
> > > > the
> > > > driver. And within a driver, the ring buffer should never be
> >
> > accessed
> > > > unless it is already allocated. Is there another code path
> > > > we're
> >
> > not
> > > > aware of? I'm wondering if these changes are really needed
> > > > once
> >
> > Kim
> > > > Brown's patch is finished.
> > > >
> > > > Michael
> > >
> > > I've seen one instance of the race in the netvsc driver when
> >
> > running traffic
> > > through it with iperf3 while continuously changing the channel
> >
> > settings.
> > >
> > > The following code path deallocates the ring buffer:
> > > netvsc_set_channels() -> netvsc_detach() ->
> > > rndis_filter_device_remove() -> netvsc_device_remove() ->
> >
> > vmbus_close()
> > > -> vmbus_free_ring() -> hv_ringbuffer_cleanup().
> > >
> > > netvsc_send_pkt() -> hv_get_bytes_to_write() might get called
> >
> > concurrently
> > > after vmbus_close() and before vmbus_open() returns and sets up
> > > the
> >
> > new ring
> > > buffer.
> > >
> > > The race is fairly hard to reproduce on recent upstream kernels,
> >
> > but I still
> > > managed to reproduce it.
> >
> >
> > Looking at the code from netvsc_detach() –
> > netif_tx_disable(ndev) is called before
> > rndis_filter_device_remove(hdev, nvdev).
> > So there should be no call to netvsc_send_pkt() after detaching.
> > What’s the crash stack trace?
> >
> > static int netvsc_detach(struct net_device *ndev,
> > struct netvsc_device *nvdev)
> > {
> > struct net_device_context *ndev_ctx = netdev_priv(ndev);
> > struct hv_device *hdev = ndev_ctx->device_ctx;
> > int ret;
> >
> > /* Don't try continuing to try and setup sub channels */
> > if (cancel_work_sync(&nvdev->subchan_work))
> > nvdev->num_chn = 1;
> >
> > /* If device was up (receiving) then shutdown */
> > if (netif_running(ndev)) {
> > netif_tx_disable(ndev);
> >
> > ret = rndis_filter_close(nvdev);
> > if (ret) {
> > netdev_err(ndev,
> > "unable to close device (ret
> > %d).\n", ret);
> > return ret;
> > }
> >
> > ret = netvsc_wait_until_empty(nvdev);
> > if (ret) {
> > netdev_err(ndev,
> > "Ring buffer not empty after
> > closing rndis\n");
> > return ret;
> > }
> > }
> >
> > netif_device_detach(ndev);
> >
> > rndis_filter_device_remove(hdev, nvdev);
> >
> > return 0;
> > }
> >
> > Thanks,
> > Haiyang
>
> Here is one stack trace on a 4.18 kernel, the most recent kernel I
> managed to reproduce this bug on.
> I haven't managed to reproduce on 5.0.0 yet, but I guess some recent
> changes to the netvsc driver could be masking the problem, as I tried
> backporting those changes to older RHEL 7 kernels and still managed
> to
> reproduce the problem there. I could however be wrong, and any
> pointers
> are still appreciated:
>
> [ 545.308507] BUG: unable to handle kernel NULL pointer dereference
> at
> 0000000000000004
> [ 545.308656] PGD 0 P4D 0
> [ 545.308763] Oops: 0000 [#1] SMP PTI
> [ 545.308855] CPU: 2 PID: 1800 Comm: iperf3 Kdump: loaded Not
> tainted
> 4.18.0-64.el8.test.x86_64 #1
> [ 545.308990] Hardware name: Microsoft Corporation Virtual
> Machine/Virtual Machine, BIOS Hyper-V UEFI Release v1.0 11/26/2012
> [ 545.309143] RIP: 0010:netvsc_send+0x2c9/0xce0 [hv_netvsc]
> [ 545.309298] Code: 4c 8b b1 c0 00 00 00 4f 8d 2c 64 49 c1 e5 07 4d
> 03
> ae c0 03 00 00 48 8b 84 03 30 01 00 00 4c 89 6c 24 18 48 8b 90 20 01
> 00
> 00 <8b> 72 04 8b 0a 8b 90 38 01 00 00 89 f7 01 f2 29 cf 29 ca 39 ce
> 0f
> [ 545.309321] RSP: 0018:ffffb8a305d5b6c0 EFLAGS: 00010282
> [ 545.309321] RAX: ffff926928bd7000 RBX: ffff92687dbe0000 RCX:
> ffff92687d5bec00
> [ 545.309321] RDX: 0000000000000000 RSI: ffff92691b61c654 RDI:
> 0000000000000000
> [ 545.309321] RBP: ffff926915dcde28 R08: ffff926915dcde00 R09:
> 0000000000000000
> [ 545.309321] R10: 00000000000db61c R11: 0000000000000f7e R12:
> 0000000000000001
> [ 545.309321] R13: ffff926931808180 R14: ffff926931801000 R15:
> 0000000000000000
> [ 545.309321] FS: 00007feca6a4b740(0000) GS:ffff926940080000(0000)
> knlGS:0000000000000000
> [ 545.309321] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 545.309321] CR2: 0000000000000004 CR3: 00000000dfccc004 CR4:
> 00000000003606e0
> [ 545.309321] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> 0000000000000000
> [ 545.309321] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> 0000000000000400
> [ 545.309321] Call Trace:
> [ 545.309321] netvsc_start_xmit+0x3c9/0x800 [hv_netvsc]
> [ 545.309321] ? __switch_to_asm+0x34/0x70
> [ 545.309321] ? __switch_to_asm+0x34/0x70
> [ 545.309321] ? ___slab_alloc+0x269/0x4e0
> [ 545.309321] ? __alloc_skb+0x82/0x1c0
> [ 545.309321] ? nft_do_chain+0x3d7/0x3f0 [nf_tables]
> [ 545.309321] ? nft_do_chain+0x3d7/0x3f0 [nf_tables]
> [ 545.309321] ? nft_do_chain+0x3d7/0x3f0 [nf_tables]
> [ 545.309321] ? _cond_resched+0x15/0x30
> [ 545.309321] ? netif_skb_features+0x118/0x280
> [ 545.309321] dev_hard_start_xmit+0xa5/0x210
> [ 545.309321] sch_direct_xmit+0x14f/0x340
> [ 545.309321] __dev_queue_xmit+0x799/0x8f0
> [ 545.309321] ip_finish_output2+0x2e0/0x430
> [ 545.309321] ? ip_finish_output+0x139/0x270
> [ 545.309321] ip_output+0x6c/0xe0
> [ 545.309321] ? ip_append_data.part.50+0xc0/0xc0
> [ 545.309321] ip_send_skb+0x15/0x40
> [ 545.309321] udp_send_skb.isra.43+0x153/0x340
> [ 545.309321] udp_sendmsg+0xac2/0xd30
> [ 545.309321] ? set_fd_set.part.7+0x40/0x40
> [ 545.309321] ? set_fd_set.part.7+0x40/0x40
> [ 545.309321] ? __check_object_size+0xa3/0x181
> [ 545.309321] ? sock_has_perm+0x78/0xa0
> [ 545.309321] ? core_sys_select+0x242/0x2f0
> [ 545.309321] ? sock_sendmsg+0x36/0x40
> [ 545.309321] ? udp_push_pending_frames+0x60/0x60
> [ 545.309321] sock_sendmsg+0x36/0x40
> [ 545.309321] sock_write_iter+0x8f/0xf0
> [ 545.309321] __vfs_write+0x156/0x1a0
> [ 545.309321] vfs_write+0xa5/0x1a0
> [ 545.309321] ksys_write+0x4f/0xb0
> [ 545.309321] do_syscall_64+0x5b/0x1b0
> [ 545.309321] entry_SYSCALL_64_after_hwframe+0x65/0xca
> [ 545.309321] RIP: 0033:0x7feca5fb5348
> [ 545.309321] Code: 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00
> 00
> 00 f3 0f 1e fa 48 8d 05 d5 63 2d 00 8b 00 85 c0 75 17 b8 01 00 00 00
> 0f
> 05 <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 41 54 49 89 d4
> 55
> [ 545.309321] RSP: 002b:00007ffc3ff1f108 EFLAGS: 00000246 ORIG_RAX:
> 0000000000000001
> [ 545.309321] RAX: ffffffffffffffda RBX: 00000000000005a8 RCX:
> 00007feca5fb5348
> [ 545.309321] RDX: 00000000000005a8 RSI: 00007feca6a59000 RDI:
> 0000000000000009
> [ 545.309321] RBP: 00007feca6a59000 R08: 0000000000000002 R09:
> 00cd09a3238b4e43
> [ 545.309321] R10: 0002961ecea49016 R11: 0000000000000246 R12:
> 0000000000000009
> [ 545.309321] R13: 00000000000005a8 R14: 00007ffc3ff1f180 R15:
> 0000563c1e05b260
> [ 545.309321] Modules linked in: nft_chain_nat_ipv6
> nf_conntrack_ipv6
> nf_defrag_ipv6 nf_nat_ipv6 nft_chain_route_ipv6 nft_chain_nat_ipv4
> nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat
> nft_chain_route_ipv4 nf_conntrack ip_set nf_tables nfnetlink vfat fat
> sb_edac crct10dif_pclmul crc32_pclmul ghash_clmulni_intel
> intel_rapl_perf sg hv_utils hv_balloon pcspkr joydev xfs libcrc32c
> sd_mod sr_mod cdrom serio_raw hv_storvsc hv_netvsc scsi_transport_fc
> hyperv_fb hyperv_keyboard hid_hyperv crc32c_intel hv_vmbus dm_mirror
> dm_region_hash dm_log dm_mod [last unloaded: nft_compat]
> [ 545.309321] CR2: 0000000000000004
>
> From the stack trace netvsc_send+0x2c9 points to this line:
>
> static inline u32 hv_get_bytes_to_write(const struct hv_ring_
> bu
> ffer_info *rbi)
> {
> u32 read_loc, write_loc, dsize, write;
>
> dsize = rbi->ring_datasize;
> read_loc = READ_ONCE(rbi->ring_buffer->read_index); <-------
> --
> write_loc = rbi->ring_buffer->write_index;
>
> write = write_loc >= read_loc ? dsize - (write_loc -
> read_loc)
> read_loc - write_loc;
> return write;
> }
>
> which gets called from netvsc_send_pkt().
^ permalink raw reply
* [PATCH] hyperv: a potential NULL pointer dereference
From: Kangjie Lu @ 2019-03-14 5:46 UTC (permalink / raw)
To: kjlu
Cc: pakki001, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Sasha Levin, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, x86, linux-hyperv, linux-kernel
In case alloc_page, the fix returns -ENOMEM to avoid the potential
NULL pointer dereference.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
---
arch/x86/hyperv/hv_init.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 7abb09e2eeb8..dfdb4ce1ae9c 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -102,9 +102,13 @@ static int hv_cpu_init(unsigned int cpu)
u64 msr_vp_index;
struct hv_vp_assist_page **hvp = &hv_vp_assist_page[smp_processor_id()];
void **input_arg;
+ struct page *pg;
input_arg = (void **)this_cpu_ptr(hyperv_pcpu_input_arg);
- *input_arg = page_address(alloc_page(GFP_KERNEL));
+ pg = alloc_page(GFP_KERNEL);
+ if (unlikely(!pg))
+ return -ENOMEM;
+ *input_arg = page_address(pg);
hv_get_vp_index(msr_vp_index);
--
2.17.1
^ permalink raw reply related
* RE: [PATCH] hyper-v: Check for ring buffer in hv_get_bytes_to_read/write
From: Stephen Hemminger @ 2019-03-13 21:12 UTC (permalink / raw)
To: mgamal@redhat.com, Haiyang Zhang, Michael Kelley,
linux-hyperv@vger.kernel.org, kimbrownkd
Cc: Sasha Levin, Dexuan Cui, Long Li, KY Srinivasan, vkuznets
In-Reply-To: <1552472705.4717.7.camel@redhat.com>
What test are you running?
-----Original Message-----
From: Mohammed Gamal <mgamal@redhat.com>
Sent: Wednesday, March 13, 2019 3:25 AM
To: Haiyang Zhang <haiyangz@microsoft.com>; Michael Kelley <mikelley@microsoft.com>; linux-hyperv@vger.kernel.org; kimbrownkd <kimbrownkd@gmail.com>
Cc: Sasha Levin <Alexander.Levin@microsoft.com>; Dexuan Cui <decui@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>; Long Li <longli@microsoft.com>; KY Srinivasan <kys@microsoft.com>; vkuznets <vkuznets@redhat.com>; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] hyper-v: Check for ring buffer in hv_get_bytes_to_read/write
On Tue, 2019-03-12 at 18:02 +0000, Haiyang Zhang wrote:
>
>
> > -----Original Message-----
> > From: Mohammed Gamal <mgamal@redhat.com>
> > Sent: Thursday, March 7, 2019 1:32 PM
> > To: Michael Kelley <mikelley@microsoft.com>; linux-hyperv@vger.kern
> el.org;
> > kimbrownkd <kimbrownkd@gmail.com>
> > Cc: Sasha Levin <Alexander.Levin@microsoft.com>; Dexuan Cui
> > <decui@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> > Long Li <longli@microsoft.com>; KY Srinivasan <kys@microsoft.com>;
> Haiyang
> > Zhang <haiyangz@microsoft.com>; vkuznets <vkuznets@redhat.com>;
> linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH] hyper-v: Check for ring buffer in
> > hv_get_bytes_to_read/write
> >
> > On Thu, 2019-03-07 at 17:33 +0000, Michael Kelley wrote:
> > > From: Mohammed Gamal <mgamal@redhat.com> Sent: Thursday, March 7,
> > > 2019 8:36 AM
> > > >
> > > > This patch adds a check for the presence of the ring buffer in
> > > > hv_get_bytes_to_read/write() to avoid possible NULL pointer
> > > > dereferences.
> > > > If the ring buffer is not yet allocated, return 0 bytes to be
> > > > read/written.
> > > >
> > > > The root cause is that code that accesses the ring buffer
> including
> > > > hv_get_bytes_to_read/write() could be vulnerable to the race
> > > > condition discussed in
> > > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F
> %2Flk
> > > >
> > ml.org%2Flkml%2F2018%2F10%2F18%2F779&data=02%7C01%7Chaiyangz
> > %40m
> > > >
> > icrosoft.com%7C73af013c14034bb0b1ad08d6a32b419c%7C72f988bf86f141af9
> > 1
> > > >
> > ab2d7cd011db47%7C1%7C0%7C636875803518430021&sdata=b51Xc5GUN
> > nHX0K
> > > > 08LrH3ShTyFcRZ4mYHUATd%2BDpvYDw%3D&reserved=0>;
> > > >
> > > > This race is being addressed by the patch series by Kimberly
> Brown
> > > > in
> > > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F
> %2Flk
> > > >
> > ml.org%2Flkml%2F2019%2F2%2F21%2F1236&data=02%7C01%7Chaiyangz
> > %40m
> > > >
> > icrosoft.com%7C73af013c14034bb0b1ad08d6a32b419c%7C72f988bf86f141af9
> > 1
> > > >
> > ab2d7cd011db47%7C1%7C0%7C636875803518430021&sdata=js1ff15Gbk7
> > 0MD
> > > > A2hkMZExxvAAbDuKDhfBvc5ZrckzM%3D&reserved=0 which is not
> > final
> > > > yet
> > > >
> > > > Signed-off-by: Mohammed Gamal <mgamal@redhat.com>
> > >
> > > Could you elaborate on the code paths where
> > > hv_get_bytes_to_read/write() could be called when the ring buffer
> > > isn't yet allocated? My sense is that Kim Brown's patch will
> address
> > > all of the code paths that involved sysfs access from outside the
> > > driver. And within a driver, the ring buffer should never be
> accessed
> > > unless it is already allocated. Is there another code path we're
> not
> > > aware of? I'm wondering if these changes are really needed once
> Kim
> > > Brown's patch is finished.
> > >
> > > Michael
> >
> > I've seen one instance of the race in the netvsc driver when
> running traffic
> > through it with iperf3 while continuously changing the channel
> settings.
> >
> > The following code path deallocates the ring buffer:
> > netvsc_set_channels() -> netvsc_detach() ->
> > rndis_filter_device_remove() -> netvsc_device_remove() ->
> vmbus_close()
> > -> vmbus_free_ring() -> hv_ringbuffer_cleanup().
> >
> > netvsc_send_pkt() -> hv_get_bytes_to_write() might get called
> concurrently
> > after vmbus_close() and before vmbus_open() returns and sets up the
> new ring
> > buffer.
> >
> > The race is fairly hard to reproduce on recent upstream kernels,
> but I still
> > managed to reproduce it.
>
> Looking at the code from netvsc_detach() –
> netif_tx_disable(ndev) is called before
> rndis_filter_device_remove(hdev, nvdev).
> So there should be no call to netvsc_send_pkt() after detaching.
> What’s the crash stack trace?
>
> static int netvsc_detach(struct net_device *ndev,
> struct netvsc_device *nvdev)
> {
> struct net_device_context *ndev_ctx = netdev_priv(ndev);
> struct hv_device *hdev = ndev_ctx->device_ctx;
> int ret;
>
> /* Don't try continuing to try and setup sub channels */
> if (cancel_work_sync(&nvdev->subchan_work))
> nvdev->num_chn = 1;
>
> /* If device was up (receiving) then shutdown */
> if (netif_running(ndev)) {
> netif_tx_disable(ndev);
>
> ret = rndis_filter_close(nvdev);
> if (ret) {
> netdev_err(ndev,
> "unable to close device (ret
> %d).\n", ret);
> return ret;
> }
>
> ret = netvsc_wait_until_empty(nvdev);
> if (ret) {
> netdev_err(ndev,
> "Ring buffer not empty after
> closing rndis\n");
> return ret;
> }
> }
>
> netif_device_detach(ndev);
>
> rndis_filter_device_remove(hdev, nvdev);
>
> return 0;
> }
>
> Thanks,
> Haiyang
Here is one stack trace on a 4.18 kernel, the most recent kernel I
managed to reproduce this bug on.
I haven't managed to reproduce on 5.0.0 yet, but I guess some recent
changes to the netvsc driver could be masking the problem, as I tried
backporting those changes to older RHEL 7 kernels and still managed to
reproduce the problem there. I could however be wrong, and any pointers
are still appreciated:
[ 545.308507] BUG: unable to handle kernel NULL pointer dereference at
0000000000000004
[ 545.308656] PGD 0 P4D 0
[ 545.308763] Oops: 0000 [#1] SMP PTI
[ 545.308855] CPU: 2 PID: 1800 Comm: iperf3 Kdump: loaded Not tainted
4.18.0-64.el8.test.x86_64 #1
[ 545.308990] Hardware name: Microsoft Corporation Virtual
Machine/Virtual Machine, BIOS Hyper-V UEFI Release v1.0 11/26/2012
[ 545.309143] RIP: 0010:netvsc_send+0x2c9/0xce0 [hv_netvsc]
[ 545.309298] Code: 4c 8b b1 c0 00 00 00 4f 8d 2c 64 49 c1 e5 07 4d 03
ae c0 03 00 00 48 8b 84 03 30 01 00 00 4c 89 6c 24 18 48 8b 90 20 01 00
00 <8b> 72 04 8b 0a 8b 90 38 01 00 00 89 f7 01 f2 29 cf 29 ca 39 ce
0f
[ 545.309321] RSP: 0018:ffffb8a305d5b6c0 EFLAGS: 00010282
[ 545.309321] RAX: ffff926928bd7000 RBX: ffff92687dbe0000 RCX:
ffff92687d5bec00
[ 545.309321] RDX: 0000000000000000 RSI: ffff92691b61c654 RDI:
0000000000000000
[ 545.309321] RBP: ffff926915dcde28 R08: ffff926915dcde00 R09:
0000000000000000
[ 545.309321] R10: 00000000000db61c R11: 0000000000000f7e R12:
0000000000000001
[ 545.309321] R13: ffff926931808180 R14: ffff926931801000 R15:
0000000000000000
[ 545.309321] FS: 00007feca6a4b740(0000) GS:ffff926940080000(0000)
knlGS:0000000000000000
[ 545.309321] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 545.309321] CR2: 0000000000000004 CR3: 00000000dfccc004 CR4:
00000000003606e0
[ 545.309321] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[ 545.309321] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
0000000000000400
[ 545.309321] Call Trace:
[ 545.309321] netvsc_start_xmit+0x3c9/0x800 [hv_netvsc]
[ 545.309321] ? __switch_to_asm+0x34/0x70
[ 545.309321] ? __switch_to_asm+0x34/0x70
[ 545.309321] ? ___slab_alloc+0x269/0x4e0
[ 545.309321] ? __alloc_skb+0x82/0x1c0
[ 545.309321] ? nft_do_chain+0x3d7/0x3f0 [nf_tables]
[ 545.309321] ? nft_do_chain+0x3d7/0x3f0 [nf_tables]
[ 545.309321] ? nft_do_chain+0x3d7/0x3f0 [nf_tables]
[ 545.309321] ? _cond_resched+0x15/0x30
[ 545.309321] ? netif_skb_features+0x118/0x280
[ 545.309321] dev_hard_start_xmit+0xa5/0x210
[ 545.309321] sch_direct_xmit+0x14f/0x340
[ 545.309321] __dev_queue_xmit+0x799/0x8f0
[ 545.309321] ip_finish_output2+0x2e0/0x430
[ 545.309321] ? ip_finish_output+0x139/0x270
[ 545.309321] ip_output+0x6c/0xe0
[ 545.309321] ? ip_append_data.part.50+0xc0/0xc0
[ 545.309321] ip_send_skb+0x15/0x40
[ 545.309321] udp_send_skb.isra.43+0x153/0x340
[ 545.309321] udp_sendmsg+0xac2/0xd30
[ 545.309321] ? set_fd_set.part.7+0x40/0x40
[ 545.309321] ? set_fd_set.part.7+0x40/0x40
[ 545.309321] ? __check_object_size+0xa3/0x181
[ 545.309321] ? sock_has_perm+0x78/0xa0
[ 545.309321] ? core_sys_select+0x242/0x2f0
[ 545.309321] ? sock_sendmsg+0x36/0x40
[ 545.309321] ? udp_push_pending_frames+0x60/0x60
[ 545.309321] sock_sendmsg+0x36/0x40
[ 545.309321] sock_write_iter+0x8f/0xf0
[ 545.309321] __vfs_write+0x156/0x1a0
[ 545.309321] vfs_write+0xa5/0x1a0
[ 545.309321] ksys_write+0x4f/0xb0
[ 545.309321] do_syscall_64+0x5b/0x1b0
[ 545.309321] entry_SYSCALL_64_after_hwframe+0x65/0xca
[ 545.309321] RIP: 0033:0x7feca5fb5348
[ 545.309321] Code: 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00
00 f3 0f 1e fa 48 8d 05 d5 63 2d 00 8b 00 85 c0 75 17 b8 01 00 00 00 0f
05 <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 41 54 49 89 d4 55
[ 545.309321] RSP: 002b:00007ffc3ff1f108 EFLAGS: 00000246 ORIG_RAX:
0000000000000001
[ 545.309321] RAX: ffffffffffffffda RBX: 00000000000005a8 RCX:
00007feca5fb5348
[ 545.309321] RDX: 00000000000005a8 RSI: 00007feca6a59000 RDI:
0000000000000009
[ 545.309321] RBP: 00007feca6a59000 R08: 0000000000000002 R09:
00cd09a3238b4e43
[ 545.309321] R10: 0002961ecea49016 R11: 0000000000000246 R12:
0000000000000009
[ 545.309321] R13: 00000000000005a8 R14: 00007ffc3ff1f180 R15:
0000563c1e05b260
[ 545.309321] Modules linked in: nft_chain_nat_ipv6 nf_conntrack_ipv6
nf_defrag_ipv6 nf_nat_ipv6 nft_chain_route_ipv6 nft_chain_nat_ipv4
nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat
nft_chain_route_ipv4 nf_conntrack ip_set nf_tables nfnetlink vfat fat
sb_edac crct10dif_pclmul crc32_pclmul ghash_clmulni_intel
intel_rapl_perf sg hv_utils hv_balloon pcspkr joydev xfs libcrc32c
sd_mod sr_mod cdrom serio_raw hv_storvsc hv_netvsc scsi_transport_fc
hyperv_fb hyperv_keyboard hid_hyperv crc32c_intel hv_vmbus dm_mirror
dm_region_hash dm_log dm_mod [last unloaded: nft_compat]
[ 545.309321] CR2: 0000000000000004
From the stack trace netvsc_send+0x2c9 points to this line:
static inline u32 hv_get_bytes_to_write(const struct hv_ring_bu
ffer_info *rbi)
{
u32 read_loc, write_loc, dsize, write;
dsize = rbi->ring_datasize;
read_loc = READ_ONCE(rbi->ring_buffer->read_index); <---------
write_loc = rbi->ring_buffer->write_index;
write = write_loc >= read_loc ? dsize - (write_loc - read_loc)
read_loc - write_loc;
return write;
}
which gets called from netvsc_send_pkt().
^ permalink raw reply
* Re: [PATCH v2] hv_netvsc: fix a possible NULL pointer dereference in netvsc_get_ethtool_stats()
From: David Miller @ 2019-03-13 21:08 UTC (permalink / raw)
To: kjlu
Cc: pakki001, kys, haiyangz, sthemmin, sashal, linux-hyperv, netdev,
linux-kernel
In-Reply-To: <20190312050149.3032-1-kjlu@umn.edu>
From: Kangjie Lu <kjlu@umn.edu>
Date: Tue, 12 Mar 2019 00:01:49 -0500
> In case kvmalloc_array fails, we should stop using it to avoid
> NULL pointer dereference.
>
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---
> drivers/net/hyperv/netvsc_drv.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index cf4897043e83..4b3a03029fe8 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -1426,6 +1426,9 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
> pcpu_sum = kvmalloc_array(num_possible_cpus(),
> sizeof(struct netvsc_ethtool_pcpu_stats),
> GFP_KERNEL);
> + if (!pcpu_sum)
> + return;
> +
> netvsc_get_pcpu_stats(dev, pcpu_sum);
> for_each_present_cpu(cpu) {
> struct netvsc_ethtool_pcpu_stats *this_sum = &pcpu_sum[cpu];
What a surprise it will be when the user's statistics monitoring
software sees values suddenly drop to zero with no error indication
whatsoever.
^ permalink raw reply
* RE: [PATCH 1/2] Drivers: hv: Move Hyper-V clockevents code to new clocksource driver
From: Vitaly Kuznetsov @ 2019-03-13 17:17 UTC (permalink / raw)
To: Michael Kelley
Cc: catalin.marinas@arm.com, mark.rutland@arm.com,
will.deacon@arm.com, marc.zyngier@arm.com,
linux-arm-kernel@lists.infradead.org, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com,
marcelo.cerri@canonical.com, Sunil Muthuswamy, KY Srinivasan
In-Reply-To: <DM5PR2101MB09180753D788F8587B19B993D74A0@DM5PR2101MB0918.namprd21.prod.outlook.com>
Michael Kelley <mikelley@microsoft.com> writes:
> From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Wednesday, March 13, 2019 7:23 AM
>
>> >> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> >> > index be6e0fb..a887955 100644
>> >> > --- a/drivers/clocksource/Makefile
>> >> > +++ b/drivers/clocksource/Makefile
>> >> > @@ -83,3 +83,4 @@ obj-$(CONFIG_ATCPIT100_TIMER) += timer-atcpit100.o
>> >> > obj-$(CONFIG_RISCV_TIMER) += timer-riscv.o
>> >> > obj-$(CONFIG_CSKY_MP_TIMER) += timer-mp-csky.o
>> >> > obj-$(CONFIG_GX6605S_TIMER) += timer-gx6605s.o
>> >> > +obj-$(CONFIG_HYPERV) += hyperv_syntimer.o
>> >>
>> >> (just a couple of spare thoughs)
>> >>
>> >> CONFIG_HYPERV can also be a module, are we OK with that? (we'll have to
>> >> support module loading/unloading then and honestly I see no reason for
>> >> that. I would prefer everything but VMBus devices to be in
>> >> kernel.) If we don't want it to be a module we can create a hidden
>> >> CONFIG_HYPERV_STIMER or something like that - just like we already do
>> >> for CONFIG_HYPERV_TSCPAGE.
>> >>
>> >> There is, however, one additional dependency here: when running in
>> >> non-direct mode, Hyper-V clockevent devices require functional Hyper-V
>> >> messaging - which currently lives in VMBus code so that may explain why
>> >> you may want to keep stimer code in the same entity. Or, alternatively,
>> >> we can move Hyper-V messaging out of VMBus code (is it actually
>> >> architecture-agnostic?)
>> >>
>> >
>> > I thought about introducing CONFIG_HYPERV_STIMER, but in my
>> > judgment it was just unnecessary complexity. The Hyper-V clocksource
>> > driver can't exist independent of Hyper-V, and vice versa. When both the
>> > clocksource and clockevents code is considered, the VMbus driver and
>> > Hyper-V initialization code has to call directly into the driver since the
>> > Hyper-V synthetic timers and reference time counter aren't independently
>> > enumerated. Even if we could get the Hyper-V messaging out of VMbus
>> > code, we would still need the clocksource initialization call directly from
>> > hyperv_init(), which is not in a module (see the 2nd patch of the series).
>> >
>>
>> Right, so hv_init_clocksource() cannot live in hv_vmbus module and we
>> need to somehow prevent hyperv_syntimer.o from going in there. And
>>
>> +obj-$(CONFIG_HYPERV) += hyperv_syntimer.o
>>
>> will do exactly the opposite - put it in hv_vmbus module. Or am I
>> missing something? (I haven't tried to build your code yet, sorry).
>>
>
> That line just controls whether hyperv_syntimer.o is built. It doesn't put
> it in the hv_vmbus module. All of the clocksource .o files that are built go
> into the kernel, not in a module. But thinking about it more, the above works
> correctly when CONFIG_HYPERV=y, but not when CONFIG_HYPERV=m.
Yes, that's what I meant.
> I'll have to introduce CONFIG_HYPERV_TIMER after all. Will fix this in v2. Thanks
> for the discussion!
Thanks!
--
Vitaly
^ permalink raw reply
* RE: [PATCH 1/2] Drivers: hv: Move Hyper-V clockevents code to new clocksource driver
From: Michael Kelley @ 2019-03-13 16:19 UTC (permalink / raw)
To: vkuznets
Cc: catalin.marinas@arm.com, mark.rutland@arm.com,
will.deacon@arm.com, marc.zyngier@arm.com,
linux-arm-kernel@lists.infradead.org, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com,
marcelo.cerri@canonical.com, Sunil Muthuswamy, KY Srinivasan
In-Reply-To: <87pnqusvjq.fsf@vitty.brq.redhat.com>
From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Wednesday, March 13, 2019 7:23 AM
> >> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> >> > index be6e0fb..a887955 100644
> >> > --- a/drivers/clocksource/Makefile
> >> > +++ b/drivers/clocksource/Makefile
> >> > @@ -83,3 +83,4 @@ obj-$(CONFIG_ATCPIT100_TIMER) += timer-atcpit100.o
> >> > obj-$(CONFIG_RISCV_TIMER) += timer-riscv.o
> >> > obj-$(CONFIG_CSKY_MP_TIMER) += timer-mp-csky.o
> >> > obj-$(CONFIG_GX6605S_TIMER) += timer-gx6605s.o
> >> > +obj-$(CONFIG_HYPERV) += hyperv_syntimer.o
> >>
> >> (just a couple of spare thoughs)
> >>
> >> CONFIG_HYPERV can also be a module, are we OK with that? (we'll have to
> >> support module loading/unloading then and honestly I see no reason for
> >> that. I would prefer everything but VMBus devices to be in
> >> kernel.) If we don't want it to be a module we can create a hidden
> >> CONFIG_HYPERV_STIMER or something like that - just like we already do
> >> for CONFIG_HYPERV_TSCPAGE.
> >>
> >> There is, however, one additional dependency here: when running in
> >> non-direct mode, Hyper-V clockevent devices require functional Hyper-V
> >> messaging - which currently lives in VMBus code so that may explain why
> >> you may want to keep stimer code in the same entity. Or, alternatively,
> >> we can move Hyper-V messaging out of VMBus code (is it actually
> >> architecture-agnostic?)
> >>
> >
> > I thought about introducing CONFIG_HYPERV_STIMER, but in my
> > judgment it was just unnecessary complexity. The Hyper-V clocksource
> > driver can't exist independent of Hyper-V, and vice versa. When both the
> > clocksource and clockevents code is considered, the VMbus driver and
> > Hyper-V initialization code has to call directly into the driver since the
> > Hyper-V synthetic timers and reference time counter aren't independently
> > enumerated. Even if we could get the Hyper-V messaging out of VMbus
> > code, we would still need the clocksource initialization call directly from
> > hyperv_init(), which is not in a module (see the 2nd patch of the series).
> >
>
> Right, so hv_init_clocksource() cannot live in hv_vmbus module and we
> need to somehow prevent hyperv_syntimer.o from going in there. And
>
> +obj-$(CONFIG_HYPERV) += hyperv_syntimer.o
>
> will do exactly the opposite - put it in hv_vmbus module. Or am I
> missing something? (I haven't tried to build your code yet, sorry).
>
That line just controls whether hyperv_syntimer.o is built. It doesn't put
it in the hv_vmbus module. All of the clocksource .o files that are built go
into the kernel, not in a module. But thinking about it more, the above works
correctly when CONFIG_HYPERV=y, but not when CONFIG_HYPERV=m. I'll
have to introduce CONFIG_HYPERV_TIMER after all. Will fix this in v2. Thanks
for the discussion!
Michael
^ permalink raw reply
* RE: [PATCH 1/2] Drivers: hv: Move Hyper-V clockevents code to new clocksource driver
From: Vitaly Kuznetsov @ 2019-03-13 14:23 UTC (permalink / raw)
To: Michael Kelley
Cc: catalin.marinas@arm.com, mark.rutland@arm.com,
will.deacon@arm.com, marc.zyngier@arm.com,
linux-arm-kernel@lists.infradead.org, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com,
marcelo.cerri@canonical.com, Sunil Muthuswamy, KY Srinivasan
In-Reply-To: <DM5PR2101MB091897108980449F12FDDB3DD74A0@DM5PR2101MB0918.namprd21.prod.outlook.com>
Michael Kelley <mikelley@microsoft.com> writes:
> From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Wednesday, March 13, 2019 1:28 AM
>>
>> > Clockevents code for Hyper-V synthetic timers is currently mixed
>> > in with other Hyper-V code. Move the code to a Hyper-V specific
>> > driver in the "clocksource" directory. Update the VMbus driver
>> > to call initialization and cleanup routines since the Hyper-V
>> > synthetic timers are not independently enumerated in ACPI.
>> >
>>
>> I like the idea! Would it also make sense to consider moving Hyper-V
>> clocksource from arch/x86/hyperv/hv_init.c? The thing is that we use it
>> from arch-independent code (e.g. seee 'hyperv_cs' usage in
>> drivers/hv/hv_util.c).
>
> That's what the second patch in the series does. :-) But let me
> know if there's something you think I've missed.
>
Oh, sorry, it's just me - your subject lines messed with my brain :-) I
like your idea even more then!
>>
>> Nitpicking:
>>
>> This has nothing to do with your patch but we have a mix of 'stimer',
>> 'timer', 'syntimer' names when we refer to Hyper-V Synthetic timers, it
>> may make sense to try to converge on something (stimer would probably be
>> my personal preference but I don't really care as long as we use the
>> same abbreviation). Examples:
>>
>> hv_init_timer_config(...)
>> HV_MSR_SYNTIMER_AVAILABLE
>> HYPERV_STIMER0_VECTOR
>> hv_syntimer_init(...)
>> ...
>
> Yes, you are right about the mish-mash of names. I also like converging
> on "stimer". I'll certainly change things like hv_syntimer_init() to
> hv_stimer_init() as part of the patch and avoid making the problem
> worse.
>
> What about the name of the new .c and .h files? They include code
> both the stimer-based clockevents, as well as the Hyper-V
> reference time source based clocksource. Stay with "syntimer" or
> shorten to "stimer", even though the code is slightly broader than
> just the stimers? (which highlights the generic Linux naming issue that
> "clocksource drivers" include code for both clockevents and
> clocksources)
"hyperv_timers.*" maybe? (those who read TLFS may get confused because
'Synthetic timers' doesn't refer to clocksources - just to clockevents).
>
>>
>> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> > index be6e0fb..a887955 100644
>> > --- a/drivers/clocksource/Makefile
>> > +++ b/drivers/clocksource/Makefile
>> > @@ -83,3 +83,4 @@ obj-$(CONFIG_ATCPIT100_TIMER) += timer-atcpit100.o
>> > obj-$(CONFIG_RISCV_TIMER) += timer-riscv.o
>> > obj-$(CONFIG_CSKY_MP_TIMER) += timer-mp-csky.o
>> > obj-$(CONFIG_GX6605S_TIMER) += timer-gx6605s.o
>> > +obj-$(CONFIG_HYPERV) += hyperv_syntimer.o
>>
>> (just a couple of spare thoughs)
>>
>> CONFIG_HYPERV can also be a module, are we OK with that? (we'll have to
>> support module loading/unloading then and honestly I see no reason for
>> that. I would prefer everything but VMBus devices to be in
>> kernel.) If we don't want it to be a module we can create a hidden
>> CONFIG_HYPERV_STIMER or something like that - just like we already do
>> for CONFIG_HYPERV_TSCPAGE.
>>
>> There is, however, one additional dependency here: when running in
>> non-direct mode, Hyper-V clockevent devices require functional Hyper-V
>> messaging - which currently lives in VMBus code so that may explain why
>> you may want to keep stimer code in the same entity. Or, alternatively,
>> we can move Hyper-V messaging out of VMBus code (is it actually
>> architecture-agnostic?)
>>
>
> I thought about introducing CONFIG_HYPERV_STIMER, but in my
> judgment it was just unnecessary complexity. The Hyper-V clocksource
> driver can't exist independent of Hyper-V, and vice versa. When both the
> clocksource and clockevents code is considered, the VMbus driver and
> Hyper-V initialization code has to call directly into the driver since the
> Hyper-V synthetic timers and reference time counter aren't independently
> enumerated. Even if we could get the Hyper-V messaging out of VMbus
> code, we would still need the clocksource initialization call directly from
> hyperv_init(), which is not in a module (see the 2nd patch of the series).
>
Right, so hv_init_clocksource() cannot live in hv_vmbus module and we
need to somehow prevent hyperv_syntimer.o from going in there. And
+obj-$(CONFIG_HYPERV) += hyperv_syntimer.o
will do exactly the opposite - put it in hv_vmbus module. Or am I
missing something? (I haven't tried to build your code yet, sorry).
> Michael
--
Vitaly
^ permalink raw reply
* RE: [PATCH 1/2] Drivers: hv: Move Hyper-V clockevents code to new clocksource driver
From: Michael Kelley @ 2019-03-13 13:38 UTC (permalink / raw)
To: vkuznets
Cc: catalin.marinas@arm.com, mark.rutland@arm.com,
will.deacon@arm.com, marc.zyngier@arm.com,
linux-arm-kernel@lists.infradead.org, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com,
marcelo.cerri@canonical.com, Sunil Muthuswamy, KY Srinivasan
In-Reply-To: <874l87tbz2.fsf@vitty.brq.redhat.com>
From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Wednesday, March 13, 2019 1:28 AM
>
> > Clockevents code for Hyper-V synthetic timers is currently mixed
> > in with other Hyper-V code. Move the code to a Hyper-V specific
> > driver in the "clocksource" directory. Update the VMbus driver
> > to call initialization and cleanup routines since the Hyper-V
> > synthetic timers are not independently enumerated in ACPI.
> >
>
> I like the idea! Would it also make sense to consider moving Hyper-V
> clocksource from arch/x86/hyperv/hv_init.c? The thing is that we use it
> from arch-independent code (e.g. seee 'hyperv_cs' usage in
> drivers/hv/hv_util.c).
That's what the second patch in the series does. :-) But let me
know if there's something you think I've missed.
>
> Nitpicking:
>
> This has nothing to do with your patch but we have a mix of 'stimer',
> 'timer', 'syntimer' names when we refer to Hyper-V Synthetic timers, it
> may make sense to try to converge on something (stimer would probably be
> my personal preference but I don't really care as long as we use the
> same abbreviation). Examples:
>
> hv_init_timer_config(...)
> HV_MSR_SYNTIMER_AVAILABLE
> HYPERV_STIMER0_VECTOR
> hv_syntimer_init(...)
> ...
Yes, you are right about the mish-mash of names. I also like converging
on "stimer". I'll certainly change things like hv_syntimer_init() to
hv_stimer_init() as part of the patch and avoid making the problem
worse.
What about the name of the new .c and .h files? They include code
both the stimer-based clockevents, as well as the Hyper-V
reference time source based clocksource. Stay with "syntimer" or
shorten to "stimer", even though the code is slightly broader than
just the stimers? (which highlights the generic Linux naming issue that
"clocksource drivers" include code for both clockevents and clocksources)
>
> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> > index be6e0fb..a887955 100644
> > --- a/drivers/clocksource/Makefile
> > +++ b/drivers/clocksource/Makefile
> > @@ -83,3 +83,4 @@ obj-$(CONFIG_ATCPIT100_TIMER) += timer-atcpit100.o
> > obj-$(CONFIG_RISCV_TIMER) += timer-riscv.o
> > obj-$(CONFIG_CSKY_MP_TIMER) += timer-mp-csky.o
> > obj-$(CONFIG_GX6605S_TIMER) += timer-gx6605s.o
> > +obj-$(CONFIG_HYPERV) += hyperv_syntimer.o
>
> (just a couple of spare thoughs)
>
> CONFIG_HYPERV can also be a module, are we OK with that? (we'll have to
> support module loading/unloading then and honestly I see no reason for
> that. I would prefer everything but VMBus devices to be in
> kernel.) If we don't want it to be a module we can create a hidden
> CONFIG_HYPERV_STIMER or something like that - just like we already do
> for CONFIG_HYPERV_TSCPAGE.
>
> There is, however, one additional dependency here: when running in
> non-direct mode, Hyper-V clockevent devices require functional Hyper-V
> messaging - which currently lives in VMBus code so that may explain why
> you may want to keep stimer code in the same entity. Or, alternatively,
> we can move Hyper-V messaging out of VMBus code (is it actually
> architecture-agnostic?)
>
I thought about introducing CONFIG_HYPERV_STIMER, but in my
judgment it was just unnecessary complexity. The Hyper-V clocksource
driver can't exist independent of Hyper-V, and vice versa. When both the
clocksource and clockevents code is considered, the VMbus driver and
Hyper-V initialization code has to call directly into the driver since the
Hyper-V synthetic timers and reference time counter aren't independently
enumerated. Even if we could get the Hyper-V messaging out of VMbus
code, we would still need the clocksource initialization call directly from
hyperv_init(), which is not in a module (see the 2nd patch of the series).
Michael
^ permalink raw reply
* Re: [PATCH] hyper-v: Check for ring buffer in hv_get_bytes_to_read/write
From: Mohammed Gamal @ 2019-03-13 10:25 UTC (permalink / raw)
To: Haiyang Zhang, Michael Kelley, linux-hyperv@vger.kernel.org,
kimbrownkd
Cc: Sasha Levin, Dexuan Cui, Stephen Hemminger, Long Li,
KY Srinivasan, vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <DM5PR2101MB0725B71EE9A41E1ABE2B266ACA490@DM5PR2101MB0725.namprd21.prod.outlook.com>
On Tue, 2019-03-12 at 18:02 +0000, Haiyang Zhang wrote:
>
>
> > -----Original Message-----
> > From: Mohammed Gamal <mgamal@redhat.com>
> > Sent: Thursday, March 7, 2019 1:32 PM
> > To: Michael Kelley <mikelley@microsoft.com>; linux-hyperv@vger.kern
> el.org;
> > kimbrownkd <kimbrownkd@gmail.com>
> > Cc: Sasha Levin <Alexander.Levin@microsoft.com>; Dexuan Cui
> > <decui@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> > Long Li <longli@microsoft.com>; KY Srinivasan <kys@microsoft.com>;
> Haiyang
> > Zhang <haiyangz@microsoft.com>; vkuznets <vkuznets@redhat.com>;
> linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH] hyper-v: Check for ring buffer in
> > hv_get_bytes_to_read/write
> >
> > On Thu, 2019-03-07 at 17:33 +0000, Michael Kelley wrote:
> > > From: Mohammed Gamal <mgamal@redhat.com> Sent: Thursday, March 7,
> > > 2019 8:36 AM
> > > >
> > > > This patch adds a check for the presence of the ring buffer in
> > > > hv_get_bytes_to_read/write() to avoid possible NULL pointer
> > > > dereferences.
> > > > If the ring buffer is not yet allocated, return 0 bytes to be
> > > > read/written.
> > > >
> > > > The root cause is that code that accesses the ring buffer
> including
> > > > hv_get_bytes_to_read/write() could be vulnerable to the race
> > > > condition discussed in
> > > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F
> %2Flk
> > > >
> > ml.org%2Flkml%2F2018%2F10%2F18%2F779&data=02%7C01%7Chaiyangz
> > %40m
> > > >
> > icrosoft.com%7C73af013c14034bb0b1ad08d6a32b419c%7C72f988bf86f141af9
> > 1
> > > >
> > ab2d7cd011db47%7C1%7C0%7C636875803518430021&sdata=b51Xc5GUN
> > nHX0K
> > > > 08LrH3ShTyFcRZ4mYHUATd%2BDpvYDw%3D&reserved=0>;
> > > >
> > > > This race is being addressed by the patch series by Kimberly
> Brown
> > > > in
> > > > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F
> %2Flk
> > > >
> > ml.org%2Flkml%2F2019%2F2%2F21%2F1236&data=02%7C01%7Chaiyangz
> > %40m
> > > >
> > icrosoft.com%7C73af013c14034bb0b1ad08d6a32b419c%7C72f988bf86f141af9
> > 1
> > > >
> > ab2d7cd011db47%7C1%7C0%7C636875803518430021&sdata=js1ff15Gbk7
> > 0MD
> > > > A2hkMZExxvAAbDuKDhfBvc5ZrckzM%3D&reserved=0 which is not
> > final
> > > > yet
> > > >
> > > > Signed-off-by: Mohammed Gamal <mgamal@redhat.com>
> > >
> > > Could you elaborate on the code paths where
> > > hv_get_bytes_to_read/write() could be called when the ring buffer
> > > isn't yet allocated? My sense is that Kim Brown's patch will
> address
> > > all of the code paths that involved sysfs access from outside the
> > > driver. And within a driver, the ring buffer should never be
> accessed
> > > unless it is already allocated. Is there another code path we're
> not
> > > aware of? I'm wondering if these changes are really needed once
> Kim
> > > Brown's patch is finished.
> > >
> > > Michael
> >
> > I've seen one instance of the race in the netvsc driver when
> running traffic
> > through it with iperf3 while continuously changing the channel
> settings.
> >
> > The following code path deallocates the ring buffer:
> > netvsc_set_channels() -> netvsc_detach() ->
> > rndis_filter_device_remove() -> netvsc_device_remove() ->
> vmbus_close()
> > -> vmbus_free_ring() -> hv_ringbuffer_cleanup().
> >
> > netvsc_send_pkt() -> hv_get_bytes_to_write() might get called
> concurrently
> > after vmbus_close() and before vmbus_open() returns and sets up the
> new ring
> > buffer.
> >
> > The race is fairly hard to reproduce on recent upstream kernels,
> but I still
> > managed to reproduce it.
>
> Looking at the code from netvsc_detach() –
> netif_tx_disable(ndev) is called before
> rndis_filter_device_remove(hdev, nvdev).
> So there should be no call to netvsc_send_pkt() after detaching.
> What’s the crash stack trace?
>
> static int netvsc_detach(struct net_device *ndev,
> struct netvsc_device *nvdev)
> {
> struct net_device_context *ndev_ctx = netdev_priv(ndev);
> struct hv_device *hdev = ndev_ctx->device_ctx;
> int ret;
>
> /* Don't try continuing to try and setup sub channels */
> if (cancel_work_sync(&nvdev->subchan_work))
> nvdev->num_chn = 1;
>
> /* If device was up (receiving) then shutdown */
> if (netif_running(ndev)) {
> netif_tx_disable(ndev);
>
> ret = rndis_filter_close(nvdev);
> if (ret) {
> netdev_err(ndev,
> "unable to close device (ret
> %d).\n", ret);
> return ret;
> }
>
> ret = netvsc_wait_until_empty(nvdev);
> if (ret) {
> netdev_err(ndev,
> "Ring buffer not empty after
> closing rndis\n");
> return ret;
> }
> }
>
> netif_device_detach(ndev);
>
> rndis_filter_device_remove(hdev, nvdev);
>
> return 0;
> }
>
> Thanks,
> Haiyang
Here is one stack trace on a 4.18 kernel, the most recent kernel I
managed to reproduce this bug on.
I haven't managed to reproduce on 5.0.0 yet, but I guess some recent
changes to the netvsc driver could be masking the problem, as I tried
backporting those changes to older RHEL 7 kernels and still managed to
reproduce the problem there. I could however be wrong, and any pointers
are still appreciated:
[ 545.308507] BUG: unable to handle kernel NULL pointer dereference at
0000000000000004
[ 545.308656] PGD 0 P4D 0
[ 545.308763] Oops: 0000 [#1] SMP PTI
[ 545.308855] CPU: 2 PID: 1800 Comm: iperf3 Kdump: loaded Not tainted
4.18.0-64.el8.test.x86_64 #1
[ 545.308990] Hardware name: Microsoft Corporation Virtual
Machine/Virtual Machine, BIOS Hyper-V UEFI Release v1.0 11/26/2012
[ 545.309143] RIP: 0010:netvsc_send+0x2c9/0xce0 [hv_netvsc]
[ 545.309298] Code: 4c 8b b1 c0 00 00 00 4f 8d 2c 64 49 c1 e5 07 4d 03
ae c0 03 00 00 48 8b 84 03 30 01 00 00 4c 89 6c 24 18 48 8b 90 20 01 00
00 <8b> 72 04 8b 0a 8b 90 38 01 00 00 89 f7 01 f2 29 cf 29 ca 39 ce
0f
[ 545.309321] RSP: 0018:ffffb8a305d5b6c0 EFLAGS: 00010282
[ 545.309321] RAX: ffff926928bd7000 RBX: ffff92687dbe0000 RCX:
ffff92687d5bec00
[ 545.309321] RDX: 0000000000000000 RSI: ffff92691b61c654 RDI:
0000000000000000
[ 545.309321] RBP: ffff926915dcde28 R08: ffff926915dcde00 R09:
0000000000000000
[ 545.309321] R10: 00000000000db61c R11: 0000000000000f7e R12:
0000000000000001
[ 545.309321] R13: ffff926931808180 R14: ffff926931801000 R15:
0000000000000000
[ 545.309321] FS: 00007feca6a4b740(0000) GS:ffff926940080000(0000)
knlGS:0000000000000000
[ 545.309321] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 545.309321] CR2: 0000000000000004 CR3: 00000000dfccc004 CR4:
00000000003606e0
[ 545.309321] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[ 545.309321] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
0000000000000400
[ 545.309321] Call Trace:
[ 545.309321] netvsc_start_xmit+0x3c9/0x800 [hv_netvsc]
[ 545.309321] ? __switch_to_asm+0x34/0x70
[ 545.309321] ? __switch_to_asm+0x34/0x70
[ 545.309321] ? ___slab_alloc+0x269/0x4e0
[ 545.309321] ? __alloc_skb+0x82/0x1c0
[ 545.309321] ? nft_do_chain+0x3d7/0x3f0 [nf_tables]
[ 545.309321] ? nft_do_chain+0x3d7/0x3f0 [nf_tables]
[ 545.309321] ? nft_do_chain+0x3d7/0x3f0 [nf_tables]
[ 545.309321] ? _cond_resched+0x15/0x30
[ 545.309321] ? netif_skb_features+0x118/0x280
[ 545.309321] dev_hard_start_xmit+0xa5/0x210
[ 545.309321] sch_direct_xmit+0x14f/0x340
[ 545.309321] __dev_queue_xmit+0x799/0x8f0
[ 545.309321] ip_finish_output2+0x2e0/0x430
[ 545.309321] ? ip_finish_output+0x139/0x270
[ 545.309321] ip_output+0x6c/0xe0
[ 545.309321] ? ip_append_data.part.50+0xc0/0xc0
[ 545.309321] ip_send_skb+0x15/0x40
[ 545.309321] udp_send_skb.isra.43+0x153/0x340
[ 545.309321] udp_sendmsg+0xac2/0xd30
[ 545.309321] ? set_fd_set.part.7+0x40/0x40
[ 545.309321] ? set_fd_set.part.7+0x40/0x40
[ 545.309321] ? __check_object_size+0xa3/0x181
[ 545.309321] ? sock_has_perm+0x78/0xa0
[ 545.309321] ? core_sys_select+0x242/0x2f0
[ 545.309321] ? sock_sendmsg+0x36/0x40
[ 545.309321] ? udp_push_pending_frames+0x60/0x60
[ 545.309321] sock_sendmsg+0x36/0x40
[ 545.309321] sock_write_iter+0x8f/0xf0
[ 545.309321] __vfs_write+0x156/0x1a0
[ 545.309321] vfs_write+0xa5/0x1a0
[ 545.309321] ksys_write+0x4f/0xb0
[ 545.309321] do_syscall_64+0x5b/0x1b0
[ 545.309321] entry_SYSCALL_64_after_hwframe+0x65/0xca
[ 545.309321] RIP: 0033:0x7feca5fb5348
[ 545.309321] Code: 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00
00 f3 0f 1e fa 48 8d 05 d5 63 2d 00 8b 00 85 c0 75 17 b8 01 00 00 00 0f
05 <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 41 54 49 89 d4 55
[ 545.309321] RSP: 002b:00007ffc3ff1f108 EFLAGS: 00000246 ORIG_RAX:
0000000000000001
[ 545.309321] RAX: ffffffffffffffda RBX: 00000000000005a8 RCX:
00007feca5fb5348
[ 545.309321] RDX: 00000000000005a8 RSI: 00007feca6a59000 RDI:
0000000000000009
[ 545.309321] RBP: 00007feca6a59000 R08: 0000000000000002 R09:
00cd09a3238b4e43
[ 545.309321] R10: 0002961ecea49016 R11: 0000000000000246 R12:
0000000000000009
[ 545.309321] R13: 00000000000005a8 R14: 00007ffc3ff1f180 R15:
0000563c1e05b260
[ 545.309321] Modules linked in: nft_chain_nat_ipv6 nf_conntrack_ipv6
nf_defrag_ipv6 nf_nat_ipv6 nft_chain_route_ipv6 nft_chain_nat_ipv4
nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat
nft_chain_route_ipv4 nf_conntrack ip_set nf_tables nfnetlink vfat fat
sb_edac crct10dif_pclmul crc32_pclmul ghash_clmulni_intel
intel_rapl_perf sg hv_utils hv_balloon pcspkr joydev xfs libcrc32c
sd_mod sr_mod cdrom serio_raw hv_storvsc hv_netvsc scsi_transport_fc
hyperv_fb hyperv_keyboard hid_hyperv crc32c_intel hv_vmbus dm_mirror
dm_region_hash dm_log dm_mod [last unloaded: nft_compat]
[ 545.309321] CR2: 0000000000000004
From the stack trace netvsc_send+0x2c9 points to this line:
static inline u32 hv_get_bytes_to_write(const struct hv_ring_bu
ffer_info *rbi)
{
u32 read_loc, write_loc, dsize, write;
dsize = rbi->ring_datasize;
read_loc = READ_ONCE(rbi->ring_buffer->read_index); <---------
write_loc = rbi->ring_buffer->write_index;
write = write_loc >= read_loc ? dsize - (write_loc - read_loc)
read_loc - write_loc;
return write;
}
which gets called from netvsc_send_pkt().
^ permalink raw reply
* Re: [PATCH 1/2] Drivers: hv: Move Hyper-V clockevents code to new clocksource driver
From: Vitaly Kuznetsov @ 2019-03-13 8:28 UTC (permalink / raw)
To: Michael Kelley
Cc: catalin.marinas@arm.com, mark.rutland@arm.com,
will.deacon@arm.com, marc.zyngier@arm.com,
linux-arm-kernel@lists.infradead.org, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
olaf@aepfle.de, apw@canonical.com, jasowang@redhat.com,
marcelo.cerri@canonical.com, Sunil Muthuswamy, KY Srinivasan
In-Reply-To: <1552426813-9568-2-git-send-email-mikelley@microsoft.com>
Michael Kelley <mikelley@microsoft.com> writes:
> Clockevents code for Hyper-V synthetic timers is currently mixed
> in with other Hyper-V code. Move the code to a Hyper-V specific
> driver in the "clocksource" directory. Update the VMbus driver
> to call initialization and cleanup routines since the Hyper-V
> synthetic timers are not independently enumerated in ACPI.
>
I like the idea! Would it also make sense to consider moving Hyper-V
clocksource from arch/x86/hyperv/hv_init.c? The thing is that we use it
from arch-independent code (e.g. seee 'hyperv_cs' usage in
drivers/hv/hv_util.c).
> No behavior is changed and no new functionality is added.
>
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
> ---
> MAINTAINERS | 2 +
> arch/x86/include/asm/hyperv-tlfs.h | 6 +
> arch/x86/kernel/cpu/mshyperv.c | 2 +
> drivers/clocksource/Makefile | 1 +
> drivers/clocksource/hyperv_syntimer.c | 206 ++++++++++++++++++++++++++++++++++
> drivers/hv/hv.c | 154 -------------------------
> drivers/hv/hyperv_vmbus.h | 3 -
> drivers/hv/vmbus_drv.c | 39 +++----
> include/clocksource/hyperv_syntimer.h | 26 +++++
> 9 files changed, 263 insertions(+), 176 deletions(-)
> create mode 100644 drivers/clocksource/hyperv_syntimer.c
> create mode 100644 include/clocksource/hyperv_syntimer.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 21ab064..3352716 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7159,6 +7159,7 @@ F: arch/x86/include/asm/trace/hyperv.h
> F: arch/x86/include/asm/hyperv-tlfs.h
> F: arch/x86/kernel/cpu/mshyperv.c
> F: arch/x86/hyperv
> +F: drivers/clocksource/hyperv_syntimer.c
> F: drivers/hid/hid-hyperv.c
> F: drivers/hv/
> F: drivers/input/serio/hyperv-keyboard.c
> @@ -7168,6 +7169,7 @@ F: drivers/scsi/storvsc_drv.c
> F: drivers/uio/uio_hv_generic.c
> F: drivers/video/fbdev/hyperv_fb.c
> F: net/vmw_vsock/hyperv_transport.c
> +F: include/clocksource/hyperv_syntimer.h
Nitpicking:
This has nothing to do with your patch but we have a mix of 'stimer',
'timer', 'syntimer' names when we refer to Hyper-V Synthetic timers, it
may make sense to try to converge on something (stimer would probably be
my personal preference but I don't really care as long as we use the
same abbreviation). Examples:
hv_init_timer_config(...)
HV_MSR_SYNTIMER_AVAILABLE
HYPERV_STIMER0_VECTOR
hv_syntimer_init(...)
...
> F: include/linux/hyperv.h
> F: include/uapi/linux/hyperv.h
> F: tools/hv/
> diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
> index 2bdbbbc..ee62f57 100644
> --- a/arch/x86/include/asm/hyperv-tlfs.h
> +++ b/arch/x86/include/asm/hyperv-tlfs.h
> @@ -401,6 +401,12 @@ enum HV_GENERIC_SET_FORMAT {
> #define HV_STATUS_INVALID_CONNECTION_ID 18
> #define HV_STATUS_INSUFFICIENT_BUFFERS 19
>
> +/*
> + * The Hyper-V TimeRefCount register and the TSC
> + * page provide a guest VM clock with 100ns tick rate
> + */
> +#define HV_CLOCK_HZ (NSEC_PER_SEC/100)
> +
> typedef struct _HV_REFERENCE_TSC_PAGE {
> __u32 tsc_sequence;
> __u32 res1;
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index e81a2db..f53a35a 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -21,6 +21,7 @@
> #include <linux/irq.h>
> #include <linux/kexec.h>
> #include <linux/i8253.h>
> +#include <linux/random.h>
> #include <asm/processor.h>
> #include <asm/hypervisor.h>
> #include <asm/hyperv-tlfs.h>
> @@ -84,6 +85,7 @@ __visible void __irq_entry hv_stimer0_vector_handler(struct pt_regs *regs)
> inc_irq_stat(hyperv_stimer0_count);
> if (hv_stimer0_handler)
> hv_stimer0_handler();
> + add_interrupt_randomness(HYPERV_STIMER0_VECTOR, 0);
> ack_APIC_irq();
>
> exiting_irq();
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index be6e0fb..a887955 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -83,3 +83,4 @@ obj-$(CONFIG_ATCPIT100_TIMER) += timer-atcpit100.o
> obj-$(CONFIG_RISCV_TIMER) += timer-riscv.o
> obj-$(CONFIG_CSKY_MP_TIMER) += timer-mp-csky.o
> obj-$(CONFIG_GX6605S_TIMER) += timer-gx6605s.o
> +obj-$(CONFIG_HYPERV) += hyperv_syntimer.o
(just a couple of spare thoughs)
CONFIG_HYPERV can also be a module, are we OK with that? (we'll have to
support module loading/unloading then and honestly I see no reason for
that. I would prefer everything but VMBus devices to be in
kernel.) If we don't want it to be a module we can create a hidden
CONFIG_HYPERV_STIMER or something like that - just like we already do
for CONFIG_HYPERV_TSCPAGE.
There is, however, one additional dependency here: when running in
non-direct mode, Hyper-V clockevent devices require functional Hyper-V
messaging - which currently lives in VMBus code so that may explain why
you may want to keep stimer code in the same entity. Or, alternatively,
we can move Hyper-V messaging out of VMBus code (is it actually
architecture-agnostic?)
> diff --git a/drivers/clocksource/hyperv_syntimer.c b/drivers/clocksource/hyperv_syntimer.c
> new file mode 100644
> index 0000000..7276308
> --- /dev/null
> +++ b/drivers/clocksource/hyperv_syntimer.c
> @@ -0,0 +1,206 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Clocksource driver for the synthetic counter and timers
> + * provided by the Hyper-V hypervisor to guest VMs, as described
> + * in the Hyper-V Top Level Functional Spec (TLFS). This driver
> + * is instruction set architecture independent.
> + *
> + * Copyright (C) 2019, Microsoft, Inc.
> + *
> + * Author: Michael Kelley <mikelley@microsoft.com>
> + */
> +
> +#include <linux/percpu.h>
> +#include <linux/cpumask.h>
> +#include <linux/clockchips.h>
> +#include <linux/cpuhotplug.h>
> +#include <linux/mm.h>
> +#include <clocksource/hyperv_syntimer.h>
> +#include <asm/hyperv-tlfs.h>
> +#include <asm/mshyperv.h>
> +
> +static struct clock_event_device __percpu *hv_clock_event;
> +
> +/*
> + * If false, we're using the old mechanism for stimer0 interrupts
> + * where it sends a VMbus message when it expires. The old
> + * mechanism is used when running on older versions of Hyper-V
> + * that don't support Direct Mode. While Hyper-V provides
> + * four stimer's per CPU, Linux uses only stimer0.
> + */
> +static bool direct_mode_enabled;
> +
> +static int stimer0_irq;
> +static int stimer0_vector;
> +static int stimer0_message_sint;
> +static int stimer0_cpuhp_online;
> +
> +/*
> + * ISR for when stimer0 is operating in Direct Mode. Direct Mode
> + * does not use VMbus or any VMbus messages, so process here and not
> + * in the VMbus driver code.
> + */
> +void hv_stimer0_isr(void)
> +{
> + struct clock_event_device *ce;
> +
> + ce = this_cpu_ptr(hv_clock_event);
> + ce->event_handler(ce);
> +}
> +EXPORT_SYMBOL_GPL(hv_stimer0_isr);
> +
> +static int hv_ce_set_next_event(unsigned long delta,
> + struct clock_event_device *evt)
> +{
> + u64 current_tick;
> +
> + WARN_ON(!clockevent_state_oneshot(evt));
> +
> + current_tick = hyperv_cs->read(NULL);
> + current_tick += delta;
> + hv_init_timer(0, current_tick);
> + return 0;
> +}
> +
> +static int hv_ce_shutdown(struct clock_event_device *evt)
> +{
> + hv_init_timer(0, 0);
> + hv_init_timer_config(0, 0);
> + if (direct_mode_enabled)
> + hv_disable_stimer0_percpu_irq(stimer0_irq);
> +
> + return 0;
> +}
> +
> +static int hv_ce_set_oneshot(struct clock_event_device *evt)
> +{
> + union hv_stimer_config timer_cfg;
> +
> + timer_cfg.as_uint64 = 0;
> + timer_cfg.enable = 1;
> + timer_cfg.auto_enable = 1;
> + if (direct_mode_enabled) {
> + /*
> + * When it expires, the timer will directly interrupt
> + * on the specified hardware vector/IRQ.
> + */
> + timer_cfg.direct_mode = 1;
> + timer_cfg.apic_vector = stimer0_vector;
> + hv_enable_stimer0_percpu_irq(stimer0_irq);
> + } else {
> + /*
> + * When it expires, the timer will generate a VMbus message,
> + * to be handled by the normal VMbus interrupt handler.
> + */
> + timer_cfg.direct_mode = 0;
> + timer_cfg.sintx = stimer0_message_sint;
> + }
> + hv_init_timer_config(0, timer_cfg.as_uint64);
> + return 0;
> +}
> +
> +/*
> + * hv_syntimer_init - Per-cpu initialization of the clockevent
> + */
> +static int hv_syntimer_init(unsigned int cpu)
> +{
> + struct clock_event_device *ce;
> +
> + if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
> + ce = per_cpu_ptr(hv_clock_event, cpu);
> + ce->name = "Hyper-V clockevent";
> + ce->features = CLOCK_EVT_FEAT_ONESHOT;
> + ce->cpumask = cpumask_of(cpu);
> + ce->rating = 1000;
> + ce->set_state_shutdown = hv_ce_shutdown;
> + ce->set_state_oneshot = hv_ce_set_oneshot;
> + ce->set_next_event = hv_ce_set_next_event;
> +
> + clockevents_config_and_register(ce,
> + HV_CLOCK_HZ,
> + HV_MIN_DELTA_TICKS,
> + HV_MAX_MAX_DELTA_TICKS);
> + }
> + return 0;
> +}
> +
> +/*
> + * hv_syntimer_cleanup - Per-cpu cleanup of the clockevent
> + */
> +int hv_syntimer_cleanup(unsigned int cpu)
> +{
> + struct clock_event_device *ce;
> +
> + /* Turn off clockevent device */
> + if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
> + ce = per_cpu_ptr(hv_clock_event, cpu);
> + clockevents_unbind_device(ce, cpu);
> + hv_ce_shutdown(ce);
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(hv_syntimer_cleanup);
> +
> +/* hv_syntimer_alloc - Global initialization of the clockevent and stimer0 */
> +int hv_syntimer_alloc(int sint)
> +{
> + int ret;
> +
> + hv_clock_event = alloc_percpu(struct clock_event_device);
> + if (!hv_clock_event)
> + return -ENOMEM;
> +
> + direct_mode_enabled = ms_hyperv.misc_features &
> + HV_STIMER_DIRECT_MODE_AVAILABLE;
> + if (direct_mode_enabled &&
> + hv_setup_stimer0_irq(&stimer0_irq, &stimer0_vector,
> + hv_stimer0_isr))
> + goto err_irq;
> +
> + stimer0_message_sint = sint;
> +
> + ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hyperv/stimer0:online",
> + hv_syntimer_init, hv_syntimer_cleanup);
> + if (ret < 0)
> + goto err_cpuhp;
> + stimer0_cpuhp_online = ret;
> + return 0;
> +
> +err_cpuhp:
> + if (direct_mode_enabled)
> + hv_remove_stimer0_irq(stimer0_irq);
> +err_irq:
> + free_percpu(hv_clock_event);
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(hv_syntimer_alloc);
> +
> +/* hv_syntimer_free - Free global resources allocated by hv_syntimer_alloc() */
> +void hv_syntimer_free(void)
> +{
> + cpuhp_remove_state(stimer0_cpuhp_online);
> + if (direct_mode_enabled)
> + hv_remove_stimer0_irq(stimer0_irq);
> + free_percpu(hv_clock_event);
> +}
> +EXPORT_SYMBOL_GPL(hv_syntimer_free);
> +
> +/*
> + * Do a global cleanup of clockevents for the cases of kexec and
> + * vmbus exit
> + */
> +void hv_syntimer_global_cleanup(void)
> +{
> + int cpu;
> + struct clock_event_device *ce;
> +
> + if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE)
> + for_each_present_cpu(cpu) {
> + ce = per_cpu_ptr(hv_clock_event, cpu);
> + clockevents_unbind_device(ce, cpu);
> + }
> + hv_syntimer_free();
> +}
> +EXPORT_SYMBOL_GPL(hv_syntimer_global_cleanup);
> diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
> index 632d256..e3ee010 100644
> --- a/drivers/hv/hv.c
> +++ b/drivers/hv/hv.c
> @@ -36,21 +36,6 @@
> struct hv_context hv_context;
>
> /*
> - * If false, we're using the old mechanism for stimer0 interrupts
> - * where it sends a VMbus message when it expires. The old
> - * mechanism is used when running on older versions of Hyper-V
> - * that don't support Direct Mode. While Hyper-V provides
> - * four stimer's per CPU, Linux uses only stimer0.
> - */
> -static bool direct_mode_enabled;
> -static int stimer0_irq;
> -static int stimer0_vector;
> -
> -#define HV_TIMER_FREQUENCY (10 * 1000 * 1000) /* 100ns period */
> -#define HV_MAX_MAX_DELTA_TICKS 0xffffffff
> -#define HV_MIN_DELTA_TICKS 1
> -
> -/*
> * hv_init - Main initialization routine.
> *
> * This routine must be called before any other routines in here are called
> @@ -60,9 +45,6 @@ int hv_init(void)
> hv_context.cpu_context = alloc_percpu(struct hv_per_cpu_context);
> if (!hv_context.cpu_context)
> return -ENOMEM;
> -
> - direct_mode_enabled = ms_hyperv.misc_features &
> - HV_STIMER_DIRECT_MODE_AVAILABLE;
> return 0;
> }
>
> @@ -101,89 +83,6 @@ int hv_post_message(union hv_connection_id connection_id,
> return status & 0xFFFF;
> }
>
> -/*
> - * ISR for when stimer0 is operating in Direct Mode. Direct Mode
> - * does not use VMbus or any VMbus messages, so process here and not
> - * in the VMbus driver code.
> - */
> -
> -static void hv_stimer0_isr(void)
> -{
> - struct hv_per_cpu_context *hv_cpu;
> -
> - hv_cpu = this_cpu_ptr(hv_context.cpu_context);
> - hv_cpu->clk_evt->event_handler(hv_cpu->clk_evt);
> - add_interrupt_randomness(stimer0_vector, 0);
> -}
> -
> -static int hv_ce_set_next_event(unsigned long delta,
> - struct clock_event_device *evt)
> -{
> - u64 current_tick;
> -
> - WARN_ON(!clockevent_state_oneshot(evt));
> -
> - current_tick = hyperv_cs->read(NULL);
> - current_tick += delta;
> - hv_init_timer(0, current_tick);
> - return 0;
> -}
> -
> -static int hv_ce_shutdown(struct clock_event_device *evt)
> -{
> - hv_init_timer(0, 0);
> - hv_init_timer_config(0, 0);
> - if (direct_mode_enabled)
> - hv_disable_stimer0_percpu_irq(stimer0_irq);
> -
> - return 0;
> -}
> -
> -static int hv_ce_set_oneshot(struct clock_event_device *evt)
> -{
> - union hv_stimer_config timer_cfg;
> -
> - timer_cfg.as_uint64 = 0;
> - timer_cfg.enable = 1;
> - timer_cfg.auto_enable = 1;
> - if (direct_mode_enabled) {
> - /*
> - * When it expires, the timer will directly interrupt
> - * on the specified hardware vector/IRQ.
> - */
> - timer_cfg.direct_mode = 1;
> - timer_cfg.apic_vector = stimer0_vector;
> - hv_enable_stimer0_percpu_irq(stimer0_irq);
> - } else {
> - /*
> - * When it expires, the timer will generate a VMbus message,
> - * to be handled by the normal VMbus interrupt handler.
> - */
> - timer_cfg.direct_mode = 0;
> - timer_cfg.sintx = VMBUS_MESSAGE_SINT;
> - }
> - hv_init_timer_config(0, timer_cfg.as_uint64);
> - return 0;
> -}
> -
> -static void hv_init_clockevent_device(struct clock_event_device *dev, int cpu)
> -{
> - dev->name = "Hyper-V clockevent";
> - dev->features = CLOCK_EVT_FEAT_ONESHOT;
> - dev->cpumask = cpumask_of(cpu);
> - dev->rating = 1000;
> - /*
> - * Avoid settint dev->owner = THIS_MODULE deliberately as doing so will
> - * result in clockevents_config_and_register() taking additional
> - * references to the hv_vmbus module making it impossible to unload.
> - */
> -
> - dev->set_state_shutdown = hv_ce_shutdown;
> - dev->set_state_oneshot = hv_ce_set_oneshot;
> - dev->set_next_event = hv_ce_set_next_event;
> -}
> -
> -
> int hv_synic_alloc(void)
> {
> int cpu;
> @@ -212,14 +111,6 @@ int hv_synic_alloc(void)
> tasklet_init(&hv_cpu->msg_dpc,
> vmbus_on_msg_dpc, (unsigned long) hv_cpu);
>
> - hv_cpu->clk_evt = kzalloc(sizeof(struct clock_event_device),
> - GFP_KERNEL);
> - if (hv_cpu->clk_evt == NULL) {
> - pr_err("Unable to allocate clock event device\n");
> - goto err;
> - }
> - hv_init_clockevent_device(hv_cpu->clk_evt, cpu);
> -
> hv_cpu->synic_message_page =
> (void *)get_zeroed_page(GFP_ATOMIC);
> if (hv_cpu->synic_message_page == NULL) {
> @@ -242,11 +133,6 @@ int hv_synic_alloc(void)
> INIT_LIST_HEAD(&hv_cpu->chan_list);
> }
>
> - if (direct_mode_enabled &&
> - hv_setup_stimer0_irq(&stimer0_irq, &stimer0_vector,
> - hv_stimer0_isr))
> - goto err;
> -
> return 0;
> err:
> /*
> @@ -265,7 +151,6 @@ void hv_synic_free(void)
> struct hv_per_cpu_context *hv_cpu
> = per_cpu_ptr(hv_context.cpu_context, cpu);
>
> - kfree(hv_cpu->clk_evt);
> free_page((unsigned long)hv_cpu->synic_event_page);
> free_page((unsigned long)hv_cpu->synic_message_page);
> free_page((unsigned long)hv_cpu->post_msg_page);
> @@ -324,39 +209,10 @@ int hv_synic_init(unsigned int cpu)
>
> hv_set_synic_state(sctrl.as_uint64);
>
> - /*
> - * Register the per-cpu clockevent source.
> - */
> - if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE)
> - clockevents_config_and_register(hv_cpu->clk_evt,
> - HV_TIMER_FREQUENCY,
> - HV_MIN_DELTA_TICKS,
> - HV_MAX_MAX_DELTA_TICKS);
> return 0;
> }
>
> /*
> - * hv_synic_clockevents_cleanup - Cleanup clockevent devices
> - */
> -void hv_synic_clockevents_cleanup(void)
> -{
> - int cpu;
> -
> - if (!(ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE))
> - return;
> -
> - if (direct_mode_enabled)
> - hv_remove_stimer0_irq(stimer0_irq);
> -
> - for_each_present_cpu(cpu) {
> - struct hv_per_cpu_context *hv_cpu
> - = per_cpu_ptr(hv_context.cpu_context, cpu);
> -
> - clockevents_unbind_device(hv_cpu->clk_evt, cpu);
> - }
> -}
> -
> -/*
> * hv_synic_cleanup - Cleanup routine for hv_synic_init().
> */
> int hv_synic_cleanup(unsigned int cpu)
> @@ -401,16 +257,6 @@ int hv_synic_cleanup(unsigned int cpu)
> if (channel_found && vmbus_connection.conn_state == CONNECTED)
> return -EBUSY;
>
> - /* Turn off clockevent device */
> - if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
> - struct hv_per_cpu_context *hv_cpu
> - = this_cpu_ptr(hv_context.cpu_context);
> -
> - clockevents_unbind_device(hv_cpu->clk_evt, cpu);
> - hv_ce_shutdown(hv_cpu->clk_evt);
> - put_cpu_ptr(hv_cpu);
> - }
> -
> hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
>
> shared_sint.masked = 1;
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index cb86b133..ffd4ad8 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -151,7 +151,6 @@ struct hv_per_cpu_context {
> * per-cpu list of the channels based on their CPU affinity.
> */
> struct list_head chan_list;
> - struct clock_event_device *clk_evt;
> };
>
> struct hv_context {
> @@ -189,8 +188,6 @@ extern int hv_post_message(union hv_connection_id connection_id,
>
> extern int hv_synic_cleanup(unsigned int cpu);
>
> -extern void hv_synic_clockevents_cleanup(void);
> -
> /* Interface */
>
>
> diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
> index 000b53e..8e442c5 100644
> --- a/drivers/hv/vmbus_drv.c
> +++ b/drivers/hv/vmbus_drv.c
> @@ -43,6 +43,7 @@
> #include <linux/kdebug.h>
> #include <linux/efi.h>
> #include <linux/random.h>
> +#include <clocksource/hyperv_syntimer.h>
> #include "hyperv_vmbus.h"
>
> struct vmbus_dynid {
> @@ -939,17 +940,6 @@ static void vmbus_onmessage_work(struct work_struct *work)
> kfree(ctx);
> }
>
> -static void hv_process_timer_expiration(struct hv_message *msg,
> - struct hv_per_cpu_context *hv_cpu)
> -{
> - struct clock_event_device *dev = hv_cpu->clk_evt;
> -
> - if (dev->event_handler)
> - dev->event_handler(dev);
> -
> - vmbus_signal_eom(msg, HVMSG_TIMER_EXPIRED);
> -}
> -
> void vmbus_on_msg_dpc(unsigned long data)
> {
> struct hv_per_cpu_context *hv_cpu = (void *)data;
> @@ -1143,9 +1133,10 @@ static void vmbus_isr(void)
>
> /* Check if there are actual msgs to be processed */
> if (msg->header.message_type != HVMSG_NONE) {
> - if (msg->header.message_type == HVMSG_TIMER_EXPIRED)
> - hv_process_timer_expiration(msg, hv_cpu);
> - else
> + if (msg->header.message_type == HVMSG_TIMER_EXPIRED) {
> + hv_stimer0_isr();
> + vmbus_signal_eom(msg, HVMSG_TIMER_EXPIRED);
> + } else
> tasklet_schedule(&hv_cpu->msg_dpc);
> }
>
> @@ -1248,8 +1239,8 @@ static int vmbus_bus_init(void)
> if (ret)
> goto err_alloc;
> /*
> - * Initialize the per-cpu interrupt state and
> - * connect to the host.
> + * Initialize the per-cpu interrupt state and syntimer state.
> + * Then connect to the host.
> */
> ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online",
> hv_synic_init, hv_synic_cleanup);
> @@ -1257,6 +1248,10 @@ static int vmbus_bus_init(void)
> goto err_alloc;
> hyperv_cpuhp_online = ret;
>
> + ret = hv_syntimer_alloc(VMBUS_MESSAGE_SINT);
> + if (ret < 0)
> + goto err_syntimer_alloc;
> +
> ret = vmbus_connect();
> if (ret)
> goto err_connect;
> @@ -1301,6 +1296,8 @@ static int vmbus_bus_init(void)
> return 0;
>
> err_connect:
> + hv_syntimer_free();
> +err_syntimer_alloc:
> cpuhp_remove_state(hyperv_cpuhp_online);
> err_alloc:
> hv_synic_free();
> @@ -1971,7 +1968,7 @@ static int vmbus_acpi_add(struct acpi_device *device)
>
> static void hv_kexec_handler(void)
> {
> - hv_synic_clockevents_cleanup();
> + hv_syntimer_global_cleanup();
> vmbus_initiate_unload(false);
> vmbus_connection.conn_state = DISCONNECTED;
> /* Make sure conn_state is set as hv_synic_cleanup checks for it */
> @@ -1982,6 +1979,8 @@ static void hv_kexec_handler(void)
>
> static void hv_crash_handler(struct pt_regs *regs)
> {
> + int cpu;
> +
> vmbus_initiate_unload(true);
> /*
> * In crash handler we can't schedule synic cleanup for all CPUs,
> @@ -1989,7 +1988,9 @@ static void hv_crash_handler(struct pt_regs *regs)
> * for kdump.
> */
> vmbus_connection.conn_state = DISCONNECTED;
> - hv_synic_cleanup(smp_processor_id());
> + cpu = smp_processor_id();
> + hv_syntimer_cleanup(cpu);
> + hv_synic_cleanup(cpu);
> hyperv_cleanup();
> };
>
> @@ -2038,7 +2039,7 @@ static void __exit vmbus_exit(void)
> hv_remove_kexec_handler();
> hv_remove_crash_handler();
> vmbus_connection.conn_state = DISCONNECTED;
> - hv_synic_clockevents_cleanup();
> + hv_syntimer_global_cleanup();
> vmbus_disconnect();
> hv_remove_vmbus_irq();
> for_each_online_cpu(cpu) {
> diff --git a/include/clocksource/hyperv_syntimer.h b/include/clocksource/hyperv_syntimer.h
> new file mode 100644
> index 0000000..154138b
> --- /dev/null
> +++ b/include/clocksource/hyperv_syntimer.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +/*
> + * Definitions for the clocksource provided by the Hyper-V
> + * hypervisor to guest VMs, as described in the Hyper-V Top
> + * Level Functional Spec (TLFS).
> + *
> + * Copyright (C) 2019, Microsoft, Inc.
> + *
> + * Author: Michael Kelley <mikelley@microsoft.com>
> + */
> +
> +#ifndef __CLKSOURCE_HYPERV_SYNTIMER_H
> +#define __CLKSOURCE_HYPERV_SYNTIMER_H
> +
> +#define HV_MAX_MAX_DELTA_TICKS 0xffffffff
> +#define HV_MIN_DELTA_TICKS 1
> +
> +/* Routines called by the VMbus driver */
> +extern int hv_syntimer_alloc(int sint);
> +extern void hv_syntimer_free(void);
> +extern int hv_syntimer_cleanup(unsigned int cpu);
> +extern void hv_syntimer_global_cleanup(void);
> +extern void hv_stimer0_isr(void);
> +
> +#endif
--
Vitaly
^ permalink raw reply
* Re: [PATCH 0/2] Drivers: hv: Move Hyper-V clock/timer code to separate clocksource driver
From: gregkh @ 2019-03-12 22:01 UTC (permalink / raw)
To: Michael Kelley
Cc: will.deacon@arm.com, marc.zyngier@arm.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
olaf@aepfle.de, apw@canonical.com, vkuznets, jasowang@redhat.com,
marcelo.cerri@canonical.com, Sunil Muthuswamy, KY Srinivasan,
catalin.marinas@arm.com, mark.rutland@arm.com
In-Reply-To: <DM5PR2101MB091877CF62AD20D07D82F452D7490@DM5PR2101MB0918.namprd21.prod.outlook.com>
On Tue, Mar 12, 2019 at 09:53:28PM +0000, Michael Kelley wrote:
> From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org> Sent: Tuesday, March 12, 2019 2:47 PM
> > >
> > > Michael Kelley (2):
> > > Drivers: hv: Move Hyper-V clockevents code to new clocksource driver
> > > Drivers: hv: Move Hyper-V clocksource code to new clocksource driver
> >
> > You have two different patches that do different things, yet have the
> > same identical shortlog text :(
> >
> > That's not ok, and something that I reject for trivial patches, it
> > should never happen for a "real" patch as you don't do the same thing in
> > both of these patches.
>
> Hmmm. Not identical. The first patch is "clockevents" code, and the second
> patch is "clocksource" code.
Wow, that's not obvious at all, sorry. You still might want to make it
a bit more different :)
greg k-h
^ permalink raw reply
* RE: [PATCH 0/2] Drivers: hv: Move Hyper-V clock/timer code to separate clocksource driver
From: Michael Kelley @ 2019-03-12 21:53 UTC (permalink / raw)
To: gregkh@linuxfoundation.org
Cc: will.deacon@arm.com, marc.zyngier@arm.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
olaf@aepfle.de, apw@canonical.com, vkuznets, jasowang@redhat.com,
marcelo.cerri@canonical.com, Sunil Muthuswamy, KY Srinivasan,
catalin.marinas@arm.com, mark.rutland@arm.com
In-Reply-To: <20190312214652.GA15914@kroah.com>
From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org> Sent: Tuesday, March 12, 2019 2:47 PM
> >
> > Michael Kelley (2):
> > Drivers: hv: Move Hyper-V clockevents code to new clocksource driver
> > Drivers: hv: Move Hyper-V clocksource code to new clocksource driver
>
> You have two different patches that do different things, yet have the
> same identical shortlog text :(
>
> That's not ok, and something that I reject for trivial patches, it
> should never happen for a "real" patch as you don't do the same thing in
> both of these patches.
Hmmm. Not identical. The first patch is "clockevents" code, and the second
patch is "clocksource" code.
Michael
^ permalink raw reply
* Re: [PATCH 0/2] Drivers: hv: Move Hyper-V clock/timer code to separate clocksource driver
From: gregkh @ 2019-03-12 21:46 UTC (permalink / raw)
To: Michael Kelley
Cc: will.deacon@arm.com, marc.zyngier@arm.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
olaf@aepfle.de, apw@canonical.com, vkuznets, jasowang@redhat.com,
marcelo.cerri@canonical.com, Sunil Muthuswamy, KY Srinivasan,
catalin.marinas@arm.com, mark.rutland@arm.com
In-Reply-To: <1552426813-9568-1-git-send-email-mikelley@microsoft.com>
On Tue, Mar 12, 2019 at 09:42:09PM +0000, Michael Kelley wrote:
> This patch series moves Hyper-V clock/timer code to a separate Hyper-V
> clocksource driver. Previously, Hyper-V clock/timer code and data
> structures were mixed in with other Hyper-V code in the ISA independent
> drivers/hv code as well as in arch dependent code. The new Hyper-V
> clocksource driver is ISA independent, with a just few dependencies on
> arch specific functions. The patch series does not change any behavior
> or functionality -- it only reorganizes the existing code and fixes up
> the linkages. A few places outside of Hyper-V code are fixed up to use
> the new #include file structure.
>
> This restructuring is in response to Marc Zyngier's review comments
> on supporting Hyper-V running on ARM64, and is a good idea in general.
> It increases the amount of code shared between the x86 and ARM64
> architectures, and reduces the size of the new code for supporting
> Hyper-V on ARM64. A new version of the Hyper-V on ARM64 patches will
> follow once this clocksource restructuring is accepted.
>
> The code is currently diff'ed against Linux 5.0. I'll rebase
> to linux-next once 5.1-rc1 is available.
>
> Michael Kelley (2):
> Drivers: hv: Move Hyper-V clockevents code to new clocksource driver
> Drivers: hv: Move Hyper-V clocksource code to new clocksource driver
You have two different patches that do different things, yet have the
same identical shortlog text :(
That's not ok, and something that I reject for trivial patches, it
should never happen for a "real" patch as you don't do the same thing in
both of these patches.
thanks,
greg k-h
^ permalink raw reply
* [PATCH 1/2] Drivers: hv: Move Hyper-V clockevents code to new clocksource driver
From: Michael Kelley @ 2019-03-12 21:42 UTC (permalink / raw)
To: will.deacon@arm.com, marc.zyngier@arm.com,
linux-arm-kernel@lists.infradead.org, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
olaf@aepfle.de, apw@canonical.com, vkuznets, jasowang@redhat.com,
marcelo.cerri@canonical.com, Sunil Muthuswamy, KY Srinivasan
Cc: Michael Kelley, catalin.marinas@arm.com, mark.rutland@arm.com
In-Reply-To: <1552426813-9568-1-git-send-email-mikelley@microsoft.com>
Clockevents code for Hyper-V synthetic timers is currently mixed
in with other Hyper-V code. Move the code to a Hyper-V specific
driver in the "clocksource" directory. Update the VMbus driver
to call initialization and cleanup routines since the Hyper-V
synthetic timers are not independently enumerated in ACPI.
No behavior is changed and no new functionality is added.
Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
MAINTAINERS | 2 +
arch/x86/include/asm/hyperv-tlfs.h | 6 +
arch/x86/kernel/cpu/mshyperv.c | 2 +
drivers/clocksource/Makefile | 1 +
drivers/clocksource/hyperv_syntimer.c | 206 ++++++++++++++++++++++++++++++++++
drivers/hv/hv.c | 154 -------------------------
drivers/hv/hyperv_vmbus.h | 3 -
drivers/hv/vmbus_drv.c | 39 +++----
include/clocksource/hyperv_syntimer.h | 26 +++++
9 files changed, 263 insertions(+), 176 deletions(-)
create mode 100644 drivers/clocksource/hyperv_syntimer.c
create mode 100644 include/clocksource/hyperv_syntimer.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 21ab064..3352716 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7159,6 +7159,7 @@ F: arch/x86/include/asm/trace/hyperv.h
F: arch/x86/include/asm/hyperv-tlfs.h
F: arch/x86/kernel/cpu/mshyperv.c
F: arch/x86/hyperv
+F: drivers/clocksource/hyperv_syntimer.c
F: drivers/hid/hid-hyperv.c
F: drivers/hv/
F: drivers/input/serio/hyperv-keyboard.c
@@ -7168,6 +7169,7 @@ F: drivers/scsi/storvsc_drv.c
F: drivers/uio/uio_hv_generic.c
F: drivers/video/fbdev/hyperv_fb.c
F: net/vmw_vsock/hyperv_transport.c
+F: include/clocksource/hyperv_syntimer.h
F: include/linux/hyperv.h
F: include/uapi/linux/hyperv.h
F: tools/hv/
diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
index 2bdbbbc..ee62f57 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -401,6 +401,12 @@ enum HV_GENERIC_SET_FORMAT {
#define HV_STATUS_INVALID_CONNECTION_ID 18
#define HV_STATUS_INSUFFICIENT_BUFFERS 19
+/*
+ * The Hyper-V TimeRefCount register and the TSC
+ * page provide a guest VM clock with 100ns tick rate
+ */
+#define HV_CLOCK_HZ (NSEC_PER_SEC/100)
+
typedef struct _HV_REFERENCE_TSC_PAGE {
__u32 tsc_sequence;
__u32 res1;
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index e81a2db..f53a35a 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -21,6 +21,7 @@
#include <linux/irq.h>
#include <linux/kexec.h>
#include <linux/i8253.h>
+#include <linux/random.h>
#include <asm/processor.h>
#include <asm/hypervisor.h>
#include <asm/hyperv-tlfs.h>
@@ -84,6 +85,7 @@ __visible void __irq_entry hv_stimer0_vector_handler(struct pt_regs *regs)
inc_irq_stat(hyperv_stimer0_count);
if (hv_stimer0_handler)
hv_stimer0_handler();
+ add_interrupt_randomness(HYPERV_STIMER0_VECTOR, 0);
ack_APIC_irq();
exiting_irq();
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index be6e0fb..a887955 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -83,3 +83,4 @@ obj-$(CONFIG_ATCPIT100_TIMER) += timer-atcpit100.o
obj-$(CONFIG_RISCV_TIMER) += timer-riscv.o
obj-$(CONFIG_CSKY_MP_TIMER) += timer-mp-csky.o
obj-$(CONFIG_GX6605S_TIMER) += timer-gx6605s.o
+obj-$(CONFIG_HYPERV) += hyperv_syntimer.o
diff --git a/drivers/clocksource/hyperv_syntimer.c b/drivers/clocksource/hyperv_syntimer.c
new file mode 100644
index 0000000..7276308
--- /dev/null
+++ b/drivers/clocksource/hyperv_syntimer.c
@@ -0,0 +1,206 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Clocksource driver for the synthetic counter and timers
+ * provided by the Hyper-V hypervisor to guest VMs, as described
+ * in the Hyper-V Top Level Functional Spec (TLFS). This driver
+ * is instruction set architecture independent.
+ *
+ * Copyright (C) 2019, Microsoft, Inc.
+ *
+ * Author: Michael Kelley <mikelley@microsoft.com>
+ */
+
+#include <linux/percpu.h>
+#include <linux/cpumask.h>
+#include <linux/clockchips.h>
+#include <linux/cpuhotplug.h>
+#include <linux/mm.h>
+#include <clocksource/hyperv_syntimer.h>
+#include <asm/hyperv-tlfs.h>
+#include <asm/mshyperv.h>
+
+static struct clock_event_device __percpu *hv_clock_event;
+
+/*
+ * If false, we're using the old mechanism for stimer0 interrupts
+ * where it sends a VMbus message when it expires. The old
+ * mechanism is used when running on older versions of Hyper-V
+ * that don't support Direct Mode. While Hyper-V provides
+ * four stimer's per CPU, Linux uses only stimer0.
+ */
+static bool direct_mode_enabled;
+
+static int stimer0_irq;
+static int stimer0_vector;
+static int stimer0_message_sint;
+static int stimer0_cpuhp_online;
+
+/*
+ * ISR for when stimer0 is operating in Direct Mode. Direct Mode
+ * does not use VMbus or any VMbus messages, so process here and not
+ * in the VMbus driver code.
+ */
+void hv_stimer0_isr(void)
+{
+ struct clock_event_device *ce;
+
+ ce = this_cpu_ptr(hv_clock_event);
+ ce->event_handler(ce);
+}
+EXPORT_SYMBOL_GPL(hv_stimer0_isr);
+
+static int hv_ce_set_next_event(unsigned long delta,
+ struct clock_event_device *evt)
+{
+ u64 current_tick;
+
+ WARN_ON(!clockevent_state_oneshot(evt));
+
+ current_tick = hyperv_cs->read(NULL);
+ current_tick += delta;
+ hv_init_timer(0, current_tick);
+ return 0;
+}
+
+static int hv_ce_shutdown(struct clock_event_device *evt)
+{
+ hv_init_timer(0, 0);
+ hv_init_timer_config(0, 0);
+ if (direct_mode_enabled)
+ hv_disable_stimer0_percpu_irq(stimer0_irq);
+
+ return 0;
+}
+
+static int hv_ce_set_oneshot(struct clock_event_device *evt)
+{
+ union hv_stimer_config timer_cfg;
+
+ timer_cfg.as_uint64 = 0;
+ timer_cfg.enable = 1;
+ timer_cfg.auto_enable = 1;
+ if (direct_mode_enabled) {
+ /*
+ * When it expires, the timer will directly interrupt
+ * on the specified hardware vector/IRQ.
+ */
+ timer_cfg.direct_mode = 1;
+ timer_cfg.apic_vector = stimer0_vector;
+ hv_enable_stimer0_percpu_irq(stimer0_irq);
+ } else {
+ /*
+ * When it expires, the timer will generate a VMbus message,
+ * to be handled by the normal VMbus interrupt handler.
+ */
+ timer_cfg.direct_mode = 0;
+ timer_cfg.sintx = stimer0_message_sint;
+ }
+ hv_init_timer_config(0, timer_cfg.as_uint64);
+ return 0;
+}
+
+/*
+ * hv_syntimer_init - Per-cpu initialization of the clockevent
+ */
+static int hv_syntimer_init(unsigned int cpu)
+{
+ struct clock_event_device *ce;
+
+ if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
+ ce = per_cpu_ptr(hv_clock_event, cpu);
+ ce->name = "Hyper-V clockevent";
+ ce->features = CLOCK_EVT_FEAT_ONESHOT;
+ ce->cpumask = cpumask_of(cpu);
+ ce->rating = 1000;
+ ce->set_state_shutdown = hv_ce_shutdown;
+ ce->set_state_oneshot = hv_ce_set_oneshot;
+ ce->set_next_event = hv_ce_set_next_event;
+
+ clockevents_config_and_register(ce,
+ HV_CLOCK_HZ,
+ HV_MIN_DELTA_TICKS,
+ HV_MAX_MAX_DELTA_TICKS);
+ }
+ return 0;
+}
+
+/*
+ * hv_syntimer_cleanup - Per-cpu cleanup of the clockevent
+ */
+int hv_syntimer_cleanup(unsigned int cpu)
+{
+ struct clock_event_device *ce;
+
+ /* Turn off clockevent device */
+ if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
+ ce = per_cpu_ptr(hv_clock_event, cpu);
+ clockevents_unbind_device(ce, cpu);
+ hv_ce_shutdown(ce);
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(hv_syntimer_cleanup);
+
+/* hv_syntimer_alloc - Global initialization of the clockevent and stimer0 */
+int hv_syntimer_alloc(int sint)
+{
+ int ret;
+
+ hv_clock_event = alloc_percpu(struct clock_event_device);
+ if (!hv_clock_event)
+ return -ENOMEM;
+
+ direct_mode_enabled = ms_hyperv.misc_features &
+ HV_STIMER_DIRECT_MODE_AVAILABLE;
+ if (direct_mode_enabled &&
+ hv_setup_stimer0_irq(&stimer0_irq, &stimer0_vector,
+ hv_stimer0_isr))
+ goto err_irq;
+
+ stimer0_message_sint = sint;
+
+ ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hyperv/stimer0:online",
+ hv_syntimer_init, hv_syntimer_cleanup);
+ if (ret < 0)
+ goto err_cpuhp;
+ stimer0_cpuhp_online = ret;
+ return 0;
+
+err_cpuhp:
+ if (direct_mode_enabled)
+ hv_remove_stimer0_irq(stimer0_irq);
+err_irq:
+ free_percpu(hv_clock_event);
+ return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(hv_syntimer_alloc);
+
+/* hv_syntimer_free - Free global resources allocated by hv_syntimer_alloc() */
+void hv_syntimer_free(void)
+{
+ cpuhp_remove_state(stimer0_cpuhp_online);
+ if (direct_mode_enabled)
+ hv_remove_stimer0_irq(stimer0_irq);
+ free_percpu(hv_clock_event);
+}
+EXPORT_SYMBOL_GPL(hv_syntimer_free);
+
+/*
+ * Do a global cleanup of clockevents for the cases of kexec and
+ * vmbus exit
+ */
+void hv_syntimer_global_cleanup(void)
+{
+ int cpu;
+ struct clock_event_device *ce;
+
+ if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE)
+ for_each_present_cpu(cpu) {
+ ce = per_cpu_ptr(hv_clock_event, cpu);
+ clockevents_unbind_device(ce, cpu);
+ }
+ hv_syntimer_free();
+}
+EXPORT_SYMBOL_GPL(hv_syntimer_global_cleanup);
diff --git a/drivers/hv/hv.c b/drivers/hv/hv.c
index 632d256..e3ee010 100644
--- a/drivers/hv/hv.c
+++ b/drivers/hv/hv.c
@@ -36,21 +36,6 @@
struct hv_context hv_context;
/*
- * If false, we're using the old mechanism for stimer0 interrupts
- * where it sends a VMbus message when it expires. The old
- * mechanism is used when running on older versions of Hyper-V
- * that don't support Direct Mode. While Hyper-V provides
- * four stimer's per CPU, Linux uses only stimer0.
- */
-static bool direct_mode_enabled;
-static int stimer0_irq;
-static int stimer0_vector;
-
-#define HV_TIMER_FREQUENCY (10 * 1000 * 1000) /* 100ns period */
-#define HV_MAX_MAX_DELTA_TICKS 0xffffffff
-#define HV_MIN_DELTA_TICKS 1
-
-/*
* hv_init - Main initialization routine.
*
* This routine must be called before any other routines in here are called
@@ -60,9 +45,6 @@ int hv_init(void)
hv_context.cpu_context = alloc_percpu(struct hv_per_cpu_context);
if (!hv_context.cpu_context)
return -ENOMEM;
-
- direct_mode_enabled = ms_hyperv.misc_features &
- HV_STIMER_DIRECT_MODE_AVAILABLE;
return 0;
}
@@ -101,89 +83,6 @@ int hv_post_message(union hv_connection_id connection_id,
return status & 0xFFFF;
}
-/*
- * ISR for when stimer0 is operating in Direct Mode. Direct Mode
- * does not use VMbus or any VMbus messages, so process here and not
- * in the VMbus driver code.
- */
-
-static void hv_stimer0_isr(void)
-{
- struct hv_per_cpu_context *hv_cpu;
-
- hv_cpu = this_cpu_ptr(hv_context.cpu_context);
- hv_cpu->clk_evt->event_handler(hv_cpu->clk_evt);
- add_interrupt_randomness(stimer0_vector, 0);
-}
-
-static int hv_ce_set_next_event(unsigned long delta,
- struct clock_event_device *evt)
-{
- u64 current_tick;
-
- WARN_ON(!clockevent_state_oneshot(evt));
-
- current_tick = hyperv_cs->read(NULL);
- current_tick += delta;
- hv_init_timer(0, current_tick);
- return 0;
-}
-
-static int hv_ce_shutdown(struct clock_event_device *evt)
-{
- hv_init_timer(0, 0);
- hv_init_timer_config(0, 0);
- if (direct_mode_enabled)
- hv_disable_stimer0_percpu_irq(stimer0_irq);
-
- return 0;
-}
-
-static int hv_ce_set_oneshot(struct clock_event_device *evt)
-{
- union hv_stimer_config timer_cfg;
-
- timer_cfg.as_uint64 = 0;
- timer_cfg.enable = 1;
- timer_cfg.auto_enable = 1;
- if (direct_mode_enabled) {
- /*
- * When it expires, the timer will directly interrupt
- * on the specified hardware vector/IRQ.
- */
- timer_cfg.direct_mode = 1;
- timer_cfg.apic_vector = stimer0_vector;
- hv_enable_stimer0_percpu_irq(stimer0_irq);
- } else {
- /*
- * When it expires, the timer will generate a VMbus message,
- * to be handled by the normal VMbus interrupt handler.
- */
- timer_cfg.direct_mode = 0;
- timer_cfg.sintx = VMBUS_MESSAGE_SINT;
- }
- hv_init_timer_config(0, timer_cfg.as_uint64);
- return 0;
-}
-
-static void hv_init_clockevent_device(struct clock_event_device *dev, int cpu)
-{
- dev->name = "Hyper-V clockevent";
- dev->features = CLOCK_EVT_FEAT_ONESHOT;
- dev->cpumask = cpumask_of(cpu);
- dev->rating = 1000;
- /*
- * Avoid settint dev->owner = THIS_MODULE deliberately as doing so will
- * result in clockevents_config_and_register() taking additional
- * references to the hv_vmbus module making it impossible to unload.
- */
-
- dev->set_state_shutdown = hv_ce_shutdown;
- dev->set_state_oneshot = hv_ce_set_oneshot;
- dev->set_next_event = hv_ce_set_next_event;
-}
-
-
int hv_synic_alloc(void)
{
int cpu;
@@ -212,14 +111,6 @@ int hv_synic_alloc(void)
tasklet_init(&hv_cpu->msg_dpc,
vmbus_on_msg_dpc, (unsigned long) hv_cpu);
- hv_cpu->clk_evt = kzalloc(sizeof(struct clock_event_device),
- GFP_KERNEL);
- if (hv_cpu->clk_evt == NULL) {
- pr_err("Unable to allocate clock event device\n");
- goto err;
- }
- hv_init_clockevent_device(hv_cpu->clk_evt, cpu);
-
hv_cpu->synic_message_page =
(void *)get_zeroed_page(GFP_ATOMIC);
if (hv_cpu->synic_message_page == NULL) {
@@ -242,11 +133,6 @@ int hv_synic_alloc(void)
INIT_LIST_HEAD(&hv_cpu->chan_list);
}
- if (direct_mode_enabled &&
- hv_setup_stimer0_irq(&stimer0_irq, &stimer0_vector,
- hv_stimer0_isr))
- goto err;
-
return 0;
err:
/*
@@ -265,7 +151,6 @@ void hv_synic_free(void)
struct hv_per_cpu_context *hv_cpu
= per_cpu_ptr(hv_context.cpu_context, cpu);
- kfree(hv_cpu->clk_evt);
free_page((unsigned long)hv_cpu->synic_event_page);
free_page((unsigned long)hv_cpu->synic_message_page);
free_page((unsigned long)hv_cpu->post_msg_page);
@@ -324,39 +209,10 @@ int hv_synic_init(unsigned int cpu)
hv_set_synic_state(sctrl.as_uint64);
- /*
- * Register the per-cpu clockevent source.
- */
- if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE)
- clockevents_config_and_register(hv_cpu->clk_evt,
- HV_TIMER_FREQUENCY,
- HV_MIN_DELTA_TICKS,
- HV_MAX_MAX_DELTA_TICKS);
return 0;
}
/*
- * hv_synic_clockevents_cleanup - Cleanup clockevent devices
- */
-void hv_synic_clockevents_cleanup(void)
-{
- int cpu;
-
- if (!(ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE))
- return;
-
- if (direct_mode_enabled)
- hv_remove_stimer0_irq(stimer0_irq);
-
- for_each_present_cpu(cpu) {
- struct hv_per_cpu_context *hv_cpu
- = per_cpu_ptr(hv_context.cpu_context, cpu);
-
- clockevents_unbind_device(hv_cpu->clk_evt, cpu);
- }
-}
-
-/*
* hv_synic_cleanup - Cleanup routine for hv_synic_init().
*/
int hv_synic_cleanup(unsigned int cpu)
@@ -401,16 +257,6 @@ int hv_synic_cleanup(unsigned int cpu)
if (channel_found && vmbus_connection.conn_state == CONNECTED)
return -EBUSY;
- /* Turn off clockevent device */
- if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
- struct hv_per_cpu_context *hv_cpu
- = this_cpu_ptr(hv_context.cpu_context);
-
- clockevents_unbind_device(hv_cpu->clk_evt, cpu);
- hv_ce_shutdown(hv_cpu->clk_evt);
- put_cpu_ptr(hv_cpu);
- }
-
hv_get_synint_state(VMBUS_MESSAGE_SINT, shared_sint.as_uint64);
shared_sint.masked = 1;
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index cb86b133..ffd4ad8 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -151,7 +151,6 @@ struct hv_per_cpu_context {
* per-cpu list of the channels based on their CPU affinity.
*/
struct list_head chan_list;
- struct clock_event_device *clk_evt;
};
struct hv_context {
@@ -189,8 +188,6 @@ extern int hv_post_message(union hv_connection_id connection_id,
extern int hv_synic_cleanup(unsigned int cpu);
-extern void hv_synic_clockevents_cleanup(void);
-
/* Interface */
diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c
index 000b53e..8e442c5 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -43,6 +43,7 @@
#include <linux/kdebug.h>
#include <linux/efi.h>
#include <linux/random.h>
+#include <clocksource/hyperv_syntimer.h>
#include "hyperv_vmbus.h"
struct vmbus_dynid {
@@ -939,17 +940,6 @@ static void vmbus_onmessage_work(struct work_struct *work)
kfree(ctx);
}
-static void hv_process_timer_expiration(struct hv_message *msg,
- struct hv_per_cpu_context *hv_cpu)
-{
- struct clock_event_device *dev = hv_cpu->clk_evt;
-
- if (dev->event_handler)
- dev->event_handler(dev);
-
- vmbus_signal_eom(msg, HVMSG_TIMER_EXPIRED);
-}
-
void vmbus_on_msg_dpc(unsigned long data)
{
struct hv_per_cpu_context *hv_cpu = (void *)data;
@@ -1143,9 +1133,10 @@ static void vmbus_isr(void)
/* Check if there are actual msgs to be processed */
if (msg->header.message_type != HVMSG_NONE) {
- if (msg->header.message_type == HVMSG_TIMER_EXPIRED)
- hv_process_timer_expiration(msg, hv_cpu);
- else
+ if (msg->header.message_type == HVMSG_TIMER_EXPIRED) {
+ hv_stimer0_isr();
+ vmbus_signal_eom(msg, HVMSG_TIMER_EXPIRED);
+ } else
tasklet_schedule(&hv_cpu->msg_dpc);
}
@@ -1248,8 +1239,8 @@ static int vmbus_bus_init(void)
if (ret)
goto err_alloc;
/*
- * Initialize the per-cpu interrupt state and
- * connect to the host.
+ * Initialize the per-cpu interrupt state and syntimer state.
+ * Then connect to the host.
*/
ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "hyperv/vmbus:online",
hv_synic_init, hv_synic_cleanup);
@@ -1257,6 +1248,10 @@ static int vmbus_bus_init(void)
goto err_alloc;
hyperv_cpuhp_online = ret;
+ ret = hv_syntimer_alloc(VMBUS_MESSAGE_SINT);
+ if (ret < 0)
+ goto err_syntimer_alloc;
+
ret = vmbus_connect();
if (ret)
goto err_connect;
@@ -1301,6 +1296,8 @@ static int vmbus_bus_init(void)
return 0;
err_connect:
+ hv_syntimer_free();
+err_syntimer_alloc:
cpuhp_remove_state(hyperv_cpuhp_online);
err_alloc:
hv_synic_free();
@@ -1971,7 +1968,7 @@ static int vmbus_acpi_add(struct acpi_device *device)
static void hv_kexec_handler(void)
{
- hv_synic_clockevents_cleanup();
+ hv_syntimer_global_cleanup();
vmbus_initiate_unload(false);
vmbus_connection.conn_state = DISCONNECTED;
/* Make sure conn_state is set as hv_synic_cleanup checks for it */
@@ -1982,6 +1979,8 @@ static void hv_kexec_handler(void)
static void hv_crash_handler(struct pt_regs *regs)
{
+ int cpu;
+
vmbus_initiate_unload(true);
/*
* In crash handler we can't schedule synic cleanup for all CPUs,
@@ -1989,7 +1988,9 @@ static void hv_crash_handler(struct pt_regs *regs)
* for kdump.
*/
vmbus_connection.conn_state = DISCONNECTED;
- hv_synic_cleanup(smp_processor_id());
+ cpu = smp_processor_id();
+ hv_syntimer_cleanup(cpu);
+ hv_synic_cleanup(cpu);
hyperv_cleanup();
};
@@ -2038,7 +2039,7 @@ static void __exit vmbus_exit(void)
hv_remove_kexec_handler();
hv_remove_crash_handler();
vmbus_connection.conn_state = DISCONNECTED;
- hv_synic_clockevents_cleanup();
+ hv_syntimer_global_cleanup();
vmbus_disconnect();
hv_remove_vmbus_irq();
for_each_online_cpu(cpu) {
diff --git a/include/clocksource/hyperv_syntimer.h b/include/clocksource/hyperv_syntimer.h
new file mode 100644
index 0000000..154138b
--- /dev/null
+++ b/include/clocksource/hyperv_syntimer.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Definitions for the clocksource provided by the Hyper-V
+ * hypervisor to guest VMs, as described in the Hyper-V Top
+ * Level Functional Spec (TLFS).
+ *
+ * Copyright (C) 2019, Microsoft, Inc.
+ *
+ * Author: Michael Kelley <mikelley@microsoft.com>
+ */
+
+#ifndef __CLKSOURCE_HYPERV_SYNTIMER_H
+#define __CLKSOURCE_HYPERV_SYNTIMER_H
+
+#define HV_MAX_MAX_DELTA_TICKS 0xffffffff
+#define HV_MIN_DELTA_TICKS 1
+
+/* Routines called by the VMbus driver */
+extern int hv_syntimer_alloc(int sint);
+extern void hv_syntimer_free(void);
+extern int hv_syntimer_cleanup(unsigned int cpu);
+extern void hv_syntimer_global_cleanup(void);
+extern void hv_stimer0_isr(void);
+
+#endif
--
1.8.3.1
^ permalink raw reply related
* [PATCH 2/2] Drivers: hv: Move Hyper-V clocksource code to new clocksource driver
From: Michael Kelley @ 2019-03-12 21:42 UTC (permalink / raw)
To: will.deacon@arm.com, marc.zyngier@arm.com,
linux-arm-kernel@lists.infradead.org, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
olaf@aepfle.de, apw@canonical.com, vkuznets, jasowang@redhat.com,
marcelo.cerri@canonical.com, Sunil Muthuswamy, KY Srinivasan
Cc: Michael Kelley, catalin.marinas@arm.com, mark.rutland@arm.com
In-Reply-To: <1552426813-9568-1-git-send-email-mikelley@microsoft.com>
Code for the Hyper-V specific clocksources is currently mixed
in with other Hyper-V code. Move the code to a Hyper-V specific
driver in the "clocksource" directory, while separating out
ISA dependencies so that the new clocksource driver is ISA
independent. Update the Hyper-V initialization code to call
initialization and cleanup routines since the Hyper-V synthetic
timers are not independently enumerated in ACPI. Update Hyper-V
clocksource users KVM and VDSO to get definitions from a new
include file.
No behavior is changed and no new functionality is added.
Signed-off-by: Michael Kelley <mikelley@microsoft.com>
---
arch/x86/entry/vdso/vclock_gettime.c | 1 +
arch/x86/entry/vdso/vma.c | 2 +-
arch/x86/hyperv/hv_init.c | 91 ++-----------------------
arch/x86/include/asm/mshyperv.h | 80 +++-------------------
arch/x86/kvm/x86.c | 1 +
drivers/clocksource/hyperv_syntimer.c | 122 ++++++++++++++++++++++++++++++++++
include/clocksource/hyperv_syntimer.h | 78 ++++++++++++++++++++++
7 files changed, 219 insertions(+), 156 deletions(-)
diff --git a/arch/x86/entry/vdso/vclock_gettime.c b/arch/x86/entry/vdso/vclock_gettime.c
index 007b3fe9..b0de2a2 100644
--- a/arch/x86/entry/vdso/vclock_gettime.c
+++ b/arch/x86/entry/vdso/vclock_gettime.c
@@ -21,6 +21,7 @@
#include <linux/math64.h>
#include <linux/time.h>
#include <linux/kernel.h>
+#include <clocksource/hyperv_syntimer.h>
#define gtod (&VVAR(vsyscall_gtod_data))
diff --git a/arch/x86/entry/vdso/vma.c b/arch/x86/entry/vdso/vma.c
index babc4e7..8b81c91 100644
--- a/arch/x86/entry/vdso/vma.c
+++ b/arch/x86/entry/vdso/vma.c
@@ -22,7 +22,7 @@
#include <asm/page.h>
#include <asm/desc.h>
#include <asm/cpufeature.h>
-#include <asm/mshyperv.h>
+#include <clocksource/hyperv_syntimer.h>
#if defined(CONFIG_X86_64)
unsigned int __read_mostly vdso64_enabled = 1;
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 7abb09e..eb80f65 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -27,64 +27,13 @@
#include <linux/version.h>
#include <linux/vmalloc.h>
#include <linux/mm.h>
-#include <linux/clockchips.h>
#include <linux/hyperv.h>
#include <linux/slab.h>
#include <linux/cpuhotplug.h>
-
-#ifdef CONFIG_HYPERV_TSCPAGE
-
-static struct ms_hyperv_tsc_page *tsc_pg;
-
-struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
-{
- return tsc_pg;
-}
-EXPORT_SYMBOL_GPL(hv_get_tsc_page);
-
-static u64 read_hv_clock_tsc(struct clocksource *arg)
-{
- u64 current_tick = hv_read_tsc_page(tsc_pg);
-
- if (current_tick == U64_MAX)
- rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
-
- return current_tick;
-}
-
-static struct clocksource hyperv_cs_tsc = {
- .name = "hyperv_clocksource_tsc_page",
- .rating = 400,
- .read = read_hv_clock_tsc,
- .mask = CLOCKSOURCE_MASK(64),
- .flags = CLOCK_SOURCE_IS_CONTINUOUS,
-};
-#endif
-
-static u64 read_hv_clock_msr(struct clocksource *arg)
-{
- u64 current_tick;
- /*
- * Read the partition counter to get the current tick count. This count
- * is set to 0 when the partition is created and is incremented in
- * 100 nanosecond units.
- */
- rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
- return current_tick;
-}
-
-static struct clocksource hyperv_cs_msr = {
- .name = "hyperv_clocksource_msr",
- .rating = 400,
- .read = read_hv_clock_msr,
- .mask = CLOCKSOURCE_MASK(64),
- .flags = CLOCK_SOURCE_IS_CONTINUOUS,
-};
+#include <clocksource/hyperv_syntimer.h>
void *hv_hypercall_pg;
EXPORT_SYMBOL_GPL(hv_hypercall_pg);
-struct clocksource *hyperv_cs;
-EXPORT_SYMBOL_GPL(hyperv_cs);
u32 *hv_vp_index;
EXPORT_SYMBOL_GPL(hv_vp_index);
@@ -349,41 +298,11 @@ void __init hyperv_init(void)
x86_init.pci.arch_init = hv_pci_init;
/*
- * Register Hyper-V specific clocksource.
+ * Register Hyper-V specific clocksource. Pass 'false' as the
+ * arguemnt, indicating to not register the clocksource as the
+ * sched clock.
*/
-#ifdef CONFIG_HYPERV_TSCPAGE
- if (ms_hyperv.features & HV_MSR_REFERENCE_TSC_AVAILABLE) {
- union hv_x64_msr_hypercall_contents tsc_msr;
-
- tsc_pg = __vmalloc(PAGE_SIZE, GFP_KERNEL, PAGE_KERNEL);
- if (!tsc_pg)
- goto register_msr_cs;
-
- hyperv_cs = &hyperv_cs_tsc;
-
- rdmsrl(HV_X64_MSR_REFERENCE_TSC, tsc_msr.as_uint64);
-
- tsc_msr.enable = 1;
- tsc_msr.guest_physical_address = vmalloc_to_pfn(tsc_pg);
-
- wrmsrl(HV_X64_MSR_REFERENCE_TSC, tsc_msr.as_uint64);
-
- hyperv_cs_tsc.archdata.vclock_mode = VCLOCK_HVCLOCK;
-
- clocksource_register_hz(&hyperv_cs_tsc, NSEC_PER_SEC/100);
- return;
- }
-register_msr_cs:
-#endif
- /*
- * For 32 bit guests just use the MSR based mechanism for reading
- * the partition counter.
- */
-
- hyperv_cs = &hyperv_cs_msr;
- if (ms_hyperv.features & HV_MSR_TIME_REF_COUNT_AVAILABLE)
- clocksource_register_hz(&hyperv_cs_msr, NSEC_PER_SEC/100);
-
+ hv_init_clocksource(false);
return;
remove_cpuhp_state:
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index cc60e61..9326689 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -105,6 +105,17 @@ static inline void vmbus_signal_eom(struct hv_message *msg, u32 old_msg_type)
#define hv_get_crash_ctl(val) \
rdmsrl(HV_X64_MSR_CRASH_CTL, val)
+#define hv_get_time_ref_count(val) \
+ rdmsrl(HV_X64_MSR_TIME_REF_COUNT, val)
+
+#define hv_get_reference_tsc(val) \
+ rdmsrl(HV_X64_MSR_REFERENCE_TSC, val)
+#define hv_set_reference_tsc(val) \
+ wrmsrl(HV_X64_MSR_REFERENCE_TSC, val)
+#define hv_set_clocksource_vdso(val) \
+ ((val).archdata.vclock_mode = VCLOCK_HVCLOCK)
+#define hv_get_raw_timer() rdtsc_ordered()
+
void hyperv_callback_vector(void);
void hyperv_reenlightenment_vector(void);
#ifdef CONFIG_TRACING
@@ -387,73 +398,4 @@ static inline int hyperv_flush_guest_mapping_range(u64 as,
}
#endif /* CONFIG_HYPERV */
-#ifdef CONFIG_HYPERV_TSCPAGE
-struct ms_hyperv_tsc_page *hv_get_tsc_page(void);
-static inline u64 hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg,
- u64 *cur_tsc)
-{
- u64 scale, offset;
- u32 sequence;
-
- /*
- * The protocol for reading Hyper-V TSC page is specified in Hypervisor
- * Top-Level Functional Specification ver. 3.0 and above. To get the
- * reference time we must do the following:
- * - READ ReferenceTscSequence
- * A special '0' value indicates the time source is unreliable and we
- * need to use something else. The currently published specification
- * versions (up to 4.0b) contain a mistake and wrongly claim '-1'
- * instead of '0' as the special value, see commit c35b82ef0294.
- * - ReferenceTime =
- * ((RDTSC() * ReferenceTscScale) >> 64) + ReferenceTscOffset
- * - READ ReferenceTscSequence again. In case its value has changed
- * since our first reading we need to discard ReferenceTime and repeat
- * the whole sequence as the hypervisor was updating the page in
- * between.
- */
- do {
- sequence = READ_ONCE(tsc_pg->tsc_sequence);
- if (!sequence)
- return U64_MAX;
- /*
- * Make sure we read sequence before we read other values from
- * TSC page.
- */
- smp_rmb();
-
- scale = READ_ONCE(tsc_pg->tsc_scale);
- offset = READ_ONCE(tsc_pg->tsc_offset);
- *cur_tsc = rdtsc_ordered();
-
- /*
- * Make sure we read sequence after we read all other values
- * from TSC page.
- */
- smp_rmb();
-
- } while (READ_ONCE(tsc_pg->tsc_sequence) != sequence);
-
- return mul_u64_u64_shr(*cur_tsc, scale, 64) + offset;
-}
-
-static inline u64 hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
-{
- u64 cur_tsc;
-
- return hv_read_tsc_page_tsc(tsc_pg, &cur_tsc);
-}
-
-#else
-static inline struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
-{
- return NULL;
-}
-
-static inline u64 hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg,
- u64 *cur_tsc)
-{
- BUG();
- return U64_MAX;
-}
-#endif
#endif
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 941f932..4f83437 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -70,6 +70,7 @@
#include <asm/mshyperv.h>
#include <asm/hypervisor.h>
#include <asm/intel_pt.h>
+#include <clocksource/hyperv_syntimer.h>
#define CREATE_TRACE_POINTS
#include "trace.h"
diff --git a/drivers/clocksource/hyperv_syntimer.c b/drivers/clocksource/hyperv_syntimer.c
index 7276308..57e10bf 100644
--- a/drivers/clocksource/hyperv_syntimer.c
+++ b/drivers/clocksource/hyperv_syntimer.c
@@ -14,6 +14,8 @@
#include <linux/percpu.h>
#include <linux/cpumask.h>
#include <linux/clockchips.h>
+#include <linux/clocksource.h>
+#include <linux/sched_clock.h>
#include <linux/cpuhotplug.h>
#include <linux/mm.h>
#include <clocksource/hyperv_syntimer.h>
@@ -204,3 +206,123 @@ void hv_syntimer_global_cleanup(void)
hv_syntimer_free();
}
EXPORT_SYMBOL_GPL(hv_syntimer_global_cleanup);
+
+/*
+ * Code and definitions for the Hyper-V clocksources. Two
+ * clocksources are defined: one that reads the Hyper-V defined MSR, and
+ * the other that uses the TSC reference page feature as defined in the
+ * TLFS. The MSR version is for compatibility with old versions of
+ * Hyper-V and 32-bit x86. The TSC reference page version is preferred.
+ */
+
+struct clocksource *hyperv_cs;
+EXPORT_SYMBOL_GPL(hyperv_cs);
+
+#ifdef CONFIG_HYPERV_TSCPAGE
+
+static struct ms_hyperv_tsc_page *tsc_pg;
+
+struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
+{
+ return tsc_pg;
+}
+EXPORT_SYMBOL_GPL(hv_get_tsc_page);
+
+static u64 read_hv_sched_clock_tsc(void)
+{
+ u64 current_tick = hv_read_tsc_page(tsc_pg);
+
+ if (current_tick == U64_MAX)
+ hv_get_time_ref_count(current_tick);
+
+ return current_tick;
+}
+
+static u64 read_hv_clock_tsc(struct clocksource *arg)
+{
+ return read_hv_sched_clock_tsc();
+}
+
+static struct clocksource hyperv_cs_tsc = {
+ .name = "hyperv_clocksource_tsc_page",
+ .rating = 400,
+ .read = read_hv_clock_tsc,
+ .mask = CLOCKSOURCE_MASK(64),
+ .flags = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+#endif
+
+static u64 read_hv_sched_clock_msr(void)
+{
+ u64 current_tick;
+ /*
+ * Read the partition counter to get the current tick count. This count
+ * is set to 0 when the partition is created and is incremented in
+ * 100 nanosecond units.
+ */
+ hv_get_time_ref_count(current_tick);
+ return current_tick;
+}
+
+static u64 read_hv_clock_msr(struct clocksource *arg)
+{
+ return read_hv_sched_clock_msr();
+}
+
+static struct clocksource hyperv_cs_msr = {
+ .name = "hyperv_clocksource_msr",
+ .rating = 400,
+ .read = read_hv_clock_msr,
+ .mask = CLOCKSOURCE_MASK(64),
+ .flags = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
+void __init hv_init_clocksource(bool reg_sched_clock)
+{
+#ifdef CONFIG_HYPERV_TSCPAGE
+ if (ms_hyperv.features & HV_MSR_REFERENCE_TSC_AVAILABLE) {
+
+ u64 tsc_msr;
+ phys_addr_t phys_addr;
+
+ tsc_pg = vmalloc(PAGE_SIZE);
+ if (!tsc_pg)
+ goto register_msr_cs;
+
+ hyperv_cs = &hyperv_cs_tsc;
+ phys_addr = page_to_phys(vmalloc_to_page(tsc_pg));
+
+ /* The Hyper-V TLFS specifies to preserve the value of
+ * reserved bits in registers. So read the existing
+ * value, preserve the low order 12 bits, and add in
+ * the guest physical address (which already has at
+ * least the low 12 bits set to zero since it is page
+ * aligned). Also set the "enable" bit, which is bit 0.
+ */
+ hv_get_reference_tsc(tsc_msr);
+ tsc_msr &= GENMASK_ULL(11, 0);
+ tsc_msr = tsc_msr | 0x1 | (u64)phys_addr;
+ hv_set_reference_tsc(tsc_msr);
+
+ hv_set_clocksource_vdso(hyperv_cs_tsc);
+ clocksource_register_hz(&hyperv_cs_tsc, NSEC_PER_SEC/100);
+ if (reg_sched_clock)
+ sched_clock_register(read_hv_sched_clock_tsc,
+ 64, HV_CLOCK_HZ);
+ return;
+ }
+register_msr_cs:
+#endif
+ /*
+ * For 32 bit guests just use the MSR based mechanism for reading
+ * the partition counter.
+ */
+ hyperv_cs = &hyperv_cs_msr;
+ if (ms_hyperv.features & HV_MSR_TIME_REF_COUNT_AVAILABLE) {
+ clocksource_register_hz(&hyperv_cs_msr, NSEC_PER_SEC/100);
+ if (reg_sched_clock)
+ sched_clock_register(read_hv_sched_clock_msr,
+ 64, HV_CLOCK_HZ);
+ }
+}
+EXPORT_SYMBOL_GPL(hv_init_clocksource);
diff --git a/include/clocksource/hyperv_syntimer.h b/include/clocksource/hyperv_syntimer.h
index 154138b..8ce351b 100644
--- a/include/clocksource/hyperv_syntimer.h
+++ b/include/clocksource/hyperv_syntimer.h
@@ -13,6 +13,10 @@
#ifndef __CLKSOURCE_HYPERV_SYNTIMER_H
#define __CLKSOURCE_HYPERV_SYNTIMER_H
+#include <linux/clocksource.h>
+#include <linux/math64.h>
+#include <asm/mshyperv.h>
+
#define HV_MAX_MAX_DELTA_TICKS 0xffffffff
#define HV_MIN_DELTA_TICKS 1
@@ -23,4 +27,78 @@
extern void hv_syntimer_global_cleanup(void);
extern void hv_stimer0_isr(void);
+#ifdef CONFIG_HYPERV
+extern struct clocksource *hyperv_cs;
+extern void hv_init_clocksource(bool reg_sched_clock);
+#endif /* CONFIG_HYPERV */
+
+#ifdef CONFIG_HYPERV_TSCPAGE
+extern struct ms_hyperv_tsc_page *hv_get_tsc_page(void);
+static inline u64 hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg,
+ u64 *cur_tsc)
+{
+ u64 scale, offset;
+ u32 sequence;
+
+ /*
+ * The protocol for reading Hyper-V TSC page is specified in Hypervisor
+ * Top-Level Functional Specification ver. 3.0 and above. To get the
+ * reference time we must do the following:
+ * - READ ReferenceTscSequence
+ * A special '0' value indicates the time source is unreliable and we
+ * need to use something else. The currently published specification
+ * versions (up to 4.0b) contain a mistake and wrongly claim '-1'
+ * instead of '0' as the special value, see commit c35b82ef0294.
+ * - ReferenceTime =
+ * ((RDTSC() * ReferenceTscScale) >> 64) + ReferenceTscOffset
+ * - READ ReferenceTscSequence again. In case its value has changed
+ * since our first reading we need to discard ReferenceTime and repeat
+ * the whole sequence as the hypervisor was updating the page in
+ * between.
+ */
+ do {
+ sequence = READ_ONCE(tsc_pg->tsc_sequence);
+ if (!sequence)
+ return U64_MAX;
+ /*
+ * Make sure we read sequence before we read other values from
+ * TSC page.
+ */
+ smp_rmb();
+
+ scale = READ_ONCE(tsc_pg->tsc_scale);
+ offset = READ_ONCE(tsc_pg->tsc_offset);
+ *cur_tsc = hv_get_raw_timer();
+
+ /*
+ * Make sure we read sequence after we read all other values
+ * from TSC page.
+ */
+ smp_rmb();
+
+ } while (READ_ONCE(tsc_pg->tsc_sequence) != sequence);
+
+ return mul_u64_u64_shr(*cur_tsc, scale, 64) + offset;
+}
+
+static inline u64 hv_read_tsc_page(const struct ms_hyperv_tsc_page *tsc_pg)
+{
+ u64 cur_tsc;
+
+ return hv_read_tsc_page_tsc(tsc_pg, &cur_tsc);
+}
+
+#else /* CONFIG_HYPERV_TSC_PAGE */
+static inline struct ms_hyperv_tsc_page *hv_get_tsc_page(void)
+{
+ return NULL;
+}
+
+static inline u64 hv_read_tsc_page_tsc(const struct ms_hyperv_tsc_page *tsc_pg,
+ u64 *cur_tsc)
+{
+ return U64_MAX;
+}
+#endif /* CONFIG_HYPERV_TSCPAGE */
+
#endif
--
1.8.3.1
^ permalink raw reply related
* [PATCH 0/2] Drivers: hv: Move Hyper-V clock/timer code to separate clocksource driver
From: Michael Kelley @ 2019-03-12 21:42 UTC (permalink / raw)
To: will.deacon@arm.com, marc.zyngier@arm.com,
linux-arm-kernel@lists.infradead.org, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
olaf@aepfle.de, apw@canonical.com, vkuznets, jasowang@redhat.com,
marcelo.cerri@canonical.com, Sunil Muthuswamy, KY Srinivasan
Cc: Michael Kelley, catalin.marinas@arm.com, mark.rutland@arm.com
This patch series moves Hyper-V clock/timer code to a separate Hyper-V
clocksource driver. Previously, Hyper-V clock/timer code and data
structures were mixed in with other Hyper-V code in the ISA independent
drivers/hv code as well as in arch dependent code. The new Hyper-V
clocksource driver is ISA independent, with a just few dependencies on
arch specific functions. The patch series does not change any behavior
or functionality -- it only reorganizes the existing code and fixes up
the linkages. A few places outside of Hyper-V code are fixed up to use
the new #include file structure.
This restructuring is in response to Marc Zyngier's review comments
on supporting Hyper-V running on ARM64, and is a good idea in general.
It increases the amount of code shared between the x86 and ARM64
architectures, and reduces the size of the new code for supporting
Hyper-V on ARM64. A new version of the Hyper-V on ARM64 patches will
follow once this clocksource restructuring is accepted.
The code is currently diff'ed against Linux 5.0. I'll rebase
to linux-next once 5.1-rc1 is available.
Michael Kelley (2):
Drivers: hv: Move Hyper-V clockevents code to new clocksource driver
Drivers: hv: Move Hyper-V clocksource code to new clocksource driver
MAINTAINERS | 2 +
arch/x86/entry/vdso/vclock_gettime.c | 1 +
arch/x86/entry/vdso/vma.c | 2 +-
arch/x86/hyperv/hv_init.c | 91 +---------
arch/x86/include/asm/hyperv-tlfs.h | 6 +
arch/x86/include/asm/mshyperv.h | 80 ++-------
arch/x86/kernel/cpu/mshyperv.c | 2 +
arch/x86/kvm/x86.c | 1 +
drivers/clocksource/Makefile | 1 +
drivers/clocksource/hyperv_syntimer.c | 328 ++++++++++++++++++++++++++++++++++
drivers/hv/hv.c | 154 ----------------
drivers/hv/hyperv_vmbus.h | 3 -
drivers/hv/vmbus_drv.c | 39 ++--
include/clocksource/hyperv_syntimer.h | 104 +++++++++++
14 files changed, 482 insertions(+), 332 deletions(-)
create mode 100644 drivers/clocksource/hyperv_syntimer.c
create mode 100644 include/clocksource/hyperv_syntimer.h
--
1.8.3.1
^ permalink raw reply
* [PATCH v2] hv_netvsc: fix a possible NULL pointer dereference in netvsc_get_ethtool_stats()
From: Kangjie Lu @ 2019-03-12 5:01 UTC (permalink / raw)
To: kjlu
Cc: pakki001, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Sasha Levin, David S. Miller, linux-hyperv, netdev, linux-kernel
In case kvmalloc_array fails, we should stop using it to avoid
NULL pointer dereference.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
---
drivers/net/hyperv/netvsc_drv.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index cf4897043e83..4b3a03029fe8 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1426,6 +1426,9 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
pcpu_sum = kvmalloc_array(num_possible_cpus(),
sizeof(struct netvsc_ethtool_pcpu_stats),
GFP_KERNEL);
+ if (!pcpu_sum)
+ return;
+
netvsc_get_pcpu_stats(dev, pcpu_sum);
for_each_present_cpu(cpu) {
struct netvsc_ethtool_pcpu_stats *this_sum = &pcpu_sum[cpu];
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v5] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used
From: Kimberly Brown @ 2019-03-12 0:04 UTC (permalink / raw)
To: Greg KH
Cc: Michael Kelley, Long Li, Sasha Levin, Stephen Hemminger,
Dexuan Cui, K. Y. Srinivasan, Haiyang Zhang, linux-hyperv,
linux-kernel
In-Reply-To: <20190309072135.GG3882@kroah.com>
On Sat, Mar 09, 2019 at 08:21:35AM +0100, Greg KH wrote:
> On Fri, Mar 08, 2019 at 05:46:11PM -0500, Kimberly Brown wrote:
> > static struct kobj_type vmbus_chan_ktype = {
> > .sysfs_ops = &vmbus_chan_sysfs_ops,
> > .release = vmbus_chan_release,
> > - .default_attrs = vmbus_chan_attrs,
>
> As discussed on IRC, a kobj_type needs to get an attribute group one of
> these days, not just a attribute list :)
>
> So thanks for persisting with this, sorry for the earlier review
> comments, you were totally right about this, nice work.
>
> Minor review comments below:
>
> > };
> >
> > /*
> > @@ -1571,11 +1624,34 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel)
> > if (ret)
> > return ret;
> >
> > + ret = sysfs_create_group(kobj, &vmbus_chan_group);
> > + channel->sysfs_group_ready = !ret;
>
> Why do you need this flag?
>
I added this flag to prevent sysfs_remove_group() from being called if
sysfs_create_group() or the earlier call to kobject_init_and_add()
failed. However, it looks like that would just lead to WARN() being
called, which seems appropriate. I'll remove this flag.
> > +
> > + if (ret) {
> > + /*
> > + * If an error is returned to the calling functions, those
> > + * functions will call kobject_put() on the channel kobject,
> > + * which will cleanup the empty channel directory created by
> > + * kobject_init_and_add().
>
> Why is this comment needed?
>
Another reviewer asked why the empty directory created by
kobject_init_and_add() isn't removed here when sysfs_create_group()
fails. We decided that a comment would help clear up any future
confusion.
> > + */
> > + pr_err("Unable to set up channel sysfs files\n");
>
> dev_err() to show who had the problem with the files?
>
Yes, good point. I'll change this.
> > + return ret;
> > + }
> > +
> > kobject_uevent(kobj, KOBJ_ADD);
> >
> > return 0;
> > }
> >
> > +/*
> > + * vmbus_remove_channel_attr_group - remove the channel's attribute group
> > + */
> > +void vmbus_remove_channel_attr_group(struct vmbus_channel *channel)
> > +{
> > + if (channel->sysfs_group_ready)
> > + sysfs_remove_group(&channel->kobj, &vmbus_chan_group);
>
> You should be able to just always remove these, no need for a flag to
> say you have created them or not, right?
>
> thanks,
>
> greg k-h
^ permalink raw reply
* RE: [PATCH] net: hyperv: fix a NULL pointer dereference
From: Haiyang Zhang @ 2019-03-11 19:49 UTC (permalink / raw)
To: Haiyang Zhang, Kangjie Lu
Cc: pakki001@umn.edu, KY Srinivasan, Stephen Hemminger, Sasha Levin,
David S. Miller, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <DM5PR2101MB0725705F1CDDD3D1631CD509CA480@DM5PR2101MB0725.namprd21.prod.outlook.com>
> -----Original Message-----
> From: linux-hyperv-owner@vger.kernel.org <linux-hyperv-
> owner@vger.kernel.org> On Behalf Of Haiyang Zhang
> Sent: Monday, March 11, 2019 3:47 PM
> To: Kangjie Lu <kjlu@umn.edu>
> Cc: pakki001@umn.edu; KY Srinivasan <kys@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; Sasha Levin <sashal@kernel.org>;
> David S. Miller <davem@davemloft.net>; linux-hyperv@vger.kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] net: hyperv: fix a NULL pointer dereference
>
>
>
> > -----Original Message-----
> > From: Kangjie Lu <kjlu@umn.edu>
> > Sent: Monday, March 11, 2019 3:17 AM
> > To: kjlu@umn.edu
> > Cc: pakki001@umn.edu; KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> > <haiyangz@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>;
> > Sasha Levin <sashal@kernel.org>; David S. Miller
> > <davem@davemloft.net>; linux-hyperv@vger.kernel.org;
> > netdev@vger.kernel.org; linux- kernel@vger.kernel.org
> > Subject: [PATCH] net: hyperv: fix a NULL pointer dereference
> >
> > In case kvmalloc_array fails, we should stop using it to avoid NULL
> > pointer dereference.
> >
> > Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> > ---
>
Also, please change the subject to be more specific:
hv_netvsc: fix a possible NULL pointer dereference in netvsc_get_ethtool_stats()
Thanks.
^ permalink raw reply
* RE: [PATCH] net: hyperv: fix a NULL pointer dereference
From: Haiyang Zhang @ 2019-03-11 19:46 UTC (permalink / raw)
To: Kangjie Lu
Cc: pakki001@umn.edu, KY Srinivasan, Stephen Hemminger, Sasha Levin,
David S. Miller, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20190311071714.28631-1-kjlu@umn.edu>
> -----Original Message-----
> From: Kangjie Lu <kjlu@umn.edu>
> Sent: Monday, March 11, 2019 3:17 AM
> To: kjlu@umn.edu
> Cc: pakki001@umn.edu; KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> Sasha Levin <sashal@kernel.org>; David S. Miller <davem@davemloft.net>;
> linux-hyperv@vger.kernel.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [PATCH] net: hyperv: fix a NULL pointer dereference
>
> In case kvmalloc_array fails, we should stop using it to avoid NULL pointer
> dereference.
>
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> ---
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
Thanks!
^ permalink raw reply
* [PATCH] net: hyperv: fix a NULL pointer dereference
From: Kangjie Lu @ 2019-03-11 7:17 UTC (permalink / raw)
To: kjlu
Cc: pakki001, K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger,
Sasha Levin, David S. Miller, linux-hyperv, netdev, linux-kernel
In case kvmalloc_array fails, we should stop using it to avoid
NULL pointer dereference.
Signed-off-by: Kangjie Lu <kjlu@umn.edu>
---
drivers/net/hyperv/netvsc_drv.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index cf4897043e83..4b3a03029fe8 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1426,6 +1426,9 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
pcpu_sum = kvmalloc_array(num_possible_cpus(),
sizeof(struct netvsc_ethtool_pcpu_stats),
GFP_KERNEL);
+ if (!pcpu_sum)
+ return;
+
netvsc_get_pcpu_stats(dev, pcpu_sum);
for_each_present_cpu(cpu) {
struct netvsc_ethtool_pcpu_stats *this_sum = &pcpu_sum[cpu];
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v5] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used
From: Greg KH @ 2019-03-09 7:21 UTC (permalink / raw)
To: Kimberly Brown
Cc: Michael Kelley, Long Li, Sasha Levin, Stephen Hemminger,
Dexuan Cui, K. Y. Srinivasan, Haiyang Zhang, linux-hyperv,
linux-kernel
In-Reply-To: <20190308224611.GA3047@ubu-Virtual-Machine>
On Fri, Mar 08, 2019 at 05:46:11PM -0500, Kimberly Brown wrote:
> static struct kobj_type vmbus_chan_ktype = {
> .sysfs_ops = &vmbus_chan_sysfs_ops,
> .release = vmbus_chan_release,
> - .default_attrs = vmbus_chan_attrs,
As discussed on IRC, a kobj_type needs to get an attribute group one of
these days, not just a attribute list :)
So thanks for persisting with this, sorry for the earlier review
comments, you were totally right about this, nice work.
Minor review comments below:
> };
>
> /*
> @@ -1571,11 +1624,34 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel)
> if (ret)
> return ret;
>
> + ret = sysfs_create_group(kobj, &vmbus_chan_group);
> + channel->sysfs_group_ready = !ret;
Why do you need this flag?
> +
> + if (ret) {
> + /*
> + * If an error is returned to the calling functions, those
> + * functions will call kobject_put() on the channel kobject,
> + * which will cleanup the empty channel directory created by
> + * kobject_init_and_add().
Why is this comment needed?
> + */
> + pr_err("Unable to set up channel sysfs files\n");
dev_err() to show who had the problem with the files?
> + return ret;
> + }
> +
> kobject_uevent(kobj, KOBJ_ADD);
>
> return 0;
> }
>
> +/*
> + * vmbus_remove_channel_attr_group - remove the channel's attribute group
> + */
> +void vmbus_remove_channel_attr_group(struct vmbus_channel *channel)
> +{
> + if (channel->sysfs_group_ready)
> + sysfs_remove_group(&channel->kobj, &vmbus_chan_group);
You should be able to just always remove these, no need for a flag to
say you have created them or not, right?
thanks,
greg k-h
^ permalink raw reply
* [PATCH v5] Drivers: hv: vmbus: Expose monitor data only when monitor pages are used
From: Kimberly Brown @ 2019-03-08 22:46 UTC (permalink / raw)
To: Michael Kelley, Long Li, Sasha Levin, Stephen Hemminger,
Dexuan Cui, Greg KH
Cc: K. Y. Srinivasan, Haiyang Zhang, linux-hyperv, linux-kernel
In-Reply-To: <20190301191824.GA4108@ubu-Virtual-Machine>
There are two methods for signaling the host: the monitor page mechanism
and hypercalls. The monitor page mechanism is used by performance
critical channels (storage, networking, etc.) because it provides
improved throughput. However, latency is increased. Monitor pages are
allocated to these channels.
Monitor pages are not allocated to channels that do not use the monitor
page mechanism. Therefore, these channels do not have a valid monitor id
or valid monitor page data. In these cases, some of the "_show"
functions return incorrect data. They return an invalid monitor id and
data that is beyond the bounds of the hv_monitor_page array fields.
The "channel->offermsg.monitor_allocated" value can be used to determine
whether monitor pages have been allocated to a channel.
Add "is_visible()" callback functions for the device-level and
channel-level attribute groups. These functions will hide the monitor
sysfs files when the monitor mechanism is not used.
Remove ".default_attributes" from "vmbus_chan_attrs" and create a
channel-level attribute group, "vmbus_chan_group". These changes allow
the new "is_visible()" callback function to be applied to the
channel-level attributes. Call "sysfs_create_group()" to create the
channel sysyfs files.
Add the “bool sysfs_group_ready” field to the vmbus_channel struct.
This field is used to ensure that the attributes in the
“vmbus_chan_group” attribute group have been created. Add the
“vmbus_remove_channel_attr_group()” function, which calls
"sysfs_remove_group()" on “vmbus_chan_group” if the attributes were
created.
Signed-off-by: Kimberly Brown <kimbrownkd@gmail.com>
---
Changes in v5:
- Added the “bool sysfs_group_ready” field to vmbus_channel.
- Added the “vmbus_remove_channel_attr_group()” function which calls
"sysfs_remove_group()".
- Added a comment to "vmbus_add_channel_kobj()" to describe how the
empty directory is removed if "sysfs_create_group()" returns an
error.
- Updated the commit message.
NOTE: “.default_attrs” must be removed from vmbus_chan_ktype in order to
use the is_visible() function because "default_attrs" is an array of
attributes, not an attribute_group.
Changes in v4:
- Added “is_visible()” callback functions for the device-level and
channel-level attribute groups.
- Removed the separate monitor attribute groups proposed in v3. They’re
no longer needed because the “is_visible()” callbacks are used to
determine the attribute visibility.
- Removed "default_attributes" from "vmbus_chan_attrs" and created a
channel-level attribute group.
- Removed the "kobject_put(kobj)" call proposed in v3. The calling
functions take care of calling "kobject_put(channel->kobj)" if an
error is returned by "vmbus_add_channel_kobj()".
- Updated the commit message and subject for clarity and to reflect the
new changes in v4.
Changes in v3:
- The monitor "_show" functions no longer return an error when a
channel does not use the monitor page mechanism. Instead, the monitor
page sysfs files are created only when a channel uses the monitor
page mechanism. This change was suggested by G. Kroah-Hartman.
Note: this patch was originally patch 2/2 in a patchset. Patch 1/2 has
already been added to char-misc-testing, so I'm not resending it.
Changes in v2:
- Changed the return value for cases where monitor_allocated is not set
to "-EINVAL".
- Updated the commit message to provide more details about the monitor
page mechanism.
- Updated the sysfs documentation to describe the new return value.
Documentation/ABI/stable/sysfs-bus-vmbus | 12 +++-
drivers/hv/channel_mgmt.c | 3 +
drivers/hv/hyperv_vmbus.h | 2 +
drivers/hv/vmbus_drv.c | 80 +++++++++++++++++++++++-
include/linux/hyperv.h | 6 ++
5 files changed, 98 insertions(+), 5 deletions(-)
diff --git a/Documentation/ABI/stable/sysfs-bus-vmbus b/Documentation/ABI/stable/sysfs-bus-vmbus
index 826689dcc2e6..8e8d167eca31 100644
--- a/Documentation/ABI/stable/sysfs-bus-vmbus
+++ b/Documentation/ABI/stable/sysfs-bus-vmbus
@@ -81,7 +81,9 @@ What: /sys/bus/vmbus/devices/<UUID>/channels/<N>/latency
Date: September. 2017
KernelVersion: 4.14
Contact: Stephen Hemminger <sthemmin@microsoft.com>
-Description: Channel signaling latency
+Description: Channel signaling latency. This file is available only for
+ performance critical channels (storage, network, etc.) that use
+ the monitor page mechanism.
Users: Debugging tools
What: /sys/bus/vmbus/devices/<UUID>/channels/<N>/out_mask
@@ -95,7 +97,9 @@ What: /sys/bus/vmbus/devices/<UUID>/channels/<N>/pending
Date: September. 2017
KernelVersion: 4.14
Contact: Stephen Hemminger <sthemmin@microsoft.com>
-Description: Channel interrupt pending state
+Description: Channel interrupt pending state. This file is available only for
+ performance critical channels (storage, network, etc.) that use
+ the monitor page mechanism.
Users: Debugging tools
What: /sys/bus/vmbus/devices/<UUID>/channels/<N>/read_avail
@@ -137,7 +141,9 @@ What: /sys/bus/vmbus/devices/<UUID>/channels/<N>/monitor_id
Date: January. 2018
KernelVersion: 4.16
Contact: Stephen Hemminger <sthemmin@microsoft.com>
-Description: Monitor bit associated with channel
+Description: Monitor bit associated with channel. This file is available only
+ for performance critical channels (storage, network, etc.) that
+ use the monitor page mechanism.
Users: Debugging tools and userspace drivers
What: /sys/bus/vmbus/devices/<UUID>/channels/<N>/ring
diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
index 62703b354d6d..e4b1f0db8159 100644
--- a/drivers/hv/channel_mgmt.c
+++ b/drivers/hv/channel_mgmt.c
@@ -346,6 +346,9 @@ static void free_channel(struct vmbus_channel *channel)
{
tasklet_kill(&channel->callback_event);
+ /* Remove the channel sysfs attribute group */
+ vmbus_remove_channel_attr_group(channel);
+
kobject_put(&channel->kobj);
}
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index cb86b133eb4d..a94aab94e0b5 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -321,6 +321,8 @@ void vmbus_device_unregister(struct hv_device *device_obj);
int vmbus_add_channel_kobj(struct hv_device *device_obj,
struct vmbus_channel *channel);
+void vmbus_remove_channel_attr_group(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 000b53e5a17a..43a17e17f670 100644
--- a/drivers/hv/vmbus_drv.c
+++ b/drivers/hv/vmbus_drv.c
@@ -630,7 +630,36 @@ static struct attribute *vmbus_dev_attrs[] = {
&dev_attr_driver_override.attr,
NULL,
};
-ATTRIBUTE_GROUPS(vmbus_dev);
+
+/*
+ * Device-level attribute_group callback function. Returns the permission for
+ * each attribute, and returns 0 if an attribute is not visible.
+ */
+static umode_t vmbus_dev_attr_is_visible(struct kobject *kobj,
+ struct attribute *attr, int idx)
+{
+ struct device *dev = kobj_to_dev(kobj);
+ const struct hv_device *hv_dev = device_to_hv_device(dev);
+
+ /* Hide the monitor attributes if the monitor mechanism is not used. */
+ if (!hv_dev->channel->offermsg.monitor_allocated &&
+ (attr == &dev_attr_monitor_id.attr ||
+ attr == &dev_attr_server_monitor_pending.attr ||
+ attr == &dev_attr_client_monitor_pending.attr ||
+ attr == &dev_attr_server_monitor_latency.attr ||
+ attr == &dev_attr_client_monitor_latency.attr ||
+ attr == &dev_attr_server_monitor_conn_id.attr ||
+ attr == &dev_attr_client_monitor_conn_id.attr))
+ return 0;
+
+ return attr->mode;
+}
+
+static const struct attribute_group vmbus_dev_group = {
+ .attrs = vmbus_dev_attrs,
+ .is_visible = vmbus_dev_attr_is_visible
+};
+__ATTRIBUTE_GROUPS(vmbus_dev);
/*
* vmbus_uevent - add uevent for our device
@@ -1550,10 +1579,34 @@ static struct attribute *vmbus_chan_attrs[] = {
NULL
};
+/*
+ * Channel-level attribute_group callback function. Returns the permission for
+ * each attribute, and returns 0 if an attribute is not visible.
+ */
+static umode_t vmbus_chan_attr_is_visible(struct kobject *kobj,
+ struct attribute *attr, int idx)
+{
+ const struct vmbus_channel *channel =
+ container_of(kobj, struct vmbus_channel, kobj);
+
+ /* Hide the monitor attributes if the monitor mechanism is not used. */
+ if (!channel->offermsg.monitor_allocated &&
+ (attr == &chan_attr_pending.attr ||
+ attr == &chan_attr_latency.attr ||
+ attr == &chan_attr_monitor_id.attr))
+ return 0;
+
+ return attr->mode;
+}
+
+static struct attribute_group vmbus_chan_group = {
+ .attrs = vmbus_chan_attrs,
+ .is_visible = vmbus_chan_attr_is_visible
+};
+
static struct kobj_type vmbus_chan_ktype = {
.sysfs_ops = &vmbus_chan_sysfs_ops,
.release = vmbus_chan_release,
- .default_attrs = vmbus_chan_attrs,
};
/*
@@ -1571,11 +1624,34 @@ int vmbus_add_channel_kobj(struct hv_device *dev, struct vmbus_channel *channel)
if (ret)
return ret;
+ ret = sysfs_create_group(kobj, &vmbus_chan_group);
+ channel->sysfs_group_ready = !ret;
+
+ if (ret) {
+ /*
+ * If an error is returned to the calling functions, those
+ * functions will call kobject_put() on the channel kobject,
+ * which will cleanup the empty channel directory created by
+ * kobject_init_and_add().
+ */
+ pr_err("Unable to set up channel sysfs files\n");
+ return ret;
+ }
+
kobject_uevent(kobj, KOBJ_ADD);
return 0;
}
+/*
+ * vmbus_remove_channel_attr_group - remove the channel's attribute group
+ */
+void vmbus_remove_channel_attr_group(struct vmbus_channel *channel)
+{
+ if (channel->sysfs_group_ready)
+ sysfs_remove_group(&channel->kobj, &vmbus_chan_group);
+}
+
/*
* vmbus_device_create - Creates and registers a new child device
* on the vmbus.
diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
index 64698ec8f2ac..604a2e05af47 100644
--- a/include/linux/hyperv.h
+++ b/include/linux/hyperv.h
@@ -934,6 +934,12 @@ struct vmbus_channel {
* full outbound ring buffer.
*/
u64 out_full_first;
+
+ /*
+ * Indicates whether the channel's attribute group sysfs files have
+ * been successfully created.
+ */
+ bool sysfs_group_ready;
};
static inline bool is_hvsock_channel(const struct vmbus_channel *c)
--
2.17.1
^ permalink raw reply related
* RE: [PATCH] hyper-v: Check for ring buffer in hv_get_bytes_to_read/write
From: Michael Kelley @ 2019-03-07 19:34 UTC (permalink / raw)
To: mgamal@redhat.com, linux-hyperv@vger.kernel.org, kimbrownkd,
Haiyang Zhang, Stephen Hemminger
Cc: Sasha Levin, Dexuan Cui, Long Li, KY Srinivasan, vkuznets,
linux-kernel@vger.kernel.org
In-Reply-To: <1551983533.5436.8.camel@redhat.com>
From: Mohammed Gamal <mgamal@redhat.com> Sent: Thursday, March 7, 2019 10:32 AM
> >
> > Could you elaborate on the code paths where
> > hv_get_bytes_to_read/write() could be called when the ring buffer
> > isn't yet allocated? My sense is that Kim Brown's patch will address
> > all of the code paths that involved sysfs access from outside the
> > driver. And within a driver, the ring buffer should never be
> > accessed
> > unless it is already allocated. Is there another code path we're not
> > aware of? I'm wondering if these changes are really needed once
> > Kim Brown's patch is finished.
> >
> > Michael
>
> I've seen one instance of the race in the netvsc driver when running
> traffic through it with iperf3 while continuously changing the channel
> settings.
>
> The following code path deallocates the ring buffer:
> netvsc_set_channels() -> netvsc_detach() ->
> rndis_filter_device_remove() -> netvsc_device_remove() -> vmbus_close()
> -> vmbus_free_ring() -> hv_ringbuffer_cleanup().
>
> netvsc_send_pkt() -> hv_get_bytes_to_write() might get called
> concurrently after vmbus_close() and before vmbus_open() returns and
> sets up the new ring buffer.
>
> The race is fairly hard to reproduce on recent upstream kernels, but I
> still managed to reproduce it.
My thought is that a race like the above needs to be addressed in the
netvsc driver. The race may have other problems beyond just
accessing the ring buffer before it is (re)allocated. While adding the tests
in hv_get_bytes_to_read/write() isn't harmful, doing so has the potential
to mask the real problem. These routines are also somewhat performance
sensitive so we don't want any unnecessary overhead.
Michael
^ permalink raw reply
* Re: [PATCH] hyper-v: Check for ring buffer in hv_get_bytes_to_read/write
From: Mohammed Gamal @ 2019-03-07 18:32 UTC (permalink / raw)
To: Michael Kelley, linux-hyperv@vger.kernel.org, kimbrownkd
Cc: Sasha Levin, Dexuan Cui, Stephen Hemminger, Long Li,
KY Srinivasan, Haiyang Zhang, vkuznets,
linux-kernel@vger.kernel.org
In-Reply-To: <DM5PR2101MB09182EAC78F8EEA57E27C26AD74C0@DM5PR2101MB0918.namprd21.prod.outlook.com>
On Thu, 2019-03-07 at 17:33 +0000, Michael Kelley wrote:
> From: Mohammed Gamal <mgamal@redhat.com> Sent: Thursday, March 7,
> 2019 8:36 AM
> >
> > This patch adds a check for the presence of the ring buffer in
> > hv_get_bytes_to_read/write() to avoid possible NULL pointer
> > dereferences.
> > If the ring buffer is not yet allocated, return 0 bytes to be
> > read/written.
> >
> > The root cause is that code that accesses the ring buffer including
> > hv_get_bytes_to_read/write() could be vulnerable to the race
> > condition
> > discussed in https://lkml.org/lkml/2018/10/18/779>;
> >
> > This race is being addressed by the patch series by Kimberly Brown
> > in
> > https://lkml.org/lkml/2019/2/21/1236 which is not final yet
> >
> > Signed-off-by: Mohammed Gamal <mgamal@redhat.com>
>
> Could you elaborate on the code paths where
> hv_get_bytes_to_read/write() could be called when the ring buffer
> isn't yet allocated? My sense is that Kim Brown's patch will address
> all of the code paths that involved sysfs access from outside the
> driver. And within a driver, the ring buffer should never be
> accessed
> unless it is already allocated. Is there another code path we're not
> aware of? I'm wondering if these changes are really needed once
> Kim Brown's patch is finished.
>
> Michael
I've seen one instance of the race in the netvsc driver when running
traffic through it with iperf3 while continuously changing the channel
settings.
The following code path deallocates the ring buffer:
netvsc_set_channels() -> netvsc_detach() ->
rndis_filter_device_remove() -> netvsc_device_remove() -> vmbus_close()
-> vmbus_free_ring() -> hv_ringbuffer_cleanup().
netvsc_send_pkt() -> hv_get_bytes_to_write() might get called
concurrently after vmbus_close() and before vmbus_open() returns and
sets up the new ring buffer.
The race is fairly hard to reproduce on recent upstream kernels, but I
still managed to reproduce it.
^ 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