From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756743AbcLUS64 (ORCPT ); Wed, 21 Dec 2016 13:58:56 -0500 Received: from mail-bn3nam01on0120.outbound.protection.outlook.com ([104.47.33.120]:15033 "EHLO NAM01-BN3-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753104AbcLUS6y (ORCPT ); Wed, 21 Dec 2016 13:58:54 -0500 From: KY Srinivasan To: Roman Kagan , Paolo Bonzini , =?iso-8859-2?Q?Radim_Kr=E8m=E1=F8?= , Vitaly Kuznetsov CC: Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , "x86@kernel.org" , "Haiyang Zhang" , "kvm@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "devel@linuxdriverproject.org" , "Denis V . Lunev" Subject: RE: [PATCH 02/15] hyperv: uapi-fy synic event flags definitions Thread-Topic: [PATCH 02/15] hyperv: uapi-fy synic event flags definitions Thread-Index: AQHSWtmtc7SfJuF1VE+F3DlpGRk2zKESt6ig Date: Wed, 21 Dec 2016 18:58:38 +0000 Message-ID: References: <20161220155602.6298-1-rkagan@virtuozzo.com> <20161220155602.6298-3-rkagan@virtuozzo.com> In-Reply-To: <20161220155602.6298-3-rkagan@virtuozzo.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=kys@microsoft.com; x-originating-ip: [2601:600:8c00:1040:b076:3b2a:409c:285e] x-ms-office365-filtering-correlation-id: c12e1a69-5df9-492c-9acb-08d429d35f80 x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001);SRVR:BLUPR03MB1410; x-microsoft-exchange-diagnostics: 1;BLUPR03MB1410;7:78G2hkfhl3RC9qAQKicRS4k8aKFS8EnQT3ALwZ+O6wWZiDn658NZ3tvSh2uZgmuJQ0Suui1zt31m9FiWvzf65yIT2qMkNOt8iawsOvZyfPf6+kHbzc76POh3nYQ50UNqDZ8ynvt02pX2jz+NCyTYZgBrLU+ckvEIfubesS+xvIx/alKPDlOm48NvpzI7q4arc3UrUGmcT00LA6wwm+q5TlTMmICft09I9thYzcMdAIqM6aaMT+a/OW5johiH+9FgKniMmf+RyqQdOr8keEaA8BjdiSd95ONhqcdsnoFSCk3yNnQQrkXJm3RAyVgJ0Zj+CissUzWLJILbg9iS7N7i6dmMzPf+kqZuJSjxlodwVvr4TQZ4xUwn5+0xyaumg1EXV3jJtwg+gu1NU0BSppru09Z25/WomZWASRxc1C5DYl1EjzPWENV1eLgnUcIwkguMes/RQRk1pQMAEQE71uqNi4vg+iMRjADsiW/txe4r/uE= x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(9452136761055); x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(61425038)(6040375)(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001)(6055026)(61426038)(61427038)(6041248)(20161123558021)(20161123564025)(20161123562025)(20161123555025)(20161123560025)(6047074)(6042181)(6072148);SRVR:BLUPR03MB1410;BCL:0;PCL:0;RULEID:;SRVR:BLUPR03MB1410; x-forefront-prvs: 01630974C0 x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(6009001)(7916002)(13464003)(189002)(199003)(377454003)(6506006)(25786008)(229853002)(6436002)(102836003)(6116002)(99286002)(106116001)(106356001)(7736002)(38730400001)(77096006)(105586002)(4326007)(86612001)(76576001)(305945005)(3660700001)(8990500004)(3280700002)(10290500002)(189998001)(5005710100001)(92566002)(101416001)(7696004)(54356999)(2900100001)(68736007)(122556002)(10090500001)(97736004)(5001770100001)(76176999)(50986999)(74316002)(5660300001)(2906002)(86362001)(575784001)(7416002)(2950100002)(33656002)(81156014)(8676002)(81166006)(9686002)(8936002);DIR:OUT;SFP:1102;SCL:1;SRVR:BLUPR03MB1410;H:DM5PR03MB2490.namprd03.prod.outlook.com;FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="iso-8859-2" MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Dec 2016 18:58:38.1102 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47 X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLUPR03MB1410 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uBLIx2OB009388 > -----Original Message----- > From: Roman Kagan [mailto:rkagan@virtuozzo.com] > Sent: Tuesday, December 20, 2016 7:56 AM > To: Paolo Bonzini ; Radim Krèmáø > ; KY Srinivasan ; Vitaly > Kuznetsov > Cc: Thomas Gleixner ; Ingo Molnar > ; H. Peter Anvin ; x86@kernel.org; > Haiyang Zhang ; kvm@vger.kernel.org; linux- > kernel@vger.kernel.org; devel@linuxdriverproject.org; Denis V . Lunev > ; Roman Kagan > Subject: [PATCH 02/15] hyperv: uapi-fy synic event flags definitions > > Move definitions related to the Hyper-V SynIC event flags to a header > where they can be consumed by userspace. > > While doing so, also clean up their use by switching to standard bitops > and struct-based dereferencing. The latter is also done for message > pages. Please breakup this patch. > > Signed-off-by: Roman Kagan > --- > arch/x86/include/uapi/asm/hyperv.h | 13 +++++++++ > drivers/hv/hyperv_vmbus.h | 24 ++-------------- > drivers/hv/channel_mgmt.c | 10 +++---- > drivers/hv/connection.c | 47 ++++++++++--------------------- > drivers/hv/vmbus_drv.c | 57 ++++++++++++++------------------------ > 5 files changed, 54 insertions(+), 97 deletions(-) > > diff --git a/arch/x86/include/uapi/asm/hyperv.h > b/arch/x86/include/uapi/asm/hyperv.h > index 6098ab5..af542a3 100644 > --- a/arch/x86/include/uapi/asm/hyperv.h > +++ b/arch/x86/include/uapi/asm/hyperv.h > @@ -363,4 +363,17 @@ struct hv_timer_message_payload { > #define HV_STIMER_AUTOENABLE (1ULL << 3) > #define HV_STIMER_SINT(config) (__u8)(((config) >> 16) & > 0x0F) > > +/* Define synthetic interrupt controller flag constants. */ > +#define HV_EVENT_FLAGS_COUNT (256 * 8) > + > +/* Define the synthetic interrupt controller event flags format. */ > +struct hv_synic_event_flags { > + __u64 flags[HV_EVENT_FLAGS_COUNT / 64]; > +}; > + > +/* Define the synthetic interrupt flags page layout. */ > +struct hv_synic_event_flags_page { > + struct hv_synic_event_flags > sintevent_flags[HV_SYNIC_SINT_COUNT]; > +}; > + > #endif > diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h > index 4516498..4fab154 100644 > --- a/drivers/hv/hyperv_vmbus.h > +++ b/drivers/hv/hyperv_vmbus.h > @@ -26,7 +26,6 @@ > #define _HYPERV_VMBUS_H > > #include > -#include > #include > #include > > @@ -75,11 +74,6 @@ enum hv_cpuid_function { > > #define HV_ANY_VP (0xFFFFFFFF) > > -/* Define synthetic interrupt controller flag constants. */ > -#define HV_EVENT_FLAGS_COUNT (256 * 8) > -#define HV_EVENT_FLAGS_BYTE_COUNT (256) > -#define HV_EVENT_FLAGS_DWORD_COUNT (256 / sizeof(u32)) > - > /* Define invalid partition identifier. */ > #define HV_PARTITION_ID_INVALID ((u64)0x0) > > @@ -146,20 +140,6 @@ union hv_timer_config { > }; > }; > > -/* Define the number of message buffers associated with each port. */ > -#define HV_PORT_MESSAGE_BUFFER_COUNT (16) > - > -/* Define the synthetic interrupt controller event flags format. */ > -union hv_synic_event_flags { > - u8 flags8[HV_EVENT_FLAGS_BYTE_COUNT]; > - u32 flags32[HV_EVENT_FLAGS_DWORD_COUNT]; > -}; > - > -/* Define the synthetic interrupt flags page layout. */ > -struct hv_synic_event_flags_page { > - union hv_synic_event_flags > sintevent_flags[HV_SYNIC_SINT_COUNT]; > -}; > - > /* Define SynIC control register. */ > union hv_synic_scontrol { > u64 as_uint64; > @@ -434,8 +414,8 @@ struct hv_context { > > bool synic_initialized; > > - void *synic_message_page[NR_CPUS]; > - void *synic_event_page[NR_CPUS]; > + struct hv_message_page *synic_message_page[NR_CPUS]; > + struct hv_synic_event_flags_page *synic_event_page[NR_CPUS]; > /* > * Hypervisor's notion of virtual processor ID is different from > * Linux' notion of CPU ID. This information can only be retrieved > diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c > index 26b4192..49eaae2 100644 > --- a/drivers/hv/channel_mgmt.c > +++ b/drivers/hv/channel_mgmt.c > @@ -654,7 +654,6 @@ static void init_vp_index(struct vmbus_channel > *channel, u16 dev_type) > static void vmbus_wait_for_unload(void) > { > int cpu; > - void *page_addr; > struct hv_message *msg; > struct vmbus_channel_message_header *hdr; > u32 message_type; > @@ -673,9 +672,8 @@ static void vmbus_wait_for_unload(void) > break; > > for_each_online_cpu(cpu) { > - page_addr = hv_context.synic_message_page[cpu]; > - msg = (struct hv_message *)page_addr + > - VMBUS_MESSAGE_SINT; > + msg = &hv_context.synic_message_page[cpu]-> > + > sint_message[VMBUS_MESSAGE_SINT]; > > message_type = READ_ONCE(msg- > >header.message_type); > if (message_type == HVMSG_NONE) > @@ -699,8 +697,8 @@ static void vmbus_wait_for_unload(void) > * messages after we reconnect. > */ > for_each_online_cpu(cpu) { > - page_addr = hv_context.synic_message_page[cpu]; > - msg = (struct hv_message *)page_addr + > VMBUS_MESSAGE_SINT; > + msg = &hv_context.synic_message_page[cpu]-> > + sint_message[VMBUS_MESSAGE_SINT]; > msg->header.message_type = HVMSG_NONE; > } > } > diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c > index 6ce8b87..aaa2103 100644 > --- a/drivers/hv/connection.c > +++ b/drivers/hv/connection.c > @@ -381,17 +381,12 @@ static void process_chn_event(u32 relid) > */ > void vmbus_on_event(unsigned long data) > { > - u32 dword; > - u32 maxdword; > - int bit; > - u32 relid; > - u32 *recv_int_page = NULL; > - void *page_addr; > + u32 relid, max_relid; > + unsigned long *recv_int_page; > int cpu = smp_processor_id(); > - union hv_synic_event_flags *event; > > if (vmbus_proto_version < VERSION_WIN8) { > - maxdword = MAX_NUM_CHANNELS_SUPPORTED >> 5; > + max_relid = MAX_NUM_CHANNELS_SUPPORTED; > recv_int_page = vmbus_connection.recv_int_page; > } else { > /* > @@ -399,36 +394,22 @@ void vmbus_on_event(unsigned long data) > * can be directly checked to get the id of the channel > * that has the interrupt pending. > */ > - maxdword = HV_EVENT_FLAGS_DWORD_COUNT; > - page_addr = hv_context.synic_event_page[cpu]; > - event = (union hv_synic_event_flags *)page_addr + > - VMBUS_MESSAGE_SINT; > - recv_int_page = event->flags32; > + struct hv_synic_event_flags *event = > + &hv_context.synic_event_page[cpu]-> > + sintevent_flags[VMBUS_MESSAGE_SINT]; > + max_relid = HV_EVENT_FLAGS_COUNT; > + recv_int_page = (unsigned long *)event->flags; > } > > - > - > /* Check events */ > if (!recv_int_page) > return; > - for (dword = 0; dword < maxdword; dword++) { > - if (!recv_int_page[dword]) > - continue; > - for (bit = 0; bit < 32; bit++) { > - if (sync_test_and_clear_bit(bit, > - (unsigned long *)&recv_int_page[dword])) { > - relid = (dword << 5) + bit; > - > - if (relid == 0) > - /* > - * Special case - vmbus > - * channel protocol msg > - */ > - continue; > - > - process_chn_event(relid); > - } > - } > + > + /* relid == 0 is vmbus channel protocol msg */ > + relid = 1; > + for_each_set_bit_from(relid, recv_int_page, max_relid) { > + clear_bit(relid, recv_int_page); We are using this test_and_clear_bit paradigm for locking. The current code used the sync variant to ensure the host saw the changes we were making here (clearing the bit). You may want to add a barrier here or use the sync variant. > + process_chn_event(relid); > } > } > > diff --git a/drivers/hv/vmbus_drv.c b/drivers/hv/vmbus_drv.c > index 230c62e..13dd210 100644 > --- a/drivers/hv/vmbus_drv.c > +++ b/drivers/hv/vmbus_drv.c > @@ -872,9 +872,8 @@ static void hv_process_timer_expiration(struct > hv_message *msg, int cpu) > void vmbus_on_msg_dpc(unsigned long data) > { > int cpu = smp_processor_id(); > - void *page_addr = hv_context.synic_message_page[cpu]; > - struct hv_message *msg = (struct hv_message *)page_addr + > - VMBUS_MESSAGE_SINT; > + struct hv_message *msg = &hv_context.synic_message_page[cpu]- > > > + > sint_message[VMBUS_MESSAGE_SINT]; > struct vmbus_channel_message_header *hdr; > struct vmbus_channel_message_table_entry *entry; > struct onmessage_work_context *ctx; > @@ -911,54 +910,40 @@ void vmbus_on_msg_dpc(unsigned long data) > static void vmbus_isr(void) > { > int cpu = smp_processor_id(); > - void *page_addr; > struct hv_message *msg; > - union hv_synic_event_flags *event; > - bool handled = false; > + struct hv_synic_event_flags *event; > > - page_addr = hv_context.synic_event_page[cpu]; > - if (page_addr == NULL) > + if (!hv_context.synic_event_page[cpu]) > return; > > - event = (union hv_synic_event_flags *)page_addr + > - VMBUS_MESSAGE_SINT; > + event = &hv_context.synic_event_page[cpu]-> > + sintevent_flags[VMBUS_MESSAGE_SINT]; > /* > * Check for events before checking for messages. This is the order > * in which events and messages are checked in Windows guests on > * Hyper-V, and the Windows team suggested we do the same. > */ > > - if ((vmbus_proto_version == VERSION_WS2008) || > - (vmbus_proto_version == VERSION_WIN7)) { > - > + /* On win8 and above the channel interrupts are signaled directly in > + * the event page and will be checked in the .event_dpc > + */ > + if (vmbus_proto_version >= VERSION_WIN8 || > /* Since we are a child, we only need to check bit 0 */ > - if (sync_test_and_clear_bit(0, > - (unsigned long *) &event->flags32[0])) { > - handled = true; > - } > - } else { > - /* > - * Our host is win8 or above. The signaling mechanism > - * has changed and we can directly look at the event page. > - * If bit n is set then we have an interrup on the channel > - * whose id is n. > - */ > - handled = true; > - } > - > - if (handled) > + test_and_clear_bit(0, (unsigned long *)event->flags)) Don't we need the sync variant of test_and_clear_bit here. > tasklet_schedule(hv_context.event_dpc[cpu]); > > - > - page_addr = hv_context.synic_message_page[cpu]; > - msg = (struct hv_message *)page_addr + VMBUS_MESSAGE_SINT; > + msg = &hv_context.synic_message_page[cpu]-> > + sint_message[VMBUS_MESSAGE_SINT]; > > /* 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, cpu); > - else > - tasklet_schedule(hv_context.msg_dpc[cpu]); > + switch (READ_ONCE(msg->header.message_type)) { > + case HVMSG_NONE: > + break; > + case HVMSG_TIMER_EXPIRED: > + hv_process_timer_expiration(msg, cpu); > + break; > + default: > + tasklet_schedule(hv_context.msg_dpc[cpu]); > } > > add_interrupt_randomness(HYPERVISOR_CALLBACK_VECTOR, 0); > -- > 2.9.3