From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 92848EE49AB for ; Tue, 22 Aug 2023 20:18:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229878AbjHVUSq (ORCPT ); Tue, 22 Aug 2023 16:18:46 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47936 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229609AbjHVUSp (ORCPT ); Tue, 22 Aug 2023 16:18:45 -0400 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 9A54610B; Tue, 22 Aug 2023 13:18:43 -0700 (PDT) Received: from [192.168.0.5] (71-212-112-68.tukw.qwest.net [71.212.112.68]) by linux.microsoft.com (Postfix) with ESMTPSA id 9692B2126CD4; Tue, 22 Aug 2023 13:18:42 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 9692B2126CD4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1692735523; bh=btU0rYW/bx7YGoKcH73ygw8rCrE2wH1cwH/SWBjhpw4=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=DFC55oaMZJjU5gyX4Fty/5hcpe8EZiLuuMET7BoKHLcBAxiEFJXna19hq74swcGlC HTBv8OZaA5fbgfgXzMlSIqmKIqtQxAIih87JuxnQWsZCkg4XQAwNVfozq/7qHFGGB8 G7mAw8OA+d5/OAuXF2bxPILeSUJ7bneXfKAmIDb4= Message-ID: <8978223c-c5e8-4235-a0ed-2031583c2751@linux.microsoft.com> Date: Tue, 22 Aug 2023 13:18:51 -0700 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 15/15] Drivers: hv: Add modules to expose /dev/mshv to VMMs running on Hyper-V Content-Language: en-US To: Saurabh Singh Sengar , "linux-hyperv@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "x86@kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-arch@vger.kernel.org" Cc: "patches@lists.linux.dev" , "Michael Kelley (LINUX)" , KY Srinivasan , "wei.liu@kernel.org" , Haiyang Zhang , Dexuan Cui , "apais@linux.microsoft.com" , Tianyu Lan , "ssengar@linux.microsoft.com" , MUKESH RATHOR , "stanislav.kinsburskiy@gmail.com" , "jinankjain@linux.microsoft.com" , vkuznets , "tglx@linutronix.de" , "mingo@redhat.com" , "bp@alien8.de" , "dave.hansen@linux.intel.com" , "hpa@zytor.com" , "will@kernel.org" , "catalin.marinas@arm.com" References: <1692309711-5573-1-git-send-email-nunodasneves@linux.microsoft.com> <1692309711-5573-16-git-send-email-nunodasneves@linux.microsoft.com> <664aec4c-7ea9-447f-afab-9e31e9e106c1@linux.microsoft.com> From: Nuno Das Neves In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-hyperv@vger.kernel.org On 8/19/2023 10:19 PM, Saurabh Singh Sengar wrote: > > >> -----Original Message----- >> From: Nuno Das Neves >> Sent: Saturday, August 19, 2023 12:30 AM >> To: Saurabh Singh Sengar ; linux- >> hyperv@vger.kernel.org; linux-kernel@vger.kernel.org; x86@kernel.org; linux- >> arm-kernel@lists.infradead.org; linux-arch@vger.kernel.org >> Cc: patches@lists.linux.dev; Michael Kelley (LINUX) >> ; KY Srinivasan ; >> wei.liu@kernel.org; Haiyang Zhang ; Dexuan Cui >> ; apais@linux.microsoft.com; Tianyu Lan >> ; ssengar@linux.microsoft.com; MUKESH >> RATHOR ; stanislav.kinsburskiy@gmail.com; >> jinankjain@linux.microsoft.com; vkuznets ; >> tglx@linutronix.de; mingo@redhat.com; bp@alien8.de; >> dave.hansen@linux.intel.com; hpa@zytor.com; will@kernel.org; >> catalin.marinas@arm.com >> Subject: Re: [PATCH v2 15/15] Drivers: hv: Add modules to expose /dev/mshv >> to VMMs running on Hyper-V >> >> On 8/18/2023 6:08 AM, Saurabh Singh Sengar wrote: >>>> + >>>> +config MSHV_VTL >>>> + tristate "Microsoft Hyper-V VTL driver" >>>> + depends on MSHV >>>> + select HYPERV_VTL_MODE >>>> + select TRANSPARENT_HUGEPAGE >>> >>> TRANSPARENT_HUGEPAGE can be avoided for now. >>> >> >> I will remove it in the next version. Thanks. >>>> + >>>> +#define HV_GET_REGISTER_BATCH_SIZE \ >>>> + (HV_HYP_PAGE_SIZE / sizeof(union hv_register_value)) >>>> +#define HV_SET_REGISTER_BATCH_SIZE \ >>>> + ((HV_HYP_PAGE_SIZE - sizeof(struct hv_input_set_vp_registers)) \ >>>> + / sizeof(struct hv_register_assoc)) >>>> + >>>> +int hv_call_get_vp_registers( >>>> + u32 vp_index, >>>> + u64 partition_id, >>>> + u16 count, >>>> + union hv_input_vtl input_vtl, >>>> + struct hv_register_assoc *registers) { >>>> + struct hv_input_get_vp_registers *input_page; >>>> + union hv_register_value *output_page; >>>> + u16 completed = 0; >>>> + unsigned long remaining = count; >>>> + int rep_count, i; >>>> + u64 status; >>>> + unsigned long flags; >>>> + >>>> + local_irq_save(flags); >>>> + >>>> + input_page = *this_cpu_ptr(hyperv_pcpu_input_arg); >>>> + output_page = *this_cpu_ptr(hyperv_pcpu_output_arg); >>>> + >>>> + input_page->partition_id = partition_id; >>>> + input_page->vp_index = vp_index; >>>> + input_page->input_vtl.as_uint8 = input_vtl.as_uint8; >>>> + input_page->rsvd_z8 = 0; >>>> + input_page->rsvd_z16 = 0; >>>> + >>>> + while (remaining) { >>>> + rep_count = min(remaining, HV_GET_REGISTER_BATCH_SIZE); >>>> + for (i = 0; i < rep_count; ++i) >>>> + input_page->names[i] = registers[i].name; >>>> + >>>> + status = hv_do_rep_hypercall(HVCALL_GET_VP_REGISTERS, >>>> rep_count, >>>> + 0, input_page, output_page); >>> >>> Is there any possibility that count value is passed 0 by mistake ? In >>> that case status will remain uninitialized. >>> >> >> These lines ensure rep_count is never 0 here: >> >> while (remaining) { >> rep_count = min(remaining, HV_GET_REGISTER_BATCH_SIZE); >> >> Remaining can't be 0 or the loop would exit, and >> HV_GET_REGISTER_BATCH_SIZE is not 0, or we would never get any registers. > > There is a parameter in this function "count". I was checking if there is any possibility > that is passed as 0 by mistake ? this will lead to "remaining" value as 0 and loop will never > execute. Which results using "status" uninitialized later in the function. > > Ah ok, thanks! I will change it to return immediately in case count is 0. >>>> diff --git a/drivers/hv/mshv.h b/drivers/hv/mshv.h >>>> new file mode 100644 >>>> index 000000000000..166480a73f3f >>>> --- /dev/null >>>> +++ b/drivers/hv/mshv.h >>>> @@ -0,0 +1,156 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0-only */ >>>> +/* >>>> + * Copyright (c) 2023, Microsoft Corporation. >>>> + */ >>>> + >>>> +#ifndef _MSHV_H_ >>>> +#define _MSHV_H_ >>>> + >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +/* >>>> + * Hyper-V hypercalls >>>> + */ >>>> + >>>> +int hv_call_withdraw_memory(u64 count, int node, u64 partition_id); >>>> +int hv_call_create_partition( >>>> + u64 flags, >>>> + struct hv_partition_creation_properties creation_properties, >>>> + union hv_partition_isolation_properties isolation_properties, >>>> + u64 *partition_id); >>>> +int hv_call_initialize_partition(u64 partition_id); >>>> +int hv_call_finalize_partition(u64 partition_id); >>>> +int hv_call_delete_partition(u64 partition_id); >>>> +int hv_call_map_gpa_pages( >>>> + u64 partition_id, >>>> + u64 gpa_target, >>>> + u64 page_count, u32 flags, >>>> + struct page **pages); >>>> +int hv_call_unmap_gpa_pages( >>>> + u64 partition_id, >>>> + u64 gpa_target, >>>> + u64 page_count, u32 flags); >>>> +int hv_call_get_vp_registers( >>>> + u32 vp_index, >>>> + u64 partition_id, >>>> + u16 count, >>>> + union hv_input_vtl input_vtl, >>>> + struct hv_register_assoc *registers); >>>> +int hv_call_get_gpa_access_states( >>>> + u64 partition_id, >>>> + u32 count, >>>> + u64 gpa_base_pfn, >>>> + u64 state_flags, >>>> + int *written_total, >>>> + union hv_gpa_page_access_state *states); >>>> + >>>> +int hv_call_set_vp_registers( >>>> + u32 vp_index, >>>> + u64 partition_id, >>>> + u16 count, >>>> + union hv_input_vtl input_vtl, >>>> + struct hv_register_assoc *registers); >>> >>> Nit: Opportunity to fix many of the checkpatch.pl related to line break here >>> and many other places. >>> >> >> checkpatch.pl doesn't complain about anything in this file. > > If we use --strict switch with checkpatch.pl we observe additional CHECK(s). > I observe 159 CHECK(s) with this patch overall. > (total: 1 errors, 7 warnings, 159 checks, 7460 lines checked) > Few examples: > > CHECK: Lines should not end with a '(' > #240: FILE: drivers/hv/hv_call.c:73: > +int hv_call_set_vp_registers( > > CHECK: Alignment should match open parenthesis > #266: FILE: drivers/hv/hv_call.c:99: > + memcpy(input_page->elements, registers, > + sizeof(struct hv_register_assoc) * rep_count); > > I didn't try --strict before. Thanks, I'll fix up the formatting. > I also see an error with flexible array, possibly we can fix that as well. > > ERROR: Use C99 flexible arrays - see https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays > #7468: FILE: include/uapi/linux/mshv.h:134: > + struct mshv_msi_routing_entry entries[0]; > +}; > Indeed, I'll fix it too.